Kubeadm upload and fetch of kubeam config v1alpha3 #67944

fabriziopandini · 2018-08-28T12:29:52Z

What this PR does / why we need it:
This PR implements upload and fetch of kubeam config v1alpha3 from cluster.

More in detail:
In upload, kubeadm-config gets

ClusterConfiguration (without components config which are already stored in separated ConfigMaps)
ClusterStatus(initialised or updated with the API endpoint of the current node)

During fetch InitConfiguration is composed with:

ClusterConfiguration from kubeadm-config
The APIEndpoint of the current node from ClusterStatus in kubeadm-config
Component configs from corresponding ConfigMaps

Which issue(s) this PR fixes :
refs kubernetes/kubeadm#911, refs kubernetes/kubeadm#963

Special notes for your reviewer:
In order to implement this it was necessary to extend current component config management with a new GetFromConfigMap operation. This is implemented in a separated commit "
implement component configs GetFromConfigMap".
The real change build on this (commi "upload and fetch kubeadm v1alpha3")

Release note:

NONE

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/sig cluster-lifecycle
/area kubeadm
/kind enhancement
/assign @luxas
/assign @timothysc
/cc @chuckha @rosti @neolit123 @liztio

neolit123

thanks @fabriziopandini
added minor comments but also commented on the usage of ChooseBindAddress that we talked about last time. we might need something more unique here, unless i'm mistaken.

neolit123 · 2018-08-28T13:50:53Z

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

