Update proposal for ConfigMap volume #19923
Conversation
|
Labelling this PR as size/S |
k8s-github-robot
added
kind/design
|
GCE e2e test build/test passed for commit aa1a216fe0fb561a23fc4126a4908bef8eac49d9. |
| key: redis.conf | ||
| - name: config-map-volume | ||
| configMap: | ||
| configMapName: redis-volume-config |
There was a problem hiding this comment.
For env we chose a LocalObjectRef, and for volumes we chose a Name. That smells a bit. LocalObjectRef allows for extension into cross-namespace references, whcih seems plausible for large, complex applications. A simple name would NOT allow that. I spoke to Brian this morning and I'm coming around to cross-namespace stuff being viable.
Should we be converting to LocalObjectRef here?
There was a problem hiding this comment.
...and if we do, Secret should change, too. We can always change later, I guess, by marking this field as optional (which Secret does with SecretName) and later changing to one-of SecretName or [Namespace]+Name
There was a problem hiding this comment.
@thockin Yeah, we want to move to using LocalObjectReference eventually.
There was a problem hiding this comment.
@thockin For LocalObjectReference we would need to default the value of the old field for old clients. I can do that for both this and secrets in a follow-up if we want to do that now.
There was a problem hiding this comment.
SecretVolumeSource.SecretName is omitempty already, so you don't NEED to default it, strictly. I still would.
If we're going to move this way, why add ConfigMapName at all, if we just intend to deprecate it? We could just start with LocalObjectRef for this one and move Secret to be consistent, with a deprecated-but-working SecretName
There was a problem hiding this comment.
@thockin ugh, you're right, but that's a bug. SecretName should not be omitempty, since it is required.
Anyway, I'm fine with using LocalObjectReference now, but I thought we wanted to make this like existing volumes. For example, we used Items for the list of projections into files because that's what downward API volume uses. If we use LocalObjectReference, we should use files instead of items, too. What do you say?
There was a problem hiding this comment.
type ConfigMapVolumeSource struct {
LocalObjectReference `json:",inline"`
Files []KeyToPath `json:"files,omitempty"`
}There was a problem hiding this comment.
apiVersion: v1
kind: Pod
metadata:
name: my-pod
spec:
containers:
- name: test-container
image: my-image
volumeMounts:
# name must match the volume name below
- name: config-volume
mountPath: /etc/config-volume
volumes:
- name: config-volume
configMap:
name: my-config-map
files:
- key: my-config-key
path: path/to/config.properties
- key: another-config-key
path: path/to/logging.properties
restartPolicy: Never
|
Please move this doc out of proposals to design. |
pmorie
force-pushed
the
config-volume-proposal
branch
2 times, most recently
from
f1d0b12 to
b75f1f2
Compare
k8s-github-robot
added
size/M
|
Labelling this PR as size/M |
|
@thockin Should be consistent internally now. Let me know if you desire more detail. |
|
GCE e2e test build/test passed for commit f1d0b120b57356026147e84f88b15a89fcd45168. |
|
@k8s-bot unit test this |
|
GCE e2e test build/test passed for commit b75f1f2f2662c91bec998251bed1e0e1f6dcc359. |
|
just the one point |
|
GitHub lost all the comments. Yay GitHub.
Nooooo! It's a feature, since you want it that way, now. :)
I do want consistency. Both can consistently use LocalObjectRef :) Deprecate Secret.SecretName. Internal has only Name. Conversion takes SecretName if it exists, else Name on the way in, and produces both on the way out. Downward API still uses items, so these should use items, too. Deprecating a list is more trouble than it is worth here. |
|
Please don't duplicate/alias fields in the same API versions. It's a royal PITA for updates. |
| // is the key and content is the value. | ||
| // If specified, the listed keys will be project into the specified paths, and | ||
| // unlisted keys will not be present. | ||
| Items []KeyToPath `json:"items,optional"` |
So, keep secret as-is for now? Should we still use LocalObjectReference for config map volume? |
|
Yes, keep Secret as is. We'll have to fix it in a future API version, if ever. An inline LocalObjectReference for ConfigMap is fine. The redundant "configMap" string in the name field is unnecessary. The inconsistency is annoying, but exists in other objects also, and the fix for Secret is more complicated than it's worth. |
|
@bgrant0607 Ack |
pmorie
force-pushed
the
config-volume-proposal
branch
from
b75f1f2 to
918a694
Compare
|
@bgrant0607 @thockin feedback addressed |
|
GCE e2e test build/test passed for commit 918a694. |
|
LGTM, thanks |
bgrant0607
added
the
lgtm
|
Automatic merge from submit-queue |
Auto commit by PR queue bot
Auto commit by PR queue bot
Auto commit by PR queue bot
Updates the ConfigMap volume design to reflect the outcome of #18586; proposes a new API that will be backward compatible when applied to secrets.
@thockin @bgrant0607 @erictune @smarterclayton
cc @kubernetes/rh-cluster-infra