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

feat: add uid check for chown in initContainer #32

Merged
merged 3 commits into from
Jan 30, 2023
Merged

feat: add uid check for chown in initContainer #32

merged 3 commits into from
Jan 30, 2023

Conversation

d7oc
Copy link
Contributor

@d7oc d7oc commented Dec 15, 2022

If the initContainer is started in an environment which is rootless (e.g. OpenShift) the chown command will fail. This PR adds a check for the user id before executing chown to avoid this.

@d7oc d7oc requested a review from xoxys December 15, 2022 16:27
@wkloucek
Copy link

@d7oc
Copy link
Contributor Author

d7oc commented Dec 16, 2022

Indeed this was also the second solution I had in mind, just with the difference to use the skipChown we already have for the "normal" container. I'm agnostic here which solution we should take. The upside of the current patch would be that the user doesn't have to care, it will just work. The downside is of course missing flexibility.

@wkloucek
Copy link

Indeed this was also the second solution I had in mind, just with the difference to use the skipChown we already have for the "normal" container. I'm agnostic here which solution we should take. The upside of the current patch would be that the user doesn't have to care, it will just work. The downside is of course missing flexibility.

I wonder how you can force the initcontainer to run as root!? oCIS is doing it like this https://github.com/owncloud/ocis-charts/blob/08943e7b50b39a055bb86b90c03c39c03d51e55f/charts/ocis/templates/idm/deployment.yaml#L28-L38

@d7oc
Copy link
Contributor Author

d7oc commented Dec 16, 2022

I wonder how you can force the initcontainer to run as root!? oCIS is doing it like this https://github.com/owncloud/ocis-charts/blob/08943e7b50b39a055bb86b90c03c39c03d51e55f/charts/ocis/templates/idm/deployment.yaml#L28-L38

This happens automatically on k3d and Minikube. Only OpenShift behaves differently and just executes the initContainer with a random UID.

So I guess the securityContext defaults differ

@xoxys xoxys changed the title [FR] added id check for chown in initContainer feat: add uid check for chown in initContainer Jan 10, 2023
@xoxys
Copy link
Contributor

xoxys commented Jan 10, 2023

I would prefer the approach from the ocis chart. @d7oc can you adapt it? I'm ok to reuse the existing skipChown property.

@d7oc
Copy link
Contributor Author

d7oc commented Jan 11, 2023

I would prefer the approach from the ocis chart. @d7oc can you adapt it? I'm ok to reuse the existing skipChown property.

Changed.

@xoxys
Copy link
Contributor

xoxys commented Jan 12, 2023

I was more talking about the two dedicated init containers as shown in the ocis example https://github.com/owncloud/ocis-charts/blob/master/charts/ocis/templates/idm/deployment.yaml#L28-L50

@d7oc
Copy link
Contributor Author

d7oc commented Jan 12, 2023

Ah. I'm wondering how this works reliably. The execution order of the initContainers isn't determined AFAIK, so if the chown initContainer is executed before the mkdir initContainer this fails. So if I'm correct this would be an error-prone approach.

@xoxys
Copy link
Contributor

xoxys commented Jan 12, 2023

Then the kubelet runs the Pod's init containers in the order they appear in the Pod's spec.

https://kubernetes.io/docs/concepts/workloads/pods/init-containers/

@d7oc
Copy link
Contributor Author

d7oc commented Jan 12, 2023

Then the kubelet runs the Pod's init containers in the order they appear in the Pod's spec.

https://kubernetes.io/docs/concepts/workloads/pods/init-containers/

👍 Good to know.

Will change it.

Shouldn't the order in ocis-chart be changed in this case?

@d7oc
Copy link
Contributor Author

d7oc commented Jan 12, 2023

Adapted

@d7oc
Copy link
Contributor Author

d7oc commented Jan 12, 2023

But one thing just came to my mind, what happens if the first initContainer fails? Will the second not be executed as well?

Found the answer already so all is fine:

Each init container must exit successfully before the next container starts.

@d7oc d7oc merged commit d7f6b4d into main Jan 30, 2023
@delete-merged-branch delete-merged-branch bot deleted the d7-init-chown branch January 30, 2023 13:46
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.

3 participants