Merge pull request openshift#248 from dgoodwin/46-upgradeable

Bug 1879628: Upgradeable false if upcoming secrets are not provisioned.

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.
+	}
+}

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{

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{}
+}

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{}
+}

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{}
+}

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{}
+}

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

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