OCPCLOUD-3347: feat(machine-approver): add TLS configuration options to deployment manifests - #8019
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
do-not-merge/needs-area
area/control-plane-operator
damdo
changed the title
damdo
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@damdo: This pull request references OCPCLOUD-3347 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 story to target the "4.22.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. |
|
@damdo: This pull request references OCPCLOUD-3347 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 story to target the "4.22.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. |
|
@damdo: This pull request references OCPCLOUD-3347 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 story to target the "4.22.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. |
|
@damdo: This PR was included in a payload test run from openshift/cluster-machine-approver#292 |
1 similar comment
|
@damdo: This PR was included in a payload test run from openshift/cluster-machine-approver#292 |
|
/testwith openshift/cluster-machine-approver/main/e2e-hypershift-aws openshift/cluster-machine-approver#292 |
|
@damdo, |
|
@sunzhaohua2: This PR was included in a payload test run from openshift/cluster-machine-approver#292
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/15948fb0-244f-11f1-83e2-50fda993a409-0 |
|
@sunzhaohua2: This PR was included in a payload test run from openshift/cluster-machine-approver#292
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/15022440-267f-11f1-9a65-bf72365d4b33-0 |
…anifests Added `--tls-min-version` and `--tls-cipher-suites` arguments to the machine approver deployment configurations across multiple environments to enhance security settings.
|
/reopen |
|
reopen |
|
@damdo: Failed to re-open PR: state cannot be changed. There are no new commits on the damdo:main branch. 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 kubernetes-sigs/prow repository. |
|
@damdo: This pull request references OCPCLOUD-3347 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 story to target the "4.22.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. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/assign @sjenning @csrwng @joelanford |
|
/assign @wewang58 For testing |
|
@damdo: This pull request references OCPCLOUD-3347 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 story to target the "4.22.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. |
|
@damdo: This pull request references OCPCLOUD-3347 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 story to target the "4.22.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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: damdo 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: damdo 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 #8019 +/- ##
==========================================
- Coverage 29.96% 29.96% -0.01%
==========================================
Files 1049 1049
Lines 97523 97530 +7
==========================================
Hits 29225 29225
- Misses 65796 65803 +7
Partials 2502 2502
🚀 New features to boost your workflow:
|
|
/pipeline required |
|
Scheduling tests matching the |
|
/retest-required |
|
@damdo: 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. |
|
@sunzhaohua2 e2e jobs are expected to fail as Cluster Machine Approver doesn't have the flags until openshift/cluster-machine-approver#292 merges. |
|
/verified by @wewang58 |
openshift-ci-robot
added
the
verified
|
@wewang58: This PR has been marked as verified by 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. |
|
/hold until openshift/cluster-machine-approver#292 merges |
openshift-ci
Bot
added
the
do-not-merge/hold
|
Now I have all the information needed. Let me compile the final structured report. Test Failure Analysis Complete (Multi-Step)Job Information
Root CauseThe machine-approver binary does not support PR #8019 adds TLS configuration arguments to the machine-approver deployment manifest in if tlsMinVersion := config.MinTLSVersion(configuration.GetTLSSecurityProfile()); tlsMinVersion != "" {
c.Args = append(c.Args, fmt.Sprintf("--tls-min-version=%s", tlsMinVersion))
}
if cipherSuites := config.CipherSuites(configuration.GetTLSSecurityProfile()); len(cipherSuites) != 0 {
c.Args = append(c.Args, fmt.Sprintf("--tls-cipher-suites=%s", strings.Join(cipherSuites, ",")))
}This follows the same pattern used by Result: Every machine-approver pod across every test cluster crashes immediately on startup with:
This causes the HostedCluster to never reach All tests that create a HostedCluster then time out waiting for it to become available. Per-Job Failure Summary
Detailed AnalysisJobs 1–5: machine-approver crash-loop (Same Root Cause)Failure chain:
25 tests failed in e2e-aws alone, including: Job 6: e2e-kubevirt-aws-ovn-reduced (Different Root Cause)This job timed out during health checks with a node count mismatch — nested cluster nodes did not match desired NodePool replicas. No machine-approver errors were found. This appears to be an unrelated infrastructure/timing issue, possibly a pre-existing flake in the KubeVirt provider. EvidenceSource Code (deployment.go)// control-plane-operator/controllers/hostedcontrolplane/v2/machine_approver/deployment.go
func adaptDeployment(cpContext component.WorkloadContext, deployment *appsv1.Deployment) error {
hcp := cpContext.HCP
configuration := hcp.Spec.Configuration
util.UpdateContainer(ComponentName, deployment.Spec.Template.Spec.Containers, func(c *corev1.Container) {
c.Args = append(c.Args, fmt.Sprintf("--machine-namespace=%s", hcp.Namespace))
// ⬇ These flags are NOT supported by the current machine-approver binary
if tlsMinVersion := config.MinTLSVersion(configuration.GetTLSSecurityProfile()); tlsMinVersion != "" {
c.Args = append(c.Args, fmt.Sprintf("--tls-min-version=%s", tlsMinVersion))
}
if cipherSuites := config.CipherSuites(configuration.GetTLSSecurityProfile()); len(cipherSuites) != 0 {
c.Args = append(c.Args, fmt.Sprintf("--tls-cipher-suites=%s", strings.Join(cipherSuites, ",")))
}
})
return nil
}Container crash logPod status (from must-gather artifacts)containerStatuses:
- name: machine-approver
restartCount: 14
state:
terminated:
exitCode: 2
reason: Error
lastState:
terminated:
exitCode: 2
reason: ErrorSupported machine-approver flags (current binary)Note: Comparison with components that DO support TLS flagsThe same TLS flag pattern is used in Root Cause HypothesisPrimary cause: PR #8019 adds TLS CLI flags ( This is a deterministic, reproducible bug affecting every job that creates a HostedCluster with the default TLS security profile (which resolves to Recommendations
|
|
I now have the complete picture across all 6 Prow jobs. The root cause is identical and conclusive across every single job. Let me compile the final structured report. Test Failure Analysis Complete (Multi-Job)Job Information
Root CausePR #8019 adds The companion PR that adds flag support to the binary — openshift/cluster-machine-approver#292 ("OCPCLOUD-3347: tls: use centralized TLS profile (unrevert)") — is still OPEN and not merged. The CI release images therefore contain a ErrorContainer exits with code 2 immediately on startup. Failure Chain (identical across all 6 jobs, all platforms: AWS, Azure, KubeVirt)
Evidencemachine-approver Pod Status (consistent across all jobs)state:
waiting:
message: back-off 5m0s restarting failed container=machine-approver
reason: CrashLoopBackOff
restartCount: 10-15
lastState:
terminated:
exitCode: 2
reason: Error
message: "unknown flag: --tls-min-version"machine-approver Container LogHostedCluster Conditions (consistent across all jobs)- type: Available
status: "False"
reason: ComponentsNotAvailable
message: "Waiting for components to be available: machine-approver"
- type: Degraded
status: "True"
reason: UnavailableReplicas
message: "machine-approver deployment has 1 unavailable replicas"
- type: DataPlaneConnectionAvailable
status: Unknown
reason: NoWorkerNodesAvailable
message: "No worker nodes available"machine-approver Deployment Args (from pod YAML in artifacts)args:
- --machine-namespace=clusters-<cluster-name>
- --tls-min-version=VersionTLS12
- --tls-cipher-suites=TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,...Upstream Dependency Status
Per-Job Failure Summarye2e-v2-aws (Build
|
| Check | Likely Cause |
|---|---|
| Red Hat Konflux | Build/image issue — separate from the runtime test failures |
| codecov/patch | Test coverage threshold not met — unrelated to the functional failures |
Root Cause Hypothesis
This is a sequencing/dependency issue, not a code defect. PR #8019 correctly implements the TLS flag passthrough pattern (matching what kcm, kube-scheduler, and oauth-apiserver already do), but it depends on openshift/cluster-machine-approver#292 being merged first so the binary actually accepts the flags. The upstream PR is still open.
The failure is deterministic and 100% reproducible — it affects every single hosted cluster creation across all platforms (AWS, Azure, KubeVirt) because the machine-approver deployment always receives the unsupported flags and always crashes.
Recommendations
- Do not merge PR OCPCLOUD-3347: feat(machine-approver): add TLS configuration options to deployment manifests #8019 until openshift/cluster-machine-approver#292 is merged and the updated
cluster-machine-approverimage is available in the CI release payload - The PR already has a
do-not-merge/holdlabel — this is appropriate - Alternatively, add version gating to PR OCPCLOUD-3347: feat(machine-approver): add TLS configuration options to deployment manifests #8019 so the flags are only passed when the machine-approver image supports them (no other TLS flag additions in the codebase have such gating, so this may not be the preferred pattern)
- Once the upstream PR is merged, the CI release images need to be rebuilt with the new
cluster-machine-approverbinary before re-testing PR OCPCLOUD-3347: feat(machine-approver): add TLS configuration options to deployment manifests #8019
Artifacts
- e2e-v2-aws:
.work/prow-job-analyze-test-failure/2039885042146086912/logs/ - e2e-aws:
.work/prow-job-analyze-test-failure/2039885033765867520/logs/ - e2e-aks:
.work/prow-job-analyze-test-failure/2039885032079757312/logs/ - e2e-azure-self-managed:
.work/prow-job-analyze-test-failure/2039885037150670848/logs/ - e2e-aws-upgrade:
.work/prow-job-analyze-test-failure/2039885035435200512/logs/ - e2e-kubevirt:
.work/prow-job-analyze-test-failure/2039885039637893120/logs/
|
Now I have all the information needed. Let me compile the structured report: Test Failure Analysis Complete (Multi-Step)Job Information
Root Cause AnalysisErrorThe Failure Chain (traced to root cause)
The Offending Code ChangeFile: func adaptDeployment(cpContext component.WorkloadContext, deployment *appsv1.Deployment) error {
hcp := cpContext.HCP
configuration := hcp.Spec.Configuration
util.UpdateContainer(ComponentName, deployment.Spec.Template.Spec.Containers, func(c *corev1.Container) {
c.Args = append(c.Args, fmt.Sprintf("--machine-namespace=%s", hcp.Namespace))
+ if tlsMinVersion := config.MinTLSVersion(configuration.GetTLSSecurityProfile()); tlsMinVersion != "" {
+ c.Args = append(c.Args, fmt.Sprintf("--tls-min-version=%s", tlsMinVersion))
+ }
+ if cipherSuites := config.CipherSuites(configuration.GetTLSSecurityProfile()); len(cipherSuites) != 0 {
+ c.Args = append(c.Args, fmt.Sprintf("--tls-cipher-suites=%s", strings.Join(cipherSuites, ",")))
+ }
})
return nil
}The PR assumes the Why All 6 Jobs Fail IdenticallyEvery e2e job creates a HostedCluster. The machine-approver is a required control-plane component. Since the default TLS security profile produces non-empty Failed Jobs Summary
Non-Prow Failures
Recommendations
Artifacts
|
7 participants
What this PR does / why we need it:
Add new flags to the hosted machine-approver to comply with the centralized TLS configuration in HyperShift. These flags are to override the machine-approver's internal TLS profile, which is used for its metrics server. Flags to be added via openshift/cluster-machine-approver#292
Special notes for your reviewer:
Checklist: