Bug 1896918: add new credentials field for AWS Secrets

pkg/aws/actuator/actuator.go

@@ -54,6 +54,10 @@ const (
 	roAWSCredsSecret          = "cloud-credential-operator-iam-ro-creds"
 	openshiftClusterIDKey     = "openshiftClusterID"
 	clusterVersionObjectName  = "version"
+
+	secretDataAccessKey      = "aws_access_key_id"
+	secretDataSecretKey      = "aws_secret_access_key"
+	secretDataCredentialsKey = "credentials"
 )
 
 var _ actuatoriface.Actuator = (*AWSActuator)(nil)
@@ -153,11 +157,18 @@ func (a *AWSActuator) needsUpdate(ctx context.Context, cr *minterv1.CredentialsR
 	}
 
 	// Various checks for the kinds of reasons that would trigger a needed update
-	_, accessKey, secretKey := a.loadExistingSecret(cr)
+	_, accessKey, secretKey, credentialsKey := a.loadExistingSecret(cr)
 	awsClient, err := a.AWSClientBuilder([]byte(accessKey), []byte(secretKey), a.Client)
 	if err != nil {
 		return true, err
 	}
+
+	// Make sure we update old Secrets that don't have the new "credentials" field
+	if credentialsKey == "" || credentialsKey != string(generateAWSCredentialsConfig(accessKey, secretKey)) {
+		logger.Infof("Secret %s key needs updating, will update Secret contents", secretDataCredentialsKey)
+		return true, nil
+	}
+
 	awsSpec, err := DecodeProviderSpec(a.Codec, cr)
 	if err != nil {
 		return true, err
@@ -348,7 +359,7 @@ func (a *AWSActuator) sync(ctx context.Context, cr *minterv1.CredentialsRequest)
 }
 
 func (a *AWSActuator) syncPassthrough(ctx context.Context, cr *minterv1.CredentialsRequest, cloudCredsSecret *corev1.Secret, logger log.FieldLogger) error {
-	existingSecret, _, _ := a.loadExistingSecret(cr)
+	existingSecret, _, _, _ := a.loadExistingSecret(cr)
 	accessKeyID := string(cloudCredsSecret.Data[awsannotator.AwsAccessKeyName])
 	secretAccessKey := string(cloudCredsSecret.Data[awsannotator.AwsSecretAccessKeyName])
 	// userPolicy param empty because in passthrough mode this doesn't really have any meaning
@@ -492,7 +503,7 @@ func (a *AWSActuator) syncMint(ctx context.Context, cr *minterv1.CredentialsRequ
 		return err
 	}
 
-	existingSecret, existingAccessKeyID, _ := a.loadExistingSecret(cr)
+	existingSecret, existingAccessKeyID, _, _ := a.loadExistingSecret(cr)
 
 	var accessKey *iam.AccessKey
 	// TODO: also check if the access key ID on the request is still valid in AWS
@@ -694,10 +705,11 @@ func (a *AWSActuator) Delete(ctx context.Context, cr *minterv1.CredentialsReques
 	return nil
 }
 
-func (a *AWSActuator) loadExistingSecret(cr *minterv1.CredentialsRequest) (*corev1.Secret, string, string) {
+func (a *AWSActuator) loadExistingSecret(cr *minterv1.CredentialsRequest) (*corev1.Secret, string, string, string) {
 	logger := a.getLogger(cr)
 	var existingAccessKeyID string
 	var existingSecretAccessKey string
+	var existingCredentialsKey string
 
 	// Check if the credentials secret exists, if not we need to inform the syncer to generate a new one:
 	existingSecret := &corev1.Secret{}
@@ -707,25 +719,32 @@ func (a *AWSActuator) loadExistingSecret(cr *minterv1.CredentialsRequest) (*core
 			logger.Debug("secret does not exist")
 		}
 	} else {
-		keyBytes, ok := existingSecret.Data["aws_access_key_id"]
+		keyBytes, ok := existingSecret.Data[secretDataAccessKey]
 		if !ok {
 			// Warn, but this will trigger generation of a new key and updating the secret.
-			logger.Warning("secret did not have expected key: aws_access_key_id, will be regenerated")
+			logger.Warningf("secret did not have expected key: %s, will be regenerated", secretDataAccessKey)
 		} else {
 			decoded := string(keyBytes)
 			existingAccessKeyID = string(decoded)
 			logger.WithField("accessKeyID", existingAccessKeyID).Debug("found access key ID in target secret")
 
 		}
 
-		secretBytes, ok := existingSecret.Data["aws_secret_access_key"]
+		secretBytes, ok := existingSecret.Data[secretDataSecretKey]
 		if !ok {
-			logger.Warning("secret did not have expected key: aws_secret_access_key")
+			logger.Warningf("secret did not have expected key: %s", secretDataSecretKey)
 		} else {
 			existingSecretAccessKey = string(secretBytes)
 		}
+
+		credentialsKey, ok := existingSecret.Data[secretDataCredentialsKey]
+		if !ok {
+			logger.Warningf("secret did not have expected key: %s, will be updated", secretDataCredentialsKey)
+		} else {
+			existingCredentialsKey = string(credentialsKey)
+		}
 	}
-	return existingSecret, existingAccessKeyID, existingSecretAccessKey
+	return existingSecret, existingAccessKeyID, existingSecretAccessKey, existingCredentialsKey
 }
 
 func (a *AWSActuator) tagUser(logger log.FieldLogger, awsClient minteraws.Client, username, infraName, clusterUUID string) error {
@@ -858,8 +877,9 @@ func (a *AWSActuator) syncAccessKeySecret(cr *minterv1.CredentialsRequest, acces
 				},
 			},
 			Data: map[string][]byte{
-				"aws_access_key_id":     []byte(accessKeyID),
-				"aws_secret_access_key": []byte(secretAccessKey),
+				secretDataAccessKey:      []byte(accessKeyID),
+				secretDataSecretKey:      []byte(secretAccessKey),
+				secretDataCredentialsKey: generateAWSCredentialsConfig(accessKeyID, secretAccessKey),
 			},
 		}
 
