TRT-2761: Undo revert and fix applyClusterTLSProfile()#1335
Open
tmshort wants to merge 2 commits into
Open
Conversation
…up (#3860) In HyperShift hosted clusters the API server is not ready during initial packageserver startup, causing applyClusterTLSProfile() to timeout and return a fatal error, crash-looping the packageserver. Change error handling to warn-and-continue so packageserver starts with secure TLS defaults. The PSM will inject the correct --tls-min-version / --tls-cipher-suites flags via the CSV once the API becomes available, triggering a clean restart with the cluster-configured profile. Signed-off-by: Todd Short <tshort@redhat.com> Upstream-repository: operator-lifecycle-manager Upstream-commit: 601f296cf140af32c687109b065df1dbdcd2943d
openshift-ci-robot
added
the
jira/valid-reference
|
@tmshort: This pull request references TRT-2761 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. 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. |
WalkthroughAdds cluster TLS profile auto-detection to the package-server: at startup, if ChangesCluster TLS Profile Auto-Detection
Dependency and mapstructure/hashstructure migration
CI actions/checkout v7 upgrade
Sequence Diagram(s)sequenceDiagram
participant Run as PackageServerOptions.Run
participant applyClusterTLSProfile
participant applyClusterTLSProfileWithClients
participant DiscoveryClient
participant ConfigV1Client as ConfigV1Client (OpenShift)
Run->>Run: build rest.Config from kubeconfig/in-cluster
alt MinTLSVersion unset
Run->>applyClusterTLSProfile: rest.Config
applyClusterTLSProfile->>applyClusterTLSProfileWithClients: k8s + configv1 clients (10s timeout)
applyClusterTLSProfileWithClients->>DiscoveryClient: check config.openshift.io API group
alt API group absent (non-OpenShift)
applyClusterTLSProfileWithClients-->>Run: no-op, no error
else API group present
applyClusterTLSProfileWithClients->>ConfigV1Client: get APIServer/cluster
ConfigV1Client-->>applyClusterTLSProfileWithClients: TLS security profile
applyClusterTLSProfileWithClients->>Run: set MinTLSVersion + CipherSuites on SecureServing
end
end
Run->>Run: o.Config(ctx), EnableMetrics, start API server
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ 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: 1
🤖 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 225-236: The fallback guard in server.go only checks
o.SecureServing.MinTLSVersion, but applyClusterTLSProfile merges MinTLSVersion
and CipherSuites independently, so the cluster profile is skipped when only
--tls-cipher-suites is missing. Update the condition in the package-server
startup path to run the fallback whenever either SecureServing.MinTLSVersion or
SecureServing.CipherSuites is unset, while keeping the
applyClusterTLSProfile(ctx, clientConfig, o.SecureServing) call and its warning
handling unchanged.
🪄 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: 4d6dcd6e-0c78-4dc4-8a50-35ade49e7fa3
⛔ Files ignored due to path filters (53)
go.sumis excluded by!**/*.sumstaging/operator-lifecycle-manager/go.sumis excluded by!**/*.sumvendor/github.com/containerd/containerd/labels/labels.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/containerd/containerd/labels/validate.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/containerd/containerd/version/version.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-viper/mapstructure/v2/.editorconfigis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-viper/mapstructure/v2/.envrcis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-viper/mapstructure/v2/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-viper/mapstructure/v2/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-viper/mapstructure/v2/CHANGELOG.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-viper/mapstructure/v2/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-viper/mapstructure/v2/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-viper/mapstructure/v2/decode_hooks.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-viper/mapstructure/v2/devenv.lockis excluded by!**/*.lock,!**/vendor/**,!vendor/**vendor/github.com/go-viper/mapstructure/v2/devenv.nixis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-viper/mapstructure/v2/devenv.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-viper/mapstructure/v2/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-viper/mapstructure/v2/internal/errors/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-viper/mapstructure/v2/internal/errors/join.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-viper/mapstructure/v2/internal/errors/join_go1_19.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-viper/mapstructure/v2/mapstructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-viper/mapstructure/v2/reflect_go1_19.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-viper/mapstructure/v2/reflect_go1_20.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mitchellh/hashstructure/v2/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/mitchellh/hashstructure/v2/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/mitchellh/hashstructure/v2/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mitchellh/hashstructure/v2/hashstructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mitchellh/hashstructure/v2/include.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mitchellh/mapstructure/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/mitchellh/mapstructure/decode_hooks.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mitchellh/mapstructure/error.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/run/run_command.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/watch/watch_command.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/reporters/default_reporter.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/types/config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/types/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/ginkgo/v2/types/version.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/decorators/operator.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/lib/codec/mapstructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/lib/comparison/equal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/package-server/server/server.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/expfmt/expfmt.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/expfmt/text_create.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/expfmt/text_parse.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/model/labels.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/model/labelset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/model/metric.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/model/time.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/model/value.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/model/value_float.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/model/value_histogram.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/api/validation/objectmeta.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (16)
go.modpkg/manifests/csv.yamlstaging/operator-lifecycle-manager/.github/workflows/e2e-tests.ymlstaging/operator-lifecycle-manager/.github/workflows/go-verdiff.yamlstaging/operator-lifecycle-manager/.github/workflows/goreleaser.yamlstaging/operator-lifecycle-manager/.github/workflows/quickstart.ymlstaging/operator-lifecycle-manager/.github/workflows/sanity.yamlstaging/operator-lifecycle-manager/.github/workflows/unit.ymlstaging/operator-lifecycle-manager/deploy/chart/templates/_packageserver.clusterserviceversion.yamlstaging/operator-lifecycle-manager/deploy/upstream/quickstart/olm.yamlstaging/operator-lifecycle-manager/go.modstaging/operator-lifecycle-manager/pkg/controller/operators/decorators/operator.gostaging/operator-lifecycle-manager/pkg/lib/codec/mapstructure.gostaging/operator-lifecycle-manager/pkg/lib/comparison/equal.gostaging/operator-lifecycle-manager/pkg/package-server/server/server.gostaging/operator-lifecycle-manager/pkg/package-server/server/server_test.go
Comment on lines
+225
to
+236
| // If --tls-min-version was not supplied (e.g. no PSM-injected flags yet), fall | ||
| // back to a direct GET of the cluster APIServer CR so the packageserver still | ||
| // honours the cluster TLS security profile on first boot or during upgrades. | ||
| if o.SecureServing.MinTLSVersion == "" { | ||
| // Warn and continue on failure: the API server may not be reachable | ||
| // during initial startup (e.g. cluster bootstrap). Packageserver starts | ||
| // with secure TLS defaults; operators can supply --tls-min-version and | ||
| // --tls-cipher-suites flags to override the profile on a later restart. | ||
| if err := applyClusterTLSProfile(ctx, clientConfig, o.SecureServing); err != nil { | ||
| log.WithError(err).Warn("Failed to apply cluster TLS profile to serving options, using defaults") | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Call the fallback when either TLS field is unset.
This guard only looks at MinTLSVersion, but the helper below merges MinTLSVersion and CipherSuites independently. If someone supplies --tls-min-version and leaves --tls-cipher-suites unset, we skip the fallback entirely and never pick up the cluster profile's cipher suites.
Suggested fix
- if o.SecureServing.MinTLSVersion == "" {
+ if o.SecureServing.MinTLSVersion == "" || len(o.SecureServing.CipherSuites) == 0 {
// Warn and continue on failure: the API server may not be reachable
// during initial startup (e.g. cluster bootstrap). Packageserver starts
// with secure TLS defaults; operators can supply --tls-min-version and📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // If --tls-min-version was not supplied (e.g. no PSM-injected flags yet), fall | |
| // back to a direct GET of the cluster APIServer CR so the packageserver still | |
| // honours the cluster TLS security profile on first boot or during upgrades. | |
| if o.SecureServing.MinTLSVersion == "" { | |
| // Warn and continue on failure: the API server may not be reachable | |
| // during initial startup (e.g. cluster bootstrap). Packageserver starts | |
| // with secure TLS defaults; operators can supply --tls-min-version and | |
| // --tls-cipher-suites flags to override the profile on a later restart. | |
| if err := applyClusterTLSProfile(ctx, clientConfig, o.SecureServing); err != nil { | |
| log.WithError(err).Warn("Failed to apply cluster TLS profile to serving options, using defaults") | |
| } | |
| } | |
| // If --tls-min-version was not supplied (e.g. no PSM-injected flags yet), fall | |
| // back to a direct GET of the cluster APIServer CR so the packageserver still | |
| // honours the cluster TLS security profile on first boot or during upgrades. | |
| if o.SecureServing.MinTLSVersion == "" || len(o.SecureServing.CipherSuites) == 0 { | |
| // Warn and continue on failure: the API server may not be reachable | |
| // during initial startup (e.g. cluster bootstrap). Packageserver starts | |
| // with secure TLS defaults; operators can supply --tls-min-version and | |
| // --tls-cipher-suites flags to override the profile on a later restart. | |
| if err := applyClusterTLSProfile(ctx, clientConfig, o.SecureServing); err != nil { | |
| log.WithError(err).Warn("Failed to apply cluster TLS profile to serving options, using defaults") | |
| } | |
| } |
🤖 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 `@staging/operator-lifecycle-manager/pkg/package-server/server/server.go`
around lines 225 - 236, The fallback guard in server.go only checks
o.SecureServing.MinTLSVersion, but applyClusterTLSProfile merges MinTLSVersion
and CipherSuites independently, so the cluster profile is skipped when only
--tls-cipher-suites is missing. Update the condition in the package-server
startup path to run the fallback whenever either SecureServing.MinTLSVersion or
SecureServing.CipherSuites is unset, while keeping the
applyClusterTLSProfile(ctx, clientConfig, o.SecureServing) call and its warning
handling unchanged.
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.
This undoes the revert #1334 (which reverted #1330). The downstream sync is pulled back in, and the fix operator-framework/operator-lifecycle-manager#3860 is downstreamed in. This should be the only necessary fix, and the only commit that needs to be downstreamed.
This has to be a manual downstream due to the revert. Following this, subsequent automatic syncing should succeed.
Alternative to #1333
Summary by CodeRabbit
New Features
Bug Fixes