feat(helm): helm chart improvements

[michaelgoodrx]

Summary

When using the Kuma helm chart at GoodRx, we had to make some tweaks to improve availability and integrate with our deployment and monitoring systems. These changes should all be backwards compatible and useful for the community at large. I realize we didn't discuss any of this with the maintainers beforehand. If you choose not to accept, we will maintain these changes in our own fork.

Full changelog

• allow setting topology spread constraints
• allow defining pod disruption budgets
• allow defining autoscaling on ingress and egress
• make service creation optional (because we make them ourselves using terraform)
• allow adding optional extra labels (to assist our monitoring system)
• allow creating kds tls secrets in helm, and any other k8s secrets (so we don't have to add an extra step to our pipeline)
• allow specifying the resource requirements for ingress
• allow setting pod lifecycle and termination grace period (necessary to avoid outages when using AWS)
• standardize labels for all cp, egress, and ingress resources; some of them will get additional labels as a result. The selector labels are unaffected.

Issues resolved

Documentation

Testing

• Unit tests
• E2E tests
• Manual testing on Universal
• Manual testing on Kubernetes

I've tested these changes manually but I doubt I have tested every possible configuration.

Backwards compatibility

It should be backwards compatible, but others should verify, because this is very difficult to test. :)

[lahabana]

Thanks for the contrib!
I'm not opposed to any of these changes.
They make sense and provide more flexibility to the chart. I have a few comments which are mostly aesthetic.

[michaelgoodrx]

Thank you @lahabana for the fast review! I will make updates or respond to feedback ASAP. Might take a few days.

[michaelgoodrx]

Added sign-off to all commits

[michaelgoodrx]

Rendered updated documentation

[codecov-commenter]

Codecov Report

Merging #4220 (7e992dc) into master (6225a80) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4220      +/-   ##
==========================================
- Coverage   55.65%   55.62%   -0.04%     
==========================================
  Files         933      933              
  Lines       56251    56251              
==========================================
- Hits        31305    31288      -17     
- Misses      22460    22479      +19     
+ Partials     2486     2484       -2     

Impacted Files	Coverage Δ
pkg/insights/components.go	58.33% <0.00%> (-25.01%)	⬇️
pkg/events/eventbus.go	85.18% <0.00%> (-7.41%)	⬇️
pkg/plugins/runtime/gateway/route/sorter.go	66.66% <0.00%> (-5.13%)	⬇️
pkg/defaults/components.go	82.14% <0.00%> (-3.58%)	⬇️
pkg/insights/resyncer.go	71.60% <0.00%> (-2.47%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6225a80...7e992dc. Read the comment docs.

[michaelgoodrx]

Tested again in our dev environment. Everything appears functional.

[lahabana]

/golden_files /format

[jakubdyszkiewicz]

Those are great changes, thank you for taking the time to improve the chart!

[jakubdyszkiewicz]

It's not a standard K8S TLS Secret (type: kubernetes.io/tls with cert+key). It's a secret that contains ca.crt with CA cert to validate.

I'd like to keep a comment with an explanation of what is this cert. How about something like this

Name of the K8s Secret resource that contains ca.crt which was used to sign the certificate of KDS Global Server. If you set this and don't set create=true, you have to create the secret manually.

[jakubdyszkiewicz]

Can we be a bit more explicit and name it controlPlane.tls.kdsZoneClient.caCert ? With a comment

CA bundle that was used to sign the certificate of KDS Global Server

[jakubdyszkiewicz]

optional: not caused by this PR, but it's a bit awkward that we have this documented as a potential option to modify, but really all available options are listed below. I'd delete # -- URL of Global Kuma CP comment in values.yaml

[jakubdyszkiewicz]

So those secrets are not mounted to the CP?
Do they need to have CP labels (I'm thinking about moving it to root)?

Can you describe a practical example of this?

I'm wondering if there is a better pattern to solve this with HELM. Have you considered creating a simple chart that has a dependency on Kuma and includes your additional resources?

[lahabana]

@michaelgoodrx the release freeze is in 1 week so please update this soon to make sure we get it in :)

[bartsmykla]

Hi @michaelgoodrx, as today is a deadline for code freeze before the release, and we want this change in it, I decided to step in and address all @jakubdyszkiewicz's review remarks and fix failing tests. I removed the change related to additional secrets, as we were not sure if it's the best solution for this problem, but we are as always open for discussion if it's a crucial for you (give us more context and maybe we'll have some suggestions, or maybe we'll just decide it's the best solution so it'll be possible to add it in the future release). I hope you don't mind.

[michaelgoodrx]

Hey @bartsmykla thanks, I really appreciate that you were able to merge it in my absence. I was on vacation and then caught COVID, it took a long time to recover.

RE the additional secrets, we are using them to create a secret for the kong mesh license key. I think some people might want it if they want to use a custom certificate for mutual TLS, or any other features you add in the future.

I think, once you do the release, we might be able to manage the kong mesh license key using the kong mesh chart as a subchart instead, if you don't want it in the kuma helm chart.