@@ -881,10 +901,13 @@ func (a *AWSActuator) syncAccessKeySecret(cr *minterv1.CredentialsRequest, acces
 	existingSecret.Annotations[minterv1.AnnotationCredentialsRequest] = fmt.Sprintf("%s/%s", cr.Namespace, cr.Name)
 	existingSecret.Annotations[minterv1.AnnotationAWSPolicyLastApplied] = userPolicy
 	if accessKeyID != "" && secretAccessKey != "" {
-		existingSecret.Data["aws_access_key_id"] = []byte(accessKeyID)
-		existingSecret.Data["aws_secret_access_key"] = []byte(secretAccessKey)
+		existingSecret.Data[secretDataAccessKey] = []byte(accessKeyID)
+		existingSecret.Data[secretDataSecretKey] = []byte(secretAccessKey)
 	}
 
+	// Make sure credentials config data is synced with the stored access key / secret key
+	existingSecret.Data[secretDataCredentialsKey] = generateAWSCredentialsConfig(string(existingSecret.Data[secretDataAccessKey]), string(existingSecret.Data[secretDataSecretKey]))
+
 	if !reflect.DeepEqual(existingSecret, origSecret) {
 		sLog.Info("target secret has changed, updating")
 		err := a.Client.Update(context.TODO(), existingSecret)
@@ -1296,3 +1319,11 @@ func (a *AWSActuator) getUpcomingCredSecrets() []types.NamespacedName {
 	}
 	return constants.AWSUpcomingSecrets
 }
+
+func generateAWSCredentialsConfig(accessKeyID, secretAccessKey string) []byte {
+	awsConfig := fmt.Sprintf(`[default]
+%s = %s
+%s = %s`, secretDataAccessKey, accessKeyID, secretDataSecretKey, secretAccessKey)
+
+	return []byte(awsConfig)
+}

pkg/aws/actuator/actuator_test.go

@@ -17,12 +17,14 @@ limitations under the License.
 package actuator
 
 import (
+	"context"
 	"fmt"
 	"strings"
 	"testing"
 
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	configv1 "github.com/openshift/api/config/v1"
 	v1 "github.com/openshift/api/operator/v1"
@@ -49,6 +51,9 @@ const (
 
 	testRootAccessKeyID     = "TestRootAccessKeyID"
 	testRootSecretAccessKey = "TestRootSecretAccessKey"
+
+	testTargetSecret    = "testTargetSecretName"
+	testTargetNamespace = "testTargetNamespace"
 )
 
 type awsClientBuilderRecorder struct {
@@ -372,6 +377,92 @@ func TestUpgradeable(t *testing.T) {
 		})
 	}
 }
