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

fix: Add parameters for tuning revisionHistory and securityContext #2670

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

bodgit
Copy link
Contributor

@bodgit bodgit commented Nov 16, 2023

What type of PR is this?

feature

Which issue does this PR fix:

n/a

What does this PR do / Why do we need it:

This PR adds support for overriding the revisionHistoryLimit setting on the Deployment as well as the setting the securityContext at both the Pod and container level.

If an issue # is not available please add repro steps and logs from IPAMD/CNI showing the issue:

Testing done on this change:

Will this PR introduce any new dependencies?:

Will this break upgrades or downgrades? Has updating a running cluster been tested?:

Defaults values are set to the implicit Kubernetes defaults.

Does this change require updates to the CNI daemonset config files to work?:

Does this PR introduce any user-facing change?:


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@bodgit bodgit requested a review from a team as a code owner November 16, 2023 14:55
@jdn5126
Copy link
Contributor

jdn5126 commented Nov 16, 2023

@bodgit two questions here:

  1. What is the value of the revisionHistoryLimit?
  2. Why would you want a different securityContext for the pod spec and container spec where there is only one container in this pod?

@bodgit
Copy link
Contributor Author

bodgit commented Nov 16, 2023

  1. What is the value of the revisionHistoryLimit?

10 is the implicit default in current Kubernetes clusters if it's not set, so that's used as the default. My intent with the change is to be able to set this to 0 to not keep any prior revisions. Unfortunately Helm considers 0 a false value so trying to wrap the line with {{ if .Values.revisionHistoryLimit }}...{{ end }} results in no line being added for that use case.

  1. Why would you want a different securityContext for the pod spec and container spec where there is only one container in this pod?

Force of habit on my part, however there's a common subset in both places but some settings can only be set in either place, (https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.28/#securitycontext-v1-core vs https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.28/#podsecuritycontext-v1-core)

@jdn5126
Copy link
Contributor

jdn5126 commented Nov 16, 2023

  1. What is the value of the revisionHistoryLimit?

10 is the implicit default in current Kubernetes clusters if it's not set, so that's used as the default. My intent with the change is to be able to set this to 0 to not keep any prior revisions. Unfortunately Helm considers 0 a false value so trying to wrap the line with {{ if .Values.revisionHistoryLimit }}...{{ end }} results in no line being added for that use case.

  1. Why would you want a different securityContext for the pod spec and container spec where there is only one container in this pod?

Force of habit on my part, however there's a common subset in both places but some settings can only be set in either place, (https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.28/#securitycontext-v1-core vs https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.28/#podsecuritycontext-v1-core)

Got it, thank you for explaining, and thank you for taking this up! I am fine with both of these changes. For the YAML comment, you check if the value is greater than or equal to 0, but it's not a big deal

@bodgit bodgit force-pushed the chart-enhancements branch 2 times, most recently from 3b73575 to 511c51e Compare November 17, 2023 09:58
Signed-off-by: Matt Dainty <matt@bodgit-n-scarper.com>
@bodgit
Copy link
Contributor Author

bodgit commented Nov 17, 2023

Parameter names tweaked and rebased.

@jdn5126
Copy link
Contributor

jdn5126 commented Nov 17, 2023

Thanks @bodgit! Since you are using CNI Metrics Helper, you may also be interested in #2603, which takes the aggregated metrics published to CloudWatch and makes some of them available via Prometheus

@bodgit
Copy link
Contributor Author

bodgit commented Nov 17, 2023

Thanks @bodgit! Since you are using CNI Metrics Helper, you may also be interested in #2603, which takes the aggregated metrics published to CloudWatch and makes some of them available via Prometheus

Yes, that does look useful. Thanks.

@jdn5126 jdn5126 merged commit 35f6f53 into aws:master Nov 17, 2023
4 checks passed
@bodgit bodgit deleted the chart-enhancements branch November 17, 2023 16:41
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

2 participants