From 452bbc497556a0a4f10a0bc11e122e50f01c579d Mon Sep 17 00:00:00 2001 From: Joel Diaz Date: Mon, 2 Nov 2020 14:36:22 -0500 Subject: [PATCH] add new credentials field for AWS Secrets Start storing a usable AWS credentials config file in the 'credentials' field of the Secret. This should allow a consumer of the credentials to just point to the config stored in that field when setting up an AWS client. Also make sure we are re-queueing CredentialsRequest objects every 1hr10min (so that we are at least periodically doing a full reconcile to reestore any lost credentials). --- pkg/aws/actuator/actuator.go | 57 ++++++++--- pkg/aws/actuator/actuator_test.go | 97 +++++++++++++++++++ .../credentialsrequest_controller.go | 19 +++- .../credentialsrequest_controller_test.go | 49 +++++++++- 4 files changed, 204 insertions(+), 18 deletions(-) diff --git a/pkg/aws/actuator/actuator.go b/pkg/aws/actuator/actuator.go index 625e70ff5c..e8bf4f3aed 100644 --- a/pkg/aws/actuator/actuator.go +++ b/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,10 +719,10 @@ 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) @@ -718,14 +730,21 @@ func (a *AWSActuator) loadExistingSecret(cr *minterv1.CredentialsRequest) (*core } - 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) +} diff --git a/pkg/aws/actuator/actuator_test.go b/pkg/aws/actuator/actuator_test.go index 57e8b8c0e6..e69dbeba9c 100644 --- a/pkg/aws/actuator/actuator_test.go +++ b/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, + }, + }, } } diff --git a/pkg/operator/credentialsrequest/credentialsrequest_controller.go b/pkg/operator/credentialsrequest/credentialsrequest_controller.go index ff057f6bb3..f09cd0279e 100644 --- a/pkg/operator/credentialsrequest/credentialsrequest_controller.go +++ b/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) { diff --git a/pkg/operator/credentialsrequest/credentialsrequest_controller_test.go b/pkg/operator/credentialsrequest/credentialsrequest_controller_test.go index 4822d8e397..800193d588 100644 --- a/pkg/operator/credentialsrequest/credentialsrequest_controller_test.go +++ b/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")) }