Skip to content

Conversation

@knrc
Copy link

@knrc knrc commented Nov 4, 2025

This replaces #1418, implementing auto detection based on the registered API Services.

The original race condition was caused by kubernetes aggregated API servers, with the resources chosen by the original detection method supported by the openshift-apiserver. Since the openshift-apiserver was not guaranteed to have been running at the start of the operator, this led to the race.

If a Local resource had been chosen instead, i.e. one supported by kube-apiserver, there would have been no race.

This PR removes the config CRD proposed in #1418, and implements a detection method based on the APIServices. The detection will query all APIServices and search for one which has an associated service (i.e. aggregated endpoint) with the openshift-apiserver name or namespace.

I checked this behaviour on a number of OCP versions, and also on kind. The versions and APIServices are

  • openshift-apiserver/api
    -- AWS 4.12
    -- AWS 4.20
  • default/openshift-apiserver
    -- rosa 4.16
    -- hypershift 4.19
    -- hypershift 4.20

Summary by Sourcery

Add APIService-based auto-detection for OpenShift platform, replacing manual flag configuration and outdated overlays, while updating RBAC, flags, CI workflows, and build dependencies.

New Features:

  • Automatically detect OpenShift platform via aggregated APIService matching instead of manual configuration

Bug Fixes:

  • Resolve race condition in platform detection by querying APIService resources

Enhancements:

  • Introduce OpenshiftAPIServerName flag and IsFlagProvided utility for conditional platform logic
  • Consolidate kustomize overlays and remove deprecated env config directories
  • Extend operator RBAC to grant get/list/watch permissions on apiservices

Build:

  • Add k8s.io/kube-aggregator dependency

CI:

  • Remove explicit OPENSHIFT environment variables from GitHub workflows to rely on auto-detection

Chores:

  • Remove deprecated OpenShift config CRDs and related env overlays

@sourcery-ai
Copy link

sourcery-ai bot commented Nov 4, 2025

Reviewer's Guide

Introduce auto-detection of OpenShift at startup by querying aggregated APIService resources, remove previous manual configuration, and update RBAC, configuration schema, and CI/build files accordingly.

Sequence diagram for OpenShift platform auto-detection at startup

sequenceDiagram
    participant main as "main()"
    participant utils as "utils.IsFlagProvided()"
    participant k8s as "kubernetes.DetectOpenShiftPlatform()"
    participant apiserver as "APIServiceList"
    participant config as "appconfig"
    main->>utils: Check if 'openshift' flag/env is provided
    alt Flag/env not provided
        main->>k8s: Call DetectOpenShiftPlatform()
        k8s->>apiserver: Query APIService resources
        apiserver-->>k8s: Return APIServiceList
        k8s-->>main: Return detected platform (true/false)
        main->>config: Set appconfig.Openshift
    else Flag/env provided
        main->>config: Use explicit appconfig.Openshift value
    end
    main->>main: Log platform detection result
Loading

Entity relationship diagram for RBAC changes to APIService access

