From a58a09cfe838e43783a1847649139b414caf69c4 Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Thu, 22 Jun 2023 07:44:32 -0600 Subject: [PATCH 1/9] *: use a filtered LIST + WATCH on Secrets for AWS STS 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, two caveats: - in passthrough and mint mode, admin credentials are also provided by the user through Secrets, and we need to watch those, but we can't label them - on existing clusters, secrets exist from previous work these reconcilers did that will not have Secrets labelled We could solve the second issue with an interim release of this controller that labels all previous Secrets, but does not restrict the watch stream. Due to the way that controller-runtime closes over the client/cache concepts, it's difficult to solve the first issue, though, since we'd need two sets of clients and caches, both for Secrets, and ensure that we use one for client access to Secrets we're creating or mutating and the other when we're interacting with admin credentials. Not impossible to do, but tricky to implement and complex. Until we undertake that effort, we apply a simplification to the space: only when AWS STS mode is enabled, we will try to filter the LIST + WATCH. This mode is brand new, so we can be reasonably sure that there are no previous secrets on the cluster, and, we make the filtering best-effort in order to check if that assumption held. Second, AWS STS mode only runs in clusters without admin credentials, so if we apply the filter, we should not see failures downstream from clients that hope to see those objects but can't. Signed-off-by: Steve Kuznetsov --- .../v1/types_credentialsrequest.go | 4 + pkg/aws/actuator/actuator.go | 18 +++- pkg/aws/actuator/actuator_test.go | 2 +- pkg/azure/actuator.go | 3 + pkg/cmd/operator/cmd.go | 94 +++++++++++++++++-- pkg/gcp/actuator/actuator.go | 20 ++-- pkg/kubevirt/actuator.go | 8 ++ pkg/openstack/actuator.go | 18 +++- pkg/operator/controller.go | 12 +-- .../credentialsrequest_controller.go | 10 +- pkg/operator/platform/platform.go | 3 +- pkg/ovirt/actuator.go | 16 +++- pkg/vsphere/actuator/actuator.go | 16 +++- 13 files changed, 180 insertions(+), 44 deletions(-) diff --git a/pkg/apis/cloudcredential/v1/types_credentialsrequest.go b/pkg/apis/cloudcredential/v1/types_credentialsrequest.go index 69ba6f0e0e..d8bad90814 100644 --- a/pkg/apis/cloudcredential/v1/types_credentialsrequest.go +++ b/pkg/apis/cloudcredential/v1/types_credentialsrequest.go @@ -27,6 +27,10 @@ const ( // credentials in AWS before allowing the CredentialsRequest to be deleted in etcd. FinalizerDeprovision string = "cloudcredential.openshift.io/deprovision" + // LabelCredentialsRequest is to mark Secrets created as a target of CredentailsRequests. + LabelCredentialsRequest string = "cloudcredential.openshift.io/credentials-request" + LabelCredentialsRequestValue string = "true" + // AnnotationCredentialsRequest is used on Secrets created as a target of CredentailsRequests. // The annotation value will map back to the namespace/name of the CredentialsRequest that created // or adopted the secret. diff --git a/pkg/aws/actuator/actuator.go b/pkg/aws/actuator/actuator.go index 14b54305d8..0c66d9bee3 100644 --- a/pkg/aws/actuator/actuator.go +++ b/pkg/aws/actuator/actuator.go @@ -507,7 +507,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, existingSecret, "", logger) if err != nil { msg := "error creating/updating secret" logger.WithError(err).Error(msg) @@ -692,7 +692,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, existingSecret, desiredUserPolicy, logger) if err != nil { log.WithError(err).Error("error saving access key to secret") return err @@ -995,7 +995,7 @@ 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, existingSecret *corev1.Secret, 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), @@ -1015,6 +1015,9 @@ func (a *AWSActuator) syncAccessKeySecret(cr *minterv1.CredentialsRequest, acces ObjectMeta: metav1.ObjectMeta{ Name: cr.Spec.SecretRef.Name, Namespace: cr.Spec.SecretRef.Namespace, + Labels: map[string]string{ + minterv1.LabelCredentialsRequest: minterv1.LabelCredentialsRequestValue, + }, Annotations: map[string]string{ minterv1.AnnotationCredentialsRequest: fmt.Sprintf("%s/%s", cr.Namespace, cr.Name), minterv1.AnnotationAWSPolicyLastApplied: userPolicy, @@ -1027,7 +1030,7 @@ func (a *AWSActuator) syncAccessKeySecret(cr *minterv1.CredentialsRequest, acces }, } - err := a.Client.Create(context.TODO(), secret) + err := a.Client.Create(ctx, secret) if err != nil { sLog.WithError(err).Error("error creating secret") return err @@ -1039,6 +1042,11 @@ func (a *AWSActuator) syncAccessKeySecret(cr *minterv1.CredentialsRequest, acces // Update the existing secret: sLog.Debug("updating secret") origSecret := existingSecret.DeepCopy() + if existingSecret.Labels == nil { + existingSecret.Labels = map[string]string{} + } + existingSecret.Labels[minterv1.LabelCredentialsRequest] = minterv1.LabelCredentialsRequestValue + if existingSecret.Annotations == nil { existingSecret.Annotations = map[string]string{} } @@ -1054,7 +1062,7 @@ func (a *AWSActuator) syncAccessKeySecret(cr *minterv1.CredentialsRequest, acces if !reflect.DeepEqual(existingSecret, origSecret) { sLog.Info("target secret has changed, updating") - err := a.Client.Update(context.TODO(), existingSecret) + err := a.Client.Update(ctx, existingSecret) if err != nil { msg := "error updating secret" sLog.WithError(err).Error(msg) diff --git a/pkg/aws/actuator/actuator_test.go b/pkg/aws/actuator/actuator_test.go index 6b8e6fe8a1..33e1722845 100644 --- a/pkg/aws/actuator/actuator_test.go +++ b/pkg/aws/actuator/actuator_test.go @@ -458,7 +458,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, test.existingSecret, "exampleAWSPolicy", logger) require.NoError(t, err, "unexpected error creating/updating Secret") diff --git a/pkg/azure/actuator.go b/pkg/azure/actuator.go index 55185d8a9f..b173ffcb16 100644 --- a/pkg/azure/actuator.go +++ b/pkg/azure/actuator.go @@ -329,6 +329,9 @@ func copyCredentialsSecret(cr *minterv1.CredentialsRequest, src, dest *corev1.Se dest.ObjectMeta = metav1.ObjectMeta{ Name: cr.Spec.SecretRef.Name, Namespace: cr.Spec.SecretRef.Namespace, + Labels: map[string]string{ + minterv1.LabelCredentialsRequest: minterv1.LabelCredentialsRequestValue, + }, Annotations: map[string]string{ minterv1.AnnotationCredentialsRequest: fmt.Sprintf("%s/%s", cr.Namespace, cr.Name), }, diff --git a/pkg/cmd/operator/cmd.go b/pkg/cmd/operator/cmd.go index 78c6442747..5a4853eefb 100644 --- a/pkg/cmd/operator/cmd.go +++ b/pkg/cmd/operator/cmd.go @@ -35,8 +35,13 @@ import ( "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,6 +104,70 @@ 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") + } + + // 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, two caveats: + // - in passthrough and mint mode, admin credentials are also provided by the user through Secrets, + // and we need to watch those, but we can't label them + // - on existing clusters, secrets exist from previous work these reconcilers did that will not have + // Secrets labelled + // We could solve the second issue with an interim release of this controller that labels all previous + // Secrets, but does not restrict the watch stream. + // Due to the way that controller-runtime closes over the client/cache concepts, it's difficult to + // solve the first issue, though, since we'd need two sets of clients and caches, both for Secrets, + // and ensure that we use one for client access to Secrets we're creating or mutating and the other + // when we're interacting with admin credentials. Not impossible to do, but tricky to implement and + // complex. + // Until we undertake that effort, we apply a simplification to the space: only when AWS STS mode is + // enabled, we will try to filter the LIST + WATCH. This mode is brand new, so we can be reasonably + // sure that there are no previous secrets on the cluster, and, we make the filtering best-effort + // in order to check if that assumption held. Second, AWS STS mode only runs in clusters without + // admin credentials, so if we apply the filter, we should not see failures downstream from clients + // that hope to see those objects but can't. + var filteredWatchPossible bool + if awsSecurityTokenServiceGateEnabled { + 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 isMissingSecretLabel(secret) { + missing = append(missing, types.NamespacedName{ + Namespace: secret.Namespace, + Name: secret.Name, + }) + } + } + if len(missing) != 0 { + log.WithField("missing", missing).Warnf("%s feature gate enabled but not all secrets labelled, falling back to caching all secrets on cluster", configv1.FeatureGateAWSSecurityTokenService) + } else { + filteredWatchPossible = true + } + } + // Create a new Cmd to provide shared dependencies and start components log.Info("setting up manager") mgr, err := manager.New(cfg, manager.Options{ @@ -113,6 +182,13 @@ func NewOperator() *cobra.Command { "metadata.name": constants.CloudCredOperatorConfigMap, }), } + if filteredWatchPossible { + opts.ByObject[&corev1.Secret{}] = cache.ByObject{ + Label: labels.SelectorFromSet(labels.Set{ + minterv1.LabelCredentialsRequest: minterv1.LabelCredentialsRequestValue, + }), + } + } return cache.New(config, opts) }, }) @@ -125,16 +201,9 @@ func NewOperator() *cobra.Command { // 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) - // Setup all Controllers log.Info("setting up controller") - kubeconfigCommandLinePath := cmd.PersistentFlags().Lookup("kubeconfig").Value.String() - if err := controller.AddToManager(mgr, kubeconfigCommandLinePath, awsSecurityTokenServiveGateEnaled); err != nil { + if err := controller.AddToManager(mgr, kubeconfigCommandLinePath, coreClient, awsSecurityTokenServiceGateEnabled); err != nil { log.WithError(err).Fatal("unable to register controllers to the manager") } @@ -230,6 +299,15 @@ func NewOperator() *cobra.Command { return cmd } +// isMissingSecretLabel determines if the secret was created by the CCO but has not been labelled yet +func isMissingSecretLabel(secret corev1.Secret) bool { + _, hasAnnotation := secret.GetAnnotations()[minterv1.AnnotationCredentialsRequest] + value, hasLabel := secret.GetLabels()[minterv1.LabelCredentialsRequest] + hasValue := hasLabel && value == minterv1.LabelCredentialsRequestValue + + return hasAnnotation && (!hasLabel || !hasValue) +} + 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..1d28d02b41 100644 --- a/pkg/gcp/actuator/actuator.go +++ b/pkg/gcp/actuator/actuator.go @@ -289,7 +289,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 +466,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 @@ -775,14 +775,14 @@ 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) + 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.Info("no existing secret found, will create one") @@ -791,6 +791,9 @@ func (a *Actuator) syncSecret(cr *minterv1.CredentialsRequest, privateKeyData [] ObjectMeta: metav1.ObjectMeta{ Name: cr.Spec.SecretRef.Name, Namespace: cr.Spec.SecretRef.Namespace, + Labels: map[string]string{ + minterv1.LabelCredentialsRequest: minterv1.LabelCredentialsRequestValue, + }, Annotations: map[string]string{ minterv1.AnnotationCredentialsRequest: fmt.Sprintf("%s/%s", cr.Namespace, cr.Name), }, @@ -800,7 +803,7 @@ func (a *Actuator) syncSecret(cr *minterv1.CredentialsRequest, privateKeyData [] }, } - err := a.Client.Create(context.TODO(), secret) + err := a.Client.Create(ctx, secret) if err != nil { sLog.WithError(err).Error("error creating secret") return err @@ -816,6 +819,11 @@ func (a *Actuator) syncSecret(cr *minterv1.CredentialsRequest, privateKeyData [] sLog.Info("updating existing secret") origSecret := existingSecret.DeepCopy() + if existingSecret.Labels == nil { + existingSecret.Labels = map[string]string{} + } + existingSecret.Labels[minterv1.LabelCredentialsRequest] = minterv1.LabelCredentialsRequestValue + if existingSecret.Annotations == nil { existingSecret.Annotations = map[string]string{} } @@ -824,7 +832,7 @@ func (a *Actuator) syncSecret(cr *minterv1.CredentialsRequest, privateKeyData [] if !reflect.DeepEqual(existingSecret, origSecret) { sLog.Info("secret changed, updating") - err := a.Client.Update(context.TODO(), existingSecret) + err := a.Client.Update(ctx, existingSecret) if err != nil { return fmt.Errorf("error updating existing secret: %v", err) } diff --git a/pkg/kubevirt/actuator.go b/pkg/kubevirt/actuator.go index eedb94804c..480e52e105 100644 --- a/pkg/kubevirt/actuator.go +++ b/pkg/kubevirt/actuator.go @@ -191,6 +191,11 @@ func (a *KubevirtActuator) updateExistingSecret(logger log.FieldLogger, existing // Update the existing secret: logger.Debug("updating secret") origSecret := existingSecret.DeepCopy() + if existingSecret.Labels == nil { + existingSecret.Labels = map[string]string{} + } + existingSecret.Labels[minterv1.LabelCredentialsRequest] = minterv1.LabelCredentialsRequestValue + if existingSecret.Annotations == nil { existingSecret.Annotations = map[string]string{} } @@ -224,6 +229,9 @@ func (a *KubevirtActuator) createNewSecret(logger log.FieldLogger, cr *minterv1. ObjectMeta: metav1.ObjectMeta{ Name: cr.Spec.SecretRef.Name, Namespace: cr.Spec.SecretRef.Namespace, + Labels: map[string]string{ + minterv1.LabelCredentialsRequest: minterv1.LabelCredentialsRequestValue, + }, Annotations: map[string]string{ minterv1.AnnotationCredentialsRequest: fmt.Sprintf("%s/%s", cr.Namespace, cr.Name), }, diff --git a/pkg/openstack/actuator.go b/pkg/openstack/actuator.go index ba98fb1fcc..6be994568a 100644 --- a/pkg/openstack/actuator.go +++ b/pkg/openstack/actuator.go @@ -67,7 +67,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") @@ -119,7 +119,7 @@ func (a *OpenStackActuator) sync(ctx context.Context, cr *minterv1.CredentialsRe return err } - err = a.syncCredentialSecret(cr, clouds, existingSecret, "", logger) + err = a.syncCredentialSecret(ctx, cr, clouds, existingSecret, "", logger) if err != nil { msg := "error creating/updating secret" logger.WithError(err).Error(msg) @@ -132,7 +132,7 @@ 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, existingSecret *corev1.Secret, 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), @@ -152,6 +152,9 @@ func (a *OpenStackActuator) syncCredentialSecret(cr *minterv1.CredentialsRequest ObjectMeta: metav1.ObjectMeta{ Name: cr.Spec.SecretRef.Name, Namespace: cr.Spec.SecretRef.Namespace, + Labels: map[string]string{ + minterv1.LabelCredentialsRequest: minterv1.LabelCredentialsRequestValue, + }, Annotations: map[string]string{ minterv1.AnnotationCredentialsRequest: fmt.Sprintf("%s/%s", cr.Namespace, cr.Name), }, @@ -159,7 +162,7 @@ func (a *OpenStackActuator) syncCredentialSecret(cr *minterv1.CredentialsRequest Data: map[string][]byte{RootOpenStackCredsSecretKey: []byte(clouds)}, } - err := a.Client.Create(context.TODO(), secret) + err := a.Client.Create(ctx, secret) if err != nil { sLog.WithError(err).Error("error creating secret") return err @@ -171,6 +174,11 @@ func (a *OpenStackActuator) syncCredentialSecret(cr *minterv1.CredentialsRequest // Update the existing secret: sLog.Debug("updating secret") origSecret := existingSecret.DeepCopy() + if existingSecret.Labels == nil { + existingSecret.Labels = map[string]string{} + } + existingSecret.Labels[minterv1.LabelCredentialsRequest] = minterv1.LabelCredentialsRequestValue + if existingSecret.Annotations == nil { existingSecret.Annotations = map[string]string{} } @@ -181,7 +189,7 @@ func (a *OpenStackActuator) syncCredentialSecret(cr *minterv1.CredentialsRequest if !reflect.DeepEqual(existingSecret, origSecret) { sLog.Info("target secret has changed, updating") - err := a.Client.Update(context.TODO(), existingSecret) + err := a.Client.Update(ctx, existingSecret) if err != nil { msg := "error updating secret" sLog.WithError(err).Error(msg) diff --git a/pkg/operator/controller.go b/pkg/operator/controller.go index e4b5e049a2..702c2c86d5 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" @@ -62,10 +62,10 @@ func init() { var AddToManagerFuncs []func(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, 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 manager.Manager, explicitKubeconfig string, coreClient corev1client.CoreV1Interface, awsSecurityTokenServiceGateEnabled bool) error { for _, f := range AddToManagerFuncs { if err := f(m, explicitKubeconfig); err != nil { return err @@ -85,7 +85,7 @@ 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(), m.GetScheme(), awsSecurityTokenServiceGateEnabled) 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, 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..e3c97a5da6 100644 --- a/pkg/operator/credentialsrequest/credentialsrequest_controller.go +++ b/pkg/operator/credentialsrequest/credentialsrequest_controller.go @@ -24,6 +24,7 @@ import ( log "github.com/sirupsen/logrus" "golang.org/x/time/rate" + corev1client "k8s.io/client-go/kubernetes/typed/core/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -54,7 +55,8 @@ import ( ) const ( - controllerName = "credreq" + controllerName = "credreq" + labelControllerName = controllerName + "_labeller" namespaceMissing = "NamespaceMissing" namespaceExists = "NamespaceExists" @@ -84,8 +86,8 @@ 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 manager.Manager, actuator actuator.Actuator, platType configv1.PlatformType, mutatingClient corev1client.CoreV1Interface) error { + return add(mgr, newReconciler(mgr, actuator, platType), mutatingClient) } // newReconciler returns a new reconcile.Reconciler @@ -101,7 +103,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 manager.Manager, r reconcile.Reconciler, mutatingClient corev1client.CoreV1Interface) error { operatorCache := mgr.GetCache() name := "credentialsrequest_controller" diff --git a/pkg/operator/platform/platform.go b/pkg/operator/platform/platform.go index 47f0ab0747..5f15700a2b 100644 --- a/pkg/operator/platform/platform.go +++ b/pkg/operator/platform/platform.go @@ -3,10 +3,11 @@ 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" + log "github.com/sirupsen/logrus" configv1 "github.com/openshift/api/config/v1" diff --git a/pkg/ovirt/actuator.go b/pkg/ovirt/actuator.go index e9442b3ff1..b23ab45718 100644 --- a/pkg/ovirt/actuator.go +++ b/pkg/ovirt/actuator.go @@ -144,7 +144,7 @@ func (a *OvirtActuator) sync(ctx context.Context, cr *minterv1.CredentialsReques return err } - err = a.syncCredentialSecret(cr, &ovirtCreds, existingSecret, logger) + err = a.syncCredentialSecret(ctx, cr, &ovirtCreds, existingSecret, logger) if err != nil { msg := "error creating/updating secret" logger.WithError(err).Error(msg) @@ -157,7 +157,7 @@ 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, existingSecret *corev1.Secret, 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), @@ -177,6 +177,9 @@ func (a *OvirtActuator) syncCredentialSecret(cr *minterv1.CredentialsRequest, ov ObjectMeta: metav1.ObjectMeta{ Name: cr.Spec.SecretRef.Name, Namespace: cr.Spec.SecretRef.Namespace, + Labels: map[string]string{ + minterv1.LabelCredentialsRequest: minterv1.LabelCredentialsRequestValue, + }, Annotations: map[string]string{ minterv1.AnnotationCredentialsRequest: fmt.Sprintf("%s/%s", cr.Namespace, cr.Name), }, @@ -184,7 +187,7 @@ func (a *OvirtActuator) syncCredentialSecret(cr *minterv1.CredentialsRequest, ov Data: secretDataFrom(ovirtCreds), } - err := a.Client.Create(context.TODO(), secret) + err := a.Client.Create(ctx, secret) if err != nil { sLog.WithError(err).Error("error creating secret") return err @@ -196,6 +199,11 @@ func (a *OvirtActuator) syncCredentialSecret(cr *minterv1.CredentialsRequest, ov // Update the existing secret: sLog.Debug("updating secret") origSecret := existingSecret.DeepCopy() + if existingSecret.Labels == nil { + existingSecret.Labels = map[string]string{} + } + existingSecret.Labels[minterv1.LabelCredentialsRequest] = minterv1.LabelCredentialsRequestValue + if existingSecret.Annotations == nil { existingSecret.Annotations = map[string]string{} } @@ -206,7 +214,7 @@ func (a *OvirtActuator) syncCredentialSecret(cr *minterv1.CredentialsRequest, ov if !reflect.DeepEqual(existingSecret, origSecret) { sLog.Info("target secret has changed, updating") - err := a.Client.Update(context.TODO(), existingSecret) + err := a.Client.Update(ctx, existingSecret) if err != nil { msg := "error updating secret" sLog.WithError(err).Error(msg) diff --git a/pkg/vsphere/actuator/actuator.go b/pkg/vsphere/actuator/actuator.go index 6758cf21af..bd57411f09 100644 --- a/pkg/vsphere/actuator/actuator.go +++ b/pkg/vsphere/actuator/actuator.go @@ -220,7 +220,7 @@ func (a *VSphereActuator) syncPassthrough(ctx context.Context, cr *minterv1.Cred return err } - err = a.syncTargetSecret(cr, cloudCredsSecret.Data, existingSecret, logger) + err = a.syncTargetSecret(ctx, cr, cloudCredsSecret.Data, existingSecret, logger) if err != nil { msg := "error creating/updating secret" logger.WithError(err).Error(msg) @@ -286,7 +286,7 @@ 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, existingSecret *corev1.Secret, 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), @@ -298,6 +298,9 @@ func (a *VSphereActuator) syncTargetSecret(cr *minterv1.CredentialsRequest, secr ObjectMeta: metav1.ObjectMeta{ Name: cr.Spec.SecretRef.Name, Namespace: cr.Spec.SecretRef.Namespace, + Labels: map[string]string{ + minterv1.LabelCredentialsRequest: minterv1.LabelCredentialsRequestValue, + }, Annotations: map[string]string{ minterv1.AnnotationCredentialsRequest: fmt.Sprintf("%s/%s", cr.Namespace, cr.Name), }, @@ -305,7 +308,7 @@ func (a *VSphereActuator) syncTargetSecret(cr *minterv1.CredentialsRequest, secr Data: secretData, } - err := a.Client.Create(context.TODO(), secret) + err := a.Client.Create(ctx, secret) if err != nil { sLog.WithError(err).Error("error creating secret") return err @@ -317,6 +320,11 @@ func (a *VSphereActuator) syncTargetSecret(cr *minterv1.CredentialsRequest, secr // Update the existing secret: sLog.Debug("updating secret") origSecret := existingSecret.DeepCopy() + if existingSecret.Labels == nil { + existingSecret.Labels = map[string]string{} + } + existingSecret.Labels[minterv1.LabelCredentialsRequest] = minterv1.LabelCredentialsRequestValue + if existingSecret.Annotations == nil { existingSecret.Annotations = map[string]string{} } @@ -326,7 +334,7 @@ func (a *VSphereActuator) syncTargetSecret(cr *minterv1.CredentialsRequest, secr if !reflect.DeepEqual(existingSecret, origSecret) { sLog.Info("target secret has changed, updating") - err := a.Client.Update(context.TODO(), existingSecret) + err := a.Client.Update(ctx, existingSecret) if err != nil { msg := "error updating secret" sLog.WithError(err).Error(msg) From e84a01c748b8836c9cb67a7ed6ae07b61db60a04 Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Thu, 29 Jun 2023 08:03:19 -0600 Subject: [PATCH 2/9] *: label existing secrets In order to reduce the cache footprint we have, we need to label the secrets we interact with so we can use a label selector on the LIST + WATCH requests we make. The previous commit labels newly-created objects, this commit adds a controller that will react to the set of secrets on the cluster today and puts labels where they are needed, so we can bring clusters without correct labels into conformance. A future release of the CCO will remove this controller and restrict the cache using our new label selector in all cases. Signed-off-by: Steve Kuznetsov --- pkg/cmd/operator/cmd.go | 12 +-- .../credentialsrequest_controller.go | 101 ++++++++++++++++++ 2 files changed, 103 insertions(+), 10 deletions(-) diff --git a/pkg/cmd/operator/cmd.go b/pkg/cmd/operator/cmd.go index 5a4853eefb..859433afd1 100644 --- a/pkg/cmd/operator/cmd.go +++ b/pkg/cmd/operator/cmd.go @@ -31,6 +31,7 @@ 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" @@ -154,7 +155,7 @@ func NewOperator() *cobra.Command { } var missing []types.NamespacedName for _, secret := range secrets.Items { - if isMissingSecretLabel(secret) { + if credentialsrequest.IsMissingSecretLabel(&secret) { missing = append(missing, types.NamespacedName{ Namespace: secret.Namespace, Name: secret.Name, @@ -299,15 +300,6 @@ func NewOperator() *cobra.Command { return cmd } -// isMissingSecretLabel determines if the secret was created by the CCO but has not been labelled yet -func isMissingSecretLabel(secret corev1.Secret) bool { - _, hasAnnotation := secret.GetAnnotations()[minterv1.AnnotationCredentialsRequest] - value, hasLabel := secret.GetLabels()[minterv1.LabelCredentialsRequest] - hasValue := hasLabel && value == minterv1.LabelCredentialsRequestValue - - return hasAnnotation && (!hasLabel || !hasValue) -} - func initializeGlog(flags *pflag.FlagSet) { golog.SetOutput(glogWriter{}) // Redirect all regular go log output to glog golog.SetFlags(0) diff --git a/pkg/operator/credentialsrequest/credentialsrequest_controller.go b/pkg/operator/credentialsrequest/credentialsrequest_controller.go index e3c97a5da6..d54af3f256 100644 --- a/pkg/operator/credentialsrequest/credentialsrequest_controller.go +++ b/pkg/operator/credentialsrequest/credentialsrequest_controller.go @@ -24,6 +24,7 @@ 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" @@ -315,9 +316,109 @@ func add(mgr manager.Manager, r reconcile.Reconciler, mutatingClient corev1clien return err } + labelControllerName := "credentialsrequest_labeller_controller" + labelController, err := controller.New(labelControllerName, mgr, controller.Options{ + Reconciler: &ReconcileSecretMissingLabel{ + cachedClient: mgr.GetClient(), + mutatingClient: mutatingClient, + }, + }) + 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) + }, + } + // Watch Secrets and reconcile if we see an event for an admin credential secret in kube-system. + err = labelController.Watch( + source.Kind(operatorCache, &corev1.Secret{}), + missingLabelSecretsMapFn, + missingLabelCredSecretPredicate) + if err != nil { + return err + } + return nil } +// IsMissingSecretLabel determines if the secret was createded by the CCO but has not been labelled yet +func IsMissingSecretLabel(secret metav1.Object) bool { + isAdmin := isAdminCredSecret(secret.GetNamespace(), secret.GetName()) + _, hasAnnotation := secret.GetAnnotations()[minterv1.AnnotationCredentialsRequest] + value, hasLabel := secret.GetLabels()[minterv1.LabelCredentialsRequest] + hasValue := hasLabel && value == minterv1.LabelCredentialsRequestValue + + return (isAdmin || hasAnnotation) && (!hasLabel || !hasValue) +} + +type ReconcileSecretMissingLabel struct { + cachedClient client.Client + mutatingClient corev1client.CoreV1Interface +} + +var _ reconcile.Reconciler = &ReconcileSecretMissingLabel{} + +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, requeuing") + 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 From 1dd9815b730eaa70de6b8b4b7a2759913637357b Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Thu, 29 Jun 2023 12:39:38 -0600 Subject: [PATCH 3/9] pkg/operator: expose the status of labelling When this controller is somewhere in the middle of labelling the secrets it needs to, we will publish a progressing status. Signed-off-by: Steve Kuznetsov --- .../v1/types_credentialsrequest.go | 4 +- .../credentialsrequest_controller.go | 59 ++++++++++++++++--- 2 files changed, 52 insertions(+), 11 deletions(-) diff --git a/pkg/apis/cloudcredential/v1/types_credentialsrequest.go b/pkg/apis/cloudcredential/v1/types_credentialsrequest.go index d8bad90814..dd1c19aea0 100644 --- a/pkg/apis/cloudcredential/v1/types_credentialsrequest.go +++ b/pkg/apis/cloudcredential/v1/types_credentialsrequest.go @@ -27,11 +27,11 @@ const ( // credentials in AWS before allowing the CredentialsRequest to be deleted in etcd. FinalizerDeprovision string = "cloudcredential.openshift.io/deprovision" - // LabelCredentialsRequest is to mark 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 CredentailsRequests. + // 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/operator/credentialsrequest/credentialsrequest_controller.go b/pkg/operator/credentialsrequest/credentialsrequest_controller.go index d54af3f256..51ad299316 100644 --- a/pkg/operator/credentialsrequest/credentialsrequest_controller.go +++ b/pkg/operator/credentialsrequest/credentialsrequest_controller.go @@ -88,7 +88,10 @@ var ( // 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, mutatingClient corev1client.CoreV1Interface) error { - return add(mgr, newReconciler(mgr, actuator, platType), mutatingClient) + if err := add(mgr, newReconciler(mgr, actuator, platType)); err != nil { + return err + } + return addLabelController(mgr, mutatingClient) } // newReconciler returns a new reconcile.Reconciler @@ -104,7 +107,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, mutatingClient corev1client.CoreV1Interface) error { +func add(mgr manager.Manager, r reconcile.Reconciler) error { operatorCache := mgr.GetCache() name := "credentialsrequest_controller" @@ -316,12 +319,17 @@ func add(mgr manager.Manager, r reconcile.Reconciler, mutatingClient corev1clien return err } - labelControllerName := "credentialsrequest_labeller_controller" + 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: &ReconcileSecretMissingLabel{ - cachedClient: mgr.GetClient(), - mutatingClient: mutatingClient, - }, + Reconciler: labelReconciler, }) if err != nil { return err @@ -351,12 +359,13 @@ func add(mgr manager.Manager, r reconcile.Reconciler, mutatingClient corev1clien } // Watch Secrets and reconcile if we see an event for an admin credential secret in kube-system. err = labelController.Watch( - source.Kind(operatorCache, &corev1.Secret{}), + source.Kind(mgr.GetCache(), &corev1.Secret{}), missingLabelSecretsMapFn, missingLabelCredSecretPredicate) if err != nil { return err } + status.AddHandler(labelControllerName, labelReconciler) return nil } @@ -376,7 +385,39 @@ type ReconcileSecretMissingLabel struct { mutatingClient corev1client.CoreV1Interface } -var _ reconcile.Reconciler = &ReconcileSecretMissingLabel{} +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() From 1083f6b4e157034af8cfba0aff07b1ebb190d166 Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Thu, 29 Jun 2023 14:18:27 -0600 Subject: [PATCH 4/9] *: always upsert Secrets When we use a LIST + WATCH that is filtered, we will get DELETE events when someone removes a label from a Secret that we're watching. Similarly, if the controller is down when this happens, the next re-LIST will not have the object in the returned set, thereby making our controller think that no such secret exists. We need to amend our logic here, as simply issuing a CREATE call may either succeed (if the object does not yet exist) or fail with AlreadyExist (if the object is just missing our label). Thankfully, all of the controllers already implemented this algorithm, albeit a little verbosely. This commit makes all of them use the upstream controller utilities to achieve the same result. Signed-off-by: Steve Kuznetsov --- pkg/aws/actuator/actuator.go | 167 +++++++++--------- pkg/aws/actuator/actuator_test.go | 2 +- pkg/azure/actuator.go | 78 ++++---- pkg/gcp/actuator/actuator.go | 84 +++------ pkg/kubevirt/actuator.go | 108 ++++------- pkg/openstack/actuator.go | 118 +++---------- .../credentialsrequest_controller.go | 8 +- ...dentialsrequest_controller_vsphere_test.go | 2 +- pkg/ovirt/actuator.go | 124 +++---------- pkg/vsphere/actuator/actuator.go | 87 +++------ 10 files changed, 266 insertions(+), 512 deletions(-) diff --git a/pkg/aws/actuator/actuator.go b/pkg/aws/actuator/actuator.go index 0c66d9bee3..fb3f5035bd 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" @@ -362,7 +363,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 +411,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 +525,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(ctx, 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 +710,7 @@ func (a *AWSActuator) syncMint(ctx context.Context, cr *minterv1.CredentialsRequ accessKeyString = *accessKey.AccessKeyId secretAccessKeyString = *accessKey.SecretAccessKey } - err = a.syncAccessKeySecret(ctx, 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 @@ -995,86 +1013,65 @@ func (a *AWSActuator) getLogger(cr *minterv1.CredentialsRequest) log.FieldLogger }) } -func (a *AWSActuator) syncAccessKeySecret(ctx context.Context, 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, - Labels: map[string]string{ - minterv1.LabelCredentialsRequest: minterv1.LabelCredentialsRequestValue, - }, - 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(ctx, 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.Labels == nil { - existingSecret.Labels = map[string]string{} - } - existingSecret.Labels[minterv1.LabelCredentialsRequest] = minterv1.LabelCredentialsRequestValue + // Make sure credentials config data is synced with the stored access key / secret key + secret.Data[constants.AWSSecretDataCredentialsKey] = generateAWSCredentialsConfig(accessKeyID, secretAccessKey) - 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])) - - if !reflect.DeepEqual(existingSecret, origSecret) { - sLog.Info("target secret has changed, updating") - err := a.Client.Update(ctx, 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 } diff --git a/pkg/aws/actuator/actuator_test.go b/pkg/aws/actuator/actuator_test.go index 33e1722845..70a3d3ef5f 100644 --- a/pkg/aws/actuator/actuator_test.go +++ b/pkg/aws/actuator/actuator_test.go @@ -458,7 +458,7 @@ func TestSecretFormat(t *testing.T) { cr := testCredentialsRequest() logger := a.getLogger(cr) - err := a.syncAccessKeySecret(context.Background(), 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") diff --git a/pkg/azure/actuator.go b/pkg/azure/actuator.go index b173ffcb16..6b5e1ae398 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" @@ -325,28 +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, - Labels: map[string]string{ - minterv1.LabelCredentialsRequest: minterv1.LabelCredentialsRequestValue, - }, - 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 { @@ -408,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 diff --git a/pkg/gcp/actuator/actuator.go b/pkg/gcp/actuator/actuator.go index 1d28d02b41..66883e38f2 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" @@ -780,66 +781,35 @@ func (a *Actuator) syncSecret(ctx context.Context, cr *minterv1.CredentialsReque "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(ctx, 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, - Labels: map[string]string{ - minterv1.LabelCredentialsRequest: minterv1.LabelCredentialsRequestValue, - }, - Annotations: map[string]string{ - minterv1.AnnotationCredentialsRequest: fmt.Sprintf("%s/%s", cr.Namespace, cr.Name), - }, - }, - Data: map[string][]byte{ - gcpSecretJSONKey: privateKeyData, - }, - } - - err := a.Client.Create(ctx, 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.Labels == nil { - existingSecret.Labels = map[string]string{} - } - existingSecret.Labels[minterv1.LabelCredentialsRequest] = minterv1.LabelCredentialsRequestValue - - 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(ctx, 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 } diff --git a/pkg/kubevirt/actuator.go b/pkg/kubevirt/actuator.go index 480e52e105..75b4f9c7c3 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" @@ -142,13 +142,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,87 +165,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.Labels == nil { - existingSecret.Labels = map[string]string{} - } - existingSecret.Labels[minterv1.LabelCredentialsRequest] = minterv1.LabelCredentialsRequestValue - - 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, - Labels: map[string]string{ - minterv1.LabelCredentialsRequest: minterv1.LabelCredentialsRequestValue, - }, - 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 } diff --git a/pkg/openstack/actuator.go b/pkg/openstack/actuator.go index 6be994568a..8bffa765c5 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" @@ -114,12 +114,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(ctx, 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,100 +127,43 @@ func (a *OpenStackActuator) sync(ctx context.Context, cr *minterv1.CredentialsRe return nil } -func (a *OpenStackActuator) syncCredentialSecret(ctx context.Context, 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, - Labels: map[string]string{ - minterv1.LabelCredentialsRequest: minterv1.LabelCredentialsRequestValue, - }, - 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(ctx, 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.Labels == nil { - existingSecret.Labels = map[string]string{} - } - existingSecret.Labels[minterv1.LabelCredentialsRequest] = minterv1.LabelCredentialsRequestValue - - 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(ctx, 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. diff --git a/pkg/operator/credentialsrequest/credentialsrequest_controller.go b/pkg/operator/credentialsrequest/credentialsrequest_controller.go index 51ad299316..507a74deb4 100644 --- a/pkg/operator/credentialsrequest/credentialsrequest_controller.go +++ b/pkg/operator/credentialsrequest/credentialsrequest_controller.go @@ -357,7 +357,6 @@ func addLabelController(mgr manager.Manager, mutatingClient corev1client.CoreV1I return IsMissingSecretLabel(e.Object) }, } - // Watch Secrets and reconcile if we see an event for an admin credential secret in kube-system. err = labelController.Watch( source.Kind(mgr.GetCache(), &corev1.Secret{}), missingLabelSecretsMapFn, @@ -370,14 +369,13 @@ func addLabelController(mgr manager.Manager, mutatingClient corev1client.CoreV1I return nil } -// IsMissingSecretLabel determines if the secret was createded by the CCO but has not been labelled yet +// IsMissingSecretLabel determines if the secret was created by the CCO but has not been labelled yet func IsMissingSecretLabel(secret metav1.Object) bool { - isAdmin := isAdminCredSecret(secret.GetNamespace(), secret.GetName()) _, hasAnnotation := secret.GetAnnotations()[minterv1.AnnotationCredentialsRequest] value, hasLabel := secret.GetLabels()[minterv1.LabelCredentialsRequest] hasValue := hasLabel && value == minterv1.LabelCredentialsRequestValue - return (isAdmin || hasAnnotation) && (!hasLabel || !hasValue) + return hasAnnotation && (!hasLabel || !hasValue) } type ReconcileSecretMissingLabel struct { @@ -440,7 +438,7 @@ func (r *ReconcileSecretMissingLabel) Reconcile(ctx context.Context, request rec logger.Debug("secret no longer exists") return reconcile.Result{}, nil } - logger.WithError(err).Error("error getting secret, requeuing") + logger.WithError(err).Error("error getting secret, re-queuing") return reconcile.Result{}, err } diff --git a/pkg/operator/credentialsrequest/credentialsrequest_controller_vsphere_test.go b/pkg/operator/credentialsrequest/credentialsrequest_controller_vsphere_test.go index bc0a5ab27f..5c4ef03c05 100644 --- a/pkg/operator/credentialsrequest/credentialsrequest_controller_vsphere_test.go +++ b/pkg/operator/credentialsrequest/credentialsrequest_controller_vsphere_test.go @@ -160,7 +160,7 @@ func TestCredentialsRequestVSphereReconcile(t *testing.T) { createTestNamespace(testSecretNamespace), testVSphereCredentialsRequest(t), testVSphereCredsSecretPassthrough(), - testSecret(testSecretNamespace, testSecretName, map[string][]byte{"oldkey": []byte("olddata")}), + testSecret(testSecretNamespace, testSecretName, map[string][]byte{"key1": []byte("olddata")}), }, validate: func(c client.Client, t *testing.T) { targetSecret := getCredRequestTargetSecret(c) diff --git a/pkg/ovirt/actuator.go b/pkg/ovirt/actuator.go index b23ab45718..a044752e11 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" @@ -138,13 +138,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(ctx, 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,109 +155,43 @@ func (a *OvirtActuator) sync(ctx context.Context, cr *minterv1.CredentialsReques return nil } -func (a *OvirtActuator) syncCredentialSecret(ctx context.Context, 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, - Labels: map[string]string{ - minterv1.LabelCredentialsRequest: minterv1.LabelCredentialsRequestValue, - }, - 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(ctx, 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.Labels == nil { - existingSecret.Labels = map[string]string{} - } - existingSecret.Labels[minterv1.LabelCredentialsRequest] = minterv1.LabelCredentialsRequestValue - - 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(ctx, 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. diff --git a/pkg/vsphere/actuator/actuator.go b/pkg/vsphere/actuator/actuator.go index bd57411f09..f5e24af366 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" @@ -215,12 +216,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(ctx, 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,67 +282,42 @@ func (a *VSphereActuator) getLogger(cr *minterv1.CredentialsRequest) log.FieldLo }) } -func (a *VSphereActuator) syncTargetSecret(ctx context.Context, 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, - Labels: map[string]string{ - minterv1.LabelCredentialsRequest: minterv1.LabelCredentialsRequestValue, - }, - 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(ctx, 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.Labels == nil { - existingSecret.Labels = map[string]string{} - } - existingSecret.Labels[minterv1.LabelCredentialsRequest] = minterv1.LabelCredentialsRequestValue - - 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(ctx, 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 } From 8d56106d0bc9ab6f3f2bcbe364e040d0b8501c8d Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Fri, 30 Jun 2023 07:55:49 -0600 Subject: [PATCH 5/9] pkg/operator: be more explicit in startup check We keep the TechPreview feature flag as a precondition for this code functioning, but add another logical check for the other precondition we have on a filtered watch. Signed-off-by: Steve Kuznetsov --- pkg/cmd/operator/cmd.go | 12 +++++++++++- .../credentialsrequest_controller.go | 8 ++++---- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/operator/cmd.go b/pkg/cmd/operator/cmd.go index 859433afd1..2a96411c69 100644 --- a/pkg/cmd/operator/cmd.go +++ b/pkg/cmd/operator/cmd.go @@ -154,6 +154,7 @@ func NewOperator() *cobra.Command { log.WithError(err).Fatal("failed to list secrets") } var missing []types.NamespacedName + var admin []types.NamespacedName for _, secret := range secrets.Items { if credentialsrequest.IsMissingSecretLabel(&secret) { missing = append(missing, types.NamespacedName{ @@ -161,10 +162,19 @@ func NewOperator() *cobra.Command { Name: secret.Name, }) } + if credentialsrequest.IsAdminCredSecret(secret.Namespace, secret.Name) { + admin = append(admin, types.NamespacedName{ + Namespace: secret.Namespace, + Name: secret.Name, + }) + } } if len(missing) != 0 { - log.WithField("missing", missing).Warnf("%s feature gate enabled but not all secrets labelled, falling back to caching all secrets on cluster", configv1.FeatureGateAWSSecurityTokenService) + log.WithField("missing", missing).Warn("not all secrets labelled, falling back to caching all secrets on cluster") + } else if len(admin) != 0 { + log.WithField("admin", admin).Warn("admin secrets present, falling back to caching all secrets on cluster") } else { + log.Info("filtering the set of secrets we watch") filteredWatchPossible = true } } diff --git a/pkg/operator/credentialsrequest/credentialsrequest_controller.go b/pkg/operator/credentialsrequest/credentialsrequest_controller.go index 507a74deb4..5a66c4bea7 100644 --- a/pkg/operator/credentialsrequest/credentialsrequest_controller.go +++ b/pkg/operator/credentialsrequest/credentialsrequest_controller.go @@ -211,13 +211,13 @@ 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. @@ -463,7 +463,7 @@ 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 || From 358a11f42b1812babd9bf07c27de5fe97d414ee2 Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Fri, 30 Jun 2023 10:49:46 -0600 Subject: [PATCH 6/9] *: use a separate client for root credentials If we're using a filtered LIST + WATCH for Secrets to do our work on CredentialRequests, we also need to open a second LIST + WATCH for the root credential, as this won't have the labels we add to our own secrets. Signed-off-by: Steve Kuznetsov --- pkg/aws/actuator/actuator.go | 12 +- pkg/aws/actuator/actuator_test.go | 16 +- pkg/azure/actuator.go | 12 +- pkg/azure/actuator_test.go | 84 +++------- pkg/azure/client.go | 13 +- pkg/cmd/operator/cmd.go | 153 ++++++++++++------ pkg/gcp/actuator/actuator.go | 10 +- pkg/kubevirt/actuator.go | 12 +- pkg/kubevirt/actuator_test.go | 32 ++-- pkg/openstack/actuator.go | 16 +- .../awspodidentitywebhook_controller.go | 2 +- pkg/operator/controller.go | 22 +-- .../credentialsrequest_controller.go | 14 +- ...redentialsrequest_controller_azure_test.go | 6 +- .../credentialsrequest_controller_gcp_test.go | 75 ++++++--- .../credentialsrequest_controller_test.go | 137 ++++++++++++---- ...dentialsrequest_controller_vsphere_test.go | 38 +++-- pkg/operator/platform/platform.go | 6 +- .../secretannotator_controller.go | 2 +- pkg/operator/status/status_controller.go | 2 +- pkg/ovirt/actuator.go | 16 +- pkg/vsphere/actuator/actuator.go | 16 +- 22 files changed, 438 insertions(+), 258 deletions(-) diff --git a/pkg/aws/actuator/actuator.go b/pkg/aws/actuator/actuator.go index fb3f5035bd..1ee8886758 100644 --- a/pkg/aws/actuator/actuator.go +++ b/pkg/aws/actuator/actuator.go @@ -69,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 @@ -76,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") @@ -86,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, @@ -943,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 @@ -1109,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{ @@ -1397,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 70a3d3ef5f..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) @@ -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 6b5e1ae398..42f1651994 100644 --- a/pkg/azure/actuator.go +++ b/pkg/azure/actuator.go @@ -54,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, @@ -71,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, } @@ -437,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{ @@ -506,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 2a96411c69..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" @@ -125,62 +126,45 @@ func NewOperator() *cobra.Command { 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, two caveats: - // - in passthrough and mint mode, admin credentials are also provided by the user through Secrets, - // and we need to watch those, but we can't label them - // - on existing clusters, secrets exist from previous work these reconcilers did that will not have - // Secrets labelled - // We could solve the second issue with an interim release of this controller that labels all previous - // Secrets, but does not restrict the watch stream. - // Due to the way that controller-runtime closes over the client/cache concepts, it's difficult to - // solve the first issue, though, since we'd need two sets of clients and caches, both for Secrets, - // and ensure that we use one for client access to Secrets we're creating or mutating and the other - // when we're interacting with admin credentials. Not impossible to do, but tricky to implement and - // complex. - // Until we undertake that effort, we apply a simplification to the space: only when AWS STS mode is - // enabled, we will try to filter the LIST + WATCH. This mode is brand new, so we can be reasonably - // sure that there are no previous secrets on the cluster, and, we make the filtering best-effort - // in order to check if that assumption held. Second, AWS STS mode only runs in clusters without - // admin credentials, so if we apply the filter, we should not see failures downstream from clients - // that hope to see those objects but can't. + // 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 - if awsSecurityTokenServiceGateEnabled { - 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 - var admin []types.NamespacedName - for _, secret := range secrets.Items { - if credentialsrequest.IsMissingSecretLabel(&secret) { - missing = append(missing, types.NamespacedName{ - Namespace: secret.Namespace, - Name: secret.Name, - }) - } - if credentialsrequest.IsAdminCredSecret(secret.Namespace, secret.Name) { - admin = append(admin, 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 if len(admin) != 0 { - log.WithField("admin", admin).Warn("admin secrets present, falling back to caching all secrets on cluster") - } else { - log.Info("filtering the set of secrets we watch") - filteredWatchPossible = true + 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) { @@ -194,6 +178,7 @@ func NewOperator() *cobra.Command { }), } 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, @@ -207,21 +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()) + util.SetupScheme(rootMgr.GetScheme()) // Setup all Controllers - log.Info("setting up controller") - if err := controller.AddToManager(mgr, kubeconfigCommandLinePath, coreClient, awsSecurityTokenServiceGateEnabled); 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") + } } } @@ -310,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 66883e38f2..29429f13fc 100644 --- a/pkg/gcp/actuator/actuator.go +++ b/pkg/gcp/actuator/actuator.go @@ -61,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) } @@ -70,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") @@ -80,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 @@ -649,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 } @@ -687,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{ @@ -869,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 75b4f9c7c3..c7e29df342 100644 --- a/pkg/kubevirt/actuator.go +++ b/pkg/kubevirt/actuator.go @@ -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{ @@ -235,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 8bffa765c5..53909f3663 100644 --- a/pkg/openstack/actuator.go +++ b/pkg/openstack/actuator.go @@ -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 } @@ -174,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{ @@ -219,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..8af38a14cd 100644 --- a/pkg/operator/awspodidentity/awspodidentitywebhook_controller.go +++ b/pkg/operator/awspodidentity/awspodidentitywebhook_controller.go @@ -129,7 +129,7 @@ func (c *awsPodIdentityController) Start(ctx context.Context) error { } func Add(mgr manager.Manager, kubeconfig string) error { - infraStatus, err := platform.GetInfraStatusUsingKubeconfig(mgr, kubeconfig) + infraStatus, err := platform.GetInfraStatusUsingKubeconfig(kubeconfig) if err != nil { return err } diff --git a/pkg/operator/controller.go b/pkg/operator/controller.go index 702c2c86d5..9eb037b5ce 100644 --- a/pkg/operator/controller.go +++ b/pkg/operator/controller.go @@ -62,10 +62,10 @@ func init() { var AddToManagerFuncs []func(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, corev1client.CoreV1Interface) 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, coreClient corev1client.CoreV1Interface, awsSecurityTokenServiceGateEnabled 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 { return err @@ -77,7 +77,7 @@ func AddToManager(m manager.Manager, explicitKubeconfig string, coreClient corev // 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, coreClient corev switch platformType { case configv1.AWSPlatformType: log.Info("initializing AWS actuator") - a, err = awsactuator.NewAWSActuator(m.GetClient(), m.GetScheme(), awsSecurityTokenServiceGateEnabled) + 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, coreClient corev 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, coreClient corev 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, coreClient corev log.Info("initializing no-op actuator (unsupported platform)") a = &actuator.DummyActuator{} } - if err := f(m, a, platformType, coreClient); 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 5a66c4bea7..16e6d0b840 100644 --- a/pkg/operator/credentialsrequest/credentialsrequest_controller.go +++ b/pkg/operator/credentialsrequest/credentialsrequest_controller.go @@ -87,17 +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, mutatingClient corev1client.CoreV1Interface) error { - if err := add(mgr, newReconciler(mgr, actuator, platType)); err != nil { +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, } @@ -107,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" @@ -222,7 +223,7 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error { } // 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 { @@ -484,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 } @@ -523,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_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 5c4ef03c05..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,9 +167,11 @@ func TestCredentialsRequestVSphereReconcile(t *testing.T) { testOperatorConfig(""), createTestNamespace(testSecretNamespace), testVSphereCredentialsRequest(t), - testVSphereCredsSecretPassthrough(), testSecret(testSecretNamespace, testSecretName, map[string][]byte{"key1": []byte("olddata")}), }, + 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") @@ -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/platform/platform.go b/pkg/operator/platform/platform.go index 5f15700a2b..21e645b823 100644 --- a/pkg/operator/platform/platform.go +++ b/pkg/operator/platform/platform.go @@ -7,6 +7,7 @@ import ( "time" "github.com/openshift/cloud-credential-operator/pkg/operator/constants" + "github.com/openshift/cloud-credential-operator/pkg/util" log "github.com/sirupsen/logrus" @@ -21,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 @@ -71,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..ffd47f57fc 100644 --- a/pkg/operator/secretannotator/secretannotator_controller.go +++ b/pkg/operator/secretannotator/secretannotator_controller.go @@ -30,7 +30,7 @@ import ( ) func Add(mgr 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/operator/status/status_controller.go b/pkg/operator/status/status_controller.go index ac77bfbfa6..3e17cd5644 100644 --- a/pkg/operator/status/status_controller.go +++ b/pkg/operator/status/status_controller.go @@ -90,7 +90,7 @@ 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 { - 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 a044752e11..3aaa29dd39 100644 --- a/pkg/ovirt/actuator.go +++ b/pkg/ovirt/actuator.go @@ -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 } @@ -202,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{ @@ -307,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 f5e24af366..d8047c52c3 100644 --- a/pkg/vsphere/actuator/actuator.go +++ b/pkg/vsphere/actuator/actuator.go @@ -44,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 { @@ -53,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") @@ -61,8 +62,9 @@ func NewVSphereActuator(client client.Client) (*VSphereActuator, error) { } return &VSphereActuator{ - Codec: codec, - Client: client, + Codec: codec, + Client: client, + RootCredClient: rootCredClient, }, nil } @@ -329,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{ @@ -383,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()) } From ae2828fa39671e55b21e62acb72af279690f8130 Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Mon, 3 Jul 2023 12:45:01 -0600 Subject: [PATCH 7/9] manifests: add more static related objects Signed-off-by: Steve Kuznetsov --- manifests/04-cluster-operator.yaml | 39 ++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) 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 From caf857fde38771d5aa153eaf002dfffcf1e89696 Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Wed, 5 Jul 2023 11:50:24 -0600 Subject: [PATCH 8/9] pkg/operator: use cloud cred manager for annotation controller Signed-off-by: Steve Kuznetsov --- .../awspodidentitywebhook_controller.go | 2 +- pkg/operator/cleanup/cleanup_controller.go | 2 +- pkg/operator/controller.go | 4 ++-- pkg/operator/loglevel/controller.go | 2 +- pkg/operator/metrics/metrics.go | 2 +- .../secretannotator/secretannotator_controller.go | 14 +++++++------- pkg/operator/status/status_controller.go | 2 +- 7 files changed, 14 insertions(+), 14 deletions(-) diff --git a/pkg/operator/awspodidentity/awspodidentitywebhook_controller.go b/pkg/operator/awspodidentity/awspodidentitywebhook_controller.go index 8af38a14cd..3eead6ce94 100644 --- a/pkg/operator/awspodidentity/awspodidentitywebhook_controller.go +++ b/pkg/operator/awspodidentity/awspodidentitywebhook_controller.go @@ -128,7 +128,7 @@ func (c *awsPodIdentityController) Start(ctx context.Context) error { return nil } -func Add(mgr manager.Manager, kubeconfig string) error { +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 9eb037b5ce..f7c1807556 100644 --- a/pkg/operator/controller.go +++ b/pkg/operator/controller.go @@ -59,7 +59,7 @@ 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, manager.Manager, actuator.Actuator, configv1.PlatformType, corev1client.CoreV1Interface) error @@ -67,7 +67,7 @@ var AddToManagerWithActuatorFuncs []func(manager.Manager, manager.Manager, actua // AddToManager adds all Controllers to the Manager 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 } } 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/secretannotator/secretannotator_controller.go b/pkg/operator/secretannotator/secretannotator_controller.go index ffd47f57fc..dff44841e3 100644 --- a/pkg/operator/secretannotator/secretannotator_controller.go +++ b/pkg/operator/secretannotator/secretannotator_controller.go @@ -29,7 +29,7 @@ import ( log "github.com/sirupsen/logrus" ) -func Add(mgr manager.Manager, kubeconfig string) error { +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 3e17cd5644..85d107f623 100644 --- a/pkg/operator/status/status_controller.go +++ b/pkg/operator/status/status_controller.go @@ -88,7 +88,7 @@ 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(kubeConfig) if err != nil { From c0b4a13c46bc2a005957a8e7742d9155214fffb3 Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Mon, 10 Jul 2023 07:35:52 -0600 Subject: [PATCH 9/9] pkg/operator: add unit tests for label reconciler Signed-off-by: Steve Kuznetsov --- go.mod | 2 +- .../credentialsrequest_controller.go | 2 +- ...redentialsrequest_controller_label_test.go | 260 ++++++++++++++++++ 3 files changed, 262 insertions(+), 2 deletions(-) create mode 100644 pkg/operator/credentialsrequest/credentialsrequest_controller_label_test.go 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/pkg/operator/credentialsrequest/credentialsrequest_controller.go b/pkg/operator/credentialsrequest/credentialsrequest_controller.go index 16e6d0b840..e54fa55df5 100644 --- a/pkg/operator/credentialsrequest/credentialsrequest_controller.go +++ b/pkg/operator/credentialsrequest/credentialsrequest_controller.go @@ -381,7 +381,7 @@ func IsMissingSecretLabel(secret metav1.Object) bool { type ReconcileSecretMissingLabel struct { cachedClient client.Client - mutatingClient corev1client.CoreV1Interface + mutatingClient corev1client.SecretsGetter } func (r *ReconcileSecretMissingLabel) GetConditions(logger log.FieldLogger) ([]configv1.ClusterOperatorStatusCondition, error) { 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) + } + }) + } +}