kubeadm: Refactor InitConfiguration init APIs #73701

rosti · 2019-02-04T15:47:25Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Currently ConfigFileAndDefaultsToInternalConfig and FetchConfigFromFileOrCluster are used to default and load InitConfiguration from file or cluster. These two APIs do a couple of completely separate things depending on how they were invoked.

In the case of ConfigFileAndDefaultsToInternalConfig, an InitConfiguration could be either defaulted with external override parameters, or loaded from file.
With FetchConfigFromFileOrCluster an InitConfiguration is either loaded from file or from the config map in the cluster.

The two share both some functionality, but not enough code. They are also quite difficult to use and sometimes even error prone.

To solve the issues, the following steps were taken:

Introduce DefaultedInitConfiguration which returns defaulted version agnostic InitConfiguration. The function takes InitConfiguration for overriding the defaults.
Introduce LoadInitConfigurationFromFile, which loads, converts, validates and defaults an InitConfiguration from file.
Introduce FetchConfigFromCluster that fetches InitConfiguration from the config map.
Reduce, when possible, the usage of ConfigFileAndDefaultsToInternalConfig by replacing it with DefaultedInitConfiguration or LoadInitConfigurationFromFile invocations.
Replace all usages of FetchConfigFromFileOrCluster with calls to LoadInitConfigurationFromFile or FetchConfigFromCluster whenever needed.
Delete FetchConfigFromFileOrCluster as it's no longer used.

Which issue(s) this PR fixes:

Refs kubernetes/kubeadm#1296

Special notes for your reviewer:

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/area kubeadm
/assign @fabriziopandini
/assign @neolit123

Does this PR introduce a user-facing change?:

NONE

rosti · 2019-02-04T15:49:32Z

/priority important-longterm

rosti · 2019-02-05T11:47:38Z

This one should be ready for review now.

neolit123

/approve
this looks good thanks @rosti

will leave the lgtm to another set of 👀

fabriziopandini

@rosti this looks great! thanks!
I left some comments; I'm looking forward to can get this merged asap!
/approve

fabriziopandini · 2019-02-08T06:58:47Z

cmd/kubeadm/app/cmd/alpha/kubeconfig.go

@@ -80,7 +80,7 @@ func newCmdUserKubeConfig(out io.Writer) *cobra.Command {
 			}

 			// This call returns the ready-to-use configuration based on the configuration file that might or might not exist and the default cfg populated by flags


This comment should reflect the fact the we are not loading config file

fabriziopandini · 2019-02-08T07:12:21Z

cmd/kubeadm/app/cmd/join.go

@@ -647,7 +647,7 @@ func fetchInitConfiguration(tlsBootstrapCfg *clientcmdapi.Config) (*kubeadmapi.I
 	}

 	// Fetches the init configuration
-	initConfiguration, err := configutil.FetchConfigFromFileOrCluster(tlsClient, os.Stdout, "join", "", true)
+	initConfiguration, err := configutil.FetchConfigFromCluster(tlsClient, os.Stdout, "join", true)


Just a note, not in the scope of this PR but interesting for a potential follow issue/PR.

What we really need here is the ClusterConfiguration only, because the components configs are not used by join and the data for the current node are extracted from the Join configuration.

Having a method that allows fetching the ClusterConfiguration only could be beneficial in many ways:

