OCPBUGS-60993: Enrich IBI config image proxy NoProxy with cluster networks#10649
OCPBUGS-60993: Enrich IBI config image proxy NoProxy with cluster networks#10649mlorenzofr wants to merge 2 commits into
Conversation
Remove the Networking asset dependency from createNoProxy and use installConfig directly for ClusterNetwork, matching the existing approach for ServiceNetwork and MachineNetwork. The Networking asset performs no business logic on ClusterNetwork data, only type conversion from ipnet.IPNet to string. Using installConfig directly for all three network types makes the code more consistent and removes an unnecessary dependency.
openshift-ci-robot
added
jira/severity-critical
|
@mlorenzofr: This pull request references Jira Issue OCPBUGS-60993, 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. |
|
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 (5)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughA shared ChangesCentralized NO_PROXY construction
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)
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)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
Please git-fixup your additional commit into the appropriate existing commit |
mlorenzofr
force-pushed
the
fix-ibi-no-proxy-v2
branch
from
603ed9d to
beaf436
Compare
|
/retest-required |
|
/retest |
mlorenzofr
force-pushed
the
fix-ibi-no-proxy-v2
branch
from
beaf436 to
ccca0dc
Compare
|
/retest |
IBI cluster configuration was forwarding the user-provided Proxy config directly without adding cluster, service, and machine network CIDRs to NoProxy, which would cause in-cluster traffic to be routed through the proxy. Extract BuildNoProxySet into pkg/types so the manifests proxy asset and the IBI config image asset share the same NoProxy building logic. Update tests accordingly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
mlorenzofr
force-pushed
the
fix-ibi-no-proxy-v2
branch
from
ccca0dc to
554180d
Compare
|
@mlorenzofr: 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. |
|
/lgtm |
|
/assign @dtantsur |
|
/jira refresh |
|
@andfasano: This pull request references Jira Issue OCPBUGS-60993, which is invalid:
Comment 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. |
andfasano
left a comment
andfasano
left a comment
There was a problem hiding this comment.
Just providing my 2 cents here, but leaving the final more authoritative review to @patrickdillon for this part.
The overall approach seems to be a little bit over-complicated and risky, especially because it modifies a widely used and common asset Proxy: in particular it removes the Network asset from the dependency list - apparently for extraction reasons, but still impacting the fundamental asset architecture.And moreover the Proxy asset is not used by the IBI subsystem.
From a longer term point of view, I think it'd be more useful to review the IBI part in order to reuse as much as possible the common downstream installer assets - but I understand that such step could be beyond the scope of this fix.
As a short term fix I think it'd be more acceptable to have just a local IBI fix (ie define enrichNoProxy fully within the imagebased/configimage folder) with a little bit of duplication, rather than impacting a commonly used asset.
| set.Insert(network.CIDR.String()) | ||
| } | ||
|
|
||
| for _, network := range config.Networking.ClusterNetwork { |
There was a problem hiding this comment.
I could be wrong bu this part doesn't sound correct to me, since previously the Network manifest as a source of truth (network.Config.Spec.ClusterNetwork). Reading directly from the installconfig it's not the same thing, and violates the previous dependency.
| @@ -82,7 +79,7 @@ func (p *Proxy) Generate(_ context.Context, dependencies asset.Parents) error { | |||
| } | |||
|
|
|||
| if p.Config.Spec.HTTPProxy != "" || p.Config.Spec.HTTPSProxy != "" { | |||
| noProxy, err := createNoProxy(installConfig, network) | |||
| noProxy, err := createNoProxy(installConfig) | |||
There was a problem hiding this comment.
See my comment below, removing the network asset as a source of truth from the dependency list doesn't sound a good idea.
|
/approve This is fine to fix the bug, but ideally we would not need to duplicate this logic between image based and non image based installs... |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon, trewest 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
|
@andfasano FWIW I explain the logic behind changing the dependencies in my first commit message. Hopefully that clears things up and makes people a bit more comfortable with the change.
|
8 participants
This PR fixes OCPBUGS-60993 by enriching the IBI proxy NoProxy configuration with internal cluster network CIDRs and the internal API server hostname, ensuring that internal cluster traffic bypasses the proxy.
/cc @omertuc @carbonin
Summary by CodeRabbit
Release Notes
Improvements
NO_PROXYhandling by enriching it with localhost defaults, internal service/API entries, and cluster/service/machine network CIDRs derived from the install configuration.NO_PROXYis already set to*.NO_PROXYvalues with trimming, empty-value filtering, and deduplication.Bug Fixes
NO_PROXYconsistency between proxy manifest generation and image-based installer manifest generation.Tests
NO_PROXYbehavior.