diff --git a/pkg/aws/actuator/actuator.go b/pkg/aws/actuator/actuator.go index 2cd2736ff..6552af706 100644 --- a/pkg/aws/actuator/actuator.go +++ b/pkg/aws/actuator/actuator.go @@ -60,10 +60,11 @@ var _ actuatoriface.Actuator = (*AWSActuator)(nil) // AWSActuator implements the CredentialsRequest Actuator interface to create credentials in AWS. type AWSActuator struct { - Client client.Client - Codec *minterv1.ProviderCodec - AWSClientBuilder func(accessKeyID, secretAccessKey []byte, c client.Client) (ccaws.Client, error) - Scheme *runtime.Scheme + Client client.Client + Codec *minterv1.ProviderCodec + AWSClientBuilder func(accessKeyID, secretAccessKey []byte, c client.Client) (ccaws.Client, error) + Scheme *runtime.Scheme + testUpcomingSecrets []types.NamespacedName } // NewAWSActuator creates a new AWSActuator. @@ -1195,3 +1196,85 @@ func isAWSCredentials(providerSpec *runtime.RawExtension) (bool, error) { } return isAWS, nil } + +func (a *AWSActuator) Upgradeable(mode operatorv1.CloudCredentialsMode) *configv1.ClusterOperatorStatusCondition { + upgradeableCondition := &configv1.ClusterOperatorStatusCondition{ + Status: configv1.ConditionTrue, + Type: configv1.OperatorUpgradeable, + } + toRelease := "4.7" + if mode == operatorv1.CloudCredentialsModeManual { + // Check for the existence of credentials we know are coming in 4.6. If any do not exist, then we consider + // ourselves upgradable=false and inform the user why. + missingSecrets := []types.NamespacedName{} + for _, nsSecret := range a.GetUpcomingCredSecrets() { + + secLog := log.WithField("secret", nsSecret).WithField("toRelease", toRelease) + secLog.Debug("checking that secrets exist for manual mode cluster upgrade") + secret := &corev1.Secret{} + + err := a.Client.Get(context.Background(), nsSecret, secret) + if err != nil { + if errors.IsNotFound(err) { + secLog.Info("identified missing secret for upgrade") + missingSecrets = append(missingSecrets, nsSecret) + } else { + // If we can't figure out if you're upgradeable, you're not upgradeable: + secLog.WithError(err).Error("unexpected error looking up secret, marking upgradeable=false") + upgradeableCondition.Status = configv1.ConditionFalse + upgradeableCondition.Reason = constants.ErrorDeterminingUpgradeableReason + upgradeableCondition.Message = fmt.Sprintf("Error determining if cluster can be upgraded to %s: %v", + toRelease, err) + return upgradeableCondition + } + } + } + + if len(missingSecrets) > 0 { + log.Infof("%d secrets missing, marking upgradeable=false", len(missingSecrets)) + upgradeableCondition.Status = configv1.ConditionFalse + upgradeableCondition.Reason = constants.MissingSecretsForUpgradeReason + upgradeableCondition.Message = fmt.Sprintf( + "Cannot upgrade manual mode cluster to %s due to missing secret(s): %v Please see the Manually Creating IAM for AWS OpenShift documentation.", + toRelease, + missingSecrets) + } + } else { + // If in mint or passthrough, make sure the root cred secret exists, if not it must be restored prior to upgrade. + rootCred := a.GetCredentialsRootSecretLocation() + secret := &corev1.Secret{} + + err := a.Client.Get(context.Background(), rootCred, secret) + if err != nil { + + if errors.IsNotFound(err) { + log.WithField("secret", rootCred).Info("parent cred secret must be restored prior to upgrade, marking upgradeable=false") + upgradeableCondition.Status = configv1.ConditionFalse + upgradeableCondition.Reason = constants.MissingRootCredentialUpgradeableReason + upgradeableCondition.Message = fmt.Sprintf("Parent credential secret must be restored prior to upgrade: %s/%s", + rootCred.Namespace, rootCred.Name) + return upgradeableCondition + } + + log.WithError(err).Error("unexpected error looking up parent secret, marking upgradeable=false") + // If we can't figure out if you're upgradeable, you're not upgradeable: + upgradeableCondition.Status = configv1.ConditionFalse + upgradeableCondition.Reason = constants.ErrorDeterminingUpgradeableReason + upgradeableCondition.Message = fmt.Sprintf("Error determining if cluster can be upgraded to %s: %v", + toRelease, err) + return upgradeableCondition + } + } + + return upgradeableCondition +} + +func (a *AWSActuator) GetUpcomingCredSecrets() []types.NamespacedName { + // For unit testing only, otherwise we want these to be a static list. + if a.testUpcomingSecrets != nil { + return a.testUpcomingSecrets + } + return []types.NamespacedName{ + // Add new 4.7 credentials here, but none are known yet. + } +} diff --git a/pkg/aws/actuator/actuator_test.go b/pkg/aws/actuator/actuator_test.go index 7ec15e768..066610001 100644 --- a/pkg/aws/actuator/actuator_test.go +++ b/pkg/aws/actuator/actuator_test.go @@ -24,9 +24,13 @@ import ( "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" + configv1 "github.com/openshift/api/config/v1" + v1 "github.com/openshift/api/operator/v1" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/client" @@ -271,6 +275,98 @@ func TestGenerateUserName(t *testing.T) { } } +func TestUpgradeable(t *testing.T) { + apis.AddToScheme(scheme.Scheme) + + codec, err := minterv1.NewCodec() + if err != nil { + t.Fatalf("failed to set up codec for tests: %v", err) + } + + tests := []struct { + name string + mode v1.CloudCredentialsMode + existing []runtime.Object + expectedStatus configv1.ConditionStatus + expectedReason string + overrideUpcomingSecrets []types.NamespacedName + }{ + { + name: "mint mode with root cred", + mode: v1.CloudCredentialsModeMint, + existing: []runtime.Object{ + testRootSecret(), + }, + expectedStatus: configv1.ConditionTrue, + }, + { + name: "implicit mint mode with root cred", + mode: v1.CloudCredentialsModeDefault, + existing: []runtime.Object{ + testRootSecret(), + }, + expectedStatus: configv1.ConditionTrue, + }, + { + name: "mint mode with missing root cred", + mode: v1.CloudCredentialsModeMint, + existing: []runtime.Object{}, + expectedStatus: configv1.ConditionFalse, + expectedReason: constants.MissingRootCredentialUpgradeableReason, + }, + { + name: "implicit mint mode with missing root cred", + mode: v1.CloudCredentialsModeDefault, + existing: []runtime.Object{}, + expectedStatus: configv1.ConditionFalse, + expectedReason: constants.MissingRootCredentialUpgradeableReason, + }, + { + name: "manual mode with future version secrets pre-provisioned", + mode: v1.CloudCredentialsModeManual, + existing: []runtime.Object{ + testSecret("kube-system", "foo"), + testSecret("kube-system", "bar"), + }, + expectedStatus: configv1.ConditionTrue, + overrideUpcomingSecrets: []types.NamespacedName{ + {Namespace: "kube-system", Name: "foo"}, + {Namespace: "kube-system", Name: "bar"}, + }, + }, + { + name: "manual mode without future version secrets pre-provisioned", + mode: v1.CloudCredentialsModeManual, + existing: []runtime.Object{}, + expectedStatus: configv1.ConditionFalse, + expectedReason: constants.MissingSecretsForUpgradeReason, + overrideUpcomingSecrets: []types.NamespacedName{ + {Namespace: "kube-system", Name: "foo"}, + {Namespace: "kube-system", Name: "bar"}, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + fakeClient := fake.NewFakeClient(test.existing...) + + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + a := &AWSActuator{ + Client: fakeClient, + Codec: codec, + } + if test.overrideUpcomingSecrets != nil { + a.testUpcomingSecrets = test.overrideUpcomingSecrets + } + cond := a.Upgradeable(test.mode) + assert.Equal(t, test.expectedStatus, cond.Status) + assert.Equal(t, test.expectedReason, cond.Reason) + }) + } +} func testReadOnlySecret() *corev1.Secret { return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -284,6 +380,19 @@ func testReadOnlySecret() *corev1.Secret { } } +func testSecret(namespace, name string) *corev1.Secret { + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Data: map[string][]byte{ + "aws_access_key_id": []byte(testROAccessKeyID), + "aws_secret_access_key": []byte(testROSecretAccessKey), + }, + } +} + func testRootSecret() *corev1.Secret { return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/azure/actuator.go b/pkg/azure/actuator.go index 6b5c67d4f..f2df2e5e1 100644 --- a/pkg/azure/actuator.go +++ b/pkg/azure/actuator.go @@ -23,6 +23,7 @@ import ( "reflect" "time" + operatorv1 "github.com/openshift/api/operator/v1" minterv1 "github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential/v1" corev1 "k8s.io/api/core/v1" @@ -654,3 +655,15 @@ func (a *Actuator) getLogger(cr *minterv1.CredentialsRequest) log.FieldLogger { "cr": fmt.Sprintf("%s/%s", cr.Namespace, cr.Name), }) } + +func (a *Actuator) Upgradeable(mode operatorv1.CloudCredentialsMode) *configv1.ClusterOperatorStatusCondition { + upgradeableCondition := &configv1.ClusterOperatorStatusCondition{ + Status: configv1.ConditionTrue, + Type: configv1.OperatorUpgradeable, + } + return upgradeableCondition +} + +func (a *Actuator) GetUpcomingCredSecrets() []types.NamespacedName { + return []types.NamespacedName{} +} diff --git a/pkg/azure/passthrough.go b/pkg/azure/passthrough.go index fee530e73..44931590e 100644 --- a/pkg/azure/passthrough.go +++ b/pkg/azure/passthrough.go @@ -21,6 +21,8 @@ import ( "fmt" "reflect" + configv1 "github.com/openshift/api/config/v1" + operatorv1 "github.com/openshift/api/operator/v1" minterv1 "github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential/v1" "github.com/openshift/cloud-credential-operator/pkg/operator/constants" "github.com/openshift/cloud-credential-operator/pkg/operator/credentialsrequest/actuator" @@ -99,3 +101,15 @@ func copySecret(cr *minterv1.CredentialsRequest, src *secret, dest *secret) { AzureTenantID: src.Data[AzureTenantID], } } + +func (a *passthrough) Upgradeable(mode operatorv1.CloudCredentialsMode) *configv1.ClusterOperatorStatusCondition { + upgradeableCondition := &configv1.ClusterOperatorStatusCondition{ + Status: configv1.ConditionTrue, + Type: configv1.OperatorUpgradeable, + } + return upgradeableCondition +} + +func (a *passthrough) GetUpcomingCredSecrets() []types.NamespacedName { + return []types.NamespacedName{} +} diff --git a/pkg/gcp/actuator/actuator.go b/pkg/gcp/actuator/actuator.go index 9d6e1a41f..8ac8bfb23 100644 --- a/pkg/gcp/actuator/actuator.go +++ b/pkg/gcp/actuator/actuator.go @@ -21,6 +21,8 @@ import ( "fmt" "reflect" + configv1 "github.com/openshift/api/config/v1" + operatorv1 "github.com/openshift/api/operator/v1" log "github.com/sirupsen/logrus" "sigs.k8s.io/controller-runtime/pkg/client" @@ -767,3 +769,15 @@ func checkServicesEnabled(gcpClient ccgcp.Client, permList []string, logger log. return serviceAPIsEnabled, nil } + +func (a *Actuator) Upgradeable(mode operatorv1.CloudCredentialsMode) *configv1.ClusterOperatorStatusCondition { + upgradeableCondition := &configv1.ClusterOperatorStatusCondition{ + Status: configv1.ConditionTrue, + Type: configv1.OperatorUpgradeable, + } + return upgradeableCondition +} + +func (a *Actuator) GetUpcomingCredSecrets() []types.NamespacedName { + return []types.NamespacedName{} +} diff --git a/pkg/openstack/actuator.go b/pkg/openstack/actuator.go index e5ccaeda6..a7c53c0db 100644 --- a/pkg/openstack/actuator.go +++ b/pkg/openstack/actuator.go @@ -20,6 +20,8 @@ import ( "fmt" "reflect" + configv1 "github.com/openshift/api/config/v1" + operatorv1 "github.com/openshift/api/operator/v1" log "github.com/sirupsen/logrus" corev1 "k8s.io/api/core/v1" @@ -270,3 +272,15 @@ func (a *OpenStackActuator) getLogger(cr *minterv1.CredentialsRequest) log.Field "cr": fmt.Sprintf("%s/%s", cr.Namespace, cr.Name), }) } + +func (a *OpenStackActuator) Upgradeable(mode operatorv1.CloudCredentialsMode) *configv1.ClusterOperatorStatusCondition { + upgradeableCondition := &configv1.ClusterOperatorStatusCondition{ + Status: configv1.ConditionTrue, + Type: configv1.OperatorUpgradeable, + } + return upgradeableCondition +} + +func (a *OpenStackActuator) GetUpcomingCredSecrets() []types.NamespacedName { + return []types.NamespacedName{} +} diff --git a/pkg/operator/constants/constants.go b/pkg/operator/constants/constants.go index 4a6b05a9e..3af38b8db 100644 --- a/pkg/operator/constants/constants.go +++ b/pkg/operator/constants/constants.go @@ -41,6 +41,18 @@ const ( // the operator config CR specifies an invalide mode StatusModeInvalid = "ModeInvalid" + // MissingSecretsForUpgradeReason is used when a manual mode cluster is not upgradable due to missing + // secrets the operator knows will be required for the next minor release of OpenShift. + MissingSecretsForUpgradeReason = "ManualModeMissingSecrets" + + // ErrorDeterminingUpgradeableReason is used when we encounter unexpected errors checking if a cluster can + // be updated. + ErrorDeterminingUpgradeableReason = "ErrorDeterminingUpgradeable" + + // MissingRootCredentialUpgradeableReason is used a cluster is in mint mode with the root credential removed. + // In this state the root credential must be resotred before we can upgrade to the next minor release of OpenShift. + MissingRootCredentialUpgradeableReason = "MissingRootCredential" + // secret annoation vars // AnnotationKey is the annotation the cloud credentials secret will be annotated with to indicate diff --git a/pkg/operator/credentialsrequest/actuator/actuator.go b/pkg/operator/credentialsrequest/actuator/actuator.go index e130d7ccd..be0038ae5 100644 --- a/pkg/operator/credentialsrequest/actuator/actuator.go +++ b/pkg/operator/credentialsrequest/actuator/actuator.go @@ -18,6 +18,9 @@ package actuator import ( "context" + configv1 "github.com/openshift/api/config/v1" + operatorv1 "github.com/openshift/api/operator/v1" + "k8s.io/apimachinery/pkg/types" minterv1 "github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential/v1" @@ -37,6 +40,12 @@ type Actuator interface { Exists(context.Context, *minterv1.CredentialsRequest) (bool, error) // GetCredentialsRootSecretLocation returns the namespace and name where the credentials root secret is stored. GetCredentialsRootSecretLocation() types.NamespacedName + // Upgradeable returns a ClusterOperator Upgradeable condition to indicate whether or not this cluster can + // be safely upgraded to the next "minor" (4.y) Openshift release. + Upgradeable(operatorv1.CloudCredentialsMode) *configv1.ClusterOperatorStatusCondition + // GetUpcomingCredSecrets returns a slice of NamespacedNames for secrets holding credentials that we know are coming in the next OpenShift release. + // Used to pre-check if Upgradeable condition should be true or false for manual mode deployments of the credentials operator. + GetUpcomingCredSecrets() []types.NamespacedName } type DummyActuator struct { @@ -63,6 +72,18 @@ func (a *DummyActuator) GetCredentialsRootSecretLocation() types.NamespacedName return types.NamespacedName{Namespace: constants.CloudCredSecretNamespace, Name: constants.AWSCloudCredSecretName} } +func (a *DummyActuator) Upgradeable(mode operatorv1.CloudCredentialsMode) *configv1.ClusterOperatorStatusCondition { + upgradeableCondition := &configv1.ClusterOperatorStatusCondition{ + Status: configv1.ConditionTrue, + Type: configv1.OperatorUpgradeable, + } + return upgradeableCondition +} + +func (a *DummyActuator) GetUpcomingCredSecrets() []types.NamespacedName { + return []types.NamespacedName{} +} + type ActuatorError struct { ErrReason minterv1.CredentialsRequestConditionType Message string diff --git a/pkg/operator/credentialsrequest/credentialsrequest_controller.go b/pkg/operator/credentialsrequest/credentialsrequest_controller.go index 7301b7491..8380646ac 100644 --- a/pkg/operator/credentialsrequest/credentialsrequest_controller.go +++ b/pkg/operator/credentialsrequest/credentialsrequest_controller.go @@ -69,6 +69,11 @@ const ( cloudCredDeprovisionSuccess = "CloudCredDeprovisionSuccess" credentialsRequestInfraMismatch = "InfrastructureMismatch" + + // Hack: send a fake CredentialsRequest to get us a status update when we don't have one to reconcile. + // Should be removed when status update is broken out into it's own controller. + fakeCredRequestName = "no-op-fake-cred-request" + fakeCredReuestNamespace = "no-op-fake-cred-request-namespace" ) // AddWithActuator creates a new CredentialsRequest Controller and adds it to the Manager with @@ -89,6 +94,16 @@ func newReconciler(mgr manager.Manager, actuator actuator.Actuator, platType con return r } +// isFutureCredRequestSecret is used to identify if a given secret namespace + name is one we're expecting in a future +func isFutureCredRequestSecret(a actuator.Actuator, namespace, name string) bool { + for _, nsn := range a.GetUpcomingCredSecrets() { + if nsn.Namespace == namespace && nsn.Name == name { + return true + } + } + return false +} + // add adds a new Controller to mgr with r as the reconcile.Reconciler func add(mgr manager.Manager, r reconcile.Reconciler) error { // Inject dependencies into Reconciler @@ -129,12 +144,26 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error { return err } + actuator := (r.(*ReconcileCredentialsRequest)).Actuator + // Define a mapping for secrets to the credentials requests that created them. (if applicable) // We use an annotation on secrets that refers back to their owning credentials request because // the normal owner reference is not namespaced, and we want to support credentials requests being // in a centralized namespace, but writing secrets into component namespaces. targetCredSecretMapFunc := handler.ToRequestsFunc( func(a handler.MapObject) []reconcile.Request { + + // Hack: reconcile a fake cred request we're watching for in the Reconcile loop, intended to be + // a no-op so we can get a status update. (remove once we break status out into it's own controller) + if isFutureCredRequestSecret(actuator, a.Meta.GetNamespace(), a.Meta.GetName()) { + return []reconcile.Request{ + {NamespacedName: types.NamespacedName{ + Name: fakeCredRequestName, + Namespace: fakeCredReuestNamespace, + }}, + } + } + // Predicate below should ensure this map function is not called if the // secret does not have our label: namespace, name, err := cache.SplitMetaNamespaceKey(a.Meta.GetAnnotations()[minterv1.AnnotationCredentialsRequest]) @@ -154,9 +183,15 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error { } }) - // These functions are used to determine if a event for the given object should trigger a sync: + // These functions are used to determine if a event for the given Secret should trigger a sync: p := predicate.Funcs{ UpdateFunc: func(e event.UpdateEvent) bool { + + // Watch for Secrets from future release cred requests: + if isFutureCredRequestSecret(actuator, e.MetaNew.GetNamespace(), e.MetaNew.GetName()) { + return true + } + // The object doesn't contain our label, so we have nothing to reconcile. if _, ok := e.MetaOld.GetAnnotations()[minterv1.AnnotationCredentialsRequest]; !ok { return false @@ -164,12 +199,22 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error { return true }, CreateFunc: func(e event.CreateEvent) bool { + // Watch for Secrets from future release cred requests: + if isFutureCredRequestSecret(actuator, e.Meta.GetNamespace(), e.Meta.GetName()) { + return true + } + if _, ok := e.Meta.GetAnnotations()[minterv1.AnnotationCredentialsRequest]; !ok { return false } return true }, DeleteFunc: func(e event.DeleteEvent) bool { + // Watch for Secrets from future release cred requests: + if isFutureCredRequestSecret(actuator, e.Meta.GetNamespace(), e.Meta.GetName()) { + return true + } + if _, ok := e.Meta.GetAnnotations()[minterv1.AnnotationCredentialsRequest]; !ok { return false } @@ -308,6 +353,13 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error { ToRequests: allCredRequestsMapFn, }, configMapPredicate) + + // Watch the CloudCredential config object and reconcile everything on changes. + err = c.Watch( + &source.Kind{Type: &operatorv1.CloudCredential{}}, + &handler.EnqueueRequestsFromMapFunc{ + ToRequests: allCredRequestsMapFn, + }) if err != nil { return err } @@ -363,7 +415,7 @@ func (r *ReconcileCredentialsRequest) Reconcile(request reconcile.Request) (reco }) // Ensure we always sync operator status after reconciling, even if an error occurred. - // TODO: Should this be another controller? + // TODO: Move to another controller: defer func() { err := r.syncOperatorStatus() if err != nil { @@ -373,6 +425,13 @@ func (r *ReconcileCredentialsRequest) Reconcile(request reconcile.Request) (reco metrics.MetricControllerReconcileTime.WithLabelValues(controllerName).Observe(dur.Seconds()) }() + // Hack: reconcile a fake cred request we're watching for in the Reconcile loop, intended to be + // a no-op so we can get a status update. (remove once we break status out into it's own controller) + if request.NamespacedName.Namespace == fakeCredReuestNamespace && request.NamespacedName.Name == fakeCredRequestName { + logger.Info("detected reconcile of fake credentials request name for status update purposes, returning") + return reconcile.Result{}, nil + } + mode, conflict, err := utils.GetOperatorConfiguration(r.Client, logger) if err != nil { logger.WithError(err).Error("error checking if operator is disabled") diff --git a/pkg/operator/credentialsrequest/status.go b/pkg/operator/credentialsrequest/status.go index 1f2948fb9..61d74520d 100644 --- a/pkg/operator/credentialsrequest/status.go +++ b/pkg/operator/credentialsrequest/status.go @@ -5,13 +5,13 @@ import ( "fmt" "os" - "github.com/pkg/errors" log "github.com/sirupsen/logrus" configv1 "github.com/openshift/api/config/v1" operatorv1 "github.com/openshift/api/operator/v1" minterv1 "github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential/v1" + "github.com/openshift/cloud-credential-operator/pkg/operator/credentialsrequest/actuator" "github.com/openshift/cloud-credential-operator/pkg/operator/utils" "github.com/openshift/cloud-credential-operator/pkg/util/clusteroperator" @@ -40,15 +40,16 @@ func (r *ReconcileCredentialsRequest) syncOperatorStatus() error { var _ clusteroperator.StatusHandler = &ReconcileCredentialsRequest{} func (r *ReconcileCredentialsRequest) GetConditions(logger log.FieldLogger) ([]configv1.ClusterOperatorStatusCondition, error) { - _, credRequests, operatorIsDisabled, err := r.getOperatorState(logger) + _, credRequests, mode, err := r.getOperatorState(logger) if err != nil { return []configv1.ClusterOperatorStatusCondition{}, fmt.Errorf("failed to get operator state: %v", err) } - parentSecretExists, err := r.parentSecretExists() - if err != nil { - return []configv1.ClusterOperatorStatusCondition{}, errors.Wrap(err, "error checking if parent secret exists") - } - return computeStatusConditions(credRequests, r.platformType, operatorIsDisabled, parentSecretExists, r.Actuator.GetCredentialsRootSecretLocation(), logger), nil + return computeStatusConditions( + r.Actuator, + mode, + credRequests, + r.platformType, + logger), nil } func (r *ReconcileCredentialsRequest) GetRelatedObjects(logger log.FieldLogger) ([]configv1.ObjectReference, error) { @@ -63,28 +64,17 @@ func (r *ReconcileCredentialsRequest) Name() string { return controllerName } -func (r *ReconcileCredentialsRequest) parentSecretExists() (bool, error) { - parentSecret := &corev1.Secret{} - if err := r.Client.Get(context.TODO(), r.Actuator.GetCredentialsRootSecretLocation(), parentSecret); err != nil { - if apierrors.IsNotFound(err) { - return false, nil - } - return false, err - } - return true, nil -} - // getOperatorState gets and returns the resources necessary to compute the // operator's current state. -func (r *ReconcileCredentialsRequest) getOperatorState(logger log.FieldLogger) (*corev1.Namespace, []minterv1.CredentialsRequest, bool, error) { +func (r *ReconcileCredentialsRequest) getOperatorState(logger log.FieldLogger) (*corev1.Namespace, []minterv1.CredentialsRequest, operatorv1.CloudCredentialsMode, error) { ns := &corev1.Namespace{} if err := r.Client.Get(context.TODO(), types.NamespacedName{Name: minterv1.CloudCredOperatorNamespace}, ns); err != nil { if apierrors.IsNotFound(err) { - return nil, nil, false, nil + return nil, nil, operatorv1.CloudCredentialsModeDefault, nil } - return nil, nil, false, fmt.Errorf( + return nil, nil, operatorv1.CloudCredentialsModeDefault, fmt.Errorf( "error getting Namespace %s: %v", minterv1.CloudCredOperatorNamespace, err) } @@ -94,16 +84,16 @@ func (r *ReconcileCredentialsRequest) getOperatorState(logger log.FieldLogger) ( credRequestList := &minterv1.CredentialsRequestList{} err := r.Client.List(context.TODO(), credRequestList, client.InNamespace(minterv1.CloudCredOperatorNamespace)) if err != nil { - return nil, nil, false, fmt.Errorf( + return nil, nil, operatorv1.CloudCredentialsModeDefault, fmt.Errorf( "failed to list CredentialsRequests: %v", err) } mode, _, err := utils.GetOperatorConfiguration(r.Client, logger) if err != nil { - return nil, nil, false, fmt.Errorf("error checking if operator is disabled: %v", err) + return nil, nil, mode, fmt.Errorf("error checking if operator is disabled: %v", err) } - return ns, credRequestList.Items, mode == operatorv1.CloudCredentialsModeManual, nil + return ns, credRequestList.Items, mode, nil } func computeClusterOperatorVersions() []configv1.OperandVersion { @@ -119,12 +109,19 @@ func computeClusterOperatorVersions() []configv1.OperandVersion { // computeStatusConditions computes the operator's current state. func computeStatusConditions( + actuator actuator.Actuator, + mode operatorv1.CloudCredentialsMode, credRequests []minterv1.CredentialsRequest, clusterCloudPlatform configv1.PlatformType, - operatorIsDisabled bool, parentCredExists bool, parentCredLocation types.NamespacedName, logger log.FieldLogger) []configv1.ClusterOperatorStatusCondition { + operatorIsDisabled := mode == operatorv1.CloudCredentialsModeManual var conditions []configv1.ClusterOperatorStatusCondition + + upgradeableCondition := actuator.Upgradeable(mode) + log.WithField("condition", upgradeableCondition).Info("calculated upgradeable condition") + conditions = append(conditions, *upgradeableCondition) + if operatorIsDisabled { return conditions } @@ -185,6 +182,7 @@ func computeStatusConditions( credRequestsNotProvisioned = credRequestsNotProvisioned + 1 } } + if credRequestsNotProvisioned > 0 || failingCredRequests > 0 { var progressingCondition configv1.ClusterOperatorStatusCondition progressingCondition.Type = configv1.OperatorProgressing @@ -196,19 +194,6 @@ func computeStatusConditions( conditions = append(conditions, progressingCondition) } - // If the operator is not disabled, and we do not have a parent cred in kube-system, we are not - // upgradable. Admin cred removal is a valid mode to run in but it should be restored before - // you can upgrade. - if !operatorIsDisabled && !parentCredExists { - var upgradeableCondition configv1.ClusterOperatorStatusCondition - upgradeableCondition.Type = configv1.OperatorUpgradeable - upgradeableCondition.Status = configv1.ConditionFalse - upgradeableCondition.Reason = reasonCredentialsRootSecretMissing - upgradeableCondition.Message = fmt.Sprintf("Parent credential secret %s/%s must be restored prior to upgrade", - parentCredLocation.Namespace, parentCredLocation.Name) - conditions = append(conditions, upgradeableCondition) - } - // Log all conditions we set: for _, c := range conditions { logger.WithFields(log.Fields{ diff --git a/pkg/operator/credentialsrequest/status_test.go b/pkg/operator/credentialsrequest/status_test.go index d61f9bf9d..d93215cbc 100644 --- a/pkg/operator/credentialsrequest/status_test.go +++ b/pkg/operator/credentialsrequest/status_test.go @@ -23,6 +23,7 @@ import ( "time" "github.com/golang/mock/gomock" + v1 "github.com/openshift/api/operator/v1" log "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -38,7 +39,6 @@ import ( configv1 "github.com/openshift/api/config/v1" minterv1 "github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential/v1" - "github.com/openshift/cloud-credential-operator/pkg/operator/constants" "github.com/openshift/cloud-credential-operator/pkg/operator/credentialsrequest/actuator" schemeutils "github.com/openshift/cloud-credential-operator/pkg/util" ) @@ -81,7 +81,6 @@ func TestClusterOperatorStatus(t *testing.T) { credRequests []minterv1.CredentialsRequest cloudPlatform configv1.PlatformType operatorDisabled bool - parentCredRemoved bool expectedConditions []configv1.ClusterOperatorStatusCondition }{ { @@ -184,14 +183,6 @@ func TestClusterOperatorStatus(t *testing.T) { cloudPlatform: configv1.GCPPlatformType, expectedConditions: []configv1.ClusterOperatorStatusCondition{}, }, - { - name: "upgradable false if parent cred removed", - credRequests: []minterv1.CredentialsRequest{}, - parentCredRemoved: true, - expectedConditions: []configv1.ClusterOperatorStatusCondition{ - testCondition(configv1.OperatorUpgradeable, configv1.ConditionFalse, reasonCredentialsRootSecretMissing), - }, - }, { name: "operator disabled", credRequests: []minterv1.CredentialsRequest{ @@ -210,11 +201,18 @@ func TestClusterOperatorStatus(t *testing.T) { t.Run(test.name, func(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() - clusterOperatorConditions := computeStatusConditions(test.credRequests, - test.cloudPlatform, test.operatorDisabled, - !test.parentCredRemoved, - types.NamespacedName{Namespace: constants.CloudCredSecretNamespace, Name: constants.AWSCloudCredSecretName}, + dummyActuator := &actuator.DummyActuator{} + operatorMode := v1.CloudCredentialsModeMint + if test.operatorDisabled { + operatorMode = v1.CloudCredentialsModeManual + } + clusterOperatorConditions := computeStatusConditions( + dummyActuator, + operatorMode, + test.credRequests, + test.cloudPlatform, log.WithField("test", test.name)) + for _, ec := range test.expectedConditions { c := findClusterOperatorCondition(clusterOperatorConditions, ec.Type) if assert.NotNil(t, c, "unexpected nil for condition %s", ec.Type) { diff --git a/pkg/operator/secretannotator/aws/reconciler.go b/pkg/operator/secretannotator/aws/reconciler.go index 5a620f13a..794f9ae08 100644 --- a/pkg/operator/secretannotator/aws/reconciler.go +++ b/pkg/operator/secretannotator/aws/reconciler.go @@ -6,6 +6,7 @@ import ( "time" log "github.com/sirupsen/logrus" + "k8s.io/apimachinery/pkg/api/errors" "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/predicate" @@ -143,6 +144,10 @@ func (r *ReconcileCloudCredSecret) Reconcile(request reconcile.Request) (returnR secret := &corev1.Secret{} err = r.Get(context.Background(), request.NamespacedName, secret) if err != nil { + if errors.IsNotFound(err) { + r.Logger.Info("parent credential secret does not exist") + return reconcile.Result{}, nil + } r.Logger.WithError(err).Error("failed to fetch secret") return reconcile.Result{}, err } diff --git a/pkg/operator/utils/utils.go b/pkg/operator/utils/utils.go index 6b3f0318e..a2f352e19 100644 --- a/pkg/operator/utils/utils.go +++ b/pkg/operator/utils/utils.go @@ -157,7 +157,6 @@ func isOperatorDisabledViaConfigmap(kubeClient client.Client, logger log.FieldLo cm, err := GetLegacyConfigMap(kubeClient) if err != nil { if errors.IsNotFound(err) { - logger.Debugf("%s ConfigMap does not exist, assuming default behavior", constants.CloudCredOperatorConfigMap) return OperatorDisabledDefault, nil } return OperatorDisabledDefault, err diff --git a/pkg/ovirt/actuator.go b/pkg/ovirt/actuator.go index 02276ddde..54cf641b7 100644 --- a/pkg/ovirt/actuator.go +++ b/pkg/ovirt/actuator.go @@ -21,6 +21,8 @@ import ( "reflect" "strconv" + configv1 "github.com/openshift/api/config/v1" + operatorv1 "github.com/openshift/api/operator/v1" log "github.com/sirupsen/logrus" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -345,3 +347,15 @@ func secretDataFrom(ovirtCreds *OvirtCreds) map[string][]byte { cabundleKey: []byte(ovirtCreds.CABundle), } } + +func (a *OvirtActuator) Upgradeable(mode operatorv1.CloudCredentialsMode) *configv1.ClusterOperatorStatusCondition { + upgradeableCondition := &configv1.ClusterOperatorStatusCondition{ + Status: configv1.ConditionTrue, + Type: configv1.OperatorUpgradeable, + } + return upgradeableCondition +} + +func (a *OvirtActuator) GetUpcomingCredSecrets() []types.NamespacedName { + return []types.NamespacedName{} +} diff --git a/pkg/vsphere/actuator/actuator.go b/pkg/vsphere/actuator/actuator.go index 72ae8ff3a..be85c8ebb 100644 --- a/pkg/vsphere/actuator/actuator.go +++ b/pkg/vsphere/actuator/actuator.go @@ -20,6 +20,8 @@ import ( "fmt" "reflect" + configv1 "github.com/openshift/api/config/v1" + operatorv1 "github.com/openshift/api/operator/v1" log "github.com/sirupsen/logrus" minterv1 "github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential/v1" @@ -385,3 +387,15 @@ func isVSphereCredentials(providerSpec *runtime.RawExtension) (bool, error) { } return isVSphere, nil } + +func (a *VSphereActuator) Upgradeable(mode operatorv1.CloudCredentialsMode) *configv1.ClusterOperatorStatusCondition { + upgradeableCondition := &configv1.ClusterOperatorStatusCondition{ + Status: configv1.ConditionTrue, + Type: configv1.OperatorUpgradeable, + } + return upgradeableCondition +} + +func (a *VSphereActuator) GetUpcomingCredSecrets() []types.NamespacedName { + return []types.NamespacedName{} +}