From 02feeea16979751b7796c4d94eca0209618702a1 Mon Sep 17 00:00:00 2001 From: Alan J Castonguay Date: Mon, 28 Jan 2019 20:56:32 -0500 Subject: [PATCH 1/9] Use AKS Service Principal credentials for ACR Azure Kubernetes Service (AKS) nodes' kubelet can use the cluster's service principal to fetch images from Azure Container Registry (ACR). If azure.json is projected into the Flux pod, consume the service principal credentials when authenticating to azure container registry. Note that this method may eventually be deprecated, possibly replaced by Managed Identity + OAuth, similar to the GCP implementation. See https://github.com/kubernetes/kubernetes/issues/58034 --- registry/azure.go | 55 +++++++++++++++++++++++++++++++++++++++++ registry/credentials.go | 7 ++++++ 2 files changed, 62 insertions(+) create mode 100644 registry/azure.go diff --git a/registry/azure.go b/registry/azure.go new file mode 100644 index 000000000..52726edf9 --- /dev/null +++ b/registry/azure.go @@ -0,0 +1,55 @@ +package registry + +import ( + "encoding/json" + "os" + yaml "gopkg.in/yaml.v2" +) + +const ( + // Mount volume from hostpath. + azureCloudConfigJsonFile = "/etc/kubernetes/azure.json" + + // List from https://github.com/kubernetes/kubernetes/blob/master/pkg/credentialprovider/azure/azure_credentials.go + azureContainerRegistryHosts = []string{".azurecr.io", ".azurecr.cn", ".azurecr.de", ".azurecr.us"}; +) + +type azureCloudConfig struct { + aadClientId string `json:"aadClientId"` + aadClientSecret string `json:"aadClientSecret"` +} + +// Fetch Azure Active Directory clientid/secret pair from azure.json, usable for container registry authentication. +// +// Note: azure.json is populated by AKS/AKS-Engine script kubernetesconfigs.sh. The file is then passed to kubelet via +// --azure-container-registry-config=/etc/kubernetes/azure.json, parsed by kubernetes/kubernetes' azure_credentials.go +// https://github.com/kubernetes/kubernetes/issues/58034 seeks to deprecate this kubelet command-line argument, possibly +// replacing it with managed identity for the Node VMs. See https://github.com/Azure/acr/blob/master/docs/AAD-OAuth.md +func GetAzureCloudConfigAADToken(host string) (creds, error) { + jsonFile, err := os.Open(azureCloudConfigJsonFile) + if err != nil { + return creds{}, err + } + defer jsonFile.Close() + + var token azureCloudConfig + decoder := json.NewDecoder(jsonFile) + if err := decoder.Decode(&token); err != nil { + return creds{}, err + } + + return creds{ + registry: host, + provenance: "azure.json", + username: token.aadClientId, + password: token.aadClientSecret}, nil +} + +func HostIsAzureContainerRegistry(host string) (bool) { + for _, v := range azureContainerRegistryHosts { + if strings.HasSuffix(host, v) { + return true + } + } + return false +} diff --git a/registry/credentials.go b/registry/credentials.go index 278568bca..aaa5d5c0c 100644 --- a/registry/credentials.go +++ b/registry/credentials.go @@ -144,6 +144,13 @@ func (cs Credentials) credsFor(host string) creds { return cred } } + + if HostIsAzureContainerRegistry(host) { + if cred, err := GetAzureCloudConfigAADToken(host); err == nil { + return cred + } + } + return creds{} } From a351ad1c84e916d6219afcd263b55d0e1a9b0288 Mon Sep 17 00:00:00 2001 From: Alan J Castonguay Date: Mon, 28 Jan 2019 21:32:46 -0500 Subject: [PATCH 2/9] Fix imports --- registry/azure.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/registry/azure.go b/registry/azure.go index 52726edf9..3d65dc3e2 100644 --- a/registry/azure.go +++ b/registry/azure.go @@ -3,7 +3,7 @@ package registry import ( "encoding/json" "os" - yaml "gopkg.in/yaml.v2" + "strings" ) const ( From b210248d0b9d7afbe28729334190e8ad1d968e12 Mon Sep 17 00:00:00 2001 From: Alan J Castonguay Date: Mon, 28 Jan 2019 21:33:22 -0500 Subject: [PATCH 3/9] A []string map can't be declared const. --- registry/azure.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/registry/azure.go b/registry/azure.go index 3d65dc3e2..d699ba3b0 100644 --- a/registry/azure.go +++ b/registry/azure.go @@ -9,9 +9,6 @@ import ( const ( // Mount volume from hostpath. azureCloudConfigJsonFile = "/etc/kubernetes/azure.json" - - // List from https://github.com/kubernetes/kubernetes/blob/master/pkg/credentialprovider/azure/azure_credentials.go - azureContainerRegistryHosts = []string{".azurecr.io", ".azurecr.cn", ".azurecr.de", ".azurecr.us"}; ) type azureCloudConfig struct { @@ -45,8 +42,9 @@ func GetAzureCloudConfigAADToken(host string) (creds, error) { password: token.aadClientSecret}, nil } +// List from https://github.com/kubernetes/kubernetes/blob/master/pkg/credentialprovider/azure/azure_credentials.go func HostIsAzureContainerRegistry(host string) (bool) { - for _, v := range azureContainerRegistryHosts { + for _, v := range []string{".azurecr.io", ".azurecr.cn", ".azurecr.de", ".azurecr.us"} { if strings.HasSuffix(host, v) { return true } From 63f148b09b2de281cc16e1a8dfd78eef38a5f26d Mon Sep 17 00:00:00 2001 From: Alan J Castonguay Date: Mon, 28 Jan 2019 22:05:45 -0500 Subject: [PATCH 4/9] Use ioutil.ReadFile higher-level abstraction --- registry/azure.go | 18 +++++++------- registry/azure_test.go | 56 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 9 deletions(-) create mode 100644 registry/azure_test.go diff --git a/registry/azure.go b/registry/azure.go index d699ba3b0..50131a5da 100644 --- a/registry/azure.go +++ b/registry/azure.go @@ -2,7 +2,7 @@ package registry import ( "encoding/json" - "os" + "io/ioutil" "strings" ) @@ -12,8 +12,8 @@ const ( ) type azureCloudConfig struct { - aadClientId string `json:"aadClientId"` - aadClientSecret string `json:"aadClientSecret"` + AADClientId string `json:"aadClientId"` + AADClientSecret string `json:"aadClientSecret"` } // Fetch Azure Active Directory clientid/secret pair from azure.json, usable for container registry authentication. @@ -23,23 +23,23 @@ type azureCloudConfig struct { // https://github.com/kubernetes/kubernetes/issues/58034 seeks to deprecate this kubelet command-line argument, possibly // replacing it with managed identity for the Node VMs. See https://github.com/Azure/acr/blob/master/docs/AAD-OAuth.md func GetAzureCloudConfigAADToken(host string) (creds, error) { - jsonFile, err := os.Open(azureCloudConfigJsonFile) + jsonFile, err := ioutil.ReadFile(azureCloudConfigJsonFile) if err != nil { return creds{}, err } - defer jsonFile.Close() var token azureCloudConfig - decoder := json.NewDecoder(jsonFile) - if err := decoder.Decode(&token); err != nil { + + err = json.Unmarshal(jsonFile, &token) + if err != nil { return creds{}, err } return creds{ registry: host, provenance: "azure.json", - username: token.aadClientId, - password: token.aadClientSecret}, nil + username: token.AADClientId, + password: token.AADClientSecret}, nil } // List from https://github.com/kubernetes/kubernetes/blob/master/pkg/credentialprovider/azure/azure_credentials.go diff --git a/registry/azure_test.go b/registry/azure_test.go new file mode 100644 index 000000000..d5ad822c4 --- /dev/null +++ b/registry/azure_test.go @@ -0,0 +1,56 @@ +package registry + +import ( + "testing" +) + +func Test_HostIsAzureContainerRegistry(t *testing.T) { + for _, v := range []struct { + host string + isACR bool + }{ + { + host: "azurecr.io", + isACR: false, + }, + { + host: "", + isACR: false, + }, + { + host: "gcr.io", + isACR: false, + }, + { + host: "notazurecr.io", + isACR: false, + }, + { + host: "example.azurecr.io.not", + isACR: false, + }, + // Public cloud + { + host: "example.azurecr.io", + isACR: true, + }, + // Sovereign clouds + { + host: "example.azurecr.cn", + isACR: true, + }, + { + host: "example.azurecr.de", + isACR: true, + }, + { + host: "example.azurecr.us", + isACR: true, + }, + } { + result := HostIsAzureContainerRegistry(v.host) + if result != v.isACR { + t.Fatalf("For test %q, expected isACR = %v but got %v", v.host, v.isACR, result) + } + } +} From 3cb2179a74e617de69767979582fa4bfb7d21e3f Mon Sep 17 00:00:00 2001 From: Alan J Castonguay Date: Fri, 8 Feb 2019 13:34:41 -0500 Subject: [PATCH 5/9] gofmt only --- registry/azure.go | 56 +++++++++++++++++++++--------------------- registry/azure_test.go | 4 +-- 2 files changed, 30 insertions(+), 30 deletions(-) diff --git a/registry/azure.go b/registry/azure.go index 50131a5da..24968b72c 100644 --- a/registry/azure.go +++ b/registry/azure.go @@ -7,13 +7,13 @@ import ( ) const ( - // Mount volume from hostpath. - azureCloudConfigJsonFile = "/etc/kubernetes/azure.json" + // Mount volume from hostpath. + azureCloudConfigJsonFile = "/etc/kubernetes/azure.json" ) type azureCloudConfig struct { - AADClientId string `json:"aadClientId"` - AADClientSecret string `json:"aadClientSecret"` + AADClientId string `json:"aadClientId"` + AADClientSecret string `json:"aadClientSecret"` } // Fetch Azure Active Directory clientid/secret pair from azure.json, usable for container registry authentication. @@ -23,31 +23,31 @@ type azureCloudConfig struct { // https://github.com/kubernetes/kubernetes/issues/58034 seeks to deprecate this kubelet command-line argument, possibly // replacing it with managed identity for the Node VMs. See https://github.com/Azure/acr/blob/master/docs/AAD-OAuth.md func GetAzureCloudConfigAADToken(host string) (creds, error) { - jsonFile, err := ioutil.ReadFile(azureCloudConfigJsonFile) - if err != nil { - return creds{}, err - } - - var token azureCloudConfig - - err = json.Unmarshal(jsonFile, &token) - if err != nil { - return creds{}, err - } - - return creds{ - registry: host, - provenance: "azure.json", - username: token.AADClientId, - password: token.AADClientSecret}, nil + jsonFile, err := ioutil.ReadFile(azureCloudConfigJsonFile) + if err != nil { + return creds{}, err + } + + var token azureCloudConfig + + err = json.Unmarshal(jsonFile, &token) + if err != nil { + return creds{}, err + } + + return creds{ + registry: host, + provenance: "azure.json", + username: token.AADClientId, + password: token.AADClientSecret}, nil } // List from https://github.com/kubernetes/kubernetes/blob/master/pkg/credentialprovider/azure/azure_credentials.go -func HostIsAzureContainerRegistry(host string) (bool) { - for _, v := range []string{".azurecr.io", ".azurecr.cn", ".azurecr.de", ".azurecr.us"} { - if strings.HasSuffix(host, v) { - return true - } - } - return false +func HostIsAzureContainerRegistry(host string) bool { + for _, v := range []string{".azurecr.io", ".azurecr.cn", ".azurecr.de", ".azurecr.us"} { + if strings.HasSuffix(host, v) { + return true + } + } + return false } diff --git a/registry/azure_test.go b/registry/azure_test.go index d5ad822c4..206577af3 100644 --- a/registry/azure_test.go +++ b/registry/azure_test.go @@ -6,8 +6,8 @@ import ( func Test_HostIsAzureContainerRegistry(t *testing.T) { for _, v := range []struct { - host string - isACR bool + host string + isACR bool }{ { host: "azurecr.io", From df5ca66fa9a69b9199c1b4bc442a0bb07fca21d3 Mon Sep 17 00:00:00 2001 From: Alan J Castonguay Date: Fri, 8 Feb 2019 13:37:20 -0500 Subject: [PATCH 6/9] Make previously-Exported ACR-centric functions private to the package. --- registry/azure.go | 4 ++-- registry/azure_test.go | 2 +- registry/credentials.go | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/registry/azure.go b/registry/azure.go index 24968b72c..61884813b 100644 --- a/registry/azure.go +++ b/registry/azure.go @@ -22,7 +22,7 @@ type azureCloudConfig struct { // --azure-container-registry-config=/etc/kubernetes/azure.json, parsed by kubernetes/kubernetes' azure_credentials.go // https://github.com/kubernetes/kubernetes/issues/58034 seeks to deprecate this kubelet command-line argument, possibly // replacing it with managed identity for the Node VMs. See https://github.com/Azure/acr/blob/master/docs/AAD-OAuth.md -func GetAzureCloudConfigAADToken(host string) (creds, error) { +func getAzureCloudConfigAADToken(host string) (creds, error) { jsonFile, err := ioutil.ReadFile(azureCloudConfigJsonFile) if err != nil { return creds{}, err @@ -43,7 +43,7 @@ func GetAzureCloudConfigAADToken(host string) (creds, error) { } // List from https://github.com/kubernetes/kubernetes/blob/master/pkg/credentialprovider/azure/azure_credentials.go -func HostIsAzureContainerRegistry(host string) bool { +func hostIsAzureContainerRegistry(host string) bool { for _, v := range []string{".azurecr.io", ".azurecr.cn", ".azurecr.de", ".azurecr.us"} { if strings.HasSuffix(host, v) { return true diff --git a/registry/azure_test.go b/registry/azure_test.go index 206577af3..7a670c10e 100644 --- a/registry/azure_test.go +++ b/registry/azure_test.go @@ -48,7 +48,7 @@ func Test_HostIsAzureContainerRegistry(t *testing.T) { isACR: true, }, } { - result := HostIsAzureContainerRegistry(v.host) + result := hostIsAzureContainerRegistry(v.host) if result != v.isACR { t.Fatalf("For test %q, expected isACR = %v but got %v", v.host, v.isACR, result) } diff --git a/registry/credentials.go b/registry/credentials.go index aaa5d5c0c..b8c49616c 100644 --- a/registry/credentials.go +++ b/registry/credentials.go @@ -145,8 +145,8 @@ func (cs Credentials) credsFor(host string) creds { } } - if HostIsAzureContainerRegistry(host) { - if cred, err := GetAzureCloudConfigAADToken(host); err == nil { + if hostIsAzureContainerRegistry(host) { + if cred, err := getAzureCloudConfigAADToken(host); err == nil { return cred } } From 0ce0668ff002be8ad1b07974892a8246e6cd04d4 Mon Sep 17 00:00:00 2001 From: Alan J Castonguay Date: Fri, 8 Feb 2019 14:08:06 -0500 Subject: [PATCH 7/9] Add AKS-ACR SP HostPath mount to helm chart. --- chart/flux/README.md | 2 ++ chart/flux/templates/deployment.yaml | 11 +++++++++++ chart/flux/values.yaml | 4 ++++ 3 files changed, 17 insertions(+) diff --git a/chart/flux/README.md b/chart/flux/README.md index b33454d02..b5c0c7b49 100755 --- a/chart/flux/README.md +++ b/chart/flux/README.md @@ -221,6 +221,8 @@ The following tables lists the configurable parameters of the Weave Flux chart a | `registry.ecr.region` | Restrict ECR scanning to these AWS regions; if empty, only the cluster's region will be scanned | `None` | `registry.ecr.includeId` | Restrict ECR scanning to these AWS account IDs; if empty, all account IDs that aren't excluded may be scanned | `None` | `registry.ecr.excludeId` | Do not scan ECR for images in these AWS account IDs; the default is to exclude the EKS system account | `602401143452` +| `registry.acr.enabled` | Mount `azure.json` via HostPath into the Flux Pod, enabling Flux to use AKS's service principal for ACR authentication | `false` +| `registry.acr.hostPath` | Alternative location of `azure.json` on the host | `/etc/kubernetes/azure.json` | `memcached.verbose` | Enable request logging in memcached | `false` | `memcached.maxItemSize` | Maximum size for one item | `1m` | `memcached.maxMemory` | Maximum memory to use, in megabytes | `64` diff --git a/chart/flux/templates/deployment.yaml b/chart/flux/templates/deployment.yaml index c05bf7e7d..ca6df43b8 100644 --- a/chart/flux/templates/deployment.yaml +++ b/chart/flux/templates/deployment.yaml @@ -45,6 +45,12 @@ spec: - name: git-keygen emptyDir: medium: Memory + {{- if .Values.registry.acr.enabled }} + - name: acr-credentials + hostPath: + path: "{{ .Values.registry.acr.mountPath }}" + type: "" + {{- end }} containers: - name: {{ .Chart.Name }} image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}" @@ -66,6 +72,11 @@ spec: readOnly: true - name: git-keygen mountPath: /var/fluxd/keygen + {{- if .Values.registry.acr.enabled }} + - name: acr-credentials + mountPath: /etc/kubernetes/azure.json + readOnly: true + {{- end }} env: - name: KUBECONFIG value: /root/.kubectl/config diff --git a/chart/flux/values.yaml b/chart/flux/values.yaml index d6d101ca0..ee7760260 100644 --- a/chart/flux/values.yaml +++ b/chart/flux/values.yaml @@ -136,6 +136,10 @@ registry: region: includeId: excludeId: + # Azure ACR settings + acr: + enabled: false + hostPath: /etc/kubernetes/azure.json memcached: repository: memcached From cf30277d0ba50e8cfdf4538ee8f5e8e6b924e5b9 Mon Sep 17 00:00:00 2001 From: Alan J Castonguay Date: Fri, 8 Feb 2019 14:08:26 -0500 Subject: [PATCH 8/9] Documentation in FAQ alongside AWS --- site/faq.md | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/site/faq.md b/site/faq.md index 7577d3c50..0011413ad 100644 --- a/site/faq.md +++ b/site/faq.md @@ -246,7 +246,7 @@ happen: if you've only just started using a particular image in a workload. - Flux can't get suitable credentials for the image repository. At present, it looks at `imagePullSecret`s attached to workloads, - service accounts, platform-provided credentials on GCP or AWS, and + service accounts, platform-provided credentials on GCP, AWS or Azure, and a Docker config file if you mount one into the fluxd container (see the [command-line usage](./daemon.md)). - When using images in ECR, from EC2, the `NodeInstanceRole` for the @@ -256,6 +256,25 @@ happen: [`kops`](https://github.com/kubernetes/kops) (with [`.iam.allowContainerRegistry=true`](https://github.com/kubernetes/kops/blob/master/docs/iam_roles.md#iam-roles)) both make sure this is the case. + - When using images from ACR in AKS, the HostPath `/etc/kubernetes/azure.json` + should be [mounted](https://kubernetes.io/docs/concepts/storage/volumes/) into the Flux Pod. + Set `registry.acr.enabled=True` in the [helm chart](../chart/flux/README.md) + or alter the [Deployment](../deploy/flux-deployment.yaml): + ```yaml + spec: + containers: + image: quay.io/weaveworks/flux + ... + volumeMounts: + - name: acr-credentials + mountPath: /etc/kubernetes/azure.json + readOnly: true + volumes: + - name: acr-credentials + hostPath: + path: /etc/kubernetes/azure.json + type: "" + ``` - Flux excludes images with no suitable manifest (linux amd64) in manifestlist - Flux doesn't yet understand image refs that use digests instead of tags; see From f0a4cc71a4e29364cfb00d33da29468c1e9bd1e6 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Mon, 11 Feb 2019 15:25:32 -0500 Subject: [PATCH 9/9] Update chart/flux/templates/deployment.yaml hostPath != mountPath Co-Authored-By: alanjcastonguay --- chart/flux/templates/deployment.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/chart/flux/templates/deployment.yaml b/chart/flux/templates/deployment.yaml index ca6df43b8..6231b54d8 100644 --- a/chart/flux/templates/deployment.yaml +++ b/chart/flux/templates/deployment.yaml @@ -48,7 +48,7 @@ spec: {{- if .Values.registry.acr.enabled }} - name: acr-credentials hostPath: - path: "{{ .Values.registry.acr.mountPath }}" + path: "{{ .Values.registry.acr.hostPath }}" type: "" {{- end }} containers: