WIP: OCPBUGS-88737: Update PSM/package server to support TLS profiles#1328
Open
tmshort wants to merge 1 commit into
Open
WIP: OCPBUGS-88737: Update PSM/package server to support TLS profiles#1328tmshort wants to merge 1 commit into
tmshort wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (7)
WalkthroughThe PR propagates the cluster OpenShift APIServer TLS security profile into PackageServer through two independent execution paths: the package-server-manager controller reads the cluster ChangesTLS profile propagation to PackageServer
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over Reconcile,CSV: Controller path: APIServer TLS → CSV flags
Reconcile->>APIServer: GET "cluster"
APIServer-->>Reconcile: TLSSecurityProfile
Reconcile->>Reconcile: derive --tls-min-version<br/>--tls-cipher-suites
Reconcile->>reconcileCSV: flags []string
reconcileCSV->>ensureCSV: flags
ensureCSV->>CSV: WithRunFlags(flags)
end
rect rgba(144, 238, 144, 0.5)
Note over Run,SecureServing: Server path: APIServer TLS → SecureServing
Run->>Run: build clientConfig
Run->>applyClusterTLSProfile: clientConfig
applyClusterTLSProfile->>APIServer: GET "cluster"
APIServer-->>applyClusterTLSProfile: TLSSecurityProfile
applyClusterTLSProfile->>SecureServing: MinTLSVersion<br/>CipherSuites
applyClusterTLSProfile-->>Run: error or nil
Run->>Run: o.Config(ctx)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tmshort The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/package-server-manager/controller.go (1)
87-116:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep
--intervalout of the extra flags passed toensureCSV.
flagsalready includes--intervalhere, andensureCSVprependsintervalagain before appending itsflagsargument. Whenr.Intervalis set, the reconciled CSV command ends up with duplicate--intervalargs. Keep the controller’s TLS flags separate from the full run flag list.Proposed fix
- flags := []string{} - if r.Interval != "" { - flags = append(flags, "--interval", r.Interval) - } + extraFlags := []string{} var apiServer configv1.APIServer if err := r.Client.Get(ctx, types.NamespacedName{Name: "cluster"}, &apiServer); err != nil { if !apierrors.IsNotFound(err) { return ctrl.Result{}, err } } else { minVersion, cipherSuites := olmapiserver.GetSecurityProfileConfig(apiServer.Spec.TLSSecurityProfile) - flags = append(flags, + extraFlags = append(extraFlags, "--tls-min-version", libcrypto.TLSVersionToNameOrDie(minVersion), "--tls-cipher-suites", strings.Join(libcrypto.CipherSuitesToNamesOrDie(cipherSuites), ","), ) } + runFlags := []string{} + if r.Interval != "" { + runFlags = append(runFlags, "--interval", r.Interval) + } + runFlags = append(runFlags, extraFlags...) + required, err := manifests.NewPackageServerCSV( manifests.WithName(r.Name), manifests.WithNamespace(r.Namespace), manifests.WithImage(r.Image), - manifests.WithRunFlags(flags), + manifests.WithRunFlags(runFlags), ) if err != nil { log.Error(err, "failed to serialize a new packageserver csv from the base YAML manifest") return ctrl.Result{}, err } res, err := controllerutil.CreateOrUpdate(ctx, r.Client, required, func() error { - return reconcileCSV(r.Log, r.Image, r.Interval, flags, required, highAvailabilityMode) + return reconcileCSV(r.Log, r.Image, r.Interval, extraFlags, required, highAvailabilityMode) })-func reconcileCSV(log logr.Logger, image string, interval string, flags []string, csv *olmv1alpha1.ClusterServiceVersion, highAvailabilityMode bool) error { +func reconcileCSV(log logr.Logger, image string, interval string, extraFlags []string, csv *olmv1alpha1.ClusterServiceVersion, highAvailabilityMode bool) error { if csv.ObjectMeta.CreationTimestamp.IsZero() { log.Info("attempting to create the packageserver csv") } - modified, err := ensureCSV(log, image, interval, flags, csv, highAvailabilityMode) + modified, err := ensureCSV(log, image, interval, extraFlags, csv, highAvailabilityMode)Also applies to: 143-148
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/package-server-manager/controller.go` around lines 87 - 116, The flags variable contains both the --interval argument and TLS security flags that are passed to reconcileCSV, but reconcileCSV already prepends --interval before appending its flags argument, causing duplicate --interval arguments when r.Interval is set. Refactor the code to separate concerns by creating a separate variable (e.g., tlsFlags) to hold only the TLS configuration flags from the APIServer spec, then pass only the tlsFlags to reconcileCSV instead of the combined flags list. The --interval flag should be handled by reconcileCSV independently. Apply this same refactoring to both locations where this pattern occurs (the main location at lines 87-116 and the consolidated site at lines 143-148).
🧹 Nitpick comments (1)
pkg/package-server-manager/controller_test.go (1)
264-264: ⚡ Quick winAdd a non-nil flags case for the new run-flag path.
This call only exercises the legacy
nilpath, so the new TLS flag merge behavior is untested. Add a table case with extra flags, e.g.--tls-min-versionand--tls-cipher-suites, and assert the expected CSV command contains--intervalfollowed by those flags exactly once.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/package-server-manager/controller_test.go` at line 264, The current test case for ensureCSV only tests the legacy path with nil flags, leaving the new TLS flag merge behavior untested. Add a new table-driven test case to the existing test that provides non-nil flags containing TLS options (such as --tls-min-version and --tls-cipher-suites) to the ensureCSV function, and assert that the resulting CSV command includes the --interval flag followed by those TLS flags appearing exactly once in the output. This ensures the flag merging behavior works correctly for the new run-flag path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@staging/operator-lifecycle-manager/pkg/package-server/server/server.go`:
- Around line 350-369: The applyClusterTLSProfile function makes API discovery
and APIServers().Get calls using the long-lived Run context without timeouts,
which could hang startup indefinitely. Create a short timeout context and a
copied rest.Config with a Timeout field set, then use these for the
IsAPIAvailable and APIServers().Get calls instead of the original context and
config. This ensures that stalled API calls cannot block PackageServer startup.
- Line 9: Remove the redundant join/split operation that wraps the result of
CipherSuitesToNamesOrDie. When CipherSuitesToNamesOrDie returns an empty slice,
the current join/split converts it to a slice with an empty string element,
which causes downstream parsing to fail. Find where CipherSuitesToNamesOrDie is
being called and joined/split, and instead assign its result directly to the
cipher suites variable without the join/split conversion. Then remove the
now-unused strings import from the imports section.
- Around line 225-231: The error handling in the applyClusterTLSProfile call
currently logs a warning and allows startup with default TLS settings when the
cluster profile cannot be applied, which undermines cluster security policies.
Change this to fail closed by updating the error handling logic: instead of
logging a warning and continuing, return the error or log it at a fatal level to
prevent startup with insecure defaults. Additionally, add the required RBAC rule
to the PackageServer ClusterServiceVersion to grant the cluster service account
permission to read apiservers resources in config.openshift.io, which will allow
applyClusterTLSProfile to succeed and retrieve the cluster's TLS configuration.
---
Outside diff comments:
In `@pkg/package-server-manager/controller.go`:
- Around line 87-116: The flags variable contains both the --interval argument
and TLS security flags that are passed to reconcileCSV, but reconcileCSV already
prepends --interval before appending its flags argument, causing duplicate
--interval arguments when r.Interval is set. Refactor the code to separate
concerns by creating a separate variable (e.g., tlsFlags) to hold only the TLS
configuration flags from the APIServer spec, then pass only the tlsFlags to
reconcileCSV instead of the combined flags list. The --interval flag should be
handled by reconcileCSV independently. Apply this same refactoring to both
locations where this pattern occurs (the main location at lines 87-116 and the
consolidated site at lines 143-148).
---
Nitpick comments:
In `@pkg/package-server-manager/controller_test.go`:
- Line 264: The current test case for ensureCSV only tests the legacy path with
nil flags, leaving the new TLS flag merge behavior untested. Add a new
table-driven test case to the existing test that provides non-nil flags
containing TLS options (such as --tls-min-version and --tls-cipher-suites) to
the ensureCSV function, and assert that the resulting CSV command includes the
--interval flag followed by those TLS flags appearing exactly once in the
output. This ensures the flag merging behavior works correctly for the new
run-flag path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f464048d-8014-4442-bdac-70924ccf18bb
📒 Files selected for processing (4)
pkg/package-server-manager/config.gopkg/package-server-manager/controller.gopkg/package-server-manager/controller_test.gostaging/operator-lifecycle-manager/pkg/package-server/server/server.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/package-server-manager/controller.go (1)
87-90:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep
flagsas TLS-only before passing it toensureCSV.
ensureCSVnow receivesintervalseparately and combines it with caller-provided flags. Because this slice already contains--interval, the reconciled CSV can get duplicate interval arguments whilemanifests.WithRunFlagsappends them verbatim.Proposed fix
- flags := []string{} + flags := []string{} + runFlags := []string{} if r.Interval != "" { - flags = append(flags, "--interval", r.Interval) + runFlags = append(runFlags, "--interval", r.Interval) } @@ flags = append(flags, "--tls-min-version", minVersionStr, "--tls-cipher-suites", cipherSuitesStr, ) log.Info("applying cluster TLS security profile to packageserver", "minVersion", minVersionStr, "cipherSuites", cipherSuitesStr) } + runFlags = append(runFlags, flags...) required, err := manifests.NewPackageServerCSV( manifests.WithName(r.Name), manifests.WithNamespace(r.Namespace), manifests.WithImage(r.Image), - manifests.WithRunFlags(flags), + manifests.WithRunFlags(runFlags), ) @@ - return reconcileCSV(r.Log, r.Image, r.Interval, flags, required, highAvailabilityMode) + return reconcileCSV(r.Log, r.Image, r.Interval, flags, required, highAvailabilityMode)Also applies to: 101-104, 108-119
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/package-server-manager/controller.go` around lines 87 - 90, The flags slice is currently being populated with both TLS and interval arguments before being passed to ensureCSV, but ensureCSV now receives interval separately and combines it with the caller-provided flags, causing duplicate interval arguments. Remove the code blocks that append interval flags to the flags slice. In the section around lines 87-90 in pkg/package-server-manager/controller.go where the code checks if r.Interval is not empty and appends "--interval" and r.Interval to flags, delete this entire conditional block. Apply the same fix at lines 101-104 and lines 108-119 to remove any similar interval flag appending. Keep only TLS-related flags in the flags slice before passing it to ensureCSV, since the interval will be passed separately to that function.
🧹 Nitpick comments (1)
pkg/package-server-manager/controller_test.go (1)
264-264: ⚡ Quick winAdd coverage for non-nil CSV run flags.
This updates the call site but still only tests the old
nilpath. Please add at least one case with TLS flags soensureCSVproves it preserves--tls-min-version/--tls-cipher-suitesin the PackageServer command.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/package-server-manager/controller_test.go` at line 264, The test at the ensureCSV function call is still only testing the nil path for CSV run flags. Add at least one test case to the test suite where non-nil CSV run flags containing TLS parameters (such as --tls-min-version and --tls-cipher-suites) are passed to the ensureCSV function, and verify that these TLS flags are properly preserved in the resulting PackageServer command output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/package-server-manager/controller.go`:
- Around line 87-90: The flags slice is currently being populated with both TLS
and interval arguments before being passed to ensureCSV, but ensureCSV now
receives interval separately and combines it with the caller-provided flags,
causing duplicate interval arguments. Remove the code blocks that append
interval flags to the flags slice. In the section around lines 87-90 in
pkg/package-server-manager/controller.go where the code checks if r.Interval is
not empty and appends "--interval" and r.Interval to flags, delete this entire
conditional block. Apply the same fix at lines 101-104 and lines 108-119 to
remove any similar interval flag appending. Keep only TLS-related flags in the
flags slice before passing it to ensureCSV, since the interval will be passed
separately to that function.
---
Nitpick comments:
In `@pkg/package-server-manager/controller_test.go`:
- Line 264: The test at the ensureCSV function call is still only testing the
nil path for CSV run flags. Add at least one test case to the test suite where
non-nil CSV run flags containing TLS parameters (such as --tls-min-version and
--tls-cipher-suites) are passed to the ensureCSV function, and verify that these
TLS flags are properly preserved in the resulting PackageServer command output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2d4ac933-857f-4c78-a0a5-bac94d1a2b9e
📒 Files selected for processing (4)
pkg/package-server-manager/config.gopkg/package-server-manager/controller.gopkg/package-server-manager/controller_test.gostaging/operator-lifecycle-manager/pkg/package-server/server/server.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/package-server-manager/config.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/package-server-manager/controller_test.go (1)
264-264: ⚡ Quick winAdd a non-nil flags case for the new
ensureCSVcontract.Passing only
nilpreserves the old behavior but does not cover the TLS flag path this PR adds. Add a case that passes--tls-min-version/--tls-cipher-suitesand asserts the PackageServer command contains them exactly once, in the expected order.Suggested test coverage direction
for _, tc := range tt { tc := tc t.Run(tc.name, func(t *testing.T) { - gotBool, gotErr := ensureCSV(logger, image, interval, nil, tc.inputCSV, tc.highlyAvailable) + gotBool, gotErr := ensureCSV(logger, image, interval, nil, tc.inputCSV, tc.highlyAvailable) require.EqualValues(t, tc.want.expectedBool, gotBool) require.EqualValues(t, tc.want.expectedErr, gotErr) require.EqualValues(t, tc.inputCSV.Spec, tc.expectedCSV.Spec) }) } + + t.Run("IncludesAdditionalRunFlags", func(t *testing.T) { + flags := []string{ + "--tls-min-version", "VersionTLS12", + "--tls-cipher-suites", "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", + } + inputCSV := newTestCSV() + + gotBool, gotErr := ensureCSV(logger, image, interval, flags, inputCSV, true) + require.NoError(t, gotErr) + require.True(t, gotBool) + + command := inputCSV.Spec.InstallStrategy.StrategySpec.DeploymentSpecs[0].Spec.Template.Spec.Containers[0].Command + require.Equal(t, 1, countOccurrences(command, "--tls-min-version")) + require.Equal(t, 1, countOccurrences(command, "--tls-cipher-suites")) + require.Contains(t, command, "VersionTLS12") + require.Contains(t, command, "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256") + }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/package-server-manager/controller_test.go` at line 264, The current test at line 264 only passes nil flags to the ensureCSV function, which does not exercise the TLS flag path newly added in this PR. Add a test case that calls ensureCSV with non-nil flags containing --tls-min-version and --tls-cipher-suites flags, and then assert that the resulting PackageServer command includes these flags exactly once each in the correct order. This ensures the new TLS flag handling logic is properly covered by tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/manifests/csv.yaml`:
- Line 8: The olm.version field is hardcoded with a commit-derived prerelease
version that differs from the standard versioning pattern used throughout the
codebase (such as 0.0.1-snapshot or release versions like 0.18.3). Regenerate
the CSV version using the proper release versioning flow and standard version
pattern instead of the hardcoded commit hash to ensure correct PackageServer CSV
upgrade ordering. This change applies to all instances of the hardcoded version
string in the manifest file.
- Around line 72-77: The APIServer RBAC rules across three files currently only
grant the `get` verb, but the controller registers a watch on configv1.APIServer
via SharedInformer which requires `list` and `watch` permissions. Update the
verbs array in all three locations to include these missing permissions. In
pkg/manifests/csv.yaml at lines 72-77, add `list` and `watch` verbs to the
apiservers rule. Apply the same change to
staging/operator-lifecycle-manager/deploy/chart/templates/_packageserver.clusterserviceversion.yaml
at lines 70-75, and to
staging/operator-lifecycle-manager/deploy/upstream/quickstart/olm.yaml at lines
267-272. Each location has the same RBAC block with apiGroups containing
"config.openshift.io" and resources containing "apiservers" that needs to be
updated.
---
Nitpick comments:
In `@pkg/package-server-manager/controller_test.go`:
- Line 264: The current test at line 264 only passes nil flags to the ensureCSV
function, which does not exercise the TLS flag path newly added in this PR. Add
a test case that calls ensureCSV with non-nil flags containing --tls-min-version
and --tls-cipher-suites flags, and then assert that the resulting PackageServer
command includes these flags exactly once each in the correct order. This
ensures the new TLS flag handling logic is properly covered by tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 118a19da-b64d-475e-9b9c-3f9a5bbeebaf
📒 Files selected for processing (7)
pkg/manifests/csv.yamlpkg/package-server-manager/config.gopkg/package-server-manager/controller.gopkg/package-server-manager/controller_test.gostaging/operator-lifecycle-manager/deploy/chart/templates/_packageserver.clusterserviceversion.yamlstaging/operator-lifecycle-manager/deploy/upstream/quickstart/olm.yamlstaging/operator-lifecycle-manager/pkg/package-server/server/server.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/package-server-manager/config.go
- staging/operator-lifecycle-manager/pkg/package-server/server/server.go
- pkg/package-server-manager/controller.go
| namespace: openshift-operator-lifecycle-manager | ||
| labels: | ||
| olm.version: 0.0.1-snapshot | ||
| olm.version: 0.0.0-69a5c8cdf920607f2c125bdf0d9e644b8f980dc6 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find committed PackageServer CSV version stamps and check whether this
# commit-derived version is isolated to generated artifacts or part of the intended release flow.
rg -n --iglob '*.yaml' --iglob '*.yml' \
'olm\.version:|^[[:space:]]*version: 0\.0\.0-[0-9a-f]{40}|0\.0\.1-snapshot|0\.0\.0-69a5c8cdf920607f2c125bdf0d9e644b8f980dc6'Repository: openshift/operator-framework-olm
Length of output: 2849
Regenerate the CSV version using the intended release versioning flow.
Lines 8 and 171 hardcode a commit-derived prerelease version (0.0.0-69a5c8cdf920607f2c125bdf0d9e644b8f980dc6) that differs from the standard pattern used elsewhere (0.0.1-snapshot in manifests, {{ .Chart.Version }} in Helm templates, 0.18.3 in upstream releases). This isolated version stamp suggests it came from a local test build and will unintentionally affect PackageServer CSV upgrade ordering.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/manifests/csv.yaml` at line 8, The olm.version field is hardcoded with a
commit-derived prerelease version that differs from the standard versioning
pattern used throughout the codebase (such as 0.0.1-snapshot or release versions
like 0.18.3). Regenerate the CSV version using the proper release versioning
flow and standard version pattern instead of the hardcoded commit hash to ensure
correct PackageServer CSV upgrade ordering. This change applies to all instances
of the hardcoded version string in the manifest file.
Comment on lines
+72
to
+77
| - apiGroups: | ||
| - "config.openshift.io" | ||
| resources: | ||
| - apiservers | ||
| verbs: | ||
| - get |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm the ServiceAccount that runs the APIServer watcher has
# list/watch on config.openshift.io/apiservers, not only get.
echo "Controller APIServer watch/read sites:"
rg -n -C5 --type=go 'configv1\.APIServer|Watches\(.*APIServer|For\(.*APIServer'
echo
echo "RBAC rules mentioning config.openshift.io/apiservers:"
rg -n -C6 --iglob '*.yaml' --iglob '*.yml' 'config\.openshift\.io|apiservers|olm-operator-serviceaccount'Repository: openshift/operator-framework-olm
Length of output: 50389
🏁 Script executed:
echo "=== pkg/manifests/csv.yaml (lines 60-85) ==="
sed -n '60,85p' pkg/manifests/csv.yaml
echo -e "\n=== staging/operator-lifecycle-manager/deploy/chart/templates/_packageserver.clusterserviceversion.yaml (lines 58-85) ==="
sed -n '58,85p' staging/operator-lifecycle-manager/deploy/chart/templates/_packageserver.clusterserviceversion.yaml
echo -e "\n=== staging/operator-lifecycle-manager/deploy/upstream/quickstart/olm.yaml (lines 255-280) ==="
sed -n '255,280p' staging/operator-lifecycle-manager/deploy/upstream/quickstart/olm.yaml
echo -e "\n=== Check which ServiceAccount runs the manager in controller.go ==="
rg -n 'ServiceAccountName|ServiceAccount' pkg/package-server-manager/controller.go -A 2 -B 2 | head -30Repository: openshift/operator-framework-olm
Length of output: 2768
Add list and watch verbs to the APIServer RBAC rules. The added rules grant only get, which works for direct reads, but the controller registers a watch on configv1.APIServer (controller.go:201) via a SharedInformer. Informers require list and watch permissions to function.
Update all three RBAC definitions to include these verbs:
pkg/manifests/csv.yaml#L72-L77staging/operator-lifecycle-manager/deploy/chart/templates/_packageserver.clusterserviceversion.yaml#L70-L75staging/operator-lifecycle-manager/deploy/upstream/quickstart/olm.yaml#L267-L272
Example fix for pkg/manifests/csv.yaml
- apiGroups:
- "config.openshift.io"
resources:
- apiservers
verbs:
- get
- list
- watch📍 Affects 3 files
pkg/manifests/csv.yaml#L72-L77(this comment)staging/operator-lifecycle-manager/deploy/chart/templates/_packageserver.clusterserviceversion.yaml#L70-L75staging/operator-lifecycle-manager/deploy/upstream/quickstart/olm.yaml#L267-L272
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/manifests/csv.yaml` around lines 72 - 77, The APIServer RBAC rules across
three files currently only grant the `get` verb, but the controller registers a
watch on configv1.APIServer via SharedInformer which requires `list` and `watch`
permissions. Update the verbs array in all three locations to include these
missing permissions. In pkg/manifests/csv.yaml at lines 72-77, add `list` and
`watch` verbs to the apiservers rule. Apply the same change to
staging/operator-lifecycle-manager/deploy/chart/templates/_packageserver.clusterserviceversion.yaml
at lines 70-75, and to
staging/operator-lifecycle-manager/deploy/upstream/quickstart/olm.yaml at lines
267-272. Each location has the same RBAC block with apiGroups containing
"config.openshift.io" and resources containing "apiservers" that needs to be
updated.
Contributor
Author
|
The upstream commit is also part of operator-framework/operator-lifecycle-manager#3849 |
tmshort
changed the title
openshift-ci-robot
added
jira/severity-important
|
@tmshort: This pull request references Jira Issue OCPBUGS-88737, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Signed-off-by: Todd Short <tshort@redhat.com>
|
@tmshort: This pull request references Jira Issue OCPBUGS-88737, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Contributor
|
@tmshort: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
config.openshift.ioapiservers(get) to the PackageServer ClusterServiceVersion, including Helm chart and quickstart manifests.