rainest
commented
10 months ago Draft still needs the ServiceAccount sorted out, test/linter issues fixed, and manual migration testing.
https://github.com/Kong/charts/pull/923 was, to put it lightly, overly ambitious. The vast majority of the chart, including the Deployment template and most templates it calls, was written with the expectation that it'd be called at the global scope only. Converting these to scoped templates has proven difficult because most pull inputs from multiple sections of values.yaml. Helm's lack of static analysis and unit testing makes this particularly difficult, as the only way to confirm you haven't broken anything is to render and hope you have a values.yaml configuration that triggers any bugs you've introduced.
This simpler approach isn't particularly clean, but it's much less likely to break. Given that I'd like to resolve the kong/ingress split as soon as possible, it makes sense to go with this approach (more or less complete after about 4h) than to try and use the opportunity to revamp everything (still mired in issues after at least 36h). I don't want to use globals still in the new controller Deployment and don't want the Deployment templates to not share future updates, but I think it makes sense to do so at present in the interest of time.
pmalek
czeslavo
What this PR does / why we need it:
A much simpler alternative to https://github.com/Kong/charts/pull/923.
Only creates a new
ingressController.deploymentsection for the new controller Deployment. The existing proxy Deployment configuration is left as-is in values.yaml. The Deployment configurations are not homogenous as a result, and some "global" (root-level) configuration (e.g.podAnnotations,containerSecurityContext) confusingly only apply to the proxy Deployment.The contents of this PR are ready for review, but I want to figure out how we can maintain the 2.x branch. The chart repo hasn't ever released two major release branches before. Merge blocked pending that.
Which issue this PR fixes
A much quicker implementation of the minimum in #921 needed to satisfy the separate Deployment, single chart criterion.
Special notes for your reviewer:
Although not strictly required, I'd like https://github.com/Kong/charts/pull/952 to be in place before actually adding the 3.0.0-rc1 bump here.
Differences between Deployment features
The ingress controller Deployment does not support using an HPA or DaemonSet mode. These feel misleading to offer given the controller's leader/inactive model. Scaling will not distribute load across instances and the controller does not have an obvious use case for running across all kubelets (whereas the proxy does if being used as a bare metal proxy).
The chart generates a separate ServiceAccount for each Deployment, but only assigns permissions to the controller account. I do not know of any reason the proxy Deployment would need API server permissions--on its own I don't think it needs a ServiceAccount at all, but I think it does need one when used in a mesh, even if the proxy container doesn't usually mount the SAT.
Migration from existing
kong/konginstallsSee https://gist.github.com/rainest/9a6961d43858e833decc8976856c757c#file-moved-csv for keys that have moved as part of this change. There are also a number of new keys that aren't listed outside the diff.
Differences from
kong/ingressSee https://gist.github.com/rainest/9a6961d43858e833decc8976856c757c#file-kong-ingress-diff for a diff between default configuration
kong/ingressand kong/kong` (with these changes).The admin Service is NodePort instead of headless ClusterIP. NodePort isn't really necessary (it was just the
kongdefault), but headless Services are a annoying to check for diagnostic purposes. Since discovery works fine when the Service has a ClusterIP, I'd prefer to keep it in place for human usage.KONG_ROLE=traditionalis not explicitly set (from defaults). This is the default in the proxy, we shouldn't need it.KONG_KICis set again. This doesn't work iningressdue to the cross-subchart border.Volumes differ for the controller and Kong. This is expected; there's no reason the proxy needs, for example, the webhook cert.
Checklist
[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]
mainbranch.