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: Add controllerManager tlsMinVersion option to values #2289

Merged
merged 6 commits into from
Sep 28, 2022

Conversation

gracedo
Copy link
Contributor

@gracedo gracedo commented Sep 27, 2022

Signed-off-by: Grace Do xgrace@gmail.com

What this PR does / why we need it:
Add option to pass in extra arguments to the controller manager. This allows a user to, for example, pass in the --tls-min-version flag to override the default value.

Add tlsMinVersion values field to allow user to override the default value of the --tls-min-version flag (which is set to 1.3)

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 #2288

Special notes for your reviewer:

Signed-off-by: Grace Do <xgrace@gmail.com>
@@ -91,6 +91,7 @@ spec:
- HELMSUBST_DEPLOYMENT_CONTROLLER_MANAGER_DISABLED_BUILTIN
- HELMSUBST_DEPLOYMENT_CONTROLLER_MANAGER_EXEMPT_NAMESPACES
- HELMSUBST_DEPLOYMENT_CONTROLLER_MANAGER_EXEMPT_NAMESPACE_PREFIXES
- HELMSUBST_DEPLOYMENT_CONTROLLER_MANAGER_EXTRA_ARGS
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the PR @gracedo!
In this case, I think it's more readable and debuggable if we explicitly have a helm values field to set the tls-min-version flag instead of a generic arg field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ritazh Thanks for reviewing! I will add a field for tls-min-version - but I think keeping the extraArgs might still be useful in general and keep things flexible, wdyt? Otherwise, maybe we should explicitly add an option for all the possible flags that can be passed into the controller manager?

Copy link
Member

Choose a reason for hiding this comment

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

so far we have been explicitly creating fields for each flag so that by enabling a field, you are enabling the feature and its related resources can all be conditionally added/removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted the generic args in 80e7d30 and added new tlsMinVersion field in 3d2446e

This reverts commit 7257903.

Signed-off-by: Grace Do <xgrace@gmail.com>
Signed-off-by: Grace Do <xgrace@gmail.com>
@gracedo gracedo changed the title feat: Add controllerManager extraArgs option to values feat: Add controllerManager tlsMinVersion option to values Sep 27, 2022
Copy link
Member

@sozercan sozercan left a comment

Choose a reason for hiding this comment

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

small nit, otherwise LGTM

Co-authored-by: Sertaç Özercan <852750+sozercan@users.noreply.github.com>
Signed-off-by: Grace Do <xgrace@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Sep 27, 2022

Codecov Report

Base: 53.33% // Head: 53.41% // Increases project coverage by +0.08% 🎉

Coverage data is based on head (af3a092) compared to base (ef443f0).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2289      +/-   ##
==========================================
+ Coverage   53.33%   53.41%   +0.08%     
==========================================
  Files         116      116              
  Lines       10161    10161              
==========================================
+ Hits         5419     5428       +9     
+ Misses       4321     4315       -6     
+ Partials      421      418       -3     
Flag Coverage Δ
unittests 53.41% <ø> (+0.08%) ⬆️

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%) ⬇️
pkg/readiness/object_tracker.go 83.98% <0.00%> (+1.06%) ⬆️
...onstrainttemplate/constrainttemplate_controller.go 57.17% <0.00%> (+2.39%) ⬆️

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: Grace Do <xgrace@gmail.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

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.

LGTM

@ritazh ritazh merged commit 3fde9bd into open-policy-agent:master Sep 28, 2022
@gracedo gracedo deleted the gracedo/helmchart_cm_args branch September 28, 2022 17:02
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.

Support overriding the --tls-min-version controller manager flag in the Helm chart
5 participants