villebro commented on issue #434: URL: https://github.com/apache/superset-kubernetes-operator/issues/434#issuecomment-6171797343
Thanks for the proposal @JosephSun2003, and great design sketch @younsl. Some additional thoughts: - **Identity and renames:** It should be possible to rename a database/dataset or update connection details through these CRs, keyed on the Superset UUID. The UUID should be immutable (CEL `self == oldSelf`). It could default to a deterministic value derived from the CR's namespace/name, but it needs to be settable so existing objects created in the UI can be adopted. - **When to resync:** With dedicated CRs, `metadata.generation`/`observedGeneration` covers spec changes. We'll still need a `trigger`, like the lifecycle tasks have, for Secret rotations behind an unchanged selector, since the operator can't see those. Today imports piggyback on `superset init` (Helm's init job; for the operator, our [migration guide](https://apache.github.io/superset-kubernetes-operator/user-guide/migration/#importing-datasources) has users add it to the lifecycle init command), which is why changing the import file doesn't re-run anything. - **Reconciliation:** I'd keep this out of the sequential lifecycle pipeline. Lifecycle tasks gate component rollouts and can drain the instance, so a broken dataset definition would block a Superset upgrade. A per-kind reconciler in the same operator that reuses the lifecycle Job machinery (image, metastore env, retry/backoff) seems like a better fit. It would wait until the referenced `Superset` has completed its lifecycle for the current image. - **Secrets:** Agree on `passwordFrom`. Following #367/#369, every connection field (host, port, database, username, password) should also have a `*From` counterpart. One catch if we want to relax `passwordFrom` in dev mode: CEL on a separate CR can't see the parent's `environment`, so that would have to be checked at reconcile time. It may be simpler to always require `passwordFrom` and add a Secret to the dev-dependencies manifest. - **Trust model:** The threat model currently assumes a single public CRD. Write access to a `SupersetDatabase` effectively lets the author send any Secret in the namespace to a host of their choosing, so for now we should document it as equivalent to `Superset` write access. This matters most if one of the goals is delegating dataset management to other teams. - **Deletion:** +1 on `deletionPolicy` defaulting to `Retain`. `Delete` will need a finalizer and a defined behavior for when the parent `Superset` is already gone, so CRs don't hang in `Terminating` on namespace deletion. - **Samples:** We could add a database pointing at the dev-dependencies Postgres, with a dataset on the `logs` table. It should be clearly marked dev-only, since that database is the metastore (users, password hashes, etc.). - **Upstream:** Any bugs or missing functionality may need to be addressed in apache/superset. For example, as far as I know there's no CLI for deleting a database/dataset by UUID, which `deletionPolicy: Delete` would need. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
