Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(helm): helm chart improvements #4337

Merged
merged 22 commits into from
May 24, 2022
Merged

Conversation

bartsmykla
Copy link
Contributor

Summary

@bartsmykla: As we have a code freeze today I decided to take over the PR and address some of the comments from @jakubdyszkiewicz to make this PR in. The only change we removed, as we were not sure if it was the best solution (additional secrets creations) in current form, but we are definitely open for the discussion @michaelgoodrx

Original PR:
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

Supersedes #4220

Documentation

n/a

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. :)

michaelgoodrx and others added 21 commits May 3, 2022 16:28
This includes a change to how affinty is defined so it is more flexible
and can be removed entirely if desired.

Signed-off-by: Michael Younkin <[email protected]>
This was tricky, because we had to get rid of the enabled flag in order
to maintain backwards compatibility. Because in the past, we only turned
on the kds cert checking code if a name was set, but now we want to also
turn it on if no name is set but create is true.

Signed-off-by: Michael Younkin <[email protected]>
Signed-off-by: Michael Younkin <[email protected]>
Signed-off-by: Bart Smykla <[email protected]>
Signed-off-by: Bart Smykla <[email protected]>
Signed-off-by: Bart Smykla <[email protected]>
@bartsmykla bartsmykla requested a review from a team as a code owner May 24, 2022 10:35
@bartsmykla bartsmykla changed the title Goodrx helm chart fixes feat(helm): helm chart improvements May 24, 2022
@bartsmykla bartsmykla changed the title feat(helm): helm chart improvements feat(helm): helm chart improvements May 24, 2022
@bartsmykla bartsmykla enabled auto-merge (squash) May 24, 2022 11:22
@codecov-commenter
Copy link

Codecov Report

Merging #4337 (27555d8) into master (56280f6) will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4337      +/-   ##
==========================================
+ Coverage   55.38%   55.41%   +0.02%     
==========================================
  Files         941      941              
  Lines       57125    57125              
==========================================
+ Hits        31641    31657      +16     
+ Misses      22974    22968       -6     
+ Partials     2510     2500      -10     
Impacted Files Coverage Δ
pkg/plugins/common/postgres/listener.go 53.84% <0.00%> (-15.39%) ⬇️
pkg/plugins/resources/postgres/events/listener.go 16.66% <0.00%> (-14.59%) ⬇️
pkg/mads/server/server.go 80.90% <0.00%> (-1.82%) ⬇️
pkg/xds/generator/direct_access_proxy_generator.go 89.65% <0.00%> (-1.15%) ⬇️
pkg/plugins/resources/postgres/store.go 76.79% <0.00%> (+0.42%) ⬆️
api/observability/v1/mads.pb.go 35.56% <0.00%> (+1.03%) ⬆️
pkg/api-server/server.go 87.44% <0.00%> (+2.09%) ⬆️
pkg/mads/v1/client/client.go 43.75% <0.00%> (+2.50%) ⬆️
...s/authn/api-server/tokens/admin_token_bootstrap.go 86.00% <0.00%> (+4.00%) ⬆️
pkg/core/tokens/signing_key_manager.go 90.00% <0.00%> (+5.00%) ⬆️
... and 4 more

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 56280f6...27555d8. Read the comment docs.

@bartsmykla bartsmykla merged commit c2b2297 into master May 24, 2022
@bartsmykla bartsmykla deleted the goodrx-helm-chart-fixes branch May 24, 2022 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants