add print-manifest flag to print addon manifests to STDOUT #109995

wangyysde · 2022-05-12T07:58:45Z

Signed-off-by: wangyysde net_use@bzhy.com

What type of PR is this?

What this PR does / why we need it:

It can be useful to allow kubeadm users to skip addons but still be able to obtain the addon manifests that kubeadm will apply for version X. This PR add print-manifest flag to support printing addon manifests to STDOUT.

Which issue(s) this PR fixes:

Fixes kubernetes/kubeadm#2681

Special notes for your reviewer:

Does this PR introduce a user-facing change?

kubeadm: add support for the flag "--print-manifest" to the addon phases "kube-proxy" and "coredns" of "kubeadm init phase addon". If this flag is used kubeadm will not apply a given addon and instead print to the terminal the API objects that will be applied.

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

NONE

neolit123 · 2022-05-16T14:04:53Z

cmd/kubeadm/app/cmd/init.go

+	flagSet.BoolVar(
+		&initOptions.printManifest, options.PrintManifest, initOptions.printManifest,
+		"Print the addon manifests to STDOUT instead of  install them.",
+	)


i think we should not add the flag to the main init or init phase addon, but only to the addon phases:

init phase addon coredns --print-manifest

init phase addon kube-proxy --print-manifest

Thanks. I will move the flag to the addon phases

Print the addon manifests to STDOUT instead of install them.
->
Print the addon manifest to STDOUT instead of installing them.

The plural form is correct here (with s / them) or we can remove "s" and have "it". We can adjust the message again once the rest of the code org is done.

cmd/kubeadm/app/cmd/init.go

cmd/kubeadm/app/phases/addons/dns/dns.go

cmd/kubeadm/app/phases/addons/proxy/proxy.go

cmd/kubeadm/app/phases/addons/dns/dns.go

cmd/kubeadm/app/phases/addons/proxy/proxy.go

neolit123

thanks for the PR @wangyysde
i've added a few comments.

basically what we want is the following:

add the new flag only to the addon phases them self, but not to the parent addon command or init.
Ensure* addon functions can accept the printManifest bool flag, but do the printing on an io.Writer and not always to STDOUT.

that seems like the easiest refactor.
please let me know if you have questions.

/cc @pacoxu
PTAL for reviews too if you have time.

neolit123 · 2022-05-16T14:19:16Z

/remove-area code-generation provider/gcp
/ kind feature
/priority backlog
/triage accepted
/remove-sig api-machinery cloud-provider

neolit123

@wangyysde thanks for continuing to work on this.
added a few minor comments, but it seems we are going to be able to merge this soon.

neolit123 · 2022-06-06T15:48:14Z

cmd/kubeadm/app/phases/addons/dns/dns.go

+		fmt.Fprintf(out, "%s\n", []byte(CoreDNSServiceAccount))
+		return nil
+	}
+
 	if err := createCoreDNSAddon(coreDNSDeploymentBytes, coreDNSServiceBytes, coreDNSConfigMapBytes, client); err != nil {
 		return err
 	}


let's use the fmt.Println(out, ...) instead of fmt.Println("[addons] Applied essential addon: CoreDNS") on the line below (154), given we now have the proper target passed to this function.

neolit123 · 2022-06-06T15:48:40Z

cmd/kubeadm/app/phases/addons/proxy/proxy.go

-	if err := CreateServiceAccount(client); err != nil {
-		return errors.Wrap(err, "error when creating kube-proxy service account")
-	}
+func EnsureProxyAddon(cfg *kubeadmapi.ClusterConfiguration, localEndpoint *kubeadmapi.APIEndpoint, client clientset.Interface, out io.Writer, printManifest bool) error {



let's remove this empty line (side cleanup)

neolit123 · 2022-06-06T15:49:09Z

cmd/kubeadm/app/phases/addons/proxy/proxy.go

 	}

 	fmt.Println("[addons] Applied essential addon: kube-proxy")
 	return nil
 }

