DNM/REFACTOR EFFORT: OCPBUGS-81741: Watch Network and Infrastructure in proxyconfig controller#3033
DNM/REFACTOR EFFORT: OCPBUGS-81741: Watch Network and Infrastructure in proxyconfig controller#3033jluhrsen wants to merge 5 commits into
Conversation
- Extract shared proxy validation and trust bundle generation. - Remove duplicated construction logic from both reconcile paths. - Preserve existing validation order and degraded status behavior.
- Move Proxy event handling into reconcileProxy. - Keep dependency reads and Proxy status updates together. - Preserve existing errors, logs, and reconciliation ordering.
- Move additional trust bundle handling into a focused helper. - Reduce Reconcile to request dispatch and final bundle synchronization. - Retain ConfigMap-specific validation and degraded reasons.
- Rename local objects to match their API resource types. - Distinguish the cluster ConfigMap from cluster-scoped configuration. - Keep the change mechanical with no control-flow changes.
- Watch Network and Infrastructure changes through the Proxy request. - Recompute Proxy status from current cluster configuration. - Add fake-client status support and regression coverage.
openshift-ci-robot
added
jira/severity-important
|
@jluhrsen: This pull request references Jira Issue OCPBUGS-81741, which is valid. 3 validation(s) were run on this bug
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. |
WalkthroughThe proxyconfig controller gains watches on Changesproxyconfig Controller: Network/Infrastructure watches, reconcile refactor, and tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 passed)
✨ 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/controller/proxyconfig/controller.go (1)
230-268: ⚖️ Poor tradeoffDuplicate validation and merge when trustedCA is set.
When the ConfigMap matches proxy.Spec.TrustedCA.Name,
validateTrustedCAandmergeTrustBundlesToConfigMapare called here (lines 231, 243), then called again insidedesiredTrustBundle(lines 308, 319). The first merge result is discarded.Consider passing the already-validated data to
desiredTrustBundleor refactoring to avoid duplicate work.🤖 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 `@pkg/controller/proxyconfig/controller.go` around lines 230 - 268, The validateTrustedCA and mergeTrustBundlesToConfigMap functions are being called twice unnecessarily: once in the current code block (validateTrustedCA and mergeTrustBundlesToConfigMap) and again inside the desiredTrustBundle function, with the first result being discarded. Refactor the desiredTrustBundle function to accept the already-validated proxyData and systemData parameters that were computed earlier, eliminating the duplicate validation and merge operations, or alternatively restructure the code to only perform these operations once and reuse the results in desiredTrustBundle.
🤖 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`:
- Around line 188-195: In the error handling block where r.client.Get fails for
the cluster config, replace the references to clusterConfigMap.Namespace and
clusterConfigMap.Name with the literal hardcoded values "kube-system" and
"cluster-config-v1" respectively. Since the Get operation failed, the
clusterConfigMap struct was never populated, so these fields are empty strings.
Update all three places where the namespace and name are logged: the log.Printf
call, the status message in r.status.MaybeSetDegraded, and the returned error
message.
---
Nitpick comments:
In `@pkg/controller/proxyconfig/controller.go`:
- Around line 230-268: The validateTrustedCA and mergeTrustBundlesToConfigMap
functions are being called twice unnecessarily: once in the current code block
(validateTrustedCA and mergeTrustBundlesToConfigMap) and again inside the
desiredTrustBundle function, with the first result being discarded. Refactor the
desiredTrustBundle function to accept the already-validated proxyData and
systemData parameters that were computed earlier, eliminating the duplicate
validation and merge operations, or alternatively restructure the code to only
perform these operations once and reuse the results in desiredTrustBundle.
🪄 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: ec4ca8cc-5198-4ba3-83b9-e6457b3a4ff9
📒 Files selected for processing (3)
pkg/client/fake/fake_client.gopkg/controller/proxyconfig/controller.gopkg/controller/proxyconfig/controller_test.go
| if err := r.client.Get(ctx, types.NamespacedName{Name: "cluster-config-v1", Namespace: "kube-system"}, | ||
| clusterConfigMap); err != nil { | ||
| log.Printf("Failed to get configmap '%s/%s': %v", clusterConfigMap.Namespace, clusterConfigMap.Name, err) | ||
| r.status.MaybeSetDegraded(statusmanager.ProxyConfig, "ClusterConfigError", | ||
| fmt.Sprintf("Error getting cluster config configmap '%s/%s': %v.", clusterConfigMap.Namespace, | ||
| clusterConfigMap.Name, err)) | ||
| return nil, fmt.Errorf("failed to get configmap '%s/%s': %v", clusterConfigMap.Namespace, clusterConfigMap.Name, err) | ||
| } |
There was a problem hiding this comment.
Log message uses unpopulated struct fields on Get failure.
When Get fails, clusterConfigMap.Namespace and clusterConfigMap.Name are empty strings since the struct wasn't populated. The log would show Failed to get configmap '/': instead of the intended namespace/name.
Proposed fix
- if err := r.client.Get(ctx, types.NamespacedName{Name: "cluster-config-v1", Namespace: "kube-system"},
- clusterConfigMap); err != nil {
- log.Printf("Failed to get configmap '%s/%s': %v", clusterConfigMap.Namespace, clusterConfigMap.Name, err)
+ clusterConfigKey := types.NamespacedName{Name: "cluster-config-v1", Namespace: "kube-system"}
+ if err := r.client.Get(ctx, clusterConfigKey, clusterConfigMap); err != nil {
+ log.Printf("Failed to get configmap '%s/%s': %v", clusterConfigKey.Namespace, clusterConfigKey.Name, err)
r.status.MaybeSetDegraded(statusmanager.ProxyConfig, "ClusterConfigError",
- fmt.Sprintf("Error getting cluster config configmap '%s/%s': %v.", clusterConfigMap.Namespace,
- clusterConfigMap.Name, err))
- return nil, fmt.Errorf("failed to get configmap '%s/%s': %v", clusterConfigMap.Namespace, clusterConfigMap.Name, err)
+ fmt.Sprintf("Error getting cluster config configmap '%s/%s': %v.", clusterConfigKey.Namespace,
+ clusterConfigKey.Name, err))
+ return nil, fmt.Errorf("failed to get configmap '%s/%s': %v", clusterConfigKey.Namespace, clusterConfigKey.Name, err)
}📝 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.
| if err := r.client.Get(ctx, types.NamespacedName{Name: "cluster-config-v1", Namespace: "kube-system"}, | |
| clusterConfigMap); err != nil { | |
| log.Printf("Failed to get configmap '%s/%s': %v", clusterConfigMap.Namespace, clusterConfigMap.Name, err) | |
| r.status.MaybeSetDegraded(statusmanager.ProxyConfig, "ClusterConfigError", | |
| fmt.Sprintf("Error getting cluster config configmap '%s/%s': %v.", clusterConfigMap.Namespace, | |
| clusterConfigMap.Name, err)) | |
| return nil, fmt.Errorf("failed to get configmap '%s/%s': %v", clusterConfigMap.Namespace, clusterConfigMap.Name, err) | |
| } | |
| clusterConfigKey := types.NamespacedName{Name: "cluster-config-v1", Namespace: "kube-system"} | |
| if err := r.client.Get(ctx, clusterConfigKey, clusterConfigMap); err != nil { | |
| log.Printf("Failed to get configmap '%s/%s': %v", clusterConfigKey.Namespace, clusterConfigKey.Name, err) | |
| r.status.MaybeSetDegraded(statusmanager.ProxyConfig, "ClusterConfigError", | |
| fmt.Sprintf("Error getting cluster config configmap '%s/%s': %v.", clusterConfigKey.Namespace, | |
| clusterConfigKey.Name, err)) | |
| return nil, fmt.Errorf("failed to get configmap '%s/%s': %v", clusterConfigKey.Namespace, clusterConfigKey.Name, err) | |
| } |
🤖 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 `@pkg/controller/proxyconfig/controller.go` around lines 188 - 195, In the
error handling block where r.client.Get fails for the cluster config, replace
the references to clusterConfigMap.Namespace and clusterConfigMap.Name with the
literal hardcoded values "kube-system" and "cluster-config-v1" respectively.
Since the Get operation failed, the clusterConfigMap struct was never populated,
so these fields are empty strings. Update all three places where the namespace
and name are logged: the log.Printf call, the status message in
r.status.MaybeSetDegraded, and the returned error message.
|
@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
Summary by CodeRabbit
Release Notes
New Features
Tests