Merge pull request #380 from sunzhaohua2/41827

OCPBUGS-41827: update injector to use a secret rather than an environment variable

Author: openshift-merge-bot[bot]

cmd/azure-config-credentials-injector/credentials_injector_test.go

@@ -69,112 +69,6 @@ func Test_mergeCloudConfig(t *testing.T) {
 			args:           []string{"--cloud-config-file-path", "foo"},
 			expectedErrMsg: "stat foo: no such file or directory",
 		},
-		{
-			name:           "AZURE_CLIENT_ID not set",
-			args:           []string{"--cloud-config-file-path", inputFile.Name(), "--output-file-path", outputFile.Name()},
-			expectedErrMsg: "azure_client_id should be set up",
-		},
-		{
-			name:           "AZURE_CLIENT_SECRET not set",
-			args:           []string{"--cloud-config-file-path", inputFile.Name(), "--output-file-path", outputFile.Name()},
-			envVars:        map[string]string{"AZURE_CLIENT_ID": "foo"},
-			expectedErrMsg: "azure_client_secret should be set up",
-		},
-		{
-			name:           "input file content is not a valid json",
-			args:           []string{"--cloud-config-file-path", inputFile.Name(), "--output-file-path", outputFile.Name()},
-			envVars:        map[string]string{"AZURE_CLIENT_ID": "foo", "AZURE_CLIENT_SECRET": "bar"},
-			fileContent:    "{*&(&#@!}",
-			expectedErrMsg: "couldn't read cloud config from file: invalid character '*' looking for beginning of object key string",
-		},
-		{
-			name:           "input file content is valid json, but format is unexpected",
-			args:           []string{"--cloud-config-file-path", inputFile.Name(), "--output-file-path", outputFile.Name()},
-			envVars:        map[string]string{"AZURE_CLIENT_ID": "foo", "AZURE_CLIENT_SECRET": "bar"},
-			fileContent:    "[1]",
-			expectedErrMsg: "couldn't read cloud config from file: json: cannot unmarshal array into Go value of type map[string]interface {}",
-		},
-		{
-			name:            "all ok, file is empty json object",
-			args:            []string{"--cloud-config-file-path", inputFile.Name(), "--output-file-path", outputFile.Name()},
-			envVars:         map[string]string{"AZURE_CLIENT_ID": "foo", "AZURE_CLIENT_SECRET": "bar"},
-			fileContent:     "{}",
-			expectedContent: "{\"aadClientId\":\"foo\",\"aadClientSecret\":\"bar\"}",
-		},
-		{
-			name:            "all ok, some content in json",
-			args:            []string{"--cloud-config-file-path", inputFile.Name(), "--output-file-path", outputFile.Name()},
-			envVars:         map[string]string{"AZURE_CLIENT_ID": "foo", "AZURE_CLIENT_SECRET": "bar"},
-			fileContent:     "{\"bar\": \"baz\"}",
-			expectedContent: "{\"aadClientId\":\"foo\",\"aadClientSecret\":\"bar\",\"bar\":\"baz\"}",
-		},
-		{
-			name:            "all ok, client_id and client_secret overrides",
-			args:            []string{"--cloud-config-file-path", inputFile.Name(), "--output-file-path", outputFile.Name()},
-			envVars:         map[string]string{"AZURE_CLIENT_ID": "foo", "AZURE_CLIENT_SECRET": "bar"},
-			fileContent:     "{\"aadClientSecret\":\"fizz\",\"aadClientId\":\"baz\"}",
-			expectedContent: "{\"aadClientId\":\"foo\",\"aadClientSecret\":\"bar\"}",
-		},
-		{
-			name:           "output file write error",
-			args:           []string{"--cloud-config-file-path", inputFile.Name(), "--output-file-path", "/tmp"},
-			envVars:        map[string]string{"AZURE_CLIENT_ID": "foo", "AZURE_CLIENT_SECRET": "bar"},
-			fileContent:    "{}",
-			expectedErrMsg: "couldn't write prepared cloud config to file: open /tmp: is a directory",
-		},
-		{
-			name:            "all ok, useManagedIdentityExtension not disabled",
-			args:            []string{"--cloud-config-file-path", inputFile.Name(), "--output-file-path", outputFile.Name()},
-			envVars:         map[string]string{"AZURE_CLIENT_ID": "foo", "AZURE_CLIENT_SECRET": "bar"},
-			fileContent:     "{\"aadClientSecret\":\"fizz\",\"aadClientId\":\"baz\",\"useManagedIdentityExtension\":true}",
-			expectedContent: "{\"aadClientId\":\"foo\",\"aadClientSecret\":\"bar\",\"useManagedIdentityExtension\":true}",
-		},
-		{
-			name:            "all ok, useManagedIdentityExtension disabled",
-			args:            []string{"--cloud-config-file-path", inputFile.Name(), "--output-file-path", outputFile.Name(), "--disable-identity-extension-auth"},
-			envVars:         map[string]string{"AZURE_CLIENT_ID": "foo", "AZURE_CLIENT_SECRET": "bar"},
-			fileContent:     "{\"aadClientSecret\":\"fizz\",\"aadClientId\":\"baz\",\"useManagedIdentityExtension\":true}",
-			expectedContent: "{\"aadClientId\":\"foo\",\"aadClientSecret\":\"bar\",\"useManagedIdentityExtension\":false}",
-		},
-		{
-			name:            "all ok, invalid useManagedIdentityExtension value",
-			args:            []string{"--cloud-config-file-path", inputFile.Name(), "--output-file-path", outputFile.Name(), "--disable-identity-extension-auth"},
-			envVars:         map[string]string{"AZURE_CLIENT_ID": "foo", "AZURE_CLIENT_SECRET": "bar"},
-			fileContent:     "{\"aadClientSecret\":\"fizz\",\"aadClientId\":\"baz\",\"useManagedIdentityExtension\":\"true\"}",
-			expectedContent: "{\"aadClientId\":\"foo\",\"aadClientSecret\":\"bar\",\"useManagedIdentityExtension\":false}",
-		},
-		{
-			name:            "all ok, use workload identity while client secret is not present",
-			args:            []string{"--cloud-config-file-path", inputFile.Name(), "--output-file-path", outputFile.Name(), "--disable-identity-extension-auth", "--enable-azure-workload-identity=true"},
-			envVars:         map[string]string{"AZURE_TENANT_ID": "bar", "AZURE_CLIENT_ID": "buzz", "AZURE_FEDERATED_TOKEN_FILE": "baz"},
-			fileContent:     "{\"tenantId\":\"foo\",\"aadClientId\":\"fizz\"}",
-			expectedContent: "{\"aadClientId\":\"buzz\",\"aadFederatedTokenFile\":\"baz\",\"tenantId\":\"bar\",\"useFederatedWorkloadIdentityExtension\":true}",
-		},
-		{
-			name:            "all ok, use workload identity while managed identity is not explicitly disabled",
-			args:            []string{"--cloud-config-file-path", inputFile.Name(), "--output-file-path", outputFile.Name(), "--enable-azure-workload-identity=true"},
-			envVars:         map[string]string{"AZURE_TENANT_ID": "bar", "AZURE_CLIENT_ID": "buzz", "AZURE_FEDERATED_TOKEN_FILE": "baz"},
-			fileContent:     "{\"tenantId\":\"foo\",\"aadClientId\":\"fizz\"}",
-			expectedContent: "{\"aadClientId\":\"buzz\",\"aadFederatedTokenFile\":\"baz\",\"tenantId\":\"bar\",\"useFederatedWorkloadIdentityExtension\":true}",
-		},
-		{
-			name:           "should fail, client secret is present while federated token file is present",
-			args:           []string{"--cloud-config-file-path", inputFile.Name(), "--output-file-path", outputFile.Name(), "--disable-identity-extension-auth", "--enable-azure-workload-identity=true"},
-			envVars:        map[string]string{"AZURE_TENANT_ID": "baz", "AZURE_CLIENT_ID": "foo", "AZURE_CLIENT_SECRET": "bar", "AZURE_FEDERATED_TOKEN_FILE": "baz"},
-			expectedErrMsg: "azure_client_secret is set while workload identity is enabled using azure_federated_token_file, this should never happen.\nPlease consider reporting a bug: https://issues.redhat.com",
-		},
-		{
-			name:           "should fail, tenant id missing while federated token file is present",
-			args:           []string{"--cloud-config-file-path", inputFile.Name(), "--output-file-path", outputFile.Name(), "--disable-identity-extension-auth", "--enable-azure-workload-identity=true"},
-			envVars:        map[string]string{"AZURE_CLIENT_ID": "buzz", "AZURE_FEDERATED_TOKEN_FILE": "baz"},
-			expectedErrMsg: "azure_tenant_id should be set up while workload identity is enabled using azure_federated_token_file, this should never happen.\nPlease consider reporting a bug: https://issues.redhat.com",
-		},
-		{
-			name:           "should fail, workload identity can't be enabled because federated token missing, expect secret provided",
-			args:           []string{"--cloud-config-file-path", inputFile.Name(), "--output-file-path", outputFile.Name(), "--disable-identity-extension-auth", "--enable-azure-workload-identity=true"},
-			envVars:        map[string]string{"AZURE_TENANT_ID": "bar", "AZURE_CLIENT_ID": "buzz"},
-			expectedErrMsg: "azure_client_secret should be set up",
-		},
 	}
 
 	for _, tc := range testCases {

cmd/azure-config-credentials-injector/main.go

@@ -11,11 +11,6 @@ import (
 )
 
 const (
-	clientIDEnvKey       = "AZURE_CLIENT_ID"
-	clientSecretEnvKey   = "AZURE_CLIENT_SECRET"
-	tenantIDEnvKey       = "AZURE_TENANT_ID"
-	federatedTokenEnvKey = "AZURE_FEDERATED_TOKEN_FILE"
-
 	clientIDCloudConfigKey               = "aadClientId"
 	clientSecretCloudConfigKey           = "aadClientSecret"
 	useManagedIdentityExtensionConfigKey = "useManagedIdentityExtension"
@@ -60,7 +55,6 @@ func main() {
 func mergeCloudConfig(_ *cobra.Command, args []string) error {
 	var (
 		azureClientId           string
-		azureClientIdFound      bool
 		tenantId                string
 		tenantIdFound           bool
 		federatedTokenFile      string
@@ -74,54 +68,31 @@ func mergeCloudConfig(_ *cobra.Command, args []string) error {
 		return err
 	}
 
-	// Read credentials from mounted Secret files or environment variables
+	// Read credentials from mounted Secret files
 	azureClientId, err = readSecretFile(fmt.Sprintf("%s/azure_client_id", injectorOpts.credentialsPath))
 	if err != nil {
-		if os.IsNotExist(err) {
-			// Fallback to environment variable if secret file does not exist
-			azureClientId, azureClientIdFound = mustLookupEnvValue(clientIDEnvKey)
-			if !azureClientIdFound {
-				return fmt.Errorf("azure_client_id should be set up")
-			}
-		} else {
-			return fmt.Errorf("failed to read azure_client_id from secret: %w", err)
-		}
+		return fmt.Errorf("failed to read azure_client_id from secret: %w", err)
 	}
 
 	azureClientSecret, err = readSecretFile(fmt.Sprintf("%s/azure_client_secret", injectorOpts.credentialsPath))
-	if err != nil {
-		if os.IsNotExist(err) {
-			// Fallback to environment variable if secret file does not exist
-			azureClientSecret, secretFound = mustLookupEnvValue(clientSecretEnvKey)
-		} else {
-			return fmt.Errorf("failed to read azure_client_secret from secret: %w", err)
-		}
-	} else {
+	if err == nil {
 		secretFound = true
-	}
-
-	tenantId, err = readSecretFile(fmt.Sprintf("%s/azure_tenant_id", injectorOpts.credentialsPath))
-	if err != nil {
-		if os.IsNotExist(err) {
-			// Fallback to environment variable if secret file does not exist
-			tenantId, tenantIdFound = mustLookupEnvValue(tenantIDEnvKey)
-		} else {
-			return fmt.Errorf("failed to read azure_tenant_id from secret: %w", err)
-		}
-	} else {
-		tenantIdFound = true
+	} else if !os.IsNotExist(err) {
+		return fmt.Errorf("failed to read azure_client_secret from secret: %w", err)
 	}
 
 	federatedTokenFile, err = readSecretFile(fmt.Sprintf("%s/azure_federated_token_file", injectorOpts.credentialsPath))
-	if err != nil {
-		if os.IsNotExist(err) {
-			// Fallback to environment variable if secret file does not exist
-			federatedTokenFile, federatedTokenFileFound = mustLookupEnvValue(federatedTokenEnvKey)
-		} else {
-			return fmt.Errorf("failed to read azure_federated_token_file from secret: %w", err)
-		}
-	} else {
+	if err == nil {
 		federatedTokenFileFound = true
+	} else if !os.IsNotExist(err) {
+		return fmt.Errorf("failed to read azure_federated_token_file from secret: %w", err)
+	}
+
+	tenantId, err = readSecretFile(fmt.Sprintf("%s/azure_tenant_id", injectorOpts.credentialsPath))
+	if err == nil {
+		tenantIdFound = true
+	} else if !os.IsNotExist(err) {
+		return fmt.Errorf("failed to read azure_tenant_id from secret: %w", err)
 	}
 
 	// If federatedTokenFile found, workload identity should be used
@@ -199,7 +170,7 @@ func prepareCloudConfig(cloudConfig map[string]interface{}, clientId, clientSecr
 		cloudConfig[aadFederatedTokenFileConfigKey] = federatedTokenFile
 		cloudConfig[useFederatedWorkloadIdentityExtensionConfigKey] = true
 	} else {
-		klog.V(4).Infof("%s env variable is set, client secret authentication will be used", clientSecretEnvKey)
+		klog.V(4).Infof("azure_client_secret is set, client secret authentication will be used")
 		cloudConfig[clientSecretCloudConfigKey] = clientSecret
 	}
 
@@ -217,11 +188,3 @@ func writeCloudConfig(path string, preparedConfig []byte) error {
 	}
 	return nil
 }
-
-func mustLookupEnvValue(key string) (string, bool) {
-	value, found := os.LookupEnv(key)
-	if !found || len(value) == 0 {
-		return "", false
-	}
-	return value, true
-}

pkg/cloud/azure/assets/cloud-controller-manager-deployment.yaml

@@ -71,30 +71,6 @@ spec:
                 --disable-identity-extension-auth \
                 --enable-azure-workload-identity=true \
                 --creds-path=/etc/azure/credentials
-          env:
-            - name: AZURE_CLIENT_ID
-              valueFrom:
-                secretKeyRef:
-                  name: azure-cloud-credentials
-                  key: azure_client_id
-            - name: AZURE_CLIENT_SECRET
-              valueFrom:
-                secretKeyRef:
-                  name: azure-cloud-credentials
-                  key: azure_client_secret
-                  optional: true
-            - name: AZURE_TENANT_ID
-              valueFrom:
-                secretKeyRef:
-                  name: azure-cloud-credentials
-                  key: azure_tenant_id
-                  optional: true
-            - name: AZURE_FEDERATED_TOKEN_FILE
-              valueFrom:
-                secretKeyRef:
-                  name: azure-cloud-credentials
-                  key: azure_federated_token_file
-                  optional: true
           terminationMessagePolicy: FallbackToLogsOnError
           volumeMounts:
             - name: config-accm
@@ -114,12 +90,6 @@ spec:
               value: /etc/kubernetes-cloud-config/cloud.conf
             - name: OCP_INFRASTRUCTURE_NAME
               value: {{ .infrastructureName }}
-            - name: AZURE_FEDERATED_TOKEN_FILE
-              valueFrom:
-                secretKeyRef:
-                  name: azure-cloud-credentials
-                  key: azure_federated_token_file
-                  optional: true
           resources:
             requests:
               cpu: 200m

pkg/cloud/azure/assets/cloud-node-manager-daemonset.yaml

@@ -63,30 +63,6 @@ spec:
                 --disable-identity-extension-auth \
                 --enable-azure-workload-identity=true \
                 --creds-path=/etc/azure/credentials
-          env:
-            - name: AZURE_CLIENT_ID
-              valueFrom:
-                secretKeyRef:
-                  name: azure-cloud-credentials
-                  key: azure_client_id
-            - name: AZURE_CLIENT_SECRET
-              valueFrom:
-                secretKeyRef:
-                  name: azure-cloud-credentials
-                  key: azure_client_secret
-                  optional: true
-            - name: AZURE_TENANT_ID
-              valueFrom:
-                secretKeyRef:
-                  name: azure-cloud-credentials
-                  key: azure_tenant_id
-                  optional: true
-            - name: AZURE_FEDERATED_TOKEN_FILE
-              valueFrom:
-                secretKeyRef:
-                  name: azure-cloud-credentials
-                  key: azure_federated_token_file
-                  optional: true
           terminationMessagePolicy: FallbackToLogsOnError
           volumeMounts:
             - name: host-etc-kube

pkg/cloud/azurestack/assets/cloud-controller-manager-deployment.yaml

@@ -62,17 +62,6 @@ spec:
             - --cloud-config-file-path=/tmp/cloud-config/cloud.conf
             - --output-file-path=/tmp/merged-cloud-config/cloud.conf
             - --creds-path=/etc/azure/credentials
-          env:
-            - name: AZURE_CLIENT_ID
-              valueFrom:
-                secretKeyRef:
-                  name: azure-cloud-credentials
-                  key: azure_client_id
-            - name: AZURE_CLIENT_SECRET
-              valueFrom:
-                secretKeyRef:
-                  name: azure-cloud-credentials
-                  key: azure_client_secret
           terminationMessagePolicy: FallbackToLogsOnError
           volumeMounts:
             - name: config-accm

pkg/cloud/azurestack/assets/cloud-node-manager-daemonset.yaml

@@ -54,17 +54,6 @@ spec:
             - --cloud-config-file-path=/tmp/cloud-config/cloud.conf
             - --output-file-path=/tmp/merged-cloud-config/cloud.conf
             - --creds-path=/etc/azure/credentials
-          env:
-            - name: AZURE_CLIENT_ID
-              valueFrom:
-                secretKeyRef:
-                  name: azure-cloud-credentials
-                  key: azure_client_id
-            - name: AZURE_CLIENT_SECRET
-              valueFrom:
-                secretKeyRef:
-                  name: azure-cloud-credentials
-                  key: azure_client_secret
           terminationMessagePolicy: FallbackToLogsOnError
           volumeMounts:
             - name: config-accm