Skip to content
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

fix: make kubeadm set defaults to kubelet configuration only when no values are set. #81903

Merged
merged 1 commit into from Aug 28, 2019

Conversation

@jfbai
Copy link
Contributor

commented Aug 25, 2019

What type of PR is this?

/kind bug

What this PR does / why we need it:

Currently, kubeadm set default values to several kubelet configurations which will overwrite the values set by users. Take ReadOnlyPort for example, in our cluster, we have to enable this port to make other component work well on the same host. But, it is unable to customize this config via kubeadm workflow in a fully kubeadm managed cluster.

So, it would be better to set defaults only when the config item is not set (nil or ""), and should not change users' settings, otherwise, it may break users' expectation.

Which issue(s) this PR fixes:

Fixes #kubernetes/kubeadm#1751

Special notes for your reviewer:

None

Does this PR introduce a user-facing change?:

kubeadm: prevent overriding of certain kubelet security configuration parameters if the user wished to modify them

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

None

/sig cluster-lifecycle
/area kubeadm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2019

Hi @jfbai. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jfbai

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2019

/assign @neolit123
/assign @luxas

@neolit123

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

/assign @rosti

@jfbai
thanks for the PR, please add a release note under Does this PR introduce a user-facing change?:

kubeadm: prevent overriding of certain kubelet security configuration parameters if the user wished to modify them

also please have a look at the ticket kubernetes/kubeadm#1378
which already tracked this problem. i think we agreed that if we decide the user to set these values, kubeadm should show warnings in the lines of:

klog.Warning("changing the KubeletConfiguration parameter X from the default value of Y is not recommended")

@neolit123

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

/priority backlog

@jfbai

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2019

/assign @rosti

@jfbai
thanks for the PR, please add a release note under Does this PR introduce a user-facing change?:

kubeadm: prevent overriding of certain kubelet security configuration parameters if the user wished to modify them

also please have a look at the ticket kubernetes/kubeadm#1378
which already tracked this problem. i think we agreed that if we decide the user to set these values, kubeadm should show warnings in the lines of:

klog.Warning("changing the KubeletConfiguration parameter X from the default value of Y is not recommended")

@neolit123 Thanks a lot, I will take a look at the tracked issue and update the PR as you recommended.

@jfbai jfbai force-pushed the jfbai:fix-kubeadm-kubelet-default branch from 2078f27 to 8305c54 Aug 26, 2019
@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Aug 26, 2019
@jfbai jfbai force-pushed the jfbai:fix-kubeadm-kubelet-default branch from 8305c54 to 74b05c6 Aug 26, 2019
@jfbai

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

@neolit123 I have updated the PR as you summarized in kubernetes/kubeadm#1378, PTAL, thanks a lot.

Copy link
Member

left a comment

Thanks @jfbai !

If we do this sort of thing for the kubelet component config, then let's do it for the kube-proxy's too.

@@ -240,6 +241,25 @@ const (
// This file should exist under KubeletRunDirectory
KubeletConfigurationFileName = "config.yaml"

// KubeletReadOnlyPort specifies the default insecure http server port
// 0 will disable insecure http server.
KubeletReadOnlyPort int32 = 0

This comment has been minimized.

Copy link
@rosti

rosti Aug 26, 2019

Member

These consts are currently used only by cmd/kubeadm/app/componentconfigs/defaults.go, hence, let's move them there.

This comment has been minimized.

Copy link
@jfbai

jfbai Aug 26, 2019

Author Contributor

Thanks, Fixed.

"k8s.io/klog"
)

