OCPBUGS-81741: Watch Network and Infrastructure in proxyconfig controller#2968
OCPBUGS-81741: Watch Network and Infrastructure in proxyconfig controller#2968jluhrsen wants to merge 1 commit into
Conversation
openshift-ci-robot
added
jira/severity-important
|
@jluhrsen: This pull request references Jira Issue OCPBUGS-81741, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
openshift-ci-robot
added
the
jira/invalid-bug
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
WalkthroughAdds Proxy to the fake client's status subresources, registers watches for configv1.Network and configv1.Infrastructure in the proxycontroller, and adds unit tests that verify Proxy.Status.NoProxy updates from Network and Infrastructure changes and error handling when resources are missing. ChangesProxy status and watches
Sequence Diagram(s)sequenceDiagram
participant Network as Network (configv1)
participant Infrastructure as Infrastructure (configv1)
participant Controller as Controller
participant Reconciler as Reconciler
participant Proxy as Proxy (configv1)
Note over Network,Infrastructure: Resource change events
Network->>Controller: change event (ClusterNetwork)
Infrastructure->>Controller: change event (APIServerInternalURL)
Controller->>Reconciler: Enqueue reconcile request
Reconciler->>Network: Read ClusterNetwork CIDRs
Reconciler->>Infrastructure: Read APIServerInternalURL
Reconciler->>Proxy: Update Status.NoProxy (CIDRs + hostname)
Proxy->>Reconciler: Status updated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in :\n\tgithub.com/Masterminds/semver@v1.5.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/Masterminds/sprig/v3@v3.2.3: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/containernetworking/cni@v0.8.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ghodss/yaml@v1.0.1-0.20190212211648-25d852aebe32: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/go-bindata/go-bindata@v3.1.2+incompatible: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/onsi/gomega@v1.39.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ope ... [truncated 17357 characters] ... red in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/gengo/v2@v2.0.0-20251215205346-5ee0d033ba5b: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/kms@v0.35.2: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/kube-aggregator@v0.35.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/randfill@v1.0.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/structured-merge-diff/v6@v6.3.2: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jluhrsen 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.
🧹 Nitpick comments (1)
pkg/controller/proxyconfig/controller_test.go (1)
220-228: Also assert that the old API hostname is removed.Right now this test only proves the new hostname was added. It would still pass if reconciliation appended the new host without dropping the stale one.
Suggested assertion
if !strings.Contains(proxy.Status.NoProxy, updatedAPIServer) { t.Errorf("Expected proxy.Status.NoProxy to contain updated API server %s, got: %s", updatedAPIServer, proxy.Status.NoProxy) } + if strings.Contains(proxy.Status.NoProxy, initialAPIServer) { + t.Errorf("proxy.Status.NoProxy still contains old API server %s, got: %s", + initialAPIServer, proxy.Status.NoProxy) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/proxyconfig/controller_test.go` around lines 220 - 228, The test currently only asserts that proxy.Status.NoProxy contains updatedAPIServer; also assert that the previous API hostname is removed by checking that strings.Contains(proxy.Status.NoProxy, oldAPIServer) is false (use whatever variable name holds the pre-update hostname in this test), i.e., add an assertion after fetching proxy that proxy.Status.NoProxy does NOT contain the old API hostname to ensure reconciliation replaced rather than appended the host.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/controller/proxyconfig/controller_test.go`:
- Around line 220-228: The test currently only asserts that proxy.Status.NoProxy
contains updatedAPIServer; also assert that the previous API hostname is removed
by checking that strings.Contains(proxy.Status.NoProxy, oldAPIServer) is false
(use whatever variable name holds the pre-update hostname in this test), i.e.,
add an assertion after fetching proxy that proxy.Status.NoProxy does NOT contain
the old API hostname to ensure reconciliation replaced rather than appended the
host.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 41e9f990-8f6b-40f0-b896-52eb993948a5
📒 Files selected for processing (3)
pkg/client/fake/fake_client.gopkg/controller/proxyconfig/controller.gopkg/controller/proxyconfig/controller_test.go
|
/retest |
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@jluhrsen: This pull request references Jira Issue OCPBUGS-81741, which is valid. 3 validation(s) were run on this bug
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. |
openshift-ci-robot
removed
the
jira/invalid-bug
The proxyconfig controller reads Network.Status.ClusterNetwork and Infrastructure.Status to compute Proxy.Status.NoProxy, but only watched Proxy and ConfigMaps. Network or Infrastructure changes would not trigger reconciliation, leaving proxy status stale. Add watches for Network and Infrastructure resources to ensure reconciliation occurs when these resources change. Also add Proxy status subresource support to fake client and unit tests covering reconciliation logic. Signed-off-by: Jamo Luhrsen <jluhrsen@gmail.com> Co-authored-by: Claude Code <noreply@anthropic.com)
jluhrsen
force-pushed
the
OCPBUGS-81741-master
branch
from
16d068b to
74e43ba
Compare
|
@danwinship @tssurya @kyrtapz @pliurh looking for someone to check on this. it's been sitting idle on my open bug list for a while now. haven't really followed up to find a reviewer yet. I've verified the fix manually and passed this through coderabbit review (locally). should be good for a human to look now. |
|
@jluhrsen: The following tests 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. |
2 participants
The proxyconfig controller reads Network.Status.ClusterNetwork and Infrastructure.Status to compute Proxy.Status.NoProxy, but only watched Proxy and ConfigMaps. Network or Infrastructure changes would not trigger reconciliation, leaving proxy status stale.
Add watches for Network and Infrastructure resources to ensure reconciliation occurs when these resources change.
Also add Proxy status subresource support to fake client and unit tests covering reconciliation logic.
Co-authored-by: Claude Code <noreply@anthropic.com)
Summary by CodeRabbit
Tests
Improvements
Chores