erDiagram
    ROLE ||--o{ APIService : grants
    ROLE {
        string apiGroups
        string resources
        string verbs
    }
    APIService {
        string name
        string namespace
    }
Loading

Class diagram for new and updated configuration and detection types

classDiagram
    class appconfig {
        +int64 CreateTreeDeadline
        +bool Openshift
        +string OpenshiftAPIServerName
    }
    class kubernetes {
        +DetectOpenShiftPlatform(log logr.Logger, apiServiceName string) bool, error
    }
    class utils {
        +IsFlagProvided(name string, envName string) bool
    }
    appconfig <.. utils : uses
    appconfig <.. kubernetes : uses
Loading

File-Level Changes

Change Details Files
Implement auto-detect logic and flag utilities
  • Add openshift-apiserver-name flag with default value
  • Implement IsFlagProvided utility to detect explicit flag or env usage
  • Branch between auto-detection and explicit setting in main based on flag utility
  • Remove redundant strconv import and related logging
cmd/main.go
internal/utils/flag_or_env.go
Add OpenShift detection function based on APIService listing
  • Create DetectOpenShiftPlatform to list APIServices and match service namespace/name
  • Add kubebuilder RBAC marker and grant get/list/watch on apiservices
  • Include kube-aggregator dependency in go.mod
  • Update role.yaml to allow apiservices operations
internal/utils/kubernetes/platform.go
go.mod
config/rbac/role.yaml
Extend operator configuration schema
  • Add OpenshiftAPIServerName field to config struct
internal/config/config.go
Clean up CI workflows and build defaults
  • Remove explicit OPENSHIFT env vars from GitHub workflows
  • Simplify Makefile CONFIG_DEFAULT to a single path
  • Delete obsolete config/env overlays for kubernetes and openshift
.github/workflows/main.yml
Makefile
config/env/kubernetes/kustomization.yaml
config/env/openshift/kustomization.yaml
config/env/openshift/manager_openshift_patch.yaml

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 4, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
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

Generic: Robust Error Handling and Edge Case Management

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

Status: Passed

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

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

Generic: Comprehensive Audit Trails

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

Status:
Detection logging: The new OpenShift auto-detection performs a critical platform decision without explicit
audit-style logging (user/context/outcome) beyond generic info logs, which may be
acceptable for operators but warrants verification of audit requirements elsewhere.

Referred Code
if !utils.IsFlagProvided("openshift", "OPENSHIFT") {
	openshift, err := kubernetes.DetectOpenShiftPlatform(setupLog, appconfig.OpenshiftAPIServerName)
	if err != nil {
		setupLog.Info("Platform auto-detection failed, falling back to kubernetes", "error", err)
		openshift = false
	}
	appconfig.Openshift = openshift
	setupLog.Info("Platform auto-detected", "openshift", appconfig.Openshift)
} else {
	setupLog.Info("Platform explicitly configured via flag/env", "openshift", appconfig.Openshift)
}
Generic: Security-First Input Validation and Data Handling

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

Status:
Input defaults: The API service name used for detection is taken from flag/env with a default and compared
directly without additional validation or normalization, which seems acceptable here but
should be verified against spoofing or unexpected values in cluster.

Referred Code
func DetectOpenShiftPlatform(log logr.Logger, apiServiceName string) (bool, error) {
	if apiServiceName == "" {
		return false, nil
	}
	cfg, err := config.GetConfig()
  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

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 and they look great!

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location> `config/rbac/role.yaml:71-78` </location>
<code_context>
   - persistentvolumeclaims/finalizers
   verbs:
   - update
+- apiGroups:
+  - apiregistration.k8s.io
+  resources:
+  - apiservices
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - apps
</code_context>

<issue_to_address>
**🚨 suggestion (security):** RBAC permissions for apiservices may be broader than necessary.

If platform detection only needs get or list, restrict the RBAC verbs accordingly to minimize permissions.

```suggestion
- apiGroups:
  - apiregistration.k8s.io
  resources:
  - apiservices
  verbs:
  - get
  - list
```
</issue_to_address>

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 4, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Use a more standard detection method

Instead of detecting OpenShift by listing APIServices and matching a service
name, a more robust method is to check for the existence of the
clusterversions.config.openshift.io resource, which is the standard way to
identify an OpenShift cluster.

Examples:

internal/utils/kubernetes/platform.go [15-49]
// DetectOpenShiftPlatform detects whether the operator is running on OpenShift.
// It checks for API services with the specified OpenShift API service name.
func DetectOpenShiftPlatform(log logr.Logger, apiServiceName string) (bool, error) {
	if apiServiceName == "" {
		return false, nil
	}
	cfg, err := config.GetConfig()
	if err != nil {
		return false, err
	}

 ... (clipped 25 lines)

Solution Walkthrough:

Before:

// internal/utils/kubernetes/platform.go
func DetectOpenShiftPlatform(log logr.Logger, apiServiceName string) (bool, error) {
    // ... create client ...

    apiServiceList := &apiregistrationv1.APIServiceList{}
    if err := cl.List(context.Background(), apiServiceList); err != nil {
        return false, err
    }

    for _, apiService := range apiServiceList.Items {
        if service := apiService.Spec.Service; service != nil {
            // Match against a specific service name or namespace
            if apiServiceName == service.Namespace || apiServiceName == service.Name {
                return true, nil
            }
        }
    }
    return false, nil
}

After:

import (
    "k8s.io/client-go/discovery"
    "k8s.io/client-go/rest"
)

// internal/utils/kubernetes/platform.go
func DetectOpenShiftPlatform() (bool, error) {
    cfg, err := rest.GetConfig()
    // ... error handling ...

    discoveryClient, err := discovery.NewDiscoveryClientForConfig(cfg)
    // ... error handling ...

    apiGroupList, err := discoveryClient.ServerGroups()
    // ... error handling ...

    for _, group := range apiGroupList.Groups {
        if group.Name == "config.openshift.io" {
            return true, nil
        }
    }
    return false, nil
}
Suggestion importance[1-10]: 8

__

Why: This suggestion provides a more robust and idiomatic design for OpenShift detection by checking for a canonical API resource, which is less fragile than relying on implementation-specific APIService names.

Medium
Possible issue
Fix incorrect flag detection logic

Fix the IsFlagProvided function to correctly detect if a configuration is
provided via an environment variable, regardless of its value (true or false).

internal/utils/flag_or_env.go [37-50]

-// IsFlagProvided checks if a flag was explicitly provided on the command line or via environment variable
+// IsFlagProvided checks if a flag was explicitly provided via environment variable
 func IsFlagProvided(name string, envName string) bool {
-	if envName != "" && os.Getenv(envName) != "" {
-		return true
+	if envName != "" {
+		if _, ok := os.LookupEnv(envName); ok {
+			return true
+		}
 	}
 
 	provided := false
 	flag.Visit(func(f *flag.Flag) {
 		if f.Name == name {
 			provided = true
 		}
 	})
 	return provided
 }
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a critical bug where setting an environment variable to false would incorrectly trigger auto-detection, and provides a correct fix.

Medium
Improve OpenShift detection logic

Improve the OpenShift detection logic by checking for the existence of the
well-known v1.packages.operators.coreos.com APIService instead of relying on a
configurable service name.

internal/utils/kubernetes/platform.go [38-46]

 	for _, apiService := range apiServiceList.Items {
-		if service := apiService.Spec.Service; service != nil {
-			// The service will be default/openshift-apiserver or openshift-apiserver/api
-			if apiServiceName == service.Namespace || apiServiceName == service.Name {
-				log.Info("Discovered APIService matching API service name", "namespace", service.Namespace, "name", service.Name)
-				return true, nil
-			}
+		if apiService.Name == "v1.packages.operators.coreos.com" {
+			log.Info("Discovered OpenShift APIService", "APIService", apiService.Name)
+			return true, nil
 		}
 	}
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion proposes a valid and potentially more robust method for OpenShift detection by checking for a specific APIService instead of a configurable service name.

Low
  • Update

@knrc knrc requested review from osmman and petrpinkas November 4, 2025 14:21
@knrc
Copy link
Author

knrc commented Nov 5, 2025

Moving this PR to the securesign repo, so the checks will work

@knrc knrc closed this Nov 5, 2025
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.

1 participant