we should have override / de-dup tests

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

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.

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.

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!

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

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.

+1, this is just a testcase.

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.

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

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:

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

2. 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!