From 7d35d34be58c4c33da2699a39fdea3cd18dc97d7 Mon Sep 17 00:00:00 2001 From: Brett Tofel Date: Mon, 26 Jun 2023 18:38:47 +0200 Subject: [PATCH] Changes to address PR comments from Steve ~3d ago --- pkg/aws/actuator/actuator.go | 14 ++-- pkg/aws/actuator/actuator_test.go | 6 +- pkg/cmd/operator/cmd.go | 6 +- .../credentialsrequest_controller.go | 65 +++++++++++-------- pkg/operator/platform/platform.go | 3 +- pkg/operator/utils/utils.go | 13 ++-- pkg/ovirt/actuator.go | 4 +- 7 files changed, 54 insertions(+), 57 deletions(-) diff --git a/pkg/aws/actuator/actuator.go b/pkg/aws/actuator/actuator.go index b66ef9fec..92474bfee 100644 --- a/pkg/aws/actuator/actuator.go +++ b/pkg/aws/actuator/actuator.go @@ -339,7 +339,7 @@ func (a *AWSActuator) sync(ctx context.Context, cr *minterv1.CredentialsRequest) stsDetected := false stsFeatureGateEnabled := a.STSFeatureGateEnabled() if stsFeatureGateEnabled { - stsDetected, err = utils.IsTimedTokenCluster(a.Client, logger) + stsDetected, err = utils.IsTimedTokenCluster(a.Client, ctx, logger) if err != nil { return err } @@ -347,9 +347,6 @@ func (a *AWSActuator) sync(ctx context.Context, cr *minterv1.CredentialsRequest) logger.Infof("stsFeatureGateEnabled: %v", stsFeatureGateEnabled) logger.Infof("stsDetected: %v", stsDetected) if stsFeatureGateEnabled && stsDetected { - if a.Codec == nil { - return fmt.Errorf("invalid codec, nil value") - } logger.Debug("actuator detected STS enabled cluster, enabling STS secret brokering for CredentialsRequests providing an IAM Role ARN") awsSTSIAMRoleARN, err := awsSTSIAMRoleARN(a.Codec, cr) if err != nil { @@ -365,7 +362,7 @@ func (a *AWSActuator) sync(ctx context.Context, cr *minterv1.CredentialsRequest) cloudTokenPath = "/var/run/secrets/kubernetes.io/serviceaccount/token" } if awsSTSIAMRoleARN != "" { - err = a.createSecret(awsSTSIAMRoleARN, cloudTokenPath, cr.Spec.SecretRef.Name, cr.Spec.SecretRef.Namespace, logger) + err = a.createSTSSecret(awsSTSIAMRoleARN, cloudTokenPath, cr.Spec.SecretRef.Name, cr.Spec.SecretRef.Namespace, logger, ctx) if err != nil { return err } @@ -413,14 +410,13 @@ func (a *AWSActuator) sync(ctx context.Context, cr *minterv1.CredentialsRequest) return nil } -// createSecret makes a time-based token available in a Secret in the namespace of an operator that +// createSTSSecret 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) createSecret(awsSTSIAMRoleARN string, cloudTokenPath string, secretRef string, - secretRefNamespace string, log log.FieldLogger) error { +func (a *AWSActuator) createSTSSecret(awsSTSIAMRoleARN string, cloudTokenPath string, secretRef string, secretRefNamespace string, log log.FieldLogger, ctx context.Context) error { log.Infof("creating secret") secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -432,7 +428,7 @@ func (a *AWSActuator) createSecret(awsSTSIAMRoleARN string, cloudTokenPath strin }, Type: corev1.SecretTypeOpaque, } - err := a.Client.Create(context.TODO(), secret) + err := a.Client.Create(ctx, secret) if err != nil { log.Errorf("error creating secret") return err diff --git a/pkg/aws/actuator/actuator_test.go b/pkg/aws/actuator/actuator_test.go index 457b89151..a5e903bdc 100644 --- a/pkg/aws/actuator/actuator_test.go +++ b/pkg/aws/actuator/actuator_test.go @@ -586,7 +586,6 @@ func TestDetectSTS(t *testing.T) { name string existing []runtime.Object wantErr assert.ErrorAssertionFunc - ctx context.Context CredentialsRequest *minterv1.CredentialsRequest issuer string stsEnabled bool @@ -597,7 +596,6 @@ func TestDetectSTS(t *testing.T) { testInfrastructure(), testOperatorConfig(operatorv1.CloudCredentialsModeManual), }, - ctx: context.TODO(), CredentialsRequest: func() *minterv1.CredentialsRequest { cr := testCredentialsRequest() cr.Spec.ProviderSpec, err = testAWSProviderConfig(codec, "") @@ -616,7 +614,6 @@ func TestDetectSTS(t *testing.T) { testInfrastructure(), testOperatorConfig(operatorv1.CloudCredentialsModeManual), }, - ctx: context.TODO(), CredentialsRequest: func() *minterv1.CredentialsRequest { cr := testCredentialsRequest() cr.Spec.ProviderSpec, err = testAWSProviderConfig(codec, "") @@ -636,7 +633,6 @@ func TestDetectSTS(t *testing.T) { testInfrastructure(), testOperatorConfig(operatorv1.CloudCredentialsModeManual), }, - ctx: context.TODO(), CredentialsRequest: func() *minterv1.CredentialsRequest { cr := testCredentialsRequest() cr.Spec.ProviderSpec, err = testAWSProviderConfig(codec, "cloud-token") @@ -666,7 +662,7 @@ func TestDetectSTS(t *testing.T) { Codec: codec, AWSSecurityTokenServiveGateEnaled: test.stsEnabled, } - test.wantErr(t, a.sync(test.ctx, test.CredentialsRequest), fmt.Sprintf("sync(%v, %v)", test.ctx, test.CredentialsRequest)) + test.wantErr(t, a.sync(context.Background(), test.CredentialsRequest), fmt.Sprintf("sync(%v)", test.CredentialsRequest)) }) } } diff --git a/pkg/cmd/operator/cmd.go b/pkg/cmd/operator/cmd.go index e48b0a011..78c644274 100644 --- a/pkg/cmd/operator/cmd.go +++ b/pkg/cmd/operator/cmd.go @@ -40,7 +40,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" - v1 "github.com/openshift/api/config/v1" + configv1 "github.com/openshift/api/config/v1" minterv1 "github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential/v1" controller "github.com/openshift/cloud-credential-operator/pkg/operator" "github.com/openshift/cloud-credential-operator/pkg/operator/platform" @@ -125,11 +125,11 @@ func NewOperator() *cobra.Command { // Setup Scheme for all resources util.SetupScheme(mgr.GetScheme()) - featureGates, err := platform.GetFeatureGates() + featureGates, err := platform.GetFeatureGates(ctx) if err != nil { log.WithError(err).Fatal("unable to read feature gates") } - awsSecurityTokenServiveGateEnaled := featureGates.Enabled(v1.FeatureGateAWSSecurityTokenService) + awsSecurityTokenServiveGateEnaled := featureGates.Enabled(configv1.FeatureGateAWSSecurityTokenService) // Setup all Controllers log.Info("setting up controller") diff --git a/pkg/operator/credentialsrequest/credentialsrequest_controller.go b/pkg/operator/credentialsrequest/credentialsrequest_controller.go index 4d415ec24..a4d63c7f6 100644 --- a/pkg/operator/credentialsrequest/credentialsrequest_controller.go +++ b/pkg/operator/credentialsrequest/credentialsrequest_controller.go @@ -186,7 +186,7 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error { allCredRequestsMapFn := handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, a client.Object) []reconcile.Request { log.Info("requeueing all CredentialsRequests") crs := &minterv1.CredentialsRequestList{} - err := mgr.GetClient().List(context.TODO(), crs) + err := mgr.GetClient().List(ctx, crs) var requests []reconcile.Request if err != nil { log.WithError(err).Error("error listing all cred requests for requeue") @@ -235,7 +235,7 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error { log.WithField("namespace", newNamespace).Debug("checking for credentails requests targeting namespace") crs := &minterv1.CredentialsRequestList{} // Fixme: check for errors - mgr.GetClient().List(context.TODO(), crs) + mgr.GetClient().List(ctx, crs) requests := []reconcile.Request{} for _, cr := range crs.Items { if !cr.Status.Provisioned && cr.Spec.SecretRef.Namespace == newNamespace { @@ -375,7 +375,7 @@ func (r *ReconcileCredentialsRequest) Reconcile(ctx context.Context, request rec stsFeatureGateEnabled := r.Actuator.STSFeatureGateEnabled() if stsFeatureGateEnabled { - stsDetected, _ = utils.IsTimedTokenCluster(r.Client, logger) + stsDetected, _ = utils.IsTimedTokenCluster(r.Client, ctx, logger) } if err != nil { logger.WithError(err).Error("error checking if operator is disabled") @@ -394,7 +394,7 @@ func (r *ReconcileCredentialsRequest) Reconcile(ctx context.Context, request rec logger.Info("syncing credentials request") cr := &minterv1.CredentialsRequest{} - err = r.Get(context.TODO(), request.NamespacedName, cr) + err = r.Get(ctx, request.NamespacedName, cr) if err != nil { if errors.IsNotFound(err) { logger.Debug("credentials request no longer exists") @@ -429,7 +429,7 @@ func (r *ReconcileCredentialsRequest) Reconcile(ctx context.Context, request rec // Handle deletion and the deprovision finalizer: if cr.DeletionTimestamp != nil { if HasFinalizer(cr, minterv1.FinalizerDeprovision) { - err = r.Actuator.Delete(context.TODO(), cr) + err = r.Actuator.Delete(ctx, cr) if err != nil { logger.WithError(err).Error("actuator error deleting credentials exist") @@ -450,7 +450,7 @@ func (r *ReconcileCredentialsRequest) Reconcile(ctx context.Context, request rec // Delete the target secret if it exists: targetSecret := &corev1.Secret{} - err := r.Client.Get(context.TODO(), types.NamespacedName{Namespace: cr.Spec.SecretRef.Namespace, Name: cr.Spec.SecretRef.Name}, targetSecret) + err := r.Client.Get(ctx, types.NamespacedName{Namespace: cr.Spec.SecretRef.Namespace, Name: cr.Spec.SecretRef.Name}, targetSecret) sLog := logger.WithFields(log.Fields{ "targetSecret": fmt.Sprintf("%s/%s", cr.Spec.SecretRef.Namespace, cr.Spec.SecretRef.Name), }) @@ -462,7 +462,7 @@ func (r *ReconcileCredentialsRequest) Reconcile(ctx context.Context, request rec return reconcile.Result{}, err } } else { - err := r.Client.Delete(context.TODO(), targetSecret) + err := r.Client.Delete(ctx, targetSecret) if err != nil { sLog.WithError(err).Error("error deleting target secret") return reconcile.Result{}, err @@ -472,7 +472,7 @@ func (r *ReconcileCredentialsRequest) Reconcile(ctx context.Context, request rec } logger.Info("actuator deletion complete, removing finalizer") - err = r.removeDeprovisionFinalizer(cr) + err = r.removeDeprovisionFinalizer(ctx, cr) if err != nil { logger.WithError(err).Error("error removing deprovision finalizer") return reconcile.Result{}, err @@ -486,7 +486,7 @@ func (r *ReconcileCredentialsRequest) Reconcile(ctx context.Context, request rec if !HasFinalizer(cr, minterv1.FinalizerDeprovision) { // Ensure the finalizer is set on any not-deleted requests: logger.Infof("adding finalizer: %s", minterv1.FinalizerDeprovision) - err = r.addDeprovisionFinalizer(cr) + err = r.addDeprovisionFinalizer(ctx, cr) if err != nil { logger.WithError(err).Error("error adding finalizer") } @@ -497,7 +497,7 @@ func (r *ReconcileCredentialsRequest) Reconcile(ctx context.Context, request rec // Ensure the target namespace exists for the secret, if not, there's no point // continuing: targetNS := &corev1.Namespace{} - err = r.Get(context.TODO(), types.NamespacedName{Name: cr.Spec.SecretRef.Namespace}, targetNS) + err = r.Get(ctx, types.NamespacedName{Name: cr.Spec.SecretRef.Namespace}, targetNS) if err != nil { if errors.IsNotFound(err) { // TODO: technically we should deprovision if a credential was in this ns, but it @@ -523,7 +523,7 @@ func (r *ReconcileCredentialsRequest) Reconcile(ctx context.Context, request rec var crSecretExists bool crSecret := &corev1.Secret{} secretKey := types.NamespacedName{Name: cr.Spec.SecretRef.Name, Namespace: cr.Spec.SecretRef.Namespace} - if err := r.Get(context.TODO(), secretKey, crSecret); err != nil { + if err := r.Get(ctx, secretKey, crSecret); err != nil { if errors.IsNotFound(err) { crSecretExists = false } else { @@ -537,18 +537,14 @@ func (r *ReconcileCredentialsRequest) Reconcile(ctx context.Context, request rec // create time-based tokens based on settings in CredentialsRequests logger.Debugf("timed token access cluster detected: %t, so not trying to provision with root secret", stsDetected) - credsExists, err := r.Actuator.Exists(context.TODO(), cr) + credsExists, err := r.Actuator.Exists(ctx, cr) if err != nil { logger.Errorf("error checking whether credentials already exists: %v", err) return reconcile.Result{}, err } var syncErr error - if !credsExists { - syncErr = r.Actuator.Create(context.TODO(), cr) - } else { - syncErr = r.Actuator.Update(context.TODO(), cr) - } + syncErr = r.CreateOrUpdateOnCredsExist(ctx, credsExists, syncErr, cr) var provisionErr bool if syncErr != nil { switch t := syncErr.(type) { @@ -577,7 +573,7 @@ func (r *ReconcileCredentialsRequest) Reconcile(ctx context.Context, request rec cr.Status.Provisioned = true } } else { - credentialsRootSecret, err := r.Actuator.GetCredentialsRootSecret(context.TODO(), cr) + credentialsRootSecret, err := r.Actuator.GetCredentialsRootSecret(ctx, cr) if err != nil { log.WithError(err).Debug("error retrieving cloud credentials secret, admin can remove root credentials in mint mode") } @@ -586,6 +582,14 @@ func (r *ReconcileCredentialsRequest) Reconcile(ctx context.Context, request rec hasRecentlySynced := cr.Status.LastSyncTimestamp != nil && cr.Status.LastSyncTimestamp.Add(syncPeriod).After(time.Now()) hasActiveFailureConditions := checkForFailureConditions(cr) + log.WithFields(log.Fields{ + "cloudCredsSecretUpdated": cloudCredsSecretUpdated, + "NOT isStale": isStale, + "hasRecentlySynced": hasRecentlySynced, + "crSecretExists": crSecretExists, + "NOT hasActiveFailureConditions": hasActiveFailureConditions, + "cr.Status.Provisioned": cr.Status.Provisioned, + }).Debug("The above are ANDed together to determine: lastsyncgeneration is current and lastsynctimestamp < an hour ago") if !cloudCredsSecretUpdated && !isStale && hasRecentlySynced && crSecretExists && !hasActiveFailureConditions && cr.Status.Provisioned { logger.Debug("lastsyncgeneration is current and lastsynctimestamp was less than an hour ago, so no need to sync") // Since we get no events for changes made directly to the cloud/platform, set the requeueAfter so that we at @@ -594,18 +598,14 @@ func (r *ReconcileCredentialsRequest) Reconcile(ctx context.Context, request rec return reconcile.Result{RequeueAfter: defaultRequeueTime}, nil } - credsExists, err := r.Actuator.Exists(context.TODO(), cr) + credsExists, err := r.Actuator.Exists(ctx, cr) if err != nil { logger.Errorf("error checking whether credentials already exists: %v", err) return reconcile.Result{}, err } var syncErr error - if !credsExists { - syncErr = r.Actuator.Create(context.TODO(), cr) - } else { - syncErr = r.Actuator.Update(context.TODO(), cr) - } + syncErr = r.CreateOrUpdateOnCredsExist(ctx, credsExists, syncErr, cr) var provisionErr bool if syncErr != nil { @@ -668,6 +668,15 @@ func (r *ReconcileCredentialsRequest) Reconcile(ctx context.Context, request rec return reconcile.Result{RequeueAfter: defaultRequeueTime}, nil } +func (r *ReconcileCredentialsRequest) CreateOrUpdateOnCredsExist(ctx context.Context, credsExists bool, syncErr error, cr *minterv1.CredentialsRequest) error { + if !credsExists { + syncErr = r.Actuator.Create(ctx, cr) + } else { + syncErr = r.Actuator.Update(ctx, cr) + } + return syncErr +} + func (r *ReconcileCredentialsRequest) updateActuatorConditions(cr *minterv1.CredentialsRequest, reason minterv1.CredentialsRequestConditionType, conditionError error) { if reason == minterv1.CredentialsProvisionFailure { @@ -817,14 +826,14 @@ func setIgnoredCondition(cr *minterv1.CredentialsRequest, clusterPlatform config } } -func (r *ReconcileCredentialsRequest) addDeprovisionFinalizer(cr *minterv1.CredentialsRequest) error { +func (r *ReconcileCredentialsRequest) addDeprovisionFinalizer(ctx context.Context, cr *minterv1.CredentialsRequest) error { AddFinalizer(cr, minterv1.FinalizerDeprovision) - return r.Update(context.TODO(), cr) + return r.Update(ctx, cr) } -func (r *ReconcileCredentialsRequest) removeDeprovisionFinalizer(cr *minterv1.CredentialsRequest) error { +func (r *ReconcileCredentialsRequest) removeDeprovisionFinalizer(ctx context.Context, cr *minterv1.CredentialsRequest) error { DeleteFinalizer(cr, minterv1.FinalizerDeprovision) - return r.Update(context.TODO(), cr) + return r.Update(ctx, cr) } // HasFinalizer returns true if the given object has the given finalizer diff --git a/pkg/operator/platform/platform.go b/pkg/operator/platform/platform.go index c3d73b7a4..d9144dfef 100644 --- a/pkg/operator/platform/platform.go +++ b/pkg/operator/platform/platform.go @@ -72,9 +72,8 @@ func getClient(explicitKubeconfig string) (client.Client, error) { return dynamicClient, nil } -func GetFeatureGates() (featuregates.FeatureGate, error) { +func GetFeatureGates(ctx context.Context) (featuregates.FeatureGate, error) { stop := make(chan struct{}) - ctx := context.Background() ctx, cancelFn := context.WithCancel(ctx) go func() { defer cancelFn() diff --git a/pkg/operator/utils/utils.go b/pkg/operator/utils/utils.go index 082d27c46..53a392d4c 100644 --- a/pkg/operator/utils/utils.go +++ b/pkg/operator/utils/utils.go @@ -78,7 +78,7 @@ func LoadInfrastructureTopology(c client.Client, logger log.FieldLogger) (config // 2. Is serviceAccountIssuer non-empty // // Both of these conditions must be true for any timed access token enabled clusters for the implementations mentioned above. -func IsTimedTokenCluster(c client.Client, logger log.FieldLogger) (bool, error) { +func IsTimedTokenCluster(c client.Client, ctx context.Context, logger log.FieldLogger) (bool, error) { credentialsMode, _, err := GetOperatorConfiguration(c, logger) if err != nil { logger.WithError(err).Error("error loading CCO configuration to determine mode") @@ -87,15 +87,12 @@ func IsTimedTokenCluster(c client.Client, logger log.FieldLogger) (bool, error) if credentialsMode != "Manual" { return false, nil } - authConfig, err := GetAuth(c) + authConfig, err := GetAuth(ctx, c) if err != nil { logger.WithError(err).Error("error loading authentication config") return false, err } - if authConfig.Spec.ServiceAccountIssuer == "" { - return false, nil - } - return true, nil + return authConfig.Spec.ServiceAccountIssuer != "", nil } // LoadInfrastructureName loads the cluster Infrastructure config and returns the infra name @@ -138,9 +135,9 @@ func GetInfrastructure(c client.Client) (*configv1.Infrastructure, error) { return infra, nil } -func GetAuth(c client.Client) (*configv1.Authentication, error) { +func GetAuth(ctx context.Context, c client.Client) (*configv1.Authentication, error) { auth := &configv1.Authentication{} - if err := c.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, auth); err != nil { + if err := c.Get(ctx, types.NamespacedName{Name: "cluster"}, auth); err != nil { return nil, err } return auth, nil diff --git a/pkg/ovirt/actuator.go b/pkg/ovirt/actuator.go index 4af8c5fab..e9442b3ff 100644 --- a/pkg/ovirt/actuator.go +++ b/pkg/ovirt/actuator.go @@ -53,8 +53,8 @@ type OvirtActuator struct { Codec *minterv1.ProviderCodec } -func (a *OvirtActuator) GetFeatureGates() (featuregates.FeatureGate, error) { - featureGates, err := platform.GetFeatureGates() +func (a *OvirtActuator) GetFeatureGates(ctx context.Context) (featuregates.FeatureGate, error) { + featureGates, err := platform.GetFeatureGates(nil) if err != nil { log.Fatal(err) }