CNTRLPLANE-3616: e2e tests for TLS profile change of konnectivity-server - #8886
CNTRLPLANE-3616: e2e tests for TLS profile change of konnectivity-server#8886kaleemsiddiqu wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@kaleemsiddiqu: This pull request references CNTRLPLANE-3616 which is a valid jira issue. 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 YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a new Go e2e v2 test suite for konnectivity-server TLS configuration. It registers Sequence Diagram(s)sequenceDiagram
participant Test as VerifyKonnectivityServerTLSConfigTest
participant HostedCluster
participant KubeAPIServerPod as kube-apiserver pod
participant Konnectivity as konnectivity-server
Test->>HostedCluster: capture original APIServer TLS profile
Test->>KubeAPIServerPod: inspect konnectivity-server args
Test->>KubeAPIServerPod: exec openssl s_client for TLS 1.2 and TLS 1.3
KubeAPIServerPod->>Konnectivity: handshake attempts
Konnectivity-->>KubeAPIServerPod: accept both versions
Test->>HostedCluster: update profile to Modern
Test->>KubeAPIServerPod: wait for VersionTLS13
Test->>KubeAPIServerPod: exec openssl s_client for TLS 1.3 and TLS 1.2
KubeAPIServerPod->>Konnectivity: handshake attempts
Konnectivity-->>KubeAPIServerPod: accept TLS 1.3, reject TLS 1.2
Test->>HostedCluster: remove Modern profile
Test->>KubeAPIServerPod: wait for VersionTLS12
Test->>KubeAPIServerPod: exec openssl s_client for TLS 1.2 and TLS 1.3
KubeAPIServerPod->>Konnectivity: handshake attempts
Konnectivity-->>KubeAPIServerPod: accept both versions again
Test->>HostedCluster: restore original profile
Related PRs: None identified. Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/testing
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kaleemsiddiqu 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
test/e2e/v2/tests/konnectivity_server_test.go (1)
623-623: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse plain v2 Ginkgo names instead of legacy string annotations.
The current v2 convention is to keep
Describenames plain and express filtering/classification via labels.Suggested fix
-var _ = Describe("[sig-hypershift][Jira:Hypershift][Feature:KonnectivityServer] Konnectivity Server TLS Configuration", Label("konnectivity-server"), func() { +var _ = Describe("Konnectivity Server TLS Configuration", Label("konnectivity-server"), func() {Based on learnings, v2 E2E tests should not include legacy
[sig-hypershift],[Jira:Hypershift], or[Feature:XYZ]annotations in GinkgoDescribenames.🤖 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 `@test/e2e/v2/tests/konnectivity_server_test.go` at line 623, The Describe in konnectivity_server_test.go still uses legacy Ginkgo v1-style annotations in the test title. Update the Describe name in the konnectivity server test to a plain v2 name and keep classification in the existing Label("konnectivity-server") (and any other labels if needed) instead of embedding [sig-hypershift], [Jira:Hypershift], or [Feature:...] in the string.Source: Learnings
🤖 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 `@test/e2e/v2/tests/konnectivity_server_test.go`:
- Around line 112-114: The konnectivity server test is only checking
kasPodList.Items[0], so an HA control plane can hide stale kube-apiserver pods.
Update the affected assertions in konnectivity_server_test.go to iterate over
all kube-apiserver pods returned by the list, or use a deterministic selector
only after confirming the full replica set is updated. Apply this to the
repeated kasPodList.Items[0] checks in the relevant test blocks so each pod’s
phase, args, and TLS behavior are validated consistently.
- Around line 278-285: The polling block in Eventually for kube-apiserver pod
discovery should not return early on mgmtClient.List errors, because that makes
the poll succeed without validating anything. In the konnectivity_server_test.go
checks that use mgmtClient.List with kasPodList and MatchingLabels{"app":
"kube-apiserver"}, replace the silent return with a failed Gomega expectation or
explicit failure so the Eventually retry fails loudly. Apply the same fix in
both affected polling sections so listing failures cannot be masked.
- Around line 349-352: The readiness checks over kasPod.Status.ContainerStatuses
can pass vacuously when the slice is empty, so add an explicit non-empty
assertion before the per-container loop in the konnectivity server test. Update
both ContainerStatuses iterations in this test to first verify the list has at
least one entry, then keep the existing container-by-container Ready assertions.
Use the existing kasPod.Status.ContainerStatuses and containerStatus.Ready
checks as the place to apply the fix.
- Around line 77-90: The cleanup for the HostedCluster state only restores the
TLSSecurityProfile, so it can leave empty Spec.Configuration or APIServer
structs behind when they were originally nil. In konnectivity_server_test.go,
update the BeforeAll capture logic around the
hostedCluster.Spec.Configuration.APIServer.TLSSecurityProfile snapshot to also
record whether Spec.Configuration and APIServer existed before mutation. Then
restore the exact original shape in the cleanup path using those captured flags,
not just the TLS profile.
---
Nitpick comments:
In `@test/e2e/v2/tests/konnectivity_server_test.go`:
- Line 623: The Describe in konnectivity_server_test.go still uses legacy Ginkgo
v1-style annotations in the test title. Update the Describe name in the
konnectivity server test to a plain v2 name and keep classification in the
existing Label("konnectivity-server") (and any other labels if needed) instead
of embedding [sig-hypershift], [Jira:Hypershift], or [Feature:...] in the
string.
🪄 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: 8d835ec8-e5fc-4bb8-a4cb-f644c4b39fb2
📒 Files selected for processing (1)
test/e2e/v2/tests/konnectivity_server_test.go
| kasPod := &kasPodList.Items[0] | ||
| Expect(kasPod.Status.Phase).To(Equal(corev1.PodRunning), | ||
| "kube-apiserver pod should be running") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Avoid validating only the first kube-apiserver pod.
The test repeatedly uses kasPodList.Items[0], so an HA control plane can pass while another kube-apiserver pod still has stale konnectivity args or TLS behavior. Iterate all listed pods, or select deterministically only after proving the intended replica set is fully updated.
Also applies to: 170-185, 215-239, 288-303, 333-352, 369-380, 395-409, 460-475, 506-525, 542-579
🤖 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 `@test/e2e/v2/tests/konnectivity_server_test.go` around lines 112 - 114, The
konnectivity server test is only checking kasPodList.Items[0], so an HA control
plane can hide stale kube-apiserver pods. Update the affected assertions in
konnectivity_server_test.go to iterate over all kube-apiserver pods returned by
the list, or use a deterministic selector only after confirming the full replica
set is updated. Apply this to the repeated kasPodList.Items[0] checks in the
relevant test blocks so each pod’s phase, args, and TLS behavior are validated
consistently.
Source: Path instructions
| err := mgmtClient.List(tc.Context, kasPodList, | ||
| crclient.InNamespace(tc.ControlPlaneNamespace), | ||
| crclient.MatchingLabels{"app": "kube-apiserver"}, | ||
| ) | ||
|
|
||
| if err != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not silently pass Eventually when pod listing fails.
In Eventually(func(g Gomega)), returning without a failed expectation makes that poll successful. These branches can let propagation checks pass without ever listing kube-apiserver pods.
Suggested fix
- if err != nil {
- return
- }
+ g.Expect(err).NotTo(HaveOccurred(), "failed to list kube-apiserver pods")As per path instructions, framework paths should fail loudly rather than silently swallow errors.
Also applies to: 449-457
🤖 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 `@test/e2e/v2/tests/konnectivity_server_test.go` around lines 278 - 285, The
polling block in Eventually for kube-apiserver pod discovery should not return
early on mgmtClient.List errors, because that makes the poll succeed without
validating anything. In the konnectivity_server_test.go checks that use
mgmtClient.List with kasPodList and MatchingLabels{"app": "kube-apiserver"},
replace the silent return with a failed Gomega expectation or explicit failure
so the Eventually retry fails loudly. Apply the same fix in both affected
polling sections so listing failures cannot be masked.
Source: Path instructions
| for _, containerStatus := range kasPod.Status.ContainerStatuses { | ||
| g.Expect(containerStatus.Ready).To(BeTrue(), | ||
| "container %s should be ready in pod %s", containerStatus.Name, kasPod.Name) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert container statuses are present before per-container checks.
If ContainerStatuses is empty, these readiness loops pass vacuously. Add a non-empty assertion before iterating.
Suggested fix
+ g.Expect(kasPod.Status.ContainerStatuses).NotTo(BeEmpty(),
+ "expected container statuses on pod %s", kasPod.Name)
for _, containerStatus := range kasPod.Status.ContainerStatuses {As per path instructions, before iterating a list and asserting on each item, assert the list is non-empty.
Also applies to: 522-525
🤖 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 `@test/e2e/v2/tests/konnectivity_server_test.go` around lines 349 - 352, The
readiness checks over kasPod.Status.ContainerStatuses can pass vacuously when
the slice is empty, so add an explicit non-empty assertion before the
per-container loop in the konnectivity server test. Update both
ContainerStatuses iterations in this test to first verify the list has at least
one entry, then keep the existing container-by-container Ready assertions. Use
the existing kasPod.Status.ContainerStatuses and containerStatus.Ready checks as
the place to apply the fix.
Source: Path instructions
Codecov Report✅ All modified and coverable lines are covered by tests. Please upload reports for the commit 7ea7e6e to get more accurate results. Additional details and impacted files@@ Coverage Diff @@
## main #8886 +/- ##
==========================================
- Coverage 43.45% 43.26% -0.20%
==========================================
Files 771 770 -1
Lines 95718 95479 -239
==========================================
- Hits 41597 41311 -286
- Misses 51234 51284 +50
+ Partials 2887 2884 -3 see 15 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:
|
e2e tests added for konnectivity-server to verify that TLS profile propagation is respected correctly after this new flag addition Signed-off-by: Kaleemullah Siddiqui <ksiddiqu@redhat.com>
5304af1 to
7ea7e6e
Compare
|
/retest |
|
@kaleemsiddiqu: 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. |
|
The diff's +51 maps to file line 45 (accounting for the file starting at line 1 in the diff with header offset), which matches the error exactly: Now I have all the evidence. Here's the complete analysis: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe function
Both files use the The function has the same signature and implementation in both files: func hostedClusterHasTLSProfileType(hc *hyperv1.HostedCluster, profileType configv1.TLSProfileType) bool {
return hc.Spec.Configuration != nil &&
hc.Spec.Configuration.APIServer != nil &&
hc.Spec.Configuration.APIServer.TLSSecurityProfile != nil &&
hc.Spec.Configuration.APIServer.TLSSecurityProfile.Type == profileType
}This is a straightforward Go compilation rule — a package-scoped identifier cannot be declared twice within the same package. The author likely copied helper functions from Recommendations
Evidence
|
2 participants
e2e tests added for konnectivity-server to verify that TLS profile propagation is respected correctly after this new flag addition
test for changes done in #8866
Summary by CodeRabbit