+
+func TestSecretFormat(t *testing.T) {
+	apis.AddToScheme(scheme.Scheme)
+
+	tests := []struct {
+		name            string
+		accessKeyID     string
+		secretAccessKey string
+		existingSecret  *corev1.Secret
+	}{
+		{
+			name:            "new secret with credentials field",
+			accessKeyID:     "AKFIRSTKEY",
+			secretAccessKey: "FIRSTSECRET",
+		},
+		{
+			name:            "existing secret without credentials field",
+			accessKeyID:     "AKFIRSTKEY",
+			secretAccessKey: "FIRSTSECRET",
+			existingSecret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      testTargetSecret,
+					Namespace: testTargetNamespace,
+				},
+				Data: map[string][]byte{
+					"aws_access_key_id":     []byte("SOMEACCESSKEY"),
+					"aws_secret_access_key": []byte("SOMESECRETKEY"),
+				},
+			},
+		},
+		{
+			name:            "existing secret with outdated credentials field",
+			accessKeyID:     "AKFIRSTKEY",
+			secretAccessKey: "FIRSTSECRET",
+			existingSecret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      testTargetSecret,
+					Namespace: testTargetNamespace,
+				},
+				Data: map[string][]byte{
+					"aws_access_key_id":     []byte("SOMEACCESSKEY"),
+					"aws_secret_access_key": []byte("SOMESECRETKEY"),
+					"credentials":           []byte("OLD AWS CONFIG"),
+				},
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			var fakeClient client.Client
+			if test.existingSecret != nil {
+				fakeClient = fake.NewFakeClient(test.existingSecret)
+			} else {
+				fakeClient = fake.NewFakeClient()
+			}
+
+			a := &AWSActuator{
+				Client: fakeClient,
+			}
+
+			cr := testCredentialsRequest()
+			logger := a.getLogger(cr)
+			err := a.syncAccessKeySecret(cr, test.accessKeyID, test.secretAccessKey, test.existingSecret, "exampleAWSPolicy", logger)
+
+			require.NoError(t, err, "unexpected error creating/updating Secret")
+
+			secret := &corev1.Secret{}
+			secretNSN := types.NamespacedName{Name: cr.Spec.SecretRef.Name, Namespace: cr.Spec.SecretRef.Namespace}
+
+			err = fakeClient.Get(context.TODO(), secretNSN, secret)
+			require.NoError(t, err, "unexpected error retriving Secret")
+
+			assert.Contains(t, secret.Data, "aws_access_key_id")
+			assert.Equal(t, string(secret.Data["aws_access_key_id"]), test.accessKeyID)
+			assert.Contains(t, secret.Data, "aws_secret_access_key")
+			assert.Equal(t, string(secret.Data["aws_secret_access_key"]), test.secretAccessKey)
+
+			require.Contains(t, secret.Data, "credentials")
+			credentialsConfig := string(secret.Data["credentials"])
+			assert.Contains(t, credentialsConfig, fmt.Sprintf("aws_access_key_id = %s", test.accessKeyID))
+			assert.Contains(t, credentialsConfig, fmt.Sprintf("aws_secret_access_key = %s", test.secretAccessKey))
+		})
+	}
+}
+
 func testReadOnlySecret() *corev1.Secret {
 	return &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
@@ -417,6 +508,12 @@ func testCredentialsRequest() *minterv1.CredentialsRequest {
 			Name:      "testcr",
 			Namespace: "testnamespace",
 		},
+		Spec: minterv1.CredentialsRequestSpec{
+			SecretRef: corev1.ObjectReference{
+				Name:      testTargetSecret,
+				Namespace: testTargetNamespace,
+			},
+		},
 	}
 }
 

