generic Kubernetes API server config interface #2012

jackfrancis · 2018-01-08T21:00:11Z

What this PR does / why we need it: exposes a generic key/val interface for configuring apiserver, using the configuration keys documented here:

https://kubernetes.io/docs/reference/generated/kube-apiserver/

API model usage patterns will look like this:

"kubernetesConfig": {
    <...>
    "apiServerConfig": {
          "--request-timeout": "30s"
    }
    <...>
}

Release note:

add a generic Kubernetes API Server config interface

jackfrancis · 2018-01-08T21:07:31Z

docs/clusterdefinition.md

@@ -209,7 +209,7 @@ Below is a list of controller-manager options that acs-engine will configure by
 |"--route-reconciliation-period"|"10s"|


-Below is a list of kubelet options that are *not* currently user-configurable, either because a higher order configuration vector is available that enforces kubelet configuration, or because a static configuration is required to build a functional cluster:
+Below is a list of controller-manager options that are *not* currently user-configurable, either because a higher order configuration vector is available that enforces controller-manager configuration, or because a static configuration is required to build a functional cluster:


Correcting copy/paste errors from a prior, related PR.

jackfrancis · 2018-01-08T21:21:12Z

parts/k8s/kubernetesmastercustomdata.yml

-    perl -pi -e "s|--oidc-client-id=\K(?=\")|$OIDC_CLIENT_ID| || s|--oidc-issuer-url=\K(?=\")|$OIDC_ISSUER_URL|" "/etc/kubernetes/manifests/kube-apiserver.yaml"
-{{else}}
-    sed -i "/--oidc-client-id\|--oidc-issuer-url\|--oidc-username-claim/d" "/etc/kubernetes/manifests/kube-apiserver.yaml"
+    sed -i "/--oidc-issuer-url/s/$/$AAD_TENANT_ID/" "/etc/kubernetes/manifests/kube-apiserver.yaml"


We still need to do this post-deployment transformation because I'm not aware of any other way to implement the fallback tenantID assignment. The one here uses this kubernetesmastervars.t statement:

"tenantId": "[subscription().tenantId]",

Which gives us our fallback tenantId value (see AAD_TENANT_ID=${VAR_AAD_TENANT_ID:-$VAR_TENANT_ID} above). My understanding is that subscription() is an ARM function that we won't have access to anywhere else.

jackfrancis · 2018-01-08T21:32:42Z

parts/k8s/kubernetesmastercustomdata.yml

 {{if UseCloudControllerManager }}
    sed -i "s|<kubernetesCcmImageSpec>|{{WrapAsVariable "kubernetesCcmImageSpec"}}|g; s|<masterFqdnPrefix>|{{WrapAsVariable "masterFqdnPrefix"}}|g; s|<allocateNodeCidrs>|{{WrapAsVariable "allocateNodeCidrs"}}|g; s|<kubeClusterCidr>|{{WrapAsVariable "kubeClusterCidr"}}|g; s|<kubernetesCtrlMgrRouteReconciliationPeriod>|{{GetCloudControllerManagerRouteReconciliationPeriod .OrchestratorProfile.KubernetesConfig}}|g" \
      /etc/kubernetes/manifests/cloud-controller-manager.yaml
-
-    sed -i "/--\(cloud-config\|cloud-provider\|route-reconciliation-period\)=/d" \


This should have been removed in #1960, doing so now.

jackfrancis · 2018-01-08T21:33:06Z

pkg/acsengine/defaults-controller-manager.go

@@ -12,8 +13,6 @@ func setControllerManagerConfig(cs *api.ContainerService) {
 		"--kubeconfig":                       "/var/lib/kubelet/kubeconfig",
 		"--allocate-node-cidrs":              strconv.FormatBool(!o.IsAzureCNI()),
 		"--cluster-cidr":                     o.KubernetesConfig.ClusterSubnet,
-		"--cloud-provider":                   "azure",


This should have been done in #1960, doing so now.

jackfrancis · 2018-01-08T21:33:28Z

pkg/acsengine/defaults-controller-manager.go

@@ -30,6 +29,12 @@ func setControllerManagerConfig(cs *api.ContainerService) {
 		staticLinuxControllerManagerConfig["--cluster-name"] = cs.Properties.HostedMasterProfile.DNSPrefix
 	}

+	// Enable cloudprovider if we're not using cloud controller manager


This should have been done in #1960, doing so now.

jackfrancis · 2018-01-08T21:35:29Z

parts/k8s/kubernetesmastervars.t

@@ -267,8 +266,8 @@
      "[concat(variables('masterFirstAddrPrefix'), add(3, int(variables('masterFirstAddrOctet4'))))]",
      "[concat(variables('masterFirstAddrPrefix'), add(4, int(variables('masterFirstAddrOctet4'))))]"
    ],
-    "masterEtcdServerPort": 2380,
-    "masterEtcdClientPort": 2379,
+    "masterEtcdServerPort": "[parameters('masterEtcdServerPort')]",


Moved these vals to pkg/acsengine/const.go (and added appropriate changes elswhere).

jackfrancis · 2018-01-09T16:51:15Z

pkg/acsengine/engine.go

