-
Notifications
You must be signed in to change notification settings - Fork 450
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
Allow customizing security context #332
Conversation
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.
LGTM, and is ready to be merged as soon as a unit test is added.
pkg/collector/container.go
Outdated
Args: args, | ||
Env: envVars, | ||
Resources: otelcol.Spec.Resources, | ||
SecurityContext: &otelcol.Spec.SecurityContext, |
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.
Could you add a unit test, ensuring that a security context set at the CR will propagate down to the container?
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.
Sure. Im out for the weekend. Will do on Monday.
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.
Followed existing test for volumes and added a new one for security context. LMK if you think that's enough.
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.
They look good!
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.
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.
Totally forgot about the doc... Another reason to automate it as soon as possible :-)
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.
Updated
// SecurityContext will be set as the container security context | ||
// +optional | ||
// +operator-sdk:gen-csv:customresourcedefinitions.specDescriptors=true | ||
SecurityContext v1.SecurityContext `json:"securityContext,omitempty"` |
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.
Sorry for catching this only now, but this should be a pointer. Otherwise, we won't know when it's been specified explicitly by the user. We need this signal if we ever decide to change the default value we pass down to the containers.
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.
Updated
pkg/collector/container.go
Outdated
Args: args, | ||
Env: envVars, | ||
Resources: otelcol.Spec.Resources, | ||
SecurityContext: &otelcol.Spec.SecurityContext, |
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.
They look good!
LGTM! |
Allow setting a custom security context on containers.
Allow setting a custom security context on containers.
Allow setting a custom security context on containers.
This allows users to customize the container security context settings. It's helpful when some collector features need elevated privileges.