OCPCLOUD-3359: Add TLS substitutions#408
OCPCLOUD-3359: Add TLS substitutions#408openshift-merge-bot[bot] merged 2 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@mdbooth: This pull request references OCPCLOUD-3359 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 "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. |
|
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 selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds two TLS-related command-line arguments ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 5/10 reviews remaining, refill in 27 minutes and 23 seconds. Comment |
![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 the current code and only fix it if needed.
Inline comments:
In `@openshift/capi-operator-manifests/default/manifests.yaml`:
- Around line 9128-9129: The flags --tls-min-version and --tls-cipher-suites are
using unsubstituted variables TLS_MIN_VERSION and TLS_CIPHER_SUITES which can
leave literal tokens at startup; update the manifest to provide safe defaults
using the shell-style default syntax (e.g., change
--tls-min-version=${TLS_MIN_VERSION} to
--tls-min-version=${TLS_MIN_VERSION:=VersionTLS13} and set a sensible default
list for --tls-cipher-suites like
${TLS_CIPHER_SUITES:="TLS_AES_128_GCM_SHA256,TLS_AES_256_GCM_SHA384"}) or
alternatively add documentation/comments marking TLS_MIN_VERSION and
TLS_CIPHER_SUITES as required variables so the operator always supplies them;
locate and edit the manager container args that contain --tls-min-version and
--tls-cipher-suites to apply this change.
🪄 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: 4b27a67f-1b5d-4dfb-9df2-ffabbe360d02
📒 Files selected for processing (2)
openshift/capi-operator-manifests/default/manifests.yamlopenshift/kustomization.yaml
| - --tls-min-version=${TLS_MIN_VERSION} | ||
| - --tls-cipher-suites=${TLS_CIPHER_SUITES} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find all references to TLS substitution variables =="
rg -n -C2 '\bTLS_MIN_VERSION\b|\bTLS_CIPHER_SUITES\b'
echo
echo "== Find manifest args using these substitutions =="
rg -n -C2 -- '--tls-min-version|--tls-cipher-suites'
echo
echo "== Find kustomize/env substitution sources (common patterns) =="
rg -n -C2 'replacements:|vars:|configMapGenerator:|env:|substitution|TLS_MIN_VERSION|TLS_CIPHER_SUITES'Repository: openshift/cluster-api-provider-openstack
Length of output: 50397
🏁 Script executed:
# Search for where TLS_MIN_VERSION and TLS_CIPHER_SUITES are DEFINED/ASSIGNED
echo "=== Searching for variable assignments ==="
rg -n 'TLS_MIN_VERSION\s*[:=]|TLS_CIPHER_SUITES\s*[:=]' --type yaml --type sh
echo
echo "=== Checking for env file sources ==="
fd -e env -e 'env.*' | head -20
echo
echo "=== Checking openshift directory structure ==="
ls -la openshift/ 2>/dev/null | head -20
echo
echo "=== Looking at openshift/kustomization.yaml in full context ==="
head -50 openshift/kustomization.yaml
echo
echo "=== Checking for vars or similar patterns in kustomization ==="
rg -n 'vars:|commonLabels:|commonAnnotations:|configMapGenerator:|secretGenerator:' openshift/ -A 3Repository: openshift/cluster-api-provider-openstack
Length of output: 2713
🏁 Script executed:
# Check for any validation scripts or documentation
echo "=== Searching for documentation/comments about TLS variables ==="
rg -n 'TLS_MIN_VERSION|TLS_CIPHER_SUITES' --type md --type txt --type sh
echo
echo "=== Checking Makefile for variable initialization ==="
cat openshift/Makefile
echo
echo "=== Checking if manifests.yaml has any default values ==="
sed -n '9120,9135p' openshift/capi-operator-manifests/default/manifests.yaml
echo
echo "=== Searching for any default TLS values in config ==="
rg -n 'tls-min-version|tls-cipher-suites' config/ 2>/dev/null | head -20Repository: openshift/cluster-api-provider-openstack
Length of output: 1939
Add default TLS values to prevent deployment failures from unresolved substitution.
Lines 9128-9129 lack default values, unlike the pattern used for feature gates on line 9127 (${EXP_CAPO_PRIORITY_QUEUE:=false}). Without defaults, if the cluster-capi-operator tool fails to substitute ${TLS_MIN_VERSION} and ${TLS_CIPHER_SUITES}, the manager will start with literal unresolved tokens as flag values, causing startup failures. Either:
- Add defaults using the
${VAR:=default}syntax (e.g.,--tls-min-version=${TLS_MIN_VERSION:=VersionTLS13}), or - Document these as required variables that must always be provided by the operator.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openshift/capi-operator-manifests/default/manifests.yaml` around lines 9128 -
9129, The flags --tls-min-version and --tls-cipher-suites are using
unsubstituted variables TLS_MIN_VERSION and TLS_CIPHER_SUITES which can leave
literal tokens at startup; update the manifest to provide safe defaults using
the shell-style default syntax (e.g., change
--tls-min-version=${TLS_MIN_VERSION} to
--tls-min-version=${TLS_MIN_VERSION:=VersionTLS13} and set a sensible default
list for --tls-cipher-suites like
${TLS_CIPHER_SUITES:="TLS_AES_128_GCM_SHA256,TLS_AES_256_GCM_SHA384"}) or
alternatively add documentation/comments marking TLS_MIN_VERSION and
TLS_CIPHER_SUITES as required variables so the operator always supplies them;
locate and edit the manager container args that contain --tls-min-version and
--tls-cipher-suites to apply this change.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mandre
left a comment
mandre
left a comment
There was a problem hiding this comment.
Seems fine to me on principle.
/approve
| patch: |- | ||
| - op: add | ||
| path: /spec/template/spec/containers/0/args/- | ||
| value: "--tls-min-version=${TLS_MIN_VERSION}" |
There was a problem hiding this comment.
Should we add default values, or do we expect cluster-capi-operator to always set a value?
There was a problem hiding this comment.
CAPIO will always set a value. This is part of the new requirements around PQC, and uses the cluster-wide TLS configuration.
openshift-ci
Bot
added
the
approved
|
I can see the TLS substitutions being set on the CAPO Deployment. /verified by @damdo |
|
@damdo: Jira verification commands are restricted to collaborators for this repo. 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: changing LGTM is restricted to collaborators 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. |
|
@mandre @stephenfin I added the verified & lgtm, but it appears I'm not allowed to do that, could you add it if you are happy? Thanks! |
|
/lgtm |
openshift-ci-robot
added
the
verified
|
@mandre: 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mandre 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 |
|
/override ci/prow/e2e-hypershift |
|
@mandre: Overrode contexts on behalf of mandre: ci/prow/e2e-hypershift 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. |
|
@mdbooth: 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. |
4 participants
Summary by CodeRabbit