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

Add security context for statefulset and post-upgrade/install jobs #1085

Merged
merged 11 commits into from
Mar 18, 2024

Conversation

tzahigro1
Copy link
Contributor

This PR adds the option to configure "runAsNonRoot" and "allowPrivilegeEscalation" fields under "securityContext" for the statefulset , post-install-upgrade-job and post-upgrade job inside the values file under statefulset.securityContext.allowPrivilegeEscalation and statefulset.securityContext.runAsNonRoot

@CLAassistant
Copy link

CLAassistant commented Mar 13, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Tzahi Grodzevsky seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Contributor

@RafalKorepta RafalKorepta left a comment

Choose a reason for hiding this comment

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

Hi @tzahigro1,

I don't have problem with that implementation, but there are dedicated values for post jobs

post_install_job:
enabled: true
# Resource requests and limits for the post-install batch job
# resources:
# requests:
# cpu: 1
# memory: 512Mi
# limits:
# cpu: 2
# memory: 1024Mi
# labels: {}
# annotations: {}
affinity: {}
post_upgrade_job:
enabled: true
# Resource requests and limits for the post-upgrade batch job
# resources:
# requests:
# cpu: 1
# memory: 512Mi
# limits:
# cpu: 2
# memory: 1024Mi
# labels: {}
# annotations: {}
# Additional environment variables for the Post Upgrade Job
# extraEnv:
# - name: AWS_SECRET_ACCESS_KEY
# valueFrom:
# secretKeyRef:
# name: my-secret
# key: redpanda-aws-secret-access-key
# Additional environment variables for the Post Upgrade Job mapped from Secret or ConfigMap
# extraEnvFrom:
# - secretRef:
# name: redpanda-aws-secrets
affinity: {}
# When helm upgrade is performed the post-upgrade job is scheduled before Statefulset successfully finish
# its rollout. User can extend Job default backoff limit of `6`.
# backoffLimit:

which could have that securityContext part.

It would be good, as a follow up, to give users ability to change the securityContext per container.

Could you bump the following semver? The patch version should suffice.

version: 5.7.33

Could you signed CLA?

@RafalKorepta
Copy link
Contributor

I see that CLA is signed. I will bump Chart.yaml quickly.

@RafalKorepta RafalKorepta enabled auto-merge (rebase) March 13, 2024 14:28
@tzahigro1
Copy link
Contributor Author

Hi @tzahigro1,

I don't have problem with that implementation, but there are dedicated values for post jobs

post_install_job:
enabled: true
# Resource requests and limits for the post-install batch job
# resources:
# requests:
# cpu: 1
# memory: 512Mi
# limits:
# cpu: 2
# memory: 1024Mi
# labels: {}
# annotations: {}
affinity: {}
post_upgrade_job:
enabled: true
# Resource requests and limits for the post-upgrade batch job
# resources:
# requests:
# cpu: 1
# memory: 512Mi
# limits:
# cpu: 2
# memory: 1024Mi
# labels: {}
# annotations: {}
# Additional environment variables for the Post Upgrade Job
# extraEnv:
# - name: AWS_SECRET_ACCESS_KEY
# valueFrom:
# secretKeyRef:
# name: my-secret
# key: redpanda-aws-secret-access-key
# Additional environment variables for the Post Upgrade Job mapped from Secret or ConfigMap
# extraEnvFrom:
# - secretRef:
# name: redpanda-aws-secrets
affinity: {}
# When helm upgrade is performed the post-upgrade job is scheduled before Statefulset successfully finish
# its rollout. User can extend Job default backoff limit of `6`.
# backoffLimit:

which could have that securityContext part.

It would be good, as a follow up, to give users ability to change the securityContext per container.

Could you bump the following semver? The patch version should suffice.

version: 5.7.33

Could you signed CLA?

Yes I saw that but upon looking at the previous implementation the "runAsUser" and "runAsGroup" values come from there so I figured its the place to add it.
I agree with you that a better more preferred way is to add them to the values under each "job" , although it can conflict with the current implementation because I see that there are 3 resources using the same "container-security-context" values from helpers.tpl

@tzahigro1
Copy link
Contributor Author

for example in the existing _helpers.tpl
https://github.com/tzahigro1/redpanda-helm-charts/blob/b71a4578678e0587a27abaa30f55b152b9ae2d67/charts/redpanda/templates/_helpers.tpl#L477
You can see that "container-security-context" already takes the value from ".Values.statefulset.securityContext.runAsUser" and ".Values.statefulset.securityContext.runAsGroup"
and it is used inside the post-upgrade job aswell as in the post-install-upgrade-job and statefulset.
I think this value from "_helpers.tpl" either need to be merged with the values inside "values.yaml" file or some other implementation.
@RafalKorepta Please let me know what you think

@RafalKorepta
Copy link
Contributor

@tzahigro1 I agree with you that this needs to be refactored. I will create follow up to address that.

auto-merge was automatically disabled March 13, 2024 15:36

Head branch was pushed to by a user without write access

Copy link
Contributor

@RafalKorepta RafalKorepta left a comment

Choose a reason for hiding this comment

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

Great to see that improvement!

You can always extend the values.yaml with commented example and defines new fields in values.schema.json

@RafalKorepta RafalKorepta enabled auto-merge (rebase) March 13, 2024 16:42
auto-merge was automatically disabled March 14, 2024 15:03

Head branch was pushed to by a user without write access

@tzahigro1 tzahigro1 force-pushed the tz/security-context branch 2 times, most recently from d62dd72 to 4a15fee Compare March 14, 2024 15:05
@tzahigro1
Copy link
Contributor Author

The CLA is bugged because I made commits from user without proper email address format , I re-wrote the commit history with my correct email.
@RafalKorepta

@EliranTurgeman
Copy link

@alejandroEsc , @chrisseto please review :)

@chrisseto chrisseto enabled auto-merge (squash) March 18, 2024 20:32
@chrisseto chrisseto merged commit 9162b60 into redpanda-data:main Mar 18, 2024
26 checks passed
@tzahigro1
Copy link
Contributor Author

I see that the PR is merged but don't see any new chart released?
Can you please make sure the new chart is released and let us know.
@RafalKorepta @chrisseto

@RafalKorepta
Copy link
Contributor

@tzahigro1 The chart.yaml was not changed. Probably by rebasing the semver bump was missed.

If CI passes for #1087, then your changes would be included in new chart release.

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.

5 participants