AmarNathChary
commented
1 year ago /sig storage /language en
Open lmsurpre opened 1 year ago
AmarNathChary
commented
1 year ago /sig storage /language en
niranjandarshann
commented
1 year ago @lmsurpre Yes, we can see that the secrets and configmap are by default readonly till i understood in my local. Here in the issue #62099 i see severals comments are referring toward the version change. I think the changes made in https://github.com/goauthentik/authentik/pull/2536 PR can have some solution about this issue.
sftim
commented
1 year ago /kind feature /triage accepted /priority backlog /help
k8s-ci-robot
commented
1 year ago @sftim: This request has been marked as needing help from a contributor.
Please ensure that the issue body includes answers to the following questions:
For more details on the requirements of such an issue, please see here and ensure that they are met.
If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.
joshuapare
commented
1 year ago Hey there! I'd like to make my first contribute to Kubernetes, and would like to take this on to start and get familiar with the process.
/assign @sbhrule15
lmsurpre
commented
1 year ago @sbhrule15 thanks for trying your hand at the doc update!
I'm not a project member or anything, but I did scroll through your changes and I think there's an important point we need to resolve.
My understanding is that mounted configMaps and secrets are not just read-only "by default" but rather can only be mounted as readOnly.
That is sort of alluded to by https://kubernetes.io/docs/concepts/configuration/configmap/#configmaps-and-pods where it says:
There are four different ways that you can use a ConfigMap to configure a container inside a Pod: ... Add a file in read-only volume, for the application to read ...
But I believe this should be explicitly stated in both the ConfigMap and Secret section of this Volumes page.
joshuapare
commented
1 year ago @lmsurpre Thanks for the catch! Looking into the controllers in the core show that this is in fact the case:
https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/secret/secret.go#L171 https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/configmap/configmap.go#L166
Intersting note - while looking here, one thing I found is that it looks like emptyDir behaves exactly the opposite, where it can only be readOnly: false. Not sure if this is documented, but this is definitly news to me (though it does make sense as it would be odd to be read-only).
https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/configmap/configmap.go#L166
Whereas something like the NFS mounter shows this as configurable:
https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/nfs/nfs.go#L219
Should this be documented as well?
joshuapare
commented
1 year ago Actually, I think this line in the docs implies that?
All containers in the Pod can read and write the same files in the emptyDir volume
lmsurpre
commented
1 year ago Intersting note - while looking here, one thing I found is that it looks like emptyDir behaves exactly the opposite, where it can only be readOnly: false. Not sure if this is documented, but this is definitly news to me (though it does make sense as it would be odd to be read-only).
thats news to me too.
and personally i can think of a use case: if you're mounting an emptyVolume from two containers in the same pod, it seems to me like you very well might want it read-only on one and read-write on the other.
Actually, I think this line in the docs implies that?
All containers in the Pod can read and write the same files in the emptyDir volume
sure would be nice if that were more explicit
joshuapare
commented
1 year ago @lmsurpre Actually I can think of another very common use case for this - logging. Almost all of our applications run Promtail as a sidecar to process and ingest logs, and it would be great to ensure that the mount to the sidecar is always readonly.
Same with some other reporting sidecars that I can think of that are process-local.
niranjandarshann
commented
1 year ago As i gone through the documentation and find the useful conclusion like we don't need to make Configmap and Secret as readwrite. It must be readonly. As configmap and secret are designed to store configuration data and sensitive information, respectively which can be easily consumed by containers running in pods. The "read-only" nature of ConfigMaps and Secrets ensures that the data they contain cannot be modified by the running application or any unauthorized entities. This is a security measure to prevent accidental or intentional tampering with configuration data or sensitive information. So according to explanation why do we try to make them rw as it is not good for security. Thats why Secret and Configmap should remain in readonly .
joshuapare
commented
1 year ago @niranjandarshann Agreed - we are talking about adding the ability for an emptyDir volume mount to have read-only mount permission ability, instead of the current behavior which appears to be that all mounts are currently writeable by all containers, shown by the GetAttributes call hard-coded to ReadOnly: False instead of referencing the struct property:
https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/emptydir/empty_dir.go#L228
lmsurpre
commented
1 year ago To be clear, this issue (#41103) is only about documenting what exists today.
@sbhrule15 do you want to use this issue to address the documentation around emptyDir volume mounts always being read/write, or should we close this now that the Secrets and ConfigMaps documentation is clear (the original scope of the issue)?
@sbhrule15 I suggest you open a separate issue for the idea of emptyDir volume mounts supporting read-only.
mirekphd
commented
1 year ago This is a security measure to prevent accidental or intentional tampering with configuration data or sensitive information.
How about browser-based IDEs? Jupyter Notebook, Jupyter Lab, VS Code Server (code-server), and even RStudio Server all write to their config files after each and every user change made from their Config Menus (a.k.a. "intentional tampering"), which the app deployment team may be unlucky to have initialized using the ConfigMap (now dreaded, "it no longer works, can't upgrade to OKD 4 because of that, guys"). Here's just one example from Kubeflow - no. 3 - their preferred approach.
Am I the only one thinking that k8s core dev team is getting too opinionated here?
sftim
commented
1 year ago @mirekphd, adding documentation about how Kubernetes already works doesn't sound to me like “getting too opinionated”.
(if people want to suggest adding a way to configure a different behavior, this issue is the wrong place to have that discussion)
mirekphd
commented
1 year ago @sftim So this equal treatment of config files like they could only contain immutable secrets by hard-coding a read-only file attribute for both of them is a done deal now, etched in stone?
I did not object to the use of loaded/biased terms like "tampering" or "sensitive information" there, because everyone has a right to their opinion.
Back to the issue though. Apparently no feature poll was conducted, no broad-based opinion was sought and this undocumented change is being rationalized after the fact based on a necessarily limited (some may even argue too narrow) view of possible use cases, disregarding most stateful containers, not just the AI/ML workloads I mentioned above.
To be constructive, I've found a small user opinion poll on the usefulness of writing by any pod user (with arbitrary UID) to config files uploaded from Config Maps (up to 228 votes in favor, 0 against): https://github.com/kubernetes/kubernetes/issues/71356#issuecomment-441169334
On the basis of this evidence this should not be legitimized by documenting, on the contrary, this hard-coding of the read-only attribute for Config Maps should be reverted IMHO.
k8s-triage-robot
commented
2 months ago This issue has not been updated in over 1 year, and should be re-triaged.
You can:
/triage accepted (org members only)/closeFor more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/
/remove-triage accepted
I assumed this to be the case, but I couldn't find any confirmation in the documentation on the Volumes page where I expected it:
Instead I only found my confirmation at https://github.com/kubernetes/kubernetes/issues/62099 and so I thought it would be good to update the page. I'm happy to draft some minor changes if that helps.