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.

Sign up for GitHub

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

Jump to bottom

feat: use safe securityContext as default #267

Merged
merged 5 commits into from
Apr 29, 2022
Merged

feat: use safe securityContext as default #267

merged 5 commits into from
Apr 29, 2022

Conversation

JorTurFer
Copy link
Member

Signed-off-by: Jorge Turrado jorge_turrado@hotmail.es

Provide a description of what has been changed

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • A PR is opened to update KEDA core (repo) (if applicable, ie. when deployment manifests are modified)
  • README is updated with new configuration values (if applicable)

Related to kedacore/keda#2933

@JorTurFer JorTurFer requested a review from a team as a code owner April 26, 2022 20:56
@JorTurFer
use safe values for securityContext as default
36b5c1b 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
partial doc update
5e97b36 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
fix typo in securityContexts
53e886f 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
update docs
63a812c 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
tomkerkhove
tomkerkhove approved these changes Apr 29, 2022
View reviewed changes
Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

LGTM, did we port all changes from core here?

keda/README.md Outdated Show resolved Hide resolved
keda/README.md Outdated Show resolved Hide resolved
apply feedback
c6b36a7 
Signed-off-by: Jorge Turrado <jorge.turrado@docplanner.com>
@JorTurFer
Copy link
Member Author

LGTM, did we port all changes from core here

Yes, I have added here exactly the same configuration added to core in this PR to be consistent

@tomkerkhove tomkerkhove merged commit 58409d7 into kedacore:main Apr 29, 2022
@tomkerkhove tomkerkhove added this to the KEDA Core vNext milestone Apr 29, 2022
@JorTurFer JorTurFer deleted the use-non-root branch April 29, 2022 07:46
lsolovey
lsolovey reviewed May 11, 2022
View reviewed changes
keda/templates/12-keda-deployment.yaml
Comment on lines +43 to +54
{{- if .Values.securityContext.operator }}
{{- toYaml .Values.securityContext.operator | nindent 8 }}
{{- else }}
{{- toYaml .Values.podSecurityContext | nindent 8 }}
{{- toYaml .Values.securityContext | nindent 8 }}
{{- end }}
containers:
- name: {{ .Values.operator.name }}
securityContext:
{{- if .Values.securityContext.operator }}
{{- toYaml .Values.securityContext.operator | nindent 12 }}
{{- if .Values.podSecurityContext.operator }}
{{- toYaml .Values.podSecurityContext.operator | nindent 12 }}
{{- else }}
{{- toYaml .Values.securityContext | nindent 12 }}
{{- toYaml .Values.podSecurityContext | nindent 12 }}
Copy link

Choose a reason for hiding this comment

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

Hi @JorTurFer , I wonder why do you flip the logical meaning of securityContext and podSecurityContext Helm values?

Previously securityContext was mapped to container.securityContext, and podSecurityContext was mapped to pod-level securityContext. So the naming seemed to be correct.

Now it's flipped vice versa. Would it cause any backward compatibility issues for users that were previously passing custom securityContext and podSecurityContext values to KEDA Helm chart?

Copy link
Member Author

Choose a reason for hiding this comment

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

lol, you are right!
I confused them 🤦 till now I have been thinking in podSecurityContext as containerSecurityContext...
are you willing to contribute solving this?

Copy link

Choose a reason for hiding this comment

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

I'd be glad to help but I don't think I can do it quickly. I still have PR for #261 on my to-do list but never have time :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't worry, I will do it tomorrow with the chart release :)
Nice catch!!

Copy link

Choose a reason for hiding this comment

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

Thank you @JorTurFer !

Copy link
Member Author

Choose a reason for hiding this comment

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

#274

@JorTurFer
Copy link
Member Author

hey @lsolovey
we have just released new chart version fixing the problem

@lsolovey
Copy link

hey @lsolovey we have just released new chart version fixing the problem

Great, thank you for quick turnaround, @JorTurFer .
I've just installed it. Looks good!

@tomkerkhove tomkerkhove removed this from the KEDA Core vNext milestone Aug 10, 2022
@joebowbeer joebowbeer mentioned this pull request Aug 11, 2022
Closed
@joebowbeer joebowbeer mentioned this pull request Jul 23, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lsolovey lsolovey lsolovey left review comments

@joebowbeer joebowbeer joebowbeer left review comments

@tomkerkhove tomkerkhove tomkerkhove approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JorTurFer @lsolovey @joebowbeer @tomkerkhove