Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

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

Merged
merged 6 commits into from
Mar 7, 2020
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8cbd6f4
chore: don't require azure.json on node vms
jackfrancis Mar 5, 2020
7761fc1
test: move to base.json cluster config test
jackfrancis Mar 6, 2020
c2543da
test: only run cloudprovider config test on nodes that have azure.json
jackfrancis Mar 6, 2020
3d9809d
chore: undo this unnecessary change
jackfrancis Mar 6, 2020
50dc576
chore: restore set +x
jackfrancis Mar 6, 2020
d8c862e
chore: generated code
jackfrancis Mar 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ github.com/google/btree v1.0.0 h1:0udJVsspx3VBr5FwtLhQQtuAsVc79tTq0ocGIPAU6qo=
github.com/google/btree v1.0.0/go.mod h1:lNA+9X1NB3Zf8V7Ke586lFgjr2dZNuvo3lPJSGZ5JPQ=
github.com/google/go-cmp v0.2.0 h1:+dTQ8DZQJz0Mb/HjFlkptS1FeQ4cWSnN941F8aEG4SQ=
github.com/google/go-cmp v0.2.0/go.mod h1:oXzfMopK8JAjlY9xF4vHSVASa0yLyX7SntLO5aqRK0M=
github.com/google/go-cmp v0.4.0 h1:xsAVV57WRhGj6kEIi8ReJzQlHHqcBYCElAvkovg3B/4=
github.com/google/gofuzz v1.0.0 h1:A8PeW59pxE9IoFRqBp37U+mSNaQoZ46F1f0f863XSXw=
github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg=
github.com/google/uuid v1.1.1 h1:Gkbcsh/GbpXz7lPftLA3P6TYMwjCLYm83jiFQZF/3gY=
Expand Down
28 changes: 13 additions & 15 deletions parts/k8s/cloud-init/artifacts/cse_config.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,14 +165,19 @@ configureK8s() {
chmod 0644 "${APISERVER_PUBLIC_KEY_PATH}"
chown root:root "${APISERVER_PUBLIC_KEY_PATH}"

AZURE_JSON_PATH="/etc/kubernetes/azure.json"
touch "${AZURE_JSON_PATH}"
chmod 0600 "${AZURE_JSON_PATH}"
chown root:root "${AZURE_JSON_PATH}"

set +x
jackfrancis marked this conversation as resolved.
Show resolved Hide resolved
echo "${KUBELET_PRIVATE_KEY}" | base64 --decode > "${KUBELET_PRIVATE_KEY_PATH}"
echo "${APISERVER_PUBLIC_KEY}" | base64 --decode > "${APISERVER_PUBLIC_KEY_PATH}"
configureKubeletServerCert
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto moved this up earlier as well.

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 */}}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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)
  2. 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.

wait_for_file 1 1 $AZURE_JSON_PATH || return
fi

{{/* Perform the required JSON escaping */}}
SERVICE_PRINCIPAL_CLIENT_SECRET=${SERVICE_PRINCIPAL_CLIENT_SECRET//\\/\\\\}
SERVICE_PRINCIPAL_CLIENT_SECRET=${SERVICE_PRINCIPAL_CLIENT_SECRET//\"/\\\"}
Expand Down Expand Up @@ -218,16 +223,9 @@ configureK8s() {
EOF
set -x
if [[ "${CLOUDPROVIDER_BACKOFF_MODE}" = "v2" ]]; then
sed -i "/cloudProviderBackoffExponent/d" /etc/kubernetes/azure.json
sed -i "/cloudProviderBackoffJitter/d" /etc/kubernetes/azure.json
sed -i "/cloudProviderBackoffExponent/d" $AZURE_JSON_PATH
sed -i "/cloudProviderBackoffJitter/d" $AZURE_JSON_PATH
fi
if [[ -n "${MASTER_NODE}" ]]; then
if [[ "${ENABLE_AGGREGATED_APIS}" = True ]]; then
generateAggregatedAPICerts
fi
fi

configureKubeletServerCert
}

configureCNI() {
Expand Down
8 changes: 8 additions & 0 deletions parts/k8s/cloud-init/nodecustomdata.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
#cloud-config

write_files:
{{- if .RequiresCloudproviderConfig}}
mboersma marked this conversation as resolved.
Show resolved Hide resolved
- path: /etc/kubernetes/azure.json
permissions: "0600"
owner: root
content: |
#EOF
{{end}}

- path: {{GetCSEHelpersScriptFilepath}}
permissions: "0744"
encoding: gzip
Expand Down
9 changes: 1 addition & 8 deletions pkg/api/defaults-kubelet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
}
Expand Down Expand Up @@ -270,13 +270,6 @@ func removeKubeletFlags(k map[string]string, v string) {
delete(k, key)
}
}

// Get rid of keys with empty string values
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

for key, val := range k {
if val == "" {
delete(k, key)
}
}
}

