WIP: use TLS groups/curve preferences from controller-runtime-common - #308
WIP: use TLS groups/curve preferences from controller-runtime-common#308damdo wants to merge 4 commits into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
5cbdecd to
fbcafc1
Compare
openshift-ci
Bot
added
the
needs-rebase
|
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:
WalkthroughThe module targets Go 1.26.0 and refreshes OpenShift dependencies. The application adds a TLS curve-preferences flag, validates curve IDs, applies them to server TLS configuration, and logs unsupported TLS groups. ChangesTLS curve preference support
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant main.go
participant ResolveTLSConfig
participant tls.Config
CLI->>main.go: Provide TLS curve preferences
main.go->>ResolveTLSConfig: Pass preferences with startup context
ResolveTLSConfig->>tls.Config: Set CurvePreferences
Possibly related issues
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 5
🤖 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 `@go.mod`:
- Around line 133-136: Update the controller-runtime-common replace directive in
go.mod to reference an existing immutable commit in
github.com/damdo/controller-runtime-common or a released upstream version,
preserving the TLS support dependency and ensuring Go module resolution and go
mod verify succeed.
- Line 54: Upgrade the go.opentelemetry.io/otel dependency to at least v1.41.0
and go.opentelemetry.io/otel/sdk to at least v1.40.0, updating go.mod and go.sum
with the resulting module hashes. Ensure the SBOM and signed provenance
attestation record the exact upgraded versions and hashes.
- Around line 15-20: Update the go.mod dependency train to Kubernetes v0.36.0,
including the k8s.io modules shown, and update sigs.k8s.io/controller-runtime to
v0.24.0. Change the go directive to 1.26 while preserving the existing
dependency structure.
In `@main.go`:
- Around line 157-166: Create the signal-backed cancellable context before
calling ResolveTLSConfig, derive a finite startup-timeout context from it, and
pass that timeout context to pkgtls.ResolveTLSConfig instead of
context.Background(). Preserve the existing fatal error handling, and ensure
cancellation cleanup remains deferred for the created contexts.
In `@pkg/tls/tls.go`:
- Line 25: Update the Kubernetes dependency versions used by pkg/tls/tls.go so
k8s.io/component-base is v0.36 or later and provides
cliflag.TLSCurvePreferences; the import at pkg/tls/tls.go lines 25-25 and its
usage at lines 75-77 require no direct code change once the dependency versions
are aligned.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| k8s.io/api v0.35.2 | ||
| k8s.io/apimachinery v0.35.2 | ||
| k8s.io/client-go v0.35.2 | ||
| k8s.io/klog/v2 v2.140.0 | ||
| k8s.io/utils v0.0.0-20260210185600-b8788abfbbc2 | ||
| sigs.k8s.io/controller-runtime v0.23.3 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
go list -m -f '{{.Path}} {{.Version}}' \
k8s.io/api \
k8s.io/apimachinery \
k8s.io/client-go \
k8s.io/component-base \
sigs.k8s.io/controller-runtime
go doc k8s.io/component-base/cli/flag.TLSCurvePreferencesRepository: openshift/cluster-machine-approver
Length of output: 390
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- go.mod ---'
cat -n go.mod | sed -n '1,145p'
printf '%s\n' '--- relevant symbols and references ---'
rg -n --glob '*.go' --glob '*.mod' 'TLSCurvePreferences|controller-runtime|k8s.io/component-base|go 1\.' .
printf '%s\n' '--- module metadata from the Go proxy ---'
for module_version in \
'k8s.io/component-base/@v/v0.35.2.mod' \
'k8s.io/component-base/@v/v0.36.0.mod' \
'sigs.k8s.io/controller-runtime/@v/v0.23.3.mod' \
'sigs.k8s.io/controller-runtime/@v/v0.24.0.mod'
do
printf '\n[%s]\n' "$module_version"
curl --fail --silent --show-error "https://proxy.golang.org/$module_version" \
| sed -n '1,45p'
done
printf '%s\n' '--- tagged source for TLSCurvePreferences ---'
for ref in v0.35.2 v0.36.0; do
printf '\n[%s]\n' "$ref"
curl --fail --silent --show-error \
"https://raw.githubusercontent.com/kubernetes/kubernetes/$ref/staging/src/k8s.io/component-base/cli/flag/ciphers.go" \
| rg -n -C 3 'TLSCurvePreferences' || true
doneRepository: openshift/cluster-machine-approver
Length of output: 50390
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- controller-runtime and Kubernetes module requirements ---'
for module_version in \
'k8s.io/component-base/@v/v0.35.2.mod' \
'k8s.io/component-base/@v/v0.36.0.mod' \
'sigs.k8s.io/controller-runtime/@v/v0.23.3.mod' \
'sigs.k8s.io/controller-runtime/@v/v0.24.0.mod'
do
printf '\n[%s]\n' "$module_version"
curl --fail --silent --show-error "https://proxy.golang.org/$module_version" \
| awk 'NR <= 35'
done
printf '%s\n' '--- TLS implementation and module source state ---'
cat -n pkg/tls/tls.go | sed -n '55,90p'
rg -n 'TLSCurvePreferences' vendor/k8s.io/component-base vendor/k8s.io --glob '*.go' | head -20 || true
printf '%s\n' '--- API in Kubernetes v0.36 source ---'
for ref in v0.35.2 v0.36.0; do
printf '\n[%s]\n' "$ref"
for file in ciphers.go options.go flags.go; do
url="https://raw.githubusercontent.com/kubernetes/kubernetes/$ref/staging/src/k8s.io/component-base/cli/flag/$file"
body="$(curl --fail --silent --show-error "$url" 2>/dev/null || true)"
if printf '%s' "$body" | rg -q 'TLSCurvePreferences'; then
printf '%s\n' "$url"
printf '%s\n' "$body" | rg -n -C 3 'TLSCurvePreferences'
fi
done
doneRepository: openshift/cluster-machine-approver
Length of output: 6433
Align the Kubernetes dependency train with TLSCurvePreferences.
k8s.io/component-base v0.35.2 does not provide cliflag.TLSCurvePreferences, so the current code does not compile. Update the Kubernetes modules to v0.36.0, update sigs.k8s.io/controller-runtime to v0.24.0, and set the go directive to 1.26.
🤖 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 `@go.mod` around lines 15 - 20, Update the go.mod dependency train to
Kubernetes v0.36.0, including the k8s.io modules shown, and update
sigs.k8s.io/controller-runtime to v0.24.0. Change the go directive to 1.26 while
preserving the existing dependency structure.
Source: MCP tools
| github.com/x448/float16 v0.8.4 // indirect | ||
| go.opentelemetry.io/auto/sdk v1.1.0 // indirect | ||
| go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.61.0 // indirect | ||
| go.opentelemetry.io/otel v1.36.0 // indirect |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
govulncheck -show verbose ./...
go list -m -f '{{.Path}} {{.Version}}' \
go.opentelemetry.io/otel \
go.opentelemetry.io/otel/sdkRepository: openshift/cluster-machine-approver
Length of output: 221
🏁 Script executed:
#!/bin/bash
set -u
echo '--- go.mod dependency context ---'
cat -n go.mod | sed -n '45,62p'
echo '--- module metadata ---'
if command -v go >/dev/null 2>&1; then
go version
go list -m -f '{{.Path}} {{.Version}}' \
go.opentelemetry.io/otel \
go.opentelemetry.io/otel/sdk
else
echo 'go: unavailable'
fi
echo '--- OSV records ---'
python3 - <<'PY'
import json, urllib.request
for module in ("go.opentelemetry.io/otel", "go.opentelemetry.io/otel/sdk"):
req = urllib.request.Request(
"https://api.osv.dev/v1/query",
data=json.dumps({"package": {"ecosystem": "Go", "name": module}}).encode(),
headers={"Content-Type": "application/json"},
)
try:
with urllib.request.urlopen(req, timeout=20) as r:
data = json.load(r)
print(module)
for vuln in data.get("vulns", []):
print(json.dumps({
"id": vuln.get("id"),
"summary": vuln.get("summary"),
"affected": vuln.get("affected"),
"references": vuln.get("references"),
}, separators=(",", ":")))
except Exception as exc:
print(module, "OSV query failed:", repr(exc))
PYRepository: openshift/cluster-machine-approver
Length of output: 1886
🌐 Web query:
OSV GO-2026-5506 go.opentelemetry.io/otel fixed v1.41.0
💡 Result:
GO-2026-5506 (also tracked as CVE-2026-29181 and GHSA-mh2q-q3fh-2475) is a vulnerability in go.opentelemetry.io/otel affecting versions from 1.36.0 up to, but not including, 1.41.0 [1][2][3][4]. The vulnerability involves the extraction of multi-value baggage headers [1][3]. In affected versions, the OpenTelemetry-Go library parsed each header field-value independently and aggregated members across values [3][4]. This behavior allowed an attacker to trigger excessive memory allocations and CPU consumption by sending many baggage header lines, even if each individual value complied with the 8192-byte per-value parse limit [3][4]. This effectively acted as a remote denial-of-service (DoS) amplification vector [3]. The issue was fixed in v1.41.0, which updated baggage handling in go.opentelemetry.io/otel/propagation and go.opentelemetry.io/otel/baggage to comply with W3C Baggage specification limits [5][6]. As part of this fix, the New and Parse functions now return partial baggage along with an error when limits are exceeded, and errors from baggage extraction are reported to the global error handler [5][6].
Citations:
- 1: https://osv.dev/vulnerability/GO-2026-5506
- 2: https://pkg.go.dev/vuln/GO-2026-5506
- 3: https://osv.dev/vulnerability/CVE-2026-29181
- 4: https://nvd.nist.gov/vuln/detail/cve-2026-29181
- 5: https://github.com/open-telemetry/opentelemetry-go/releases/tag/v1.41.0
- 6: https://newreleases.io/project/github/open-telemetry/opentelemetry-go/release/v1.41.0
🌐 Web query:
OSV GO-2026-4394 go.opentelemetry.io/otel/sdk fixed v1.40.0
💡 Result:
GO-2026-4394 (also identified as CVE-2026-24051 and GHSA-9h8m-3fm2-qjrq) is a security vulnerability in the OpenTelemetry Go SDK that allows for Arbitrary Code Execution (ACE) via PATH hijacking [1][2][3]. The vulnerability affects macOS/Darwin systems where the SDK's resource detection code in sdk/resource/host_id.go executes the ioreg system command using an untrusted search path [4][5]. By locally modifying the PATH environment variable, an attacker can cause the application to execute a malicious binary instead of the intended ioreg command [4][6]. The issue affects versions of go.opentelemetry.io/otel/sdk from v1.21.0 up to (but not including) v1.40.0 [1][7]. It was officially patched in release v1.40.0 [8][5]. Users of the affected versions are advised to upgrade to v1.40.0 or later to mitigate this risk [2][5].
Citations:
- 1: https://pkg.go.dev/vuln/GO-2026-4394
- 2: https://go.googlesource.com/vulndb/+/1ce1ea923dd2fd0db2b8e35ca60f25096e9f242c/data/reports/GO-2026-4394.yaml
- 3: GHSA-9h8m-3fm2-qjrq
- 4: https://advisories.gitlab.com/golang/go.opentelemetry.io/otel/sdk/CVE-2026-24051/
- 5: GHSA-9h8m-3fm2-qjrq
- 6: https://osv.dev/vulnerability/GHSA-9h8m-3fm2-qjrq
- 7: https://deps.dev/advisory/osv/GO-2026-4394
- 8: https://deps.dev/advisory/osv/GHSA-9h8m-3fm2-qjrq
🏁 Script executed:
#!/bin/bash
set -u
echo '--- repository OpenTelemetry references ---'
rg -n -i --glob '!go.sum' --glob '!vendor/**' 'go\.opentelemetry\.io|opentelemetry|otel' . || true
echo '--- module checksums ---'
rg -n '^go\.opentelemetry\.io/(otel|otel/metric|otel/sdk|otel/trace)( |/)' go.sum || true
echo '--- module graph metadata ---'
if command -v go >/dev/null 2>&1; then
go list -m -f '{{.Path}} {{.Version}} {{.Indirect}}' \
go.opentelemetry.io/otel \
go.opentelemetry.io/otel/metric \
go.opentelemetry.io/otel/sdk \
go.opentelemetry.io/otel/trace
else
echo 'go: unavailable'
fiRepository: openshift/cluster-machine-approver
Length of output: 2511
Upgrade the vulnerable OpenTelemetry modules before merging.
The indirect dependencies go.opentelemetry.io/otel v1.36.0 and go.opentelemetry.io/otel/sdk v1.36.0 fall within known vulnerable ranges. Upgrade the OpenTelemetry modules to compatible fixed releases (otel >=v1.41.0, sdk >=v1.40.0) and update go.sum. Include the exact versions and hashes in the SBOM and signed provenance attestation.
🧰 Tools
🪛 OSV Scanner (2.4.0)
[HIGH] 54-54: go.opentelemetry.io/otel 1.36.0: OpenTelemetry-Go: Multi-value baggage header extraction causes excessive allocations in go.opentelemetry.io/otel
(GO-2026-5506)
[HIGH] 54-54: go.opentelemetry.io/otel 1.36.0: OpenTelemetry-Go: multi-value baggage header extraction causes excessive allocations (remote dos amplification)
🤖 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 `@go.mod` at line 54, Upgrade the go.opentelemetry.io/otel dependency to at
least v1.41.0 and go.opentelemetry.io/otel/sdk to at least v1.40.0, updating
go.mod and go.sum with the resulting module hashes. Ensure the SBOM and signed
provenance attestation record the exact upgraded versions and hashes.
Sources: Path instructions, MCP tools, Linters/SAST tools
| "fmt" | ||
|
|
||
| configv1 "github.com/openshift/api/config/v1" | ||
| utiltls "github.com/openshift/controller-runtime-common/pkg/tls" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 'controller-runtime-common|k8s.io/component-base|replace ' go.mod go.sum
rg -n -C2 'TLSCurvePreferences|NewTLSConfigFromProfile' pkg/tls/tls.go main.go
# Expected: a resolvable controller-runtime-common revision and a component-base
# version compatible with cliflag.TLSCurvePreferences.Repository: openshift/cluster-machine-approver
Length of output: 3997
Use compatible dependency versions before merge.
controller-runtime-common resolves through the declared replacement. However, k8s.io/component-base is pinned to v0.35.2, while cliflag.TLSCurvePreferences requires v0.36 or later. Bump the Kubernetes dependencies or defer this API use.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 25-25: could not import github.com/openshift/controller-runtime-common/pkg/tls (pkg/tls/tls.go:25:2: github.com/damdo/controller-runtime-common@v0.0.0-20260703112942-fc0245fff371: invalid version: unknown revision fc0245fff371)
(typecheck)
📍 Affects 1 file
pkg/tls/tls.go#L25-L25(this comment)pkg/tls/tls.go#L75-L77
🤖 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 `@pkg/tls/tls.go` at line 25, Update the Kubernetes dependency versions used by
pkg/tls/tls.go so k8s.io/component-base is v0.36 or later and provides
cliflag.TLSCurvePreferences; the import at pkg/tls/tls.go lines 25-25 and its
usage at lines 75-77 require no direct code change once the dependency versions
are aligned.
Source: Linters/SAST tools
fbcafc1 to
59771be
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
go.mod (1)
133-134: 🔒 Security & Privacy | 🔵 TrivialRecord the fork in release metadata.
The fork uses Apache-2.0. Its checksums are present,
go mod verifypasses, and OSV reports no advisories. Before release, record production approval and include the resolved commit and checksums in the SBOM and signed provenance attestation.🤖 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 `@go.mod` around lines 133 - 134, Record the approved controller-runtime-common fork in the release metadata, including its resolved commit and Go module checksums. Update the SBOM and signed provenance attestation for the dependency referenced by the replace directive, and include production approval before release.Source: Path instructions
🤖 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 `@pkg/tls/tls.go`:
- Around line 75-80: Remove the obsolete Kubernetes-version TODO comments
immediately above the cliflag.TLSCurvePreferences call, leaving the existing
curve preference parsing and error handling unchanged.
- Around line 82-87: Update every ResolveTLSConfig call in pkg/tls/tls_test.go
to pass the fifth tlsCurvePreferences argument, then add coverage for valid,
ordered, duplicate, zero, out-of-range, and unsupported curve IDs plus
curve-only CLI input. Remove the stale TODO in ResolveTLSConfig-related code in
pkg/tls/tls.go, relying on k8s.io/component-base v0.36.2’s TLSCurvePreferences.
- Around line 51-54: Update every ResolveTLSConfig call site, especially the
twelve calls in tls_test.go, to pass the tlsCurvePreferences argument before
merging. Preserve each call’s existing values and supply the appropriate
curve-preferences value as the fifth argument.
---
Nitpick comments:
In `@go.mod`:
- Around line 133-134: Record the approved controller-runtime-common fork in the
release metadata, including its resolved commit and Go module checksums. Update
the SBOM and signed provenance attestation for the dependency referenced by the
replace directive, and include production approval before release.
🪄 Autofix
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: 2373a3e1-c2a4-4786-a0d8-263b642efbee
⛔ Files ignored due to path filters (57)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_kmsencryption.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-Hypershift.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-SelfManagedHA.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/route/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/awsplatformstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/baremetalplatformstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/gcpplatformstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/vsphereplatformfailuredomainspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/vsphereplatformspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/nodeexportercollectorconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/nodeexportercollectordevicemappermultipathconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/nodeexportercollectorzoneinfoconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/remotewritespec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/kmsencryptionstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/kmspluginhealthreport.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/nodestatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/controller-runtime-common/pkg/tls/tls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/crypto/crypto.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/resource/resourceapply/resource_cache.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (3)
go.modmain.gopkg/tls/tls.go
🚧 Files skipped from review as they are similar to previous changes (1)
- main.go
4640692 to
6d17d12
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/tls/tls_test.go (1)
38-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for non-empty curve preferences.
All updated calls in this file pass
nil, so they do not exercise the newtlsCurvePreferencespath inResolveTLSConfigor verifytls.Config.CurvePreferences. Add a focused test with a non-empty[]int32and assert the applied curve preferences.🤖 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 `@pkg/tls/tls_test.go` at line 38, Add a focused test in tls_test.go that invokes ResolveTLSConfig with a non-empty []int32 tlsCurvePreferences value and asserts the resulting tls.Config.CurvePreferences matches the supplied curves, covering the non-nil preference path while preserving existing nil-input tests.
🤖 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 `@pkg/tls/tls_test.go`:
- Line 38: Add a focused test in tls_test.go that invokes ResolveTLSConfig with
a non-empty []int32 tlsCurvePreferences value and asserts the resulting
tls.Config.CurvePreferences matches the supplied curves, covering the non-nil
preference path while preserving existing nil-input tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ab72d880-1783-4870-a368-8d17fc453b34
📒 Files selected for processing (1)
pkg/tls/tls_test.go
Bump openshift/api, openshift/client-go, and openshift/library-go to latest upstream master to pick up the TLS Groups field in TLSProfileSpec and the TLSGroupsToCurveIDs function. Add replace directive for controller-runtime-common fork that implements curve preferences support (openshift/controller-runtime-common#22). TODO: Remove the replace directive once the TLS groups/curve preferences support is merged and released upstream in controller-runtime-common.
6d17d12 to
d816d37
Compare
|
@damdo: The following test 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. |
1 participant
Summary
controller-runtime-commonandlibrary-goforks to pick up TLS groups/curve preferences supportNewTLSConfigFromProfilecurve preferences in the cluster TLS profile path--tls-curve-preferencesCLI flag (numerictls.CurveIDvalues, matchingk8s.io/component-baseconvention)The
--tls-curve-preferencesflag usescliflag.TLSCurvePreferencesfromk8s.io/component-basev0.36+. This won't compile until the k8s 1.36 dependency bump lands.CPO (HyperShift) can leverage
libgocrypto.CurveIDsForTLSGroups()fromo/library-goto convert IANA TLS group names fromo/apiinto numerictls.CurveIDvalues, and pass them to this flag on the CMA Deployment spec.Dependencies
NewTLSConfigFromProfileCurveIDsForTLSGroups/ValidTLSGroupsin library-go cryptocliflag.TLSCurvePreferences)Test plan
--tls-curve-preferences) is expected to fail compilation until the k8s 1.36 bump--tls-curve-preferences 29,23correctly setsCurvePreferenceson the TLS configSummary by CodeRabbit
New Features
Bug Fixes