-
Notifications
You must be signed in to change notification settings - Fork 152
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 PodSecurity and ContainerSecurity params to PodOptions structure #1674
Conversation
Thanks for submitting this pull request 🎉. The team will review it soon and get back to you. If you haven't already, please take a moment to review our project contributing guideline and Code of Conduct document. |
4e9cc55
to
83e2f9e
Compare
if opts.PodSecurityContext != nil { | ||
pod.Spec.SecurityContext = opts.PodSecurityContext | ||
} | ||
|
||
if opts.ContainerSecurityContext != nil { | ||
pod.Spec.Containers[0].SecurityContext = opts.ContainerSecurityContext | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @TimonOmsk
Can we not have pod and container security context as part of podOverride
itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @TimonOmsk Can we not have pod and container security context as part of
podOverride
itself?
I would prefer to avoid podOverride. Actually I think that the whole concept of podOverride is not a very good idea because there is no control over what exactly we got using override and how it was merged. Some overrides may clash which can lead to unpredictable behaviour. Also, I believe that security is one of basic parameter for Pod, so I think it would be great to set it explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree about the complexity that we get with using podOverride
. But I think it's going to be a bit confusing on how do we want to set the security context. If we really want to get this PR in, I don't have a problem but we should heavily document.
Also, does this impact the podOverride
field that we specify for some kanister functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@viveksinghggits can you please point me to those "some kanister functions" which can be affected?
About docs. Just say what you think I should document and I'll do that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.kanister.io/functions.html#kubetask is one example of kanister function where we can pass the pod spec to override the spec of the pod that kanister creates.
My concern is, should something be changed/documented here?
if, just comment in the source code would be sufficient, specifying that because of the complexity of podOverride
we are accepting the pod and container security context separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can see we don't have any mentions about security specific settings for podOverride field in the docs. So for me it means that we can specify somewhere in docs the default behaviour of pods(running as root) and some recommendations how user can set other security related settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If behaviour of those functions is not changing in anyway in that case I recommend we should just document this in source file with comments. Instead of mentioning it in public docs.
f62dd6f
to
fc3736f
Compare
Co-authored-by: Vivek Singh <vsingh.ggits.2010@gmail.com>
@viveksinghggits @pavannd1 Guys, can we merge it? |
I was only hesitant about introducing new field and not using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TimonOmsk LGTM. I think we can add unit tests in pod_test.go
. Could you please follow up with tests for these fields in a separate PR?
Change Overview
Adding two new fields to the PodOptions structure will allow to configure security specification for a new pod in a more secure way and avoid using PodOverride parameter.
Also, the PR includes small fix for comments' formatting
Pull request type
Please check the type of change your PR introduces:
Test Plan
Not sure if we need any tests here. Please let me know if you think we need to add some