Repository navigation
NO-JIRA: Gate control-plane TLS options by payload version - #9626
stevekuznetsov wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@stevekuznetsov: This pull request explicitly references no jira issue. 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
|
Skipping CI for Draft Pull Request. |
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (28)
📒 Files selected for processing (9)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughDeployment adapters now parse the control-plane release version and return errors for invalid or empty versions. CVO and machine approver TLS arguments apply only to supported releases. KAS applies version-specific TLS behavior to KAS, AWS, and Azure components. Registry operator removes its configuration argument for OpenShift 4.21 and older releases. Tests now cover release versions, TLS profiles, argument presence, and parsing errors. Sequence Diagram(s)sequenceDiagram
participant ReleaseImageProvider
participant DeploymentAdapter
participant TLSConfiguration
participant Deployment
ReleaseImageProvider->>DeploymentAdapter: provide release version
DeploymentAdapter->>DeploymentAdapter: parse semantic version
DeploymentAdapter->>TLSConfiguration: evaluate version and TLS profile
TLSConfiguration->>Deployment: add supported arguments
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The compatibility gates align with the stated payload-version behavior, and no actionable merge-blocking risk remains. Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 passed)
Full details: Stable And Deterministic Test NamesExplanation A newly added subtest title includes a date and timestamp. In Resolution Use stable, descriptive case names for subtests. Keep the full release version, including nightly dates, only in the test data and assertions. For example, add a static Full details: Test Structure And QualityExplanation The changed unit tests use Gomega assertions without meaningful failure messages, including the exact discouraged pattern from the check. Examples include Resolution Add meaningful failure messages to every new Gomega assertion. Include the release version, TLS profile, component, and expected flag where relevant. For example, replace bare error assertions with messages such as
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/control-plane-operator
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: stevekuznetsov The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9626 +/- ##
==========================================
+ Coverage 47.54% 47.59% +0.04%
==========================================
Files 796 796
Lines 100027 100057 +30
==========================================
+ Hits 47560 47619 +59
+ Misses 49298 49268 -30
- Partials 3169 3170 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…ride The performance experiment paired a current custom CPO with older 4.21 payload binaries, causing unsupported TLS flags and registry configuration to be emitted. This addresses overridden CPO/payload skew, not a demonstrated failure of normal payload-matched HyperShift. A newer HO alone does not cause this mismatch: CPO and management-side binaries normally come from the same control-plane release payload. Ordinary upgrades render target images and arguments together. Gate options by the control-plane payload version and retain older shipped defaults. Older binaries still cannot enforce every centralized TLS profile, including Modern. Version/profile tests and fixtures cover these specific mismatches, not general support for arbitrary main-CPO/older-payload combinations. An upstream support requirement for this override compatibility remains to be established. Signed-off-by: Steve Kuznetsov <stekuznetsov@microsoft.com>
79bb76d to
d6b493f
Compare
|
Stale PRs are closed after 21d of inactivity. If this PR is still relevant, comment to refresh it or remove the stale label. If this PR is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
lifecycle/stale
|
PR needs rebase. 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. |
This branch has not been deployed
2 participants
What this PR does / why we need it:
Scope: compatibility with an overridden, mismatched CPO; not a demonstrated fix required for normal payload-matched HyperShift operation.
The performance experiment paired a current custom CPO with older 4.21 payload binaries. The newer rendering code emitted TLS options/configuration that those binaries do not support. The audit documents that pairing, but the original launch command and exact deployed attempt artifact were unavailable; the particular harness override invocation is not proven.
Normally, HO selects CPO from the effective control-plane payload, and CPO selects management-side binaries from that same payload. A newer HO managing an older cluster does not itself cause this mismatch. Ordinary upgrades render target images and arguments together; temporarily running older pods is not evidence that these gates are necessary.
Consequently, this PR should be evaluated as accommodation for forced/custom CPO version skew, not as a general production regression fix. Release-specific production hotfix overrides are another possible source of decoupling, but do not imply arbitrary main-CPO/older-payload support. No concrete normal-operation requirement for these TLS gates has been established. Keep this draft pending agreement that this override compatibility belongs upstream.
The implementation gates against ReleaseImageProvider.Version(), not the potentially different user payload version:
Preserve supported konnectivity cipher arguments, existing certificates/file watches, and older shipped defaults. Add version/profile matrices including split payloads and regenerate 28 affected deployment fixtures. This does not make arbitrary CPO/payload skew supported or restore unavailable TLS-profile enforcement.
Code references: CPO image selection, release reconciliation guard.
Which issue(s) this PR fixes:
No Jira issue associated yet.
Special notes for your reviewer:
This restores specific startup compatibility, not equivalent centralized TLS-profile enforcement on older binaries. In particular, older konnectivity retains TLS 1.2 even for Modern, and older Azure retains its shipped default. Newer binaries retain configured profile controls. No plaintext transport workaround, resource policy, budgets, replicas, or NodePool hash changes are introduced. CPO adoption can roll affected control-plane deployments.
Feature source references:
The prior experiment audit recorded 4.21.32/4.22 binary checks and source-based 4.23 evidence. Those binary checks were not rerun during extraction. Minor-version gates do not certify every early nightly or overridden image, or general current-CPO compatibility with older releases.
Validation passed locally with vendored modules and bounded Go parallelism:
go test -race -count=1 ./control-plane-operator/controllers/hostedcontrolplane/v2/{cvo,kas,machine_approver,registryoperator}go test -race -count=1 -run "^TestControlPlaneComponents$" ./control-plane-operator/controllers/hostedcontrolplane(full fixture verification without update/filter)make lint(API and main modules: zero issues)make run-gitlint,git diff --check, andgit verify-commit HEADThe attempted full pre-commit gate completed binary/e2e builds but was cancelled during repository-wide test compilation. Full pre-commit and live AWS/Azure bring-up/upgrade validation remain incomplete. Keep draft pending remaining validation. One signed-off, GPG-signed commit.
Checklist:
Summary by CodeRabbit
Compatibility
Bug Fixes