pkg/operator/credentialsrequest/credentialsrequest_controller.go

@@ -71,6 +71,13 @@ const (
 	credentialsRequestInfraMismatch = "InfrastructureMismatch"
 )
 
+var (
+	syncPeriod = time.Hour
+	// Set some extra time when requeueing so we are guaranteed that the
+	// syncPeriod has elapsed when we re-reconcile an object.
+	defaultRequeueTime = syncPeriod + time.Minute*10
+)
+
 // AddWithActuator creates a new CredentialsRequest Controller and adds it to the Manager with
 // default RBAC. The Manager will set fields on the Controller and Start it when
 // the Manager is Started.
@@ -537,12 +544,15 @@ func (r *ReconcileCredentialsRequest) Reconcile(request reconcile.Request) (reco
 	}
 	cloudCredsSecretUpdated := credentialsRootSecret != nil && credentialsRootSecret.ResourceVersion != cr.Status.LastSyncCloudCredsSecretResourceVersion
 	isStale := cr.Generation != cr.Status.LastSyncGeneration
-	hasRecentlySynced := cr.Status.LastSyncTimestamp != nil && cr.Status.LastSyncTimestamp.Add(time.Hour*1).After(time.Now())
+	hasRecentlySynced := cr.Status.LastSyncTimestamp != nil && cr.Status.LastSyncTimestamp.Add(syncPeriod).After(time.Now())
 	hasActiveFailureConditions := checkForFailureConditions(cr)
 
 	if !cloudCredsSecretUpdated && !isStale && hasRecentlySynced && crSecretExists && !hasActiveFailureConditions && cr.Status.Provisioned {
 		logger.Debug("lastsyncgeneration is current and lastsynctimestamp was less than an hour ago, so no need to sync")
-		return reconcile.Result{}, nil
+		// Since we get no events for changes made directly to the cloud/platform, set the requeueAfter so that we at
+		// least periodically check that nothing out in the cloud/platform was modified that would require us to fix up
+		// users/permissions/tags/etc.
+		return reconcile.Result{RequeueAfter: defaultRequeueTime}, nil
 	}
 
 	credsExists, err := r.Actuator.Exists(context.TODO(), cr)
@@ -592,7 +602,10 @@ func (r *ReconcileCredentialsRequest) Reconcile(request reconcile.Request) (reco
 		return reconcile.Result{}, err
 	}
 
