kubeadm: Support custom env in control plane component #118867

chendave · 2023-06-26T11:02:35Z

/kind feature

What type of PR is this?

What this PR does / why we need it:

There are some cases,  the custom environments such as `GOMAXPROCS` could be consumed by the control plane component.
This change supports the configuration of custom environments in control plane components with
the `ClusterConfiguration.Etcd.Local.ExtraEnvs`, `cfg.APIServer.ExtraEnvs`, `cfg.ControllerManager.ExtraEnvs`, `cfg.Scheduler.ExtraEnvs`.

Fixed some typos in the v1beta4/doc.go

Which issue(s) this PR fixes:

xref: kubernetes/kubeadm#2890

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

chendave · 2023-06-26T11:08:25Z

cc other members signed with the v1beta4.

/cc @neolit123 @ruquanzhao

I tested this against v1beta3, and it works, set the GOMAXPROCS in the config file and apiserver show me that env is set inside the container.

I0626 09:17:44.842730 1 server.go:167] "Golang settings" GOGC="" GOMAXPROCS="10" GOTRACEBACK=""

neolit123

thanks @chendave
added some comments

cmd/kubeadm/app/apis/kubeadm/types.go

cmd/kubeadm/app/apis/kubeadm/v1beta3/conversion.go

cmd/kubeadm/app/apis/kubeadm/types.go

cmd/kubeadm/app/apis/kubeadm/v1beta4/types.go

neolit123 · 2023-06-26T11:41:46Z

cmd/kubeadm/app/phases/controlplane/manifests_test.go

