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

Removed unnecessary change of group ownership in chmod initContainer #486

Merged
merged 3 commits into from
Jul 21, 2022
Merged

Removed unnecessary change of group ownership in chmod initContainer #486

merged 3 commits into from
Jul 21, 2022

Conversation

hvaghani221
Copy link
Contributor

@hvaghani221 hvaghani221 commented Jul 13, 2022

Instead of modifying permissions of the entire pod/container directory, modify only logs file.

A file access control list (ACL) can provide permissions to a specific user or group without modifying the user/group owner of the file.
So, there is no need to change the group owner here:

chgrp -Rv {{ $agent.securityContext.runAsGroup | default 20000 }} /var/lib/docker/containers;
chmod -R g+rxs /var/lib/docker/containers;
setfacl -n -Rm d:g:{{ $agent.securityContext.runAsGroup | default 20000 }}:rx,g:{{ $agent.securityContext.runAsGroup | default 20000 }}:rx /var/lib/docker/containers;

Copy link
Contributor

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

This is an init container which applies chown one time only, any new pods created after that won't get the right owner flags.

@dmitryax
Copy link
Contributor

I don't think we can apply this change

@hvaghani221
Copy link
Contributor Author

This is an init container which applies chown one time only, any new pods created after that won't get the right owner flags.

right! I'll close the PR

@hvaghani221
Copy link
Contributor Author

@dmitryax

chgrp -Rv {{ $agent.securityContext.runAsGroup | default 20000 }} /var/lib/docker/containers;
chmod -R g+rxs /var/lib/docker/containers;
setfacl -n -Rm d:g:{{ $agent.securityContext.runAsGroup | default 20000 }}:rx,g:{{ $agent.securityContext.runAsGroup | default 20000 }}:rx /var/lib/docker/containers;

Is there any specific reason we need to change the group of all files inside the container directory?
I think we can avoid that and use setfacl without changing the original group.

@dmitryax
Copy link
Contributor

Is there any specific reason we need to change the group of all files inside the container directory?
I think we can avoid that and use setfacl without changing the original group.

I'm not sure. @rockb1017, who introduced this, maybe can answer. Feel free to play with this and see if it's really required

@hvaghani221 hvaghani221 reopened this Jul 15, 2022
@hvaghani221 hvaghani221 marked this pull request as draft July 15, 2022 12:40
@hvaghani221 hvaghani221 changed the title Restrict chown to only log file [WIP] Fix file modifications by chown init container Jul 15, 2022
@hvaghani221
Copy link
Contributor Author

hvaghani221 commented Jul 19, 2022

I tested these changes in minikube, EKS and AKS. It is working there. In there,

  • Group owner is kept as root
  • initContainer applies default ACL in pod directory. So, when a new pod is created, it will automatically inherit the parent's ACL

For some reason, it is not working in openshift(cri).

  • If I manually create a file or directory in /var/log/pods, it inherits the ACL
  • If I create a pod, the pod directory inherits ACL but the actual log file doesn't inherit, so it is not accessible to the non-root user

EDIT: Same behaviour is mentioned here https://github.com/signalfx/splunk-otel-collector-chart/blob/main/docs/advanced-configuration.md#running-the-container-in-non-root-user-mode

hvaghani221 and others added 3 commits July 20, 2022 12:41
Instead of modifying permissions of the entire pod/container directory, modify only logs file.
@hvaghani221 hvaghani221 changed the title [WIP] Fix file modifications by chown init container Removed unnecessary change of group ownership in chmod initContainer Jul 20, 2022
@hvaghani221 hvaghani221 marked this pull request as ready for review July 20, 2022 12:21
@dmitryax
Copy link
Contributor

@harshit-splunk does it break this functionality openshift?

@hvaghani221
Copy link
Contributor Author

@harshit-splunk does it break this functionality openshift?

it doesn't work with the current implementation either for openshift. It only gives read access to existing pod files. Agent will not have read access to newly created pods.

@dmitryax
Copy link
Contributor

dmitryax commented Jul 21, 2022

Sounds good. @harshit-splunk can you please make the statement that it doesn't work in cri-o more clear here, not just didn't work during testing

@dmitryax dmitryax merged commit 153db1f into signalfx:main Jul 21, 2022
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.

None yet

2 participants