Skip to content

Commit

Permalink
Changes to address PR comments from Steve ~3d ago
Browse files Browse the repository at this point in the history
  • Loading branch information
@bentito
bentito committed Jun 26, 2023
1 parent f38b079 commit 7d35d34
Show file tree
Hide file tree
Showing 7 changed files with 54 additions and 57 deletions.
14 changes: 5 additions & 9 deletions pkg/aws/actuator/actuator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -339,17 +339,14 @@ 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
}
}
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 {
Expand All @@ -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
}
Expand Down Expand Up @@ -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{
Expand All @@ -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
Expand Down
6 changes: 1 addition & 5 deletions pkg/aws/actuator/actuator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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, "")
Expand All @@ -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, "")
Expand All @@ -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")
Expand Down Expand Up @@ -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))
})
}
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/cmd/operator/cmd.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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")
Expand Down
65 changes: 37 additions & 28 deletions pkg/operator/credentialsrequest/credentialsrequest_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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")
Expand All @@ -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")
Expand Down Expand Up @@ -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")

Expand All @@ -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),
})
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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")
}
Expand All @@ -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
Expand All @@ -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 {
Expand All @@ -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) {
Expand Down Expand Up @@ -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")
}
Expand All @@ -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
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down
3 changes: 1 addition & 2 deletions pkg/operator/platform/platform.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
13 changes: 5 additions & 8 deletions pkg/operator/utils/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions pkg/ovirt/actuator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down

0 comments on commit 7d35d34

Please sign in to comment.