feat: add support for private registry #523

ritazh · 2019-02-15T20:14:29Z

Reason for Change:

Add support to pull custom hyperkube image from private registry

Issue Fixed:

Fixes #271
This should also fix the broken cloud-provider-azure prow job as that uses ACR.

Requirements:

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

Notes:

cc @feiskyer

codecov · 2019-02-15T20:31:57Z

Codecov Report

Merging #523 into master will decrease coverage by <.01%.
The diff coverage is 68.75%.

@@            Coverage Diff             @@
##           master     #523      +/-   ##
==========================================
- Coverage   54.56%   54.55%   -0.01%     
==========================================
  Files          97       97              
  Lines       14622    14614       -8     
==========================================
- Hits         7978     7973       -5     
+ Misses       5971     5969       -2     
+ Partials      673      672       -1

jackfrancis · 2019-02-15T21:25:04Z

pkg/engine/params_k8s.go

@@ -46,6 +46,7 @@ func assignKubernetesParameters(properties *api.Properties, parametersMap params

 			addValue(parametersMap, "kubeDNSServiceIP", kubernetesConfig.DNSServiceIP)
 			addValue(parametersMap, "kubernetesHyperkubeSpec", kubernetesHyperkubeSpec)
+			addValue(parametersMap, "privateRegistryServer", kubernetesConfig.PrivateRegistryServer)


Should we only add the param if it's not empty string?

jackfrancis · 2019-02-15T21:25:46Z

parts/k8s/kubernetesinstalls.sh

@@ -273,6 +273,11 @@ installKubeletAndKubectl() {
 pullContainerImage() {
    CLI_TOOL=$1
    DOCKER_IMAGE_URL=$2
+    if [ ! -z "${PRIVATE_REGISTRY_SERVER}" ]; then
+        if [[ "$CLI_TOOL" == "docker" ]]; then
+            $CLI_TOOL login -u $SERVICE_PRINCIPAL_CLIENT_ID -p $SERVICE_PRINCIPAL_CLIENT_SECRET $PRIVATE_REGISTRY_SERVER


Does this in practice not work w/ img?

Seems like it should:

https://github.com/genuinetools/img

In any event, if img login doesn't work in this context, we'll want to document something like the following:

"Private registry support is for default docker (moby) runtime only. It is not supported for non-docker cluster configuration scenarios, e.g., containerd, clear-containers, kata-containers."

Added support for img as well.

jackfrancis · 2019-02-15T22:12:25Z

docs/topics/kubernetes-developers.md

+```
+"kubernetesConfig": {
+    "customHyperkubeImage": "<your-private-registry>/hyperkube-amd64:<your-custom-version>",
+    "privateRegistryServer": "<your-private-registry>"


Does privateRegistryServer make any sense in a context when customHyperkubeImage is not provided? If not, we should probably add validation.

jackfrancis · 2019-02-15T23:06:02Z

Final high-level comment: because privateRegistryServer seems to be implemented in a MS-specific context (i.e., service principals are accepted auth), should be call this privateMSRegistryServer, or something like that? I don't see this implementation working with any old private image repository, or am I missing something?

ritazh · 2019-02-15T23:30:43Z

@jackfrancis In theory this code can be expanded to work for other private container repositories as well, but the real reason is that only azure credentials can actually pull this image from ACR if you are using the Kubernetes Azure cloud provider. So I will update this variable to reflect that.

Should we change it to privateMSRegistryServer or privateAzureRegistryServer?

jackfrancis · 2019-02-15T23:31:49Z

How about just azureRegistryServer?

ritazh · 2019-02-15T23:39:44Z

Can we keep private as I think there are plans to enable public ACR in the future? WDYT?

jackfrancis · 2019-02-16T00:50:20Z

/lgtm

acs-bot · 2019-02-16T00:51:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis, ritazh

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:

~~OWNERS~~ [jackfrancis]

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

jsturtevant · 2019-02-16T01:08:20Z

pkg/api/vlabs/validate.go

+		}
+	} else {
+		valid = true
+	}


I found this logic hard to read. would something like the following be more more readable? I think that would remove the need for the valid check

k.PrivateAzureRegistryServer != "" and k.CustomHyperkubeImage == "" { return errors.Errorf("customHyperkubeImage must be provided when privateAzureRegistryServer is provided") }

jsturtevant · 2019-02-16T01:08:25Z

parts/k8s/kubernetesinstalls.sh

@@ -278,6 +278,9 @@ installKubeletAndKubectl() {
 pullContainerImage() {
    CLI_TOOL=$1
    DOCKER_IMAGE_URL=$2
+    if [ ! -z "${PRIVATE_AZURE_REGISTRY_SERVER}" ]; then
+        $CLI_TOOL login -u $SERVICE_PRINCIPAL_CLIENT_ID -p $SERVICE_PRINCIPAL_CLIENT_SECRET $PRIVATE_AZURE_REGISTRY_SERVER


should this use --password-stdin here? https://docs.docker.com/engine/reference/commandline/login/#provide-a-password-using-stdin

tariq1890 · 2019-02-20T00:32:46Z

parts/k8s/kubernetesmastervars.t

@@ -145,7 +145,7 @@
    "mountetcdScript": "{{GetKubernetesB64Mountetcd}}",
    "customSearchDomainsScript": "{{GetKubernetesB64CustomSearchDomainsScript}}",
    "sshdConfig": "{{GetB64sshdConfig}}",
-    "provisionScriptParametersCommon": "[concat('ADMINUSER=',parameters('linuxAdminUsername'),' ETCD_DOWNLOAD_URL=',parameters('etcdDownloadURLBase'),' ETCD_VERSION=',parameters('etcdVersion'),' CONTAINERD_VERSION=',parameters('containerdVersion'),' MOBY_VERSION=',parameters('mobyVersion'),' DOCKER_ENGINE_REPO=',parameters('dockerEngineDownloadRepo'),' TENANT_ID=',variables('tenantID'),' KUBERNETES_VERSION={{.OrchestratorProfile.OrchestratorVersion}} HYPERKUBE_URL=',parameters('kubernetesHyperkubeSpec'),' APISERVER_PUBLIC_KEY=',parameters('apiserverCertificate'),' SUBSCRIPTION_ID=',variables('subscriptionId'),' RESOURCE_GROUP=',variables('resourceGroup'),' LOCATION=',variables('location'),' VM_TYPE=',variables('vmType'),' SUBNET=',variables('subnetName'),' NETWORK_SECURITY_GROUP=',variables('nsgName'),' VIRTUAL_NETWORK=',variables('virtualNetworkName'),' VIRTUAL_NETWORK_RESOURCE_GROUP=',variables('virtualNetworkResourceGroupName'),' ROUTE_TABLE=',variables('routeTableName'),' PRIMARY_AVAILABILITY_SET=',variables('primaryAvailabilitySetName'),' PRIMARY_SCALE_SET=',variables('primaryScaleSetName'),' SERVICE_PRINCIPAL_CLIENT_ID=',variables('servicePrincipalClientId'),' SERVICE_PRINCIPAL_CLIENT_SECRET=',variables('singleQuote'),variables('servicePrincipalClientSecret'),variables('singleQuote'),' KUBELET_PRIVATE_KEY=',parameters('clientPrivateKey'),' TARGET_ENVIRONMENT=',parameters('targetEnvironment'),' NETWORK_PLUGIN=',parameters('networkPlugin'),' NETWORK_POLICY=',parameters('networkPolicy'),' VNET_CNI_PLUGINS_URL=',parameters('vnetCniLinuxPluginsURL'),' CNI_PLUGINS_URL=',parameters('cniPluginsURL'),' CLOUDPROVIDER_BACKOFF=',toLower(string(parameters('cloudproviderConfig').cloudProviderBackoff)),' CLOUDPROVIDER_BACKOFF_RETRIES=',parameters('cloudproviderConfig').cloudProviderBackoffRetries,' CLOUDPROVIDER_BACKOFF_EXPONENT=',parameters('cloudproviderConfig').cloudProviderBackoffExponent,' CLOUDPROVIDER_BACKOFF_DURATION=',parameters('cloudproviderConfig').cloudProviderBackoffDuration,' CLOUDPROVIDER_BACKOFF_JITTER=',parameters('cloudproviderConfig').cloudProviderBackoffJitter,' CLOUDPROVIDER_RATELIMIT=',toLower(string(parameters('cloudproviderConfig').cloudProviderRatelimit)),' CLOUDPROVIDER_RATELIMIT_QPS=',parameters('cloudproviderConfig').cloudProviderRatelimitQPS,' CLOUDPROVIDER_RATELIMIT_BUCKET=',parameters('cloudproviderConfig').cloudProviderRatelimitBucket,' USE_MANAGED_IDENTITY_EXTENSION=',variables('useManagedIdentityExtension'),' USER_ASSIGNED_IDENTITY_ID=',variables('userAssignedClientID'),' USE_INSTANCE_METADATA=',variables('useInstanceMetadata'),' LOAD_BALANCER_SKU=',variables('loadBalancerSku'),' EXCLUDE_MASTER_FROM_STANDARD_LB=',variables('excludeMasterFromStandardLB'),' MAXIMUM_LOADBALANCER_RULE_COUNT=',variables('maximumLoadBalancerRuleCount'),' CONTAINER_RUNTIME=',parameters('containerRuntime'),' CONTAINERD_DOWNLOAD_URL_BASE=',parameters('containerdDownloadURLBase'),' POD_INFRA_CONTAINER_SPEC=',parameters('kubernetesPodInfraContainerSpec'),' KMS_PROVIDER_VAULT_NAME=',variables('clusterKeyVaultName'),' IS_HOSTED_MASTER={{IsHostedMaster}}')]",
+    "provisionScriptParametersCommon": "[concat('ADMINUSER=',parameters('linuxAdminUsername'),' ETCD_DOWNLOAD_URL=',parameters('etcdDownloadURLBase'),' ETCD_VERSION=',parameters('etcdVersion'),' CONTAINERD_VERSION=',parameters('containerdVersion'),' MOBY_VERSION=',parameters('mobyVersion'),' DOCKER_ENGINE_REPO=',parameters('dockerEngineDownloadRepo'),' TENANT_ID=',variables('tenantID'),' KUBERNETES_VERSION={{.OrchestratorProfile.OrchestratorVersion}} HYPERKUBE_URL=',parameters('kubernetesHyperkubeSpec'),' APISERVER_PUBLIC_KEY=',parameters('apiserverCertificate'),' SUBSCRIPTION_ID=',variables('subscriptionId'),' RESOURCE_GROUP=',variables('resourceGroup'),' LOCATION=',variables('location'),' VM_TYPE=',variables('vmType'),' SUBNET=',variables('subnetName'),' NETWORK_SECURITY_GROUP=',variables('nsgName'),' VIRTUAL_NETWORK=',variables('virtualNetworkName'),' VIRTUAL_NETWORK_RESOURCE_GROUP=',variables('virtualNetworkResourceGroupName'),' ROUTE_TABLE=',variables('routeTableName'),' PRIMARY_AVAILABILITY_SET=',variables('primaryAvailabilitySetName'),' PRIMARY_SCALE_SET=',variables('primaryScaleSetName'),' SERVICE_PRINCIPAL_CLIENT_ID=',variables('servicePrincipalClientId'),' SERVICE_PRINCIPAL_CLIENT_SECRET=',variables('singleQuote'),variables('servicePrincipalClientSecret'),variables('singleQuote'),' KUBELET_PRIVATE_KEY=',parameters('clientPrivateKey'),' TARGET_ENVIRONMENT=',parameters('targetEnvironment'),' NETWORK_PLUGIN=',parameters('networkPlugin'),' NETWORK_POLICY=',parameters('networkPolicy'),' VNET_CNI_PLUGINS_URL=',parameters('vnetCniLinuxPluginsURL'),' CNI_PLUGINS_URL=',parameters('cniPluginsURL'),' CLOUDPROVIDER_BACKOFF=',toLower(string(parameters('cloudproviderConfig').cloudProviderBackoff)),' CLOUDPROVIDER_BACKOFF_RETRIES=',parameters('cloudproviderConfig').cloudProviderBackoffRetries,' CLOUDPROVIDER_BACKOFF_EXPONENT=',parameters('cloudproviderConfig').cloudProviderBackoffExponent,' CLOUDPROVIDER_BACKOFF_DURATION=',parameters('cloudproviderConfig').cloudProviderBackoffDuration,' CLOUDPROVIDER_BACKOFF_JITTER=',parameters('cloudproviderConfig').cloudProviderBackoffJitter,' CLOUDPROVIDER_RATELIMIT=',toLower(string(parameters('cloudproviderConfig').cloudProviderRatelimit)),' CLOUDPROVIDER_RATELIMIT_QPS=',parameters('cloudproviderConfig').cloudProviderRatelimitQPS,' CLOUDPROVIDER_RATELIMIT_BUCKET=',parameters('cloudproviderConfig').cloudProviderRatelimitBucket,' USE_MANAGED_IDENTITY_EXTENSION=',variables('useManagedIdentityExtension'),' USER_ASSIGNED_IDENTITY_ID=',variables('userAssignedClientID'),' USE_INSTANCE_METADATA=',variables('useInstanceMetadata'),' LOAD_BALANCER_SKU=',variables('loadBalancerSku'),' EXCLUDE_MASTER_FROM_STANDARD_LB=',variables('excludeMasterFromStandardLB'),' MAXIMUM_LOADBALANCER_RULE_COUNT=',variables('maximumLoadBalancerRuleCount'),' CONTAINER_RUNTIME=',parameters('containerRuntime'),' CONTAINERD_DOWNLOAD_URL_BASE=',parameters('containerdDownloadURLBase'),' POD_INFRA_CONTAINER_SPEC=',parameters('kubernetesPodInfraContainerSpec'),' KMS_PROVIDER_VAULT_NAME=',variables('clusterKeyVaultName'),' IS_HOSTED_MASTER={{IsHostedMaster}} PRIVATE_AZURE_REGISTRY_SERVER=',parameters('privateAzureRegistryServer'))]",


This should have been separated by a comma. I'll fix this in another PR.

acs-bot added the size/M label Feb 15, 2019

jackfrancis reviewed Feb 15, 2019

View reviewed changes

acs-bot added size/L and removed size/M labels Feb 15, 2019

ritazh added 2 commits February 15, 2019 16:38

feat: add support for private registry

1646f89

regen templates_generated.go

a13db9c

ritazh force-pushed the feat-privateregistry branch from d6fad33 to a13db9c Compare February 16, 2019 00:48

acs-bot assigned jackfrancis Feb 16, 2019

acs-bot added the lgtm label Feb 16, 2019

acs-bot added the approved label Feb 16, 2019

jackfrancis merged commit 7e175c8 into Azure:master Feb 16, 2019

jackfrancis pushed a commit that referenced this pull request Feb 16, 2019

feat: add support for private registry (#523)

18ea90a

jsturtevant reviewed Feb 16, 2019

View reviewed changes

ritazh mentioned this pull request Feb 17, 2019

Bump aks-engine to v0.31.1 to support private ACR kubernetes-sigs/cloud-provider-azure#95

Merged

tariq1890 reviewed Feb 20, 2019

View reviewed changes

juhacket pushed a commit to juhacket/aks-engine that referenced this pull request Mar 14, 2019

feat: add support for private registry (Azure#523)

a151f18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for private registry #523

feat: add support for private registry #523

ritazh commented Feb 15, 2019 •

edited

Loading

codecov bot commented Feb 15, 2019 •

edited

Loading

jackfrancis Feb 15, 2019

jackfrancis Feb 15, 2019

jackfrancis Feb 15, 2019

jackfrancis Feb 15, 2019

ritazh Feb 15, 2019

jackfrancis Feb 15, 2019

jackfrancis commented Feb 15, 2019

ritazh commented Feb 15, 2019

jackfrancis commented Feb 15, 2019

ritazh commented Feb 15, 2019

jackfrancis commented Feb 16, 2019

acs-bot commented Feb 16, 2019

jsturtevant Feb 16, 2019

jsturtevant Feb 16, 2019

tariq1890 Feb 20, 2019

feat: add support for private registry #523

feat: add support for private registry #523

Conversation

ritazh commented Feb 15, 2019 • edited Loading

codecov bot commented Feb 15, 2019 • 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

jackfrancis commented Feb 15, 2019

ritazh commented Feb 15, 2019

jackfrancis commented Feb 15, 2019

ritazh commented Feb 15, 2019

jackfrancis commented Feb 16, 2019

acs-bot commented Feb 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ritazh commented Feb 15, 2019 •

edited

Loading

codecov bot commented Feb 15, 2019 •

edited

Loading