join became resilient to the fact the kube-proxy add-on exists or not (if I'm not wrong @neolit123 is working on this)

it is possible to get rid of the newControlPlane flag in fetch config

Ideally FetchConfigFromCluster would be renamed to FetchClusterConfiguration and would return ClusterConfiguration only. getNodeRegistration and getAPIEndpoint will become public separate functions.
Another way to tackle this is to introduce an interface, that has GetClientConfiguration(), GetNodeRegistration() and GetLocalAPIEndpoint(). InitConfiguration will implement this, so it will be straightforward to use. Also, there will be an internal implementation, that takes kubeconfig and/or JoinConfiguration and then implement the interface by fetching stuff from the cluster or JoinConfiguration. This approach has the benefit of replacing InitConfiguration in most places with a single type and of adding caching (so that we fetch ClusterConfiguration only once for example).
I am experimenting with both approaches, but the first code will probably become available in the 1.15 time frame.

became resilient to the fact the kube-proxy add-on exists or not (if I'm not wrong @neolit123 is working on this)

waiting on the join phases work to finish.

but i think we might have a problem related to that.
i think the original reason to fetch the component config for kubeproxy was to be able to perform specific preflight checks on the joining node (was it about IPVS?). if we fetch cluster config only, these preflight checks will have to be removed (or made optional, somehow)

fabriziopandini · 2019-02-08T07:30:34Z

cmd/kubeadm/app/cmd/reset.go

@@ -299,7 +299,7 @@ func resetConfigDir(configPathDir, pkiPathDir string) {

 func resetDetectCRISocket(client clientset.Interface) (string, error) {
 	// first try to connect to the cluster for the CRI socket
-	cfg, err := configutil.FetchConfigFromFileOrCluster(client, os.Stdout, "reset", "", false)
+	cfg, err := configutil.FetchConfigFromCluster(client, os.Stdout, "reset", false)


Another note for follow-up PR.
We should avoid fetching the config twice in reset 😢

fabriziopandini · 2019-02-08T08:07:28Z

cmd/kubeadm/app/util/config/cluster.go

-	}
-
+// FetchConfigFromCluster fetches configuration from a ConfigMap in the cluster
+func FetchConfigFromCluster(client clientset.Interface, w io.Writer, logPrefix string, newControlPlane bool) (*kubeadmapi.InitConfiguration, error) {


what about using FetchInitConfigurationFromCluster instead of FetchConfigFromCluster (same pattern of LoadInitConfigurationFromFile or DefaultedInitConfiguration)?

Originally I envisioned FetchConfigFromCluster to be reduced to returning ClusterConfiguration only. However I am still not fully committed on this approach and this probably won't happen in 1.14, so I'll rename it to contain Init in its name.

fabriziopandini · 2019-02-08T08:14:31Z

cmd/kubeadm/app/util/config/initconfiguration.go

+// Then the external, versioned configuration is defaulted and converted to the internal type.
+// Right thereafter, the configuration is defaulted again with dynamic values (like IP addresses of a machine, etc)
+// Lastly, the internal config is validated and returned.
+func ConfigFileAndDefaultsToInternalConfig(cfgPath string, defaultversionedcfg *kubeadmapiv1beta1.InitConfiguration) (*kubeadmapi.InitConfiguration, error) {


What about renaming into LoadOrDefaultInitConfiguration or something similar (that uses InitConfiguration)?

This is done by @bruceauyeung in a yet to be merged PR #68595 .
It's better if it goes in this one though as it's more tied to the subject at hand.
@fabriziopandini @bruceauyeung WDYT?

Currently ConfigFileAndDefaultsToInternalConfig and FetchConfigFromFileOrCluster are used to default and load InitConfiguration from file or cluster. These two APIs do a couple of completely separate things depending on how they were invoked. In the case of ConfigFileAndDefaultsToInternalConfig, an InitConfiguration could be either defaulted with external override parameters, or loaded from file. With FetchConfigFromFileOrCluster an InitConfiguration is either loaded from file or from the config map in the cluster. The two share both some functionality, but not enough code. They are also quite difficult to use and sometimes even error prone. To solve the issues, the following steps were taken: - Introduce DefaultedInitConfiguration which returns defaulted version agnostic InitConfiguration. The function takes InitConfiguration for overriding the defaults. - Introduce LoadInitConfigurationFromFile, which loads, converts, validates and defaults an InitConfiguration from file. - Introduce FetchInitConfigurationFromCluster that fetches InitConfiguration from the config map. - Reduce, when possible, the usage of ConfigFileAndDefaultsToInternalConfig by replacing it with DefaultedInitConfiguration or LoadInitConfigurationFromFile invocations. - Replace all usages of FetchConfigFromFileOrCluster with calls to LoadInitConfigurationFromFile or FetchInitConfigurationFromCluster. - Delete FetchConfigFromFileOrCluster as it's no longer used. - Rename ConfigFileAndDefaultsToInternalConfig to LoadOrDefaultInitConfiguration in order to better describe what the function is actually doing. Signed-off-by: Rostislav M. Georgiev <rostislavg@vmware.com>

fabriziopandini

Thanks, @rosti
/approve
/lgtm

k8s-ci-robot · 2019-02-11T15:34:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, neolit123, 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:

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

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

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Feb 4, 2019

k8s-ci-robot assigned fabriziopandini and neolit123 Feb 4, 2019

k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Feb 4, 2019

rosti changed the title ~~kubeadm: Refactor InitConfiguration init APIs~~ [WIP] kubeadm: Refactor InitConfiguration init APIs Feb 4, 2019

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 4, 2019

rosti force-pushed the refactor-initcfg-apis branch 2 times, most recently from 77700e3 to 22f843a Compare February 5, 2019 10:18

rosti changed the title ~~[WIP] kubeadm: Refactor InitConfiguration init APIs~~ kubeadm: Refactor InitConfiguration init APIs Feb 5, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 5, 2019

neolit123 approved these changes Feb 5, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 5, 2019

fabriziopandini approved these changes Feb 8, 2019

View reviewed changes

rosti force-pushed the refactor-initcfg-apis branch from 22f843a to c9a4d35 Compare February 11, 2019 10:21

rosti force-pushed the refactor-initcfg-apis branch from c9a4d35 to 51197e4 Compare February 11, 2019 12:48

fabriziopandini approved these changes Feb 11, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2019

k8s-ci-robot merged commit 1e6afac into kubernetes:master Feb 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: Refactor InitConfiguration init APIs #73701

kubeadm: Refactor InitConfiguration init APIs #73701

rosti commented Feb 4, 2019

rosti commented Feb 4, 2019

rosti commented Feb 5, 2019

neolit123 left a comment

fabriziopandini left a comment

fabriziopandini Feb 8, 2019

fabriziopandini Feb 8, 2019

rosti Feb 8, 2019

neolit123 Feb 8, 2019 •

edited

fabriziopandini Feb 8, 2019

fabriziopandini Feb 8, 2019

rosti Feb 8, 2019

fabriziopandini Feb 8, 2019

rosti Feb 8, 2019

fabriziopandini left a comment

k8s-ci-robot commented Feb 11, 2019

		@@ -80,7 +80,7 @@ func newCmdUserKubeConfig(out io.Writer) *cobra.Command {
		}

		// This call returns the ready-to-use configuration based on the configuration file that might or might not exist and the default cfg populated by flags

kubeadm: Refactor InitConfiguration init APIs #73701

kubeadm: Refactor InitConfiguration init APIs #73701

Conversation

rosti commented Feb 4, 2019

rosti commented Feb 4, 2019

rosti commented Feb 5, 2019

neolit123 left a comment

Choose a reason for hiding this comment

fabriziopandini 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

neolit123 Feb 8, 2019 • 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

fabriziopandini left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 11, 2019

neolit123 Feb 8, 2019 •

edited