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(controller) support user-defined mounts and bump version #560

Merged
merged 4 commits into from
Mar 17, 2022

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Mar 16, 2022

What this PR does / why we need it:

Allow user-defined controller mounts, largely to enable usage of the functionality added in Kong/kubernetes-ingress-controller#2258

Also bumps Kong version to 2.8 because it's a chore and I didn't want to bother with a separate PR.

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • PR is based off the current tip of the main branch.
  • Changes are documented under the "Unreleased" header in CHANGELOG.md
  • New or modified sections of values.yaml are documented in the README.md
  • Commits follow the Kong commit message guidelines

@rainest rainest requested a review from a team as a code owner March 16, 2022 18:50
@rainest rainest marked this pull request as draft March 16, 2022 18:50
@rainest rainest changed the title feat(controller) support user-defined mounts feat(controller) support user-defined mounts and bump version Mar 16, 2022
@rainest rainest force-pushed the feat/controller-mounts branch 5 times, most recently from 1a05cb2 to b4daf0b Compare March 16, 2022 23:34
Add support for user-defined volume mounts in the ingress controller
container.

Update the volume/volume mount helpers to take a context argument rather
than using absolute paths.

Add tests for mixed controller and Kong volume usage.
@rainest rainest marked this pull request as ready for review March 17, 2022 00:04
Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, just some small comments.

Also I'm sure you've tested this manually but it occurs to me that we should be able to add an integration test for this fairly simply, that seems like something we should do: the most obvious test would seem to be to add certs to a mount in the test and verify they are mounted properly and in use.

charts/kong/README.md Outdated Show resolved Hide resolved
@rainest rainest enabled auto-merge (squash) March 17, 2022 18:36
@rainest
Copy link
Contributor Author

rainest commented Mar 17, 2022

Stuffing emptyDir mounts into the test values is sufficient for what I want to test here: we trust that Kubernetes does indeed mount the volume given well-formed volume and mount configuration, so for our part we just want to ensure that the template inserts the user blocks into the appropriate locations. If we don't, Kubernetes rejects the Deployment as invalid (the tests don't show this because it's a failure state, but it very much did happen earlier in development when I screwed up indentation and variable references).

@rainest rainest requested a review from shaneutt March 17, 2022 18:44
Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to talk more soon about testing, but that doesn't need to hold this up.

@rainest rainest merged commit e4c7f91 into main Mar 17, 2022
@rainest rainest deleted the feat/controller-mounts branch March 17, 2022 21:45
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.

Add support for controller volume mounts
2 participants