Refactor addons into multiple packages #50214

andrewrynhard · 2017-08-06T22:58:51Z

What this PR does / why we need it:
kubernetes/kubeadm#348

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
kubernetes/kubeadm#348

k8s-ci-robot · 2017-08-06T22:58:53Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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. I understand the commands that are listed here.

k8s-ci-robot · 2017-08-06T22:58:59Z

Hi @andrewrynhard. 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.

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. I understand the commands that are listed here.

andrewrynhard · 2017-08-06T23:00:34Z

cmd/kubeadm/app/cmd/init.go

@@ -283,7 +284,11 @@ func (i *Init) Run(out io.Writer) error {
 		return err
 	}

-	if err := addonsphase.CreateEssentialAddons(i.cfg, client); err != nil {
+	if err := dnsaddonphase.CreateEsentialDNSAddon(i.cfg, client); err != nil {


Wasn't certain if we wanted to keep this as just addonsphase. I can change if so.

This is fine but can just be EnsureDNSAddon()

andrewrynhard · 2017-08-06T23:00:41Z

cmd/kubeadm/app/cmd/init.go

+		return err
+	}
+
+	if err := proxyaddonphase.CreateEssentialProxyAddon(i.cfg, client); err != nil {


Wasn't certain if we wanted to keep this as just addonsphase. I can change if so.

proxyaddonphase is fine but maybe EnsureProxyAddon()

andrewrynhard · 2017-08-06T23:02:20Z

@luxas I have a few things I can clean up. Still a WIP.

luxas

I know this is WIP, but left a couple of comments for guidance.

What I yet want to see here is moving the ServiceAccount generation from the apiconfig phase to each addon pkg respectively. The same goes for the kubeadm:node-proxier ClusterRoleBinding.

Sounds good to you? Thanks!

luxas · 2017-08-07T07:56:35Z

cmd/kubeadm/app/cmd/init.go

@@ -283,7 +284,11 @@ func (i *Init) Run(out io.Writer) error {
 		return err
 	}

-	if err := addonsphase.CreateEssentialAddons(i.cfg, client); err != nil {
+	if err := dnsaddonphase.CreateEsentialDNSAddon(i.cfg, client); err != nil {


This is fine but can just be EnsureDNSAddon()

luxas · 2017-08-07T07:56:59Z

cmd/kubeadm/app/cmd/init.go

+		return err
+	}
+
+	if err := proxyaddonphase.CreateEssentialProxyAddon(i.cfg, client); err != nil {


proxyaddonphase is fine but maybe EnsureProxyAddon()

luxas · 2017-08-07T07:57:24Z

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

-	}
-
+// CreateEsentialDNSAddon creates the kube-dns addon
+func CreateEsentialDNSAddon(cfg *kubeadmapi.MasterConfiguration, client clientset.Interface) error {


EnsureDNSAddon

luxas · 2017-08-07T07:57:44Z

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

-	fmt.Println("[addons] Applied essential addon: kube-proxy")
-
-	err = CreateKubeDNSAddon(dnsDeploymentBytes, dnsServiceBytes, client)
+	err = createKubeDNSAddon(dnsDeploymentBytes, dnsServiceBytes, client)


...IfNotExists

And inline this in the if-clause below

luxas · 2017-08-07T07:58:42Z

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

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

-package addons
+package dns


KubeProxy* shouldn't be left here, right?

luxas · 2017-08-07T07:59:05Z

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

+		expected bool
+	}{
+		{
+			manifest: KubeProxyConfigMap,


KubeProxy* shouldn't be here, right?

luxas · 2017-08-07T07:59:33Z

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

+          path: /run/xtables.lock
+`
+
+	KubeDNSVersion = "1.14.4"


KubeDNS* shouldn't be in this file

luxas · 2017-08-07T08:00:09Z

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

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

-package addons
+package proxy


Move over TestCompileManifests with the right test cases.

luxas · 2017-08-07T08:00:43Z

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

+		return fmt.Errorf("error when parsing kube-proxy daemonset template: %v", err)
+	}
+
+	err = createKubeProxyAddon(proxyConfigMapBytes, proxyDaemonSetBytes, client)


move into if clause

luxas · 2017-08-07T08:05:48Z

/ok-to-test

luxas

Thanks a bunch! Only one primary comment; and some small nits to fix:

From golint:

W0814 06:27:07.535] cmd/kubeadm/app/phases/addons/dns/manifests.go:20:2: exported const KubeDNSVersion should have comment (or a comment on this block) or be unexported
W0814 06:27:07.535] cmd/kubeadm/app/phases/addons/proxy/manifests.go:20:2: exported const KubeProxyConfigMap should have comment (or a comment on this block) or be unexported

From unit/bazel:

cmd/kubeadm/app/phases/apiconfig/clusterroles_test.go:60: undefined: CreateServiceAccounts

luxas · 2017-08-14T11:41:37Z

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

-	if err := kuberuntime.DecodeInto(api.Codecs.UniversalDecoder(), daemonSetbytes, kubeproxyDaemonSet); err != nil {
-		return fmt.Errorf("unable to decode kube-proxy daemonset %v", err)
+// CreateServiceAccount creates the necessary serviceaccounts that kubeadm uses/might use, if they don't already exist.
+func CreateServiceAccount(client clientset.Interface) error {


I don't think this needs to be exposed separately.
Well, if we do that it's fine by me, but at least EnsureDNSAddon should call it.
(EnsureDNSAddon should not have dependencies)

luxas · 2017-08-14T11:43:25Z

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

+
+const (
+	// KubeProxyClusterRoleName sets the name for the kube-proxy ClusterRole
+	KubeProxyClusterRoleName = "system:node-proxier"


nit: Add // TODO: This k8s-generic, well-known constant should be fetchable from an other source, not be in this package

luxas · 2017-08-14T11:44:04Z

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

+)
+
+// EnsureProxyAddon creates the kube-proxy and kube-dns addons
+func EnsureProxyAddon(cfg *kubeadmapi.MasterConfiguration, client clientset.Interface) error {


Again, the serviceaccount should be created as the first step here

luxas · 2017-08-14T11:44:32Z

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

+}
+
+func createClusterRoleBindings(client clientset.Interface) error {
+	// TODO: This ClusterRoleBinding should be created by the kube-proxy phase, not here


can remove this TODO now :)

luxas · 2017-08-14T11:46:32Z

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

+		return err
+	}
+
+	fmt.Println("[proxy] Created RBAC rules")


I don't think we should print this; one line per addon phase is enough

luxas · 2017-08-14T11:56:16Z

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

+
+// CreateRBACRules creates the essential RBAC rules for a minimally set-up cluster
+func CreateRBACRules(client clientset.Interface, k8sVersion *k8sversion.Version) error {
+	if err := createClusterRoleBindings(client); err != nil {


I really like the idea of having EnsureFooAddon, CreateServiceAccount and CreateRBACRules as the three exported funcs of every addon phase (even if some of them no-op).

luxas · 2017-08-14T11:57:15Z

cmd/kubeadm/app/cmd/init.go

 		return err
 	}

-	if err := apiconfigphase.CreateRBACRules(client, k8sVersion); err != nil {
+	if err := dnsaddonphase.CreateServiceAccount(client); err != nil {


This should not be necessary

luxas

/lgtm

Awesome to get this done!

k8s-github-robot · 2017-08-14T15:00:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewrynhard, luxas

Associated issue: 348

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kubeadm/OWNERS~~ [luxas]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

luxas · 2017-08-14T17:07:14Z

@andrewrynhard kind of annoying, but please fix this

W0814 15:57:13.081] Some packages in hack/.golint_failures do not exist anymore. Please remove them.
W0814 15:57:13.082] 
W0814 15:57:13.082]   cmd/kubeadm/app/phases/addons

k8s-github-robot · 2017-08-14T20:09:57Z

Automatic merge from submit-queue (batch tested with PRs 49904, 50484, 50214)

Automatic merge from submit-queue (batch tested with PRs 49904, 50484, 50214) Refactor addons into multiple packages **What this PR does / why we need it**: kubernetes/kubeadm#348 **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # kubernetes/kubeadm#348

@mattmoyer

Automatic merge from submit-queue (batch tested with PRs 50626, 50683, 50679, 50684, 50460) kubeadm: Centralize client create-or-update logic in one package **What this PR does / why we need it**: Moves all Create-or-Update logic into one package instead of duplicating that logic all around in the codebase. **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # **Special notes for your reviewer**: This PR depends on #50214. Note that commit 2 is the only one that needs reviewing. This PR is required for #48899 (kubeadm upgrade) **Release note**: ```release-note NONE ``` @kubernetes/sig-cluster-lifecycle-pr-reviews @mattmoyer @fabriziopandini

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 6, 2017

k8s-github-robot assigned lukemarsden and dmmcquay Aug 6, 2017

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-label-needed labels Aug 6, 2017

andrewrynhard force-pushed the refactor_addons branch from 2259bfd to d0dbc58 Compare August 6, 2017 22:59

andrewrynhard commented Aug 6, 2017

View reviewed changes

andrewrynhard force-pushed the refactor_addons branch from d0dbc58 to b156470 Compare August 6, 2017 23:04

luxas reviewed Aug 7, 2017

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 7, 2017

luxas added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Aug 7, 2017

luxas assigned luxas and unassigned lukemarsden Aug 7, 2017

luxas mentioned this pull request Aug 8, 2017

kubeadm: Move all node bootstrap token related code in one phase package #49982

Merged

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2017

andrewrynhard force-pushed the refactor_addons branch from b156470 to a4dbc2f Compare August 14, 2017 05:58

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 14, 2017

andrewrynhard force-pushed the refactor_addons branch from a4dbc2f to f3358ff Compare August 14, 2017 05:59

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 14, 2017

luxas reviewed Aug 14, 2017

View reviewed changes

Refactor addons into multiple packages

bbbf530

andrewrynhard force-pushed the refactor_addons branch from f3358ff to bbbf530 Compare August 14, 2017 14:57

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 14, 2017

luxas approved these changes Aug 14, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 14, 2017

Update bazel

8947726

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 14, 2017

luxas added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 14, 2017

Remove package from hack/.golint_failures

967a5bb

k8s-github-robot removed approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 14, 2017

luxas added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 14, 2017

luxas mentioned this pull request Aug 14, 2017

kubeadm: Centralize client create-or-update logic in one package #50626

Merged

k8s-github-robot merged commit a4996c9 into kubernetes:master Aug 14, 2017

andrewrynhard deleted the refactor_addons branch August 14, 2017 20:11

luxas mentioned this pull request Aug 18, 2017

Split up app/phases/addons into app/phases/addons/{dns,proxy} kubernetes/kubeadm#348

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor addons into multiple packages #50214

Refactor addons into multiple packages #50214

andrewrynhard commented Aug 6, 2017

k8s-ci-robot commented Aug 6, 2017

k8s-ci-robot commented Aug 6, 2017

andrewrynhard Aug 6, 2017

luxas Aug 7, 2017

andrewrynhard Aug 6, 2017

luxas Aug 7, 2017

andrewrynhard commented Aug 6, 2017

luxas left a comment

luxas Aug 7, 2017

luxas Aug 7, 2017

luxas Aug 7, 2017

luxas Aug 7, 2017

luxas Aug 7, 2017

luxas Aug 7, 2017

luxas Aug 7, 2017

luxas Aug 7, 2017

luxas Aug 7, 2017

luxas Aug 7, 2017

luxas commented Aug 7, 2017

luxas left a comment

luxas Aug 14, 2017

luxas Aug 14, 2017

luxas Aug 14, 2017

luxas Aug 14, 2017

luxas Aug 14, 2017

luxas Aug 14, 2017

luxas Aug 14, 2017

luxas left a comment

k8s-github-robot commented Aug 14, 2017

luxas commented Aug 14, 2017

k8s-github-robot commented Aug 14, 2017

Refactor addons into multiple packages #50214

Refactor addons into multiple packages #50214

Conversation

andrewrynhard commented Aug 6, 2017

k8s-ci-robot commented Aug 6, 2017

k8s-ci-robot commented Aug 6, 2017

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

andrewrynhard commented Aug 6, 2017

luxas 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

luxas commented Aug 7, 2017

luxas 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

luxas left a comment

Choose a reason for hiding this comment

k8s-github-robot commented Aug 14, 2017

luxas commented Aug 14, 2017

k8s-github-robot commented Aug 14, 2017