diff --git a/go.mod b/go.mod index af19bbd700..bae8573497 100644 --- a/go.mod +++ b/go.mod @@ -61,6 +61,7 @@ require ( github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/storage/armstorage v1.1.0 github.com/Azure/azure-sdk-for-go/sdk/storage/azblob v0.5.1 github.com/go-logr/logr v1.2.4 + github.com/google/go-cmp v0.5.9 github.com/microsoft/kiota-authentication-azure-go v0.6.0 github.com/microsoftgraph/msgraph-sdk-go v0.59.0 github.com/openshift/client-go v0.0.0-20230503144108-75015d2347cb @@ -96,7 +97,6 @@ require ( github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect github.com/golang/protobuf v1.5.3 // indirect github.com/google/gnostic v0.5.7-v3refs // indirect - github.com/google/go-cmp v0.5.9 // indirect github.com/google/gofuzz v1.2.0 // indirect github.com/googleapis/gax-go/v2 v2.1.1 // indirect github.com/imdario/mergo v0.3.12 // indirect diff --git a/manifests/04-cluster-operator.yaml b/manifests/04-cluster-operator.yaml index a5d7b6ff57..cf7953e68b 100644 --- a/manifests/04-cluster-operator.yaml +++ b/manifests/04-cluster-operator.yaml @@ -9,3 +9,42 @@ status: versions: - name: operator version: "0.0.1-snapshot" + relatedObjects: + - group: admissionregistration.k8s.io + name: pod-identity-webhook + resource: mutatingwebhookconfigurations + - group: apps + name: pod-identity-webhook + namespace: openshift-cloud-credential-operator + resource: deployments + - group: cloudcredential.openshift.io + name: "" + resource: credentialsrequests + - group: "" + name: openshift-cloud-credential-operator + resource: namespaces + - group: "" + name: pod-identity-webhook + namespace: openshift-cloud-credential-operator + resource: serviceaccounts + - group: "" + name: pod-identity-webhook + namespace: openshift-cloud-credential-operator + resource: services + - group: operator.openshift.io + name: cluster + resource: cloudcredentials + - group: rbac.authorization.k8s.io + name: pod-identity-webhook + namespace: openshift-cloud-credential-operator + resource: rolebindings + - group: rbac.authorization.k8s.io + name: pod-identity-webhook + namespace: openshift-cloud-credential-operator + resource: roles + - group: rbac.authorization.k8s.io + name: pod-identity-webhook + resource: clusterrolebindings + - group: rbac.authorization.k8s.io + name: pod-identity-webhook + resource: clusterroles \ No newline at end of file diff --git a/pkg/apis/cloudcredential/v1/types_credentialsrequest.go b/pkg/apis/cloudcredential/v1/types_credentialsrequest.go index 69ba6f0e0e..dd1c19aea0 100644 --- a/pkg/apis/cloudcredential/v1/types_credentialsrequest.go +++ b/pkg/apis/cloudcredential/v1/types_credentialsrequest.go @@ -27,7 +27,11 @@ const ( // credentials in AWS before allowing the CredentialsRequest to be deleted in etcd. FinalizerDeprovision string = "cloudcredential.openshift.io/deprovision" - // AnnotationCredentialsRequest is used on Secrets created as a target of CredentailsRequests. + // LabelCredentialsRequest is to mark Secrets created as a target of CredentialsRequests. + LabelCredentialsRequest string = "cloudcredential.openshift.io/credentials-request" + LabelCredentialsRequestValue string = "true" + + // AnnotationCredentialsRequest is used on Secrets created as a target of CredentialsRequests. // The annotation value will map back to the namespace/name of the CredentialsRequest that created // or adopted the secret. AnnotationCredentialsRequest string = "cloudcredential.openshift.io/credentials-request" diff --git a/pkg/aws/actuator/actuator.go b/pkg/aws/actuator/actuator.go index 14b54305d8..1ee8886758 100644 --- a/pkg/aws/actuator/actuator.go +++ b/pkg/aws/actuator/actuator.go @@ -23,6 +23,7 @@ import ( "reflect" log "github.com/sirupsen/logrus" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/awserr" @@ -68,6 +69,7 @@ var _ actuatoriface.Actuator = (*AWSActuator)(nil) // AWSActuator implements the CredentialsRequest Actuator interface to create credentials in AWS. type AWSActuator struct { Client client.Client + RootCredClient client.Client Codec *minterv1.ProviderCodec AWSClientBuilder func(accessKeyID, secretAccessKey []byte, c client.Client) (ccaws.Client, error) Scheme *runtime.Scheme @@ -75,7 +77,7 @@ type AWSActuator struct { } // NewAWSActuator creates a new AWSActuator. -func NewAWSActuator(client client.Client, scheme *runtime.Scheme, awsSecurityTokenServiceGateEnabled bool) (*AWSActuator, error) { +func NewAWSActuator(client, rootCredClient client.Client, scheme *runtime.Scheme, awsSecurityTokenServiceGateEnabled bool) (*AWSActuator, error) { codec, err := minterv1.NewCodec() if err != nil { log.WithError(err).Error("error creating AWS codec") @@ -85,6 +87,7 @@ func NewAWSActuator(client client.Client, scheme *runtime.Scheme, awsSecurityTok return &AWSActuator{ Codec: codec, Client: client, + RootCredClient: rootCredClient, AWSClientBuilder: awsutils.ClientBuilder, Scheme: scheme, AWSSecurityTokenServiceGateEnabled: awsSecurityTokenServiceGateEnabled, @@ -362,7 +365,7 @@ func (a *AWSActuator) sync(ctx context.Context, cr *minterv1.CredentialsRequest) cloudTokenPath = "/var/run/secrets/kubernetes.io/serviceaccount/token" } if awsSTSIAMRoleARN != "" { - err = a.createSTSSecret(awsSTSIAMRoleARN, cloudTokenPath, cr.Spec.SecretRef.Name, cr.Spec.SecretRef.Namespace, logger, ctx) + err = a.syncSTSSecret(awsSTSIAMRoleARN, cloudTokenPath, cr, logger, ctx) if err != nil { return err } @@ -410,34 +413,51 @@ func (a *AWSActuator) sync(ctx context.Context, cr *minterv1.CredentialsRequest) return nil } -// createSTSSecret makes a time-based token available in a Secret in the namespace of an operator that +// syncSTSSecret makes a time-based token available in a Secret in the namespace of an operator that // has supplied the following in the CredentialsRequest: // a non-nil spec.CloudTokenString // a path to the JWT token: spec.cloudTokenPath // a spec.SecretRef.Name // a cr.Spec.SecretRef.Namespace -func (a *AWSActuator) createSTSSecret(awsSTSIAMRoleARN string, cloudTokenPath string, secretRef string, secretRefNamespace string, log log.FieldLogger, ctx context.Context) error { - log.Infof("creating secret") +func (a *AWSActuator) syncSTSSecret(awsSTSIAMRoleARN string, cloudTokenPath string, cr *minterv1.CredentialsRequest, logger log.FieldLogger, ctx context.Context) error { + sLog := logger.WithFields(log.Fields{ + "targetSecret": fmt.Sprintf("%s/%s", cr.Spec.SecretRef.Namespace, cr.Spec.SecretRef.Name), + "cr": fmt.Sprintf("%s/%s", cr.Namespace, cr.Name), + }) + sLog.Infof("processing secret") secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: secretRef, - Namespace: secretRefNamespace, - }, - StringData: map[string]string{ - "credentials": fmt.Sprintf(awsSTSCredsTemplate, awsSTSIAMRoleARN, cloudTokenPath), + Name: cr.Spec.SecretRef.Name, + Namespace: cr.Spec.SecretRef.Namespace, }, - Type: corev1.SecretTypeOpaque, } - err := a.Client.Create(ctx, secret) + op, err := controllerutil.CreateOrPatch(ctx, a.Client, secret, func() error { + if secret.Labels == nil { + secret.Labels = map[string]string{} + } + secret.Labels[minterv1.LabelCredentialsRequest] = minterv1.LabelCredentialsRequestValue + if secret.Annotations == nil { + secret.Annotations = map[string]string{} + } + secret.Annotations[minterv1.AnnotationCredentialsRequest] = fmt.Sprintf("%s/%s", cr.Namespace, cr.Name) + if secret.StringData == nil { + secret.StringData = map[string]string{} + } + secret.StringData["credentials"] = fmt.Sprintf(awsSTSCredsTemplate, awsSTSIAMRoleARN, cloudTokenPath) + secret.Type = corev1.SecretTypeOpaque + return nil + }) + sLog.WithField("operation", op).Info("processed secret") if err != nil { - log.Errorf("error creating secret") - return err + return &actuatoriface.ActuatorError{ + ErrReason: minterv1.CredentialsProvisionFailure, + Message: "error processing secret", + } } return nil } func (a *AWSActuator) syncPassthrough(ctx context.Context, cr *minterv1.CredentialsRequest, cloudCredsSecret *corev1.Secret, logger log.FieldLogger) error { - existingSecret, _, _, _ := a.loadExistingSecret(cr) accessKeyID := string(cloudCredsSecret.Data[awsannotator.AwsAccessKeyName]) secretAccessKey := string(cloudCredsSecret.Data[awsannotator.AwsSecretAccessKeyName]) @@ -507,7 +527,7 @@ func (a *AWSActuator) syncPassthrough(ctx context.Context, cr *minterv1.Credenti } // userPolicy param empty because in passthrough mode this doesn't really have any meaning - err = a.syncAccessKeySecret(cr, accessKeyID, secretAccessKey, existingSecret, "", logger) + err = a.syncAccessKeySecret(ctx, cr, accessKeyID, secretAccessKey, "", logger) if err != nil { msg := "error creating/updating secret" logger.WithError(err).Error(msg) @@ -692,7 +712,7 @@ func (a *AWSActuator) syncMint(ctx context.Context, cr *minterv1.CredentialsRequ accessKeyString = *accessKey.AccessKeyId secretAccessKeyString = *accessKey.SecretAccessKey } - err = a.syncAccessKeySecret(cr, accessKeyString, secretAccessKeyString, existingSecret, desiredUserPolicy, logger) + err = a.syncAccessKeySecret(ctx, cr, accessKeyString, secretAccessKeyString, desiredUserPolicy, logger) if err != nil { log.WithError(err).Error("error saving access key to secret") return err @@ -925,13 +945,13 @@ func (a *AWSActuator) buildRootAWSClient(cr *minterv1.CredentialsRequest) (minte logger.Debug("loading AWS credentials from secret") // TODO: Running in a 4.0 cluster we expect this secret to exist. When we run in a Hive // cluster, we need to load different secrets for each cluster. - accessKeyID, secretAccessKey, err := utils.LoadCredsFromSecret(a.Client, constants.CloudCredSecretNamespace, constants.AWSCloudCredSecretName) + accessKeyID, secretAccessKey, err := utils.LoadCredsFromSecret(a.RootCredClient, constants.CloudCredSecretNamespace, constants.AWSCloudCredSecretName) if err != nil { return nil, err } logger.Debug("creating root AWS client") - return a.AWSClientBuilder(accessKeyID, secretAccessKey, a.Client) + return a.AWSClientBuilder(accessKeyID, secretAccessKey, a.RootCredClient) } // buildReadAWSClient will return an AWS client using the the scaled down read only AWS creds @@ -995,78 +1015,65 @@ func (a *AWSActuator) getLogger(cr *minterv1.CredentialsRequest) log.FieldLogger }) } -func (a *AWSActuator) syncAccessKeySecret(cr *minterv1.CredentialsRequest, accessKeyID, secretAccessKey string, existingSecret *corev1.Secret, userPolicy string, logger log.FieldLogger) error { +func (a *AWSActuator) syncAccessKeySecret(ctx context.Context, cr *minterv1.CredentialsRequest, accessKeyID, secretAccessKey string, userPolicy string, logger log.FieldLogger) error { sLog := logger.WithFields(log.Fields{ "targetSecret": fmt.Sprintf("%s/%s", cr.Spec.SecretRef.Namespace, cr.Spec.SecretRef.Name), "cr": fmt.Sprintf("%s/%s", cr.Namespace, cr.Name), }) + sLog.Infof("processing secret") - if existingSecret == nil || existingSecret.Name == "" { - if accessKeyID == "" || secretAccessKey == "" { - msg := "new access key secret needed but no key data provided" - sLog.Error(msg) - return &actuatoriface.ActuatorError{ - ErrReason: minterv1.CredentialsProvisionFailure, - Message: msg, + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: cr.Spec.SecretRef.Name, + Namespace: cr.Spec.SecretRef.Namespace, + }, + } + op, err := controllerutil.CreateOrPatch(ctx, a.Client, secret, func() error { + if secret.Labels == nil { + secret.Labels = map[string]string{} + } + secret.Labels[minterv1.LabelCredentialsRequest] = minterv1.LabelCredentialsRequestValue + + if secret.Annotations == nil { + secret.Annotations = map[string]string{} + } + secret.Annotations[minterv1.AnnotationCredentialsRequest] = fmt.Sprintf("%s/%s", cr.Namespace, cr.Name) + secret.Annotations[minterv1.AnnotationAWSPolicyLastApplied] = userPolicy + + if secret.Data == nil { + secret.Data = map[string][]byte{} + } + // either we know the access key ID and secret access key, or we get them from the secret itself + for identifier, into := range map[string]*string{ + secretDataAccessKey: &accessKeyID, + secretDataSecretKey: &secretAccessKey, + } { + if *into == "" { + value, exists := secret.Data[identifier] + if !exists { + return &actuatoriface.ActuatorError{ + ErrReason: minterv1.CredentialsProvisionFailure, + Message: fmt.Sprintf("%s needed but no data provided", identifier), + } + } + *into = string(value) + } else { + secret.Data[identifier] = []byte(*into) } } - sLog.Info("creating secret") - secret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: cr.Spec.SecretRef.Name, - Namespace: cr.Spec.SecretRef.Namespace, - Annotations: map[string]string{ - minterv1.AnnotationCredentialsRequest: fmt.Sprintf("%s/%s", cr.Namespace, cr.Name), - minterv1.AnnotationAWSPolicyLastApplied: userPolicy, - }, - }, - Data: map[string][]byte{ - secretDataAccessKey: []byte(accessKeyID), - secretDataSecretKey: []byte(secretAccessKey), - constants.AWSSecretDataCredentialsKey: generateAWSCredentialsConfig(accessKeyID, secretAccessKey), - }, - } - - err := a.Client.Create(context.TODO(), secret) - if err != nil { - sLog.WithError(err).Error("error creating secret") - return err - } - sLog.Info("secret created successfully") - return nil - } - // Update the existing secret: - sLog.Debug("updating secret") - origSecret := existingSecret.DeepCopy() - if existingSecret.Annotations == nil { - existingSecret.Annotations = map[string]string{} - } - existingSecret.Annotations[minterv1.AnnotationCredentialsRequest] = fmt.Sprintf("%s/%s", cr.Namespace, cr.Name) - existingSecret.Annotations[minterv1.AnnotationAWSPolicyLastApplied] = userPolicy - if accessKeyID != "" && 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[constants.AWSSecretDataCredentialsKey] = generateAWSCredentialsConfig(string(existingSecret.Data[secretDataAccessKey]), string(existingSecret.Data[secretDataSecretKey])) + // Make sure credentials config data is synced with the stored access key / secret key + secret.Data[constants.AWSSecretDataCredentialsKey] = generateAWSCredentialsConfig(accessKeyID, secretAccessKey) - if !reflect.DeepEqual(existingSecret, origSecret) { - sLog.Info("target secret has changed, updating") - err := a.Client.Update(context.TODO(), existingSecret) - if err != nil { - msg := "error updating secret" - sLog.WithError(err).Error(msg) - return &actuatoriface.ActuatorError{ - ErrReason: minterv1.CredentialsProvisionFailure, - Message: msg, - } + return nil + }) + sLog.WithField("operation", op).Info("processed secret") + if err != nil { + return &actuatoriface.ActuatorError{ + ErrReason: minterv1.CredentialsProvisionFailure, + Message: "error processing secret", } - } else { - sLog.Debug("target secret unchanged") } - return nil } @@ -1104,7 +1111,7 @@ func (a *AWSActuator) GetCredentialsRootSecretLocation() types.NamespacedName { func (a *AWSActuator) GetCredentialsRootSecret(ctx context.Context, cr *minterv1.CredentialsRequest) (*corev1.Secret, error) { logger := a.getLogger(cr) cloudCredSecret := &corev1.Secret{} - if err := a.Client.Get(ctx, a.GetCredentialsRootSecretLocation(), cloudCredSecret); err != nil { + if err := a.RootCredClient.Get(ctx, a.GetCredentialsRootSecretLocation(), cloudCredSecret); err != nil { msg := "unable to fetch root cloud cred secret" logger.WithError(err).Error(msg) return nil, &actuatoriface.ActuatorError{ @@ -1392,7 +1399,7 @@ func awsSTSIAMRoleARN(codec *minterv1.ProviderCodec, credentialsRequest *minterv // if the system is considered not upgradeable. Otherwise, return nil as the default // value is for things to be upgradeable. func (a *AWSActuator) Upgradeable(mode operatorv1.CloudCredentialsMode) *configv1.ClusterOperatorStatusCondition { - return utils.UpgradeableCheck(a.Client, mode, a.GetCredentialsRootSecretLocation()) + return utils.UpgradeableCheck(a.RootCredClient, mode, a.GetCredentialsRootSecretLocation()) } func generateAWSCredentialsConfig(accessKeyID, secretAccessKey string) []byte { diff --git a/pkg/aws/actuator/actuator_test.go b/pkg/aws/actuator/actuator_test.go index 6b8e6fe8a1..f8d38c5df3 100644 --- a/pkg/aws/actuator/actuator_test.go +++ b/pkg/aws/actuator/actuator_test.go @@ -84,6 +84,7 @@ func TestCredentialsFetching(t *testing.T) { tests := []struct { name string existing []runtime.Object + existingAdmin []runtime.Object credentialsRequest *minterv1.CredentialsRequest expectedError bool validate func(*testing.T, *awsClientBuilderRecorder) @@ -110,8 +111,9 @@ func TestCredentialsFetching(t *testing.T) { }, }, { - name: "no read only secret", - existing: []runtime.Object{ + name: "no read only secret", + existing: []runtime.Object{}, + existingAdmin: []runtime.Object{ testRootSecret(), }, clientBuilderRecorder: func(mockCtrl *gomock.Controller) *awsClientBuilderRecorder { @@ -131,6 +133,8 @@ func TestCredentialsFetching(t *testing.T) { name: "read only creds not ready", existing: []runtime.Object{ testReadOnlySecret(), + }, + existingAdmin: []runtime.Object{ testRootSecret(), }, clientBuilderRecorder: func(mockCtrl *gomock.Controller) *awsClientBuilderRecorder { @@ -196,6 +200,7 @@ func TestCredentialsFetching(t *testing.T) { test.existing = append(test.existing, test.credentialsRequest) fakeClient := fake.NewClientBuilder().WithRuntimeObjects(test.existing...).Build() + fakeAdminClient := fake.NewClientBuilder().WithRuntimeObjects(test.existingAdmin...).Build() mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() @@ -204,6 +209,7 @@ func TestCredentialsFetching(t *testing.T) { a := &AWSActuator{ Client: fakeClient, + RootCredClient: fakeAdminClient, Codec: codec, AWSClientBuilder: clientRecorder.ClientBuilder, } @@ -363,8 +369,8 @@ func TestUpgradeable(t *testing.T) { defer mockCtrl.Finish() a := &AWSActuator{ - Client: fakeClient, - Codec: codec, + RootCredClient: fakeClient, + Codec: codec, } cond := a.Upgradeable(test.mode) @@ -458,7 +464,7 @@ func TestSecretFormat(t *testing.T) { cr := testCredentialsRequest() logger := a.getLogger(cr) - err := a.syncAccessKeySecret(cr, test.accessKeyID, test.secretAccessKey, test.existingSecret, "exampleAWSPolicy", logger) + err := a.syncAccessKeySecret(context.Background(), cr, test.accessKeyID, test.secretAccessKey, "exampleAWSPolicy", logger) require.NoError(t, err, "unexpected error creating/updating Secret") @@ -653,12 +659,14 @@ func TestDetectSTS(t *testing.T) { WithScheme(scheme.Scheme). WithStatusSubresource(&minterv1.CredentialsRequest{}). WithRuntimeObjects(test.existing...).Build() + fakeAdminClient := fake.NewClientBuilder().Build() err := fakeClient.Create(context.TODO(), testAuthentication(test.issuer)) if err != nil { panic(err) } a := &AWSActuator{ Client: fakeClient, + RootCredClient: fakeAdminClient, Codec: codec, AWSSecurityTokenServiceGateEnabled: test.stsEnabled, } diff --git a/pkg/azure/actuator.go b/pkg/azure/actuator.go index 55185d8a9f..42f1651994 100644 --- a/pkg/azure/actuator.go +++ b/pkg/azure/actuator.go @@ -23,6 +23,7 @@ import ( "reflect" log "github.com/sirupsen/logrus" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" corev1 "k8s.io/api/core/v1" kerrors "k8s.io/apimachinery/pkg/api/errors" @@ -53,14 +54,14 @@ func (a *Actuator) STSFeatureGateEnabled() bool { return false } -func NewActuator(c client.Client, cloudName configv1.AzureCloudEnvironment) (*Actuator, error) { +func NewActuator(c, rootCredClient client.Client, cloudName configv1.AzureCloudEnvironment) (*Actuator, error) { codec, err := minterv1.NewCodec() if err != nil { log.WithError(err).Error("error creating Azure codec") return nil, fmt.Errorf("error creating Azure codec: %v", err) } - client := newClientWrapper(c) + client := newClientWrapper(c, rootCredClient) return &Actuator{ client: client, codec: codec, @@ -70,11 +71,11 @@ func NewActuator(c client.Client, cloudName configv1.AzureCloudEnvironment) (*Ac }, nil } -func NewFakeActuator(c client.Client, codec *minterv1.ProviderCodec, +func NewFakeActuator(c, rootCredClient client.Client, codec *minterv1.ProviderCodec, credentialMinterBuilder credentialMinterBuilder, ) *Actuator { return &Actuator{ - client: newClientWrapper(c), + client: newClientWrapper(c, rootCredClient), codec: codec, credentialMinterBuilder: credentialMinterBuilder, } @@ -325,25 +326,6 @@ func (a *Actuator) updateProviderStatus(ctx context.Context, logger log.FieldLog return nil } -func copyCredentialsSecret(cr *minterv1.CredentialsRequest, src, dest *corev1.Secret) { - dest.ObjectMeta = metav1.ObjectMeta{ - Name: cr.Spec.SecretRef.Name, - Namespace: cr.Spec.SecretRef.Namespace, - Annotations: map[string]string{ - minterv1.AnnotationCredentialsRequest: fmt.Sprintf("%s/%s", cr.Namespace, cr.Name), - }, - } - dest.Data = map[string][]byte{ - AzureClientID: src.Data[AzureClientID], - AzureClientSecret: src.Data[AzureClientSecret], - AzureRegion: src.Data[AzureRegion], - AzureResourceGroup: src.Data[AzureResourceGroup], - AzureResourcePrefix: src.Data[AzureResourcePrefix], - AzureSubscriptionID: src.Data[AzureSubscriptionID], - AzureTenantID: src.Data[AzureTenantID], - } -} - func (a *Actuator) syncPassthrough(ctx context.Context, cr *minterv1.CredentialsRequest, cloudCredsSecret *corev1.Secret, logger log.FieldLogger) error { syncErr := a.syncCredentialSecrets(ctx, cr, cloudCredsSecret, logger) if syncErr != nil { @@ -405,26 +387,43 @@ func (a *Actuator) cleanupAfterPassthroughPivot(ctx context.Context, cr *minterv return nil } func (a *Actuator) syncCredentialSecrets(ctx context.Context, cr *minterv1.CredentialsRequest, cloudCredsSecret *corev1.Secret, logger log.FieldLogger) error { - existing := &corev1.Secret{} - key := client.ObjectKey{Namespace: cr.Spec.SecretRef.Namespace, Name: cr.Spec.SecretRef.Name} - err := a.client.Get(ctx, key, existing) - if err != nil && kerrors.IsNotFound(err) { - s := &corev1.Secret{} - copyCredentialsSecret(cr, cloudCredsSecret, s) - return a.client.Create(ctx, s) - } else if err != nil { - return err + sLog := logger.WithFields(log.Fields{ + "targetSecret": fmt.Sprintf("%s/%s", cr.Spec.SecretRef.Namespace, cr.Spec.SecretRef.Name), + "cr": fmt.Sprintf("%s/%s", cr.Namespace, cr.Name), + }) + sLog.Infof("processing secret") + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: cr.Spec.SecretRef.Name, + Namespace: cr.Spec.SecretRef.Namespace, + }, } - - updated := existing.DeepCopy() - copyCredentialsSecret(cr, cloudCredsSecret, updated) - if !reflect.DeepEqual(existing, updated) { - err := a.client.Update(ctx, updated) - if err != nil { - return &actuatoriface.ActuatorError{ - ErrReason: minterv1.CredentialsProvisionFailure, - Message: "error updating secret", - } + op, err := controllerutil.CreateOrPatch(ctx, a.client, secret, func() error { + if secret.Labels == nil { + secret.Labels = map[string]string{} + } + secret.Labels[minterv1.LabelCredentialsRequest] = minterv1.LabelCredentialsRequestValue + if secret.Annotations == nil { + secret.Annotations = map[string]string{} + } + secret.Annotations[minterv1.AnnotationCredentialsRequest] = fmt.Sprintf("%s/%s", cr.Namespace, cr.Name) + if secret.Data == nil { + secret.Data = map[string][]byte{} + } + secret.Data[AzureClientID] = cloudCredsSecret.Data[AzureClientID] + secret.Data[AzureClientSecret] = cloudCredsSecret.Data[AzureClientSecret] + secret.Data[AzureRegion] = cloudCredsSecret.Data[AzureRegion] + secret.Data[AzureResourceGroup] = cloudCredsSecret.Data[AzureResourceGroup] + secret.Data[AzureResourcePrefix] = cloudCredsSecret.Data[AzureResourcePrefix] + secret.Data[AzureSubscriptionID] = cloudCredsSecret.Data[AzureSubscriptionID] + secret.Data[AzureTenantID] = cloudCredsSecret.Data[AzureTenantID] + return nil + }) + sLog.WithField("operation", op).Info("processed secret") + if err != nil { + return &actuatoriface.ActuatorError{ + ErrReason: minterv1.CredentialsProvisionFailure, + Message: "error processing secret", } } return nil @@ -438,7 +437,7 @@ func (a *Actuator) GetCredentialsRootSecretLocation() types.NamespacedName { func (a *Actuator) GetCredentialsRootSecret(ctx context.Context, cr *minterv1.CredentialsRequest) (*corev1.Secret, error) { logger := a.getLogger(cr) cloudCredSecret := &corev1.Secret{} - if err := a.client.Client.Get(ctx, a.GetCredentialsRootSecretLocation(), cloudCredSecret); err != nil { + if err := a.client.RootCredClient.Get(ctx, a.GetCredentialsRootSecretLocation(), cloudCredSecret); err != nil { msg := "unable to fetch root cloud cred secret" logger.WithError(err).Error(msg) return nil, &actuatoriface.ActuatorError{ @@ -507,5 +506,5 @@ func (a *Actuator) getLogger(cr *minterv1.CredentialsRequest) log.FieldLogger { // if the system is considered not upgradeable. Otherwise, return nil as the default // value is for things to be upgradeable. func (a *Actuator) Upgradeable(mode operatorv1.CloudCredentialsMode) *configv1.ClusterOperatorStatusCondition { - return utils.UpgradeableCheck(a.client.Client, mode, a.GetCredentialsRootSecretLocation()) + return utils.UpgradeableCheck(a.client.RootCredClient, mode, a.GetCredentialsRootSecretLocation()) } diff --git a/pkg/azure/actuator_test.go b/pkg/azure/actuator_test.go index 8a435f828d..24773fff47 100644 --- a/pkg/azure/actuator_test.go +++ b/pkg/azure/actuator_test.go @@ -117,16 +117,6 @@ var ( }, } - rootSecretBadAnnotation = corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: constants.AzureCloudCredSecretName, - Namespace: constants.CloudCredSecretNamespace, - Annotations: map[string]string{ - constants.AnnotationKey: "blah", - }, - }, - } - rootSecretNoAnnotation = corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: constants.AzureCloudCredSecretName, @@ -135,13 +125,6 @@ var ( }, } - validSecret = corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: validName, - Namespace: validNamespace, - }, - } - clusterInfra = openshiftapiv1.Infrastructure{ ObjectMeta: metav1.ObjectMeta{ Name: "cluster", @@ -190,32 +173,6 @@ func TestDecodeToUnknown(t *testing.T) { } } -func TestAnnotations(t *testing.T) { - tests := []struct { - name string - in corev1.Secret - errRegexp string - }{ - {"TestValidSecretAnnotation", validPassthroughRootSecret, ""}, - {"TestBadSecretAnnotation", rootSecretBadAnnotation, "invalid mode"}, - {"TestMissingSecretAnnotation", rootSecretNoAnnotation, "cannot proceed without cloud cred secret annotation.*"}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - f := fake.NewClientBuilder().WithRuntimeObjects(&tt.in, &validSecret).Build() - actuator, err := azure.NewActuator(f, openshiftapiv1.AzurePublicCloud) - if err != nil { - assert.Regexp(t, tt.errRegexp, err) - assert.Nil(t, actuator) - return - } - assert.Nil(t, err) - assert.NotNil(t, actuator) - }) - } -} - func getCredRequest(t *testing.T, c client.Client) *minterv1.CredentialsRequest { cr := &minterv1.CredentialsRequest{} require.NoError(t, c.Get(context.TODO(), types.NamespacedName{Namespace: testNamespace, Name: testCredRequestName}, cr), "error while retriving credreq from fake client") @@ -251,25 +208,27 @@ func TestActuator(t *testing.T) { tests := []struct { name string existing []runtime.Object + existingAdmin []runtime.Object mockAppClient func(*gomock.Controller) *azuremock.MockAppClient op func(*azure.Actuator, *minterv1.CredentialsRequest) error credentialsRequest *minterv1.CredentialsRequest expectedErr error - validate func(*testing.T, client.Client) + validate func(*testing.T, client.Client, client.Client) }{ { name: "Process a CredentialsRequest", existing: defaultExistingObjects(), + existingAdmin: []runtime.Object{&validPassthroughRootSecret}, credentialsRequest: testCredentialsRequest(t), op: func(actuator *azure.Actuator, cr *minterv1.CredentialsRequest) error { return actuator.Create(context.TODO(), cr) }, - validate: func(t *testing.T, c client.Client) { + validate: func(t *testing.T, c client.Client, adminC client.Client) { cr := getCredRequest(t, c) targetSecret := getCredRequestTargetSecret(t, c, cr) - rootSecret := getRootSecret(t, c) + rootSecret := getRootSecret(t, adminC) assertSecretEquality(t, rootSecret, targetSecret) }, @@ -301,6 +260,7 @@ func TestActuator(t *testing.T) { return objects }(), + existingAdmin: []runtime.Object{&validPassthroughRootSecret}, credentialsRequest: func() *minterv1.CredentialsRequest { // Create a credreq that resembles what one previously handled via mint mode // would look like. @@ -325,12 +285,12 @@ func TestActuator(t *testing.T) { op: func(actuator *azure.Actuator, cr *minterv1.CredentialsRequest) error { return actuator.Update(context.TODO(), cr) }, - validate: func(t *testing.T, c client.Client) { + validate: func(t *testing.T, c client.Client, adminC client.Client) { cr := getCredRequest(t, c) targetSecret := getCredRequestTargetSecret(t, c, cr) - rootSecret := getRootSecret(t, c) + rootSecret := getRootSecret(t, adminC) // Post mint-to-passthrough pivot the targetSecret should be a copy of the // root secret. @@ -378,6 +338,7 @@ func TestActuator(t *testing.T) { return objects }(), + existingAdmin: []runtime.Object{&validPassthroughRootSecret}, credentialsRequest: func() *minterv1.CredentialsRequest { // Create a credreq that resembles what one previously handled via mint mode // would look like. @@ -404,12 +365,12 @@ func TestActuator(t *testing.T) { op: func(actuator *azure.Actuator, cr *minterv1.CredentialsRequest) error { return actuator.Update(context.TODO(), cr) }, - validate: func(t *testing.T, c client.Client) { + validate: func(t *testing.T, c client.Client, adminC client.Client) { cr := getCredRequest(t, c) targetSecret := getCredRequestTargetSecret(t, c, cr) - rootSecret := getRootSecret(t, c) + rootSecret := getRootSecret(t, adminC) // The targetSecret should be a copy of the root secret. assertSecretEquality(t, rootSecret, targetSecret) @@ -460,6 +421,7 @@ func TestActuator(t *testing.T) { return objects }(), + existingAdmin: []runtime.Object{&validPassthroughRootSecret}, credentialsRequest: func() *minterv1.CredentialsRequest { // Create a credreq that resembles what one previously handled via mint mode // would look like. @@ -480,12 +442,12 @@ func TestActuator(t *testing.T) { op: func(actuator *azure.Actuator, cr *minterv1.CredentialsRequest) error { return actuator.Update(context.TODO(), cr) }, - validate: func(t *testing.T, c client.Client) { + validate: func(t *testing.T, c client.Client, adminC client.Client) { cr := getCredRequest(t, c) targetSecret := getCredRequestTargetSecret(t, c, cr) - rootSecret := getRootSecret(t, c) + rootSecret := getRootSecret(t, adminC) // The targetSecret should be a copy of the root secret. assertSecretEquality(t, rootSecret, targetSecret) @@ -503,10 +465,9 @@ func TestActuator(t *testing.T) { name: "Missing annotation", expectedErr: fmt.Errorf("error determining whether a credentials update is needed"), existing: func() []runtime.Object { - objects := []runtime.Object{&rootSecretNoAnnotation} - - return objects + return nil }(), + existingAdmin: []runtime.Object{&rootSecretNoAnnotation}, credentialsRequest: testCredentialsRequest(t), op: func(actuator *azure.Actuator, cr *minterv1.CredentialsRequest) error { return actuator.Create(context.TODO(), cr) @@ -516,10 +477,9 @@ func TestActuator(t *testing.T) { name: "Mint annotation", expectedErr: fmt.Errorf("error determining whether a credentials update is needed"), existing: func() []runtime.Object { - objects := []runtime.Object{&rootSecretMintAnnotation} - - return objects + return nil }(), + existingAdmin: []runtime.Object{&rootSecretMintAnnotation}, credentialsRequest: testCredentialsRequest(t), op: func(actuator *azure.Actuator, cr *minterv1.CredentialsRequest) error { return actuator.Create(context.TODO(), cr) @@ -533,6 +493,8 @@ func TestActuator(t *testing.T) { fakeClient := fake.NewClientBuilder(). WithStatusSubresource(&minterv1.CredentialsRequest{}). WithRuntimeObjects(allObjects...).Build() + fakeAdminClient := fake.NewClientBuilder(). + WithRuntimeObjects(test.existingAdmin...).Build() mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() @@ -543,6 +505,7 @@ func TestActuator(t *testing.T) { actuator := azure.NewFakeActuator( fakeClient, + fakeAdminClient, codec, func(logger log.FieldLogger, clientID, clientSecret, tenantID, subscriptionID string) (*azure.AzureCredentialsMinter, error) { return azure.NewFakeAzureCredentialsMinter(logger, @@ -561,11 +524,11 @@ func TestActuator(t *testing.T) { assert.Error(t, testErr) assert.Equal(t, test.expectedErr.Error(), testErr.Error()) if test.validate != nil { - test.validate(t, fakeClient) + test.validate(t, fakeClient, fakeAdminClient) } } else { require.NoError(t, testErr, "unexpected error returned during test case") - test.validate(t, fakeClient) + test.validate(t, fakeClient, fakeAdminClient) } }) } @@ -631,7 +594,6 @@ func mockAppClientNoCalls(mockCtrl *gomock.Controller) *azuremock.MockAppClient func defaultExistingObjects() []runtime.Object { objs := []runtime.Object{ &clusterInfra, - &validPassthroughRootSecret, &clusterDNS, } return objs diff --git a/pkg/azure/client.go b/pkg/azure/client.go index 59b1212deb..70e414fa42 100644 --- a/pkg/azure/client.go +++ b/pkg/azure/client.go @@ -31,14 +31,15 @@ var RootSecretKey = client.ObjectKey{Name: constants.AzureCloudCredSecretName, N type clientWrapper struct { client.Client + RootCredClient client.Client } -func newClientWrapper(c client.Client) *clientWrapper { - return &clientWrapper{Client: c} +func newClientWrapper(c, rootC client.Client) *clientWrapper { + return &clientWrapper{Client: c, RootCredClient: rootC} } func (cw *clientWrapper) RootSecret(ctx context.Context) (*secret, error) { - secret, err := cw.Secret(ctx, RootSecretKey) + secret, err := cw.secret(ctx, cw.RootCredClient, RootSecretKey) if err != nil { return nil, err } @@ -54,8 +55,12 @@ func (cw *clientWrapper) RootSecret(ctx context.Context) (*secret, error) { } func (cw *clientWrapper) Secret(ctx context.Context, key client.ObjectKey) (*secret, error) { + return cw.secret(ctx, cw.Client, key) +} + +func (cw *clientWrapper) secret(ctx context.Context, c client.Client, key client.ObjectKey) (*secret, error) { s := &secret{} - if err := cw.Get(ctx, key, &s.Secret); err != nil { + if err := c.Get(ctx, key, &s.Secret); err != nil { return nil, err } return s, nil diff --git a/pkg/cmd/operator/cmd.go b/pkg/cmd/operator/cmd.go index 78c6442747..b33afe543f 100644 --- a/pkg/cmd/operator/cmd.go +++ b/pkg/cmd/operator/cmd.go @@ -24,6 +24,7 @@ import ( "os" "os/signal" "path/filepath" + "sync" "syscall" "time" @@ -31,12 +32,18 @@ import ( "github.com/golang/glog" "github.com/google/uuid" "github.com/openshift/cloud-credential-operator/pkg/operator/constants" + "github.com/openshift/cloud-credential-operator/pkg/operator/credentialsrequest" log "github.com/sirupsen/logrus" "github.com/spf13/cobra" "github.com/spf13/pflag" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/types" + corev1client "k8s.io/client-go/kubernetes/typed/core/v1" "k8s.io/client-go/rest" + "k8s.io/client-go/tools/clientcmd" "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" @@ -99,8 +106,65 @@ func NewOperator() *cobra.Command { // logger to do nothing. ctrlruntimelog.SetLogger(logr.New(ctrlruntimelog.NullLogSink{})) + log.Info("checking prerequisites") + featureGates, err := platform.GetFeatureGates(ctx) + if err != nil { + log.WithError(err).Fatal("unable to read feature gates") + } + awsSecurityTokenServiceGateEnabled := featureGates.Enabled(configv1.FeatureGateAWSSecurityTokenService) + + kubeconfigCommandLinePath := cmd.PersistentFlags().Lookup("kubeconfig").Value.String() + rules := clientcmd.NewDefaultClientConfigLoadingRules() + rules.ExplicitPath = kubeconfigCommandLinePath + kubeconfig := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(rules, &clientcmd.ConfigOverrides{}) + cfg, err := kubeconfig.ClientConfig() + if err != nil { + log.WithError(err).Fatal("failed to parse kubeconfig") + } + coreClient, err := corev1client.NewForConfig(cfg) + if err != nil { + log.WithError(err).Fatal("failed to set up client") + } + + infraStatus, err := platform.GetInfraStatusUsingKubeconfig(kubeconfigCommandLinePath) + if err != nil { + log.Fatal(err) + } + platformType := platform.GetType(infraStatus) + + // The status quo for this controller is to LIST + WATCH all Secrets on the cluster. This consumes + // more resources than necessary on clusters where users put other data in Secrets themselves, as we + // hold that data in our cache and never do anything with it. The reconcilers mainly need to react to + // changes in Secrets created for CredentialRequests, which they control and can label, allowing us + // to filter the LIST + WATCH down and hold the minimal set of data in memory. However, on existing + // clusters, secrets exist from previous work these reconcilers did that will not have Secrets labelled. + // + // We solve this second issue two-fold: + // - a new controller that labels all previous Secrets, but does not restrict the watch stream + // - a check on startup that detects that no Secrets remain to be labelled and applies the filtering + var filteredWatchPossible bool + secrets, err := coreClient.Secrets(metav1.NamespaceAll).List(ctx, metav1.ListOptions{}) + if err != nil { + log.WithError(err).Fatal("failed to list secrets") + } + var missing []types.NamespacedName + for _, secret := range secrets.Items { + if credentialsrequest.IsMissingSecretLabel(&secret) { + missing = append(missing, types.NamespacedName{ + Namespace: secret.Namespace, + Name: secret.Name, + }) + } + } + if len(missing) != 0 { + log.WithField("missing", missing).Warn("not all secrets labelled, falling back to caching all secrets on cluster") + } else { + log.Info("filtering the set of secrets we watch") + filteredWatchPossible = true + } + // Create a new Cmd to provide shared dependencies and start components - log.Info("setting up manager") + log.Info("setting up managers") mgr, err := manager.New(cfg, manager.Options{ MetricsBindAddress: ":2112", NewCache: func(config *rest.Config, opts cache.Options) (cache.Cache, error) { @@ -113,6 +177,14 @@ func NewOperator() *cobra.Command { "metadata.name": constants.CloudCredOperatorConfigMap, }), } + if filteredWatchPossible { + log.Infof("adding label selector %s=%s to cache options for Secrets", minterv1.LabelCredentialsRequest, minterv1.LabelCredentialsRequestValue) + opts.ByObject[&corev1.Secret{}] = cache.ByObject{ + Label: labels.SelectorFromSet(labels.Set{ + minterv1.LabelCredentialsRequest: minterv1.LabelCredentialsRequestValue, + }), + } + } return cache.New(config, opts) }, }) @@ -120,28 +192,55 @@ func NewOperator() *cobra.Command { log.WithError(err).Fatal("unable to set up overall controller manager") } + rootMgr, err := manager.New(cfg, manager.Options{ + MetricsBindAddress: ":2113", + NewCache: func(config *rest.Config, opts cache.Options) (cache.Cache, error) { + if opts.ByObject == nil { + opts.ByObject = map[client.Object]cache.ByObject{} + } + opts.ByObject[&corev1.Secret{}] = cache.ByObject{ + Field: selectorForRootCredential(platformType), + } + return cache.New(config, opts) + }, + }) + if err != nil { + log.WithError(err).Fatal("unable to set up root credential controller manager") + } + log.Info("registering components") // Setup Scheme for all resources util.SetupScheme(mgr.GetScheme()) - - featureGates, err := platform.GetFeatureGates(ctx) - if err != nil { - log.WithError(err).Fatal("unable to read feature gates") - } - awsSecurityTokenServiveGateEnaled := featureGates.Enabled(configv1.FeatureGateAWSSecurityTokenService) + util.SetupScheme(rootMgr.GetScheme()) // Setup all Controllers - log.Info("setting up controller") - kubeconfigCommandLinePath := cmd.PersistentFlags().Lookup("kubeconfig").Value.String() - if err := controller.AddToManager(mgr, kubeconfigCommandLinePath, awsSecurityTokenServiveGateEnaled); err != nil { + log.Info("setting up controllers") + if err := controller.AddToManager(mgr, rootMgr, kubeconfigCommandLinePath, coreClient, awsSecurityTokenServiceGateEnabled); err != nil { log.WithError(err).Fatal("unable to register controllers to the manager") } - // Start the Cmd - log.Info("starting the cmd") - if err := mgr.Start(signals.SetupSignalHandler()); err != nil { - log.WithError(err).Fatal("unable to run the manager") + // Start the managers + log.Info("starting the managers") + runCtx := signals.SetupSignalHandler() + errs := make(chan error) + wg := sync.WaitGroup{} + for _, m := range []manager.Manager{mgr, rootMgr} { + wg.Add(1) + go func(m manager.Manager, ctx context.Context) { + defer wg.Done() + errs <- m.Start(ctx) + + }(m, runCtx) + } + go func() { + wg.Wait() + close(errs) + }() + for err := range errs { + if err != nil { + log.WithError(err).Fatal("unable to run the manager") + } } } @@ -230,6 +329,34 @@ func NewOperator() *cobra.Command { return cmd } +func selectorForRootCredential(platformType configv1.PlatformType) fields.Selector { + var name string + switch platformType { + case configv1.AWSPlatformType: + name = constants.AWSCloudCredSecretName + case configv1.AzurePlatformType: + name = constants.AzureCloudCredSecretName + case configv1.GCPPlatformType: + name = constants.GCPCloudCredSecretName + case configv1.OpenStackPlatformType: + name = constants.OpenStackCloudCredsSecretName + case configv1.OvirtPlatformType: + name = constants.OvirtCloudCredsSecretName + case configv1.KubevirtPlatformType: + name = constants.KubevirtCloudCredSecretName + case configv1.VSpherePlatformType: + name = constants.VSphereCloudCredSecretName + default: + return fields.Nothing() + } + selector := fields.SelectorFromSet(fields.Set{ + "metadata.namespace": constants.CloudCredSecretNamespace, + "metadata.name": name, + }) + log.WithField("selector", selector.String()).Info("setting up field selector for root credential Secret") + return selector +} + func initializeGlog(flags *pflag.FlagSet) { golog.SetOutput(glogWriter{}) // Redirect all regular go log output to glog golog.SetFlags(0) diff --git a/pkg/gcp/actuator/actuator.go b/pkg/gcp/actuator/actuator.go index 6f4b5d021c..29429f13fc 100644 --- a/pkg/gcp/actuator/actuator.go +++ b/pkg/gcp/actuator/actuator.go @@ -22,6 +22,7 @@ import ( "reflect" log "github.com/sirupsen/logrus" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" // GCP packages iamadminpb "google.golang.org/genproto/googleapis/iam/admin/v1" @@ -60,6 +61,7 @@ var _ actuatoriface.Actuator = (*Actuator)(nil) type Actuator struct { ProjectName string Client client.Client + RootCredClient client.Client Codec *minterv1.ProviderCodec GCPClientBuilder func(string, []byte) (ccgcp.Client, error) } @@ -69,7 +71,7 @@ func (a *Actuator) STSFeatureGateEnabled() bool { } // NewActuator initializes and returns a new Actuator for GCP. -func NewActuator(c client.Client, projectName string) (*Actuator, error) { +func NewActuator(c, rootCredClient client.Client, projectName string) (*Actuator, error) { codec, err := minterv1.NewCodec() if err != nil { log.WithError(err).Error("error creating GCP codec") @@ -79,6 +81,7 @@ func NewActuator(c client.Client, projectName string) (*Actuator, error) { return &Actuator{ ProjectName: projectName, Client: c, + RootCredClient: rootCredClient, Codec: codec, GCPClientBuilder: ccgcp.NewClientFromJSON, }, nil @@ -289,7 +292,7 @@ func (a *Actuator) syncPassthrough(ctx context.Context, cr *minterv1.Credentials } } - err = a.syncSecret(cr, rootAuthJSONByes, logger) + err = a.syncSecret(ctx, cr, rootAuthJSONByes, logger) if err != nil { msg := "error creating/updating secret" logger.WithError(err).Error(msg) @@ -466,7 +469,7 @@ func (a *Actuator) syncMint(ctx context.Context, cr *minterv1.CredentialsRequest // Save key into secret if key != nil { - err = a.syncSecret(cr, key.PrivateKeyData, logger) + err = a.syncSecret(ctx, cr, key.PrivateKeyData, logger) } return err @@ -648,7 +651,7 @@ func (a *Actuator) buildRootGCPClient(cr *minterv1.CredentialsRequest) (ccgcp.Cl logger := a.getLogger(cr).WithField("secret", fmt.Sprintf("%s/%s", constants.CloudCredSecretNamespace, constants.GCPCloudCredSecretName)) logger.Debug("loading GCP credentials from secret") - jsonBytes, err := loadCredsFromSecret(a.Client, constants.CloudCredSecretNamespace, constants.GCPCloudCredSecretName) + jsonBytes, err := loadCredsFromSecret(a.RootCredClient, constants.CloudCredSecretNamespace, constants.GCPCloudCredSecretName) if err != nil { return nil, err } @@ -686,7 +689,7 @@ func (a *Actuator) GetCredentialsRootSecretLocation() types.NamespacedName { func (a *Actuator) GetCredentialsRootSecret(ctx context.Context, cr *minterv1.CredentialsRequest) (*corev1.Secret, error) { logger := a.getLogger(cr) cloudCredSecret := &corev1.Secret{} - if err := a.Client.Get(ctx, a.GetCredentialsRootSecretLocation(), cloudCredSecret); err != nil { + if err := a.RootCredClient.Get(ctx, a.GetCredentialsRootSecretLocation(), cloudCredSecret); err != nil { msg := "unable to fetch root cloud cred secret" logger.WithError(err).Error(msg) return nil, &actuatoriface.ActuatorError{ @@ -775,63 +778,40 @@ func (a *Actuator) secretAlreadySynced(cr *minterv1.CredentialsRequest) (bool, e return true, nil } -func (a *Actuator) syncSecret(cr *minterv1.CredentialsRequest, privateKeyData []byte, logger log.FieldLogger) error { +func (a *Actuator) syncSecret(ctx context.Context, cr *minterv1.CredentialsRequest, privateKeyData []byte, logger log.FieldLogger) error { sLog := logger.WithFields(log.Fields{ "targetSecret": fmt.Sprintf("%s/%s", cr.Spec.SecretRef.Namespace, cr.Spec.SecretRef.Name), "cr": fmt.Sprintf("%s/%s", cr.Namespace, cr.Name), }) - - existingSecret := &corev1.Secret{} - err := a.Client.Get(context.TODO(), types.NamespacedName{Namespace: cr.Spec.SecretRef.Namespace, Name: cr.Spec.SecretRef.Name}, existingSecret) + sLog.Infof("processing secret") + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: cr.Spec.SecretRef.Name, + Namespace: cr.Spec.SecretRef.Namespace, + }, + } + op, err := controllerutil.CreateOrPatch(ctx, a.Client, secret, func() error { + if secret.Labels == nil { + secret.Labels = map[string]string{} + } + secret.Labels[minterv1.LabelCredentialsRequest] = minterv1.LabelCredentialsRequestValue + if secret.Annotations == nil { + secret.Annotations = map[string]string{} + } + secret.Annotations[minterv1.AnnotationCredentialsRequest] = fmt.Sprintf("%s/%s", cr.Namespace, cr.Name) + if secret.Data == nil { + secret.Data = map[string][]byte{} + } + secret.Data[gcpSecretJSONKey] = privateKeyData + return nil + }) + sLog.WithField("operation", op).Info("processed secret") if err != nil { - if errors.IsNotFound(err) { - logger.Info("no existing secret found, will create one") - - secret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: cr.Spec.SecretRef.Name, - Namespace: cr.Spec.SecretRef.Namespace, - Annotations: map[string]string{ - minterv1.AnnotationCredentialsRequest: fmt.Sprintf("%s/%s", cr.Namespace, cr.Name), - }, - }, - Data: map[string][]byte{ - gcpSecretJSONKey: privateKeyData, - }, - } - - err := a.Client.Create(context.TODO(), secret) - if err != nil { - sLog.WithError(err).Error("error creating secret") - return err - } - sLog.Info("secret created successfully") - return nil - - } else { - return fmt.Errorf("error checking for existing secret: %v", err) - } - } - - sLog.Info("updating existing secret") - - origSecret := existingSecret.DeepCopy() - if existingSecret.Annotations == nil { - existingSecret.Annotations = map[string]string{} - } - existingSecret.Annotations[minterv1.AnnotationCredentialsRequest] = fmt.Sprintf("%s/%s", cr.Namespace, cr.Name) - existingSecret.Data[gcpSecretJSONKey] = privateKeyData - - if !reflect.DeepEqual(existingSecret, origSecret) { - sLog.Info("secret changed, updating") - err := a.Client.Update(context.TODO(), existingSecret) - if err != nil { - return fmt.Errorf("error updating existing secret: %v", err) + return &actuatoriface.ActuatorError{ + ErrReason: minterv1.CredentialsProvisionFailure, + Message: "error processing secret", } - } else { - sLog.Debug("target secret unchanged") } - return nil } @@ -891,5 +871,5 @@ func checkServicesEnabled(gcpClient ccgcp.Client, permList []string, logger log. // if the system is considered not upgradeable. Otherwise, return nil as the default // value is for things to be upgradeable. func (a *Actuator) Upgradeable(mode operatorv1.CloudCredentialsMode) *configv1.ClusterOperatorStatusCondition { - return utils.UpgradeableCheck(a.Client, mode, a.GetCredentialsRootSecretLocation()) + return utils.UpgradeableCheck(a.RootCredClient, mode, a.GetCredentialsRootSecretLocation()) } diff --git a/pkg/kubevirt/actuator.go b/pkg/kubevirt/actuator.go index eedb94804c..c7e29df342 100644 --- a/pkg/kubevirt/actuator.go +++ b/pkg/kubevirt/actuator.go @@ -18,9 +18,9 @@ package kubevirt import ( "context" "fmt" - "reflect" log "github.com/sirupsen/logrus" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -37,7 +37,8 @@ import ( ) type KubevirtActuator struct { - Client client.Client + Client client.Client + RootCredClient client.Client } func (a *KubevirtActuator) STSFeatureGateEnabled() bool { @@ -49,9 +50,10 @@ const ( ) // NewActuator creates a new Kubevirt actuator. -func NewActuator(client client.Client) (*KubevirtActuator, error) { +func NewActuator(client, rootCredClient client.Client) (*KubevirtActuator, error) { return &KubevirtActuator{ - Client: client, + Client: client, + RootCredClient: rootCredClient, }, nil } @@ -112,7 +114,7 @@ func (a *KubevirtActuator) GetCredentialsRootSecret(ctx context.Context, cr *min // get the secret of the kubevirt credentials kubevirtCredentialsSecret := &corev1.Secret{} - if err := a.Client.Get(ctx, a.GetCredentialsRootSecretLocation(), kubevirtCredentialsSecret); err != nil { + if err := a.RootCredClient.Get(ctx, a.GetCredentialsRootSecretLocation(), kubevirtCredentialsSecret); err != nil { msg := "unable to fetch root cloud cred secret" logger.WithError(err).Error(msg) return nil, &actuatoriface.ActuatorError{ @@ -142,13 +144,8 @@ func (a *KubevirtActuator) sync(ctx context.Context, cr *minterv1.CredentialsReq // get the existing secret in order to check if need to update or create a new logger.Debug("provisioning secret") - existingSecret, err := a.getSecret(ctx, cr, logger) - if err != nil { - return err - } - // check if need to update or create a new one - if err = a.syncCredentialSecret(ctx, cr, &kubevirtCredentialData, existingSecret, logger); err != nil { + if err = a.syncCredentialSecret(ctx, cr, &kubevirtCredentialData, logger); err != nil { msg := "error creating/updating secret" logger.WithError(err).Error(msg) return &actuatoriface.ActuatorError{ @@ -170,79 +167,44 @@ func (a *KubevirtActuator) getCredentialsSecretData(cloudCredSecret *corev1.Secr return infraClusterKubeconfig, nil } -func (a *KubevirtActuator) syncCredentialSecret(ctx context.Context, cr *minterv1.CredentialsRequest, kubevirtCredentialData *[]byte, existingSecret *corev1.Secret, logger log.FieldLogger) error { - if existingSecret == nil { - if kubevirtCredentialData == nil { - msg := "new access key secret needed but no key data provided" - logger.Error(msg) - return &actuatoriface.ActuatorError{ - ErrReason: minterv1.CredentialsProvisionFailure, - Message: msg, - } - } - - return a.createNewSecret(logger, cr, kubevirtCredentialData, ctx) - } - - return a.updateExistingSecret(logger, existingSecret, cr, kubevirtCredentialData) -} - -func (a *KubevirtActuator) updateExistingSecret(logger log.FieldLogger, existingSecret *corev1.Secret, cr *minterv1.CredentialsRequest, kubevirtCredentialData *[]byte) error { - // Update the existing secret: - logger.Debug("updating secret") - origSecret := existingSecret.DeepCopy() - if existingSecret.Annotations == nil { - existingSecret.Annotations = map[string]string{} - } - existingSecret.Annotations[minterv1.AnnotationCredentialsRequest] = fmt.Sprintf("%s/%s", cr.Namespace, cr.Name) - if kubevirtCredentialData != nil { - existingSecret.Data = map[string][]byte{ - KubevirtCredentialsSecretKey: *kubevirtCredentialData, - } - } - - if !reflect.DeepEqual(existingSecret, origSecret) { - logger.Info("target secret has changed, updating") - if err := a.Client.Update(context.TODO(), existingSecret); err != nil { - msg := "error updating secret" - logger.WithError(err).Error(msg) - return &actuatoriface.ActuatorError{ - ErrReason: minterv1.CredentialsProvisionFailure, - Message: msg, - } - } - } else { - logger.Debug("target secret unchanged") - } - - return nil -} - -func (a *KubevirtActuator) createNewSecret(logger log.FieldLogger, cr *minterv1.CredentialsRequest, kubevirtCredentialData *[]byte, ctx context.Context) error { - logger.Info("creating secret") +func (a *KubevirtActuator) syncCredentialSecret(ctx context.Context, cr *minterv1.CredentialsRequest, kubevirtCredentialData *[]byte, logger log.FieldLogger) error { + sLog := logger.WithFields(log.Fields{ + "targetSecret": fmt.Sprintf("%s/%s", cr.Spec.SecretRef.Namespace, cr.Spec.SecretRef.Name), + "cr": fmt.Sprintf("%s/%s", cr.Namespace, cr.Name), + }) + sLog.Infof("processing secret") secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: cr.Spec.SecretRef.Name, Namespace: cr.Spec.SecretRef.Namespace, - Annotations: map[string]string{ - minterv1.AnnotationCredentialsRequest: fmt.Sprintf("%s/%s", cr.Namespace, cr.Name), - }, - }, - Data: map[string][]byte{ - KubevirtCredentialsSecretKey: *kubevirtCredentialData, }, } - - if err := a.Client.Create(ctx, secret); err != nil { - msg := "error in creating a secret" - logger.WithError(err).Error(msg) + op, err := controllerutil.CreateOrPatch(ctx, a.Client, secret, func() error { + if secret.Labels == nil { + secret.Labels = map[string]string{} + } + secret.Labels[minterv1.LabelCredentialsRequest] = minterv1.LabelCredentialsRequestValue + if secret.Annotations == nil { + secret.Annotations = map[string]string{} + } + secret.Annotations[minterv1.AnnotationCredentialsRequest] = fmt.Sprintf("%s/%s", cr.Namespace, cr.Name) + if kubevirtCredentialData != nil { + if secret.Data == nil { + secret.Data = map[string][]byte{} + } + secret.Data = map[string][]byte{ + KubevirtCredentialsSecretKey: *kubevirtCredentialData, + } + } + return nil + }) + sLog.WithField("operation", op).Info("processed secret") + if err != nil { return &actuatoriface.ActuatorError{ ErrReason: minterv1.CredentialsProvisionFailure, - Message: msg, + Message: "error processing secret", } } - - logger.Info("secret created successfully") return nil } @@ -275,5 +237,5 @@ func (a *KubevirtActuator) getLogger(cr *minterv1.CredentialsRequest) log.FieldL } func (a *KubevirtActuator) Upgradeable(mode operatorv1.CloudCredentialsMode) *configv1.ClusterOperatorStatusCondition { - return utils.UpgradeableCheck(a.Client, mode, a.GetCredentialsRootSecretLocation()) + return utils.UpgradeableCheck(a.RootCredClient, mode, a.GetCredentialsRootSecretLocation()) } diff --git a/pkg/kubevirt/actuator_test.go b/pkg/kubevirt/actuator_test.go index 24384e6706..18546ebc3f 100644 --- a/pkg/kubevirt/actuator_test.go +++ b/pkg/kubevirt/actuator_test.go @@ -19,10 +19,11 @@ package kubevirt_test import ( "context" "fmt" - "k8s.io/apimachinery/pkg/api/errors" "reflect" "testing" + "k8s.io/apimachinery/pkg/api/errors" + minterv1 "github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential/v1" "github.com/openshift/cloud-credential-operator/pkg/kubevirt" "github.com/openshift/cloud-credential-operator/pkg/operator/constants" @@ -97,6 +98,7 @@ func TestCreateCR(t *testing.T) { tests := []struct { name string existing []runtime.Object + existingAdmin []runtime.Object credentialsRequest *minterv1.CredentialsRequest expectedErr error errRegexp string @@ -105,6 +107,7 @@ func TestCreateCR(t *testing.T) { { name: "Create CR happy flow", existing: defaultExistingObjects(), + existingAdmin: []runtime.Object{&kubevirtCredentialsSecret}, credentialsRequest: testCredentialsRequest(t), validate: func(t *testing.T, c kubernetesclient.Client) { cr := getCredRequest(t, c) @@ -126,8 +129,9 @@ func TestCreateCR(t *testing.T) { t.Run(test.name, func(t *testing.T) { allObjects := append(test.existing, test.credentialsRequest) fakeClient := fake.NewFakeClientWithScheme(scheme.Scheme, allObjects...) + fakeAdminClient := fake.NewFakeClientWithScheme(scheme.Scheme, test.existingAdmin...) - actuator, err := kubevirt.NewActuator(fakeClient) + actuator, err := kubevirt.NewActuator(fakeClient, fakeAdminClient) if err != nil { assert.Regexp(t, test.errRegexp, err) assert.Nil(t, actuator) @@ -151,6 +155,7 @@ func TestDeleteCR(t *testing.T) { tests := []struct { name string existing []runtime.Object + existingAdmin []runtime.Object credentialsRequest *minterv1.CredentialsRequest expectedErr error errRegexp string @@ -159,12 +164,14 @@ func TestDeleteCR(t *testing.T) { { name: "Delete CR happy flow", existing: existingObjectsAfterCreate(t), + existingAdmin: []runtime.Object{&kubevirtCredentialsSecret}, credentialsRequest: testCredentialsRequest(t), validate: func(t *testing.T, c kubernetesclient.Client) {}, }, { name: "Delete CR happy flow - existingSecret not exist", existing: defaultExistingObjects(), + existingAdmin: []runtime.Object{&kubevirtCredentialsSecret}, credentialsRequest: testCredentialsRequest(t), validate: func(t *testing.T, c kubernetesclient.Client) {}, }, @@ -173,8 +180,9 @@ func TestDeleteCR(t *testing.T) { t.Run(test.name, func(t *testing.T) { allObjects := append(test.existing, test.credentialsRequest) fakeClient := fake.NewFakeClientWithScheme(scheme.Scheme, allObjects...) + fakeAdminClient := fake.NewFakeClientWithScheme(scheme.Scheme, test.existingAdmin...) - actuator, err := kubevirt.NewActuator(fakeClient) + actuator, err := kubevirt.NewActuator(fakeClient, fakeAdminClient) if err != nil { assert.Regexp(t, test.errRegexp, err) assert.Nil(t, actuator) @@ -198,6 +206,7 @@ func TestExistsCR(t *testing.T) { tests := []struct { name string existing []runtime.Object + existingAdmin []runtime.Object credentialsRequest *minterv1.CredentialsRequest expectedErr error errRegexp string @@ -206,6 +215,7 @@ func TestExistsCR(t *testing.T) { { name: "Exists CR happy flow (true)", existing: existingObjectsAfterCreate(t), + existingAdmin: []runtime.Object{&kubevirtCredentialsSecret}, credentialsRequest: testCredentialsRequest(t), validate: func(t *testing.T, c kubernetesclient.Client, isExists bool) { secret := getSecret(t, c) @@ -216,6 +226,7 @@ func TestExistsCR(t *testing.T) { { name: "Non Exists CR happy flow (false)", existing: defaultExistingObjects(), + existingAdmin: []runtime.Object{&kubevirtCredentialsSecret}, credentialsRequest: testCredentialsRequest(t), validate: func(t *testing.T, c kubernetesclient.Client, isExists bool) { secret := getSecret(t, c) @@ -228,8 +239,9 @@ func TestExistsCR(t *testing.T) { t.Run(test.name, func(t *testing.T) { allObjects := append(test.existing, test.credentialsRequest) fakeClient := fake.NewFakeClientWithScheme(scheme.Scheme, allObjects...) + fakeAdminClient := fake.NewFakeClientWithScheme(scheme.Scheme, test.existingAdmin...) - actuator, err := kubevirt.NewActuator(fakeClient) + actuator, err := kubevirt.NewActuator(fakeClient, fakeAdminClient) if err != nil { assert.Regexp(t, test.errRegexp, err) assert.Nil(t, actuator) @@ -253,6 +265,7 @@ func TestUpdateCR(t *testing.T) { tests := []struct { name string existing []runtime.Object + existingAdmin []runtime.Object credentialsRequest *minterv1.CredentialsRequest expectedErr error errRegexp string @@ -261,6 +274,7 @@ func TestUpdateCR(t *testing.T) { { name: "Update CR happy flow - non exists", existing: defaultExistingObjects(), + existingAdmin: []runtime.Object{&kubevirtCredentialsSecret}, credentialsRequest: testCredentialsRequest(t), validate: func(t *testing.T, c kubernetesclient.Client) { cr := getCredRequest(t, c) @@ -270,6 +284,7 @@ func TestUpdateCR(t *testing.T) { { name: "Update CR happy flow - exists", existing: existingObjectsAfterCreate(t), + existingAdmin: []runtime.Object{&kubevirtCredentialsSecret}, credentialsRequest: testCredentialsRequest(t), validate: func(t *testing.T, c kubernetesclient.Client) { cr := getCredRequest(t, c) @@ -279,6 +294,7 @@ func TestUpdateCR(t *testing.T) { { name: "Update CR fail on getCredentialSecret kube-system:kubevirt-credentials", existing: []runtime.Object{}, + existingAdmin: []runtime.Object{}, credentialsRequest: testCredentialsRequest(t), expectedErr: fmt.Errorf("unable to fetch root cloud cred secret: secrets \"kubevirt-credentials\" not found"), }, @@ -287,8 +303,9 @@ func TestUpdateCR(t *testing.T) { t.Run(test.name, func(t *testing.T) { allObjects := append(test.existing, test.credentialsRequest) fakeClient := fake.NewFakeClientWithScheme(scheme.Scheme, allObjects...) + fakeAdminClient := fake.NewFakeClientWithScheme(scheme.Scheme, test.existingAdmin...) - actuator, err := kubevirt.NewActuator(fakeClient) + actuator, err := kubevirt.NewActuator(fakeClient, fakeAdminClient) if err != nil { assert.Regexp(t, test.errRegexp, err) assert.Nil(t, actuator) @@ -329,15 +346,12 @@ func getCredRequest(t *testing.T, c kubernetesclient.Client) *minterv1.Credentia } func defaultExistingObjects() []runtime.Object { - objs := []runtime.Object{ - &kubevirtCredentialsSecret, - } + objs := []runtime.Object{} return objs } func existingObjectsAfterCreate(t *testing.T) []runtime.Object { objs := []runtime.Object{ - &kubevirtCredentialsSecret, &kubevirtOpenshiftMachineApiSecret, } return objs diff --git a/pkg/openstack/actuator.go b/pkg/openstack/actuator.go index ba98fb1fcc..53909f3663 100644 --- a/pkg/openstack/actuator.go +++ b/pkg/openstack/actuator.go @@ -18,12 +18,12 @@ package openstack import ( "context" "fmt" - "reflect" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/client" @@ -38,8 +38,9 @@ import ( ) type OpenStackActuator struct { - Client client.Client - Codec *minterv1.ProviderCodec + Client client.Client + RootCredClient client.Client + Codec *minterv1.ProviderCodec } func (a *OpenStackActuator) STSFeatureGateEnabled() bool { @@ -47,7 +48,7 @@ func (a *OpenStackActuator) STSFeatureGateEnabled() bool { } // NewOpenStackActuator creates a new OpenStack actuator. -func NewOpenStackActuator(client client.Client) (*OpenStackActuator, error) { +func NewOpenStackActuator(client, rootCredClient client.Client) (*OpenStackActuator, error) { codec, err := minterv1.NewCodec() if err != nil { log.WithError(err).Error("error creating OpenStack codec") @@ -55,8 +56,9 @@ func NewOpenStackActuator(client client.Client) (*OpenStackActuator, error) { } return &OpenStackActuator{ - Codec: codec, - Client: client, + Codec: codec, + Client: client, + RootCredClient: rootCredClient, }, nil } @@ -67,7 +69,7 @@ func (a *OpenStackActuator) Exists(ctx context.Context, cr *minterv1.Credentials var err error existingSecret := &corev1.Secret{} - err = a.Client.Get(context.TODO(), types.NamespacedName{Namespace: cr.Spec.SecretRef.Namespace, Name: cr.Spec.SecretRef.Name}, existingSecret) + err = a.Client.Get(ctx, types.NamespacedName{Namespace: cr.Spec.SecretRef.Namespace, Name: cr.Spec.SecretRef.Name}, existingSecret) if err != nil { if errors.IsNotFound(err) { logger.Debug("target secret does not exist") @@ -114,12 +116,7 @@ func (a *OpenStackActuator) sync(ctx context.Context, cr *minterv1.CredentialsRe } logger.Debugf("provisioning secret") - existingSecret, err := a.loadExistingSecret(cr) - if err != nil { - return err - } - - err = a.syncCredentialSecret(cr, clouds, existingSecret, "", logger) + err = a.syncCredentialSecret(ctx, cr, clouds, logger) if err != nil { msg := "error creating/updating secret" logger.WithError(err).Error(msg) @@ -132,92 +129,43 @@ func (a *OpenStackActuator) sync(ctx context.Context, cr *minterv1.CredentialsRe return nil } -func (a *OpenStackActuator) syncCredentialSecret(cr *minterv1.CredentialsRequest, clouds string, existingSecret *corev1.Secret, userPolicy string, logger log.FieldLogger) error { +func (a *OpenStackActuator) syncCredentialSecret(ctx context.Context, cr *minterv1.CredentialsRequest, clouds string, logger log.FieldLogger) error { sLog := logger.WithFields(log.Fields{ "targetSecret": fmt.Sprintf("%s/%s", cr.Spec.SecretRef.Namespace, cr.Spec.SecretRef.Name), "cr": fmt.Sprintf("%s/%s", cr.Namespace, cr.Name), }) - - if existingSecret == nil || existingSecret.Name == "" { - if clouds == "" { - msg := "new access key secret needed but no key data provided" - sLog.Error(msg) - return &actuatoriface.ActuatorError{ - ErrReason: minterv1.CredentialsProvisionFailure, - Message: msg, - } - } - sLog.Info("creating secret") - secret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: cr.Spec.SecretRef.Name, - Namespace: cr.Spec.SecretRef.Namespace, - Annotations: map[string]string{ - minterv1.AnnotationCredentialsRequest: fmt.Sprintf("%s/%s", cr.Namespace, cr.Name), - }, - }, - Data: map[string][]byte{RootOpenStackCredsSecretKey: []byte(clouds)}, + sLog.Infof("processing secret") + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: cr.Spec.SecretRef.Name, + Namespace: cr.Spec.SecretRef.Namespace, + }, + } + op, err := controllerutil.CreateOrPatch(ctx, a.Client, secret, func() error { + if secret.Labels == nil { + secret.Labels = map[string]string{} } - - err := a.Client.Create(context.TODO(), secret) - if err != nil { - sLog.WithError(err).Error("error creating secret") - return err + secret.Labels[minterv1.LabelCredentialsRequest] = minterv1.LabelCredentialsRequestValue + if secret.Annotations == nil { + secret.Annotations = map[string]string{} } - sLog.Info("secret created successfully") - return nil - } - - // Update the existing secret: - sLog.Debug("updating secret") - origSecret := existingSecret.DeepCopy() - if existingSecret.Annotations == nil { - existingSecret.Annotations = map[string]string{} - } - existingSecret.Annotations[minterv1.AnnotationCredentialsRequest] = fmt.Sprintf("%s/%s", cr.Namespace, cr.Name) - if clouds != "" { - existingSecret.Data[RootOpenStackCredsSecretKey] = []byte(clouds) - } - - if !reflect.DeepEqual(existingSecret, origSecret) { - sLog.Info("target secret has changed, updating") - err := a.Client.Update(context.TODO(), existingSecret) - if err != nil { - msg := "error updating secret" - sLog.WithError(err).Error(msg) - return &actuatoriface.ActuatorError{ - ErrReason: minterv1.CredentialsProvisionFailure, - Message: msg, + secret.Annotations[minterv1.AnnotationCredentialsRequest] = fmt.Sprintf("%s/%s", cr.Namespace, cr.Name) + if clouds != "" { + if secret.Data == nil { + secret.Data = map[string][]byte{} } + secret.Data[RootOpenStackCredsSecretKey] = []byte(clouds) } - } else { - sLog.Debug("target secret unchanged") - } - - return nil -} - -func (a *OpenStackActuator) loadExistingSecret(cr *minterv1.CredentialsRequest) (*corev1.Secret, error) { - logger := a.getLogger(cr) - - // Check if the credentials secret exists, if not we need to inform the syncer to generate a new one: - existingSecret := &corev1.Secret{} - err := a.Client.Get(context.TODO(), types.NamespacedName{Namespace: cr.Spec.SecretRef.Namespace, Name: cr.Spec.SecretRef.Name}, existingSecret) + return nil + }) + sLog.WithField("operation", op).Info("processed secret") if err != nil { - if errors.IsNotFound(err) { - logger.Debug("secret does not exist") - } else { - return nil, err + return &actuatoriface.ActuatorError{ + ErrReason: minterv1.CredentialsProvisionFailure, + Message: "error processing secret", } } - - if _, ok := existingSecret.Data[RootOpenStackCredsSecretKey]; !ok { - logger.Warning("secret did not have expected key: " + RootOpenStackCredsSecretKey) - } else { - logger.Debug("found clouds.yaml in existing secret") - } - - return existingSecret, nil + return nil } // GetCredentialsRootSecretLocation returns the namespace and name where the parent credentials secret is stored. @@ -228,7 +176,7 @@ func (a *OpenStackActuator) GetCredentialsRootSecretLocation() types.NamespacedN func (a *OpenStackActuator) GetCredentialsRootSecret(ctx context.Context, cr *minterv1.CredentialsRequest) (*corev1.Secret, error) { logger := a.getLogger(cr) cloudCredSecret := &corev1.Secret{} - if err := a.Client.Get(ctx, a.GetCredentialsRootSecretLocation(), cloudCredSecret); err != nil { + if err := a.RootCredClient.Get(ctx, a.GetCredentialsRootSecretLocation(), cloudCredSecret); err != nil { msg := "unable to fetch root cloud cred secret" logger.WithError(err).Error(msg) return nil, &actuatoriface.ActuatorError{ @@ -273,5 +221,5 @@ func (a *OpenStackActuator) getLogger(cr *minterv1.CredentialsRequest) log.Field // if the system is considered not upgradeable. Otherwise, return nil as the default // value is for things to be upgradeable. func (a *OpenStackActuator) Upgradeable(mode operatorv1.CloudCredentialsMode) *configv1.ClusterOperatorStatusCondition { - return utils.UpgradeableCheck(a.Client, mode, a.GetCredentialsRootSecretLocation()) + return utils.UpgradeableCheck(a.RootCredClient, mode, a.GetCredentialsRootSecretLocation()) } diff --git a/pkg/operator/awspodidentity/awspodidentitywebhook_controller.go b/pkg/operator/awspodidentity/awspodidentitywebhook_controller.go index b45294bd8c..3eead6ce94 100644 --- a/pkg/operator/awspodidentity/awspodidentitywebhook_controller.go +++ b/pkg/operator/awspodidentity/awspodidentitywebhook_controller.go @@ -128,8 +128,8 @@ func (c *awsPodIdentityController) Start(ctx context.Context) error { return nil } -func Add(mgr manager.Manager, kubeconfig string) error { - infraStatus, err := platform.GetInfraStatusUsingKubeconfig(mgr, kubeconfig) +func Add(mgr, rootCredentialManager manager.Manager, kubeconfig string) error { + infraStatus, err := platform.GetInfraStatusUsingKubeconfig(kubeconfig) if err != nil { return err } diff --git a/pkg/operator/cleanup/cleanup_controller.go b/pkg/operator/cleanup/cleanup_controller.go index aa82d07f5c..3961b84878 100644 --- a/pkg/operator/cleanup/cleanup_controller.go +++ b/pkg/operator/cleanup/cleanup_controller.go @@ -44,7 +44,7 @@ func newReconciler(mgr manager.Manager) reconcile.Reconciler { } // Add creates a new Cleanup Controller and adds it to the Manager. -func Add(mgr manager.Manager, kubeConfig string) error { +func Add(mgr, rootCredentialManager manager.Manager, kubeConfig string) error { r := newReconciler(mgr) // Create a new controller diff --git a/pkg/operator/controller.go b/pkg/operator/controller.go index e4b5e049a2..f7c1807556 100644 --- a/pkg/operator/controller.go +++ b/pkg/operator/controller.go @@ -17,6 +17,7 @@ limitations under the License. package controller import ( + configv1 "github.com/openshift/api/config/v1" awsactuator "github.com/openshift/cloud-credential-operator/pkg/aws/actuator" "github.com/openshift/cloud-credential-operator/pkg/azure" gcpactuator "github.com/openshift/cloud-credential-operator/pkg/gcp/actuator" @@ -34,8 +35,7 @@ import ( "github.com/openshift/cloud-credential-operator/pkg/ovirt" "github.com/openshift/cloud-credential-operator/pkg/util" vsphereactuator "github.com/openshift/cloud-credential-operator/pkg/vsphere/actuator" - - configv1 "github.com/openshift/api/config/v1" + corev1client "k8s.io/client-go/kubernetes/typed/core/v1" "sigs.k8s.io/controller-runtime/pkg/manager" @@ -59,15 +59,15 @@ func init() { // AddToManagerFuncs is a list of functions to add all Controllers to the Manager. // String parameter is to pass in any specific override for the kubeconfig file to use. -var AddToManagerFuncs []func(manager.Manager, string) error +var AddToManagerFuncs []func(manager.Manager, manager.Manager, string) error // AddToManagerWithActuatorFuncs is a list of functions to add all Controllers with Actuators to the Manager -var AddToManagerWithActuatorFuncs []func(manager.Manager, actuator.Actuator, configv1.PlatformType) error +var AddToManagerWithActuatorFuncs []func(manager.Manager, manager.Manager, actuator.Actuator, configv1.PlatformType, corev1client.CoreV1Interface) error // AddToManager adds all Controllers to the Manager -func AddToManager(m manager.Manager, explicitKubeconfig string, awsSecurityTokenServiveGateEnaled bool) error { +func AddToManager(m, rootM manager.Manager, explicitKubeconfig string, coreClient corev1client.CoreV1Interface, awsSecurityTokenServiceGateEnabled bool) error { for _, f := range AddToManagerFuncs { - if err := f(m, explicitKubeconfig); err != nil { + if err := f(m, rootM, explicitKubeconfig); err != nil { return err } } @@ -77,7 +77,7 @@ func AddToManager(m manager.Manager, explicitKubeconfig string, awsSecurityToken // https://github.com/openshift/api/blob/master/config/v1/types_infrastructure.go#L11 var err error var a actuator.Actuator - infraStatus, err := platform.GetInfraStatusUsingKubeconfig(m, explicitKubeconfig) + infraStatus, err := platform.GetInfraStatusUsingKubeconfig(explicitKubeconfig) if err != nil { log.Fatal(err) } @@ -85,19 +85,19 @@ func AddToManager(m manager.Manager, explicitKubeconfig string, awsSecurityToken switch platformType { case configv1.AWSPlatformType: log.Info("initializing AWS actuator") - a, err = awsactuator.NewAWSActuator(m.GetClient(), m.GetScheme(), awsSecurityTokenServiveGateEnaled) + a, err = awsactuator.NewAWSActuator(m.GetClient(), rootM.GetClient(), m.GetScheme(), awsSecurityTokenServiceGateEnabled) if err != nil { return err } case configv1.AzurePlatformType: log.Info("initializing Azure actuator") - a, err = azure.NewActuator(m.GetClient(), util.GetAzureCloudName(infraStatus)) + a, err = azure.NewActuator(m.GetClient(), rootM.GetClient(), util.GetAzureCloudName(infraStatus)) if err != nil { return err } case configv1.OpenStackPlatformType: log.Info("initializing OpenStack actuator") - a, err = openstack.NewOpenStackActuator(m.GetClient()) + a, err = openstack.NewOpenStackActuator(m.GetClient(), rootM.GetClient()) if err != nil { return err } @@ -106,7 +106,7 @@ func AddToManager(m manager.Manager, explicitKubeconfig string, awsSecurityToken if infraStatus.PlatformStatus == nil || infraStatus.PlatformStatus.GCP == nil { log.Fatalf("missing GCP configuration in platform status") } - a, err = gcpactuator.NewActuator(m.GetClient(), infraStatus.PlatformStatus.GCP.ProjectID) + a, err = gcpactuator.NewActuator(m.GetClient(), rootM.GetClient(), infraStatus.PlatformStatus.GCP.ProjectID) if err != nil { return err } @@ -115,19 +115,19 @@ func AddToManager(m manager.Manager, explicitKubeconfig string, awsSecurityToken if infraStatus.PlatformStatus == nil || infraStatus.PlatformStatus.Ovirt == nil { log.Fatalf("missing Ovirt configuration in platform status") } - a, err = ovirt.NewActuator(m.GetClient()) + a, err = ovirt.NewActuator(m.GetClient(), rootM.GetClient()) if err != nil { return err } case configv1.VSpherePlatformType: log.Info("initializing VSphere actuator") - a, err = vsphereactuator.NewVSphereActuator(m.GetClient()) + a, err = vsphereactuator.NewVSphereActuator(m.GetClient(), rootM.GetClient()) if err != nil { return err } case configv1.KubevirtPlatformType: log.Info("initializing Kubevirt actuator") - a, err = kubevirt.NewActuator(m.GetClient()) + a, err = kubevirt.NewActuator(m.GetClient(), rootM.GetClient()) if err != nil { return err } @@ -135,7 +135,7 @@ func AddToManager(m manager.Manager, explicitKubeconfig string, awsSecurityToken log.Info("initializing no-op actuator (unsupported platform)") a = &actuator.DummyActuator{} } - if err := f(m, a, platformType); err != nil { + if err := f(m, rootM, a, platformType, coreClient); err != nil { return err } } diff --git a/pkg/operator/credentialsrequest/credentialsrequest_controller.go b/pkg/operator/credentialsrequest/credentialsrequest_controller.go index a4d63c7f67..e54fa55df5 100644 --- a/pkg/operator/credentialsrequest/credentialsrequest_controller.go +++ b/pkg/operator/credentialsrequest/credentialsrequest_controller.go @@ -24,6 +24,8 @@ import ( log "github.com/sirupsen/logrus" "golang.org/x/time/rate" + corev1applyconfigurations "k8s.io/client-go/applyconfigurations/core/v1" + corev1client "k8s.io/client-go/kubernetes/typed/core/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -54,7 +56,8 @@ import ( ) const ( - controllerName = "credreq" + controllerName = "credreq" + labelControllerName = controllerName + "_labeller" namespaceMissing = "NamespaceMissing" namespaceExists = "NamespaceExists" @@ -84,14 +87,18 @@ var ( // 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. -func AddWithActuator(mgr manager.Manager, actuator actuator.Actuator, platType configv1.PlatformType) error { - return add(mgr, newReconciler(mgr, actuator, platType)) +func AddWithActuator(mgr, adminMgr manager.Manager, actuator actuator.Actuator, platType configv1.PlatformType, mutatingClient corev1client.CoreV1Interface) error { + if err := add(mgr, adminMgr, newReconciler(mgr, adminMgr, actuator, platType)); err != nil { + return err + } + return addLabelController(mgr, mutatingClient) } // newReconciler returns a new reconcile.Reconciler -func newReconciler(mgr manager.Manager, actuator actuator.Actuator, platType configv1.PlatformType) reconcile.Reconciler { +func newReconciler(mgr, adminMgr manager.Manager, actuator actuator.Actuator, platType configv1.PlatformType) reconcile.Reconciler { r := &ReconcileCredentialsRequest{ Client: mgr.GetClient(), + AdminClient: adminMgr.GetClient(), Actuator: actuator, platformType: platType, } @@ -101,7 +108,7 @@ func newReconciler(mgr manager.Manager, actuator actuator.Actuator, platType con } // add adds a new Controller to mgr with r as the reconcile.Reconciler -func add(mgr manager.Manager, r reconcile.Reconciler) error { +func add(mgr, adminMgr manager.Manager, r reconcile.Reconciler) error { operatorCache := mgr.GetCache() name := "credentialsrequest_controller" @@ -205,18 +212,18 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error { adminCredSecretPredicate := predicate.Funcs{ UpdateFunc: func(e event.UpdateEvent) bool { - return isAdminCredSecret(e.ObjectNew.GetNamespace(), e.ObjectNew.GetName()) + return IsAdminCredSecret(e.ObjectNew.GetNamespace(), e.ObjectNew.GetName()) }, CreateFunc: func(e event.CreateEvent) bool { - return isAdminCredSecret(e.Object.GetNamespace(), e.Object.GetName()) + return IsAdminCredSecret(e.Object.GetNamespace(), e.Object.GetName()) }, DeleteFunc: func(e event.DeleteEvent) bool { - return isAdminCredSecret(e.Object.GetNamespace(), e.Object.GetName()) + return IsAdminCredSecret(e.Object.GetNamespace(), e.Object.GetName()) }, } // Watch Secrets and reconcile if we see an event for an admin credential secret in kube-system. err = c.Watch( - source.Kind(operatorCache, &corev1.Secret{}), + source.Kind(adminMgr.GetCache(), &corev1.Secret{}), allCredRequestsMapFn, adminCredSecretPredicate) if err != nil { @@ -316,12 +323,148 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error { return nil } +// addLabelController adds a new Controller managing labels to mgr +func addLabelController(mgr manager.Manager, mutatingClient corev1client.CoreV1Interface) error { + labelReconciler := &ReconcileSecretMissingLabel{ + cachedClient: mgr.GetClient(), + mutatingClient: mutatingClient, + } + labelController, err := controller.New(labelControllerName, mgr, controller.Options{ + Reconciler: labelReconciler, + }) + if err != nil { + return err + } + + missingLabelSecretsMapFn := handler.EnqueueRequestsFromMapFunc(func(_ context.Context, a client.Object) []reconcile.Request { + return []reconcile.Request{ + { + NamespacedName: types.NamespacedName{ + Name: a.GetName(), + Namespace: a.GetNamespace(), + }, + }, + } + }) + + missingLabelCredSecretPredicate := predicate.Funcs{ + UpdateFunc: func(e event.UpdateEvent) bool { + return IsMissingSecretLabel(e.ObjectNew) + }, + CreateFunc: func(e event.CreateEvent) bool { + return IsMissingSecretLabel(e.Object) + }, + DeleteFunc: func(e event.DeleteEvent) bool { + return IsMissingSecretLabel(e.Object) + }, + } + err = labelController.Watch( + source.Kind(mgr.GetCache(), &corev1.Secret{}), + missingLabelSecretsMapFn, + missingLabelCredSecretPredicate) + if err != nil { + return err + } + status.AddHandler(labelControllerName, labelReconciler) + + return nil +} + +// IsMissingSecretLabel determines if the secret was created by the CCO but has not been labelled yet +func IsMissingSecretLabel(secret metav1.Object) bool { + _, hasAnnotation := secret.GetAnnotations()[minterv1.AnnotationCredentialsRequest] + value, hasLabel := secret.GetLabels()[minterv1.LabelCredentialsRequest] + hasValue := hasLabel && value == minterv1.LabelCredentialsRequestValue + + return hasAnnotation && (!hasLabel || !hasValue) +} + +type ReconcileSecretMissingLabel struct { + cachedClient client.Client + mutatingClient corev1client.SecretsGetter +} + +func (r *ReconcileSecretMissingLabel) GetConditions(logger log.FieldLogger) ([]configv1.ClusterOperatorStatusCondition, error) { + var secrets corev1.SecretList + if err := r.cachedClient.List(context.TODO(), &secrets); err != nil { + return nil, err + } + var missing int + for _, item := range secrets.Items { + if IsMissingSecretLabel(&item) { + missing += 1 + } + } + + if missing > 0 { + return []configv1.ClusterOperatorStatusCondition{{ + Type: configv1.OperatorProgressing, + Status: configv1.ConditionTrue, + Reason: "LabelsMissing", + Message: fmt.Sprintf("%d secrets created for CredentialsRequests have not been labelled", missing), + }}, nil + } + return []configv1.ClusterOperatorStatusCondition{}, nil +} + +func (r *ReconcileSecretMissingLabel) GetRelatedObjects(logger log.FieldLogger) ([]configv1.ObjectReference, error) { + return nil, nil +} + +func (r *ReconcileSecretMissingLabel) Name() string { + return labelControllerName +} + +var _ reconcile.Reconciler = (*ReconcileSecretMissingLabel)(nil) +var _ status.Handler = (*ReconcileSecretMissingLabel)(nil) + +func (r *ReconcileSecretMissingLabel) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) { + start := time.Now() + + logger := log.WithFields(log.Fields{ + "controller": labelControllerName, + "secret": fmt.Sprintf("%s/%s", request.NamespacedName.Namespace, request.NamespacedName.Name), + }) + + defer func() { + dur := time.Since(start) + metrics.MetricControllerReconcileTime.WithLabelValues(labelControllerName).Observe(dur.Seconds()) + }() + + logger.Info("syncing secret") + secret := &corev1.Secret{} + err := r.cachedClient.Get(ctx, request.NamespacedName, secret) + if err != nil { + if errors.IsNotFound(err) { + logger.Debug("secret no longer exists") + return reconcile.Result{}, nil + } + logger.WithError(err).Error("error getting secret, re-queuing") + return reconcile.Result{}, err + } + + applyConfig := corev1applyconfigurations.Secret(secret.Name, secret.Namespace) + applyConfig.WithLabels(map[string]string{ + minterv1.LabelCredentialsRequest: minterv1.LabelCredentialsRequestValue, + }) + + if _, err := r.mutatingClient.Secrets(secret.Namespace).Apply(ctx, applyConfig, metav1.ApplyOptions{ + Force: true, // we're the authoritative owner of this field and should not allow anyone to stomp it + FieldManager: labelControllerName, + }); err != nil { + logger.WithError(err).Error("failed to update label") + return reconcile.Result{}, err + } + + return reconcile.Result{}, nil +} + // isCloudCredOperatorConfigMap returns true if given configmap is cloud-credential-operator-config configmap func isCloudCredOperatorConfigMap(cm metav1.Object) bool { return cm.GetName() == constants.CloudCredOperatorConfigMap && cm.GetNamespace() == minterv1.CloudCredOperatorNamespace } -func isAdminCredSecret(namespace, secretName string) bool { +func IsAdminCredSecret(namespace, secretName string) bool { if namespace == constants.CloudCredSecretNamespace { if secretName == constants.AWSCloudCredSecretName || secretName == constants.AzureCloudCredSecretName || @@ -342,6 +485,7 @@ var _ reconcile.Reconciler = &ReconcileCredentialsRequest{} // ReconcileCredentialsRequest reconciles a CredentialsRequest object type ReconcileCredentialsRequest struct { client.Client + AdminClient client.Client Actuator actuator.Actuator platformType configv1.PlatformType } @@ -381,7 +525,7 @@ func (r *ReconcileCredentialsRequest) Reconcile(ctx context.Context, request rec logger.WithError(err).Error("error checking if operator is disabled") return reconcile.Result{}, err } else if conflict { - logger.Error("configuration conflict betwen legacy configmap and operator config") + logger.Error("configuration conflict between legacy configmap and operator config") return reconcile.Result{}, fmt.Errorf("configuration conflict") } else if mode == operatorv1.CloudCredentialsModeManual { if !stsDetected { diff --git a/pkg/operator/credentialsrequest/credentialsrequest_controller_azure_test.go b/pkg/operator/credentialsrequest/credentialsrequest_controller_azure_test.go index 4c77b25d0c..567dfafb45 100644 --- a/pkg/operator/credentialsrequest/credentialsrequest_controller_azure_test.go +++ b/pkg/operator/credentialsrequest/credentialsrequest_controller_azure_test.go @@ -102,7 +102,6 @@ func TestCredentialsRequestAzureReconcile(t *testing.T) { testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), - testAzureCredsSecret(constants.CloudCredSecretNamespace, constants.AzureCloudCredSecretName), testAzureCredentialsRequest(t), }, mockAzureAppClient: func(mockCtrl *gomock.Controller) *mockazure.MockAppClient { @@ -127,7 +126,6 @@ func TestCredentialsRequestAzureReconcile(t *testing.T) { testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), - testAzureCredsSecret(constants.CloudCredSecretNamespace, constants.AzureCloudCredSecretName), testAzureCredentialsRequestNeedingCleanup(t), testAzureTargetSecret(testSecretNamespace, testSecretName, "mintedAzureClientID"), }, @@ -165,7 +163,6 @@ func TestCredentialsRequestAzureReconcile(t *testing.T) { testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), - testAzureCredsSecret(constants.CloudCredSecretNamespace, constants.AzureCloudCredSecretName), testAzureCredentialsRequestWithOrphanedCloudResource(t), testAzureTargetSecret(testSecretNamespace, testSecretName, testAzureClientID), }, @@ -212,9 +209,12 @@ func TestCredentialsRequestAzureReconcile(t *testing.T) { fakeClient := fake.NewClientBuilder(). WithStatusSubresource(&minterv1.CredentialsRequest{}). WithRuntimeObjects(test.existing...).Build() + fakeAdminClient := fake.NewClientBuilder(). + WithRuntimeObjects(testAzureCredsSecret(constants.CloudCredSecretNamespace, constants.AzureCloudCredSecretName)).Build() azureActuator := azureactuator.NewFakeActuator( fakeClient, + fakeAdminClient, codec, func(logger log.FieldLogger, clientID, clientSecret, tenantID, subscriptionID string) (*azureactuator.AzureCredentialsMinter, error) { return azureactuator.NewFakeAzureCredentialsMinter(logger, diff --git a/pkg/operator/credentialsrequest/credentialsrequest_controller_gcp_test.go b/pkg/operator/credentialsrequest/credentialsrequest_controller_gcp_test.go index fa71e29035..bbe9b328ff 100644 --- a/pkg/operator/credentialsrequest/credentialsrequest_controller_gcp_test.go +++ b/pkg/operator/credentialsrequest/credentialsrequest_controller_gcp_test.go @@ -27,7 +27,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - cloudresourcemanager "google.golang.org/api/cloudresourcemanager/v1" + "google.golang.org/api/cloudresourcemanager/v1" iamadminpb "google.golang.org/genproto/googleapis/iam/admin/v1" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -102,6 +102,7 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { tests := []struct { name string existing []runtime.Object + existingAdmin []runtime.Object expectErr bool mockRootGCPClient func(mockCtrl *gomock.Controller) *mockgcp.MockClient mockReadGCPClient func(mockCtrl *gomock.Controller) *mockgcp.MockClient @@ -117,11 +118,13 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), - testGCPCredsSecret("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), testGCPCredentialsRequest(t), testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{ + testGCPCredsSecret("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), + }, mockRootGCPClient: func(mockCtrl *gomock.Controller) *mockgcp.MockClient { mockGCPClient := mockgcp.NewMockClient(mockCtrl) mockGetProjectName(mockGCPClient) @@ -158,11 +161,13 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), - testGCPCredsSecret("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), testGCPCredentialsRequestWithPermissions(t), testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{ + testGCPCredsSecret("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), + }, mockRootGCPClient: func(mockCtrl *gomock.Controller) *mockgcp.MockClient { mockGCPClient := mockgcp.NewMockClient(mockCtrl) mockGetProjectName(mockGCPClient) @@ -204,10 +209,12 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testGCPCredentialsRequest(t), - testGCPCredsSecret("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), testClusterVersion(), testInfrastructure(""), }, + existingAdmin: []runtime.Object{ + testGCPCredsSecret("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), + }, mockRootGCPClient: func(mockCtrl *gomock.Controller) *mockgcp.MockClient { mockGCPClient := mockgcp.NewMockClient(mockCtrl) mockGetProjectName(mockGCPClient) @@ -248,6 +255,7 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{}, mockRootGCPClient: func(mockCtrl *gomock.Controller) *mockgcp.MockClient { mockGCPClient := mockgcp.NewMockClient(mockCtrl) return mockGCPClient @@ -279,6 +287,7 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { // only the read-only creds exist testGCPCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-gcp-ro-creds", testReadOnlyGCPAuth), }, + existingAdmin: []runtime.Object{}, mockReadGCPClient: func(mockCtrl *gomock.Controller) *mockgcp.MockClient { mockGCPClient := mockgcp.NewMockClient(mockCtrl) @@ -318,10 +327,12 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testGCPCredentialsRequest(t), - testGCPCredsSecret("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{ + testGCPCredsSecret("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), + }, mockRootGCPClient: func(mockCtrl *gomock.Controller) *mockgcp.MockClient { mockGCPClient := mockgcp.NewMockClient(mockCtrl) mockGetProjectName(mockGCPClient) @@ -358,11 +369,13 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testGCPCredentialsRequest(t), - testGCPCredsSecret("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), testGCPCredsSecret(testSecretNamespace, testSecretName, testServiceAccountKeyPrivateData), testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{ + testGCPCredsSecret("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), + }, mockRootGCPClient: func(mockCtrl *gomock.Controller) *mockgcp.MockClient { mockGCPClient := mockgcp.NewMockClient(mockCtrl) mockGetProjectName(mockGCPClient) @@ -405,6 +418,9 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{ + testGCPCredsSecret("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), + }, mockRootGCPClient: func(mockCtrl *gomock.Controller) *mockgcp.MockClient { mockGCPClient := mockgcp.NewMockClient(mockCtrl) mockGetProjectName(mockGCPClient) @@ -470,6 +486,7 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { // only the read-only creds exist testGCPCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-gcp-ro-creds", testReadOnlyGCPAuth), }, + existingAdmin: []runtime.Object{}, mockReadGCPClient: func(mockCtrl *gomock.Controller) *mockgcp.MockClient { mockGCPClient := mockgcp.NewMockClient(mockCtrl) @@ -514,6 +531,7 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { // only the read-only creds exist testGCPCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-gcp-ro-creds", testReadOnlyGCPAuth), }, + existingAdmin: []runtime.Object{}, mockReadGCPClient: func(mockCtrl *gomock.Controller) *mockgcp.MockClient { mockGCPClient := mockgcp.NewMockClient(mockCtrl) @@ -586,7 +604,8 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { // target secret exists createTestNamespace(testSecretNamespace), testGCPCredsSecret(testSecretNamespace, testSecretName, `{"private_key_id": "testGCPKeyName"}`), - + }, + existingAdmin: []runtime.Object{ testGCPCredsSecret("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), }, mockRootGCPClient: func(mockCtrl *gomock.Controller) *mockgcp.MockClient { @@ -629,11 +648,13 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testGCPCredentialsRequestWithDeletionTimestamp(t), - testGCPCredsSecret("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), testGCPCredsSecret(testSecretNamespace, testSecretName, testServiceAccountKeyPrivateData), testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{ + testGCPCredsSecret("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), + }, mockRootGCPClient: func(mockCtrl *gomock.Controller) *mockgcp.MockClient { mockGCPClient := mockgcp.NewMockClient(mockCtrl) mockGetProjectName(mockGCPClient) @@ -656,11 +677,13 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testGCPCredentialsRequestWithPermissionsWithDeletionTimestamp(t), - testGCPCredsSecret("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), testGCPCredsSecret(testSecretNamespace, testSecretName, testServiceAccountKeyPrivateData), testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{ + testGCPCredsSecret("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), + }, mockRootGCPClient: func(mockCtrl *gomock.Controller) *mockgcp.MockClient { mockGCPClient := mockgcp.NewMockClient(mockCtrl) mockGetProjectName(mockGCPClient) @@ -683,10 +706,12 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { testOperatorConfig(""), createTestNamespace(testSecretNamespace), testGCPCredentialsRequest(t), - testGCPCredsSecret("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{ + testGCPCredsSecret("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), + }, mockRootGCPClient: func(mockCtrl *gomock.Controller) *mockgcp.MockClient { mockGCPClient := mockgcp.NewMockClient(mockCtrl) mockGetProjectName(mockGCPClient) @@ -717,10 +742,12 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { testOperatorConfig(""), createTestNamespace(testSecretNamespace), testGCPCredentialsRequestWithDeletionTimestamp(t), - testGCPCredsSecret("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{ + testGCPCredsSecret("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), + }, mockRootGCPClient: func(mockCtrl *gomock.Controller) *mockgcp.MockClient { mockGCPClient := mockgcp.NewMockClient(mockCtrl) mockGetProjectName(mockGCPClient) @@ -743,10 +770,12 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { testOperatorConfig(""), createTestNamespace(testSecretNamespace), testGCPCredentialsRequest(t), - testGCPCredsSecretPassthrough("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{ + testGCPCredsSecretPassthrough("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), + }, mockRootGCPClient: func(mockCtrl *gomock.Controller) *mockgcp.MockClient { mockGCPClient := mockgcp.NewMockClient(mockCtrl) mockGetProjectName(mockGCPClient) @@ -777,10 +806,12 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { testOperatorConfig(""), createTestNamespace(testSecretNamespace), testGCPCredentialsRequest(t), - testGCPCredsSecretPassthrough("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{ + testGCPCredsSecretPassthrough("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), + }, mockRootGCPClient: func(mockCtrl *gomock.Controller) *mockgcp.MockClient { mockGCPClient := mockgcp.NewMockClient(mockCtrl) mockGetProjectName(mockGCPClient) @@ -814,11 +845,13 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { testOperatorConfig(""), createTestNamespace(testSecretNamespace), testGCPPassthroughCredentialsRequest(t), - testGCPCredsSecretPassthrough("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), testGCPCredsSecret(testSecretNamespace, testSecretName, testRootGCPAuth), testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{ + testGCPCredsSecretPassthrough("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), + }, mockRootGCPClient: func(mockCtrl *gomock.Controller) *mockgcp.MockClient { mockGCPClient := mockgcp.NewMockClient(mockCtrl) mockGetProjectName(mockGCPClient) @@ -846,11 +879,13 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { testOperatorConfig(""), createTestNamespace(testSecretNamespace), testGCPPassthroughCredentialsRequest(t), - testGCPCredsSecretPassthrough("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), testGCPCredsSecret(testSecretNamespace, testSecretName, testOldPassthroughPrivateData), testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{ + testGCPCredsSecretPassthrough("kube-system", constants.GCPCloudCredSecretName, testRootGCPAuth), + }, mockRootGCPClient: func(mockCtrl *gomock.Controller) *mockgcp.MockClient { mockGCPClient := mockgcp.NewMockClient(mockCtrl) mockGetProjectName(mockGCPClient) @@ -898,11 +933,15 @@ func TestCredentialsRequestGCPReconcile(t *testing.T) { fakeClient := fake.NewClientBuilder(). WithStatusSubresource(&minterv1.CredentialsRequest{}). WithRuntimeObjects(test.existing...).Build() + fakeAdminClient := fake.NewClientBuilder(). + WithRuntimeObjects(test.existingAdmin...).Build() rcr := &ReconcileCredentialsRequest{ - Client: fakeClient, + Client: fakeClient, + AdminClient: fakeAdminClient, Actuator: &actuator.Actuator{ - Client: fakeClient, - Codec: codec, + Client: fakeClient, + RootCredClient: fakeAdminClient, + Codec: codec, GCPClientBuilder: func(name string, jsonAUTH []byte) (mintergcp.Client, error) { if string(jsonAUTH) == testRootGCPAuth { return mockRootGCPClient, nil diff --git a/pkg/operator/credentialsrequest/credentialsrequest_controller_label_test.go b/pkg/operator/credentialsrequest/credentialsrequest_controller_label_test.go new file mode 100644 index 0000000000..b37f9991b8 --- /dev/null +++ b/pkg/operator/credentialsrequest/credentialsrequest_controller_label_test.go @@ -0,0 +1,260 @@ +/* +Copyright 2023 The OpenShift Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package credentialsrequest + +import ( + "context" + "testing" + + "github.com/google/go-cmp/cmp" + configv1 "github.com/openshift/api/config/v1" + minterv1 "github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential/v1" + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + clientgofake "k8s.io/client-go/kubernetes/fake" + clientgotesting "k8s.io/client-go/testing" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/reconcile" +) + +func TestReconcileSecretMissingLabel_Reconcile(t *testing.T) { + // we unconditionally send the SSA request, since it's easy and trivial to make the logic correct. + // the server determines if a mutation needs to occur. so, we expect the patch to be sent in every case + tests := []struct { + name string + existing []runtime.Object + expectErr bool + expected []*clientgotesting.PatchActionImpl + }{ + { + name: "already labeled, nothing to do", + existing: []runtime.Object{ + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: testSecretName, + Namespace: testSecretNamespace, + Annotations: map[string]string{ + minterv1.AnnotationCredentialsRequest: "whatever", + }, + Labels: map[string]string{ + minterv1.LabelCredentialsRequest: minterv1.LabelCredentialsRequestValue, + }, + }, + }, + }, + expectErr: false, + expected: []*clientgotesting.PatchActionImpl{{ + PatchType: types.ApplyPatchType, + ActionImpl: clientgotesting.ActionImpl{Namespace: "myproject", Verb: "patch", Resource: corev1.SchemeGroupVersion.WithResource("secrets")}, + Name: "test-secret", + Patch: []uint8(`{"kind":"Secret","apiVersion":"v1","metadata":{"name":"test-secret","namespace":"myproject","labels":{"cloudcredential.openshift.io/credentials-request":"true"}}}`), + }}, + }, + { + name: "no label, but no annotation, so nothing to do", + existing: []runtime.Object{ + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: testSecretName, + Namespace: testSecretNamespace, + }, + }, + }, + expectErr: false, + expected: []*clientgotesting.PatchActionImpl{{ + PatchType: types.ApplyPatchType, + ActionImpl: clientgotesting.ActionImpl{Namespace: "myproject", Verb: "patch", Resource: corev1.SchemeGroupVersion.WithResource("secrets")}, + Name: "test-secret", + Patch: []uint8(`{"kind":"Secret","apiVersion":"v1","metadata":{"name":"test-secret","namespace":"myproject","labels":{"cloudcredential.openshift.io/credentials-request":"true"}}}`), + }}, + }, + { + name: "annotation but missing label, should be labeled", + existing: []runtime.Object{ + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: testSecretName, + Namespace: testSecretNamespace, + Annotations: map[string]string{ + minterv1.AnnotationCredentialsRequest: "whatever", + }, + }, + }, + }, + expectErr: false, + expected: []*clientgotesting.PatchActionImpl{{ + PatchType: types.ApplyPatchType, + ActionImpl: clientgotesting.ActionImpl{Namespace: "myproject", Verb: "patch", Resource: corev1.SchemeGroupVersion.WithResource("secrets")}, + Name: "test-secret", + Patch: []uint8(`{"kind":"Secret","apiVersion":"v1","metadata":{"name":"test-secret","namespace":"myproject","labels":{"cloudcredential.openshift.io/credentials-request":"true"}}}`), + }}, + }, + { + name: "annotation but bad label value, should be labeled", + existing: []runtime.Object{ + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: testSecretName, + Namespace: testSecretNamespace, + Annotations: map[string]string{ + minterv1.AnnotationCredentialsRequest: "whatever", + }, + Labels: map[string]string{ + minterv1.LabelCredentialsRequest: "wrong", + }, + }, + }, + }, + expectErr: false, + expected: []*clientgotesting.PatchActionImpl{{ + PatchType: types.ApplyPatchType, + ActionImpl: clientgotesting.ActionImpl{Namespace: "myproject", Verb: "patch", Resource: corev1.SchemeGroupVersion.WithResource("secrets")}, + Name: "test-secret", + Patch: []uint8(`{"kind":"Secret","apiVersion":"v1","metadata":{"name":"test-secret","namespace":"myproject","labels":{"cloudcredential.openshift.io/credentials-request":"true"}}}`), + }}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + fakeClient := fake.NewClientBuilder(). + WithRuntimeObjects(test.existing...).Build() + + var actions []*clientgotesting.PatchActionImpl + fakeMutatingClient := clientgofake.NewSimpleClientset(test.existing...) + fakeMutatingClient.PrependReactor("*", "secrets", func(action clientgotesting.Action) (handled bool, ret runtime.Object, err error) { + impl, ok := action.(clientgotesting.PatchActionImpl) + if !ok { + return false, nil, nil + } + actions = append(actions, &impl) + return false, nil, nil + }) + + rcr := ReconcileSecretMissingLabel{ + cachedClient: fakeClient, + mutatingClient: fakeMutatingClient.CoreV1(), + } + + _, err := rcr.Reconcile(context.TODO(), reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: testSecretName, + Namespace: testSecretNamespace, + }, + }) + + if err != nil && !test.expectErr { + require.NoError(t, err, "Unexpected error: %v", err) + } + if err == nil && test.expectErr { + t.Errorf("Expected error but got none") + } + + if diff := cmp.Diff(actions, test.expected); diff != "" { + t.Errorf("incorrect actions: %v", diff) + } + }) + } +} + +func TestReconcileSecretMissingLabel_GetConditions(t *testing.T) { + tests := []struct { + name string + existing []runtime.Object + expectErr bool + // Expected conditions on the credentials cluster operator: + expectedCOConditions []configv1.ClusterOperatorStatusCondition + }{ + { + name: "no secrets, no condition", + existing: []runtime.Object{}, + expectErr: false, + expectedCOConditions: []configv1.ClusterOperatorStatusCondition{}, + }, + { + name: "no secrets missing labels, no condition", + existing: []runtime.Object{ + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: testSecretName, + Namespace: testSecretNamespace, + Annotations: map[string]string{ + minterv1.AnnotationCredentialsRequest: "whatever", + }, + Labels: map[string]string{ + minterv1.LabelCredentialsRequest: minterv1.LabelCredentialsRequestValue, + }, + }, + }, + }, + expectErr: false, + expectedCOConditions: []configv1.ClusterOperatorStatusCondition{}, + }, + { + name: "secrets missing labels, progressing condition", + existing: []runtime.Object{ + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: testSecretName, + Namespace: testSecretNamespace, + Annotations: map[string]string{ + minterv1.AnnotationCredentialsRequest: "whatever", + }, + }, + }, + }, + expectErr: false, + expectedCOConditions: []configv1.ClusterOperatorStatusCondition{ + { + Type: configv1.OperatorProgressing, + Status: configv1.ConditionTrue, + Reason: "LabelsMissing", + Message: "1 secrets created for CredentialsRequests have not been labelled", + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + fakeClient := fake.NewClientBuilder(). + WithRuntimeObjects(test.existing...).Build() + + rcr := ReconcileSecretMissingLabel{ + cachedClient: fakeClient, + mutatingClient: nil, + } + + conditions, err := rcr.GetConditions(logrus.StandardLogger()) + + if err != nil && !test.expectErr { + require.NoError(t, err, "Unexpected error: %v", err) + } + if err == nil && test.expectErr { + t.Errorf("Expected error but got none") + } + + if diff := cmp.Diff(conditions, test.expectedCOConditions); diff != "" { + t.Errorf("incorrect conditions: %v", diff) + } + }) + } +} diff --git a/pkg/operator/credentialsrequest/credentialsrequest_controller_test.go b/pkg/operator/credentialsrequest/credentialsrequest_controller_test.go index 49da99416a..e5109105f4 100644 --- a/pkg/operator/credentialsrequest/credentialsrequest_controller_test.go +++ b/pkg/operator/credentialsrequest/credentialsrequest_controller_test.go @@ -109,6 +109,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { tests := []struct { name string existing []runtime.Object + existingAdmin []runtime.Object expectErr bool mockRootAWSClient func(mockCtrl *gomock.Controller) *mockaws.MockClient mockReadAWSClient func(mockCtrl *gomock.Controller) *mockaws.MockClient @@ -131,9 +132,11 @@ func TestCredentialsRequestReconcile(t *testing.T) { cr.ObjectMeta.Finalizers = []string{} return cr }(), - testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{ + testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), + }, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) return mockAWSClient @@ -153,11 +156,13 @@ func TestCredentialsRequestReconcile(t *testing.T) { 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), testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{ + testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), + }, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) mockGetUser(mockAWSClient) @@ -194,11 +199,13 @@ func TestCredentialsRequestReconcile(t *testing.T) { 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), testClusterVersion(), testInfrastructure(""), }, + existingAdmin: []runtime.Object{ + testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), + }, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) mockGetUser(mockAWSClient) @@ -236,10 +243,12 @@ func TestCredentialsRequestReconcile(t *testing.T) { createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testCredentialsRequest(t), - testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{ + testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), + }, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) mockCreateUser(mockAWSClient) @@ -275,6 +284,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{}, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) return mockAWSClient @@ -308,12 +318,14 @@ func TestCredentialsRequestReconcile(t *testing.T) { 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), testAWSCredsSecret(testSecretNamespace, testSecretName, testAWSAccessKeyID, testAWSSecretAccessKey), testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{ + testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), + }, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) return mockAWSClient @@ -343,12 +355,14 @@ func TestCredentialsRequestReconcile(t *testing.T) { 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), testAWSCredsSecret(testSecretNamespace, testSecretName, testAWSAccessKeyID, testAWSSecretAccessKey), testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{ + testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), + }, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) mockTagUser(mockAWSClient) @@ -388,6 +402,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{}, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) return mockAWSClient @@ -418,10 +433,12 @@ func TestCredentialsRequestReconcile(t *testing.T) { createTestNamespace(testSecretNamespace), testCredentialsRequest(t), testAWSCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-iam-ro-creds", testReadAWSAccessKeyID, testReadAWSSecretAccessKey), - testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{ + testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), + }, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) mockCreateAccessKey(mockAWSClient, testAWSAccessKeyID2, testAWSSecretAccessKey2) @@ -455,12 +472,14 @@ func TestCredentialsRequestReconcile(t *testing.T) { 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), testAWSCredsSecret(testSecretNamespace, testSecretName, testAWSAccessKeyID, testAWSSecretAccessKey), testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{ + testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), + }, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) mockCreateAccessKey(mockAWSClient, testAWSAccessKeyID2, testAWSSecretAccessKey2) @@ -493,12 +512,14 @@ func TestCredentialsRequestReconcile(t *testing.T) { createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testProvisionedCredentialsRequest(t), - testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), testAWSCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-iam-ro-creds", testReadAWSAccessKeyID, testReadAWSSecretAccessKey), testAWSCredsSecret(testSecretNamespace, testSecretName, testAWSAccessKeyID, testAWSSecretAccessKey), testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{ + testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), + }, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) mockCreateAccessKey(mockAWSClient, testAWSAccessKeyID, testAWSSecretAccessKey) @@ -534,12 +555,14 @@ func TestCredentialsRequestReconcile(t *testing.T) { 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), }, + existingAdmin: []runtime.Object{ + testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), + }, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) return mockAWSClient @@ -569,12 +592,14 @@ func TestCredentialsRequestReconcile(t *testing.T) { createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testCredentialsRequestWithDeletionTimestamp(t), - testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), testAWSCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-iam-ro-creds", testReadAWSAccessKeyID, testReadAWSSecretAccessKey), testAWSCredsSecret(testSecretNamespace, testSecretName, testAWSAccessKeyID, testAWSSecretAccessKey), testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{ + testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), + }, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) mockListAccessKeys(mockAWSClient, testAWSAccessKeyID) @@ -596,9 +621,11 @@ func TestCredentialsRequestReconcile(t *testing.T) { testInfrastructure(testInfraName), createTestNamespace(testSecretNamespace), testPassthroughCredentialsRequest(t), - testPassthroughAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), testAWSCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-iam-ro-creds", testReadAWSAccessKeyID, testReadAWSSecretAccessKey), }, + existingAdmin: []runtime.Object{ + testPassthroughAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), + }, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) mockGetUser(mockAWSClient) @@ -626,9 +653,11 @@ func TestCredentialsRequestReconcile(t *testing.T) { testInfrastructure(testInfraName), createTestNamespace(testSecretNamespace), testPassthroughCredentialsRequest(t), - testPassthroughAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), testAWSCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-iam-ro-creds", testReadAWSAccessKeyID, testReadAWSSecretAccessKey), }, + existingAdmin: []runtime.Object{ + testPassthroughAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), + }, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) mockGetUser(mockAWSClient) @@ -657,10 +686,12 @@ func TestCredentialsRequestReconcile(t *testing.T) { testInfrastructure(testInfraName), createTestNamespace(testSecretNamespace), testPassthroughCredentialsRequestWithLastSyncResourceVersion(t, "12345"), - testPassthroughAWSCredsSecretWithResourceVersion("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey, "12345"), testAWSCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-iam-ro-creds", testReadAWSAccessKeyID, testReadAWSSecretAccessKey), testAWSCredsSecret(testSecretNamespace, testSecretName, testRootAWSAccessKeyID, testRootAWSSecretAccessKey), }, + existingAdmin: []runtime.Object{ + testPassthroughAWSCredsSecretWithResourceVersion("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey, "12345"), + }, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) mockGetUser(mockAWSClient) @@ -687,10 +718,12 @@ func TestCredentialsRequestReconcile(t *testing.T) { testInfrastructure(testInfraName), createTestNamespace(testSecretNamespace), testPassthroughCredentialsRequestWithLastSyncResourceVersion(t, "0001"), - testPassthroughAWSCredsSecretWithResourceVersion("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey, "0002"), testAWSCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-iam-ro-creds", testReadAWSAccessKeyID, testReadAWSSecretAccessKey), testAWSCredsSecret(testSecretNamespace, testSecretName, testAWSAccessKeyID, testAWSSecretAccessKey), }, + existingAdmin: []runtime.Object{ + testPassthroughAWSCredsSecretWithResourceVersion("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey, "0002"), + }, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) mockGetUser(mockAWSClient) @@ -718,10 +751,12 @@ func TestCredentialsRequestReconcile(t *testing.T) { testInfrastructure(testInfraName), createTestNamespace(testSecretNamespace), testPassthroughCredentialsRequest(t), - testPassthroughAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), testAWSCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-iam-ro-creds", testReadAWSAccessKeyID, testReadAWSSecretAccessKey), testAWSCredsSecret(testSecretNamespace, testSecretName, testAWSAccessKeyID, testAWSSecretAccessKey), }, + existingAdmin: []runtime.Object{ + testPassthroughAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), + }, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) return mockAWSClient @@ -745,10 +780,13 @@ func TestCredentialsRequestReconcile(t *testing.T) { testInfrastructure(testInfraName), createTestNamespace(testSecretNamespace), testPassthroughCredentialsRequestWithDeletionTimestamp(t), - testPassthroughAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), testAWSCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-iam-ro-creds", testReadAWSAccessKeyID, testReadAWSSecretAccessKey), testAWSCredsSecret(testSecretNamespace, testSecretName, testRootAWSAccessKeyID, testRootAWSSecretAccessKey), }, + existingAdmin: []runtime.Object{ + testPassthroughAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), + }, + mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) return mockAWSClient @@ -761,6 +799,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { testInfrastructure(testInfraName), testCredentialsRequest(t), }, + existingAdmin: []runtime.Object{}, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) return mockAWSClient @@ -780,6 +819,8 @@ func TestCredentialsRequestReconcile(t *testing.T) { testInfrastructure(testInfraName), createTestNamespace(testSecretNamespace), testCredentialsRequest(t), + }, + existingAdmin: []runtime.Object{ testInsufficientAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), }, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { @@ -806,6 +847,9 @@ func TestCredentialsRequestReconcile(t *testing.T) { testAWSCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-iam-ro-creds", testReadAWSAccessKeyID, testReadAWSSecretAccessKey), testClusterVersion(), }, + existingAdmin: []runtime.Object{ + testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), + }, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) return mockAWSClient @@ -831,11 +875,13 @@ func TestCredentialsRequestReconcile(t *testing.T) { createTestNamespace(testSecretNamespace), testInfrastructure(testInfraName), testCredentialsRequestWithDeletionTimestamp(t), - testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), testAWSCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-iam-ro-creds", testReadAWSAccessKeyID, testReadAWSSecretAccessKey), testAWSCredsSecret(testNamespace, testSecretName, testAWSAccessKeyID, testAWSSecretAccessKey), testClusterVersion(), }, + existingAdmin: []runtime.Object{ + testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), + }, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) mockListAccessKeys(mockAWSClient, testAWSAccessKeyID) @@ -879,10 +925,12 @@ func TestCredentialsRequestReconcile(t *testing.T) { createTestNamespace(testNamespace), testInfrastructure(testInfraName), testClusterVersion(), - testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), createTestNamespace(testSecretNamespace), testCredentialsRequestWithRecentLastSync(t), }, + existingAdmin: []runtime.Object{ + testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), + }, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) mockGetUser(mockAWSClient) @@ -914,7 +962,6 @@ func TestCredentialsRequestReconcile(t *testing.T) { cr.Generation = cr.Generation + 1 // rev the generation to trigger the sync objects = append(objects, cr) - objects = append(objects, testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey)) objects = append(objects, testAWSCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-iam-ro-creds", testReadAWSAccessKeyID, testReadAWSSecretAccessKey)) objects = append(objects, testAWSCredsSecret(testSecretNamespace, testSecretName, testAWSAccessKeyID, testAWSSecretAccessKey)) objects = append(objects, testClusterVersion()) @@ -922,6 +969,11 @@ func TestCredentialsRequestReconcile(t *testing.T) { return objects }(), + existingAdmin: func() []runtime.Object { + objects := []runtime.Object{} + objects = append(objects, testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey)) + return objects + }(), mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) return mockAWSClient @@ -944,6 +996,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { testOperatorConfig(""), testGCPCredentialsRequest(t), }, + existingAdmin: []runtime.Object{}, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { return mockaws.NewMockClient(mockCtrl) }, @@ -975,6 +1028,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { return cr }(), }, + existingAdmin: []runtime.Object{}, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { return mockaws.NewMockClient(mockCtrl) }, @@ -996,11 +1050,13 @@ func TestCredentialsRequestReconcile(t *testing.T) { 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), testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{ + testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), + }, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) mockGetUserWithPermissionsBoundary(mockAWSClient) @@ -1042,6 +1098,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{}, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) return mockAWSClient @@ -1061,6 +1118,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{}, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) return mockAWSClient @@ -1081,6 +1139,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { testClusterVersion(), testInfrastructure(testInfraName), }, + existingAdmin: []runtime.Object{}, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) return mockAWSClient @@ -1105,9 +1164,11 @@ func TestCredentialsRequestReconcile(t *testing.T) { testInfrastructure(testInfraName), createTestNamespace(testSecretNamespace), testAWSCredsSecret(testSecretNamespace, testSecretName, testAWSAccessKeyID, testAWSSecretAccessKey), - testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), testClusterVersion(), }, + existingAdmin: []runtime.Object{ + testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), + }, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) mockGetUser(mockAWSClient) @@ -1136,9 +1197,11 @@ func TestCredentialsRequestReconcile(t *testing.T) { testInfrastructure(testInfraName), createTestNamespace(testSecretNamespace), testAWSCredsSecret(testSecretNamespace, testSecretName, testAWSAccessKeyID, testAWSSecretAccessKey), - testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), testClusterVersion(), }, + existingAdmin: []runtime.Object{ + testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), + }, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) mockGetUser(mockAWSClient) @@ -1188,9 +1251,11 @@ func TestCredentialsRequestReconcile(t *testing.T) { testInfrastructure(testInfraName), createTestNamespace(testSecretNamespace), testAWSCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-iam-ro-creds", testReadAWSAccessKeyID, testReadAWSSecretAccessKey), - testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), testClusterVersion(), }, + existingAdmin: []runtime.Object{ + testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), + }, mockReadAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) // Just mock up GetUser that doesn't fail for the read-only account test @@ -1271,6 +1336,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { testAWSCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-iam-ro-creds", testReadAWSAccessKeyID, testReadAWSSecretAccessKey), testClusterVersion(), }, + existingAdmin: []runtime.Object{}, mockReadAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) // Just mock up GetUser that doesn't fail for the read-only account test @@ -1301,6 +1367,7 @@ func TestCredentialsRequestReconcile(t *testing.T) { existing: []runtime.Object{ testCredentialsRequest(t), }, + existingAdmin: []runtime.Object{}, mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) return mockAWSClient @@ -1320,9 +1387,6 @@ func TestCredentialsRequestReconcile(t *testing.T) { cr.Status.LastSyncCloudCredsSecretResourceVersion = "1" // resource version of root creds before update objects = append(objects, cr) - credentialsRootSecret := testPassthroughAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey) - credentialsRootSecret.ResourceVersion = testCredRootSecretResourceVersion // resource version of root creds after update - objects = append(objects, credentialsRootSecret) objects = append(objects, testAWSCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-iam-ro-creds", testReadAWSAccessKeyID, testReadAWSSecretAccessKey)) objects = append(objects, testAWSCredsSecret(testSecretNamespace, testSecretName, testAWSAccessKeyID, testAWSSecretAccessKey)) objects = append(objects, testClusterVersion()) @@ -1330,6 +1394,13 @@ func TestCredentialsRequestReconcile(t *testing.T) { return objects }(), + existingAdmin: func() []runtime.Object { + objects := []runtime.Object{} + credentialsRootSecret := testPassthroughAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey) + credentialsRootSecret.ResourceVersion = testCredRootSecretResourceVersion // resource version of root creds after update + objects = append(objects, credentialsRootSecret) + return objects + }(), mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { mockAWSClient := mockaws.NewMockClient(mockCtrl) return mockAWSClient @@ -1377,12 +1448,16 @@ func TestCredentialsRequestReconcile(t *testing.T) { fakeClient := fake.NewClientBuilder(). WithStatusSubresource(&minterv1.CredentialsRequest{}). WithRuntimeObjects(test.existing...).Build() + fakeAdminClient := fake.NewClientBuilder(). + WithRuntimeObjects(test.existingAdmin...).Build() rcr := &ReconcileCredentialsRequest{ - Client: fakeClient, + Client: fakeClient, + AdminClient: fakeAdminClient, Actuator: &actuator.AWSActuator{ - Client: fakeClient, - Codec: codec, - Scheme: scheme.Scheme, + Client: fakeClient, + RootCredClient: fakeAdminClient, + Codec: codec, + Scheme: scheme.Scheme, AWSClientBuilder: func(accessKeyID, secretAccessKey []byte, c client.Client) (minteraws.Client, error) { if string(accessKeyID) == testRootAWSAccessKeyID { return mockRootAWSClient, nil diff --git a/pkg/operator/credentialsrequest/credentialsrequest_controller_vsphere_test.go b/pkg/operator/credentialsrequest/credentialsrequest_controller_vsphere_test.go index bc0a5ab27f..21f7c49b78 100644 --- a/pkg/operator/credentialsrequest/credentialsrequest_controller_vsphere_test.go +++ b/pkg/operator/credentialsrequest/credentialsrequest_controller_vsphere_test.go @@ -67,10 +67,11 @@ func TestCredentialsRequestVSphereReconcile(t *testing.T) { } tests := []struct { - name string - existing []runtime.Object - expectErr bool - validate func(client.Client, *testing.T) + name string + existing []runtime.Object + existingAdmin []runtime.Object + expectErr bool + validate func(client.Client, *testing.T) // Expected conditions on the credentials request: expectedConditions []ExpectedCondition // Expected conditions on the credentials cluster operator: @@ -82,9 +83,11 @@ func TestCredentialsRequestVSphereReconcile(t *testing.T) { testOperatorConfig(""), createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), - testVSphereCredsSecretPassthrough(), testVSphereCredentialsRequest(t), }, + existingAdmin: []runtime.Object{ + testVSphereCredsSecretPassthrough(), + }, validate: func(c client.Client, t *testing.T) { targetSecret := getCredRequestTargetSecret(c) require.NotNil(t, targetSecret, "expected non-empty target secret to exist") @@ -104,7 +107,8 @@ func TestCredentialsRequestVSphereReconcile(t *testing.T) { createTestNamespace(testSecretNamespace), testVSphereCredentialsRequest(t), }, - expectErr: true, + existingAdmin: []runtime.Object{}, + expectErr: true, validate: func(c client.Client, t *testing.T) { targetSecret := getCredRequestTargetSecret(c) assert.Nil(t, targetSecret) @@ -125,9 +129,11 @@ func TestCredentialsRequestVSphereReconcile(t *testing.T) { createTestNamespace(testNamespace), createTestNamespace(testSecretNamespace), testVSphereCredentialsRequestWithDeletionTimestamp(t), - testVSphereCredsSecretPassthrough(), testSecret(testSecretNamespace, testSecretName, testVSphereCloudCredsSecretData), }, + existingAdmin: []runtime.Object{ + testVSphereCredsSecretPassthrough(), + }, validate: func(c client.Client, t *testing.T) { targetSecret := getCredRequestTargetSecret(c) assert.Nil(t, targetSecret) @@ -139,9 +145,11 @@ func TestCredentialsRequestVSphereReconcile(t *testing.T) { testOperatorConfig(""), createTestNamespace(testSecretNamespace), testVSphereCredentialsRequest(t), - testVSphereCredsSecretPassthrough(), testSecret(testSecretNamespace, testSecretName, testVSphereCloudCredsSecretData), }, + existingAdmin: []runtime.Object{ + testVSphereCredsSecretPassthrough(), + }, validate: func(c client.Client, t *testing.T) { targetSecret := getCredRequestTargetSecret(c) require.NotNil(t, targetSecret, "expected non-empty target secret to exist") @@ -159,8 +167,10 @@ func TestCredentialsRequestVSphereReconcile(t *testing.T) { testOperatorConfig(""), createTestNamespace(testSecretNamespace), testVSphereCredentialsRequest(t), + testSecret(testSecretNamespace, testSecretName, map[string][]byte{"key1": []byte("olddata")}), + }, + existingAdmin: []runtime.Object{ testVSphereCredsSecretPassthrough(), - testSecret(testSecretNamespace, testSecretName, map[string][]byte{"oldkey": []byte("olddata")}), }, validate: func(c client.Client, t *testing.T) { targetSecret := getCredRequestTargetSecret(c) @@ -184,11 +194,15 @@ func TestCredentialsRequestVSphereReconcile(t *testing.T) { fakeClient := fake.NewClientBuilder(). WithStatusSubresource(&minterv1.CredentialsRequest{}). WithRuntimeObjects(test.existing...).Build() + fakeAdminClient := fake.NewClientBuilder(). + WithRuntimeObjects(test.existingAdmin...).Build() rcr := &ReconcileCredentialsRequest{ - Client: fakeClient, + Client: fakeClient, + AdminClient: fakeAdminClient, Actuator: &actuator.VSphereActuator{ - Client: fakeClient, - Codec: codec, + Client: fakeClient, + RootCredClient: fakeAdminClient, + Codec: codec, }, platformType: configv1.VSpherePlatformType, } diff --git a/pkg/operator/loglevel/controller.go b/pkg/operator/loglevel/controller.go index 76fd38be17..e859098ca4 100644 --- a/pkg/operator/loglevel/controller.go +++ b/pkg/operator/loglevel/controller.go @@ -41,7 +41,7 @@ const ( ) // Add creates a new ConfigController and adds it to the Manager. -func Add(mgr manager.Manager, kubeConfig string) error { +func Add(mgr, rootCredentialManager manager.Manager, kubeConfig string) error { r := newReconciler(mgr) // Create a new controller diff --git a/pkg/operator/metrics/metrics.go b/pkg/operator/metrics/metrics.go index 39de41ce04..4fde1006bf 100644 --- a/pkg/operator/metrics/metrics.go +++ b/pkg/operator/metrics/metrics.go @@ -70,7 +70,7 @@ func init() { } // Add creates a new metrics Calculator and adds it to the Manager. -func Add(mgr manager.Manager, kubeConfig string) error { +func Add(mgr, rootCredentialManager manager.Manager, kubeConfig string) error { logger := log.WithField("controller", controllerName) mc := &Calculator{ diff --git a/pkg/operator/platform/platform.go b/pkg/operator/platform/platform.go index 47f0ab0747..21e645b823 100644 --- a/pkg/operator/platform/platform.go +++ b/pkg/operator/platform/platform.go @@ -3,10 +3,12 @@ package platform import ( "context" "fmt" - "github.com/openshift/cloud-credential-operator/pkg/operator/constants" "os" "time" + "github.com/openshift/cloud-credential-operator/pkg/operator/constants" + "github.com/openshift/cloud-credential-operator/pkg/util" + log "github.com/sirupsen/logrus" configv1 "github.com/openshift/api/config/v1" @@ -20,12 +22,11 @@ import ( "k8s.io/client-go/tools/clientcmd" "sigs.k8s.io/controller-runtime/pkg/client" crtconfig "sigs.k8s.io/controller-runtime/pkg/client/config" - "sigs.k8s.io/controller-runtime/pkg/manager" ) // GetInfraStatusUsingKubeconfig queries the k8s api for the infrastructure CR using the kubeconfig file // pointed to by the passed in kubeconfig (pass in empty string to use default k8s client configurations) -func GetInfraStatusUsingKubeconfig(m manager.Manager, kubeconfig string) (*configv1.InfrastructureStatus, error) { +func GetInfraStatusUsingKubeconfig(kubeconfig string) (*configv1.InfrastructureStatus, error) { c, err := getClient(kubeconfig) if err != nil { return nil, err @@ -70,6 +71,8 @@ func getClient(explicitKubeconfig string) (client.Client, error) { return nil, err } + util.SetupScheme(dynamicClient.Scheme()) + return dynamicClient, nil } diff --git a/pkg/operator/secretannotator/secretannotator_controller.go b/pkg/operator/secretannotator/secretannotator_controller.go index afad4f2fd9..dff44841e3 100644 --- a/pkg/operator/secretannotator/secretannotator_controller.go +++ b/pkg/operator/secretannotator/secretannotator_controller.go @@ -29,8 +29,8 @@ import ( log "github.com/sirupsen/logrus" ) -func Add(mgr manager.Manager, kubeconfig string) error { - infraStatus, err := platform.GetInfraStatusUsingKubeconfig(mgr, kubeconfig) +func Add(mgr, rootCredentialManager manager.Manager, kubeconfig string) error { + infraStatus, err := platform.GetInfraStatusUsingKubeconfig(kubeconfig) if err != nil { log.Fatal(err) } @@ -40,19 +40,19 @@ func Add(mgr manager.Manager, kubeconfig string) error { switch platformType { case configv1.AzurePlatformType: - return azure.Add(mgr, azure.NewReconciler(mgr)) + return azure.Add(rootCredentialManager, azure.NewReconciler(rootCredentialManager)) case configv1.AWSPlatformType: - return aws.Add(mgr, aws.NewReconciler(mgr)) + return aws.Add(rootCredentialManager, aws.NewReconciler(rootCredentialManager)) case configv1.GCPPlatformType: if infraStatus.PlatformStatus == nil || infraStatus.PlatformStatus.GCP == nil { log.Fatalf("Missing GCP configuration in infrastructure platform status") } - return gcp.Add(mgr, gcp.NewReconciler(mgr, infraStatus.PlatformStatus.GCP.ProjectID)) + return gcp.Add(rootCredentialManager, gcp.NewReconciler(rootCredentialManager, infraStatus.PlatformStatus.GCP.ProjectID)) case configv1.VSpherePlatformType: - return vsphere.Add(mgr, vsphere.NewReconciler(mgr)) + return vsphere.Add(rootCredentialManager, vsphere.NewReconciler(rootCredentialManager)) case configv1.OpenStackPlatformType: - return openstack.Add(mgr, openstack.NewReconciler(mgr)) + return openstack.Add(rootCredentialManager, openstack.NewReconciler(rootCredentialManager)) default: // returning the AWS implementation for default to avoid changing any behavior - return aws.Add(mgr, aws.NewReconciler(mgr)) + return aws.Add(rootCredentialManager, aws.NewReconciler(rootCredentialManager)) } } diff --git a/pkg/operator/status/status_controller.go b/pkg/operator/status/status_controller.go index ac77bfbfa6..85d107f623 100644 --- a/pkg/operator/status/status_controller.go +++ b/pkg/operator/status/status_controller.go @@ -88,9 +88,9 @@ func alwaysReconcileCCOConfigObject(ctx context.Context, c client.Object) []reco } // Add creates a new Status Controller and adds it to the Manager. -func Add(mgr manager.Manager, kubeConfig string) error { +func Add(mgr, rootCredentialManager manager.Manager, kubeConfig string) error { - infraStatus, err := platform.GetInfraStatusUsingKubeconfig(mgr, kubeConfig) + infraStatus, err := platform.GetInfraStatusUsingKubeconfig(kubeConfig) if err != nil { log.Fatal(err) } diff --git a/pkg/ovirt/actuator.go b/pkg/ovirt/actuator.go index e9442b3ff1..3aaa29dd39 100644 --- a/pkg/ovirt/actuator.go +++ b/pkg/ovirt/actuator.go @@ -18,11 +18,11 @@ package ovirt import ( "context" "fmt" - "reflect" "strconv" "github.com/openshift/cloud-credential-operator/pkg/operator/platform" "github.com/openshift/library-go/pkg/operator/configobserver/featuregates" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" log "github.com/sirupsen/logrus" @@ -49,8 +49,9 @@ const ( ) type OvirtActuator struct { - Client client.Client - Codec *minterv1.ProviderCodec + Client client.Client + RootCredClient client.Client + Codec *minterv1.ProviderCodec } func (a *OvirtActuator) GetFeatureGates(ctx context.Context) (featuregates.FeatureGate, error) { @@ -70,7 +71,7 @@ type OvirtCreds struct { } // NewActuator creates a new Ovirt actuator. -func NewActuator(client client.Client) (*OvirtActuator, error) { +func NewActuator(client, rootCredClient client.Client) (*OvirtActuator, error) { codec, err := minterv1.NewCodec() if err != nil { log.WithError(err).Error("error creating Ovirt codec") @@ -78,8 +79,9 @@ func NewActuator(client client.Client) (*OvirtActuator, error) { } return &OvirtActuator{ - Codec: codec, - Client: client, + Codec: codec, + Client: client, + RootCredClient: rootCredClient, }, nil } @@ -138,13 +140,11 @@ func (a *OvirtActuator) sync(ctx context.Context, cr *minterv1.CredentialsReques } logger.Debugf("provisioning secret") - existingSecret, err := a.loadExistingSecret(cr) - if err != nil && !errors.IsNotFound(err) { return err } - err = a.syncCredentialSecret(cr, &ovirtCreds, existingSecret, logger) + err = a.syncCredentialSecret(ctx, cr, &ovirtCreds, logger) if err != nil { msg := "error creating/updating secret" logger.WithError(err).Error(msg) @@ -157,101 +157,43 @@ func (a *OvirtActuator) sync(ctx context.Context, cr *minterv1.CredentialsReques return nil } -func (a *OvirtActuator) syncCredentialSecret(cr *minterv1.CredentialsRequest, ovirtCreds *OvirtCreds, existingSecret *corev1.Secret, logger log.FieldLogger) error { +func (a *OvirtActuator) syncCredentialSecret(ctx context.Context, cr *minterv1.CredentialsRequest, ovirtCreds *OvirtCreds, logger log.FieldLogger) error { sLog := logger.WithFields(log.Fields{ "targetSecret": fmt.Sprintf("%s/%s", cr.Spec.SecretRef.Namespace, cr.Spec.SecretRef.Name), "cr": fmt.Sprintf("%s/%s", cr.Namespace, cr.Name), }) - - if existingSecret == nil { - if ovirtCreds == nil { - msg := "new access key secret needed but no key data provided" - sLog.Error(msg) - return &actuatoriface.ActuatorError{ - ErrReason: minterv1.CredentialsProvisionFailure, - Message: msg, - } + sLog.Infof("processing secret") + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: cr.Spec.SecretRef.Name, + Namespace: cr.Spec.SecretRef.Namespace, + }, + } + op, err := controllerutil.CreateOrPatch(ctx, a.Client, secret, func() error { + if secret.Labels == nil { + secret.Labels = map[string]string{} } - sLog.Info("creating secret") - secret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: cr.Spec.SecretRef.Name, - Namespace: cr.Spec.SecretRef.Namespace, - Annotations: map[string]string{ - minterv1.AnnotationCredentialsRequest: fmt.Sprintf("%s/%s", cr.Namespace, cr.Name), - }, - }, - Data: secretDataFrom(ovirtCreds), + secret.Labels[minterv1.LabelCredentialsRequest] = minterv1.LabelCredentialsRequestValue + if secret.Annotations == nil { + secret.Annotations = map[string]string{} } - - err := a.Client.Create(context.TODO(), secret) - if err != nil { - sLog.WithError(err).Error("error creating secret") - return err + secret.Annotations[minterv1.AnnotationCredentialsRequest] = fmt.Sprintf("%s/%s", cr.Namespace, cr.Name) + if secret.Data == nil { + secret.Data = map[string][]byte{} } - sLog.Info("secret created successfully") - return nil - } - - // Update the existing secret: - sLog.Debug("updating secret") - origSecret := existingSecret.DeepCopy() - if existingSecret.Annotations == nil { - existingSecret.Annotations = map[string]string{} - } - existingSecret.Annotations[minterv1.AnnotationCredentialsRequest] = fmt.Sprintf("%s/%s", cr.Namespace, cr.Name) - if ovirtCreds != nil { - existingSecret.Data = secretDataFrom(ovirtCreds) - } - - if !reflect.DeepEqual(existingSecret, origSecret) { - sLog.Info("target secret has changed, updating") - err := a.Client.Update(context.TODO(), existingSecret) - if err != nil { - msg := "error updating secret" - sLog.WithError(err).Error(msg) - return &actuatoriface.ActuatorError{ - ErrReason: minterv1.CredentialsProvisionFailure, - Message: msg, - } + for key, value := range secretDataFrom(ovirtCreds) { + secret.Data[key] = value } - } else { - sLog.Debug("target secret unchanged") - } - - return nil -} - -// loadExistingSecret load the secret from the API. If the secret is not found, the NotFound -// err is returned, with nil secret. -// The NotFound error can be used to signal the creation of a new signal. -func (a *OvirtActuator) loadExistingSecret(cr *minterv1.CredentialsRequest) (*corev1.Secret, error) { - logger := a.getLogger(cr) - - // Check if the credentials secret exists, if not we need to inform the syncer to generate a new one: - loadedSecret := &corev1.Secret{} - err := a.Client.Get(context.TODO(), types.NamespacedName{Namespace: cr.Spec.SecretRef.Namespace, Name: cr.Spec.SecretRef.Name}, loadedSecret) + return nil + }) + sLog.WithField("operation", op).Info("processed secret") if err != nil { - if errors.IsNotFound(err) { - logger.Debug("secret does not exist") - return nil, err + return &actuatoriface.ActuatorError{ + ErrReason: minterv1.CredentialsProvisionFailure, + Message: "error processing secret", } } - - if _, ok := loadedSecret.Data[urlKey]; !ok { - logger.Warningf("secret did not have expected key: %s", urlKey) - } - if _, ok := loadedSecret.Data[usernameKey]; !ok { - logger.Warningf("secret did not have expected key: %s", usernameKey) - } - if _, ok := loadedSecret.Data[passwordKey]; !ok { - logger.Warningf("secret did not have expected key: %s", passwordKey) - } - if _, ok := loadedSecret.Data[cabundleKey]; !ok { - logger.Warningf("secret did not have expected key: %s", cabundleKey) - } - - return loadedSecret, nil + return nil } // GetCredentialsRootSecretLocation returns the namespace and name where the parent credentials secret is stored. @@ -262,7 +204,7 @@ func (a *OvirtActuator) GetCredentialsRootSecretLocation() types.NamespacedName func (a *OvirtActuator) GetCredentialsRootSecret(ctx context.Context, cr *minterv1.CredentialsRequest) (*corev1.Secret, error) { logger := a.getLogger(cr) cloudCredSecret := &corev1.Secret{} - if err := a.Client.Get(ctx, a.GetCredentialsRootSecretLocation(), cloudCredSecret); err != nil { + if err := a.RootCredClient.Get(ctx, a.GetCredentialsRootSecretLocation(), cloudCredSecret); err != nil { msg := "unable to fetch root cloud cred secret" logger.WithError(err).Error(msg) return nil, &actuatoriface.ActuatorError{ @@ -367,5 +309,5 @@ func secretDataFrom(ovirtCreds *OvirtCreds) map[string][]byte { // if the system is considered not upgradeable. Otherwise, return nil as the default // value is for things to be upgradeable. func (a *OvirtActuator) Upgradeable(mode operatorv1.CloudCredentialsMode) *configv1.ClusterOperatorStatusCondition { - return utils.UpgradeableCheck(a.Client, mode, a.GetCredentialsRootSecretLocation()) + return utils.UpgradeableCheck(a.RootCredClient, mode, a.GetCredentialsRootSecretLocation()) } diff --git a/pkg/vsphere/actuator/actuator.go b/pkg/vsphere/actuator/actuator.go index 6758cf21af..d8047c52c3 100644 --- a/pkg/vsphere/actuator/actuator.go +++ b/pkg/vsphere/actuator/actuator.go @@ -21,6 +21,7 @@ import ( "reflect" log "github.com/sirupsen/logrus" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -43,8 +44,9 @@ var _ actuatoriface.Actuator = (*VSphereActuator)(nil) // VSphereActuator implements the CredentialsRequest Actuator interface to process CredentialsRequests in vSphere. type VSphereActuator struct { - Codec *minterv1.ProviderCodec - Client client.Client + Codec *minterv1.ProviderCodec + Client client.Client + RootCredClient client.Client } func (a *VSphereActuator) STSFeatureGateEnabled() bool { @@ -52,7 +54,7 @@ func (a *VSphereActuator) STSFeatureGateEnabled() bool { } // NewVSphereActuator creates a new VSphereActuator. -func NewVSphereActuator(client client.Client) (*VSphereActuator, error) { +func NewVSphereActuator(client, rootCredClient client.Client) (*VSphereActuator, error) { codec, err := minterv1.NewCodec() if err != nil { log.WithError(err).Error("error creating AWS codec") @@ -60,8 +62,9 @@ func NewVSphereActuator(client client.Client) (*VSphereActuator, error) { } return &VSphereActuator{ - Codec: codec, - Client: client, + Codec: codec, + Client: client, + RootCredClient: rootCredClient, }, nil } @@ -215,12 +218,7 @@ func (a *VSphereActuator) sync(ctx context.Context, cr *minterv1.CredentialsRequ } func (a *VSphereActuator) syncPassthrough(ctx context.Context, cr *minterv1.CredentialsRequest, cloudCredsSecret *corev1.Secret, logger log.FieldLogger) error { - existingSecret, err := a.loadExistingSecret(cr) - if err != nil { - return err - } - - err = a.syncTargetSecret(cr, cloudCredsSecret.Data, existingSecret, logger) + err := a.syncTargetSecret(ctx, cr, cloudCredsSecret.Data, logger) if err != nil { msg := "error creating/updating secret" logger.WithError(err).Error(msg) @@ -286,59 +284,42 @@ func (a *VSphereActuator) getLogger(cr *minterv1.CredentialsRequest) log.FieldLo }) } -func (a *VSphereActuator) syncTargetSecret(cr *minterv1.CredentialsRequest, secretData map[string][]byte, existingSecret *corev1.Secret, logger log.FieldLogger) error { +func (a *VSphereActuator) syncTargetSecret(ctx context.Context, cr *minterv1.CredentialsRequest, secretData map[string][]byte, logger log.FieldLogger) error { sLog := logger.WithFields(log.Fields{ "targetSecret": fmt.Sprintf("%s/%s", cr.Spec.SecretRef.Namespace, cr.Spec.SecretRef.Name), "cr": fmt.Sprintf("%s/%s", cr.Namespace, cr.Name), }) - - if existingSecret == nil || existingSecret.Name == "" { - sLog.Info("creating secret") - secret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: cr.Spec.SecretRef.Name, - Namespace: cr.Spec.SecretRef.Namespace, - Annotations: map[string]string{ - minterv1.AnnotationCredentialsRequest: fmt.Sprintf("%s/%s", cr.Namespace, cr.Name), - }, - }, - Data: secretData, + sLog.Infof("processing secret") + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: cr.Spec.SecretRef.Name, + Namespace: cr.Spec.SecretRef.Namespace, + }, + } + op, err := controllerutil.CreateOrPatch(ctx, a.Client, secret, func() error { + if secret.Labels == nil { + secret.Labels = map[string]string{} } - - err := a.Client.Create(context.TODO(), secret) - if err != nil { - sLog.WithError(err).Error("error creating secret") - return err + secret.Labels[minterv1.LabelCredentialsRequest] = minterv1.LabelCredentialsRequestValue + if secret.Annotations == nil { + secret.Annotations = map[string]string{} + } + secret.Annotations[minterv1.AnnotationCredentialsRequest] = fmt.Sprintf("%s/%s", cr.Namespace, cr.Name) + if secret.Data == nil { + secret.Data = map[string][]byte{} + } + for key, value := range secretData { + secret.Data[key] = value } - sLog.Info("secret created successfully") return nil - } - - // Update the existing secret: - sLog.Debug("updating secret") - origSecret := existingSecret.DeepCopy() - if existingSecret.Annotations == nil { - existingSecret.Annotations = map[string]string{} - } - existingSecret.Annotations[minterv1.AnnotationCredentialsRequest] = fmt.Sprintf("%s/%s", cr.Namespace, cr.Name) - - existingSecret.Data = secretData - - if !reflect.DeepEqual(existingSecret, origSecret) { - sLog.Info("target secret has changed, updating") - err := a.Client.Update(context.TODO(), existingSecret) - if err != nil { - msg := "error updating secret" - sLog.WithError(err).Error(msg) - return &actuatoriface.ActuatorError{ - ErrReason: minterv1.CredentialsProvisionFailure, - Message: msg, - } + }) + sLog.WithField("operation", op).Info("processed secret") + if err != nil { + return &actuatoriface.ActuatorError{ + ErrReason: minterv1.CredentialsProvisionFailure, + Message: "error processing secret", } - } else { - sLog.Debug("target secret unchanged") } - return nil } @@ -350,7 +331,7 @@ func (a *VSphereActuator) GetCredentialsRootSecretLocation() types.NamespacedNam func (a *VSphereActuator) GetCredentialsRootSecret(ctx context.Context, cr *minterv1.CredentialsRequest) (*corev1.Secret, error) { logger := a.getLogger(cr) cloudCredSecret := &corev1.Secret{} - if err := a.Client.Get(ctx, a.GetCredentialsRootSecretLocation(), cloudCredSecret); err != nil { + if err := a.RootCredClient.Get(ctx, a.GetCredentialsRootSecretLocation(), cloudCredSecret); err != nil { msg := "unable to fetch root cloud cred secret" logger.WithError(err).Error(msg) return nil, &actuatoriface.ActuatorError{ @@ -404,5 +385,5 @@ func isVSphereCredentials(providerSpec *runtime.RawExtension) (bool, error) { // if the system is considered not upgradeable. Otherwise, return nil as the default // value is for things to be upgradeable. func (a *VSphereActuator) Upgradeable(mode operatorv1.CloudCredentialsMode) *configv1.ClusterOperatorStatusCondition { - return utils.UpgradeableCheck(a.Client, mode, a.GetCredentialsRootSecretLocation()) + return utils.UpgradeableCheck(a.RootCredClient, mode, a.GetCredentialsRootSecretLocation()) }