CNTRLPLANE-2916: restore conditional deletion of openshift-ingress NetworkPolicy - #8754
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@csrwng: This pull request references CNTRLPLANE-2916 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: csrwng 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
approved
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8754 +/- ##
==========================================
- Coverage 41.83% 41.79% -0.04%
==========================================
Files 759 759
Lines 94043 94048 +5
==========================================
- Hits 39342 39309 -33
- Misses 51946 51987 +41
+ Partials 2755 2752 -3
... and 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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hypershift-operator/controllers/hostedcluster/network_policies_test.go (1)
477-497: ⚡ Quick winAdd an explicit KubeVirt delete-path case in the ingress policy matrix.
KubeVirt currently has only the create-path assertion. Please add a KubeVirt case where APIServer uses
Routewith hostname and assert deletion, so the matrix directly covers both outcomes for that platform and guards future branch drift.💡 Suggested test-case addition
{ name: "When KubeVirt cluster uses KAS LoadBalancer it should create policy", hcluster: &hyperv1.HostedCluster{ ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "test-ns"}, Spec: hyperv1.HostedClusterSpec{ Platform: hyperv1.PlatformSpec{Type: hyperv1.KubevirtPlatform, Kubevirt: &hyperv1.KubevirtPlatformSpec{}}, Services: []hyperv1.ServicePublishingStrategyMapping{ {Service: hyperv1.APIServer, ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{Type: hyperv1.LoadBalancer}}, }, }, }, hcp: &hyperv1.HostedControlPlane{ Spec: hyperv1.HostedControlPlaneSpec{ Platform: hyperv1.PlatformSpec{Type: hyperv1.KubevirtPlatform, Kubevirt: &hyperv1.KubevirtPlatformSpec{}}, Services: []hyperv1.ServicePublishingStrategyMapping{ {Service: hyperv1.APIServer, ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{Type: hyperv1.LoadBalancer}}, }, }, }, expectCreated: true, }, + { + name: "When KubeVirt cluster uses KAS Route with hostname it should delete policy", + hcluster: &hyperv1.HostedCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "test-ns"}, + Spec: hyperv1.HostedClusterSpec{ + Platform: hyperv1.PlatformSpec{Type: hyperv1.KubevirtPlatform, Kubevirt: &hyperv1.KubevirtPlatformSpec{}}, + Services: []hyperv1.ServicePublishingStrategyMapping{ + {Service: hyperv1.APIServer, ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.Route, + Route: &hyperv1.RoutePublishingStrategy{Hostname: "api.example.com"}, + }}, + }, + }, + }, + hcp: &hyperv1.HostedControlPlane{ + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{Type: hyperv1.KubevirtPlatform, Kubevirt: &hyperv1.KubevirtPlatformSpec{}}, + Services: []hyperv1.ServicePublishingStrategyMapping{ + {Service: hyperv1.APIServer, ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.Route, + Route: &hyperv1.RoutePublishingStrategy{Hostname: "api.example.com"}, + }}, + }, + }, + }, + expectDeleted: true, + },As per coding guidelines, "Unit test any code changes and additions and include e2e tests when changes impact consumer behaviour."
🤖 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 `@hypershift-operator/controllers/hostedcluster/network_policies_test.go` around lines 477 - 497, The KubeVirt platform test case in the test matrix only covers the creation path where APIServer uses LoadBalancer type. Add a new test case entry following the existing KubeVirt test to cover the deletion path by creating a case where the KubeVirt cluster uses Route for the APIServer ServicePublishingStrategy (instead of LoadBalancer) and set expectCreated to false. This ensures the test matrix covers both creation and deletion outcomes for the KubeVirt platform and prevents future branch drift.Source: Coding guidelines
🤖 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 `@hypershift-operator/controllers/hostedcluster/network_policies_test.go`:
- Around line 477-497: The KubeVirt platform test case in the test matrix only
covers the creation path where APIServer uses LoadBalancer type. Add a new test
case entry following the existing KubeVirt test to cover the deletion path by
creating a case where the KubeVirt cluster uses Route for the APIServer
ServicePublishingStrategy (instead of LoadBalancer) and set expectCreated to
false. This ensures the test matrix covers both creation and deletion outcomes
for the KubeVirt platform and prevents future branch drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 28896680-8e1c-4b7c-9df7-5e15e637232e
📒 Files selected for processing (2)
hypershift-operator/controllers/hostedcluster/network_policies.gohypershift-operator/controllers/hostedcluster/network_policies_test.go
…gress NetworkPolicy Reverts the revert PR openshift#8662 to re-apply the original PR openshift#7872 behavior: conditionally create or delete the openshift-ingress NetworkPolicy based on whether routes are labeled for the HCP router (LabelHCPRoutes). The OVN-Kubernetes port group race that prompted the revert is being addressed via PR openshift#8689 (--hcp-egress-block-cidrs flag), making the blanket always-create workaround unnecessary. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/lgtm |
|
Scheduling tests matching the |
|
/test e2e-aks-422
|
|
/test e2e-aks-4-22 |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
Test Resultse2e-aws
e2e-aks
|
|
/test e2e-aks From failure analysis:
|
|
/test e2e-aws-4-22 This test failed the Karpenter test in the same way that e2e-aks-4-22 failed (and eventually passed): |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/retest-required Failures in e2e-aks and kubevirt are not related to this PR |
|
I now have all the evidence needed for the complete report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe PR refactored the openshift-ingress NetworkPolicy reconciliation from an unconditional
The 8 uncovered lines break down as:
The tests are otherwise well-written and thorough — they cover 10 platform/access-mode combinations across AWS, GCP, IBMCloud, Agent, and KubeVirt. The gap is specifically in error injection: no test case forces The aggregate project coverage dropped -0.04% due to these 8 uncovered lines plus indirect coverage drift in 3 other files (the Codecov baseline is 29 commits stale against Recommendations
Evidence
|
|
/override codecov/project |
|
@csrwng: Overrode contexts on behalf of csrwng: codecov/project 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 kubernetes-sigs/prow repository. |
openshift-ci-robot
added
verified-later
verified
|
@csrwng: This PR has been marked to be verified later 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. |
|
@csrwng: 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. |
49cdfcf
into
openshift:main
4 participants
Summary
openshift-ingressNetworkPolicy based on whether routes are labeled for the HCP router (LabelHCPRoutes)--hcp-egress-block-cidrsflag), making the blanket always-create workaround unnecessaryTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests