Skip to content

Conversation

@bouskaJ
Copy link
Member

@bouskaJ bouskaJ commented Nov 10, 2025

PR Type

Enhancement, Tests


Description

  • Add OLMv1 support alongside existing OLMv1alpha support

    • New olm_v1.go module with OlmV1Installer for ClusterExtension/ClusterCatalog
    • New olm_types.go with common Extension and ExtensionSource interfaces
    • Refactored olm.go to use wrapper types implementing shared interfaces
  • Simplify upgrade test by abstracting OLM installation logic

    • Replace inline OLM object creation with installer functions
    • Support both OLM versions via environment variable OLM_V1
    • Remove duplicate catalog source and subscription management code
  • Update dependencies to support OLMv1 operator-controller

    • Add github.com/operator-framework/operator-controller v1.5.1
    • Bump ginkgo and gomega to latest versions

Diagram Walkthrough

flowchart LR
  A["OLM Test Support"] --> B["OLMv1alpha Installer"]
  A --> C["OLMv1 Installer"]
  B --> D["Extension Interface"]
  C --> D
  B --> E["ExtensionSource Interface"]
  C --> E
  D --> F["Upgrade Test"]
  E --> F
Loading

File Walkthrough

Relevant files
Enhancement
olm_types.go
Define common OLM extension interfaces                                     

test/e2e/support/kubernetes/olm/olm_types.go

  • Define Extension interface with methods for readiness, version
    retrieval, and unwrapping
  • Define ExtensionSource interface with methods for readiness, image
    updates, and unwrapping
  • Provide common abstraction for both OLMv1alpha and OLMv1
    implementations
+21/-0   
olm.go
Implement OLMv1alpha installer with interface wrappers     

test/e2e/support/kubernetes/olm/olm.go

  • Implement subscriptionExtension wrapper for v1alpha1.Subscription with
    IsReady and GetVersion methods
  • Implement catalogSourceWrapper for v1alpha1.CatalogSource with IsReady
    and UpdateSourceImage methods
  • Create OlmInstaller function to set up OLMv1alpha resources
    (OperatorGroup, CatalogSource, Subscription)
  • Both wrappers implement the common Extension and ExtensionSource
    interfaces
+134/-0 
olm_v1.go
Implement OLMv1 installer with interface wrappers               

test/e2e/support/kubernetes/olm/olm_v1.go

  • Implement clusterExtensionWrapper for olmV1.ClusterExtension with
    readiness and version checks
  • Implement clusterCatalogSource for olmV1.ClusterCatalog with readiness
    and image update methods
  • Create OlmV1Installer function to set up OLMv1 resources
    (ServiceAccount, ClusterRoleBinding, ClusterCatalog, ClusterExtension)
  • Both wrappers implement the common Extension and ExtensionSource
    interfaces
+116/-0 
common.go
Remove OLM imports and scheme registration                             

test/e2e/support/common.go

  • Remove unused imports for OLMv1 and OLMv1alpha types
  • Remove scheme registration for olmAlpha and olm packages from
    CreateClient function
  • Simplify client creation by removing OLM-specific setup
+0/-4     
upgrade_test.go
Refactor upgrade test to use OLM installers                           

test/e2e/upgrade_test.go

  • Replace inline OLM object creation with abstracted installer functions
  • Add support for OLMv1 via OLM_V1 environment variable check
  • Simplify catalog source and subscription management using wrapper
    interfaces
  • Remove helper function findClusterServiceVersion and related
    CSV-specific logic
  • Use generic Extension and ExtensionSource interfaces for version
    tracking and readiness checks
  • Refactor upgrade verification to use semver comparison on version
    strings
+51/-111
Dependencies
go.mod
Add OLMv1 operator-controller dependency                                 

go.mod

  • Add github.com/operator-framework/operator-controller v1.5.1 for OLMv1
    support
  • Upgrade github.com/onsi/ginkgo/v2 from v2.23.4 to v2.25.1
  • Upgrade github.com/onsi/gomega from v1.37.0 to v1.38.2
