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

Extend options for setting security context #1647

Merged
merged 1 commit into from
Feb 5, 2025

Conversation

dmitryax
Copy link
Contributor

@dmitryax dmitryax commented Feb 5, 2025

Add option to set security contexts on:

  • container in the clusterRecever deployment
  • container in the gateway deployment
  • agent pod

@dmitryax dmitryax requested review from a team as code owners February 5, 2025 01:43
@dmitryax dmitryax force-pushed the add-container-security-context branch from de73aef to c7a1e20 Compare February 5, 2025 01:43
@@ -577,9 +577,36 @@
}
}
},
"podSecurityContext": {
"type": "object"
},
"securityContext": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can also be deprecated in favor of containerSecurityContext but I think we can defer that until fluentd is removed

@dmitryax dmitryax force-pushed the add-container-security-context branch 2 times, most recently from 5e3db36 to 74e35dd Compare February 5, 2025 01:50
@dmitryax dmitryax force-pushed the add-container-security-context branch from 74e35dd to dfadbbe Compare February 5, 2025 07:09
@dmitryax dmitryax changed the title Allow setting security context on containers Extend options for setting security context Feb 5, 2025
{{ end }}
{{- if not (eq (toString .Values.clusterReceiver.securityContext) "<nil>") }}
[WARNING] "clusterReceiver.securityContext" parameter is deprecated. Please use "clusterReceiver.podSecurityContext" instead.
{{ end }}
Copy link
Contributor

@jvoravong jvoravong Feb 5, 2025

Choose a reason for hiding this comment

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

Nit: Can we throw a failure or print a warning if one of the following cases happens? I just want to avoid confusion about which of the two available values (securityContext vs podSecurityContext) will be used. Maybe just update the message to say podSecurityContext takes priority.

  • Both clusterReceiver.securityContext and clusterReceiver.podSecurityContext are both used
  • Both gateway.securityContext and gateway.podSecurityContext are both used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand your suggestion. Please submit a suggesting diff

Copy link
Contributor

Choose a reason for hiding this comment

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

Working on this. Need to state this PR relates to #1342

Copy link
Collaborator

@jinja2 jinja2 left a comment

Choose a reason for hiding this comment

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

We are missing updates in few places in this block. I think it should be updated to podSecurityContext instead. Actually, we might have to do some checking to see which of the 2 (pod or container) securityContext has runAsUser set. Both can have this field.

@jinja2
Copy link
Collaborator

jinja2 commented Feb 5, 2025

Another thing - do we know if the original ask also wants a way to set the context on initContainer? I know these only come into play when logs are enabled, but best to check if we need to include those.

@jvoravong
Copy link
Contributor

jvoravong commented Feb 5, 2025

Another thing - do we know if the original ask also wants a way to set the context on initContainer? I know these only come into play when logs are enabled, but best to check if we need to include those.

From my current understanding initContainers are used by the agent daemonset to update permissions for enabling collecting logs and all 3 agent modes to enable using persistent data exporter queues normally only used by Splunk Enterprise/Platform customers.

Maybe we enable setting security context for initContainers and containers in a future PR for iterative growth.

@dmitryax
Copy link
Contributor Author

dmitryax commented Feb 5, 2025

We are missing updates in few places in this block. I think it should be updated to podSecurityContext instead. Actually, we might have to do some checking to see which of the 2 (pod or container) securityContext has runAsUser set. Both can have this field.

I've actually started with adding this to the init containers as well, but I'm afraid of unnecessarily bloating up the config interface, see the first version in 74e35dd. I think we can start without it at this point, see if it's needed for users, then add it. WDYT?

Another thing - do we know if the original ask also wants a way to set the context on initContainer? I know these only come into play when logs are enabled, but best to check if we need to include those.

No, as far as I know. That's why I removed it eventually

Add options to set container security contexts in clusterRecever and gateway deployments.
@dmitryax dmitryax force-pushed the add-container-security-context branch from dfadbbe to 12dcb22 Compare February 5, 2025 18:50
securityContext:
{{- include "splunk-otel-collector.securityContext" (dict "isWindows" .Values.isWindows "securityContext" $gateway.securityContext) | nindent 8 }}
{{- include "splunk-otel-collector.securityContext" (dict "isWindows" .Values.isWindows "securityContext" $podSecurityContext) | nindent 8 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{{- include "splunk-otel-collector.securityContext" (dict "isWindows" .Values.isWindows "securityContext" $podSecurityContext) | nindent 8 }}
{{- include "splunk-otel-collector.securityContext" (dict "isWindows" .Values.isWindows "securityContext" $gateway.podSecurityContext) | nindent 8 }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why?

securityContext:
{{- include "splunk-otel-collector.securityContext" (dict "isWindows" .Values.isWindows "securityContext" $clusterReceiver.securityContext) | nindent 8 }}
{{- include "splunk-otel-collector.securityContext" (dict "isWindows" .Values.isWindows "securityContext" $podSecurityContext) | nindent 8 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{{- include "splunk-otel-collector.securityContext" (dict "isWindows" .Values.isWindows "securityContext" $podSecurityContext) | nindent 8 }}
{{- include "splunk-otel-collector.securityContext" (dict "isWindows" .Values.isWindows "clusterReceiver.securityContext" $podSecurityContext) | nindent 8 }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why?

Comment on lines +87 to +89
{{- if not (eq (toString .Values.clusterReceiver.securityContext) "<nil>") }}
[WARNING] "clusterReceiver.securityContext" parameter is deprecated. Please use "clusterReceiver.podSecurityContext" instead.
{{ end }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
{{- if not (eq (toString .Values.clusterReceiver.securityContext) "<nil>") }}
[WARNING] "clusterReceiver.securityContext" parameter is deprecated. Please use "clusterReceiver.podSecurityContext" instead.
{{ end }}
{{- if not (eq (toString .Values. clusterReceiver.securityContext) "<nil>") }}
{{- if .Values. clusterReceiver.podSecurityContext }}
[WARNING] "clusterReceiver.securityContext" parameter is deprecated. Helm release will use values set in "clusterReceiver.podSecurityContext" instead.
{{- else }}
[WARNING] "clusterReceiver.securityContext" parameter is deprecated. Please use "clusterReceiver.podSecurityContext" instead.
{{- end }}
{{ end }}

@jvoravong is this what you are suggesting? How does the PR you referenced relate to this one? Tmk this is a different ask

@dmitryax dmitryax merged commit 12d08d6 into main Feb 5, 2025
64 of 65 checks passed
@dmitryax dmitryax deleted the add-container-security-context branch February 5, 2025 19:20
@github-actions github-actions bot locked and limited conversation to collaborators Feb 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants