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.

Sign up for GitHub

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

Jump to bottom

explicit kubelet config key in Node.Spec.ConfigSource.ConfigMap #59847

Merged

k8s-github-robot merged 1 commit into kubernetes:master from mtaufen:dkcfg-explicit-keys

May 10, 2018

Contributor

mtaufen commented Feb 14, 2018 •

edited

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.

You must now specify Node.Spec.ConfigSource.ConfigMap.KubeletConfigKey when using dynamic Kubelet config to tell the Kubelet which key of the ConfigMap identifies its config file.

mtaufen added area/kubelet area/kubelet-api sig/node do-not-merge/work-in-progress do-not-merge/hold labels

mtaufen added this to the v1.10 milestone

mtaufen assigned liggitt

k8s-ci-robot added release-note size/XXL labels

k8s-github-robot added the kind/api-change label

mtaufen added priority/important-soon and removed kind/api-change labels

k8s-ci-robot added the cncf-cla: yes label

k8s-ci-robot requested review from gmarek, jszczepkowski, krousey, luxas, pmorie, resouer, tallclair and thockin

February 14, 2018 03:26

k8s-github-robot added the kind/api-change label

mtaufen force-pushed the dkcfg-explicit-keys branch 4 times, most recently from 0505944 to b5df7ce Compare

February 14, 2018 14:51

mtaufen commented

View reviewed changes

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

Contributor Author

mtaufen Feb 14, 2018

TODO: Sanitize/validate this against the same rules for ConfigMap keys. Wouldn't want it to be possible to put ../x in here...

Contributor Author

mtaufen Apr 25, 2018

done via API server validation

mtaufen force-pushed the dkcfg-explicit-keys branch 2 times, most recently from e16fe5e to c0d6ef4 Compare

February 15, 2018 02:52

mtaufen mentioned this pull request

Dynamic Kubelet Configuration kubernetes/enhancements#281

Closed

27 tasks

liggitt reviewed

View reviewed changes

staging/src/k8s.io/api/core/v1/types.go Outdated

+              	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"`

Member

liggitt May 4, 2018

remove omitempty, this is required

Contributor Author

mtaufen May 4, 2018

done

liggitt reviewed

View reviewed changes

staging/src/k8s.io/api/core/v1/types.go Outdated

+              	// 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"`

Member

liggitt May 4, 2018

add +optional comment

Contributor Author

mtaufen May 4, 2018

done

liggitt reviewed

View reviewed changes

staging/src/k8s.io/api/core/v1/types.go Outdated

+              	// 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"`

Member

liggitt May 4, 2018

add +optional comment

Contributor Author

mtaufen May 4, 2018

done

liggitt reviewed

View reviewed changes

staging/src/k8s.io/api/core/v1/types.go Outdated

               // 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

Member

liggitt May 4, 2018 •

edited

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.

Contributor Author

mtaufen May 4, 2018

Added this as a comment in the file, so it doesn't just live on GitHub.

liggitt reviewed

View reviewed changes

staging/src/k8s.io/api/core/v1/types.go Outdated

+              // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+              // SerializedNodeConfigSource allows us to serialize NodeConfigSource
+              type SerializedNodeConfigSource struct {

Member

liggitt May 4, 2018

same comment, this doesn't really belong in the v1 API, it belongs to the kubelet

liggitt reviewed

View reviewed changes

staging/src/k8s.io/api/core/v1/types.go Outdated

+              	Source NodeConfigSource `json:"source,omitempty" protobuf:"bytes,1,opt,name=source"`
+              }
+              type ConfigMapNodeConfigSource struct {

Member

liggitt May 4, 2018

needs godoc

Contributor Author

mtaufen May 4, 2018

done

Member

liggitt commented May 4, 2018

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

May 4, 2018 21:49

Contributor Author

mtaufen commented May 4, 2018

/retest

mtaufen force-pushed the dkcfg-explicit-keys branch 2 times, most recently from 7bdca92 to fd41e55 Compare

May 7, 2018 16:30

Contributor Author

mtaufen commented May 7, 2018

@dchen1107 @liggitt @derekwaynecarr

this should be able to be internal to the kubelet, right? not defined/registered for the entire v1 API group

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.

Contributor Author

mtaufen commented May 7, 2018

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

May 7, 2018 20:09

Contributor Author

mtaufen commented May 7, 2018

updated the location of the serialized reference; still need to regenerate some files

Member

dchen1107 commented May 7, 2018

/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 label

mtaufen force-pushed the dkcfg-explicit-keys branch 3 times, most recently from 4d03bf6 to f573eed Compare

May 7, 2018 21:28

dchen1107 reviewed

View reviewed changes

pkg/kubelet/kubeletconfig/checkpoint/download_test.go

    
              		{"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"}}}, ""},

              		{

Member

dchen1107 May 7, 2018

can we have more test cases here, for example, empty name? empty namespace? or UID?

Contributor Author

mtaufen May 8, 2018

Those cases (empty ConfigMapNodeConfigSource fields) should already be covered by API server validation. I don't see a reason to duplicate that validation here.

pkg/apis/core/types.go

    
              	// This field is required in all cases.

              	Name string

              	// UID is the metadata.UID of the referenced ConfigMap.

Member

dchen1107 May 7, 2018

The comment here is very hard to understand, could you please add more clarification here?

Contributor Author

mtaufen May 8, 2018

I updated the comment to provide additional context.

Contributor Author

mtaufen May 8, 2018 •

edited

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.

Member

dchen1107 May 10, 2018

Thanks.

pkg/apis/core/validation/validation.go

    
            @@ -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 {

Member

dchen1107 May 8, 2018

Is there any reason separating this validation of a single object into two separate methods here since both are same spec related?

Contributor Author

mtaufen May 8, 2018

You're right, it would be cleaner to remove the Spec suffix 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 validateNodeConfigSourceSpec and validateConfigMapNodeConfigSourceSpec recognizes 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).

Member

dchen1107 May 10, 2018

Ok. I can live with it. :-)


          explicit kubelet config key in Node.Spec.ConfigSource.ConfigMap

c41cf55

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

May 8, 2018 22:37

dashpole reviewed

View reviewed changes

Contributor

dashpole left a comment

couple nits, otherwise looks good.

pkg/kubelet/kubeletconfig/checkpoint/download.go

               	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,

Contributor

dashpole May 9, 2018

should this be kubeletconfiginternal.SerializedNodeConfigSource?

Contributor Author

mtaufen May 9, 2018

No. The external version is what we load; the decoder automatically converts it to the internal version as part of the decoding process.

pkg/kubelet/kubeletconfig/checkpoint/download_test.go

+              				Name:             "name",
+              				Namespace:        "namespace",
+              				UID:              "bogus",
+              				KubeletConfigKey: "kubelet",

Contributor

dashpole May 9, 2018

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.

Contributor Author

mtaufen May 9, 2018

Kubelet should never depend on kubeadm
I'm not sure I understand your question? Whatever kubeletConfigKey is 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.

Contributor

dashpole May 9, 2018

I see, thanks for clarifying.

Contributor

dashpole commented May 9, 2018

/lgtm

k8s-ci-robot added the lgtm label

Contributor

k8s-ci-robot commented May 9, 2018

[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:

~~api/OWNERS~~ [dchen1107]
~~cmd/kubeadm/OWNERS~~ [dchen1107]
~~docs/api-reference/OWNERS~~ [dchen1107]
~~pkg/apis/core/OWNERS~~ [dchen1107]
~~pkg/kubelet/apis/kubeletconfig/OWNERS~~ [dchen1107,mtaufen]
~~pkg/kubelet/kubeletconfig/OWNERS~~ [dchen1107,mtaufen]
~~plugin/pkg/admission/noderestriction/OWNERS~~ [dchen1107]
~~plugin/pkg/auth/authorizer/node/OWNERS~~ [dchen1107]
~~staging/src/k8s.io/api/OWNERS~~ [dchen1107]
~~test/OWNERS~~ [dchen1107]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-github-robot commented May 10, 2018

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.

k8s-github-robot merged commit b2fe2a0 into kubernetes:master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

liggitt liggitt left review comments

dashpole dashpole left review comments

dchen1107 dchen1107 left review comments

gmarek Awaiting requested review from gmarek

jszczepkowski Awaiting requested review from jszczepkowski

krousey Awaiting requested review from krousey

luxas Awaiting requested review from luxas

pmorie Awaiting requested review from pmorie

resouer Awaiting requested review from resouer

tallclair Awaiting requested review from tallclair

thockin Awaiting requested review from thockin

Labels

approved area/kubeadm area/kubelet area/kubelet-api cncf-cla: yes kind/api-change kind/feature lgtm priority/important-soon release-note sig/cluster-lifecycle sig/node sig/scheduling size/XL