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

Use "ContainerAdministrator" for windows node #809

Merged

Conversation

VihasMakwana
Copy link
Contributor

For Linux nodes, the runAsUser is set to 0 by default if nothing is specified in values.yaml.
But for windows, it leaves this field empty.
It makes sense to set runAsUserName inside windowsOptions to ContainerAdministrator which is the root equivalent for Windows node

@VihasMakwana VihasMakwana requested review from a team as code owners June 13, 2023 12:10
@VihasMakwana VihasMakwana marked this pull request as draft June 13, 2023 12:12
@VihasMakwana VihasMakwana force-pushed the fix-security-context-for-windows branch from 47fd5ed to 33a6274 Compare June 13, 2023 12:32
@VihasMakwana VihasMakwana marked this pull request as ready for review June 13, 2023 12:33
@rmfitzpatrick
Copy link
Contributor

Are there specific issues this addresses and documented requirements for this user instead of ContainerUser or LocalSystem?

@VihasMakwana
Copy link
Contributor Author

Ah, actually nope.
I came across #428.
It was mainly because of runAsUser being set even for Windows deployments. It was fixed AFAIK.
The reason why I raised this PR was to maintain consistency in Linux and Windows nodes.
In Linux, the agent's runAsUser is set to 0 i.e. root user if nothing is specified, but for Windows, it doesn't set anything so I made a few changes.

@VihasMakwana VihasMakwana force-pushed the fix-security-context-for-windows branch from 3b2e20e to 8c8ad0f Compare June 14, 2023 06:48
@VihasMakwana VihasMakwana requested a review from dmitryax June 14, 2023 06:49
@VihasMakwana
Copy link
Contributor Author

@dmitryax can you please re-review this one?

@jvoravong
Copy link
Contributor

Should the proposed configuration be set when isWindows: true?
I'm surprised we didn't see these changes affect https://github.com/signalfx/splunk-otel-collector-chart/tree/main/examples/kubernetes-windows-nodes.

@VihasMakwana
Copy link
Contributor Author

Yes @jvoravong , this will set only when isWindows is true

@jvoravong
Copy link
Contributor

In that example isWindows: true. Your changes should have added content to the rendered files in the example after running a make render command, please look into this.

@VihasMakwana
Copy link
Contributor Author

Okayy, will go through that

@VihasMakwana
Copy link
Contributor Author

Okay @jvoravong, I see why this is happening.
securityContext is set if we've specified $securityContext in values.yaml or if we have logsEngine to otel and logsEnabled.
In rendered examples for windows, none of them are set, so it doesn't affect https://github.com/signalfx/splunk-otel-collector-chart/tree/main/examples/kubernetes-windows-nodes.

@jvoravong jvoravong merged commit 7356d2c into signalfx:main Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants