nathanjrobertson commented on PR #33837: URL: https://github.com/apache/superset/pull/33837#issuecomment-3162443352
> I'm not clear on why we'd want this change. IMO it adds unnecessary complexity to do something that can be done in local CI/CD pipelines. For instance, you could lint your helm repo to error out if a SQLA connection string doesn't have the required URL params. I agree that this change is not problematic in itself, especially if this is universally useful, but over time these types of customizations tend to add up, and add to maintenance burden unnecessarily. The reason this change is needed is because all managed Kubernetes vendors highly recommend TLS/SSL be switched on, and a number of them enforce it as being required to be on. A snapshot of some large managed kubernetes vendors: * [Managed Azure Kubernetes connecting to their managed PostgreSQL won't work](https://learn.microsoft.com/en-us/azure/postgresql/flexible-server/how-to-connect-tls-ssl) (as TLS/SSL is required and enforced) * TLS/SSL is [required and enforced between DigitalOcean Kubernetes and their managed PostgreSQL](https://docs.digitalocean.com/products/databases/postgresql/how-to/secure/#increase-the-ssl-mode-verification-level) * On Google Kubernetes, it is not enforced but highly recommended - ["We recommend that you enforce all connections to use SSL/TLS."](https://cloud.google.com/sql/docs/postgres/configure-ssl-instance) * On AWS Kubernetes, ["by default, RDS for PostgreSQL uses and expects all clients to connect using SSL/TLS, but you can also require it"](https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/PostgreSQL.Concepts.General.SSL.html). Basically, highly recommended, not enforced by default, with a customer configurable switch to enable requiring it. Basically **all four** highly recommend this, and two require and enforce it. The current chart can't do that without this PR. Helm charts are designed to capture the most common usage patterns and make complicated Kubernetes deployments simpler. Managed databases are common and best practice. You shouldn't use a managed database without TLS/SSL turned on, and a number of vendors have made the sensible decision and required TLS/SSL such that you can't use it without TLS/SSL being turned on. If the functionality to use an external PostgreSQL instance wasn't a feature of the helm chart, then yes, it would add complexity to the chart. But the functionality for connecting to an external (commonly managed) database is already explicitly there, and it should be - it's a very common pattern that makes life simpler (which is what Helm is all about). Without this PR you can't meet best practice on **any** Kubernetes / managed database pair, and in at least two cases it just won't work at all. The current chart provides functionality that can only be used in an insecure way, and prevents you from running in a secure fashion. And this issue can be fixed with the trivial change this PR provides (it's 12 lines of code). -- 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]
