explicit kubelet config key in Node.Spec.ConfigSource.ConfigMap #59847
Conversation
mtaufen
added
area/kubelet
area/kubelet-api
sig/node
k8s-ci-robot
added
release-note
k8s-github-robot
added
the
kind/api-change
mtaufen
added
priority/important-soon
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
requested review from
gmarek,
jszczepkowski,
krousey,
luxas,
pmorie,
resouer,
tallclair and
thockin
k8s-github-robot
added
the
kind/api-change
mtaufen
force-pushed
the
dkcfg-explicit-keys
branch
4 times, most recently
from
0505944 to
b5df7ce
Compare
pkg/apis/core/types.go
Outdated
| // ObjectReference points to the ConfigMap. Name, Namespace, and UID must be specified. | ||
| ObjectReference | ||
| // KubeletConfigKey defines which key of the referenced ConfigMap corresponds to the serialized KubeletConfiguration | ||
| KubeletConfigKey string |
There was a problem hiding this comment.
TODO: Sanitize/validate this against the same rules for ConfigMap keys. Wouldn't want it to be possible to put ../x in here...
There was a problem hiding this comment.
done via API server validation
mtaufen
force-pushed
the
dkcfg-explicit-keys
branch
2 times, most recently
from
e16fe5e to
c0d6ef4
Compare
| ResourceVersion string `json:"resourceVersion,omitempty" protobuf:"bytes,4,opt,name=resourceVersion"` | ||
|
|
||
| // KubeletConfigKey declares which key of the referenced ConfigMap corresponds to the KubeletConfiguration structure | ||
| KubeletConfigKey string `json:"kubeletConfigKey,omitempty" protobuf:"bytes,5,opt,name=kubeletConfigKey"` |
There was a problem hiding this comment.
remove omitempty, this is required
| // UID is the metadata.UID of the referenced ConfigMap. | ||
| // TODO(#61643,#56896): This will be disallowed by validation in Node.Spec, | ||
| // and only be set in Node.Status, when #61643 and #56896 are resolved, respectively. | ||
| UID types.UID `json:"uid,omitempty" protobuf:"bytes,3,opt,name=uid"` |
|
|
||
| // ResourceVersion is the metadata.ResourceVersion of the referenced ConfigMap. | ||
| // This field is disallowed by validation in Node.Spec, but will be set in Node.Status when #56896 is resolved. | ||
| ResourceVersion string `json:"resourceVersion,omitempty" protobuf:"bytes,4,opt,name=resourceVersion"` |
| // NodeConfigSource specifies a source of node configuration. Exactly one subfield (excluding metadata) must be non-nil. | ||
| type NodeConfigSource struct { | ||
| // +k8s:deprecated=kind | ||
| // +k8s:deprecated=apiVersion | ||
| // +k8s:deprecated=configMapRef,protobuf=1 |
There was a problem hiding this comment.
for posterity, this is what's going on here:
- kind/apiVersion were used by the kubelet to persist this struct to disk (they had no protobuf tags)
- configMapRef and proto tag 1 were used by the API to refer to a configmap, but used a generic ObjectReference type that didn't really have the fields we needed
all uses/persistence of the NodeConfigSource struct prior to 1.11 were gated by alpha feature flags, so there is no persisted data for these fields that needs to be migrated/handled.
There was a problem hiding this comment.
Added this as a comment in the file, so it doesn't just live on GitHub.
| // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object | ||
|
|
||
| // SerializedNodeConfigSource allows us to serialize NodeConfigSource | ||
| type SerializedNodeConfigSource struct { |
There was a problem hiding this comment.
same comment, this doesn't really belong in the v1 API, it belongs to the kubelet
| Source NodeConfigSource `json:"source,omitempty" protobuf:"bytes,1,opt,name=source"` | ||
| } | ||
|
|
||
| type ConfigMapNodeConfigSource struct { |
|
the ConfigMapNodeConfigSource looks much better to me, thanks cc @kubernetes/sig-node-api-reviews |
mtaufen
force-pushed
the
dkcfg-explicit-keys
branch
2 times, most recently
from
f410be2 to
f534fc7
Compare
|
/retest |
mtaufen
force-pushed
the
dkcfg-explicit-keys
branch
2 times, most recently
from
7bdca92 to
fd41e55
Compare
|
@dchen1107 @liggitt @derekwaynecarr
Whether to move SerializedNodeConfigSource from core API group (current state of this PR) to kubeletconfig isn't an easy question to answer. On one hand, the Kubelet is the only component that uses the serializable wrapper type right now (for locally tracking assigned and last-known-good config), which was the reason behind @liggitt's recommendation to move it. On the other hand, the feature the type pertains to (dynamic Kubelet config) is part of the core API group (NodeConfigSource). The kubeletconfig API group is concerned with the structure of the Kubelet's configuration, and I'm not sure it should also include delivery mechanisms for that API (nobody would even think of embedding ConfigMapVolumeSource in the componentconfig API group of a component that runs in a Pod; they'd just use ConfigMapVolumeSource to deliver the config). I was initially comfortable moving it, but these points are making me think twice. |
|
I thought SerializedObjectReference in core would be a good argument for SerializedNodeConfigSource in core, but the history of that field (#7322) suggests it instead fits the core purpose of "serving REST APIs," which SerializedNodeConfigSource would not. The kubeletconfig API group can assume the purpose of managing "versioned inputs to the Kubelet," and SerializedNodeConfigSource fits this description. |
mtaufen
force-pushed
the
dkcfg-explicit-keys
branch
from
fd41e55 to
17eec66
Compare
|
updated the location of the serialized reference; still need to regenerate some files |
|
/approve I approved the pr now so that I won't become the bottom-neck on promoting dynamic Kubelet Config to beta. Will take a deep review on the implementation later. |
k8s-ci-robot
added
the
approved
mtaufen
force-pushed
the
dkcfg-explicit-keys
branch
3 times, most recently
from
4d03bf6 to
f573eed
Compare
| {"ConfigMapRef: valid reference", | ||
| &apiv1.NodeConfigSource{ConfigMapRef: &apiv1.ObjectReference{Name: "name", Namespace: "namespace", UID: "uid"}}, | ||
| &remoteConfigMap{&apiv1.NodeConfigSource{ConfigMapRef: &apiv1.ObjectReference{Name: "name", Namespace: "namespace", UID: "uid"}}}, ""}, | ||
| { |
There was a problem hiding this comment.
can we have more test cases here, for example, empty name? empty namespace? or UID?
There was a problem hiding this comment.
Those cases (empty ConfigMapNodeConfigSource fields) should already be covered by API server validation. I don't see a reason to duplicate that validation here.
| // This field is required in all cases. | ||
| Name string | ||
|
|
||
| // UID is the metadata.UID of the referenced ConfigMap. |
There was a problem hiding this comment.
The comment here is very hard to understand, could you please add more clarification here?
There was a problem hiding this comment.
I updated the comment to provide additional context.
There was a problem hiding this comment.
New comment text:
// UID is the metadata.UID of the referenced ConfigMap.
// This field is currently reqired in Node.Spec.
// TODO(#61643): This field will be forbidden in Node.Spec when #61643 is resolved.
// #61643 changes the behavior of dynamic Kubelet config to respect
// ConfigMap updates, and thus removes the ability to pin the Spec to a given UID.
// TODO(#56896): This field will be required in Node.Status when #56896 is resolved.
// #63314 (the PR that resolves #56896) adds a structured status to the Node
// object for reporting information about the config. This status requires UID
// and ResourceVersion, so that it represents a fully-explicit description of
// the configuration in use, while (see previous TODO) the Spec will be
// restricted to namespace/name in #61643.
| @@ -4154,6 +4152,50 @@ func validateNodeConfigSource(source *core.NodeConfigSource, fldPath *field.Path | |||
| return allErrs | |||
| } | |||
|
|
|||
| // validation specific to Node.Spec.ConfigSource.ConfigMap | |||
| func validateConfigMapNodeConfigSourceSpec(source *core.ConfigMapNodeConfigSource, fldPath *field.Path) field.ErrorList { | |||
There was a problem hiding this comment.
Is there any reason separating this validation of a single object into two separate methods here since both are same spec related?
There was a problem hiding this comment.
- You're right, it would be cleaner to remove the
Specsuffix here, but it'll just get added back in Move to a structured status for dynamic kubelet config #63314, when NodeConfigSource starts being used as part of the structured status. - The split between
validateNodeConfigSourceSpecandvalidateConfigMapNodeConfigSourceSpecrecognizes that the former's target (NodeConfigSource) acts as a disjoint union type, and the latter's target (ConfigMapNodeConfigSource) is one of the possible subtypes (there is only one subtype today - a ConfigMap source, but it's open to extension in the future).
This makes the Kubelet config key in the ConfigMap an explicit part of the API, so we can stop using magic key names. As part of this change, we are retiring ConfigMapRef for ConfigMap.
mtaufen
force-pushed
the
dkcfg-explicit-keys
branch
from
f573eed to
c41cf55
Compare
| if err != nil { | ||
| return nil, fmt.Errorf("failed to decode, error: %v", err) | ||
| } | ||
|
|
||
| // for now we assume we are trying to load an apiv1.NodeConfigSource, | ||
| // for now we assume we are trying to load an kubeletconfigv1beta1.SerializedNodeConfigSource, |
There was a problem hiding this comment.
should this be kubeletconfiginternal.SerializedNodeConfigSource?
There was a problem hiding this comment.
No. The external version is what we load; the decoder automatically converts it to the internal version as part of the decoding process.
| Name: "name", | ||
| Namespace: "namespace", | ||
| UID: "bogus", | ||
| KubeletConfigKey: "kubelet", |
There was a problem hiding this comment.
Is there a reason why we dont use kubeadmconstants.KubeletBaseConfigurationConfigMapKey here? Using "key" or something other than the actual key would make it clearer that we aren't relying on the actual key. Or we should use the constant for the key if we are relying on that.
There was a problem hiding this comment.
- Kubelet should never depend on kubeadm
- I'm not sure I understand your question? Whatever
kubeletConfigKeyis set to determines where the Kubelet looks for the Kubelet config. There is no "actual key" - the choice of where to put the config in the ConfigMap is up to the user.
There was a problem hiding this comment.
I see, thanks for clarifying.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dashpole, dchen1107, mtaufen The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Automatic merge from submit-queue (batch tested with PRs 63624, 59847). If you want to cherry-pick this change to another branch, please follow the instructions here. |
This makes the Kubelet config key in the ConfigMap an explicit part of
the API, so we can stop using magic key names.
As part of this change, we are retiring ConfigMapRef for ConfigMap.