+5/-4     
go.sum
Update dependency checksums                                                           

go.sum

  • Add checksums for new dependencies including operator-controller,
    cel-go, and related packages
  • Update checksums for upgraded ginkgo and gomega versions
  • Add checksums for transitive dependencies (semver, antlr, cobra,
    opentelemetry)
+39/-8   

@sourcery-ai
Copy link

sourcery-ai bot commented Nov 10, 2025

Reviewer's Guide

This PR adds support for both OLM v1 and v2 in the end-to-end upgrade tests by introducing Extension and ExtensionSource abstractions and dedicated installer functions, refactors the upgrade_test to use these new installers with conditional branching on OLM version, cleans up obsolete scheme registrations, and updates related module dependencies.

Sequence diagram for conditional OLM installer selection in upgrade tests

sequenceDiagram
    participant TestRunner
    participant OLMv1Installer
    participant OLMv2Installer
    TestRunner->>OLMv1Installer: Install Extension (if OLMv1)
    TestRunner->>OLMv2Installer: Install Extension (if OLMv2)
    OLMv1Installer-->>TestRunner: Extension installed
    OLMv2Installer-->>TestRunner: Extension installed
Loading

ER diagram for ExtensionSource and Extension relationship

erDiagram
    EXTENSIONSOURCE ||--o| EXTENSION : provides
    EXTENSIONSOURCE {
        string id
        string type
    }
    EXTENSION {
        string name
        string version
    }
Loading

Class diagram for new OLMv1 test support abstractions

classDiagram
    class Extension {
        +Install()
        +Uninstall()
        +Upgrade()
    }
    class ExtensionSource {
        +GetExtension() Extension
    }
    class OLMv1Extension {
        +Install()
        +Uninstall()
        +Upgrade()
    }
    class OLMv2Extension {
        +Install()
        +Uninstall()
        +Upgrade()
    }
    Extension <|.. OLMv1Extension
    Extension <|.. OLMv2Extension
    ExtensionSource --> Extension
Loading

Class diagram for upgrade_test refactor with installer functions

classDiagram
    class UpgradeTest {
        +RunUpgradeTest()
        -installer
    }
    class OLMInstaller {
        +InstallExtension()
        +UninstallExtension()
        +UpgradeExtension()
    }
    UpgradeTest --> OLMInstaller
Loading

File-Level Changes

Change Details Files
Introduce OLM extension abstractions and installers
  • Define Extension and ExtensionSource interfaces for unified install logic
  • Implement OlmInstaller for OLM v2 using Subscription and CatalogSource wrappers
  • Implement OlmV1Installer for OLM v1 using ClusterCatalog and ClusterExtension wrappers
test/e2e/support/kubernetes/olm/olm_types.go
test/e2e/support/kubernetes/olm/olm.go
test/e2e/support/kubernetes/olm/olm_v1.go
Refactor upgrade test to use new OLM installers
  • Import olm support package and branch on OLM_V1 env var
  • Replace manual CatalogSource and CSV handling with installer abstractions
  • Remove findClusterServiceVersion helper and hard-coded subscription logic
test/e2e/upgrade_test.go
Clean up OLM scheme registration in test support
  • Remove direct imports and AddToScheme calls for operator-framework API in common.go
test/e2e/support/common.go
Update module dependencies
  • Bump ginkgo to v2.25.1 and gomega to v1.38.2
  • Add operator-framework/operator-controller and semver v3
  • Update docker/cli version and remove unused yaml.v3 indirect
go.mod

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@qodo-merge-pro
Copy link

qodo-merge-pro bot commented Nov 10, 2025

PR Compliance Guide 🔍

(Compliance updated until commit 642d86d)

Below is a summary of compliance checks for this PR:

Security Compliance
Excessive privileges

