diff --git a/pkg/apis/samples/v1/types.go b/pkg/apis/samples/v1/types.go index 926e026dca..4c13e6faa1 100644 --- a/pkg/apis/samples/v1/types.go +++ b/pkg/apis/samples/v1/types.go @@ -73,9 +73,9 @@ type ConfigSpec struct { ManagementState operatorv1.ManagementState `json:"managementState,omitempty" protobuf:"bytes,1,opt,name=managementState"` // SamplesRegistry allows for the specification of which registry is accessed - // by the ImageStreams for their image content. Defaults depend on the InstallType. - // An InstallType of 'rhel' defaults to registry.redhat.io, and an InstallType of - // 'centos' defaults to docker.io. + // by the ImageStreams for their image content. Defaults on the content in https://github.com/openshift/library + // that are pulled into this github repository, but based on our pulling only ocp content it typically + // defaults to registry.redhat.io. SamplesRegistry string `json:"samplesRegistry,omitempty" protobuf:"bytes,2,opt,name=samplesRegistry"` // Architectures determine which hardware architecture(s) to install, where x86_64 and ppc64le are the @@ -106,9 +106,9 @@ type ConfigStatus struct { Conditions []ConfigCondition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,2,rep,name=conditions"` // SamplesRegistry allows for the specification of which registry is accessed - // by the ImageStreams for their image content. Defaults depend on the InstallType. - // An InstallType of 'rhel' defaults to registry.redhat.io, and an InstallType of - // 'centos' defaults to docker.io. + // by the ImageStreams for their image content. Defaults on the content in https://github.com/openshift/library + // that are pulled into this github repository, but based on our pulling only ocp content it typically + // defaults to registry.redhat.io. SamplesRegistry string `json:"samplesRegistry,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,3,rep,name=samplesRegistry"` // Architectures determine which hardware architecture(s) to install, where x86_64 and ppc64le are the @@ -174,6 +174,9 @@ const ( // change cycle as complete in both ClusterOperator and Config; retry on import will // occur by the next relist interval if it was an intermittent issue; ImportImageErrorsExist ConfigConditionType = "ImportImageErrorsExist" + // numConfigConditionType is a helper constant that captures the number possible conditions + // defined above in this const block + numconfigConditionType = 7 ) // ConfigCondition captures various conditions of the Config @@ -349,8 +352,9 @@ func (s *Config) ClusterOperatorStatusAvailableCondition() (configv1.ConditionSt // 2) the first string is the succinct text to apply to the Progressing condition on failure // 3) the second string is the fully detailed text to apply the the Failing condition func (s *Config) ClusterOperatorStatusFailingCondition() (configv1.ConditionStatus, string, string) { - if len(s.Status.Conditions) == 0 { - // first event, have not processed default config yet + // do not start checking for bad config and needed cred until we've iterated through + // the credential / config processing to actually processed a config + if len(s.Status.Conditions) < numconfigConditionType { return configv1.ConditionFalse, "", "" } // the ordering here is not random; an invalid config will be caught first; diff --git a/pkg/stub/config.go b/pkg/stub/config.go index ccc899390b..012c521697 100644 --- a/pkg/stub/config.go +++ b/pkg/stub/config.go @@ -274,11 +274,6 @@ func (h *Handler) ProcessManagementField(cfg *v1.Config) (bool, bool, error) { condition.LastUpdateTime = now condition.Status = corev1.ConditionFalse cfg.ConditionUpdate(condition) - cred := cfg.Condition(v1.ImportCredentialsExist) - cred.LastTransitionTime = now - cred.LastUpdateTime = now - cred.Status = corev1.ConditionFalse - cfg.ConditionUpdate(cred) cfg.Status.ManagementState = operatorsv1api.Removed cfg.Status.Version = "" h.ClearStatusConfigForRemoved(cfg) diff --git a/pkg/stub/handler.go b/pkg/stub/handler.go index adfd72e47f..6fcfa55136 100644 --- a/pkg/stub/handler.go +++ b/pkg/stub/handler.go @@ -384,16 +384,8 @@ func (h *Handler) CleanUpOpenshiftNamespaceOnDelete(cfg *v1.Config) error { } } - if cfg.ConditionTrue(v1.ImportCredentialsExist) { - logrus.Println("Operator is deleting the credential in the openshift namespace that was previously created.") - logrus.Println("If you are Removing content as part of switching from 'centos' to 'rhel', the credential will be recreated in the openshift namespace when you move back to 'Managed' state.") - - } - err = h.secretclientwrapper.Delete("openshift", v1.SamplesRegistryCredentials, &metav1.DeleteOptions{}) - if err != nil && !kerrors.IsNotFound(err) { - logrus.Warnf("Problem deleting openshift secret %s on Config delete: %#v", v1.SamplesRegistryCredentials, err) - return err - } + // FYI we no longer delete the credential because the payload imagestreams like cli, must-gather that + // this operator initially installs via its manifest, but does not manage, needs the pull image secret return nil } diff --git a/pkg/stub/handler_test.go b/pkg/stub/handler_test.go index 716f6c2d66..8d77e5b68b 100644 --- a/pkg/stub/handler_test.go +++ b/pkg/stub/handler_test.go @@ -259,20 +259,21 @@ func TestProcessed(t *testing.T) { tkeys := getTKeys() mimic(&h, x86OCPContentRootDir) + processCred(&h, cfg, t) err := h.Handle(event) validate(true, err, "", cfg, conditions, - []corev1.ConditionStatus{corev1.ConditionFalse, corev1.ConditionFalse, corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionFalse, corev1.ConditionFalse, corev1.ConditionFalse}, t) + []corev1.ConditionStatus{corev1.ConditionFalse, corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionFalse, corev1.ConditionFalse, corev1.ConditionFalse}, t) err = h.Handle(event) validate(true, err, "", cfg, conditions, - []corev1.ConditionStatus{corev1.ConditionTrue, corev1.ConditionFalse, corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionFalse, corev1.ConditionFalse, corev1.ConditionFalse}, t) + []corev1.ConditionStatus{corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionFalse, corev1.ConditionFalse, corev1.ConditionFalse}, t) err = h.Handle(event) validate(true, err, "", cfg, conditions, - []corev1.ConditionStatus{corev1.ConditionTrue, corev1.ConditionFalse, corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionFalse, corev1.ConditionFalse, corev1.ConditionFalse}, t) + []corev1.ConditionStatus{corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionFalse, corev1.ConditionFalse, corev1.ConditionFalse}, t) fakeisclient := h.imageclientwrapper.(*fakeImageStreamClientWrapper) for _, key := range iskeys { @@ -313,7 +314,7 @@ func TestProcessed(t *testing.T) { err = h.Handle(event) validate(true, err, "", cfg, conditions, - []corev1.ConditionStatus{corev1.ConditionTrue, corev1.ConditionFalse, corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionFalse, corev1.ConditionFalse, corev1.ConditionFalse}, t) + []corev1.ConditionStatus{corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionTrue, corev1.ConditionFalse, corev1.ConditionFalse, corev1.ConditionFalse}, t) is, _ = fakeisclient.Get("", "foo", metav1.GetOptions{}) if is == nil || !strings.HasPrefix(is.Spec.DockerImageRepository, cfg.Spec.SamplesRegistry) { t.Fatalf("stream repo not updated %#v, %#v", is, h) @@ -507,20 +508,16 @@ func TestCreateDeleteSecretAfterCR(t *testing.T) { h.secretRetryCount = 3 // bypass retry on CR update race cfg.Spec.ManagementState = operatorsv1api.Removed - statuses[1] = corev1.ConditionFalse + statuses[1] = corev1.ConditionTrue err = h.Handle(event) - // import cred should be false if from removed, since we don't recreate + // import cred should be true if from removed, since we don't delete on removed validate(true, err, "", cfg, conditions, statuses, t) cfg.Spec.ManagementState = operatorsv1api.Managed h.secretRetryCount = 3 err = h.Handle(event) - // even though we recreate the secret, we wait for the event to come in - // to set import cred to true - validate(true, err, "", cfg, conditions, statuses, t) - // mimic event from recreate that should have occurred - processCred(&h, cfg, t) - statuses[1] = corev1.ConditionTrue + statuses[3] = corev1.ConditionTrue + // with secret still present, we should start import images validate(true, err, "", cfg, conditions, statuses, t) } @@ -529,6 +526,8 @@ func setup() (Handler, *v1.Config, v1.Event) { h := NewTestHandler() cfg, _ := h.CreateDefaultResourceIfNeeded(nil) cfg = h.initConditions(cfg) + fakesecretclient := h.secretclientwrapper.(*fakeSecretClientWrapper) + fakesecretclient.err = kerrors.NewNotFound(schema.GroupResource{}, v1.SamplesRegistryCredentials) h.crdwrapper.(*fakeCRDWrapper).cfg = cfg cache.ClearUpsertsCache() return h, cfg, v1.Event{Object: cfg} @@ -538,6 +537,7 @@ func processCred(h *Handler, cfg *v1.Config, t *testing.T) { if !cfg.ConditionFalse(v1.ImportCredentialsExist) { t.Fatalf("import cred exists unexpectedly true: %#v", cfg) } + h.secretclientwrapper.(*fakeSecretClientWrapper).err = nil secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: v1.SamplesRegistryCredentials, diff --git a/pkg/stub/secrets.go b/pkg/stub/secrets.go index 1a69d499d0..ddbad23bee 100644 --- a/pkg/stub/secrets.go +++ b/pkg/stub/secrets.go @@ -104,7 +104,10 @@ func (h *Handler) WaitingForCredential(cfg *v1.Config) (bool, bool) { // if trying to do rhel to the default registry.redhat.io registry requires the secret // be in place since registry.redhat.io requires auth to pull; since it is not ready // log error state - if cfg.ClusterNeedsCreds() { + // we check for actual existence vs. condition because in delete/recreate scenario, the condition can't + // be added out of the gate + _, err := h.secretclientwrapper.Get("openshift", v1.SamplesRegistryCredentials) + if err != nil { cred := cfg.Condition(v1.ImportCredentialsExist) // - if import cred is false, and the message is empty, that means we have NOT registered the error, and need to do so // - if cred is false, and the message is there, we can just return nil to the sdk, which "true" for the boolean return value indicates; @@ -117,8 +120,11 @@ func (h *Handler) WaitingForCredential(cfg *v1.Config) (bool, bool) { h.processError(cfg, v1.ImportCredentialsExist, corev1.ConditionFalse, err, "%v") return true, true } + if !cfg.ConditionTrue(v1.ImportCredentialsExist) { + h.GoodConditionUpdate(cfg, corev1.ConditionTrue, v1.ImportCredentialsExist) + } - // this is either centos, or the cluster admin is using their own registry for rhel content, so we do not + // the credentials are already in place, or the cluster admin is using their own registry for rhel content, so we do not // enforce the need for the credential return false, false } @@ -136,16 +142,9 @@ func (h *Handler) processSecretEvent(cfg *v1.Config, dockercfgSecret *corev1.Sec removedState := false switch cfg.Spec.ManagementState { case operatorsv1api.Removed: - // so our current recipe to switch to rhel is to - // - mark mgmt state removed - // - after that complete, edit again, mark install type to rhel and mgmt state to managed - // but what about the secret needed for rhel ... do we force the user to create the secret - // while still in managed/centos state? Even with that, the "removed" action removes the - // secret the operator creates in the openshift namespace since it was owned/created by - // the operator // So we allow the processing of the secret event while in removed state to - // facilitate the switch from centos to rhel, as necessitating use of removed as the means for - // changing from centos to rhel since we allow changing the distribution once the samples have initially been created + // facilitate the imagestreams like cli, must-gather, that are installed from the + // payload via this operator's manifest, but are not managed by this operator logrus.Printf("processing secret watch event while in Removed state; deletion event: %v", event.Deleted) removedState = true case operatorsv1api.Unmanaged: diff --git a/test/e2e/cluster_samples_operator_test.go b/test/e2e/cluster_samples_operator_test.go index 641015a585..28412634a2 100644 --- a/test/e2e/cluster_samples_operator_test.go +++ b/test/e2e/cluster_samples_operator_test.go @@ -177,8 +177,7 @@ func verifyConditionsCompleteSamplesRemoved(t *testing.T) error { return false, nil } if cfg.ConditionFalse(samplesapi.SamplesExist) && - cfg.ConditionFalse(samplesapi.ImageChangesInProgress) && - cfg.ConditionFalse(samplesapi.ImportCredentialsExist) { + cfg.ConditionFalse(samplesapi.ImageChangesInProgress) { return true, nil } diff --git a/test/e2e/main_test.go b/test/e2e/main_test.go index a7f5482b79..ea9720e870 100644 --- a/test/e2e/main_test.go +++ b/test/e2e/main_test.go @@ -20,7 +20,9 @@ import ( "testing" "time" + configv1client "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1" sampopclient "github.com/openshift/cluster-samples-operator/pkg/client" + "github.com/openshift/cluster-samples-operator/test/framework" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/wait" kubeset "k8s.io/client-go/kubernetes" @@ -46,6 +48,17 @@ func TestMain(m *testing.M) { fmt.Println("failed waiting for operator to start") os.Exit(1) } + opClient, err := configv1client.NewForConfig(kubeconfig) + if err != nil { + fmt.Printf("problem getting operator client %#v", err) + os.Exit(1) + } + err = framework.DisableCVOForOperator(opClient) + if err != nil { + fmt.Printf("problem disabling operator deployment in CVO %#v", err) + os.Exit(1) + } + os.Exit(m.Run()) } diff --git a/test/framework/cvo.go b/test/framework/cvo.go new file mode 100644 index 0000000000..9a7d758654 --- /dev/null +++ b/test/framework/cvo.go @@ -0,0 +1,50 @@ +package framework + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + configv1 "github.com/openshift/api/config/v1" + configv1client "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1" +) + +const ( + clusterVersionName = "version" +) + +func addCompomentOverride(overrides []configv1.ComponentOverride, override configv1.ComponentOverride) ([]configv1.ComponentOverride, bool) { + for i, o := range overrides { + if o.Group == override.Group && o.Kind == override.Kind && + o.Namespace == override.Namespace && o.Name == override.Name { + if overrides[i].Unmanaged == override.Unmanaged { + return overrides, false + } + overrides[i].Unmanaged = override.Unmanaged + return overrides, true + } + } + return append(overrides, override), true +} + +// DisableCVOForOperator disables the samples operator deployment so we can modify the version env +func DisableCVOForOperator(operatorClient *configv1client.ConfigV1Client) error { + cv, err := operatorClient.ClusterVersions().Get(clusterVersionName, metav1.GetOptions{}) + if err != nil { + return err + } + + var changed bool + cv.Spec.Overrides, changed = addCompomentOverride(cv.Spec.Overrides, configv1.ComponentOverride{ + Kind: "Deployment", + Namespace: "openshift-cluster-samples-operator", + Name: "cluster-samples-operator", + Unmanaged: true, + }) + if !changed { + return nil + } + if _, err := operatorClient.ClusterVersions().Update(cv); err != nil { + return err + } + + return nil +}