-
Notifications
You must be signed in to change notification settings - Fork 229
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 fluenbit container #796
Add security context for fluenbit container #796
Conversation
Signed-off-by: karan k <karan.k@oracle.com>
b52f83c
to
fed8efd
Compare
@@ -91,6 +91,8 @@ fluentbit: | |||
secrets: [] | |||
# Pod security context for Fluent Bit pods. Ref: https://kubernetes.io/docs/tasks/configure-pod-container/security-context/ | |||
podSecurityContext: {} | |||
# Security context for Fluent Bit container. Ref: https://kubernetes.io/docs/tasks/configure-pod-container/security-context/ | |||
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.
Should the value say containerSecurityContext
(i.e. the helm value is the same as the CRD option)
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.
podSecurityContext -> for pod level security context
securityContext -> for container level security context.
just to make it consistent with what we have for fluent-operator pod.
containerSecurityContext
is something I need to use internally, as securityContext was getting used for pod Security context internally.
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.
Okay if this is what we have for fluent-operator container then LGTM
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.
@adiforluls @karan56625 I think we're open to accepting any reasonable refactoring if you think it's necessary
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.
In that case, we should consider securityContext
for container level security context and podSecurityContext
for pod level security context in Flunetbit CRD. (In helm chart, those fields seem fine, we can consider refactoring Fluentbit CRD)
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.
Ideally there shouldn't be a mismatch between CR fields and helm values from a UX perspective, but I think we'll need to rev the apiVersion
of the CRD to maintain backwards compatibility, so to be pragmatic this is fine for now, when we do move to the next apiVersion
we can refactor naming of certain fields (for example, ClusterFluentBitConfig
is referenced as fluentBitConfigName
in FluentBit
CR).
…ext_for_fluentbit Add security context for fluenbit container
…ext_for_fluentbit (#11) Add security context for fluenbit container Co-authored-by: Aditya Bharadwaj <49105292+adiforluls@users.noreply.github.com>
What this PR does / why we need it:
Adding container level security context for fluentbit container.
Which issue(s) this PR fixes:
Fixes #793
Does this PR introduced a user-facing change?
Additional documentation, usage docs, etc.: