AGENT-1454: Enforce APIServer TLS profile on IRI registry#5776
AGENT-1454: Enforce APIServer TLS profile on IRI registry#5776rwsu wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@rwsu: This pull request references AGENT-1454 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughWires APIServer informer into the internalreleaseimage controller, propagates APIServer TLSSecurityProfile through bootstrap and renderer, translates TLS profile to registry env vars, updates the iri-registry systemd unit, and adds related unit, controller, and e2e tests and helpers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rwsu 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 |
rwsu
force-pushed
the
AGENT-1454-tls-policy
branch
from
c0ee0b8 to
ce4565f
Compare
![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 `@test/e2e-iri/iri_test.go`:
- Around line 153-160: The current logic only special-cases Modern and treats
any other non-nil TLSSecurityProfile as Intermediate (setting profileName,
expectedMinVersion, rejectedVersion), which is incorrect for Old and Custom
profiles; replace that branch with a switch on
apiServerCfg.Spec.TLSSecurityProfile.Type (checking configv1.TLSProfileOldType,
TLSProfileIntermediateType, TLSProfileModernType, TLSProfileCustomType) and set
profileName/expectedMinVersion/rejectedVersion per profile (e.g. Old ->
"Old"/"1.0"/"1.1", Intermediate -> "Intermediate"/"1.2"/"1.1", Modern ->
"Modern"/"1.3"/"1.2"); for Custom, inspect the TLSSecurityProfile custom
settings to derive the actual minimum TLS version (use that value for
expectedMinVersion and compute rejectedVersion accordingly) and if the custom
minimum cannot be determined, skip the test explicitly. Ensure you reference
apiServerCfg.Spec.TLSSecurityProfile, profileName, expectedMinVersion, and
rejectedVersion when making changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77703d66-53f0-40ed-b7b3-151ef7ac0edb
📒 Files selected for processing (13)
cmd/machine-config-controller/start.gopkg/apihelpers/apihelpers.gopkg/controller/bootstrap/bootstrap.gopkg/controller/internalreleaseimage/internalreleaseimage_bootstrap.gopkg/controller/internalreleaseimage/internalreleaseimage_bootstrap_test.gopkg/controller/internalreleaseimage/internalreleaseimage_controller.gopkg/controller/internalreleaseimage/internalreleaseimage_controller_test.gopkg/controller/internalreleaseimage/internalreleaseimage_helpers_test.gopkg/controller/internalreleaseimage/internalreleaseimage_renderer.gopkg/controller/internalreleaseimage/internalreleaseimage_renderer_test.gopkg/controller/internalreleaseimage/templates/master/units/iri-registry.service.yamlpkg/daemon/constants/constants.gotest/e2e-iri/iri_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/controller/internalreleaseimage/internalreleaseimage_bootstrap_test.go
- pkg/controller/bootstrap/bootstrap.go
- pkg/apihelpers/apihelpers.go
rwsu
force-pushed
the
AGENT-1454-tls-policy
branch
from
ce4565f to
d3cdd54
Compare
|
/verified by @rwsu tested IRI registry TLS settings are updated when cluster level APIServer 's TLSSecurityProfile is changed. Also have e2e test to verify enforcement of currently active profile. And unit test that verifies profile changes produces the correct systemd unit content. |
openshift-ci-robot
added
the
verified
|
@rwsu: 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e-iri/iri_test.go (1)
185-223: Keep the e2e TLS-version mapping aligned with production.This helper hard-codes 1.0-1.3, but
openShiftTLSVersionToRegistryVersion()inpkg/controller/internalreleaseimage/internalreleaseimage_renderer.gowas explicitly written to handleVersionTLS14+. A newerconfigv1.TLSProtocolVersionwill break the test even if the renderer keeps working. Derive the curl version string from theVersionTLSxxtoken here too, or extract a shared helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e-iri/iri_test.go` around lines 185 - 223, The test currently uses a hard-coded tlsVersionMap (tlsVersionMap) which will break when new configv1.TLSProtocolVersion values are added; instead derive the curl-compatible version from the TLS token or reuse the existing conversion logic (openShiftTLSVersionToRegistryVersion in pkg/controller/internalreleaseimage/internalreleaseimage_renderer.go). Replace the hard-coded lookup: compute expectedMinVersion from minTLSVersion by converting the configv1.TLSProtocolVersion token (or calling openShiftTLSVersionToRegistryVersion) and compute rejectedVersion as the prior TLS minor version (or empty if none); update the references expectedMinVersion and rejectedVersion accordingly and remove the static tlsVersionMap.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e-iri/iri_test.go`:
- Around line 171-175: The test currently treats any non-zero command exit as a
server-side TLS rejection; instead capture and inspect the command output from
helpers.ExecCmdOnNodeWithError (the returned stdout/http code) and assert the
HTTP status indicates rejection (for example require.NotEqual(t, "200", code) or
require.Equal(t, "<expected_rejection_code>", code)) rather than only checking
err != nil; update the t.Run block that calls helpers.ExecCmdOnNodeWithError to
read the returned string (HTTP code), assert on that value for server-side
rejection, and only fail on err when the command truly failed to execute (e.g.,
use require.NoError for execution problems and explicit assertions on the HTTP
code for TLS rejection).
---
Nitpick comments:
In `@test/e2e-iri/iri_test.go`:
- Around line 185-223: The test currently uses a hard-coded tlsVersionMap
(tlsVersionMap) which will break when new configv1.TLSProtocolVersion values are
added; instead derive the curl-compatible version from the TLS token or reuse
the existing conversion logic (openShiftTLSVersionToRegistryVersion in
pkg/controller/internalreleaseimage/internalreleaseimage_renderer.go). Replace
the hard-coded lookup: compute expectedMinVersion from minTLSVersion by
converting the configv1.TLSProtocolVersion token (or calling
openShiftTLSVersionToRegistryVersion) and compute rejectedVersion as the prior
TLS minor version (or empty if none); update the references expectedMinVersion
and rejectedVersion accordingly and remove the static tlsVersionMap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 650bf98d-06a6-4a1b-b1ed-ae3950651436
📒 Files selected for processing (13)
cmd/machine-config-controller/start.gopkg/apihelpers/apihelpers.gopkg/controller/bootstrap/bootstrap.gopkg/controller/internalreleaseimage/internalreleaseimage_bootstrap.gopkg/controller/internalreleaseimage/internalreleaseimage_bootstrap_test.gopkg/controller/internalreleaseimage/internalreleaseimage_controller.gopkg/controller/internalreleaseimage/internalreleaseimage_controller_test.gopkg/controller/internalreleaseimage/internalreleaseimage_helpers_test.gopkg/controller/internalreleaseimage/internalreleaseimage_renderer.gopkg/controller/internalreleaseimage/internalreleaseimage_renderer_test.gopkg/controller/internalreleaseimage/templates/master/units/iri-registry.service.yamlpkg/daemon/constants/constants.gotest/e2e-iri/iri_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/controller/internalreleaseimage/internalreleaseimage_helpers_test.go
- pkg/controller/bootstrap/bootstrap.go
- cmd/machine-config-controller/start.go
- pkg/apihelpers/apihelpers.go
| t.Run(fmt.Sprintf("below minimum is rejected with %s profile", profileName), func(t *testing.T) { | ||
| _, err := helpers.ExecCmdOnNodeWithError(cs, node, "curl", "-s", "-k", "-o", "/dev/null", "-w", "%{http_code}", | ||
| "--tlsv"+rejectedVersion, "--tls-max", rejectedVersion, | ||
| "https://localhost:22625/v2/") | ||
| require.Error(t, err, "TLS %s should be rejected with %s profile", rejectedVersion, profileName) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /repo && find . -name "iri_test.go" -path "*/test/e2e-iri/*" | head -1Repository: openshift/machine-config-operator
Length of output: 132
🏁 Script executed:
cd /repo && wc -l test/e2e-iri/iri_test.goRepository: openshift/machine-config-operator
Length of output: 132
🏁 Script executed:
cd /repo && sed -n '160,185p' test/e2e-iri/iri_test.goRepository: openshift/machine-config-operator
Length of output: 132
🏁 Script executed:
cd /repo && wc -l test/helpers/utils.goRepository: openshift/machine-config-operator
Length of output: 132
🏁 Script executed:
cd /repo && sed -n '945,970p' test/helpers/utils.goRepository: openshift/machine-config-operator
Length of output: 132
🏁 Script executed:
cd /repo && rg -n "tlsVersionsFromProfile" test/e2e-iri/Repository: openshift/machine-config-operator
Length of output: 132
🏁 Script executed:
cd /repo && rg -n "ExecCmdOnNodeWithError" test/ -A 5 | head -50Repository: openshift/machine-config-operator
Length of output: 132
🏁 Script executed:
find . -name "iri_test.go" -path "*/test/e2e-iri/*"Repository: openshift/machine-config-operator
Length of output: 103
🏁 Script executed:
wc -l test/e2e-iri/iri_test.goRepository: openshift/machine-config-operator
Length of output: 105
🏁 Script executed:
sed -n '160,185p' test/e2e-iri/iri_test.goRepository: openshift/machine-config-operator
Length of output: 1461
🏁 Script executed:
wc -l test/helpers/utils.goRepository: openshift/machine-config-operator
Length of output: 103
🏁 Script executed:
sed -n '945,970p' test/helpers/utils.goRepository: openshift/machine-config-operator
Length of output: 1142
🏁 Script executed:
rg -n "tlsVersionsFromProfile" test/e2e-iri/ -A 10Repository: openshift/machine-config-operator
Length of output: 2204
🏁 Script executed:
rg -n "ExecCmdOnNodeWithError" test/helpers/utils.go -B 5 -A 20Repository: openshift/machine-config-operator
Length of output: 5832
Make the downgrade check verify the server-side rejection, not just command failure.
Line 172 only checks err != nil. helpers.ExecCmdOnNodeWithError() returns an error whenever the command exits non-zero, but this could result from curl's client-side TLS refusal, remote exec transport failure, missing binary, or network issues—not necessarily a server-side rejection. The positive test (line 169) correctly asserts on the HTTP status code output; the negative test should do the same. Either check the HTTP response code (e.g., for a rejected connection) or assert on the specific error message indicating server rejection, rather than relying on command failure alone.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e-iri/iri_test.go` around lines 171 - 175, The test currently treats
any non-zero command exit as a server-side TLS rejection; instead capture and
inspect the command output from helpers.ExecCmdOnNodeWithError (the returned
stdout/http code) and assert the HTTP status indicates rejection (for example
require.NotEqual(t, "200", code) or require.Equal(t,
"<expected_rejection_code>", code)) rather than only checking err != nil; update
the t.Run block that calls helpers.ExecCmdOnNodeWithError to read the returned
string (HTTP code), assert on that value for server-side rejection, and only
fail on err when the command truly failed to execute (e.g., use require.NoError
for execution problems and explicit assertions on the HTTP code for TLS
rejection).
Read the cluster's APIServer TLS security profile and apply it to the IRI registry's REGISTRY_HTTP_TLS_MINIMUMTLS and REGISTRY_HTTP_TLS_CIPHERSUITES environment variables. When no profile is set, the Intermediate profile (TLS 1.2) is used as the default. The implementation: - Adds an APIServer informer to the IRI controller so TLS profile changes trigger reconciliation - Converts OpenShift TLS version strings (e.g. "VersionTLS12") to the registry format (e.g. "tls1.2") programmatically, supporting future TLS versions without code changes - Filters cipher suites to only pass configurable ones (TLS 1.2 and below) to the registry, since TLS 1.3+ ciphers are fixed by the protocol. Uses Go's CipherSuite.SupportedVersions for future-proof detection - Updates bootstrap path to pass TLS profile through - Adds a NodeDisruptionPolicy for iri-registry.service so TLS-only changes do a daemon-reload and service restart instead of a node reboot - Adds unit test for Modern TLS profile (TLS 1.3) - Adds e2e test that dynamically verifies TLS enforcement based on the cluster's active profile Note: Changing the cluster-level APIServer TLSSecurityProfile will still cause node reboots because it also updates the kubelet config and kube-rbac-proxy static pod, which require a reboot. The IRI NodeDisruptionPolicy avoids adding an additional reboot reason and will enable rebootless updates if IRI TLS settings are changed independently in the future. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
rwsu
force-pushed
the
AGENT-1454-tls-policy
branch
from
d3cdd54 to
5e89db1
Compare
openshift-ci-robot
removed
the
verified
|
/retest-required |
|
/test ? |
|
/test e2e-agent-compact-ipv4-iso-no-registry |
|
@rwsu: 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. |
3 participants
- What I did
Read the cluster's APIServer TLS security profile and apply it to the IRI registry's REGISTRY_HTTP_TLS_MINIMUMTLS and REGISTRY_HTTP_TLS_CIPHERSUITES environment variables. When no profile is set, the Intermediate profile (TLS 1.2) is used as the default.
The implementation:
- How to verify it
Update the cluster TLSSecurtyProfile to Modern (TLS 1.3)
oc patch apiserver cluster --type merge -p '{"spec":{"tlsSecurityProfile":{"type":"Modern","modern":{}}}}'Check /etc/systemd/system/iri-registry.service is updated to set TLS1.3 as mininum TLS version supported.
-e REGISTRY_HTTP_TLS_MINIMUMTLS=tls1.3- Description for the changelog
Enforce APIServer TLS security profile on the IRI registry.