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

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

Merged
merged 1 commit into from May 10, 2018

Conversation

@mtaufen
Contributor

mtaufen commented Feb 14, 2018

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

This comment has been minimized.

@mtaufen

mtaufen Feb 14, 2018

Contributor

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

This comment has been minimized.

@mtaufen

mtaufen Apr 25, 2018

Contributor

done via API server validation

@mtaufen mtaufen referenced this pull request Feb 15, 2018

Open

Dynamic Kubelet Configuration #281

27 of 27 tasks complete

@mtaufen mtaufen modified the milestones: v1.10, v1.11 Feb 26, 2018

@mtaufen mtaufen changed the title from WIP explicit kubelet config key in Node.Spec.ConfigSource.ConfigMap to explicit kubelet config key in Node.Spec.ConfigSource.ConfigMap Mar 6, 2018

@mtaufen

This comment has been minimized.

Contributor

mtaufen commented May 4, 2018

/retest

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented May 4, 2018

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@dashpole @dchen1107 @liggitt @mtaufen

Pull Request Labels
  • sig/cluster-lifecycle sig/node sig/scheduling: Pull Request will be escalated to these SIGs if needed.
  • priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
  • kind/feature: New functionality.
Help
@@ -60,7 +60,7 @@ func addKnownTypes(scheme *runtime.Scheme) error {
&ServiceProxyOptions{},
&NodeList{},
&Node{},
&NodeConfigSource{},
&SerializedNodeConfigSource{},

This comment has been minimized.

@liggitt

liggitt May 4, 2018

Member

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

This comment has been minimized.

@mtaufen

mtaufen May 4, 2018

Contributor

Hmm. It felt appropriate here, since the type it wraps is also here...
Do we expect the Kubelet to be the only component that ever wants to serialize this?

func validateConfigMapNodeConfigSourceSpec(source *core.ConfigMapNodeConfigSource, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}
// name, namespace, uid, and kubeletConfigKey must all be non-empty for ConfigMap
if source.Name == "" {

This comment has been minimized.

@liggitt

liggitt May 4, 2018

Member

move the name/namespace/kubeletconfigkey requirements into validateConfigMapNodeConfigSource and skip name format validation for each if a required error is added, so you don't end up with two errors per field ("this field is required" AND "this field's format is invalid")

This comment has been minimized.

@mtaufen

mtaufen May 4, 2018

Contributor

done

// name, namespace, and UID must all be non-empty for ConfigMapRef
if ref.Name == "" || ref.Namespace == "" || string(ref.UID) == "" {
return nil, status.FailSyncReasonPartialObjectReference, fmt.Errorf("%s, ObjectReference was: %#v", status.FailSyncReasonPartialObjectReference, ref)
// exactly one reference subfield of the config source must be non-nil, toady ConfigMap is the only reference subfield

This comment has been minimized.

@liggitt

liggitt May 4, 2018

Member

"toady ConfigMap" :)

This comment has been minimized.

@mtaufen

mtaufen May 4, 2018

Contributor

done :)

type ConfigMapNodeConfigSource struct {
// Namespace is the metadata.namespace of the referenced ConfigMap.
Namespace string `json:"namespace,omitempty" protobuf:"bytes,1,opt,name=namespace"`

This comment has been minimized.

@liggitt

liggitt May 4, 2018

Member

remove omitempty, this is required

This comment has been minimized.

@mtaufen

mtaufen May 4, 2018

Contributor

done

Namespace string `json:"namespace,omitempty" protobuf:"bytes,1,opt,name=namespace"`
// Name is the metadata.name of the referenced ConfigMap.
Name string `json:"name,omitempty" protobuf:"bytes,2,opt,name=name"`

This comment has been minimized.

@liggitt

liggitt May 4, 2018

Member

remove omitempty, this is required

This comment has been minimized.

@mtaufen

mtaufen May 4, 2018

Contributor

done

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

This comment has been minimized.

@liggitt

liggitt May 4, 2018

Member

remove omitempty, this is required

This comment has been minimized.

@mtaufen

mtaufen May 4, 2018

Contributor

done

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

This comment has been minimized.

@liggitt

liggitt May 4, 2018

Member

add +optional comment

This comment has been minimized.

@mtaufen

mtaufen May 4, 2018

Contributor

done

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

This comment has been minimized.

@liggitt

liggitt May 4, 2018

Member

add +optional comment

This comment has been minimized.

@mtaufen

mtaufen May 4, 2018

Contributor

done

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

This comment has been minimized.

@liggitt

liggitt May 4, 2018

Member

for posterity, this is what's going on here:

  1. kind/apiVersion were used by the kubelet to persist this struct to disk (they had no protobuf tags)
  2. 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.

This comment has been minimized.

@mtaufen

mtaufen May 4, 2018

Contributor

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 {

This comment has been minimized.

@liggitt

liggitt May 4, 2018

Member

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 {

This comment has been minimized.

@liggitt

liggitt May 4, 2018

Member

needs godoc

This comment has been minimized.

@mtaufen

mtaufen May 4, 2018

Contributor

done

@liggitt

This comment has been minimized.

Member

liggitt commented May 4, 2018

the ConfigMapNodeConfigSource looks much better to me, thanks

cc @kubernetes/sig-node-api-reviews

@mtaufen

This comment has been minimized.

Contributor

mtaufen commented May 4, 2018

/retest

@mtaufen

This comment has been minimized.

Contributor

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.

@mtaufen

This comment has been minimized.

Contributor

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

This comment has been minimized.

Contributor

mtaufen commented May 7, 2018

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

@dchen1107

This comment has been minimized.

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.

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

This comment has been minimized.

@dchen1107

dchen1107 May 8, 2018

Member

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

This comment has been minimized.

@mtaufen

mtaufen May 8, 2018

Contributor

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.

This comment has been minimized.

@dchen1107

dchen1107 May 8, 2018

Member

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

This comment has been minimized.

@mtaufen

mtaufen May 8, 2018

Contributor

I updated the comment to provide additional context.

This comment has been minimized.

@mtaufen

mtaufen May 8, 2018

Contributor

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.

This comment has been minimized.

@dchen1107

dchen1107 May 10, 2018

Member

Thanks.

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

This comment has been minimized.

@dchen1107

dchen1107 May 8, 2018

Member

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

This comment has been minimized.

@mtaufen

mtaufen May 8, 2018

Contributor
  1. You're right, it would be cleaner to remove the Spec suffix here, but it'll just get added back in #63314, when NodeConfigSource starts being used as part of the structured status.
  2. 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).

This comment has been minimized.

@dchen1107

dchen1107 May 10, 2018

Member

Ok. I can live with it. :-)

explicit kubelet config key in Node.Spec.ConfigSource.ConfigMap
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.
@dashpole

couple nits, otherwise looks good.

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,

This comment has been minimized.

@dashpole

dashpole May 9, 2018

Contributor

should this be kubeletconfiginternal.SerializedNodeConfigSource?

This comment has been minimized.

@mtaufen

mtaufen May 9, 2018

Contributor

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

This comment has been minimized.

@dashpole

dashpole May 9, 2018

Contributor

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.

This comment has been minimized.

@mtaufen

mtaufen May 9, 2018

Contributor
  1. Kubelet should never depend on kubeadm
  2. 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.

This comment has been minimized.

@dashpole

dashpole May 9, 2018

Contributor

I see, thanks for clarifying.

@dashpole

This comment has been minimized.

Contributor

dashpole commented May 9, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label May 9, 2018

@k8s-ci-robot

This comment has been minimized.

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:

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-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-merge-robot k8s-merge-robot merged commit b2fe2a0 into kubernetes:master May 10, 2018

16 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation mtaufen authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment