chore: don't require azure.json on node vms #2849

jackfrancis · 2020-03-06T00:01:56Z

Reason for Change:

This PR supports empty string ("") values for kubeletConfig configuration properties, to reflect that those values are interpreted significantly by the kubelet runtime. Specifically, when these three properties are empty strings, it tells the kubelet runtime that it is not running in an azure cloudprovider context (e.g., on a master vm):

"--cloud-provider":                  "",
"--cloud-config":                    "",
"--azure-container-registry-config": ""

To accommodate the significance of the above configuration set, this PR delivers the /etc/kubernetes/azure.json file to node vms only if those above three configuration properties are not all empty. In other words, if any one of the above properties is not "", then /etc/kubernetes/azure.json is paved on the vm just as it is prior to this change.

Issue Fixed:

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
tested upgrade from previous version

Notes:

codecov · 2020-03-06T00:16:15Z

Codecov Report

Merging #2849 into master will decrease coverage by 0.01%.
The diff coverage is 80.95%.

@@            Coverage Diff             @@
##           master    #2849      +/-   ##
==========================================
- Coverage   72.47%   72.45%   -0.02%     
==========================================
  Files         140      140              
  Lines       25589    25629      +40     
==========================================
+ Hits        18545    18569      +24     
- Misses       5976     5987      +11     
- Partials     1068     1073       +5

jackfrancis · 2020-03-06T19:35:59Z

parts/k8s/cloud-init/artifacts/cse_config.sh

    echo "${KUBELET_PRIVATE_KEY}" | base64 --decode > "${KUBELET_PRIVATE_KEY_PATH}"
    echo "${APISERVER_PUBLIC_KEY}" | base64 --decode > "${APISERVER_PUBLIC_KEY_PATH}"
+    configureKubeletServerCert


Moved this up here to accommodate the "return early if no azure.json" logic. It's not related to the azure.json paving, so can be done in any order.

jackfrancis · 2020-03-06T19:36:30Z

parts/k8s/cloud-init/artifacts/cse_config.sh

+    AZURE_JSON_PATH="/etc/kubernetes/azure.json"
+    if [[ -n "${MASTER_NODE}" ]]; then
+        if [[ "${ENABLE_AGGREGATED_APIS}" = True ]]; then
+            generateAggregatedAPICerts


Ditto moved this up earlier as well.

jackfrancis · 2020-03-06T19:37:10Z

pkg/api/defaults-kubelet.go

