GCP-688: Port v1 E2E validations to envtest, unit tests, and v2 Ginkgo specs#8511
Open
cblecker wants to merge 7 commits into
Open
GCP-688: Port v1 E2E validations to envtest, unit tests, and v2 Ginkgo specs#8511cblecker wants to merge 7 commits into
cblecker wants to merge 7 commits into
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
Skipping CI for Draft Pull Request. |
|
@cblecker: This pull request references GCP-688 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. |
Contributor
📝 WalkthroughWalkthroughThis PR adds multiple new unit and e2e-v2 tests and test utilities across the repository. Unit tests added/extended: HCP status reconciliation, webhook URL validation and guest webhook cleanup, payload architecture determination, and control-plane component toleration propagation. E2E v2 test infra changes: pod exec helpers ( Suggested reviewers
🚥 Pre-merge checks | ✅ 9 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cblecker 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
openshift-ci
Bot
added
area/cli
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
`@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go`:
- Around line 6824-6830: The test currently bypasses production logic by
directly assigning hcluster.Status.Configuration = hcp.Status.Configuration;
instead invoke the controller's real reconciliation path (call the HostedCluster
reconciler's Reconcile method or the helper used by hostedcluster_controller.go
that performs the "Copy the configuration status from the hostedcontrolplane"
logic) so the same code path is exercised; remove the inline assignment, run the
reconcile/helper, then assert hcluster.Status.Configuration equals
tc.hcpConfiguration.
In `@test/e2e/v2/internal/pod_exec.go`:
- Around line 35-68: The function RunCommandInPod currently returns errors;
change it to panic on failures per framework guidelines: replace the error
return after kubernetes.NewForConfig with a panic that includes a clear
diagnostic message containing namespace and podName and the wrapped error;
likewise panic if remotecommand.NewSPDYExecutor fails with a message including
namespace/podName/containerName; and panic if exec.StreamWithContext returns an
error, including namespace/podName/containerName and stderr output in the panic
text; keep the successful return of stdout.String() as-is.
- Around line 72-80: GetMetricsFromPod currently returns an error on failure;
update it to follow the framework panic-on-failure pattern used by
RunCommandInPod: call RunCommandInPod with the same args, and if err != nil
panic with a diagnostic message including namespace/podName/containerName/port
and the error; otherwise return []byte(output). Ensure the function signature
stays the same but remove the error return path and panic on failures so the
framework behavior is consistent.
In `@test/e2e/v2/internal/test_context.go`:
- Around line 130-134: Replace the silent early return in the
tc.hostedClusterRESTConfigOnce.Do closure with a panic that includes a clear
diagnostic message; specifically, in the closure that calls
tc.GetHostedCluster() and checks hc == nil || hc.Status.KubeConfig == nil, panic
(do not assign or cache nil to tc.hostedClusterRESTConfig) with a message
describing whether the HostedCluster is missing or its kubeconfig is nil so
callers fail loudly and the sync.Once does not cache a nil result.
In `@test/e2e/v2/tests/control_plane_workloads_test.go`:
- Around line 695-696: The inline comment before the test that reads
'Label("Informing"): failures skip (non-blocking) until registry is complete' is
stale because the test starting with It("all pods should belong to predefined
workloads") is now a blocking test; update or remove that comment so it reflects
the current gating behavior—either delete the "Informing/non-blocking" note or
replace it with a brief accurate comment about this being a blocking/required
test for pod workload validation, referencing the It("all pods should belong to
predefined workloads") test to locate the 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5d1857a4-dbd4-49da-87c1-4d11b0b3371e
⛔ Files ignored due to path filters (3)
cmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/stable.hostedclusters.capabilities.testsuite.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/stable.hostedclusters.services.testsuite.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/stable.hostedclusters.validation.testsuite.yamlis excluded by!cmd/install/assets/**/*.yaml
📒 Files selected for processing (13)
control-plane-operator/hostedclusterconfigoperator/controllers/hcpstatus/hcpstatus_test.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.gosupport/controlplane-component/controlplane-component_test.gotest/e2e/v2/internal/env_vars.gotest/e2e/v2/internal/pod_exec.gotest/e2e/v2/internal/test_context.gotest/e2e/v2/tests/control_plane_workloads_test.gotest/e2e/v2/tests/hosted_cluster_compliance_test.gotest/e2e/v2/tests/hosted_cluster_dns_test.gotest/e2e/v2/tests/hosted_cluster_health_test.gotest/e2e/v2/tests/hosted_cluster_metrics_test.gotest/e2e/v2/tests/hosted_cluster_security_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8511 +/- ##
==========================================
+ Coverage 40.00% 40.09% +0.09%
==========================================
Files 751 751
Lines 92838 92838
==========================================
+ Hits 37137 37227 +90
+ Misses 53014 52921 -93
- Partials 2687 2690 +3 see 3 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:
|
cblecker
force-pushed
the
worktree-smooth-hatching-thompson
branch
from
9d54baa to
3748c05
Compare
Add onUpdate test cases to three existing envtest suites verifying that Services, ControllerAvailabilityPolicy, and Capabilities fields are immutable after HostedCluster creation. Assisted-by: Claude:claude-sonnet-4-6[1m]
…pod template Add toleration to HCP spec in TestReconcile and assert it appears in the pod template, exercising the defaults.go propagation path. Assisted-by: Claude:claude-sonnet-4-6[1m]
Add unit test for the hcpStatusReconciler confirming that the guest cluster Authentication resource is propagated to HCP.Status.Configuration. Assisted-by: Claude:claude-sonnet-4-6[1m]
Add TestPayloadArch verifying DetermineHostedClusterPayloadArch integration and TestConfigurationStatus verifying HCP-to-HC configuration status copy. Assisted-by: Claude:claude-sonnet-4-6[1m]
Add TestIsAllowedWebhookUrl (4 table-driven cases) and TestEnsureGuestAdmissionWebhooksAreValid (4 cases) covering webhook deletion for CP service targets and preservation of allowed URLs. Assisted-by: Claude:claude-sonnet-4-6[1m]
…e domain Add RunCommandInPod/GetMetricsFromPod helpers in pod_exec.go, GetHostedClusterRESTConfig() to TestContext, and E2E_SERVICE_DOMAIN env var registration for custom DNS endpoint testing. Assisted-by: Claude:claude-sonnet-4-6[1m]
Add hosted_cluster_health_test.go (conditions, crashing pods, CAPI finalizers, feature gate, payload arch, config status), security_test.go (guest webhooks, admission policies, network policies), compliance_test.go (custom labels, tolerations, HCP router routes), metrics_test.go (HO metrics, forwarder, NTO endpoint), and dns_test.go (custom DNS cert stub). Promote WorkloadRegistryValidationTest from Informing to blocking. Assisted-by: Claude:claude-sonnet-4-6[1m]
cblecker
force-pushed
the
worktree-smooth-hatching-thompson
branch
from
3748c05 to
64758af
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/hosted_cluster_compliance_test.go`:
- Line 106: The It block "When routes are created in the control plane
namespace, it should label all routes for the per-HCP router" is missing an
explicit Label; modify that It declaration to include a Label(...) argument
(e.g., Label("Compliance") or the same label used by other It blocks in this
file) so the test can be filtered consistently—update the It(...) signature to
It("When routes...", Label("..."), func() { ... }) preserving the existing
description and test body.
- Around line 123-132: The test currently allows a vacuous pass when no routes
exist; after fetching routeList with
tc.MgmtClient.List(namespace=tc.ControlPlaneNamespace), assert that
routeList.Items is non-empty (e.g.,
Expect(len(routeList.Items)).To(BeNumerically(">", 0))) before iterating, then
continue to iterate each route (route := &routeList.Items[i]) and use
netutil.AddHCPRouteLabel and the existing Expect comparing route.Labels to
original.Labels so the test fails when there are zero routes instead of passing
silently.
- Around line 108-121: The test currently proceeds when tc.GetHostedCluster()
returns nil, which hides a missing precondition; update the code after calling
tc.GetHostedCluster() to fail fast if hostedCluster is nil (e.g., call t.Fatalf
or require.NotNil on the hostedCluster) so the test stops with a clear error
instead of continuing into the Route gating logic that uses
hostedCluster.Spec.Services; keep the existing APIServer/Route check and Skip()
only for the non-nil hostedCluster branch.
🪄 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: bf3b2881-c543-4a73-ab34-21aff8e58c03
⛔ Files ignored due to path filters (3)
cmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/stable.hostedclusters.capabilities.testsuite.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/stable.hostedclusters.services.testsuite.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/stable.hostedclusters.validation.testsuite.yamlis excluded by!cmd/install/assets/**/*.yaml
📒 Files selected for processing (13)
control-plane-operator/hostedclusterconfigoperator/controllers/hcpstatus/hcpstatus_test.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.gosupport/controlplane-component/controlplane-component_test.gotest/e2e/v2/internal/env_vars.gotest/e2e/v2/internal/pod_exec.gotest/e2e/v2/internal/test_context.gotest/e2e/v2/tests/control_plane_workloads_test.gotest/e2e/v2/tests/hosted_cluster_compliance_test.gotest/e2e/v2/tests/hosted_cluster_dns_test.gotest/e2e/v2/tests/hosted_cluster_health_test.gotest/e2e/v2/tests/hosted_cluster_metrics_test.gotest/e2e/v2/tests/hosted_cluster_security_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- control-plane-operator/hostedclusterconfigoperator/controllers/hcpstatus/hcpstatus_test.go
- test/e2e/v2/tests/control_plane_workloads_test.go
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go
- support/controlplane-component/controlplane-component_test.go
- test/e2e/v2/tests/hosted_cluster_dns_test.go
- test/e2e/v2/internal/test_context.go
- test/e2e/v2/tests/hosted_cluster_metrics_test.go
- test/e2e/v2/tests/hosted_cluster_security_test.go
- test/e2e/v2/tests/hosted_cluster_health_test.go
| // have the per-HCP router label applied. This test is skipped when the APIServer is not | ||
| // exposed via a Route service publishing strategy. | ||
| func EnsureAllRoutesUseHCPRouterTest(getTestCtx internal.TestContextGetter) { | ||
| It("When routes are created in the control plane namespace, it should label all routes for the per-HCP router", func() { |
Contributor
There was a problem hiding this comment.
Add an explicit label to this It block.
This test is currently unlabeled while other It blocks in the same file are labeled, which makes filtering less consistent.
Suggested change
- It("When routes are created in the control plane namespace, it should label all routes for the per-HCP router", func() {
+ It("When routes are created in the control plane namespace, it should label all routes for the per-HCP router", Label("hosted-cluster-compliance"), func() {As per coding guidelines, "Apply labels to Describe and It blocks for test filtering; only apply labels to Context blocks when you have explicit filtering intent (e.g., Label("Informing") for non-blocking tests)".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| It("When routes are created in the control plane namespace, it should label all routes for the per-HCP router", func() { | |
| It("When routes are created in the control plane namespace, it should label all routes for the per-HCP router", Label("hosted-cluster-compliance"), func() { |
🤖 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/hosted_cluster_compliance_test.go` at line 106, The It
block "When routes are created in the control plane namespace, it should label
all routes for the per-HCP router" is missing an explicit Label; modify that It
declaration to include a Label(...) argument (e.g., Label("Compliance") or the
same label used by other It blocks in this file) so the test can be filtered
consistently—update the It(...) signature to It("When routes...", Label("..."),
func() { ... }) preserving the existing description and test body.
Comment on lines
+108
to
+121
| hostedCluster := tc.GetHostedCluster() | ||
|
|
||
| if hostedCluster != nil { | ||
| isRoute := false | ||
| for _, svc := range hostedCluster.Spec.Services { | ||
| if svc.Service == hyperv1.APIServer && svc.Type == hyperv1.Route { | ||
| isRoute = true | ||
| break | ||
| } | ||
| } | ||
| if !isRoute { | ||
| Skip("route test only applies when APIServer is exposed via Route") | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Fail fast if hosted-cluster context is unexpectedly missing.
If tc.GetHostedCluster() returns nil, this test bypasses service-strategy gating and can produce misleading outcomes instead of a clear precondition failure.
Suggested change
tc := getTestCtx()
hostedCluster := tc.GetHostedCluster()
+ Expect(hostedCluster).NotTo(BeNil(),
+ "hosted cluster must be available for route compliance validation in namespace %s",
+ tc.ControlPlaneNamespace)
- if hostedCluster != nil {
- isRoute := false
- for _, svc := range hostedCluster.Spec.Services {
- if svc.Service == hyperv1.APIServer && svc.Type == hyperv1.Route {
- isRoute = true
- break
- }
+ isRoute := false
+ for _, svc := range hostedCluster.Spec.Services {
+ if svc.Service == hyperv1.APIServer && svc.Type == hyperv1.Route {
+ isRoute = true
+ break
}
- if !isRoute {
- Skip("route test only applies when APIServer is exposed via Route")
- }
+ }
+ if !isRoute {
+ Skip("route test only applies when APIServer is exposed via Route")
}🤖 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/hosted_cluster_compliance_test.go` around lines 108 - 121,
The test currently proceeds when tc.GetHostedCluster() returns nil, which hides
a missing precondition; update the code after calling tc.GetHostedCluster() to
fail fast if hostedCluster is nil (e.g., call t.Fatalf or require.NotNil on the
hostedCluster) so the test stops with a clear error instead of continuing into
the Route gating logic that uses hostedCluster.Spec.Services; keep the existing
APIServer/Route check and Skip() only for the non-nil hostedCluster branch.
Comment on lines
+123
to
+132
| routeList := &routev1.RouteList{} | ||
| Expect(tc.MgmtClient.List(tc.Context, routeList, crclient.InNamespace(tc.ControlPlaneNamespace))).To(Succeed()) | ||
|
|
||
| for i := range routeList.Items { | ||
| route := &routeList.Items[i] | ||
| original := route.DeepCopy() | ||
| netutil.AddHCPRouteLabel(route) | ||
| Expect(route.Labels).To(Equal(original.Labels), | ||
| "route %s is missing the label to use the per-HCP router", route.Name) | ||
| } |
Contributor
There was a problem hiding this comment.
Guard against vacuous pass when no routes are present.
The per-route label assertion passes trivially on an empty list, which can hide regressions in route creation/visibility for this namespace.
Suggested change
routeList := &routev1.RouteList{}
Expect(tc.MgmtClient.List(tc.Context, routeList, crclient.InNamespace(tc.ControlPlaneNamespace))).To(Succeed())
+ Expect(routeList.Items).NotTo(BeEmpty(),
+ "expected at least one route in control plane namespace %s for router-label compliance check",
+ tc.ControlPlaneNamespace)
for i := range routeList.Items {
route := &routeList.Items[i]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| routeList := &routev1.RouteList{} | |
| Expect(tc.MgmtClient.List(tc.Context, routeList, crclient.InNamespace(tc.ControlPlaneNamespace))).To(Succeed()) | |
| for i := range routeList.Items { | |
| route := &routeList.Items[i] | |
| original := route.DeepCopy() | |
| netutil.AddHCPRouteLabel(route) | |
| Expect(route.Labels).To(Equal(original.Labels), | |
| "route %s is missing the label to use the per-HCP router", route.Name) | |
| } | |
| routeList := &routev1.RouteList{} | |
| Expect(tc.MgmtClient.List(tc.Context, routeList, crclient.InNamespace(tc.ControlPlaneNamespace))).To(Succeed()) | |
| Expect(routeList.Items).NotTo(BeEmpty(), | |
| "expected at least one route in control plane namespace %s for router-label compliance check", | |
| tc.ControlPlaneNamespace) | |
| for i := range routeList.Items { | |
| route := &routeList.Items[i] | |
| original := route.DeepCopy() | |
| netutil.AddHCPRouteLabel(route) | |
| Expect(route.Labels).To(Equal(original.Labels), | |
| "route %s is missing the label to use the per-HCP router", route.Name) | |
| } |
🤖 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/hosted_cluster_compliance_test.go` around lines 123 - 132,
The test currently allows a vacuous pass when no routes exist; after fetching
routeList with tc.MgmtClient.List(namespace=tc.ControlPlaneNamespace), assert
that routeList.Items is non-empty (e.g.,
Expect(len(routeList.Items)).To(BeNumerically(">", 0))) before iterating, then
continue to iterate each route (route := &routeList.Items[i]) and use
netutil.AddHCPRouteLabel and the existing Expect comparing route.Labels to
original.Labels so the test fails when there are zero routes instead of passing
silently.
Member
Author
|
/test e2e-v2-gke |
Contributor
|
@cblecker: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
onUpdateimmutability tests for Services, ControllerAvailabilityPolicy, and Capabilities fields; unit tests for toleration propagation, PayloadArch/ConfigurationStatus controller integration, hcpstatus Authentication propagation, and guest webhook URL allowlist validationRunCommandInPod/GetMetricsFromPodpod exec helpers,GetHostedClusterRESTConfig()on TestContext, andE2E_SERVICE_DOMAINenv var registrationTestCreateClustervalidations to v2 Ginkgo specs across 5 new test files (health, security, compliance, metrics, DNS); promoteWorkloadRegistryValidationTestfromInformingto blockingTest plan
make test-envtest-ocp— validates onUpdate immutability cases in Phase 1Ago test ./support/controlplane-component/... -run TestReconcile— Phase 1B toleration testgo test ./hypershift-operator/controllers/hostedcluster/... -run "TestPayloadArch|TestConfigurationStatus"— Phase 1C/1D controller testsgo test ./control-plane-operator/hostedclusterconfigoperator/controllers/hcpstatus/...— Phase 1D hcpstatus testgo test github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/controllers/resources -run "TestIsAllowedWebhookUrl|TestEnsureGuestAdmissionWebhooksAreValid"— Phase 1E webhook testsmake e2ev2— validates Phase 2 compiles cleanlye2e-v2-gkejob must pass with Phase 2 changesSummary by CodeRabbit
Tests
Chores