func setMissingKubeletValues(p *KubernetesConfig, d map[string]string) {
Expand Down
75 changes: 75 additions & 0 deletions pkg/api/defaults-kubelet_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (

"github.com/Azure/aks-engine/pkg/api/common"
"github.com/Azure/aks-engine/pkg/helpers"
"github.com/google/go-cmp/cmp"
)

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

}

func TestRemoveKubeletFlags(t *testing.T) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This never had UT coverage

cases := []struct {
name string
kubeletConfig map[string]string
version string
expected map[string]string
}{
{
name: "v1.17.0",
kubeletConfig: map[string]string{
"--pod-max-pids": "100",
"--cadvisor-port": "1234",
"--allow-privileged": "true",
},
expected: map[string]string{
"--pod-max-pids": "100",
},
version: "1.17.0",
},
{
name: "v1.9.0",
kubeletConfig: map[string]string{
"--pod-max-pids": "100",
"--cadvisor-port": "1234",
"--allow-privileged": "true",
},
expected: map[string]string{
"--cadvisor-port": "1234",
"--allow-privileged": "true",
},
version: "1.9.0",
},
{
name: "v1.14.0",
kubeletConfig: map[string]string{
"--pod-max-pids": "100",
"--cadvisor-port": "1234",
"--allow-privileged": "true",
},
expected: map[string]string{
"--pod-max-pids": "100",
"--allow-privileged": "true",
},
version: "1.14.0",
},
{
name: "v1.11.0",
kubeletConfig: map[string]string{
"--pod-max-pids": "100",
"--cadvisor-port": "1234",
"--allow-privileged": "true",
},
expected: map[string]string{
"--pod-max-pids": "100",
"--cadvisor-port": "1234",
"--allow-privileged": "true",
},
version: "1.11.0",
},
}

for _, c := range cases {
c := c
t.Run(c.name, func(t *testing.T) {
t.Parallel()
removeKubeletFlags(c.kubeletConfig, c.version)
diff := cmp.Diff(c.kubeletConfig, c.expected)
if diff != "" {
t.Errorf("unexpected diff while expecting equal structs: %s", diff)
}
})
}
}
74 changes: 53 additions & 21 deletions pkg/api/defaults_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,16 +125,18 @@ func TestCertsAlreadyPresent(t *testing.T) {
func TestSetMissingKubeletValues(t *testing.T) {
config := &KubernetesConfig{}
defaultKubeletConfig := map[string]string{
"--network-plugin": "1",
"--pod-infra-container-image": "2",
"--max-pods": "3",
"--eviction-hard": "4",
"--node-status-update-frequency": "5",
"--image-gc-high-threshold": "6",
"--image-gc-low-threshold": "7",
"--non-masquerade-cidr": "8",
"--cloud-provider": "9",
"--pod-max-pids": "10",
"--network-plugin": "1",
"--pod-infra-container-image": "2",
"--max-pods": "3",
"--eviction-hard": "4",
"--node-status-update-frequency": "5",
"--image-gc-high-threshold": "6",
"--image-gc-low-threshold": "7",
"--non-masquerade-cidr": "8",
"--pod-max-pids": "10",
"--cloud-provider": "azure",
"--cloud-config": "/etc/kubernetes/azure.json",
"--azure-container-registry-config": "/etc/kubernetes/azure.json",
}
setMissingKubeletValues(config, defaultKubeletConfig)
for key, val := range defaultKubeletConfig {
Expand All @@ -147,20 +149,50 @@ func TestSetMissingKubeletValues(t *testing.T) {
KubeletConfig: map[string]string{
"--network-plugin": "a",
"--pod-infra-container-image": "b",
"--cloud-provider": "c",
"--cloud-provider": "",
},
}
expectedResult := map[string]string{
"--network-plugin": "a",
"--pod-infra-container-image": "b",
"--max-pods": "3",
"--eviction-hard": "4",
"--node-status-update-frequency": "5",
"--image-gc-high-threshold": "6",
"--image-gc-low-threshold": "7",
"--non-masquerade-cidr": "8",
"--cloud-provider": "c",
"--pod-max-pids": "10",
"--network-plugin": "a",
"--pod-infra-container-image": "b",
"--max-pods": "3",
"--eviction-hard": "4",
"--node-status-update-frequency": "5",
"--image-gc-high-threshold": "6",
"--image-gc-low-threshold": "7",
"--non-masquerade-cidr": "8",
"--pod-max-pids": "10",
"--cloud-provider": "",
"--cloud-config": "/etc/kubernetes/azure.json",
"--azure-container-registry-config": "/etc/kubernetes/azure.json",
}
setMissingKubeletValues(config, defaultKubeletConfig)
for key, val := range expectedResult {
if config.KubeletConfig[key] != val {
t.Fatalf("setMissingKubeletValue() did not return the expected value %s for key %s, instead returned: %s", val, key, config.KubeletConfig[key])
}
}

config = &KubernetesConfig{
KubeletConfig: map[string]string{
"--cloud-provider": "",
"--cloud-config": "",
"--azure-container-registry-config": "",
},
}
expectedResult = map[string]string{
"--network-plugin": "1",
"--pod-infra-container-image": "2",
"--max-pods": "3",
"--eviction-hard": "4",
"--node-status-update-frequency": "5",
"--image-gc-high-threshold": "6",
"--image-gc-low-threshold": "7",
"--non-masquerade-cidr": "8",
"--pod-max-pids": "10",
"--cloud-provider": "",
"--cloud-config": "",
"--azure-container-registry-config": "",
}
setMissingKubeletValues(config, defaultKubeletConfig)
for key, val := range expectedResult {
Expand Down
30 changes: 30 additions & 0 deletions pkg/api/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1638,6 +1638,36 @@ func (a *AgentPoolProfile) IsUbuntuNonVHD() bool {
return a.IsUbuntu() && !a.IsVHDDistro()
}

// 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 {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, I read the UT. :-)

if v != "" {
return true
}
} else {
return true
}
if v, ok := a.KubernetesConfig.KubeletConfig["--cloud-config"]; ok {
if v != "" {
return true
}
} else {
return true
}
if v, ok := a.KubernetesConfig.KubeletConfig["--azure-container-registry-config"]; ok {
if v != "" {
return true
}
} else {
return true
}
} else {
return true
}
return false
}

// GetKubernetesLabels returns a k8s API-compliant labels string for nodes in this profile
func (a *AgentPoolProfile) GetKubernetesLabels(rg string, deprecated bool) string {
var buf bytes.Buffer
Expand Down
85 changes: 85 additions & 0 deletions pkg/api/types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,91 @@ func TestAgentPoolProfileIsUbuntuNonVHD(t *testing.T) {
}
}

func TestRequiresCloudproviderConfig(t *testing.T) {
cases := []struct {
name string
ap AgentPoolProfile
expected bool
}{
{
name: "nil",
ap: AgentPoolProfile{},
expected: true,
},
{
name: "default",
ap: AgentPoolProfile{
KubernetesConfig: &KubernetesConfig{
KubeletConfig: map[string]string{},
},
},
expected: true,
},
{
name: "--cloud-provider provided",
ap: AgentPoolProfile{
KubernetesConfig: &KubernetesConfig{
KubeletConfig: map[string]string{
"--cloud-provider": "azure",
"--cloud-config": "",
"--azure-container-registry-config": "",
},
},
},
expected: true,
},
{
name: "--cloud-config provided",
ap: AgentPoolProfile{
KubernetesConfig: &KubernetesConfig{
KubeletConfig: map[string]string{
"--cloud-provider": "",
"--cloud-config": "/etc/kubernetes/azure.json",
"--azure-container-registry-config": "",
},
},
},
expected: true,
},
{
name: "--azure-container-registry-config provided",
ap: AgentPoolProfile{
KubernetesConfig: &KubernetesConfig{
KubeletConfig: map[string]string{
"--cloud-provider": "",
"--cloud-config": "",
"--azure-container-registry-config": "/etc/kubernetes/azure.json",
},
},
},
expected: true,
},
{
name: "all 3 flags set explicitly to empty string",
ap: AgentPoolProfile{
KubernetesConfig: &KubernetesConfig{
KubeletConfig: map[string]string{
"--cloud-provider": "",
"--cloud-config": "",
"--azure-container-registry-config": "",
},
},
},
expected: false,
},
}

for _, c := range cases {
c := c
t.Run(c.name, func(t *testing.T) {
t.Parallel()
if c.expected != c.ap.RequiresCloudproviderConfig() {
t.Fatalf("Got unexpected AgentPoolProfile.RequiresCloudproviderConfig() result. Expected: %t. Got: %t.", c.expected, c.ap.RequiresCloudproviderConfig())
}
})
}
}

func TestMasterProfileIsVHDDistro(t *testing.T) {
cases := []struct {
name string
Expand Down
Loading