Repository: operator-framework/operator-sdk (go)

Issue:
Helm operator constantly creates secret for a failed Operand upgrade
## Bug Report

When an Operand upgrade fails and the bundled helm chart has one or more PersistentVolumeClaim and ServiceAccount, the secrets generated by the `openshift-controller-manager` for each ServiceAccount increase constantly until a successful Operand update is made.

Sample helm operator demonstrating the problem: https://github.com/sukhil-suresh/openshift-operator-bug

#### What did you do?

I successfully deployed an Operand using a helm-based Operator and then proceeded to edit the Operand to reduce the size for a PersistentVolumeClaim. (Customers inadvertently reduce storage size).

#### What did you expect to see?

I expected to see the Operand upgrade fail (since reducing the storage is not allowed) and then a successful rollback should happen.

#### What did you see instead? Under which circumstances?

The helm-operator controller stays stuck in a loop of constant upgrade/rollback failure and the service account secrets increase constantly. 

The screenshot below shows secrets increasing for the `sample-sa` service account:
<img width="1100" alt="secrets" src="https://github.com/operator-framework/operator-sdk/assets/1648613/f19845c3-2af7-4065-ba0b-077f3ca81e4d">

The helm revision history constantly toggles between listing only Revision 1 and listing Revision 1, 2 and 3. 
<img width="3003" alt="helm_revision_history" src="https://github.com/operator-framework/operator-sdk/assets/1648613/59afa861-f6d7-4732-bb2f-c4ff9fea0c52">
Revision 1 is for `deployed` status with `Install complete` as description.

Revision 2 is for the upgrade failure:
```
Upgrade "sample" failed: cannot patch "sample-pvc" with kind PersistentVolumeClaim: PersistentVolumeClaim "sample-pvc" is invalid: spec.resources.requests.storage: Forbidden: field can not be less than previous value
```

Revision 3 is for the rollback failure:
```
Rollback "sample" failed: failed to replace object: PersistentVolumeClaim "sample-pvc" is invalid: spec: Forbidden: spec is immutable after creation except resources.requests for bound claims
  core.PersistentVolumeClaimSpec{
  	AccessModes:      {"ReadWriteOnce"},
  	Selector:         nil,
  	Resources:        {Requests: {s"storage": {i: {...}, s: "2Gi", Format: "BinarySI"}}},
- 	VolumeName:       "pvc-a5cf5a2c-ae5b-4e4a-bbe8-a25b75a4b9be",
+ 	VolumeName:       "",
- 	StorageClassName: &"gp3-csi",
+ 	StorageClassName: nil,
  	VolumeMode:       &"Filesystem",
  	DataSource:       nil,
  	DataSourceRef:    nil,
  }
```

#### Environment

Helm-Operator

/language helm

**Kubernetes cluster type:**

OpenShift 4.12.32

`$ operator-sdk version`
```
operator-sdk version: "v1.28.0-ocp", commit: "9faa5105bfa6ca52490a17dbaee0653dedd0e536", kubernetes version: "v1.26.0", go version: "go1.19.6", GOOS: "darwin", GOARCH: "amd64"
```

`$ kubectl version`

```
Client Version: v1.25.9
Kustomize Version: v4.5.7
Server Version: v1.25.11+c43ddea
```

#### Additional context

This problem does not happen with a regular helm install/upgrade on OpenShift and is specific to the helm-based Operator.


Pull request description:
Allow users to add annotation to set configure rollback
<!--

Welcome to the Operator SDK! Before contributing, make sure to:

- Read the contributing guidelines https://github.com/operator-framework/operator-sdk/blob/master/CONTRIBUTING.MD
- Rebase your branch on the latest upstream master
- Link any relevant issues, PR's, or documentation
- Check that the commit message is concice and helpful:
    - When fixing an issue, add "Closes #<ISSUE_NUMBER>"
    - Sign your commit https://github.com/apps/dco
- Follow the below checklist if making a user-facing change 

-->

**Description of the change:**

Whenever a helm-based operator encounters an error during reconcilliation, by default, it would attempt to perform a rollback with the `--force` option. While this works as expected in most scenarios, there are a few edge cases where performing a rollback with `--force` could have undesired side effects.

