CNTRLPLANE-3611: propagate tls profile to aws-pod-identity-webhook#8713
CNTRLPLANE-3611: propagate tls profile to aws-pod-identity-webhook#8713ricardomaraschini wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@ricardomaraschini: This pull request references CNTRLPLANE-3611 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. |
|
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:
📝 WalkthroughWalkthroughThe pull request refactors the 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/control-plane-operator
ricardomaraschini
force-pushed
the
CNTRLPLANE-3611
branch
from
ce4c692 to
c117043
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: 2
🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.go`:
- Around line 315-319: The conversion of TLS enums can fail silently: when
config.MinTLSVersion(hcp.Spec.Configuration.GetTLSSecurityProfile()) yields a
value but convertTLSVersion(...) returns "", add explicit handling to surface
that failure; e.g., after calling convertTLSVersion(tlsMinVersion) detect empty
string and either log a warning/error (including the original tlsMinVersion, the
HostedControlPlane identity from hcp, and that the --tls-min-version flag will
be omitted) or return/propagate a validation error so the misconfiguration is
visible; update the block around convertTLSVersion, tlsMinVersion, and the
command append of "--tls-min-version" to include this log/validation.
- Around line 357-370: Add a new table-driven unit test named
TestConvertTLSVersion in package kas (file deployment_test.go) that calls the
helper convertTLSVersion with inputs "VersionTLS10", "VersionTLS11",
"VersionTLS12", "VersionTLS13", an unknown string like "VersionTLS99", and the
empty string, asserting returned values "1.0", "1.1", "1.2", "1.3", and ""
respectively; use t.Run for each case and fail the test when the actual return
from convertTLSVersion does not match the expected value.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: aea43979-896a-48e1-b5b6-ce4da195d70c
⛔ Files ignored due to path filters (2)
control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_kube_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/zz_fixture_TestControlPlaneComponents_kube_apiserver_deployment.yamlis excluded by!**/testdata/**
📒 Files selected for processing (1)
control-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8713 +/- ##
==========================================
+ Coverage 41.50% 41.75% +0.25%
==========================================
Files 758 758
Lines 93689 93970 +281
==========================================
+ Hits 38882 39238 +356
+ Misses 52070 51986 -84
- Partials 2737 2746 +9
... and 23 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ricardomaraschini
force-pushed
the
CNTRLPLANE-3611
branch
3 times, most recently
from
74e07b7 to
ffd8f2e
Compare
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Dropped some comments. Thanks!
| ) | ||
| } | ||
|
|
||
| func convertTLSVersion(version string) string { |
There was a problem hiding this comment.
The upstream openshift/aws-pod-identity-webhook accepts both formats ("VersionTLS12" and "1.2") in its ValidateTLSMinVersion(). Every other component in v2/ (kube-scheduler, KCM, machine-approver, oauth-apiserver) passes config.MinTLSVersion() directly without conversion. I think you could drop convertTLSVersion entirely and pass the string through — simpler and consistent with the rest of the codebase.
There was a problem hiding this comment.
This is what I gain for only reading the --help output. Good point, make things simpler. It is done.
| } | ||
| } | ||
|
|
||
| func applyAzureWorkloadIdentityWebhookContainer(podSpec *corev1.PodSpec, hcp *hyperv1.HostedControlPlane) { |
There was a problem hiding this comment.
Should applyAzureWorkloadIdentityWebhookContainer get the same treatment? It also serves a TLS endpoint and currently uses whatever the binary defaults to. If this is part of the PQC work, the Azure path has the same gap. Might be worth a follow-up linked to CNTRLPLANE-3611.
There was a problem hiding this comment.
This is done in a different commit here. Please take a look.
| }, | ||
| }, | ||
| validatePod: func(g *GomegaWithT, podSpec *corev1.PodSpec) { | ||
| var webhookContainer *corev1.Container |
There was a problem hiding this comment.
This container-lookup pattern is repeated in all 5 test cases. You could extract a small helper to cut the noise:
func findWebhookContainer(g *GomegaWithT, podSpec *corev1.PodSpec) *corev1.Container {
for i := range podSpec.Containers {
if podSpec.Containers[i].Name == "aws-pod-identity-webhook" {
return &podSpec.Containers[i]
}
}
g.Expect(true).To(BeFalse(), "aws-pod-identity-webhook container not found")
return nil
}| } | ||
| } | ||
| g.Expect(webhookContainer).NotTo(BeNil()) | ||
| g.Expect(webhookContainer.Command).To(ContainElement("--tls-min-version=1.3")) |
There was a problem hiding this comment.
This correctly checks for --tls-min-version=1.3, but doesn't verify that --tls-cipher-suites is absent. For Modern, CipherSuites() returns empty (TLS 1.3 ciphers aren't in the OpenSSL-to-IANA map). Worth locking down:
g.Expect(slices.ContainsFunc(webhookContainer.Command, func(arg string) bool {
return strings.HasPrefix(arg, "--tls-cipher-suites=")
})).To(BeFalse())| g.Expect(webhookContainer).NotTo(BeNil()) | ||
| g.Expect(webhookContainer.Command).To(ContainElement("--tls-min-version=1.3")) | ||
| }, | ||
| }, |
There was a problem hiding this comment.
nit: might be worth adding a case for configv1.TLSProfileOldType — maps to VersionTLS10 with legacy CBC ciphers. Would complete the profile matrix.
ricardomaraschini
force-pushed
the
CNTRLPLANE-3611
branch
from
ffd8f2e to
29ae771
Compare
aws-pod-identity-webhook needs to receive two flags: - --tls-min-version - needs to comply with the hcp apiserver min tls version. - --tls-cipher-suites - needs to comply with the hcp apiserver cipher suites config. this commit propagates both configuration into the flags.
ricardomaraschini
force-pushed
the
CNTRLPLANE-3611
branch
from
29ae771 to
310c4da
Compare
openshift-ci
Bot
added
the
area/platform/azure
ricardomaraschini
force-pushed
the
CNTRLPLANE-3611
branch
from
310c4da to
0913c13
Compare
|
Now I have all the evidence. The PR adds The gci config defines these ordered sections:
So The PR placed them together in the same import group — that's the violation. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe
The PR added the new import Current (incorrect) import block in the PR: import (
"testing"
. "github.com/onsi/gomega"
configv1 "github.com/openshift/api/config/v1" // ← section 4 (openshift)
hyperv1 "github.com/openshift/hypershift/api/..." // ← section 3 (hypershift)
"github.com/openshift/hypershift/control-plane-..." // ← section 3 (hypershift)
corev1 "k8s.io/api/core/v1" // ← section 7 (k8s.io)
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" // ← section 7 (k8s.io)
)Required (correct) import ordering: import (
"testing"
. "github.com/onsi/gomega"
hyperv1 "github.com/openshift/hypershift/api/..."
"github.com/openshift/hypershift/control-plane-..."
configv1 "github.com/openshift/api/config/v1"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)The Recommendations
Evidence
|
sets both the min tls version and the cipher suites for the azure workload identity webhook container.
ricardomaraschini
force-pushed
the
CNTRLPLANE-3611
branch
from
0913c13 to
d0083c9
Compare
|
@ricardomaraschini: 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. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jparrill, ricardomaraschini 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 |
openshift-ci
Bot
added
the
approved
3 participants
What this PR does / why we need it:
It was detected that the containers aws-pod-identity-webhook and azure-workload-identity-webhook aren't respecting the HostedControlPlane TLS profile. As part of the PQC work the following flags need to be set:
--tls-min-version
needs to comply with the hcp apiserver min tls version.
--tls-cipher-suites
needs to comply with the hcp apiserver cipher suites config.
Both flags are now set according to hcp.spec.configuration.apiServer.tlsSecurityProfile for both webhook containers. These flags influence how the containers listen on their respective ports (aws-pod-identity-webhook on port 4443 and azure-workload-identity-webhook on port 4443).
Changes include:
findContainerByNameInPod()helper functionChecklist:
Summary by CodeRabbit