Description: A ClusterRoleBinding is created granting the ServiceAccount cluster-admin privileges
(rbac.authorization.k8s.io ClusterRoleBinding with RoleRef kind "ClusterRole" name
"cluster-admin"), which is overly permissive and could allow full cluster takeover if the
SA token is exposed; scope the permissions to the minimum required instead.
olm_v1.go [57-67]

Referred Code
crb := &rbacV1.ClusterRoleBinding{
	ObjectMeta: metav1.ObjectMeta{
		Name: fmt.Sprintf("%s-installer-%s", packageName, ns),
	},
	RoleRef: rbacV1.RoleRef{
		APIGroup: coreV1.SchemeGroupVersion.Group,
		Kind:     "ClusterRole",
		Name:     "cluster-admin",
	},
	Subjects: []rbacV1.Subject{{Kind: "ServiceAccount", Name: sa.Name, Namespace: ns}},
}
Unvalidated env input

Description: The OLM subscription injects the OPENSHIFT environment variable into the operator pod
directly from test input without validation, which can enable environment-based behavior
changes or feature flags; ensure only expected values are allowed and avoid sensitive or
security-impacting env controls in tests that may run against shared clusters.
olm.go [104-123]

Referred Code
subscription := &subscriptionExtension{
	v1alpha1.Subscription{
		ObjectMeta: metav1.ObjectMeta{
			Name:      packageName,
			Namespace: ns,
		},
		Spec: &v1alpha1.SubscriptionSpec{
			CatalogSource:          catalog.Name,
			CatalogSourceNamespace: catalog.Namespace,
			Package:                packageName,
			Channel:                channel,
			Config: &v1alpha1.SubscriptionConfig{
				Env: []coreV1.EnvVar{
					{
						Name:  "OPENSHIFT",
						Value: strconv.FormatBool(ocp),
					},
				},
			},
		},
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Ignored Errors: The helper returns raw errors from client calls without contextual wrapping and uses
silent nil returns on failures (e.g., list/get functions), reducing debuggability and
masking edge cases.

Referred Code
lst := v1alpha1.ClusterServiceVersionList{}
if err := cli.List(ctx, &lst, client.InNamespace(e.Namespace)); err != nil {
	return nil
}
for _, s := range lst.Items {
	if strings.Contains(s.Name, e.Name) && s.Status.Phase == v1alpha1.CSVPhaseSucceeded {
		return &s

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
No Audit Logs: The new OLM installation helpers perform cluster-scoped changes (e.g., creating
ClusterRoleBinding) without emitting any structured audit logs of these critical actions,
which may be acceptable for test code but requires human verification.

Referred Code
func OlmV1Installer(ctx context.Context, cli client.Client, catalogImage, ns, packageName string) (Extension, ExtensionSource, error) {
	sa := &coreV1.ServiceAccount{
		ObjectMeta: metav1.ObjectMeta{
			Name:      fmt.Sprintf("%s-installer", packageName),
			Namespace: ns,
		},
	}

	crb := &rbacV1.ClusterRoleBinding{
		ObjectMeta: metav1.ObjectMeta{
			Name: fmt.Sprintf("%s-installer-%s", packageName, ns),
		},
		RoleRef: rbacV1.RoleRef{
			APIGroup: coreV1.SchemeGroupVersion.Group,
			Kind:     "ClusterRole",
			Name:     "cluster-admin",
		},
		Subjects: []rbacV1.Subject{{Kind: "ServiceAccount", Name: sa.Name, Namespace: ns}},
	}

	selector := map[string]string{"catalog": "test"}


 ... (clipped 49 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Privileged Binding: The installer creates a ClusterRoleBinding granting cluster-admin to a ServiceAccount
without explicit safeguards or validation, which may be acceptable in tests but warrants
human verification for security posture.

Referred Code
crb := &rbacV1.ClusterRoleBinding{
	ObjectMeta: metav1.ObjectMeta{
		Name: fmt.Sprintf("%s-installer-%s", packageName, ns),
	},
	RoleRef: rbacV1.RoleRef{
		APIGroup: coreV1.SchemeGroupVersion.Group,
		Kind:     "ClusterRole",
		Name:     "cluster-admin",
	},
	Subjects: []rbacV1.Subject{{Kind: "ServiceAccount", Name: sa.Name, Namespace: ns}},
}

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Previous compliance checks

Compliance check up to commit d1e04dd
Security Compliance
Over-privileged RBAC

Description: The test installer binds a service account to the cluster-admin ClusterRole via a
ClusterRoleBinding, which grants full cluster privileges and could be abused if executed
outside isolated test environments.
olm_v1.go [58-68]

Referred Code
crb := &rbacV1.ClusterRoleBinding{
	ObjectMeta: metav1.ObjectMeta{
		Name: fmt.Sprintf("%s-installer-%s", packageName, ns),
	},
	RoleRef: rbacV1.RoleRef{
		APIGroup: coreV1.SchemeGroupVersion.Group,
		Kind:     "ClusterRole",
		Name:     "cluster-admin",
	},
	Subjects: []rbacV1.Subject{{Kind: "ServiceAccount", Name: sa.Name, Namespace: ns}},
}
Broad namespace scope

Description: The OLM v1 legacy path creates a cluster-scoped OperatorGroup with empty TargetNamespaces,
potentially granting operator watch over all namespaces and broad cluster access in tests.

olm.go [78-87]

Referred Code
og := &olm.OperatorGroup{
	ObjectMeta: metav1.ObjectMeta{
		Namespace: ns,
		Name:      packageName,
	},
	Spec: olm.OperatorGroupSpec{
		TargetNamespaces: []string{},
	},
}
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
No audit logs: New installer and readiness logic perform critical cluster modifications (create
OperatorGroup, CatalogSource, Subscription) without emitting any audit or trace logs to
reconstruct actions.

Referred Code
func OlmInstaller(ctx context.Context, cli client.Client, catalogImage, ns, packageName, channel string, ocp bool) (Extension, ExtensionSource, error) {
	scheme := cli.Scheme()
	utilruntime.Must(v1alpha1.AddToScheme(scheme))
	utilruntime.Must(olm.AddToScheme(scheme))

	og := &olm.OperatorGroup{
		ObjectMeta: metav1.ObjectMeta{
			Namespace: ns,
			Name:      packageName,
		},
		Spec: olm.OperatorGroupSpec{
			TargetNamespaces: []string{},
		},
	}

	catalog := &catalogSourceWrapper{

		v1alpha1.CatalogSource{
			ObjectMeta: metav1.ObjectMeta{
				Namespace: ns,
				Name:      fmt.Sprintf("%s-catalog", packageName),


 ... (clipped 40 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Thin error context: Installer returns raw errors from client.Create without contextual wrapping or retries,
and readiness checks rely on status conditions without handling transient states or nil
fields.

Referred Code
for _, obj := range []client.Object{sa, crb, es.Unwrap(), cs.Unwrap()} {
	if err := cli.Create(ctx, obj); err != nil {
		return nil, nil, err
	}
}

return es, cs, nil

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Privileged binding: The OLM v1 installer binds a service account to the cluster-admin role without validation
or least-privilege controls, which may be excessive for tests and lacks safeguards.

Referred Code
crb := &rbacV1.ClusterRoleBinding{
	ObjectMeta: metav1.ObjectMeta{
		Name: fmt.Sprintf("%s-installer-%s", packageName, ns),
	},
	RoleRef: rbacV1.RoleRef{
		APIGroup: coreV1.SchemeGroupVersion.Group,
		Kind:     "ClusterRole",
		Name:     "cluster-admin",
	},
	Subjects: []rbacV1.Subject{{Kind: "ServiceAccount", Name: sa.Name, Namespace: ns}},
}

Learn more about managing compliance generic rules or creating your own custom rules

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there - I've reviewed your changes - here's some feedback:

  • Avoid using a fixed time.Sleep for propagating catalog updates—consider polling a well-defined condition or watching for resource events to reduce test flakiness.
  • DeferCleanup currently only deletes the extension and catalog; make sure to also clean up the ServiceAccount and ClusterRoleBinding created by OlmV1Installer to prevent resource leaks.
  • There’s duplicated setup logic between OlmInstaller and OlmV1Installer—extract common creation and readiness-checking steps into shared helpers to reduce code duplication.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Avoid using a fixed time.Sleep for propagating catalog updates—consider polling a well-defined condition or watching for resource events to reduce test flakiness.
- DeferCleanup currently only deletes the extension and catalog; make sure to also clean up the ServiceAccount and ClusterRoleBinding created by OlmV1Installer to prevent resource leaks.
- There’s duplicated setup logic between OlmInstaller and OlmV1Installer—extract common creation and readiness-checking steps into shared helpers to reduce code duplication.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@qodo-merge-pro
Copy link

qodo-merge-pro bot commented Nov 10, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Clean up cluster-scoped test resources

Add cleanup logic to delete the ClusterRoleBinding created by OlmV1Installer to
prevent resource leaks after the test completes.

test/e2e/upgrade_test.go [58-63]

 BeforeAll(func(ctx SpecContext) {
-	DeferCleanup(func() {
-		_ = cli.Delete(ctx, extension.Unwrap())
-		_ = cli.Delete(ctx, catalog.Unwrap())
+	DeferCleanup(func(ctx SpecContext) {
+		if extension != nil {
+			_ = cli.Delete(ctx, extension.Unwrap())
+		}
+		if catalog != nil {
+			_ = cli.Delete(ctx, catalog.Unwrap())
+		}
+		if _, ok := os.LookupEnv("OLM_V1"); ok {
+			crb := &rbacV1.ClusterRoleBinding{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: fmt.Sprintf("%s-installer-%s", "rhtas-operator", namespace.Name),
+				},
+			}
+			_ = cli.Delete(ctx, crb)
+		}
 	})
 })
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a resource leak of a cluster-scoped ClusterRoleBinding and provides a complete solution to clean it up, which is critical for test hygiene.

Medium
Prevent resource leaks and test failures

Modify the resource creation loop to ignore AlreadyExists errors, preventing
test failures on retries and making the test setup more robust.

test/e2e/support/kubernetes/olm/olm_v1.go [109-113]

 for _, obj := range []client.Object{sa, crb, es.Unwrap(), cs.Unwrap()} {
-	if err := cli.Create(ctx, obj); err != nil {
+	if err := cli.Create(ctx, obj); client.IgnoreAlreadyExists(err) != nil {
 		return nil, nil, err
 	}
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out that not ignoring AlreadyExists errors can cause test failures and proposes a valid fix, improving test robustness.

Medium
Ensure correct resource creation order

Reorder the OLM resource creation to ensure OperatorGroup and CatalogSource are
created before the Subscription that depends on them.

test/e2e/support/kubernetes/olm/olm.go [127-131]

-for _, obj := range []client.Object{catalog.Unwrap(), subscription.Unwrap(), og} {
+for _, obj := range []client.Object{og, catalog.Unwrap(), subscription.Unwrap()} {
 	if err := cli.Create(ctx, obj); client.IgnoreAlreadyExists(err) != nil {
 		return nil, nil, err
 	}
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that the OLM resource creation order is not ideal and proposes a change to create dependencies like OperatorGroup before the Subscription, which improves test stability.

Low
  • Update

@bouskaJ bouskaJ force-pushed the SECURESIGN-3244 branch 7 times, most recently from ed394e5 to 44b578d Compare November 11, 2025 13:20
@osmman osmman merged commit eb3f619 into main Nov 18, 2025
39 checks passed
@osmman osmman deleted the SECURESIGN-3244 branch November 18, 2025 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants