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

[kong] Admission controller - Bring your own certificates (#381) #385

Merged
merged 8 commits into from
Jun 29, 2021

Conversation

ludovic-pourrat
Copy link
Contributor

Add the support of providing your own certificate to the admission controller.

What this PR does / why we need it:

Which issue this PR fixes

Special notes for your reviewer:

Checklist

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

  • PR is based off the current tip of the next branch and targets next, not main
  • New or modified sections of values.yaml are documented in the README.md
  • Title of the PR and commit headers start with chart name (e.g. [kong])

Add the support of providing your own certificate to the admission controller.
@ludovic-pourrat ludovic-pourrat requested a review from a team as a code owner June 25, 2021 07:06
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.

Thanks for this contribution! 🙇

@rainest given your expertise I would like if you could review this as well, but to me this seems functional however I'm wondering if we want to name the values differently?

charts/kong/templates/_helpers.tpl Outdated Show resolved Hide resolved
Co-authored-by: Shane Utt <shane@shaneutt.com>
Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

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

Overriding the default generated variable values in the template works around the issues I'd laid out in #252 (comment) (it's always at the default location, so the controller doesn't need to worry about pulling them out of a ConfigMap and loading them into the environment instead), so 🥳

Please change admissionWebhook.certificate.ca.bundle to admissionWebhook.certificate.caBundle. That shouldn't be a nested object unless we need to include other CA bundle parameters, and I don't think we ever will.

Along with that, I believe you need something like this:

  clientConfig:
    {{- if not .Values.ingressController.admissionWebhook.certificate.provided }}
    caBundle: {{ b64enc $caCert }}
    {{- else }}
    {{- if .Values.ingressController.admissionWebhook.certificate.caBundle }}
    caBundle: {{ b64enc .Values.ingressController.admissionWebhook.certificate.caBundle }}
    {{- end }}
    {{- end }}

Otherwise, it will always try to set a caBundle field (even if the value is nil) when you set a provided certificate, which you don't want if you issued your certificate from the cluster CA. Do you agree?

@ludovic-pourrat
Copy link
Contributor Author

@rainest, yes I agree to your feedback, I will change accordingly. Thanks !

@rainest rainest merged commit c2bb1b2 into Kong:next Jun 29, 2021
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.

None yet

3 participants