-// CreateServiceAccount creates the necessary serviceaccounts that kubeadm uses/might use, if they don't already exist.
-func CreateServiceAccount(client clientset.Interface) error {
+// Create Sa, RBACRules or print manifests of them to out if printManifest is true


Suggested change

// Create Sa, RBACRules or print manifests of them to out if printManifest is true

// Create SA, RBACRules or print manifests of them to out if printManifest is true

neolit123 · 2022-06-06T15:49:41Z

cmd/kubeadm/app/phases/addons/proxy/proxy.go

-// CreateServiceAccount creates the necessary serviceaccounts that kubeadm uses/might use, if they don't already exist.
-func CreateServiceAccount(client clientset.Interface) error {
+// Create Sa, RBACRules or print manifests of them to out if printManifest is true
+func CreateSARBACRules(cmByte []byte, dsByte []byte, client clientset.Interface, out io.Writer, printManifest bool) error {


can we make this function private. also maybe we can call it printOrCreateKubeProxyObjects()?

I have renamed the function to printOrCreateKubeProxyObjects

cmd/kubeadm/app/phases/addons/proxy/proxy.go

wangyysde · 2022-06-07T06:21:41Z

I have modified this PR. @neolit123 @pacoxu Could you review it again? Thks!

neolit123

/lgtm
/approve

good job, thanks!

k8s-ci-robot · 2022-06-07T14:14:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123, wangyysde

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]

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

neolit123 · 2022-06-07T14:16:05Z

tide — Not mergeable. PR has a merge conflict.

looks like some PRs just merged and this needs a quick rebase.

neolit123 · 2022-06-07T14:34:21Z

so i tested it locally and there are some minor problems, but overall this is working as expected.
/hold

1. the client should not be needed when we print manifests

kubeadm init phase addon kube-proxy --print-manifest
error execution phase addon/kube-proxy: failed to load admin kubeconfig: open /etc/kubernetes/admin.conf: no such file or directory
To see the stack trace of this error execute with --v=5 or higher

kubeadm init phase addon coredns --print-manifest
error execution phase addon/coredns: failed to load admin kubeconfig: open /etc/kubernetes/admin.conf: no such file or directory
To see the stack trace of this error execute with --v=5 or higher

this can be avoided with a simple fix:

diff --git a/cmd/kubeadm/app/cmd/phases/init/addons.go b/cmd/kubeadm/app/cmd/phases/init/addons.go
index c31eb779fd1..47fa070ae69 100644
--- a/cmd/kubeadm/app/cmd/phases/init/addons.go
+++ b/cmd/kubeadm/app/cmd/phases/init/addons.go
@@ -90,9 +90,13 @@ func getInitData(c workflow.RunData) (*kubeadmapi.InitConfiguration, clientset.I
                return nil, nil, nil, errors.New("addon phase invoked with an invalid data struct")
        }
        cfg := data.Cfg()
-       client, err := data.Client()
-       if err != nil {
-               return nil, nil, nil, err
+       var client clientset.Interface
+       var err error
+       if !printManifest {
+               client, err = data.Client()
+               if err != nil {
+                       return nil, nil, nil, err
+               }
        }
 
        out := data.OutputWriter()

2. even if we pass --print-manifest the lines:

[addons] Applied essential addon: kube-proxy / coredns

are printed. we should only print them if we are actually applying the addons.

3. when the manifests are printed there are extra new lines around the ---. can we avoid these somehow?

example - currently it's:

...
  verbs:
  - get

---
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
  creationTimestamp: null
  name: kube-proxy
  namespace: kube-system
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: Role
  name: kube-proxy
subjects:
- kind: Group
  name: system:bootstrappers:kubeadm:default-node-token

---

kind: ConfigMap
apiVersion: v1
metadata:
  name: kube-proxy
...

should be:

...
  verbs:
  - get
---
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
  creationTimestamp: null
  name: kube-proxy
  namespace: kube-system
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: Role
  name: kube-proxy
subjects:
- kind: Group
  name: system:bootstrappers:kubeadm:default-node-token
---
kind: ConfigMap
apiVersion: v1
metadata:
  name: kube-proxy
...

the idea is to have:

---
manifest1
---
manifest2

instead of:

---
manifest1


---

manifest2

Signed-off-by: wangyysde <net_use@bzhy.com>

wangyysde · 2022-06-08T07:08:04Z

/test pull-kubernetes-e2e-kind

wangyysde · 2022-06-08T08:50:05Z

@neolit123 @pacoxu
I have modified this PR. Could you review it again? Thanks.

the client should not be needed when we print manifests

Thks. I have modified it.

even if we pass --print-manifest the lines:

This bug has be fixed. thks.

when the manifests are printed there are extra new lines around the ---. can we avoid these somehow?

New lines have been omitted.

pacoxu · 2022-06-08T10:31:11Z

/lgtm
👍

neolit123 · 2022-06-08T14:10:02Z

/hold cancel
tested locally and works fine from what i saw.

thanks again.

neolit123 · 2022-06-08T15:15:59Z

/retest

k8s-ci-robot requested review from neolit123 and SataQiu May 12, 2022 07:59

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 May 12, 2022

wangyysde force-pushed the fix_issue_2681 branch from dd43d6b to e8ba36f Compare May 12, 2022 13:54

k8s-ci-robot added area/provider/gcp Issues or PRs related to gcp provider sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. labels May 12, 2022

wangyysde force-pushed the fix_issue_2681 branch from e8ba36f to 9edb651 Compare May 13, 2022 06:22

k8s-ci-robot added area/code-generation sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels May 13, 2022

wangyysde force-pushed the fix_issue_2681 branch 2 times, most recently from 5435e3c to 63825ed Compare May 13, 2022 09:26

neolit123 reviewed May 16, 2022

View reviewed changes

cmd/kubeadm/app/cmd/init.go Outdated Show resolved Hide resolved

neolit123 reviewed May 16, 2022

View reviewed changes

cmd/kubeadm/app/phases/addons/dns/dns.go Outdated Show resolved Hide resolved

neolit123 reviewed May 16, 2022

View reviewed changes

cmd/kubeadm/app/phases/addons/proxy/proxy.go Outdated Show resolved Hide resolved

neolit123 reviewed May 16, 2022

View reviewed changes

cmd/kubeadm/app/phases/addons/dns/dns.go Show resolved Hide resolved

neolit123 reviewed May 16, 2022

View reviewed changes

cmd/kubeadm/app/phases/addons/proxy/proxy.go Outdated Show resolved Hide resolved

neolit123 reviewed May 16, 2022

View reviewed changes

k8s-ci-robot requested a review from pacoxu May 16, 2022 14:18

k8s-ci-robot added the priority/backlog Higher priority than priority/awaiting-more-evidence. label May 16, 2022

wangyysde requested review from neolit123 and pacoxu June 6, 2022 02:01

neolit123 reviewed Jun 6, 2022

View reviewed changes

cmd/kubeadm/app/phases/addons/proxy/proxy.go Outdated Show resolved Hide resolved

wangyysde force-pushed the fix_issue_2681 branch from 0bae06d to a4d75c0 Compare June 7, 2022 02:56

wangyysde requested a review from neolit123 June 7, 2022 06:20

neolit123 reviewed Jun 7, 2022

View reviewed changes

k8s-ci-robot assigned neolit123 Jun 7, 2022

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 7, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 7, 2022

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

add print-manifest flag to print addon manifests to STDOUT

e863ebb

Signed-off-by: wangyysde <net_use@bzhy.com>

wangyysde force-pushed the fix_issue_2681 branch from a4d75c0 to e863ebb Compare June 8, 2022 05:46

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 8, 2022

wangyysde requested a review from neolit123 June 8, 2022 08:37

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 8, 2022

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

k8s-ci-robot merged commit 0985c47 into kubernetes:master Jun 8, 2022

k8s-ci-robot added this to the v1.25 milestone Jun 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add print-manifest flag to print addon manifests to STDOUT #109995

add print-manifest flag to print addon manifests to STDOUT #109995

wangyysde commented May 12, 2022 •

edited by k8s-ci-robot

neolit123 May 16, 2022

wangyysde May 17, 2022

pacoxu May 17, 2022

neolit123 May 17, 2022 •

edited

neolit123 left a comment

neolit123 commented May 16, 2022

neolit123 left a comment

neolit123 Jun 6, 2022 •

edited

wangyysde Jun 7, 2022

neolit123 Jun 6, 2022

wangyysde Jun 7, 2022

neolit123 Jun 6, 2022

wangyysde Jun 7, 2022

neolit123 Jun 6, 2022 •

edited

wangyysde Jun 7, 2022

wangyysde commented Jun 7, 2022

neolit123 left a comment

k8s-ci-robot commented Jun 7, 2022

neolit123 commented Jun 7, 2022

neolit123 commented Jun 7, 2022 •

edited

wangyysde commented Jun 8, 2022

wangyysde commented Jun 8, 2022

pacoxu commented Jun 8, 2022

neolit123 commented Jun 8, 2022

neolit123 commented Jun 8, 2022

	// Create Sa, RBACRules or print manifests of them to out if printManifest is true
	// Create SA, RBACRules or print manifests of them to out if printManifest is true

add print-manifest flag to print addon manifests to STDOUT #109995

add print-manifest flag to print addon manifests to STDOUT #109995

Conversation

wangyysde commented May 12, 2022 • edited by k8s-ci-robot

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 May 17, 2022 • edited

Choose a reason for hiding this comment

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 commented May 16, 2022

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 Jun 6, 2022 • edited

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 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangyysde commented Jun 7, 2022

neolit123 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 7, 2022

neolit123 commented Jun 7, 2022

neolit123 commented Jun 7, 2022 • edited

wangyysde commented Jun 8, 2022

wangyysde commented Jun 8, 2022

pacoxu commented Jun 8, 2022

neolit123 commented Jun 8, 2022

neolit123 commented Jun 8, 2022

wangyysde commented May 12, 2022 •

edited by k8s-ci-robot

neolit123 May 17, 2022 •

edited

neolit123 Jun 6, 2022 •

edited

neolit123 Jun 6, 2022 •

edited

neolit123 commented Jun 7, 2022 •

edited