func WarnDefaultComponentConfig(paramName string, componentName string, defaultValue interface{}) {

This comment has been minimized.

Copy link
@rosti

rosti Aug 26, 2019

Member

Again, this should be in cmd/kubeadm/app/componentconfigs/defaults.go and package private.

This comment has been minimized.

Copy link
@jfbai

jfbai Aug 26, 2019

Author Contributor

Thanks, Fixed.

externalkubeletcfg.Authentication.Anonymous.Enabled = utilpointer.BoolPtr(false)
if externalkubeletcfg.Authentication.X509.ClientCAFile == "" {
externalkubeletcfg.Authentication.X509.ClientCAFile = filepath.Join(internalcfg.CertificatesDir, constants.CACertName)
}

This comment has been minimized.

Copy link
@rosti

rosti Aug 26, 2019

Member

Keep the checks close one to another:

clientCAFile := filepath.Join(internalcfg.CertificatesDir, constants.CACertName)
if externalkubeletcfg.Authentication.X509.ClientCAFile == "" {
	externalkubeletcfg.Authentication.X509.ClientCAFile = clientCAFile
} else if externalkubeletcfg.Authentication.X509.ClientCAFile != clientCAFile {
	warnDefaultComponentConfig("client-ca-file", constants.Kubelet, clientCAFile)
}

This comment has been minimized.

Copy link
@jfbai

jfbai Aug 26, 2019

Author Contributor

Thanks, fixed.

@@ -195,16 +196,30 @@ func getAPIServerCommand(cfg *kubeadmapi.ClusterConfiguration, localAPIEndpoint
// Node,RBAC should be fixed in this order at the beginning
// AlwaysAllow and AlwaysDeny is ignored as they are only for testing
func getAuthzModes(authzModeExtraArgs string) string {

This comment has been minimized.

Copy link
@rosti

rosti Aug 26, 2019

Member

This looks like out of the scope of the PR. It may also have some security implications.

This comment has been minimized.

Copy link
@jfbai

jfbai Aug 26, 2019

Author Contributor

Sure, let's put this into another PR to discuss.

@jfbai jfbai force-pushed the jfbai:fix-kubeadm-kubelet-default branch from 74b05c6 to c0df205 Aug 26, 2019
@k8s-ci-robot k8s-ci-robot removed the size/L label Aug 26, 2019
@neolit123

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

/ok-to-test

@jfbai

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2019

/test pull-kubernetes-bazel-test

@jfbai

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2019

Hi @neolit123 @@rosti, test passed, could you please help review again and approve? Thanks a lot.

Copy link
Member

left a comment

Thanks @jfbai !

@@ -97,25 +117,54 @@ func DefaultKubeletConfiguration(internalcfg *kubeadmapi.ClusterConfiguration) {
// Enforce security-related kubelet options

This comment has been minimized.

Copy link
@rosti

rosti Aug 27, 2019

Member

We have another 3 ifs above this line, that need just a warning.

This comment has been minimized.

Copy link
@jfbai

jfbai Aug 27, 2019

Author Contributor

Thank @rosti, quick question about ClusterDNS,

ClusterDNS is an array, do we have to iterate (or sort and binary search) over this array to check if the recommended default value 10.96.0.10 is included? or just warn when ClusterDNS is not nil?

if iterate, this may introduce performance issue when user provide a large list.

So, could you please suggest a good solution?

This comment has been minimized.

Copy link
@rosti

rosti Aug 27, 2019

Member

We should really check if ClusterDNS has a single value that is the default DNS IP. The whole thing should be on the lines of:

dnsIP, err := constants.GetDNSIP(internalcfg.Networking.ServiceSubnet)
if err != nil {
	dnsIP = kubeadmapiv1beta2.DefaultClusterDNSIP
}

if externalkubeletcfg.ClusterDNS == nil {
	externalkubeletcfg.ClusterDNS = []string{dnsIP}
} else if len(externalkubeletcfg.ClusterDNS) != 1 || externalkubeletcfg.ClusterDNS[0] != dnsIP {
	warnDefaultComponentConfig(...)
}

Hence no iteration is necessary.

This comment has been minimized.

Copy link
@jfbai

jfbai Aug 27, 2019

Author Contributor

Thanks a lot, fixed.

KubeletAuthenticationWebhookEnabled = true

// KubeletHealthzBindAddress specifies the default healthz bind address
KubeletHealthzBindAddress = "127.0.0.1"
)

// DefaultKubeProxyConfiguration assigns default values for the kube-proxy ComponentConfig

This comment has been minimized.

Copy link
@rosti

rosti Aug 27, 2019

Member

Can you add the warning elses for DefaultKubeProxyConfiguration too?

This comment has been minimized.

Copy link
@jfbai

jfbai Aug 27, 2019

Author Contributor

Thanks a lot, fixed.


// KubeletReadOnlyPort specifies the default insecure http server port
// 0 will disable insecure http server.
KubeletReadOnlyPort int32 = 0

This comment has been minimized.

Copy link
@rosti

rosti Aug 27, 2019

Member

Let's make the consts package private. Under normal circumstances, they should not be used outside of this package.

This comment has been minimized.

Copy link
@jfbai

jfbai Aug 27, 2019

Author Contributor

Thanks a lot, fixed.

@jfbai jfbai force-pushed the jfbai:fix-kubeadm-kubelet-default branch from c0df205 to 63f8a9d Aug 27, 2019
@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Aug 27, 2019
@jfbai

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2019

/test pull-kubernetes-typecheck

@jfbai

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2019

/test pull-kubernetes-bazel-test

@jfbai

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2019

/test pull-kubernetes-e2e-gce-device-plugin-gpu

@jfbai

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

@rosti
rosti approved these changes Aug 28, 2019
Copy link
Member

left a comment

Thank you @jfbai !
/approve

@neolit123 @fabriziopandini I'll delegate LGTM to you

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jfbai, rosti

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


func warnDefaultComponentConfig(paramName string, componentName string, defaultValue interface{}) {
klog.Warningf("The recommended value for %s for %s is %v", paramName, componentName, defaultValue)
}

This comment has been minimized.

Copy link
@neolit123

neolit123 Aug 28, 2019

Member

i think it makes more sense to have the following utility function:

// warnDefaultComponentConfigValue prints a warning if the user modified a field in a certain
// CompomentConfig from the default recommended value in kubeadm.
func warnDefaultComponentConfigValue(componentConfigKind, paramName string, defaultValue, userValue interface{}) {
	klog.Warningf("The recommended value for %q in %q is: %v; the provided value is: %v",
		paramName, componentConfigKind, defaultValue, userValue)
}
  • added comment on top of the function.
  • renamed to add Value in function name.
  • added userValue - this will make it easier for the user to find their value.
  • use componentConfigKind; this means that the callers have to pass e.g. {externalproxycfg| externalkubeletcfg}.Kind
    this solves the potential problem if the user feeds a value from a certain Kind, but kubeadm supports multiple different Kinds for a certain component. still does not solve the problem if a certain filed name is available multiple times in different nestings under the same Kind.
  • adjusted warning to be on two lines.

This comment has been minimized.

Copy link
@jfbai

jfbai Aug 28, 2019

Author Contributor

Sure, I will fix this.

This comment has been minimized.

Copy link
@neolit123

neolit123 Aug 28, 2019

Member

one final missing item:

use componentConfigKind; this means that the callers have to pass e.g. {externalproxycfg| externalkubeletcfg}.Kind

so passing the above instead of constants.Kubelet and constants.KubeProxy.

[1]

This comment has been minimized.

Copy link
@jfbai

jfbai Aug 28, 2019

Author Contributor

Got it

@jfbai jfbai force-pushed the jfbai:fix-kubeadm-kubelet-default branch from 63f8a9d to ff7cd09 Aug 28, 2019
@@ -269,6 +269,8 @@ const (
Etcd = "etcd"
// KubeAPIServer defines variable used internally when referring to kube-apiserver component
KubeAPIServer = "kube-apiserver"
// Kubelet defines variable used internally when referring to kubelet component
Kubelet = "kubelet"

This comment has been minimized.

Copy link
@neolit123

neolit123 Aug 28, 2019

Member

not required if [1] is applied.

@jfbai jfbai force-pushed the jfbai:fix-kubeadm-kubelet-default branch from ff7cd09 to ccc4588 Aug 28, 2019
…ameters if the user wished to modify them.
@neolit123

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

thanks for the updates, i looked at the DIFF multiple times. hopefully i didn't miss anything.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 28, 2019
@jfbai

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2019

thanks for the updates, i looked at the DIFF multiple times. hopefully i didn't miss anything.
/lgtm

@neolit123 Thanks a lot. :)

@k8s-ci-robot k8s-ci-robot merged commit b3b4305 into kubernetes:master Aug 28, 2019
24 checks passed
24 checks passed
cla/linuxfoundation jfbai authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-conformance-kind-ipv6 Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.