Adding annotation to the custom resource, `helm.sdk.operatorframework.io/rollback-force: false` therefore allows a user, to change the default behavior of the helm-based operator whereby, rollbacks will be performed without the `--force` option whenever an error is encountered.%                                                                                                         

**Motivation for the change:**

This change was motivated by the issue #6494 

**Checklist**

If the pull request includes user-facing changes, extra documentation is required:
- [x] Add a new changelog fragment in `changelog/fragments` (see [`changelog/fragments/00-template.yaml`](https://github.com/operator-framework/operator-sdk/tree/master/changelog/fragments/00-template.yaml))
- [x] Add or update relevant sections of the docs website in [`website/content/en/docs`](https://github.com/operator-framework/operator-sdk/tree/master/website/content/en/docs)

Closes #6494 

Patch:
diff --git a/changelog/fragments/helm-operator-rollback.yaml b/changelog/fragments/helm-operator-rollback.yaml
new file mode 100644
index 00000000..94bda486
--- /dev/null
+++ b/changelog/fragments/helm-operator-rollback.yaml
@@ -0,0 +1,33 @@
+# entries is a list of entries to include in
+# release notes and/or the migration guide
+entries:
+  - description: >
+      For Helm-based operators, whenever the operator encounters an 
+      error during reconcilliation, it would attempt to rollback the
+      changes with the `--force` option. This behavior could have 
+      undesired side effects in some scenario.
+
+      This change allows the users to change this behavior by adding the 
+      annotation, `helm.sdk.operatorframework.io/rollback-force: false`
+       to the custom resource.
+
+    # kind is one of:
+    # - addition
+    # - change
+    # - deprecation
+    # - removal
+    # - bugfix
+    kind: "addition"
+
+    # Is this a breaking change?
+    breaking: false
+
+    # NOTE: ONLY USE `pull_request_override` WHEN ADDING THIS
+    # FILE FOR A PREVIOUSLY MERGED PULL_REQUEST!
+    #
+    # The generator auto-detects the PR number from the commit
+    # message in which this file was originally added.
+    #
+    # What is the pull request number (without the "#")?
+    # pull_request_override: 0
+
diff --git a/internal/helm/controller/reconcile.go b/internal/helm/controller/reconcile.go
index e9f2f58b..a1310891 100644
--- a/internal/helm/controller/reconcile.go
+++ b/internal/helm/controller/reconcile.go
@@ -19,6 +19,7 @@ import (
 	"errors"
 	"fmt"
 	"strconv"
+	"strings"
 	"time"
 
 	rpb "helm.sh/helm/v3/pkg/release"
@@ -63,6 +64,7 @@ const (
 	uninstallFinalizerLegacy = "uninstall-helm-release"
 
 	helmUpgradeForceAnnotation    = "helm.sdk.operatorframework.io/upgrade-force"
+	helmRollbackForceAnnotation   = "helm.sdk.operatorframework.io/rollback-force"
 	helmUninstallWaitAnnotation   = "helm.sdk.operatorframework.io/uninstall-wait"
 	helmReconcilePeriodAnnotation = "helm.sdk.operatorframework.io/reconcile-period"
 )
@@ -311,8 +313,18 @@ func (r HelmOperatorReconciler) Reconcile(ctx context.Context, request reconcile
 				"Chart value %q overridden to %q by operator's watches.yaml", k, v)
 		}
 		force := hasAnnotation(helmUpgradeForceAnnotation, o)
+
 		previousRelease, upgradedRelease, err := manager.UpgradeRelease(ctx, release.ForceUpgrade(force))
 		if err != nil {
+			if errors.Is(err, release.ErrUpgradeFailed) {
+				// the forceRollback variable takes the value of the annotation,
+				// "helm.sdk.operatorframework.io/rollback-force".
+				// The default value for the annotation is true
+				forceRollback := readBoolAnnotationWithDefault(o, helmRollbackForceAnnotation, true)
+				if err := manager.RollBack(ctx, release.ForceRollback(forceRollback)); err != nil {
+					log.Error(err, "Error rolling back release")
+				}
+			}
 			log.Error(err, "Release failed")
 			status.SetCondition(types.HelmAppCondition{
 				Type:    types.ConditionReleaseFailed,
@@ -446,6 +458,21 @@ func hasAnnotation(anno string, o *unstructured.Unstructured) bool {
 	return value
 }
 
+func readBoolAnnotationWithDefault(obj *unstructured.Unstructured, annotation string, fallback bool) bool {
+	val, ok := obj.GetAnnotations()[annotation]
+	if !ok {
+		return fallback
+	}
+	r, err := strconv.ParseBool(val)
+	if err != nil {
+		log.Error(
+			fmt.Errorf(strings.ToLower(err.Error())), "error parsing annotation", "annotation", annotation)
+		return fallback
+	}
+
+	return r
+}
+
 func (r HelmOperatorReconciler) updateResource(ctx context.Context, o client.Object) error {
 	return retry.RetryOnConflict(retry.DefaultBackoff, func() error {
 		return r.Client.Update(ctx, o)
diff --git a/internal/helm/release/manager.go b/internal/helm/release/manager.go
index 07641ccc..1aa24e41 100644
--- a/internal/helm/release/manager.go
+++ b/internal/helm/release/manager.go
@@ -54,6 +54,7 @@ type Manager interface {
 	Sync(context.Context) error
 	InstallRelease(context.Context, ...InstallOption) (*rpb.Release, error)
 	UpgradeRelease(context.Context, ...UpgradeOption) (*rpb.Release, *rpb.Release, error)
+	RollBack(context.Context, ...RollBackOption) error
 	ReconcileRelease(context.Context) (*rpb.Release, error)
 	UninstallRelease(context.Context, ...UninstallOption) (*rpb.Release, error)
 	CleanupRelease(context.Context, string) (bool, error)
@@ -79,6 +80,7 @@ type manager struct {
 type InstallOption func(*action.Install) error
 type UpgradeOption func(*action.Upgrade) error
 type UninstallOption func(*action.Uninstall) error
+type RollBackOption func(*action.Rollback) error
 
 // ReleaseName returns the name of the release.
 func (m manager) ReleaseName() string {
@@ -202,10 +204,13 @@ func ForceUpgrade(force bool) UpgradeOption {
 	}
 }
 
+var ErrUpgradeFailed = errors.New("upgrade failed; rollback required")
+
 // UpgradeRelease performs a Helm release upgrade.
 func (m manager) UpgradeRelease(ctx context.Context, opts ...UpgradeOption) (*rpb.Release, *rpb.Release, error) {
 	upgrade := action.NewUpgrade(m.actionConfig)
 	upgrade.Namespace = m.namespace
+
 	for _, o := range opts {
 		if err := o(upgrade); err != nil {
 			return nil, nil, fmt.Errorf("failed to apply upgrade option: %w", err)
@@ -216,24 +221,44 @@ func (m manager) UpgradeRelease(ctx context.Context, opts ...UpgradeOption) (*rp
 	if err != nil {
 		// Workaround for helm/helm#3338
 		if upgradedRelease != nil {
-			rollback := action.NewRollback(m.actionConfig)
-			rollback.Force = true
-
 			// As of Helm 2.13, if UpgradeRelease returns a non-nil release, that
 			// means the release was also recorded in the release store.
 			// Therefore, we should perform the rollback when we have a non-nil
 			// release. Any rollback error here would be unexpected, so always
 			// log both the upgrade and rollback errors.
-			rollbackErr := rollback.Run(m.releaseName)
-			if rollbackErr != nil {
-				return nil, nil, fmt.Errorf("failed upgrade (%s) and failed rollback: %w", err, rollbackErr)
-			}
+			fmt.Printf("release upgrade failed; %v", err)
+
+			return nil, nil, ErrUpgradeFailed
 		}
 		return nil, nil, fmt.Errorf("failed to upgrade release: %w", err)
 	}
 	return m.deployedRelease, upgradedRelease, err
 }
 
+func ForceRollback(force bool) RollBackOption {
+	return func(r *action.Rollback) error {
+		r.Force = force
+		return nil
+	}
+}
+
+// RollBack attempts to reverse any partially applied releases
+func (m manager) RollBack(ctx context.Context, opts ...RollBackOption) error {
+	rollback := action.NewRollback(m.actionConfig)
+
+	for _, fn := range opts {
+		if err := fn(rollback); err != nil {
+			return fmt.Errorf("failed to apply rollback option: %w", err)
+		}
+	}
+
+	if err := rollback.Run(m.releaseName); err != nil {
+		return fmt.Errorf("rollback failed: %w", err)
+	}
+
+	return nil
+}
+
 // ReconcileRelease creates or patches resources as necessary to match the
 // deployed release's manifest.
 func (m manager) ReconcileRelease(ctx context.Context) (*rpb.Release, error) {
diff --git a/website/content/en/docs/building-operators/helm/reference/advanced_features/annotations.md b/website/content/en/docs/building-operators/helm/reference/advanced_features/annotations.md
index 2b8653d8..336f9949 100644
--- a/website/content/en/docs/building-operators/helm/reference/advanced_features/annotations.md
+++ b/website/content/en/docs/building-operators/helm/reference/advanced_features/annotations.md
@@ -166,4 +166,19 @@ metadata:
 The value that is present under this key must be in the h/m/s format. For example, 1h2m4s, 3m0s, 4s are all valid values, but 1x3m9s is invalid. 
 
 **NOTE**: This is just one way of specifying the reconcile period for Helm-based operators. There are two other ways: using the `--reconcile-period` command-line flag and under the 'reconcilePeriod' key in the watches.yaml file. If these three methods are used simultaneously to specify reconcile period (which they should not be), the order of precedence is as follows: 
-Custom Resource Annotations > watches.yaml > command-line flag.
\ No newline at end of file
+Custom Resource Annotations > watches.yaml > command-line flag.
+
+## `helm.sdk.operatorframework.io/rollback-force`
+
+Whenever a helm-based operator encounters an error during reconcilliation, by default, it would attempt to perform a rollback with the `--force` option. While this works as expected in most scenarios, there are a few edge cases where performing a rollback with `--force` could have undesired side effects.
+
+```sh
+...
+metadata:
+  name: nginx-sample
+  annotations:
+    helm.sdk.operatorframework.io/rollback-force: false
+...
+```
+
+Adding annotation to the custom resource, `helm.sdk.operatorframework.io/rollback-force: false` therefore allows a user, to change the default behavior of the helm-based operator whereby, rollbacks will be performed without the `--force` option whenever an error is encountered.
\ No newline at end of file


Test patch:
diff --git a/internal/helm/controller/reconcile_test.go b/internal/helm/controller/reconcile_test.go
index c26c3b23..d7aa73f5 100644
--- a/internal/helm/controller/reconcile_test.go
+++ b/internal/helm/controller/reconcile_test.go
@@ -190,3 +190,64 @@ func annotations(m map[string]interface{}) *unstructured.Unstructured {
 		},
 	}
 }
+
+func Test_readBoolAnnotationWithDefault(t *testing.T) {
+	objBuilder := func(anno map[string]string) *unstructured.Unstructured {
+		object := &unstructured.Unstructured{}
+		object.SetAnnotations(anno)
+		return object
+	}
+
+	type args struct {
+		obj        *unstructured.Unstructured
+		annotation string
+		fallback   bool
+	}
+
+	tests := []struct {
+		name string
+		args args
+		want bool
+	}{
+		{
+			name: "Should return value of annotation read",
+			args: args{
+				obj: objBuilder(map[string]string{
+					"helm.sdk.operatorframework.io/rollback-force": "false",
+				}),
+				annotation: "helm.sdk.operatorframework.io/rollback-force",
+				fallback:   true,
+			},
+			want: false,
+		},
+		{
+			name: "Should return fallback when annotation is not present",
+			args: args{
+				obj: objBuilder(map[string]string{
+					"helm.sdk.operatorframework.io/upgrade-force": "true",
+				}),
+				annotation: "helm.sdk.operatorframework.io/rollback-force",
+				fallback:   false,
+			},
+			want: false,
+		},
+		{
+			name: "Should return fallback when errors while parsing bool value",
+			args: args{
+				obj: objBuilder(map[string]string{
+					"helm.sdk.operatorframework.io/rollback-force": "force",
+				}),
+				annotation: "helm.sdk.operatorframework.io/rollback-force",
+				fallback:   true,
+			},
+			want: true,
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			if got := readBoolAnnotationWithDefault(tc.args.obj, tc.args.annotation, tc.args.fallback); got != tc.want {
+				assert.Equal(t, tc.want, got, "readBoolAnnotationWithDefault() function")
+			}
+		})
+	}
+}
diff --git a/testdata/ansible/memcached-operator/Makefile b/testdata/ansible/memcached-operator/Makefile
index 8f7a290a..21e47735 100644
--- a/testdata/ansible/memcached-operator/Makefile
+++ b/testdata/ansible/memcached-operator/Makefile
@@ -48,7 +48,7 @@ endif
 
 # Set the Operator SDK version to use. By default, what is installed on the system is used.
 # This is useful for CI or a project to utilize a specific version of the operator-sdk toolkit.
-OPERATOR_SDK_VERSION ?= v1.30.0
+OPERATOR_SDK_VERSION ?= v1.31.0
 
 # Image URL to use all building/pushing image targets
 IMG ?= controller:latest
diff --git a/testdata/go/v3/memcached-operator/Makefile b/testdata/go/v3/memcached-operator/Makefile
index 456e32ea..8cfc9f6c 100644
--- a/testdata/go/v3/memcached-operator/Makefile
+++ b/testdata/go/v3/memcached-operator/Makefile
@@ -48,7 +48,7 @@ endif
 
 # Set the Operator SDK version to use. By default, what is installed on the system is used.
 # This is useful for CI or a project to utilize a specific version of the operator-sdk toolkit.
-OPERATOR_SDK_VERSION ?= v1.30.0
+OPERATOR_SDK_VERSION ?= v1.31.0
 
 # Image URL to use all building/pushing image targets
 IMG ?= controller:latest
diff --git a/testdata/go/v3/monitoring/memcached-operator/Makefile b/testdata/go/v3/monitoring/memcached-operator/Makefile
index 97eb0676..42aaf68a 100644
--- a/testdata/go/v3/monitoring/memcached-operator/Makefile
+++ b/testdata/go/v3/monitoring/memcached-operator/Makefile
@@ -48,7 +48,7 @@ endif
 
 # Set the Operator SDK version to use. By default, what is installed on the system is used.
 # This is useful for CI or a project to utilize a specific version of the operator-sdk toolkit.
-OPERATOR_SDK_VERSION ?= v1.30.0
+OPERATOR_SDK_VERSION ?= v1.31.0
 
 # Image URL to use all building/pushing image targets
 IMG ?= controller:latest
diff --git a/testdata/go/v4-alpha/memcached-operator/Makefile b/testdata/go/v4-alpha/memcached-operator/Makefile
index 54ca3963..a7cce76d 100644
--- a/testdata/go/v4-alpha/memcached-operator/Makefile
+++ b/testdata/go/v4-alpha/memcached-operator/Makefile
@@ -48,7 +48,7 @@ endif
 
 # Set the Operator SDK version to use. By default, what is installed on the system is used.
 # This is useful for CI or a project to utilize a specific version of the operator-sdk toolkit.
-OPERATOR_SDK_VERSION ?= v1.30.0
+OPERATOR_SDK_VERSION ?= v1.31.0
 
 # Image URL to use all building/pushing image targets
 IMG ?= controller:latest
diff --git a/testdata/go/v4-alpha/monitoring/memcached-operator/Makefile b/testdata/go/v4-alpha/monitoring/memcached-operator/Makefile
index b7893952..7c598cd3 100644
--- a/testdata/go/v4-alpha/monitoring/memcached-operator/Makefile
+++ b/testdata/go/v4-alpha/monitoring/memcached-operator/Makefile
@@ -48,7 +48,7 @@ endif
 
 # Set the Operator SDK version to use. By default, what is installed on the system is used.
 # This is useful for CI or a project to utilize a specific version of the operator-sdk toolkit.
-OPERATOR_SDK_VERSION ?= v1.30.0
+OPERATOR_SDK_VERSION ?= v1.31.0
 
 # Image URL to use all building/pushing image targets
 IMG ?= controller:latest
diff --git a/testdata/helm/memcached-operator/Makefile b/testdata/helm/memcached-operator/Makefile
index 7f51daee..51efb063 100644
--- a/testdata/helm/memcached-operator/Makefile
+++ b/testdata/helm/memcached-operator/Makefile
@@ -48,7 +48,7 @@ endif
 
 # Set the Operator SDK version to use. By default, what is installed on the system is used.
 # This is useful for CI or a project to utilize a specific version of the operator-sdk toolkit.
-OPERATOR_SDK_VERSION ?= v1.30.0
+OPERATOR_SDK_VERSION ?= v1.31.0
 
 # Image URL to use all building/pushing image targets
 IMG ?= controller:latest