+
+	apiEndpoint, ok := status.APIEndpoints[machineID]
+	if !ok {
+		return nil, fmt.Errorf("failed to retry APIEndpoint information for this node")


to retry -> to get?

neolit123 · 2018-08-28T13:58:24Z

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

+func GetMachineID() (string, error) {
+	// the IP of the host default network interface as a machine unique id
+	ip, err := netutil.ChooseBindAddress(nil)
+	if err != nil {


so if we have the following scenario:

machine A is a master node and ChooseBindAddress returned 192.168.0.1 for it.

machine B in a different network want to join the cluster but ChooseBindAddress also returns 192.168.0.1 for it.

both nodes have the same ID/local IP, even if technically for node B to join node A it needs it's external IP.

hostnames suffer from the same problem.
i think we might have to do UUIDs, but this needs platform specific code.

@neolit123 how frequent is this case?
However, If there is a library already vendored in kubernetes for getting UUIDs I can try to switch...

i think it's a general issue when nodes are not on the same sub-network?

I think generating some UUID is pretty useful when folks are changing nodes in a virutual infra will be useful.

i can't find a vendored "system" uuid lib in k8s - i.e. a library for returning the uuid of the machine.

there is this, but this library is just for generating a RFC compliant random uuid:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/util/uuid/uuid.go

we need something like:
https://github.com/denisbrodbeck/machineid
the methods that the library uses are outlined here:
https://github.com/denisbrodbeck/machineid#snippets

I like the idea of UUIDs here. IP change can cause issues.

On the other hand, the machineid library queries OS maintained IDs. These are usually generated upon first boot, install or regenerated if missing on boot. They can be the same if a VM is created from a template and these are not taken care of correctly.

Meanwhile, in our own documentation:
https://kubernetes.io/docs/setup/independent/install-kubeadm/#verify-the-mac-address-and-product-uuid-are-unique-for-every-node

i can't find MAC address handling in the k/k codebase.

closed PR that introduced machine IDs for host names:
#52625

closed issue about node uniqueness:
#6557

@fabriziopandini we talked about this at the kubeadm office hours today.

Jason reminded us that the node name should be unique for a cluster to work anyway so we can use that as the key for the status map:

from @detiber

More specifically, this should be "node name" being either the user-supplied override in the kubeadm config or the default detected node name (generally the hostname here)

neolit123 · 2018-08-28T13:58:36Z

cmd/kubeadm/app/apis/kubeadm/types_test.go

@@ -0,0 +1,107 @@
+/*
+Copyright 2016 The Kubernetes Authors.


neolit123 · 2018-08-28T14:03:01Z

cmd/kubeadm/app/componentconfigs/config_test.go

+	// Create temp folder for the test case
+	tmpdir, err := ioutil.TempDir("", "")
+	if err != nil {
+		t.Fatalf("Couldn't create tmpdir")


Couldn't -> couldn't

neolit123 · 2018-08-28T14:07:50Z

cmd/kubeadm/app/componentconfigs/config_test.go

+			}
+
+			if obj == nil {
+				t.Errorf("Unexpected nil return value")


Unexpected -> unexpected

neolit123 · 2018-08-28T14:08:01Z

cmd/kubeadm/app/componentconfigs/config_test.go

+			k8sVersion := version.MustParseGeneric("v1.12.0")
+			obj, err := r.GetFromConfigMap(client, k8sVersion)
+			if err != nil {
+				t.Errorf("Error getting ConfigMap: %v", err)


Error getting -> error getting

neolit123 · 2018-08-28T14:08:10Z

cmd/kubeadm/app/componentconfigs/config_test.go

+
+			err := rt.configMap.create(client)
+			if err != nil {
+				t.Errorf("Couldn't create ConfigMap %s", rt.configMap.name)


Couldn't -> couldn't

rosti

@fabriziopandini Thanks for the effort! Has a few minor nits, but otherwise it LGTM.

rosti · 2018-08-28T12:56:06Z

cmd/kubeadm/app/apis/kubeadm/types_test.go

+				return
+			}
+			if err == nil && !reflect.DeepEqual(apiEndpoint, actual) {
+				t.Errorf("actual doesn't match expected apiEndpoint")


Perhaps print actual and expected here?

rosti · 2018-08-28T12:59:01Z

cmd/kubeadm/app/componentconfigs/config.go

+	configMapName := kubeadmconstants.GetKubeletConfigMapName(version)
+	kubeletCfg, err := client.CoreV1().ConfigMaps(metav1.NamespaceSystem).Get(configMapName, metav1.GetOptions{})
+	if apierrors.IsNotFound(err) {
+		return nil, fmt.Errorf("")


The actual error message is missing here

rosti · 2018-08-28T14:11:41Z

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

 )

 // FetchConfigFromFileOrCluster fetches configuration required for upgrading your cluster from a file (which has precedence) or a ConfigMap in the cluster
-// TODO: This func should be renamed FetchClusterConfigFromFileOrCluster, and return a ClusterConfiguration instead of an InitConfiguration


We may need to keep an updated version of this TODO for the future.
Ideally we should not be forced to have InitConfiguration for anything else than kubeadm init. I am not sure, that ClusterConfiguration is the answer too (since APIEndpoint is not part of it).
It may be something else, but we may just keep some TODO here, just to mark our intent in reducing the InitConfiguration usages in the long run.

Here I removed the comment about ClusterConfiguration because we know this isn't the right approach.

For the long run I agree 100% with your intent of reducing the InitConfiguration usages, but better track this with an issue than TODOs in the code.
If you can kindly open the issue this will be great 😉

Filed kubernetes/kubeadm#1084

timothysc

I only did a quick scan, let me know when you're ready and I'll go through this in detail.

timothysc · 2018-08-28T21:54:52Z

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

+func GetMachineID() (string, error) {
+	// the IP of the host default network interface as a machine unique id
+	ip, err := netutil.ChooseBindAddress(nil)
+	if err != nil {


I think generating some UUID is pretty useful when folks are changing nodes in a virutual infra will be useful.

timothysc · 2018-08-28T21:56:16Z

cmd/kubeadm/app/componentconfigs/config.go

+
+// HasKubeletConfigMap returns true if the Kubelet is expected to have a config map stored in the cluster
+func HasKubeletConfigMap(cfg *kubeadmapi.ClusterConfiguration) bool {
+	return true


Forget it, now removed

timothysc · 2018-08-28T21:58:34Z

cmd/kubeadm/app/constants/constants.go

+	KubeProxyConfigMap = "kube-proxy"
+
+	// KubeProxyConfigMapKey specifies in what ConfigMap key the component config of kube-proxy should be stored
+	KubeProxyConfigMapKey = "config.conf"


This is the key name used in kube-proxy manifest.. just refactored in a constant because it is now used in different places.

rosti · 2018-08-29T08:59:25Z

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

@@ -79,7 +79,7 @@ spec:
        imagePullPolicy: IfNotPresent
        command:
        - /usr/local/bin/kube-proxy
-        - --config=/var/lib/kube-proxy/config.conf
+        - --config=/var/lib/kube-proxy/{{ .ProxyConfigMapKey }}s


Is the trailing s desired?

No, now fixed, thanks

fabriziopandini · 2018-09-01T20:07:40Z

@timothysc

Implemented the logic for getting the node name from the kubelet.conf. However it was necessary to get the user name from x509 certificate and manage two cases:
- kubelet.conf generated by kubeadm
- kubelet.conf generated by TLSBootstrap.
Used the node name as a key for the ClusterConfig.APIEndpoints map (insterad of the netutil.ChooseBindAddress or hostname)
Implemented a lot of unit tests for all the upload /fetch config jiggery
Fixed kubeadm join --control-plane, kubeadm upgrade and kubeadm upgrade node --control-plane workflows with the new upload /fetch config jiggery removing some TODOs left in previous PRs
completed first rounds of manual testing:
- V1.11.2 --> v1.12.0
- V1.12.0 with two masters --> v1.12.1

timothysc

1st pass run through.

timothysc · 2018-09-02T00:05:45Z

cmd/kubeadm/app/componentconfigs/config.go

+
+	// Decodes the Config map into the internal component config
+	obj := &kubeletconfig.KubeletConfiguration{}
+	err = unmarshalObject(obj, []byte(kubeletCfg.Data[kubeadmconstants.KubeletBaseConfigurationConfigMapKey]))


should you index check 1st otherwise you could SEGV here. .Data[kubeadmconstants.KubeletBaseConfigurationConfigMapKey]

fixed + added UT

timothysc · 2018-09-02T00:06:24Z

cmd/kubeadm/app/componentconfigs/config.go

+
+	// Decodes the Config map into the internal component config
+	obj := &kubeproxyconfig.KubeProxyConfiguration{}
+	err = unmarshalObject(obj, []byte(kubeletCfg.Data[kubeadmconstants.KubeProxyConfigMapKey]))


Same comment.

timothysc · 2018-09-02T00:14:18Z

cmd/kubeadm/app/componentconfigs/registrations.go

 		return nil, err
 	}
 	return obj, nil
 }

+func unmarshalObject(obj runtime.Object, fileContent []byte) error {


I swear these functions exist elsewhere.

This function uses a scheme with component configs api registered.
The other unmarshal func uses a scheme with only kubeadm types registered

timothysc · 2018-09-02T00:17:56Z

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

+	}
+
+	// Create a role for granting read only access to the kube-proxy component config ConfigMap
+	if err := apiclient.CreateOrUpdateRole(client, &rbac.Role{


Do we have tests to validate the updated RBAC rules?

The only test on RBAC I'm aware of are in test\e2e_kubeadm but I'm not sure if those test are used in test grid batches...

timothysc · 2018-09-02T00:20:43Z

cmd/kubeadm/app/phases/uploadconfig/uploadconfig.go

+
+	// Prepare the ClusterStatus for upload
+	// Gets the current cluster status
+	clusterStatus, err := getClusterStatus(client)


I think we are ok for now, but we should add a //TODO use configmap locks on this object on the get before the update. We may even bug fix that after the freeze

/cc @liztio

neolit123

LGTM

neolit123 · 2018-09-03T16:39:18Z

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

+			return "", err
+		}
+	} else {
+		return "", fmt.Errorf("Invalid kubelet.conf. X509 certificate expected")


nit / optional: errors.New() instead of fmt.Errorf()

neolit123 · 2018-09-03T16:39:46Z

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

+
+	// We are only putting one certificate in the certificate pem file, so it's safe to just pick the first one
+	// TODO: Support multiple certs here in order to be able to rotate certs
+	cert := certs[0]


do we have a tracking issue?

Not yet. I will send tracking issues for TODOs soonish (after lgtm)

rosti

@fabriziopandini at first pass this LGTM, has some tiny nits though

rosti · 2018-09-03T20:28:14Z

cmd/kubeadm/app/cmd/join.go

 	kubeConfigFile := filepath.Join(kubeadmconstants.KubernetesDir, kubeadmconstants.AdminKubeConfigFileName)

 	client, err := kubeconfigutil.ClientSetFromFile(kubeConfigFile)
 	if err != nil {
 		return errors.Wrap(err, "couldn't create kubernetes client")
 	}

-	err = markmasterphase.MarkMaster(client, j.cfg.NodeRegistration.Name, j.cfg.NodeRegistration.Taints)
-	if err != nil {
+	glog.V(1).Infof("[join] uploading currently used configuration to the cluster")


Infof -> Info

rosti · 2018-09-03T20:28:47Z

cmd/kubeadm/app/cmd/join.go

+		return fmt.Errorf("error uploading configuration: %v", err)
+	}
+
+	glog.V(1).Infof("[join] marking the master with right label")


Infof -> Info

rosti · 2018-09-03T20:36:39Z

cmd/kubeadm/app/componentconfigs/config_test.go

+			}
+
+			if obj == nil {
+				t.Errorf("unexpected nil return value")


Errorf -> Error

rosti · 2018-09-03T20:49:26Z

cmd/kubeadm/app/phases/uploadconfig/uploadconfig_test.go

 				if configData == "" {
-					t2.Fatalf("Fail to find ConfigMap key")
+					t2.Fatalf("Fail to find ClusterConfigurationConfigMapKey key")


Fatalf -> Fatal

rosti · 2018-09-03T20:49:54Z

cmd/kubeadm/app/phases/uploadconfig/uploadconfig_test.go

+
+				statusData := masterCfg.Data[kubeadmconstants.ClusterStatusConfigMapKey]
+				if statusData == "" {
+					t2.Fatalf("failed to find ClusterStatusConfigMapKey key")


Fatalf -> Fatal

rosti · 2018-09-03T20:50:11Z

cmd/kubeadm/app/phases/uploadconfig/uploadconfig_test.go

+				}
+
+				if !reflect.DeepEqual(decodedStatus, status) {
+					t2.Errorf("the initial and decoded ClusterStatus didn't match")


Errorf -> Error

rosti · 2018-09-03T20:50:41Z

cmd/kubeadm/app/phases/uploadconfig/uploadconfig_test.go

+
+			actual, err := getClusterStatus(client)
+			if err != nil {
+				t.Errorf("GetClusterStatus returned unexpected error")


Errorf -> Error

rosti · 2018-09-03T20:50:47Z

cmd/kubeadm/app/phases/uploadconfig/uploadconfig_test.go

+				return
+			}
+			if tt.expectedClusterEndpoints != len(actual.APIEndpoints) {
+				t.Errorf("actual ClusterStatus doesn't return expected endpoints")


Errorf -> Error

rosti · 2018-09-03T20:58:57Z

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

+	// gets the APIEndpoint for the current machine from the ClusterStatus
+	e, ok := clusterStatus.APIEndpoints[nodeName]
+	if !ok {
+		return fmt.Errorf("failed to get APIEndpoint information for this node")


fmt.Errorf -> errors.New

…e/upgrade node

timothysc

/lgtm
/approve

We're too late for nits, we'll have to beat on this during freeze.

k8s-ci-robot · 2018-09-04T13:36:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, timothysc

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,timothysc]

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

timothysc · 2018-09-04T13:36:45Z

/test pull-kubernetes-integration

k8s-github-robot · 2018-09-04T17:24:41Z

Automatic merge from submit-queue (batch tested with PRs 63011, 68089, 67944, 68132). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Aug 28, 2018

k8s-ci-robot assigned luxas and timothysc Aug 28, 2018

k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/kubeadm labels Aug 28, 2018

k8s-ci-robot requested review from chuckha, liztio, neolit123 and rosti August 28, 2018 12:29

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 28, 2018

fabriziopandini changed the title ~~Kubeadm config config map~~ Kubeadm upload and fetch of kubeam config v1alpha3 Aug 28, 2018

neolit123 reviewed Aug 28, 2018

View reviewed changes

rosti reviewed Aug 28, 2018

View reviewed changes

timothysc added this to the v1.12 milestone Aug 28, 2018

timothysc added the status/approved-for-milestone label Aug 28, 2018

fabriziopandini force-pushed the kubeadm-config-configMap branch from a493ddc to 18d997f Compare August 28, 2018 20:05

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 28, 2018

fabriziopandini force-pushed the kubeadm-config-configMap branch 2 times, most recently from c0c0289 to 71bf4f7 Compare August 28, 2018 21:22

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

fabriziopandini force-pushed the kubeadm-config-configMap branch from 71bf4f7 to 4c2ab68 Compare August 28, 2018 21:36

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

timothysc reviewed Aug 28, 2018

View reviewed changes

rosti reviewed Aug 29, 2018

View reviewed changes

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

fabriziopandini force-pushed the kubeadm-config-configMap branch from 4c2ab68 to a39396f Compare September 1, 2018 19:52

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2018

timothysc suggested changes Sep 2, 2018

View reviewed changes

fabriziopandini force-pushed the kubeadm-config-configMap branch from a39396f to 076ca60 Compare September 2, 2018 07:41

fabriziopandini mentioned this pull request Sep 2, 2018

Make kubeadm upgrade HA ready kubernetes/kubeadm#706

Closed

neolit123 approved these changes Sep 3, 2018

View reviewed changes

rosti approved these changes Sep 3, 2018

View reviewed changes

fabriziopandini added 3 commits September 4, 2018 09:09

upload and fetch of kubeam config v1alpha3 from cluster

3f70af3

use new kubeadm-config in kubeadm join control-plane & kubeadm upgrad…

8af751f

…e/upgrade node

autogenerated

d9b4b1f

fabriziopandini force-pushed the kubeadm-config-configMap branch from 076ca60 to d9b4b1f Compare September 4, 2018 07:12

timothysc approved these changes Sep 4, 2018

View reviewed changes

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

timothysc added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Sep 4, 2018

k8s-github-robot merged commit 5540edc into kubernetes:master Sep 4, 2018

fabriziopandini deleted the kubeadm-config-configMap branch September 6, 2018 10:31

fabriziopandini mentioned this pull request Sep 18, 2018

Checklist for kubeadm join --control-plane implementation kubernetes/kubeadm#751

Closed

11 tasks

neolit123 mentioned this pull request Sep 18, 2018

Tracking issue for "Config to v1beta1" kubernetes/kubeadm#963

Closed

28 tasks

Kubeadm upload and fetch of kubeam config v1alpha3 #67944

Kubeadm upload and fetch of kubeam config v1alpha3 #67944

Conversation

fabriziopandini commented Aug 28, 2018

neolit123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 Aug 28, 2018 • 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

neolit123 Aug 28, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosti 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

timothysc 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

fabriziopandini commented Sep 1, 2018

timothysc 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

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

rosti 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

timothysc left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 4, 2018

timothysc commented Sep 4, 2018

k8s-github-robot commented Sep 4, 2018

neolit123 Aug 28, 2018 •

edited

Loading

neolit123 Aug 28, 2018 •

edited

Loading