diff --git a/pkg/controller/install/deployment.go b/pkg/controller/install/deployment.go index fc419bacb2..c71014de17 100644 --- a/pkg/controller/install/deployment.go +++ b/pkg/controller/install/deployment.go @@ -6,6 +6,8 @@ import ( log "github.com/sirupsen/logrus" appsv1 "k8s.io/api/apps/v1" rbac "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/api/equality" + "k8s.io/apimachinery/pkg/util/diff" "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1" "github.com/operator-framework/operator-lifecycle-manager/pkg/api/wrappers" @@ -61,25 +63,7 @@ func NewStrategyDeploymentInstaller(strategyClient wrappers.InstallStrategyDeplo func (i *StrategyDeploymentInstaller) installDeployments(deps []StrategyDeploymentSpec) error { for _, d := range deps { - dep := &appsv1.Deployment{Spec: d.Spec} - dep.SetName(d.Name) - dep.SetNamespace(i.owner.GetNamespace()) - - // Merge annotations (to avoid losing info from pod template) - annotations := map[string]string{} - for k, v := range i.templateAnnotations { - annotations[k] = v - } - for k, v := range dep.Spec.Template.GetAnnotations() { - annotations[k] = v - } - dep.Spec.Template.SetAnnotations(annotations) - - ownerutil.AddNonBlockingOwner(dep, i.owner) - if err := ownerutil.AddOwnerLabels(dep, i.owner); err != nil { - return err - } - if _, err := i.strategyClient.CreateOrUpdateDeployment(dep); err != nil { + if _, err := i.strategyClient.CreateOrUpdateDeployment(i.deploymentForSpec(d.Name, d.Spec)); err != nil { return err } } @@ -87,6 +71,26 @@ func (i *StrategyDeploymentInstaller) installDeployments(deps []StrategyDeployme return nil } +func (i *StrategyDeploymentInstaller) deploymentForSpec(name string, spec appsv1.DeploymentSpec) *appsv1.Deployment { + dep := &appsv1.Deployment{Spec: spec} + dep.SetName(name) + dep.SetNamespace(i.owner.GetNamespace()) + + // Merge annotations (to avoid losing info from pod template) + annotations := map[string]string{} + for k, v := range i.templateAnnotations { + annotations[k] = v + } + for k, v := range dep.Spec.Template.GetAnnotations() { + annotations[k] = v + } + dep.Spec.Template.SetAnnotations(annotations) + + ownerutil.AddNonBlockingOwner(dep, i.owner) + ownerutil.AddOwnerLabelsForKind(dep, i.owner, v1alpha1.ClusterServiceVersionKind) + return dep +} + func (i *StrategyDeploymentInstaller) cleanupPrevious(current *StrategyDetailsDeployment, previous *StrategyDetailsDeployment) error { previousDeploymentsMap := map[string]struct{}{} for _, d := range previous.DeploymentSpecs { @@ -179,10 +183,44 @@ func (i *StrategyDeploymentInstaller) checkForDeployments(deploymentSpecs []Stra return StrategyError{Reason: StrategyErrReasonAnnotationsMissing, Message: fmt.Sprintf("annotations on deployment don't match. couldn't find %s: %s", key, value)} } } + + // check equality + calculated := i.deploymentForSpec(spec.Name, spec.Spec) + if !i.equalDeployments(&calculated.Spec, &dep.Spec) { + return StrategyError{Reason: StrategyErrDeploymentUpdated, Message: fmt.Sprintf("deployment changed, rolling update with patch: %s\n%#v\n%#v", diff.ObjectDiff(dep.Spec.Template.Spec, calculated.Spec.Template.Spec), calculated.Spec.Template.Spec, dep.Spec.Template.Spec)} + } } return nil } +func (i *StrategyDeploymentInstaller) equalDeployments(calculated, onCluster *appsv1.DeploymentSpec) bool { + // ignore template annotations, OLM injects these elsewhere + calculated.Template.Annotations = nil + + // DeepDerivative doesn't treat `0` ints as unset. Stripping them here means we miss changes to these values, + // but we don't end up getting bitten by the defaulter for deployments. + for i, c := range onCluster.Template.Spec.Containers { + o := calculated.Template.Spec.Containers[i] + if o.ReadinessProbe != nil { + o.ReadinessProbe.InitialDelaySeconds = c.ReadinessProbe.InitialDelaySeconds + o.ReadinessProbe.TimeoutSeconds = c.ReadinessProbe.TimeoutSeconds + o.ReadinessProbe.PeriodSeconds = c.ReadinessProbe.PeriodSeconds + o.ReadinessProbe.SuccessThreshold = c.ReadinessProbe.SuccessThreshold + o.ReadinessProbe.FailureThreshold = c.ReadinessProbe.FailureThreshold + } + if o.LivenessProbe != nil { + o.LivenessProbe.InitialDelaySeconds = c.LivenessProbe.InitialDelaySeconds + o.LivenessProbe.TimeoutSeconds = c.LivenessProbe.TimeoutSeconds + o.LivenessProbe.PeriodSeconds = c.LivenessProbe.PeriodSeconds + o.LivenessProbe.SuccessThreshold = c.LivenessProbe.SuccessThreshold + o.LivenessProbe.FailureThreshold = c.LivenessProbe.FailureThreshold + } + } + + // DeepDerivative ensures that, for any non-nil, non-empty value in A, the corresponding value is set in B + return equality.Semantic.DeepDerivative(calculated, onCluster) +} + // Clean up orphaned deployments after reinstalling deployments process func (i *StrategyDeploymentInstaller) cleanupOrphanedDeployments(deploymentSpecs []StrategyDeploymentSpec) error { // Map of deployments diff --git a/pkg/controller/install/errors.go b/pkg/controller/install/errors.go index 47a0ae4411..4b06c555c9 100644 --- a/pkg/controller/install/errors.go +++ b/pkg/controller/install/errors.go @@ -9,12 +9,15 @@ const ( StrategyErrReasonInvalidStrategy = "InvalidStrategy" StrategyErrReasonTimeout = "Timeout" StrategyErrReasonUnknown = "Unknown" + StrategyErrBadPatch = "PatchUnsuccessful" + StrategyErrDeploymentUpdated = "DeploymentUpdated" ) // unrecoverableErrors are the set of errors that mean we can't recover an install strategy var unrecoverableErrors = map[string]struct{}{ StrategyErrReasonInvalidStrategy: {}, StrategyErrReasonTimeout: {}, + StrategyErrBadPatch: {}, } // StrategyError is used to represent error types for install strategies diff --git a/pkg/controller/operators/olm/operator.go b/pkg/controller/operators/olm/operator.go index 9affee495d..8af92f307d 100644 --- a/pkg/controller/operators/olm/operator.go +++ b/pkg/controller/operators/olm/operator.go @@ -1271,6 +1271,10 @@ func (a *Operator) updateInstallStatus(csv *v1alpha1.ClusterServiceVersion, inst strategyInstalled, strategyErr := installer.CheckInstalled(strategy) now := a.now() + if strategyErr != nil { + a.logger.WithError(strategyErr).Debug("operator not installed") + } + if strategyInstalled && apiServicesInstalled { // if there's no error, we're successfully running csv.SetPhaseWithEventIfChanged(v1alpha1.CSVPhaseSucceeded, v1alpha1.CSVReasonInstallSuccessful, "install strategy completed with no errors", now, a.recorder) diff --git a/pkg/lib/ownerutil/util.go b/pkg/lib/ownerutil/util.go index 0f8ee56102..0348be004d 100644 --- a/pkg/lib/ownerutil/util.go +++ b/pkg/lib/ownerutil/util.go @@ -195,21 +195,26 @@ func OwnerLabel(owner Owner, kind string) map[string]string { } } -// AddOwnerLabels adds ownerref-like labels to an object +// AddOwnerLabels adds ownerref-like labels to an object by inferring the owner kind func AddOwnerLabels(object metav1.Object, owner Owner) error { err := InferGroupVersionKind(owner) if err != nil { return err } + AddOwnerLabelsForKind(object, owner, owner.GetObjectKind().GroupVersionKind().Kind) + return nil +} + +// AddOwnerLabels adds ownerref-like labels to an object, with no inference +func AddOwnerLabelsForKind(object metav1.Object, owner Owner, kind string) { labels := object.GetLabels() if labels == nil { labels = map[string]string{} } - for key, val := range OwnerLabel(owner, owner.GetObjectKind().GroupVersionKind().Kind) { + for key, val := range OwnerLabel(owner, kind) { labels[key] = val } object.SetLabels(labels) - return nil } // IsOwnedByKindLabel returns whether or not a label exists on the object pointing to an owner of a particular kind diff --git a/scripts/build_local.sh b/scripts/build_local.sh index f4f0b7984b..62bfe4a67b 100755 --- a/scripts/build_local.sh +++ b/scripts/build_local.sh @@ -13,7 +13,7 @@ fi docker build -f local.Dockerfile -t quay.io/operator-framework/olm:local -t quay.io/operator-framework/olm-e2e:local ./bin -if [ -x "$(command -v kind)" ] && [ "kubectl config current-context" -eq "kind" ]; then +if [ -x "$(command -v kind)" ] && [ "$(kubectl config current-context)" = "kind" ]; then kind load docker-image quay.io/operator-framework/olm:local kind load docker-image quay.io/operator-framework/olm-e2e:local fi diff --git a/test/e2e/csv_e2e_test.go b/test/e2e/csv_e2e_test.go index b0113647de..be3c24e3c1 100644 --- a/test/e2e/csv_e2e_test.go +++ b/test/e2e/csv_e2e_test.go @@ -148,7 +148,7 @@ func newNginxDeployment(name string) appsv1.DeploymentSpec { Containers: []corev1.Container{ { Name: genName("nginx"), - Image: "bitnami/nginx:latest", + Image: "redis", Ports: []corev1.ContainerPort{ { ContainerPort: 80, @@ -2393,6 +2393,172 @@ func TestUpdateCSVMultipleIntermediates(t *testing.T) { require.NoError(t, err) } +func TestUpdateCSVInPlace(t *testing.T) { + defer cleaner.NotifyTestComplete(t, true) + + c := newKubeClient(t) + crc := newCRClient(t) + + // Create dependency first (CRD) + crdPlural := genName("ins") + crdName := crdPlural + ".cluster.com" + cleanupCRD, err := createCRD(c, apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: crdName, + }, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Group: "cluster.com", + Version: "v1alpha1", + Names: apiextensions.CustomResourceDefinitionNames{ + Plural: crdPlural, + Singular: crdPlural, + Kind: crdPlural, + ListKind: "list" + crdPlural, + }, + Scope: "Namespaced", + }, + }) + + // Create "current" CSV + nginxName := genName("nginx-") + strategy := install.StrategyDetailsDeployment{ + DeploymentSpecs: []install.StrategyDeploymentSpec{ + { + Name: genName("dep-"), + Spec: newNginxDeployment(nginxName), + }, + }, + } + strategyRaw, err := json.Marshal(strategy) + require.NoError(t, err) + + require.NoError(t, err) + defer cleanupCRD() + csv := v1alpha1.ClusterServiceVersion{ + TypeMeta: metav1.TypeMeta{ + Kind: v1alpha1.ClusterServiceVersionKind, + APIVersion: v1alpha1.ClusterServiceVersionAPIVersion, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: genName("csv"), + }, + Spec: v1alpha1.ClusterServiceVersionSpec{ + MinKubeVersion: "0.0.0", + InstallModes: []v1alpha1.InstallMode{ + { + Type: v1alpha1.InstallModeTypeOwnNamespace, + Supported: true, + }, + { + Type: v1alpha1.InstallModeTypeSingleNamespace, + Supported: true, + }, + { + Type: v1alpha1.InstallModeTypeMultiNamespace, + Supported: true, + }, + { + Type: v1alpha1.InstallModeTypeAllNamespaces, + Supported: true, + }, + }, + InstallStrategy: v1alpha1.NamedInstallStrategy{ + StrategyName: install.InstallStrategyNameDeployment, + StrategySpecRaw: strategyRaw, + }, + CustomResourceDefinitions: v1alpha1.CustomResourceDefinitions{ + Owned: []v1alpha1.CRDDescription{ + { + Name: crdName, + Version: "v1alpha1", + Kind: crdPlural, + DisplayName: crdName, + Description: "In the cluster", + }, + }, + }, + }, + } + + cleanupCSV, err := createCSV(t, c, crc, csv, testNamespace, false, true) + require.NoError(t, err) + defer cleanupCSV() + + // Wait for current CSV to succeed + fetchedCSV, err := fetchCSV(t, crc, csv.Name, testNamespace, csvSucceededChecker) + require.NoError(t, err) + + // Should have created deployment + dep, err := c.GetDeployment(testNamespace, strategy.DeploymentSpecs[0].Name) + require.NoError(t, err) + require.NotNil(t, dep) + + // Create "updated" CSV with a different image + strategyNew := strategy + strategyNew.DeploymentSpecs[0].Spec.Template.Spec.Containers = []corev1.Container{ + { + Name: genName("hat"), + Image: "quay.io/coreos/mock-extension-apiserver:master", + Command: []string{"/bin/mock-extension-apiserver"}, + Args: []string{ + "-v=4", + "--mock-kinds", + "fedora", + "--mock-group-version", + "group.version", + "--secure-port", + "5443", + "--debug", + }, + Ports: []corev1.ContainerPort{ + { + ContainerPort: 5443, + }, + }, + ImagePullPolicy: corev1.PullIfNotPresent, + }, + } + + // Also set something outside the spec template - this should be ignored + var five int32 = 5 + strategyNew.DeploymentSpecs[0].Spec.Replicas = &five + + strategyNewRaw, err := json.Marshal(strategyNew) + require.NoError(t, err) + + fetchedCSV.Spec.InstallStrategy.StrategySpecRaw = strategyNewRaw + + // Update CSV directly + _, err = crc.OperatorsV1alpha1().ClusterServiceVersions(testNamespace).Update(fetchedCSV) + require.NoError(t, err) + + // wait for deployment spec to be updated + err = wait.Poll(pollInterval, pollDuration, func() (bool, error) { + fetched, err := c.GetDeployment(testNamespace, strategyNew.DeploymentSpecs[0].Name) + if err != nil { + return false, err + } + fmt.Println("waiting for deployment to update...") + return fetched.Spec.Template.Spec.Containers[0].Name == strategyNew.DeploymentSpecs[0].Spec.Template.Spec.Containers[0].Name, nil + }) + require.NoError(t, err) + + // Wait for updated CSV to succeed + _, err = fetchCSV(t, crc, csv.Name, testNamespace, csvSucceededChecker) + require.NoError(t, err) + + depUpdated, err := c.GetDeployment(testNamespace, strategyNew.DeploymentSpecs[0].Name) + require.NoError(t, err) + require.NotNil(t, depUpdated) + + // Deployment should have changed even though the CSV is otherwise the same + require.Equal(t, depUpdated.Spec.Template.Spec.Containers[0].Name, strategyNew.DeploymentSpecs[0].Spec.Template.Spec.Containers[0].Name) + require.Equal(t, depUpdated.Spec.Template.Spec.Containers[0].Image, strategyNew.DeploymentSpecs[0].Spec.Template.Spec.Containers[0].Image) + + // Field updated even though template spec didn't change, because it was part of a template spec change as well + require.Equal(t, *depUpdated.Spec.Replicas, *strategyNew.DeploymentSpecs[0].Spec.Replicas) +} + func TestUpdateCSVMultipleVersionCRD(t *testing.T) { defer cleaner.NotifyTestComplete(t, true) diff --git a/test/e2e/installplan_e2e_test.go b/test/e2e/installplan_e2e_test.go index 6e023a5e03..f20a574688 100644 --- a/test/e2e/installplan_e2e_test.go +++ b/test/e2e/installplan_e2e_test.go @@ -101,7 +101,7 @@ func newNginxInstallStrategy(name string, permissions []install.StrategyDeployme Spec: corev1.PodSpec{Containers: []corev1.Container{ { Name: genName("nginx"), - Image: "bitnami/nginx:latest", + Image: "redis", Ports: []corev1.ContainerPort{{ContainerPort: 80}}, ImagePullPolicy: corev1.PullIfNotPresent, }, diff --git a/test/e2e/subscription_e2e_test.go b/test/e2e/subscription_e2e_test.go index 3fb0f988fa..2087388a76 100644 --- a/test/e2e/subscription_e2e_test.go +++ b/test/e2e/subscription_e2e_test.go @@ -210,7 +210,7 @@ var ( Spec: corev1.PodSpec{Containers: []corev1.Container{ { Name: genName("nginx"), - Image: "bitnami/nginx:latest", + Image: "redis", Ports: []corev1.ContainerPort{{ContainerPort: 80}}, ImagePullPolicy: corev1.PullIfNotPresent, },