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: Expose options to allow injection of external certificates #2249

Merged

Conversation

ethanrange
Copy link
Contributor

What this PR does / why we need it:

This PR is very similar to #1359 by @romachalm which was unfortunately never merged as the author stopped responding. I had followed the same path, implementing webhook annotations in #2231 before stumbling across this PR. I have recreated this with a slightly different interface.

This PR allows for external certificates to be injected and used in place of the default, self signed certificate. This is useful when using services such as cert-manager to automatically sign and rotate certificates. It provides Helm chart config values which when set:

  • Disable the default certificate rotation
  • Overwrite the certificate secret name used by the Controller Manager and Audit Deployments
  • Disables generation of the default server certificate

This, in combination with the annotations in #2231 allows for configuring external certificate injection

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #520

Special notes for your reviewer:
N/A

* Allow overriding the certificate secret name
* Only generate cert-secret if external injection is disabled
* Disable default certificate rotation when injecting

Signed-off-by: Ethan Range <65268454+ethanrange@users.noreply.github.com>
Signed-off-by: Ethan Range <65268454+ethanrange@users.noreply.github.com>
@codecov-commenter
Copy link

Codecov Report

Merging #2249 (5c33a9c) into master (510da53) will decrease coverage by 0.17%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2249      +/-   ##
==========================================
- Coverage   54.57%   54.39%   -0.18%     
==========================================
  Files         111      111              
  Lines        9554     9554              
==========================================
- Hits         5214     5197      -17     
- Misses       3946     3958      +12     
- Partials      394      399       +5     
Flag Coverage Δ
unittests 54.39% <ø> (-0.18%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/readiness/list.go 79.41% <0.00%> (-11.77%) ⬇️
...onstrainttemplate/constrainttemplate_controller.go 56.83% <0.00%> (-3.12%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ethanrange ethanrange mentioned this pull request Aug 26, 2022
@ethanmwam
Copy link

Hi, I'm away for a while from next week onwards - if someone has a chance to review this before the weekend that'd be great - I can then make any changes requested before I leave. No worries if not, I can handle it upon my return.

@maxsmythe
Copy link
Contributor

I think @sozercan and @ritazh are out this week, so not sure if we can get review quorum :/

Copy link
Member

@ritazh ritazh left a comment

Choose a reason for hiding this comment

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

one nit, but otherwise lgtm

cmd/build/helmify/static/README.md Outdated Show resolved Hide resolved
ethanrange and others added 2 commits September 1, 2022 12:52
Co-authored-by: Rita Zhang <rita.z.zhang@gmail.com>
Signed-off-by: Ethan Range <65268454+ethanrange@users.noreply.github.com>
Signed-off-by: Ethan Range <65268454+ethanrange@users.noreply.github.com>
Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

LGTM

@ritazh ritazh merged commit 6f66057 into open-policy-agent:master Sep 2, 2022
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.

cert-manager support
5 participants