OCPCLOUD-3261: feat(cloud providers): inject centralized TLS configuration#8864
OCPCLOUD-3261: feat(cloud providers): inject centralized TLS configuration#8864ingvagabund wants to merge 3 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
do-not-merge/work-in-progress
|
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:
📝 WalkthroughWalkthroughAcross the cloud-controller-manager adapters and hosted-cluster platform deployment specs, container args now include TLS minimum version and cipher suite flags derived from the HostedControlPlane TLS security profile. Several deployment builders now construct args slices dynamically, and some component builders register deployment adaptation hooks to apply the changes. Sequence Diagram(s)sequenceDiagram
participant HostedControlPlane
participant PlatformDeploymentSpec
participant Deployment
HostedControlPlane->>PlatformDeploymentSpec: provide TLS security profile
PlatformDeploymentSpec->>PlatformDeploymentSpec: build args slice
PlatformDeploymentSpec->>Deployment: set Args with TLS flags
Suggested reviewers
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ingvagabund 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 |
openshift-ci
Bot
added
area/control-plane-operator
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/aws/component.go (1)
63-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFactor the TLS arg assembly into a shared helper.
This exact
MinTLSVersion/CipherSuitesappend block is now copied across all six CCM providers in this PR, so the next TLS flag change will be easy to miss in one of them. A small helper in the cloud-controller-manager package would keep the behavior aligned everywhere.🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/aws/component.go` around lines 63 - 70, The TLS flag assembly in the cloud-controller-manager provider setup is duplicated across the CCM implementations, so factor the repeated MinTLSVersion/CipherSuites append logic into a shared helper in the cloud-controller-manager package. Update the component wiring to call that helper from the AWS path (and the other provider component files) so all providers build TLS args consistently from hcp.Spec.Configuration.GetTLSSecurityProfile().
🤖 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.
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/aws/component.go`:
- Around line 63-70: The TLS flag assembly in the cloud-controller-manager
provider setup is duplicated across the CCM implementations, so factor the
repeated MinTLSVersion/CipherSuites append logic into a shared helper in the
cloud-controller-manager package. Update the component wiring to call that
helper from the AWS path (and the other provider component files) so all
providers build TLS args consistently from
hcp.Spec.Configuration.GetTLSSecurityProfile().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8700e9d8-9b45-47e5-8557-59d3cd06c5e7
📒 Files selected for processing (6)
control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/aws/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/gcp/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/kubevirt/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/openstack/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/powervs/deployment.go
ingvagabund
force-pushed
the
aws-cloud-controller-manager-tls
branch
from
b45a4de to
758f5be
Compare
ingvagabund
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@ingvagabund: This pull request references OCPCLOUD-3261 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 epic to target either version "5.0." or "openshift-5.0.", but it targets "openshift-4.22" instead. 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
removed
the
do-not-merge/work-in-progress
ingvagabund
force-pushed
the
aws-cloud-controller-manager-tls
branch
from
758f5be to
21b3b69
Compare
openshift-ci
Bot
added
the
area/hypershift-operator
ingvagabund
force-pushed
the
aws-cloud-controller-manager-tls
branch
from
e17a807 to
9fce135
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hypershift-operator/controllers/hostedcluster/internal/platform/agent/agent.go (1)
77-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the TLS arg append into a shared helper.
This exact
hcp→config.MinTLSVersion/config.CipherSuitesblock is now copied across all seven provider specs. Centralizing it would reduce drift the next time the TLS flag contract changes.🤖 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 `@hypershift-operator/controllers/hostedcluster/internal/platform/agent/agent.go` around lines 77 - 94, The TLS flag assembly in the agent args setup is duplicated across provider specs, so extract the repeated hcp-based TLS append logic into a shared helper and reuse it from the agent container arg builder. Move the block that calls config.MinTLSVersion and config.CipherSuites into a common function with a clear name, then have the existing arg construction path in agent.go call that helper after the base args are created. Keep the helper responsible only for appending the TLS-related flags so all provider specs share the same contract.
🤖 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.
Nitpick comments:
In
`@hypershift-operator/controllers/hostedcluster/internal/platform/agent/agent.go`:
- Around line 77-94: The TLS flag assembly in the agent args setup is duplicated
across provider specs, so extract the repeated hcp-based TLS append logic into a
shared helper and reuse it from the agent container arg builder. Move the block
that calls config.MinTLSVersion and config.CipherSuites into a common function
with a clear name, then have the existing arg construction path in agent.go call
that helper after the base args are created. Keep the helper responsible only
for appending the TLS-related flags so all provider specs share the same
contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: aca41bfd-9b08-4ded-ad68-5126f0f9527d
📒 Files selected for processing (7)
hypershift-operator/controllers/hostedcluster/internal/platform/agent/agent.gohypershift-operator/controllers/hostedcluster/internal/platform/aws/aws.gohypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.gohypershift-operator/controllers/hostedcluster/internal/platform/gcp/gcp.gohypershift-operator/controllers/hostedcluster/internal/platform/kubevirt/kubevirt.gohypershift-operator/controllers/hostedcluster/internal/platform/openstack/openstack.gohypershift-operator/controllers/hostedcluster/internal/platform/powervs/powervs.go
ingvagabund
changed the title
cblecker
left a comment
cblecker
left a comment
There was a problem hiding this comment.
Thanks for adding TLS security profile support across the CCM and CAPI providers — the implementation is clean and the fixture coverage is thorough.
One thing I'd like to see addressed (either in this PR or a follow-up): there's no enforcement mechanism that validates all components that should honor the centralized TLS configuration actually do. Today it's purely manual — if someone adds a new component that serves HTTPS, nothing flags the missing TLS configuration. This PR adds the same pattern to 13 files independently, and the next component won't know it needs to do the same.
A lightweight option would be a cross-component test that asserts every deployment with an HTTPS-serving container has --tls-min-version in its args or equivalent config. This would catch regressions and make the requirement discoverable. An even better option would be a helper (e.g., config.TLSFlags(profile)) that the component framework could apply automatically, but that's a larger change.
Would you be open to adding a tracking issue for this if it's out of scope here?
ingvagabund
force-pushed
the
aws-cloud-controller-manager-tls
branch
from
ffc14df to
6abac17
Compare
|
Hello @cblecker. Thank you for taking a closer look at the PR. So far the changes are luckily quite straightforward. There's a new tls-scanner periodic job for a HS introduced. The latest run: https://prow.ci.openshift.org/view/gs/test-platform-results/logs/periodic-ci-openshift-tls-scanner-main-periodic-hypershift-tls/2070729428597477376 (from Jun 27). Would that be a sufficient gating mechanism? |
openshift-ci
Bot
added
the
needs-rebase
Inject --tls-min-version and --tls-cipher-suites flags to all six cloud controller manager providers based on HCP TLS security profile.
ingvagabund
force-pushed
the
aws-cloud-controller-manager-tls
branch
from
6abac17 to
2f045ee
Compare
|
Just rebasing |
openshift-ci
Bot
removed
the
needs-rebase
Inject --tls-min-version and --tls-cipher-suites flags to all six cloud capi providers based on HCP TLS security profile.
Inject --tls-min-version and --tls-cipher-suites flags to capi-manager based on HCP TLS security profile.
ingvagabund
force-pushed
the
aws-cloud-controller-manager-tls
branch
from
2f045ee to
c4ef20b
Compare
|
@ingvagabund: all tests passed! 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. |
|
Now I have all the information needed. Let me compile the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryBoth Codecov checks failed because PR #8864 adds centralized TLS configuration injection ( Root CauseThe PR adds identical TLS configuration logic to two categories of files, neither of which has adequate unit test coverage for the new code: Category 1: Control-plane-operator CCM deployment files (v2/ cloud_controller_manager)
These files build Category 2: Hypershift-operator CAPI provider deployment spec files (platform/)
These Only the The root cause is missing unit tests for the TLS injection paths in 10 of the 12 modified Go files. The code is functionally correct (the updated fixture YAMLs confirm the args are generated properly), but Codecov's line-level coverage tracking shows the test suite never executes these specific lines. Recommendations
Evidence
|
3 participants
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit