-
Notifications
You must be signed in to change notification settings - Fork 71
🌱 Add uninstall feature test #2453
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| Feature: Uninstall ClusterExtension | ||
|
|
||
| As an OLM user I would like to uninstall a cluster extension. | ||
|
|
||
| Background: | ||
| Given OLM is available | ||
| And ClusterCatalog "test" serves bundles | ||
| And ServiceAccount "olm-sa" with needed permissions is available in ${TEST_NAMESPACE} | ||
| And ClusterExtension is applied | ||
| """ | ||
| apiVersion: olm.operatorframework.io/v1 | ||
| kind: ClusterExtension | ||
| metadata: | ||
| name: ${NAME} | ||
| spec: | ||
| namespace: ${TEST_NAMESPACE} | ||
| serviceAccount: | ||
| name: olm-sa | ||
| source: | ||
| sourceType: Catalog | ||
| catalog: | ||
| packageName: test | ||
| selector: | ||
| matchLabels: | ||
| "olm.operatorframework.io/metadata.name": test-catalog | ||
| """ | ||
| And bundle "test-operator.1.2.0" is installed in version "1.2.0" | ||
| And ClusterExtension is rolled out | ||
|
|
||
| Scenario: Uninstall ClusterExtension | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Be more verbose, e.g. "Removing ClusterExtension triggers the extension uninstall, removing all installed resources eventually. |
||
| When resource "clusterextension/${NAME}" is removed | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reads better if we introduce step |
||
| Then ClusterExtension is uninstalled | ||
|
|
||
| Scenario: ClusterExtension resources are cleaned up even if the ServiceAccount is no longer present | ||
| When resource "serviceaccount/olm-sa" is removed | ||
| # Ensure service account is gone before checking to ensure resources are cleaned up whether the service account | ||
| # and its permissions are present on the cluster or not | ||
| And resource "serviceaccount/olm-sa" is eventually not found | ||
| And resource "clusterextension/${NAME}" is removed | ||
| Then ClusterExtension is uninstalled | ||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,6 +2,7 @@ package steps | |||||||||
|
|
||||||||||
| import ( | ||||||||||
| "bytes" | ||||||||||
| "compress/gzip" | ||||||||||
| "context" | ||||||||||
| "crypto/tls" | ||||||||||
| "encoding/json" | ||||||||||
|
|
@@ -18,21 +19,24 @@ import ( | |||||||||
|
|
||||||||||
| "github.com/cucumber/godog" | ||||||||||
| jsonpatch "github.com/evanphx/json-patch" | ||||||||||
| "github.com/google/go-cmp/cmp" | ||||||||||
| diff "github.com/google/go-cmp/cmp" | ||||||||||
| "github.com/google/go-containerregistry/pkg/crane" | ||||||||||
| "github.com/prometheus/common/expfmt" | ||||||||||
| "github.com/prometheus/common/model" | ||||||||||
| "github.com/spf13/pflag" | ||||||||||
| "github.com/stretchr/testify/require" | ||||||||||
| "helm.sh/helm/v3/pkg/release" | ||||||||||
| appsv1 "k8s.io/api/apps/v1" | ||||||||||
| corev1 "k8s.io/api/core/v1" | ||||||||||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||||||||||
| "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" | ||||||||||
| "k8s.io/apimachinery/pkg/util/sets" | ||||||||||
| "k8s.io/utils/ptr" | ||||||||||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||||||||||
| "sigs.k8s.io/yaml" | ||||||||||
|
|
||||||||||
| ocv1 "github.com/operator-framework/operator-controller/api/v1" | ||||||||||
| "github.com/operator-framework/operator-controller/internal/operator-controller/features" | ||||||||||
| ) | ||||||||||
|
|
||||||||||
| const ( | ||||||||||
|
|
@@ -56,6 +60,7 @@ func RegisterSteps(sc *godog.ScenarioContext) { | |||||||||
| sc.Step(`^(?i)ClusterExtension is updated(?:\s+.*)?$`, ResourceIsApplied) | ||||||||||
| sc.Step(`^(?i)ClusterExtension is available$`, ClusterExtensionIsAvailable) | ||||||||||
| sc.Step(`^(?i)ClusterExtension is rolled out$`, ClusterExtensionIsRolledOut) | ||||||||||
| sc.Step(`^(?i)ClusterExtension is uninstalled$`, ClusterExtensionIsUninstalled) | ||||||||||
| sc.Step(`^(?i)ClusterExtension reports "([^"]+)" as active revision(s?)$`, ClusterExtensionReportsActiveRevisions) | ||||||||||
| sc.Step(`^(?i)ClusterExtension reports ([[:alnum:]]+) as ([[:alnum:]]+) with Reason ([[:alnum:]]+) and Message:$`, ClusterExtensionReportsCondition) | ||||||||||
| sc.Step(`^(?i)ClusterExtension reports ([[:alnum:]]+) as ([[:alnum:]]+) with Reason ([[:alnum:]]+) and Message includes:$`, ClusterExtensionReportsConditionWithMessageFragment) | ||||||||||
|
|
@@ -68,6 +73,7 @@ func RegisterSteps(sc *godog.ScenarioContext) { | |||||||||
| sc.Step(`^(?i)resource "([^"]+)" is installed$`, ResourceAvailable) | ||||||||||
| sc.Step(`^(?i)resource "([^"]+)" is available$`, ResourceAvailable) | ||||||||||
| sc.Step(`^(?i)resource "([^"]+)" is removed$`, ResourceRemoved) | ||||||||||
| sc.Step(`^(?i)resource "([^"]+)" is eventually not found$`, ResourceEventuallyNotFound) | ||||||||||
| sc.Step(`^(?i)resource "([^"]+)" exists$`, ResourceAvailable) | ||||||||||
| sc.Step(`^(?i)resource is applied$`, ResourceIsApplied) | ||||||||||
| sc.Step(`^(?i)resource "deployment/test-operator" reports as (not ready|ready)$`, MarkTestOperatorNotReady) | ||||||||||
|
|
@@ -260,6 +266,35 @@ func ClusterExtensionIsRolledOut(ctx context.Context) error { | |||||||||
| } | ||||||||||
| return condition["status"] == "True" && condition["reason"] == "Succeeded" && condition["type"] == "Progressing" | ||||||||||
| }, timeout, tick) | ||||||||||
|
|
||||||||||
| // Get ClusterExtension resources | ||||||||||
| objs, err := listExtensionResources(sc.clusterExtensionName) | ||||||||||
| require.NoError(godog.T(ctx), err, "failed to list extension resources") | ||||||||||
|
|
||||||||||
| // Ensure they are all labeled and available on the cluster | ||||||||||
| for _, obj := range objs { | ||||||||||
| labels := obj.GetLabels() | ||||||||||
| require.NotNil(godog.T(ctx), labels) | ||||||||||
| require.Equal(godog.T(ctx), "ClusterExtension", labels["olm.operatorframework.io/owner-kind"]) | ||||||||||
| require.Equal(godog.T(ctx), sc.clusterExtensionName, labels["olm.operatorframework.io/owner-name"]) | ||||||||||
| if err := ResourceAvailable(ctx, fmt.Sprintf("%s/%s", obj.GetObjectKind().GroupVersionKind().Kind, obj.GetName())); err != nil { | ||||||||||
| return err | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Store extension resources in test context | ||||||||||
| sc.extensionObjects = objs | ||||||||||
| return nil | ||||||||||
| } | ||||||||||
|
|
||||||||||
| func ClusterExtensionIsUninstalled(ctx context.Context) error { | ||||||||||
| sc := scenarioCtx(ctx) | ||||||||||
| require.NotNil(godog.T(ctx), sc.extensionObjects) | ||||||||||
| for _, obj := range sc.extensionObjects { | ||||||||||
| if err := ResourceEventuallyNotFound(ctx, fmt.Sprintf("%s/%s", obj.GetObjectKind().GroupVersionKind().Kind, obj.GetName())); err != nil { | ||||||||||
| return err | ||||||||||
| } | ||||||||||
| } | ||||||||||
| return nil | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
@@ -398,24 +433,25 @@ func ClusterExtensionRevisionIsArchived(ctx context.Context, revisionName string | |||||||||
| func ResourceAvailable(ctx context.Context, resource string) error { | ||||||||||
| sc := scenarioCtx(ctx) | ||||||||||
| resource = substituteScenarioVars(resource, sc) | ||||||||||
| rtype, name, found := strings.Cut(resource, "/") | ||||||||||
| kind, name, found := strings.Cut(resource, "/") | ||||||||||
| if !found { | ||||||||||
| return fmt.Errorf("resource %s is not in the format <type>/<name>", resource) | ||||||||||
| return fmt.Errorf("resource %s is not in the format <kind>/<name>", resource) | ||||||||||
| } | ||||||||||
| waitFor(ctx, func() bool { | ||||||||||
| _, err := k8sClient("get", rtype, name, "-n", sc.namespace) | ||||||||||
| _, err := k8sClient("get", kind, name, "-n", sc.namespace) | ||||||||||
| return err == nil | ||||||||||
perdasilva marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
| }) | ||||||||||
| return nil | ||||||||||
| } | ||||||||||
|
|
||||||||||
| func ResourceRemoved(ctx context.Context, resource string) error { | ||||||||||
| sc := scenarioCtx(ctx) | ||||||||||
| rtype, name, found := strings.Cut(resource, "/") | ||||||||||
| resource = substituteScenarioVars(resource, sc) | ||||||||||
| kind, name, found := strings.Cut(resource, "/") | ||||||||||
| if !found { | ||||||||||
| return fmt.Errorf("resource %s is not in the format <type>/<name>", resource) | ||||||||||
| return fmt.Errorf("resource %s is not in the format <kind>/<name>", resource) | ||||||||||
| } | ||||||||||
| yaml, err := k8sClient("get", rtype, name, "-n", sc.namespace, "-o", "yaml") | ||||||||||
| yaml, err := k8sClient("get", kind, name, "-n", sc.namespace, "-o", "yaml") | ||||||||||
| if err != nil { | ||||||||||
| return err | ||||||||||
| } | ||||||||||
|
|
@@ -424,23 +460,38 @@ func ResourceRemoved(ctx context.Context, resource string) error { | |||||||||
| return err | ||||||||||
| } | ||||||||||
| sc.removedResources = append(sc.removedResources, *obj) | ||||||||||
| _, err = k8sClient("delete", rtype, name, "-n", sc.namespace) | ||||||||||
| _, err = k8sClient("delete", kind, name, "-n", sc.namespace) | ||||||||||
| return err | ||||||||||
| } | ||||||||||
|
|
||||||||||
| func ResourceEventuallyNotFound(ctx context.Context, resource string) error { | ||||||||||
| sc := scenarioCtx(ctx) | ||||||||||
| resource = substituteScenarioVars(resource, sc) | ||||||||||
| kind, name, found := strings.Cut(resource, "/") | ||||||||||
| if !found { | ||||||||||
| return fmt.Errorf("resource %s is not in the format <kind>/<name>", resource) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| require.Eventually(godog.T(ctx), func() bool { | ||||||||||
| obj, err := k8sClient("get", kind, name, "-n", sc.namespace, "--ignore-not-found", "-o", "yaml") | ||||||||||
| return err == nil && obj == "" | ||||||||||
|
||||||||||
| return err == nil && obj == "" | |
| return err == nil && strings.TrimSpace(obj) == "" |
perdasilva marked this conversation as resolved.
Show resolved
Hide resolved
perdasilva marked this conversation as resolved.
Show resolved
Hide resolved
perdasilva marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When kubectl get secrets ... returns an empty string, this function returns (nil, err) where err is nil. That propagates a nil Secret to callers and can cause a panic later. Return a non-nil error when no matching release Secret is found.
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assumes exactly one Secret matches the selector, but Helm release storage typically keeps multiple Secrets (history / revisions). As written, len(secretList.Items) != 1 returns (nil, err) where err is nil, and also makes the lookup fail in normal cases. Select the latest deployed revision (e.g., by Helm labels like version) or otherwise disambiguate, and return a real error if the result set is empty/ambiguous.
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
helmReleaseFromSecret assumes a gzipped JSON-encoded Helm release is stored in secret.Data["chunk"] and reads it via string(...) + strings.NewReader. This is very unlikely to match Helm’s standard release Secret encoding (and will panic if secret is nil). Consider using Helm’s storage/driver decoding logic (or the helm-operator-plugins action client) and validate the expected data key/format; at minimum, guard against secret == nil and missing data keys with a clear error.
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in variable name: menifests should be manifests.
| menifests := strings.Split(rel.Manifest, "\n") | |
| for _, manifest := range menifests { | |
| manifests := strings.Split(rel.Manifest, "\n") | |
| for _, manifest := range manifests { |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
collectHelmReleaseObjects splits rel.Manifest by newline and unmarshals each line independently. This will fail if the manifest contains standard multi-line YAML documents (or if objects aren’t newline-delimited). Prefer using the same manifest parsing approach used elsewhere in the codebase (e.g., internal/operator-controller/rukpak/util.ManifestObjects over the full manifest string), or otherwise split on YAML document boundaries and trim whitespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps we should extend description, stating what is the expected outcome of such operation?