-	return reconcile.Result{}, syncErr
+	// Since we get no events for changes made directly to the cloud/platform, set the requeueAfter so that we at
+	// least periodically check that nothing out in the cloud/platform was modified that would require us to fix up
+	// users/permissions/tags/etc.
+	return reconcile.Result{RequeueAfter: defaultRequeueTime}, syncErr
 }
 
 func (r *ReconcileCredentialsRequest) updateActuatorConditions(cr *minterv1.CredentialsRequest, reason minterv1.CredentialsRequestConditionType, conditionError error) {

pkg/operator/credentialsrequest/credentialsrequest_controller_test.go

@@ -387,7 +387,7 @@ func TestCredentialsRequestReconcile(t *testing.T) {
 				createTestNamespace(testNamespace),
 				createTestNamespace(testSecretNamespace),
 				testCredentialsRequest(t),
-				testAWSCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-iam-ro-creds", testReadAWSAccessKeyID, testReadAWSSecretAccessKey),
+				testLegacyAWSCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-iam-ro-creds", testReadAWSAccessKeyID, testReadAWSSecretAccessKey),
 				testAWSCredsSecret(testSecretNamespace, testSecretName, testAWSAccessKeyID, testAWSSecretAccessKey),
 				testClusterVersion(),
 				testInfrastructure(testInfraName),
@@ -492,6 +492,41 @@ func TestCredentialsRequestReconcile(t *testing.T) {
 				assert.True(t, cr.Status.Provisioned)
 			},
 		},
+		{
+			name: "cred exists credentials key missing",
+			existing: []runtime.Object{
+				testOperatorConfig(""),
+				createTestNamespace(testNamespace),
+				createTestNamespace(testSecretNamespace),
+				testCredentialsRequest(t),
+				testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey),
+				testAWSCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-iam-ro-creds", testReadAWSAccessKeyID, testReadAWSSecretAccessKey),
+				testLegacyAWSCredsSecret(testSecretNamespace, testSecretName, testAWSAccessKeyID, testAWSSecretAccessKey),
+				testClusterVersion(),
+				testInfrastructure(testInfraName),
+			},
+			mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient {
+				mockAWSClient := mockaws.NewMockClient(mockCtrl)
+				return mockAWSClient
+			},
+			mockReadAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient {
+				mockAWSClient := mockaws.NewMockClient(mockCtrl)
+				mockGetUser(mockAWSClient)
+				mockListAccessKeys(mockAWSClient, testAWSAccessKeyID)
+				mockGetUserPolicy(mockAWSClient, testPolicy1)
+				return mockAWSClient
+			},
+			validate: func(c client.Client, t *testing.T) {
+				targetSecret := getSecret(c)
+				require.NotNil(t, targetSecret)
+				require.Contains(t, targetSecret.Data, "credentials", "expected Secret to be updated with 'credentials' field")
+				assert.Contains(t, string(targetSecret.Data["credentials"]), testAWSAccessKeyID, "didn't find access key data in credentials field")
+				assert.Contains(t, string(targetSecret.Data["credentials"]), testAWSSecretAccessKey, "didn't find secret key data in credentials field")
+
+				cr := getCR(c)
+				assert.True(t, cr.Status.Provisioned)
+			},
+		},
 		{
 			name: "cred deletion",
 			existing: []runtime.Object{
@@ -1480,7 +1515,7 @@ func testPassthroughAWSCredsSecret(namespace, name, accessKeyID, secretAccessKey
 	return s
 }
 
-func testAWSCredsSecret(namespace, name, accessKeyID, secretAccessKey string) *corev1.Secret {
+func testLegacyAWSCredsSecret(namespace, name, accessKeyID, secretAccessKey string) *corev1.Secret {
 	s := &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      name,
@@ -1497,6 +1532,16 @@ func testAWSCredsSecret(namespace, name, accessKeyID, secretAccessKey string) *c
 	return s
 }
 
+func testAWSCredsSecret(namespace, name, accessKeyID, secretAccessKey string) *corev1.Secret {
+	s := testLegacyAWSCredsSecret(namespace, name, accessKeyID, secretAccessKey)
+
+	s.Data["credentials"] = []byte(fmt.Sprintf(`[default]
+aws_access_key_id = %s
+aws_secret_access_key = %s`, accessKeyID, secretAccessKey))
+
+	return s
+}
+
 func genericAWSError() error {
 	return awserr.New("GenericFailure", "An error besides NotFound", fmt.Errorf("Just a generic AWS error for test purposes"))
 }