OCPBUGS-77457: Respect proxy configuration on gwapi provisioning#1383
Conversation
openshift-ci-robot
added
jira/severity-important
|
@rikatz: This pull request references Jira Issue OCPBUGS-77457, 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. |
rikatz
force-pushed
the
use-system-proxy-for-istio
branch
from
f2521d5 to
7107797
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe changes add support for reading the OpenShift cluster-scoped 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@rikatz: This pull request references Jira Issue OCPBUGS-77457, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (iamin@redhat.com), skipping review request. 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. |
![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/operator/controller/gatewayclass/controller.go (1)
149-151: Scope Proxy watch to theclusterresource.This controller only consumes
Proxynamedcluster(Line 241). Adding a name predicate here avoids unnecessary reconciles and keeps intent explicit.Proposed refactor
+ isClusterProxy := predicate.NewPredicateFuncs(func(o client.Object) bool { + return o.GetName() == "cluster" + }) // Watch for Proxy configuration to set the right options on Istio resource - if err := c.Watch(source.Kind[client.Object](operatorCache, &configv1.Proxy{}, reconciler.enqueueRequestForSomeGatewayClass())); err != nil { + if err := c.Watch(source.Kind[client.Object](operatorCache, &configv1.Proxy{}, reconciler.enqueueRequestForSomeGatewayClass(), isClusterProxy)); err != nil { return nil, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/controller/gatewayclass/controller.go` around lines 149 - 151, The Proxy watch is currently broad; restrict c.Watch(source.Kind[client.Object](operatorCache, &configv1.Proxy{}, reconciler.enqueueRequestForSomeGatewayClass())) to only trigger for the Proxy named "cluster". Update the Watch call to include a name predicate (e.g., using controller-runtime predicate.Funcs or predicate.NewPredicateFuncs) that checks object.GetName() == "cluster" so only the configv1.Proxy named "cluster" causes reconciles via reconciler.enqueueRequestForSomeGatewayClass().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/operator/controller/gatewayclass/istio.go`:
- Around line 219-225: The code writes all three proxy keys into
istio.Spec.Values.MeshConfig.DefaultConfig.ProxyMetadata even when some proxycfg
fields are empty, leading to empty env vars; update the block that currently
checks if proxycfg.HTTPProxy != "" || proxycfg.HTTPSProxy != "" ||
proxycfg.NoProxy != "" to set each key conditionally—only assign
ProxyMetadata["HTTP_PROXY"] when proxycfg.HTTPProxy != "", only assign
ProxyMetadata["HTTPS_PROXY"] when proxycfg.HTTPSProxy != "", and only assign
ProxyMetadata["NO_PROXY"] when proxycfg.NoProxy != ""; ensure ProxyMetadata map
creation remains as-is (create if nil) before these conditional assignments.
---
Nitpick comments:
In `@pkg/operator/controller/gatewayclass/controller.go`:
- Around line 149-151: The Proxy watch is currently broad; restrict
c.Watch(source.Kind[client.Object](operatorCache, &configv1.Proxy{},
reconciler.enqueueRequestForSomeGatewayClass())) to only trigger for the Proxy
named "cluster". Update the Watch call to include a name predicate (e.g., using
controller-runtime predicate.Funcs or predicate.NewPredicateFuncs) that checks
object.GetName() == "cluster" so only the configv1.Proxy named "cluster" causes
reconciles via reconciler.enqueueRequestForSomeGatewayClass().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: c7f1262f-41c7-400b-b086-31a3ae34bd0d
📒 Files selected for processing (3)
pkg/operator/controller/gatewayclass/controller.gopkg/operator/controller/gatewayclass/controller_test.gopkg/operator/controller/gatewayclass/istio.go
rikatz
force-pushed
the
use-system-proxy-for-istio
branch
from
7107797 to
d47b784
Compare
|
@rikatz: This pull request references Jira Issue OCPBUGS-77457, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (iamin@redhat.com), skipping review request. 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. |
|
/retest |
|
@coderabbitai do a full analysis of this PR, but tell me just if there are any critical or major issues |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me read the actual code changes to do a thorough analysis. 🏁 Script executed: Length of output: 36084
🔴 Major: Missing name predicate on the Proxy watchIn // Current — no filter
if err := c.Watch(source.Kind[client.Object](operatorCache, &configv1.Proxy{},
reconciler.enqueueRequestForSomeGatewayClass())); err != nil {
return nil, err
}But isClusterProxy := predicate.NewPredicateFuncs(func(o client.Object) bool {
return o.GetName() == "cluster"
})
if err := c.Watch(source.Kind[client.Object](operatorCache, &configv1.Proxy{},
reconciler.enqueueRequestForSomeGatewayClass(), isClusterProxy)); err != nil {
return nil, err
}🟠 Major:
|
rikatz
force-pushed
the
use-system-proxy-for-istio
branch
from
d47b784 to
e6461cc
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/operator/controller/gatewayclass/controller_test.go (1)
54-65: Add regression cases for partial and removed proxy config.Please add table cases where only one/two proxy fields are set, and where proxy config transitions from set → empty/not-found, to lock in key omission/removal behavior.
Also applies to: 140-302
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/controller/gatewayclass/controller_test.go` around lines 54 - 65, Add table-driven test cases in controller_test.go around the proxyConfig and istio helpers to cover (1) partial proxy fields (only HTTP, only HTTPS, only NoProxy, and combinations of two) and (2) transitions where a proxy entry goes from set → empty string and set → not-found (i.e., removed from the config map). For each case, use the proxyConfig(...) constructor to create the initial cluster proxy status and the istio(...) helper to build the Istio resource, then assert the controller behavior/expected output when reconciling after the partial configs and after the transition from a populated proxy to an empty or missing proxy config; ensure you add explicit table rows for each permutation so omission/removal behavior is exercised and locked in.pkg/operator/controller/gatewayclass/controller.go (1)
243-278: Preserve explicit nil semantics for missing cluster Proxy.Line 244 and Line 276 currently pass a non-nil proxy pointer even when the
clusterProxy is not found. Consider passingnilin that path soextraConfig.proxyConfig != nilremains meaningful and future-safe.Suggested refactor
- var proxyConfig configv1.Proxy - if err := r.cache.Get(ctx, types.NamespacedName{Name: "cluster"}, &proxyConfig); err != nil && !errors.IsNotFound(err) { - return reconcile.Result{}, err - } + var proxyConfig *configv1.Proxy + var clusterProxy configv1.Proxy + if err := r.cache.Get(ctx, types.NamespacedName{Name: "cluster"}, &clusterProxy); err != nil { + if !errors.IsNotFound(err) { + return reconcile.Result{}, err + } + } else { + proxyConfig = &clusterProxy + } ... - if _, _, err := r.ensureIstio(ctx, &gatewayclass, istioVersion, &extraIstioConfig{ - proxyConfig: &proxyConfig, + if _, _, err := r.ensureIstio(ctx, &gatewayclass, istioVersion, &extraIstioConfig{ + proxyConfig: proxyConfig, }); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/controller/gatewayclass/controller.go` around lines 243 - 278, The code currently always passes a non-nil pointer for proxyConfig even when the "cluster" Proxy is missing; change the Get logic to preserve explicit nil semantics by using a proxy pointer variable (e.g., proxyConfigPtr *configv1.Proxy) initialized to nil, call r.cache.Get(ctx, types.NamespacedName{Name: "cluster"}, &proxyConfig) and if err == nil set proxyConfigPtr = &proxyConfig, if errors.IsNotFound(err) leave proxyConfigPtr nil, and if other error return it; then pass proxyConfigPtr into ensureIstio via extraIstioConfig{ proxyConfig: proxyConfigPtr } so extraIstioConfig.proxyConfig != nil stays meaningful.
🤖 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/operator/controller/gatewayclass/controller_test.go`:
- Around line 54-65: Add table-driven test cases in controller_test.go around
the proxyConfig and istio helpers to cover (1) partial proxy fields (only HTTP,
only HTTPS, only NoProxy, and combinations of two) and (2) transitions where a
proxy entry goes from set → empty string and set → not-found (i.e., removed from
the config map). For each case, use the proxyConfig(...) constructor to create
the initial cluster proxy status and the istio(...) helper to build the Istio
resource, then assert the controller behavior/expected output when reconciling
after the partial configs and after the transition from a populated proxy to an
empty or missing proxy config; ensure you add explicit table rows for each
permutation so omission/removal behavior is exercised and locked in.
In `@pkg/operator/controller/gatewayclass/controller.go`:
- Around line 243-278: The code currently always passes a non-nil pointer for
proxyConfig even when the "cluster" Proxy is missing; change the Get logic to
preserve explicit nil semantics by using a proxy pointer variable (e.g.,
proxyConfigPtr *configv1.Proxy) initialized to nil, call r.cache.Get(ctx,
types.NamespacedName{Name: "cluster"}, &proxyConfig) and if err == nil set
proxyConfigPtr = &proxyConfig, if errors.IsNotFound(err) leave proxyConfigPtr
nil, and if other error return it; then pass proxyConfigPtr into ensureIstio via
extraIstioConfig{ proxyConfig: proxyConfigPtr } so extraIstioConfig.proxyConfig
!= nil stays meaningful.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f8717dff-4fc4-40c3-b0d4-cdf9b1dac1a9
📒 Files selected for processing (3)
pkg/operator/controller/gatewayclass/controller.gopkg/operator/controller/gatewayclass/controller_test.gopkg/operator/controller/gatewayclass/istio.go
|
/retest-required |
1 similar comment
|
/retest-required |
|
/test e2e-gcp-operator |
|
/assign @bentito |
|
/lgtm |
|
/assign @Thealisyed |
|
added everything and tested here: |
|
and applying a wasmfilter on my Gateway I can see it fails to download because of proxy: and removing the proxy configuration it works: |
|
Everything makes sense logically in the code, and seems like a straightforward change. /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gcs278 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
|
/lgtm |
|
/cc @lihongan @ShudiLi @melvinjoseph86 @rhamini3 the test of hypershift-conformance is failing but not due to this PR, so we may override it. |
openshift-ci
bot
requested review from
ShudiLi,
lihongan,
melvinjoseph86 and
rhamini3
|
Confirming, I have tested it on both OLM and non-OLM environment:
Installed gateway class
|
|
/retest-required |
1 similar comment
|
/retest-required |
|
/retest-required some tests timeout (15m) in hypershift-conformance |
|
/verified by @lihongan launch proxy cluster and the related ENV are applied |
openshift-ci-robot
added
the
verified
|
@lihongan: This PR has been marked as verified 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. |
|
/override ci/prow/e2e-aws-ovn-hypershift-conformance test is permafailing: https://redhat.atlassian.net/browse/OCPBUGS-78977 |
|
@rikatz: Overrode contexts on behalf of rikatz: ci/prow/e2e-aws-ovn-hypershift-conformance 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. |
|
@rikatz: 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. |
|
@rikatz: Jira Issue Verification Checks: Jira Issue OCPBUGS-77457 Jira Issue OCPBUGS-77457 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
|
Fix included in accepted release 4.22.0-0.nightly-2026-03-23-022245 |
6 participants
This PR adds support for global proxy configuration on Gateway API provisioning.
It mutates the Istio resource creation to pass the
*_PROXYenvironment variables when deploying Istio controller, and this is passed down to Gateway instances created by users.This configuration allows envoy proxies that rely on downloading WASMPlugins, on an air-gapped environment that enforces the usage of a network proxy, to download the artifacts using the network proxy.
Test executed
kubectl edit proxy clusterkubectl get proxy cluster -o yamlkubectl get pods -n openshift-ingress testcustom-openshift-default-7686989d6f-nxwmw -o jsonpath='{range .spec.containers[*].env[*]}{.name}{"="}{.value}{"\n"}{end}' |grep PROXY