@@ -51,6 +53,9 @@ func TestGetStaticPodSpecs(t *testing.T) {
 	// Creates a Cluster Configuration
 	cfg := &kubeadmapi.ClusterConfiguration{
 		KubernetesVersion: "v1.9.0",
+		Scheduler: kubeadmapi.ControlPlaneComponent{ExtraEnvs: []v1.EnvVar{
+			{Name: "Foo", Value: "Bar"},


we should have override / de-dup tests

this will be tested in cmd/kubeadm/app/util/env package.

cmd/kubeadm/app/phases/controlplane/manifests_test.go

neolit123 · 2023-06-26T11:44:11Z

cmd/kubeadm/app/phases/controlplane/manifests_test.go

@@ -28,8 +28,10 @@ import (
 	"strings"
 	"testing"

+	"github.com/google/go-cmp/cmp"


we already import reflect. maybe we can use it instead of the extra import of cmp

Actually, I think we should move from the reflect.DeepEqual to cmp.Diff,
here is the context,

https://pkg.go.dev/github.com/google/go-cmp/cmp

Use go-cmp instead of reflect.DeepEqual stretchr/testify#535

I remember there is problem to compare two struct with embed struct when compare with reflect.DeepEqual

reflect should be fine for slices, but i don't insist on not using cmp

Gotcha, I will switch to reflect in the next iteration.

@neolit123 have switched to reflect.

cmd/kubeadm/app/phases/etcd/local_test.go

cmd/kubeadm/app/apis/kubeadm/types.go

cmd/kubeadm/app/phases/controlplane/manifests.go

neolit123 · 2023-06-27T09:24:47Z

cmd/kubeadm/app/util/env.go

@@ -41,3 +41,17 @@ func GetProxyEnvVars() []v1.EnvVar {
 	}
 	return envs
 }
+
+// MergedEnv merge the extraEnvs with proxyEnv, extraEnvs take precedence over the proxyEnv.


func should be called with a verb. MergeEnv

it should be a generic, variadic argument function that takes a slice if env slices and makes the last slices override / take precedence.

Done, pls let me know if the current version address your comment.

chendave · 2023-06-28T07:35:10Z

cmd/kubeadm/app/phases/controlplane/manifests_test.go

-
+				if tc.env != nil {
+					envs := []v1.EnvVar{}
+					// Just compare the env that testcase set as the proxy env might be set an arbitrary value on the test node.


we must remove the "_proxy" env here, as the test node has the "no_proxy"env configured.

this sounds like it needs refactor to not have tests that depend on the host env. i am talking without checking, but the host env could be passed only on actual runs / outside of tests.

I am not sure if am following here, correct me pls, how the host env is set seems not matter here as we just need to make sure the env set in the clusterCfg is parsed correctly.

you've mentioned "test node", which i understood as the host where the unit test is run.

we must remove the "_proxy" env here, as the test node has the "no_proxy"env configured.

where does the no_proxy env come from?

@neolit123 pls see the test result without the tweak here: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/118867/pull-kubernetes-unit/1673936678231740416

seems to me we should refactor our test / tested function here not to get:

[{no_proxy 127.0.0.1,localhost nil}

if i am not mistaken these come from the host?

yes, this comes from the host. I will leave a TODO here so that we can revisit it later if we have a better way to tackle it.

i think we can fix it here as it's relatively simple.

make GetStaticPodSpecs() accept a []v1.EnvVar called proxyEnvs

if the slice is passed as nil populate it with GetProxyEnvVars() inside GetStaticPodSpecs() and use it in manifest construction

if the slice is non-nil use its value instead of GetProxyEnvVars()

then in this unit test just pass an empty slice

does it make sense to you?

Done, thanks for the guidance!

chendave

@pacoxu I replied all your comments, pls let me know is that make sense to you, thanks!

chendave · 2023-07-03T09:25:01Z

cmd/kubeadm/app/apis/kubeadm/v1beta4/doc.go

 //   - TODO https://github.com/kubernetes/kubeadm/issues/2890
+//   - Support custom environment variables in control plane components under `ClusterConfiguration`.
+//     Use `APIServer.ExtraEnvs`, `ControllerManager.ExtraEnvs`, `Scheduler.ExtraEnvs`,  `Etcd.Local.ExtraEnvs`.


look like to me addons are not the resource maintained by kubeadm. @neolit123 @SataQiu anything to comment here?

chendave · 2023-07-03T09:38:30Z

cmd/kubeadm/app/phases/controlplane/manifests.go

 	// Get the required hostpath mounts
 	mounts := getHostPathVolumesForTheControlPlane(cfg)
+	if proxyEnvs == nil {
+		proxyEnvs = kubeadmutil.GetProxyEnvVars()
+	}


this was discussed here: #118867 (comment), I am fine with either of them but a little in favor of the current approach (we cannot guarantee extraEnv is always the last env, and actually it's a slice and we cannot inspect easily how many envs the user sets), the original approach is filter the proxy env out, @neolit123 suggest me to remove the dep over the host.

chendave · 2023-07-03T09:39:05Z

cmd/kubeadm/app/util/env_test.go

+	}{
+		{
+			name: "normal case without duplicated env",
+			proxyEnv: []v1.EnvVar{


+1, this is just a testcase.

k8s-ci-robot · 2023-07-03T09:58:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chendave, neolit123, pacoxu

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:

~~cmd/kubeadm/OWNERS~~ [neolit123,pacoxu]

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

pacoxu · 2023-07-03T10:44:18Z

cmd/kubeadm/app/apis/kubeadm/types.go

@@ -262,6 +267,10 @@ type LocalEtcd struct {
 	// command line except without leading dash(es).
 	ExtraArgs map[string]string

+	// ExtraEnvs is an extra set of environment variables to pass to the control plane component.
+	// +optional
+	ExtraEnvs []v1.EnvVar


Comments are not updated. https://github.com/kubernetes/kubernetes/pull/118867/files#diff-a919b4d8ff1a3b03bb5691a0725e0683ff5e3fa4e12c4f81690ed46b156816d7R290 too,

Overall LGTM.

@pacoxu this is etcd, there is no such proxy env passed to etcd compared with ControlPlaneComponent, and we haven't overridden anything like what we do for ControlPlaneComponent.

https://github.com/kubernetes/kubernetes/pull/118867/files#diff-509876ddb9971f8787c885617ebaabe5da81755c5f1cddc1e2877af0d47928b2R227

/hold

please add it everywhere. doesn't matter if we skip proxy.

actually why are we not adding proxy envs to etcd? isn't that a bug?

Generally etcd will not access outside, only peers should be accessed by etcd. And only local apiserver can access etcd.

For apiserver, it may access webhooks. Scheduler also can have webhook.

hm, maybe a peer would not be able to reach a peer because of proxy.

there is no such a request for passing the proxy env so far, we can add it when there is a clear usecase, and this is not a API change, it will be easy to address then.

added the comment for all the occurrences! @pacoxu @neolit123 looking for LGTM again.

This sounds like a corner case that is nice to support😄. Either is OK with me for this point.

chendave · 2023-07-03T10:44:57Z

@pacoxu anything more to add?

pacoxu · 2023-07-03T11:12:21Z

/lgtm

k8s-ci-robot · 2023-07-03T11:12:27Z

LGTM label has been added.

Git tree hash: 8fb2cfb6b573ae8592b2bd2197ea3903e389444e

Signed-off-by: Dave Chen <dave.chen@arm.com>

…nent Signed-off-by: Dave Chen <dave.chen@arm.com>

neolit123 · 2023-07-04T05:01:11Z

/lgtm
/hold cancel

k8s-ci-robot · 2023-07-04T05:01:18Z

LGTM label has been added.

Git tree hash: 6c132e46b561d7c7442d9d2acb5e1b37749c0fc4

pacoxu · 2023-09-11T05:36:06Z

cmd/kubeadm/app/apis/kubeadm/v1beta4/types.go

+	// ExtraEnvs is an extra set of environment variables to pass to the control plane component.
+	// Environment variables passed using ExtraEnvs will override any existing environment variables, or *_proxy environment variables that kubeadm adds by default.
+	// +optional
+	ExtraEnvs []corev1.EnvVar `json:"extraEnvs,omitempty"`


This caused the import of apis/core/v1 in generated.defaults below.

Should we avoid that or how to avoid the defaulting logic?

we already had this import:
https://github.com/chendave/kubernetes/blob/c68a6b07456301f6a8102c01e99ba8a53bc9bf4f/cmd/kubeadm/app/apis/kubeadm/v1beta4/types.go#L20

https://github.com/chendave/kubernetes/blob/c68a6b07456301f6a8102c01e99ba8a53bc9bf4f/cmd/kubeadm/app/apis/kubeadm/v1beta4/types.go#L226
https://github.com/chendave/kubernetes/blob/c68a6b07456301f6a8102c01e99ba8a53bc9bf4f/cmd/kubeadm/app/apis/kubeadm/v1beta4/types.go#L244
https://github.com/chendave/kubernetes/blob/c68a6b07456301f6a8102c01e99ba8a53bc9bf4f/cmd/kubeadm/app/apis/kubeadm/v1beta4/types.go#L441

So this not results to the import in generated_default.go?

https://github.com/kubernetes/kubernetes/pull/118867/files#r1321045615

pacoxu · 2023-09-11T06:23:22Z

cmd/kubeadm/app/apis/kubeadm/v1beta4/zz_generated.defaults.go

@@ -23,6 +23,7 @@ package v1beta4

 import (
 	runtime "k8s.io/apimachinery/pkg/runtime"
+	v1 "k8s.io/kubernetes/pkg/apis/core/v1"


As I tested, this is the key import that is related.

@pacoxu as mentioned the kubernetes/pkg import is not allowed in kubeadm!

@chendave we must find out how to solve it. maybe it's a generator bug.
if we cannot solve it we can try avoiding the defaulting []...{}

if we are out of ideas, we must revert the PR.

We must drop this import first.
In the past, we have made a lot of efforts to do this, and we cannot go back.
But I'm confused, why didn't the .import-restrictions prevent PRs from being merged?

perhaps the import-boss tool is not running or buggy.
it can be tested with a simple PR for cmd/kubeadm or other packages that have an .import-restrictions file.

EDIT: i do remember a recent contributor PR that failed with an import restriction problem.

I tested it locally, and it seems that the generated code(zz_generated.defaults.go) won't be checked...

It makes sense. So generated code are skipped for that import restrictions.

i think code-gen here makes the decision poorly. it imports v1 "k8s.io/kubernetes/pkg/apis/core/v1" so that it can call the defaulter v1.SetDefaults_ObjectFieldSelector.

we have two options:

play around with our types to make code-gen not import defaulters.
seems to boil down to if a.ValueFrom != nil {
an alias might trick the defaulter too type EnvVars []corev1.EnvVar...has to be tested.

ask #sig-api-machinery if we can patch code-gen to not import the defaulters under certain conditions.
maybe there is already +foo directive for it, like +optional.

2. maybe there is already +foo directive for it, like +optional.

~/go/src/k8s.io/kubernetes$ git diff diff --git a/cmd/kubeadm/app/apis/kubeadm/v1beta4/defaults.go b/cmd/kubeadm/app/apis/kubeadm/v1beta4/defaults.go index 314be0b30c7..38819c814f4 100644 --- a/cmd/kubeadm/app/apis/kubeadm/v1beta4/defaults.go +++ b/cmd/kubeadm/app/apis/kubeadm/v1beta4/defaults.go @@ -74,6 +74,7 @@ func SetDefaults_InitConfiguration(obj *InitConfiguration) { } // SetDefaults_ClusterConfiguration assigns default values for the ClusterConfiguration +// +k8s:defaulter-gen=covers func SetDefaults_ClusterConfiguration(obj *ClusterConfiguration) { if obj.KubernetesVersion == "" { obj.KubernetesVersion = DefaultKubernetesVersion

results in no import of k8s.io/kubernetes/pkg/api, after running ./hack/update-codegen.sh

// An existing defaulter method (SetDefaults_TYPE) can provide the
// comment tag:
//
// // +k8s:defaulter-gen=covers
//
// to indicate that the defaulter does not or should not call any nested
// defaulters.

https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/code-generator/cmd/defaulter-gen/main.go#L35-L41

result diff:

diff --git a/cmd/kubeadm/app/apis/kubeadm/v1beta4/zz_generated.defaults.go b/cmd/kubeadm/app/apis/kubeadm/v1beta4/zz_generated.defaults.go index b36b0f0f952..d2409b0de6a 100644 --- a/cmd/kubeadm/app/apis/kubeadm/v1beta4/zz_generated.defaults.go +++ b/cmd/kubeadm/app/apis/kubeadm/v1beta4/zz_generated.defaults.go @@ -23,7 +23,6 @@ package v1beta4 import ( runtime "k8s.io/apimachinery/pkg/runtime" - v1 "k8s.io/kubernetes/pkg/apis/core/v1" ) // RegisterDefaults adds defaulters functions to the given scheme. @@ -39,41 +38,6 @@ func RegisterDefaults(scheme *runtime.Scheme) error { func SetObjectDefaults_ClusterConfiguration(in *ClusterConfiguration) { SetDefaults_ClusterConfiguration(in) - if in.Etcd.Local != nil { - for i := range in.Etcd.Local.ExtraEnvs { - a := &in.Etcd.Local.ExtraEnvs[i] - if a.ValueFrom != nil { - if a.ValueFrom.FieldRef != nil { - v1.SetDefaults_ObjectFieldSelector(a.ValueFrom.FieldRef) - } - } - } - } - SetDefaults_APIServer(&in.APIServer) - for i := range in.APIServer.ControlPlaneComponent.ExtraEnvs { - a := &in.APIServer.ControlPlaneComponent.ExtraEnvs[i] - if a.ValueFrom != nil { - if a.ValueFrom.FieldRef != nil { - v1.SetDefaults_ObjectFieldSelector(a.ValueFrom.FieldRef) - } - } - } - for i := range in.ControllerManager.ExtraEnvs { - a := &in.ControllerManager.ExtraEnvs[i] - if a.ValueFrom != nil { - if a.ValueFrom.FieldRef != nil { - v1.SetDefaults_ObjectFieldSelector(a.ValueFrom.FieldRef) - } - } - }

questions:

would that be a problem for us in the future? probably no, because if we want a defaulter for some field in ClusterConfiguration we can explicitly call it in SetDefaults_ClusterConfiguration()

can this missing defaulting for a.ValueFrom.FieldRef be a problem? i think no. it just sets that field selector to "v1", but technically the kube-apiserver must also do this defaulting when the mirror Pod (of the static pod) is created, right?

okay, I like this approach, it's more neat to look!

k8s-ci-robot requested review from pacoxu and SataQiu June 26, 2023 11:03

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 26, 2023

chendave mentioned this pull request Jun 26, 2023

release v1beta4 kubernetes/kubeadm#2890

Closed

26 tasks

k8s-ci-robot requested review from neolit123 and ruquanzhao June 26, 2023 11:08

neolit123 reviewed Jun 26, 2023

View reviewed changes

SataQiu reviewed Jun 26, 2023

View reviewed changes

cmd/kubeadm/app/phases/controlplane/manifests.go Outdated Show resolved Hide resolved

SataQiu reviewed Jun 26, 2023

View reviewed changes

cmd/kubeadm/app/phases/controlplane/manifests.go Outdated Show resolved Hide resolved

chendave force-pushed the env_custom branch from 893c790 to d43cf68 Compare June 27, 2023 08:46

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 27, 2023

chendave force-pushed the env_custom branch 2 times, most recently from 5df01d7 to 4b22b8b Compare June 27, 2023 08:54

neolit123 reviewed Jun 27, 2023

View reviewed changes

chendave force-pushed the env_custom branch 2 times, most recently from 8bf3cb8 to 1f56b3d Compare June 28, 2023 07:30

chendave commented Jun 28, 2023

View reviewed changes

chendave commented Jul 3, 2023

View reviewed changes

pacoxu approved these changes Jul 3, 2023

View reviewed changes

chendave force-pushed the env_custom branch from 4ddb52b to 7a6e1a7 Compare July 3, 2023 10:29

pacoxu reviewed Jul 3, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 3, 2023

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 3, 2023

chendave added 2 commits July 4, 2023 09:57

kubeadm: Support custom env in control plane component

80b035a

Signed-off-by: Dave Chen <dave.chen@arm.com>

kubeadm: fix fuzzing for ExtraEnvs defined in the control plane compo…

c68a6b0

…nent Signed-off-by: Dave Chen <dave.chen@arm.com>

chendave force-pushed the env_custom branch from 7a6e1a7 to c68a6b0 Compare July 4, 2023 02:01

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 4, 2023

k8s-ci-robot requested review from pacoxu and SataQiu July 4, 2023 02:01

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 4, 2023

k8s-ci-robot assigned neolit123 Jul 4, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 4, 2023

k8s-ci-robot merged commit 746b88c into kubernetes:master Jul 4, 2023

k8s-ci-robot added this to the v1.28 milestone Jul 4, 2023

chendave deleted the env_custom branch July 4, 2023 05:09

chendave mentioned this pull request Jul 5, 2023

Support custom env in kubeadm ControlPlaneComponent kubernetes/kubeadm#2845

Closed

neolit123 mentioned this pull request Sep 9, 2023

upgrade-1-28-latest and upgrade-addons-before-controlplane-1-28-latest failed kubernetes/kubeadm#2927

Closed

10 tasks

pacoxu mentioned this pull request Sep 11, 2023

Revert #118867: for default unmarshal behavior chang #120554

Closed

pacoxu reviewed Sep 11, 2023

View reviewed changes

chendave mentioned this pull request Nov 8, 2023

kubeadm upgrade from v1.28.0 to v1.28.3 fails kubernetes/kubeadm#2957

Closed

kubeadm: Support custom env in control plane component #118867

kubeadm: Support custom env in control plane component #118867

Conversation

chendave commented Jun 26, 2023 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

chendave commented Jun 26, 2023

neolit123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 Jun 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 Jul 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chendave left a comment

Choose a reason for hiding this comment

chendave Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 3, 2023

Choose a reason for hiding this comment

chendave Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pacoxu Jul 4, 2023 • edited Loading

Choose a reason for hiding this comment

chendave commented Jul 3, 2023

pacoxu commented Jul 3, 2023

k8s-ci-robot commented Jul 3, 2023

neolit123 commented Jul 4, 2023

k8s-ci-robot commented Jul 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

neolit123 Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

neolit123 Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chendave commented Jun 26, 2023 •

edited

Loading

neolit123 Jun 29, 2023 •

edited

Loading

neolit123 Jul 2, 2023 •

edited

Loading

chendave Jul 3, 2023 •

edited

Loading

chendave Jul 3, 2023 •

edited

Loading

pacoxu Jul 4, 2023 •

edited

Loading

neolit123 Sep 11, 2023 •

edited

Loading

neolit123 Sep 11, 2023 •

edited

Loading

neolit123 Sep 11, 2023 •

edited

Loading

neolit123 Sep 11, 2023 •

edited

Loading