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
|
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:
WalkthroughAdds Proxy to the fake client's status subresources, registers watches for configv1.Network and configv1.Infrastructure in the proxy controller, adjusts reconcile dispatch/logging for cluster-scoped requests, and adds tests verifying Proxy.Status.NoProxy updates and error handling. 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 | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
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. |
|
/retest |
1 similar comment
|
/retest |
|
/test 4.22-upgrade-from-stable-4.21-e2e-azure-ovn-upgrade |
danwinship
left a comment
danwinship
left a comment
There was a problem hiding this comment.
(oops, I reviewed this the other day but I guess I forgot to hit "submit")
| } | ||
|
|
||
| // Watch for changes to the network resource. | ||
| err = c.Watch(source.Kind[crclient.Object](mgr.GetCache(), &configv1.Network{}, &handler.EnqueueRequestForObject{})) |
There was a problem hiding this comment.
This should require a change to Reconcile() too shouldn't it? (It checks what kind of object it's reconciling.)
Assuming that's correct, that means this patch doesn't actually work at all, which makes me feel like it probably ought to have had a corresponding e2e test?
There was a problem hiding this comment.
Thanks @danwinship . I tested this fix with a live cluster so I am pretty confident in the change, but obviously don't know this code base like you do.
But, I think it works because Reconcile() is interested in the "Name" which I guess always will be "cluster" for these.
I think an e2e might be complicated, but if you need one figured out to validate this even further, I try to figure it out.
There was a problem hiding this comment.
huh... you should fix Reconcile so that the switch does something that actually looks like it should work, rather than than something that just coincidentally happens to work. Possibly just split out the "configmap" and "not a configmap" cases.
There was a problem hiding this comment.
ok, I think I see what you mean. I gave it another try to hopefully make it a little more clear
jluhrsen
force-pushed
the
OCPBUGS-81741-master
branch
from
74e43ba to
67eeb38
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/controller/proxyconfig/controller.go`:
- Line 117: The log call using log.Println in the proxyconfig controller (the
line that currently reads "Ignoring unknown cluster-scoped object,
reconciliation will be skipped", "request", request) should be changed to use
log.Printf with a format specifier so the request is rendered correctly; update
the logging in the reconcile/controller function where that log.Println appears
(reference the log.Println call) to something like log.Printf("Ignoring unknown
cluster-scoped object, reconciliation will be skipped: request=%v", request) so
the request value is formatted into the message.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f179ee0c-097c-4ca1-b085-57f324f1b0c9
📒 Files selected for processing (3)
pkg/client/fake/fake_client.gopkg/controller/proxyconfig/controller.gopkg/controller/proxyconfig/controller_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/client/fake/fake_client.go
- pkg/controller/proxyconfig/controller_test.go
jluhrsen
force-pushed
the
OCPBUGS-81741-master
branch
2 times, most recently
from
fc3adac to
9818f9c
Compare
|
/retest |
|
/test e2e-metal-ipi-ovn-dualstack-bgp-local-gw |
| switch request.Namespace { | ||
| case "": | ||
| if request.Name != names.CLUSTER_CONFIG { | ||
| log.Printf("Ignoring unknown cluster-scoped object, reconciliation will be skipped: request=%v", request) |
There was a problem hiding this comment.
Yeah, so what I meant before was, change Reconcile() so that basically it checks "does request point to a ConfigMap in ADDL_TRUST_BUNDLE_CONFIGMAP_NS", and if it does, then it does that case, and if not, then it doesn't look at request again, it just assumes it's supposed to re-resolve the proxy config from the top down.
Maybe split those out into two functions, reconcileTrustBundle(request) and reconcileProxyConfig().
| log.Printf("Reconciling proxy '%s'", request.Name) | ||
| if err := r.client.Get(ctx, request.NamespacedName, proxyConfig); err != nil { | ||
| log.Printf("Reconciling proxy '%s'", names.PROXY_CONFIG) | ||
| if err := r.client.Get(ctx, names.Proxy(), proxyConfig); err != nil { |
There was a problem hiding this comment.
(This change and the one below are correct.)
jluhrsen
force-pushed
the
OCPBUGS-81741-master
branch
from
9818f9c to
0afe4d3
Compare
|
/retest |
1 similar comment
|
/retest |
|
So it's hard to point out in the diff, but it seems that a bunch of code got deleted in the rewrite. Before, the trust bundle case was now it's That is, previously, half of the "Reconciling proxy" case was duplicated into the "Reconciling additional trust bundle" case, and now it isn't any longer. (The duplication of the code before was bad and ought to have been done in a better way, but we need to make sure we're doing all of the necessary steps. Or were those steps unnecessary?) (Sorry, I'm not really familiar with this code, so I keep discovering more things as the diff touches more parts of the file...) |
jluhrsen
force-pushed
the
OCPBUGS-81741-master
branch
from
0afe4d3 to
e4f6dcd
Compare
@danwinship , you are many times more familiar with this code base than I am, so no worries there. I think the short answer is, yes those duplication steps were unnecessary, but maybe it wasn't perfectly de-duped 🤷🏻 the problem is that the original fix here was small and worked, but I get the confusion. so now we are in more of a refactor mode on top of that fix, and claude is recognizing that duplication and refactoring as it sees fit. I've spun the changes through codex and coderabbit double and triple checking that they believe functionality is not affected here. and yes, apparently there was unneeded duplication that got removed. However, I've posted a new try. keeping the duplication so the review is easier to look at. But, it's not breaking things out in to your idea of two functions, Does it, at least, make what's happening in Reconcile() a little more clear? |
|
also, @danwinship , I created this as maybe a better idea to do this kind of refactoring you are noticing we need. It tries to make smaller refactor commits for easier review. please don't spend time on it yet, but let me know if you like this idea better and I'll take it further. I need to review it myself, let the e2e's run and who knows what the rabbit is gonna say about it. |
|
|
||
| switch { | ||
| case request.NamespacedName == names.Proxy(): | ||
| case request.Namespace != names.ADDL_TRUST_BUNDLE_CONFIGMAP_NS: |
There was a problem hiding this comment.
| case request.Namespace != names.ADDL_TRUST_BUNDLE_CONFIGMAP_NS: | |
| case request.Namespace != names.ADDL_TRUST_BUNDLE_CONFIGMAP_NS: | |
| // Additional trust bundles are handled below; for all other config changes, | |
| // regardless of which object changed, we just re-resolve everything starting | |
| // from the Proxy config. |
|
From what I was seeing when I looked at it before, it seemed like it might all boil down to but anyway, the current simple fix (with the added explanatory comment) is good enough I guess |
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. Refactor Reconcile so non-ConfigMap events re-resolve proxy config from the current cluster state rather than depending on the triggering request. 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
e4f6dcd to
8db6228
Compare
ok, thank you. if you think it's worth following up with the full refactor idea, let me know and I'll chase that down after this PR lands. |
|
/retest |
1 similar comment
|
/retest |
|
/lgtm I don't think the full refactor is worth it, given that neither of us really knows the code well enough to know exactly how it should be refactored 😬 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, jluhrsen 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
|
@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. |
3 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