+			sort.Strings(keys)
+			var buf bytes.Buffer
+			for _, key := range keys {
+				buf.WriteString(fmt.Sprintf("\\\"%s=%s\\\", ", key, apiServerConfig[key]))


The escaped slashes \\ in front of the escaped quote \" is needed in order to produce a literal quote char in the resulting yaml file.

This should be done in the above getters as well, will do so in a follow-up PR.

JunSun17

Looks good to me, some comments are inline.

JunSun17 · 2018-01-09T18:52:46Z

parts/k8s/manifests/kubernetesmaster-cloud-controller-manager.yaml

@@ -21,7 +21,6 @@ spec:
        - "--cloud-config=/etc/kubernetes/azure.json"
        - "--leader-elect=true"
 # TODO: RBAC support


Do you want to remove RBAC placeholder here? Seems not related to this PR. If so, remove the above TODO comment too?

JunSun17 · 2018-01-09T19:13:03Z

pkg/acsengine/defaults-apiserver.go

+		overrideAPIServerConfig = staticLinuxAPIServerConfig
+	}
+	for key, val := range overrideAPIServerConfig {
+		o.KubernetesConfig.APIServerConfig[key] = val


Some argument like "--admission-control" and "--authorization-mode" takes a list of features, and probably should allow feature addition (as long as static config is a subset of the user input config), instead of override. It can be done in a later change if needed.

I agree this would be a useful addition to this surface area. Feature gates also are special, in that they essentially contain key/vals themselves. Yeah, I'd prefer a follow-up PR for that.

JunSun17 · 2018-01-09T19:51:14Z

parts/k8s/kubernetesmastercustomdata.yml

 {{end}}
    sed -i "s|<kubernetesControllerManagerConfig>|{{GetControllerManagerConfigKeyVals .OrchestratorProfile.KubernetesConfig}}|g" "/etc/kubernetes/manifests/kube-controller-manager.yaml"
+    sed -i "s|<kubernetesAPIServerConfig>|{{GetAPIServerConfigKeyVals .OrchestratorProfile.KubernetesConfig}}|g" "/etc/kubernetes/manifests/kube-apiserver.yaml"


It is probably better to take APIServerConfig (instead of KubernetesConfig) as argument for GetAPIServerConfigKeyVals.

The original code logic is pretty tangled here and I have not dig very deep, I assume GetAPIServerConfigKeyVals will return the already processed argument map. Please just confirm it.

This is good feedback. Let's do this: after all of these config refactors are done, we can optimize the number of key/val getter functions in engine.go (we have two types, I think, which is better than having one function per config file). A part of that improvement would be passing in the specific config as the arg instead of kubernetesConfig.

JunSun17 · 2018-01-09T19:53:21Z

parts/k8s/kubernetesmastercustomdata.yml

+    VAR_AAD_TENANT_ID={{WrapAsVariable "aadTenantId"}}
+    VAR_TENANT_ID={{WrapAsVariable "tenantId"}}
+    AAD_TENANT_ID=${VAR_AAD_TENANT_ID:-$VAR_TENANT_ID}
+    sed -i "/--oidc-issuer-url/s/$/$AAD_TENANT_ID/" "/etc/kubernetes/manifests/kube-apiserver.yaml"


If the above GetAPIServerConfigKeyVals returns already processed map, why are we overriding "--oidc-issuer-url" here?

Good question. It's because the only way (that I know of) to assign the right tenantId value is in the flow of this template: we depend upon {{WrapAsVariable "tenantId"}} as a fallback value, and that fallback value is not available in the flow of the map-producing code. See the subscription() method which the tenantId variable depends upon in kubernetesmastervars.t.

generic apiserver config

e19a6f1

ghost assigned jackfrancis Jan 8, 2018

ghost added the in progress label Jan 8, 2018

jackfrancis commented Jan 8, 2018

View reviewed changes

jackfrancis added 2 commits January 8, 2018 13:09

add spaces for visual improvement

704e9dc

rationalize aad vars

01422ac

jackfrancis commented Jan 8, 2018

View reviewed changes

same ccm config approach for both apiserver and controller-manager

6f052c5

jackfrancis commented Jan 8, 2018

View reviewed changes

jackfrancis added 5 commits January 8, 2018 13:52

reverting changes to masterEtcdServerPort and masterEtcdClientPort

33d9b33

etcd client port is static 2397

f0817ac

moving sed bits around

72e149d

fun w/ sed

5b49dae

humans are the best

74e9471

jackfrancis commented Jan 9, 2018

View reviewed changes

using vars for etcd master ports

f40d7c0

JunSun17 approved these changes Jan 9, 2018

View reviewed changes

jackfrancis merged commit d216b94 into Azure:master Jan 9, 2018

ghost removed the in progress label Jan 9, 2018

jackfrancis deleted the generic-apiserver branch January 9, 2018 20:12

CecileRobertMichon mentioned this pull request Jan 10, 2018

Add support for kubernetes --feature-gates in template #2021

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

generic Kubernetes API server config interface #2012

generic Kubernetes API server config interface #2012

jackfrancis commented Jan 8, 2018

jackfrancis Jan 8, 2018

jackfrancis Jan 8, 2018

jackfrancis Jan 8, 2018

jackfrancis Jan 8, 2018 •

edited

jackfrancis Jan 8, 2018

jackfrancis Jan 8, 2018

jackfrancis Jan 9, 2018

JunSun17 left a comment

JunSun17 Jan 9, 2018

JunSun17 Jan 9, 2018

jackfrancis Jan 9, 2018

JunSun17 Jan 9, 2018

jackfrancis Jan 9, 2018

JunSun17 Jan 9, 2018

jackfrancis Jan 9, 2018

generic Kubernetes API server config interface #2012

generic Kubernetes API server config interface #2012

Conversation

jackfrancis commented Jan 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfrancis Jan 8, 2018 • 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

JunSun17 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

jackfrancis Jan 8, 2018 •

edited