@@ -33,7 +33,7 @@ func (cs *ContainerService) setKubeletConfig(isUpgrade bool) {
 		switch key {
 		case "--anonymous-auth", "--client-ca-file":
 			if !to.Bool(o.KubernetesConfig.EnableSecureKubelet) { // Don't add if EnableSecureKubelet is disabled
-				staticLinuxKubeletConfig[key] = ""
+				delete(staticLinuxKubeletConfig, key)


Now that we are no longer deleting empty string keys in the removeKubeletFlags func we do it here.

jackfrancis · 2020-03-06T19:37:29Z

pkg/api/defaults-kubelet.go

@@ -270,13 +270,6 @@ func removeKubeletFlags(k map[string]string, v string) {
 			delete(k, key)
 		}
 	}
-
-	// Get rid of keys with empty string values


We don't remove empty string valued keys because they are significant

jackfrancis · 2020-03-06T19:37:43Z

pkg/api/defaults-kubelet_test.go

@@ -2166,3 +2167,77 @@ func TestReadOnlyPort(t *testing.T) {
 	}

 }
+
+func TestRemoveKubeletFlags(t *testing.T) {


This never had UT coverage

jackfrancis · 2020-03-06T19:38:33Z

pkg/api/types.go

+// RequiresCloudproviderConfig returns true if the azure.json cloudprovider config should be delivered to the nodes in this pool
+func (a *AgentPoolProfile) RequiresCloudproviderConfig() bool {
+	if a.KubernetesConfig != nil && a.KubernetesConfig.KubeletConfig != nil {
+		if v, ok := a.KubernetesConfig.KubeletConfig["--cloud-provider"]; ok {


Open to a better way to implement this logic. The UT cases below tell the truth in terms of proving that we're doing what we want to do.

The logic could also be stated as:

if all 3 keys exist in the config with a value of empty string ""
return false
else
return true

Right? Just making sure I'm reading it correctly.

Never mind, I read the UT. :-)

jackfrancis · 2020-03-06T19:48:04Z

parts/k8s/cloud-init/artifacts/cse_config.sh

+            generateAggregatedAPICerts
+        fi
+    else
+        {{- /* If we are a node vm then we only proceed w/ local azure.json configuration if cloud-init has pre-paved that file */}}


To be clear, we are not pre-paving azure.json for master vms (note that there is no diff to masternodecustomdata.yml. The existing code (specifically cat << EOF > "${AZURE_JSON_PATH}") works as-is whether or not the file is there (the > "${AZURE_JSON_PATH}" part). So we are just using the pre-existence of azure.json, in a non-master flow only, to determine whether or not we will include the azure.json on this node. If the "check for file" test fails, we return immediately, because that tells us we aren't including azure.json on this node.

This works because:

we are paving azure.json on nodes, if needed, prior to paving this file itself (cse_config.sh in code, and /opt/azure/containers/provision_configs.sh on the local vm fs)

we are waiting for this file in cse_main.sh before executing code in this file, and because the cloud-init write_files property is a yaml sequence, those will be processed in order. Thus, by the time we are in this execution flow, if azure.json should be here, it will be here.

parts/k8s/cloud-init/artifacts/cse_config.sh

acs-bot · 2020-03-06T20:04:18Z

pkg/api/types.go

+// RequiresCloudproviderConfig returns true if the azure.json cloudprovider config should be delivered to the nodes in this pool
+func (a *AgentPoolProfile) RequiresCloudproviderConfig() bool {
+	if a.KubernetesConfig != nil && a.KubernetesConfig.KubeletConfig != nil {
+		if v, ok := a.KubernetesConfig.KubeletConfig["--cloud-provider"]; ok {


The logic could also be stated as:

if all 3 keys exist in the config with a value of empty string ""
return false
else
return true

Right? Just making sure I'm reading it correctly.

acs-bot · 2020-03-06T20:05:30Z

pkg/api/types.go

+// RequiresCloudproviderConfig returns true if the azure.json cloudprovider config should be delivered to the nodes in this pool
+func (a *AgentPoolProfile) RequiresCloudproviderConfig() bool {
+	if a.KubernetesConfig != nil && a.KubernetesConfig.KubeletConfig != nil {
+		if v, ok := a.KubernetesConfig.KubeletConfig["--cloud-provider"]; ok {


Never mind, I read the UT. :-)

parts/k8s/cloud-init/nodecustomdata.yml

mboersma

/lgtm

chore: don't require azure.json on node vms

8cbd6f4

jackfrancis added 2 commits March 5, 2020 16:44

test: move to base.json cluster config test

7761fc1

test: only run cloudprovider config test on nodes that have azure.json

c2543da

jackfrancis commented Mar 6, 2020

View reviewed changes

chore: undo this unnecessary change

3d9809d

jackfrancis commented Mar 6, 2020

View reviewed changes

shliderm reviewed Mar 6, 2020

View reviewed changes

parts/k8s/cloud-init/artifacts/cse_config.sh Show resolved Hide resolved

acs-bot reviewed Mar 6, 2020

View reviewed changes

jackfrancis added 2 commits March 6, 2020 12:28

chore: restore set +x

50dc576

chore: generated code

d8c862e

mboersma approved these changes Mar 6, 2020

View reviewed changes

jackfrancis merged commit 3d52e83 into Azure:master Mar 7, 2020

jackfrancis deleted the kubelet-agentpool-cloudprovider branch March 7, 2020 00:11

fmotrifork mentioned this pull request Mar 23, 2020

aks-engine 0.48.0 fishworks/fish-food#639

Merged

jackfrancis mentioned this pull request Sep 28, 2020

chore: create azure.json via CSE #3876

Merged

8 tasks

CecileRobertMichon mentioned this pull request Nov 3, 2020

Pod+PVC based on CSI storage class stays in Pending state kubernetes-sigs/azuredisk-csi-driver#583

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: don't require azure.json on node vms #2849

chore: don't require azure.json on node vms #2849

jackfrancis commented Mar 6, 2020

codecov bot commented Mar 6, 2020 •

edited

Loading

jackfrancis Mar 6, 2020

jackfrancis Mar 6, 2020

jackfrancis Mar 6, 2020

jackfrancis Mar 6, 2020

jackfrancis Mar 6, 2020

jackfrancis Mar 6, 2020

acs-bot Mar 6, 2020

acs-bot Mar 6, 2020

jackfrancis Mar 6, 2020

acs-bot Mar 6, 2020

acs-bot Mar 6, 2020

mboersma left a comment

chore: don't require azure.json on node vms #2849

chore: don't require azure.json on node vms #2849

Conversation

jackfrancis commented Mar 6, 2020

codecov bot commented Mar 6, 2020 • edited Loading

Codecov Report

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

mboersma left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 6, 2020 •

edited

Loading