feat(infra): Add remote infrastructure provider#9027
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d82daacea8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| c := &InfraClientImpl{ | ||
| k8sClient: k8sClient, | ||
| remoteService: extensionCfg, | ||
| } |
There was a problem hiding this comment.
When a remote infrastructure service enables TLS/mTLS and its certificateRef or clientCertificateRef omits namespace, this client leaves InfraClientImpl.namespace empty, so GenerateGRPCOptions calls ValidateSecretObjectReference(..., "") and looks for the Secret in an empty namespace. The existing extension manager passes cfg.ControllerNamespace for the same helper; without doing that here, otherwise valid remote-infra TLS configs fail during EG startup unless every Secret ref explicitly sets a namespace.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This code is just refactored from the existing gRPC client code, I don't think we want to change this behavior.
df4ff88 to
ae677f4
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae677f47fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| span.AddEvent("translate", trace.WithAttributes(attribute.Int("resources.count", len(*val)))) | ||
| needModifyListenerPort := r.EnvoyGateway.Provider == nil || r.EnvoyGateway.Provider.Type == egv1a1.ProviderTypeKubernetes |
There was a problem hiding this comment.
This gate makes ModifyListenerPort false for every custom provider, so remote infrastructure never runs servicePortToContainerPort rewriting. As a result, remote deployments ignore the documented default behavior of shifting privileged listener ports (and any per-EnvoyProxy override), which can leave listeners on ports like 80/443 that non-privileged runtimes cannot bind.
Useful? React with 👍 / 👎.
| if e.Spec.Provider.Type == EnvoyProxyProviderTypeRemote && e.Spec.Provider.Remote == nil { | ||
| if e.Spec.Provider.Remote == nil { | ||
| return false | ||
| } | ||
| if e.Spec.Provider.Kubernetes.UseListenerPortAsContainerPort == nil { |
There was a problem hiding this comment.
The remote branch is guarded by Remote == nil and then immediately returns false on the same condition, so the configured remote provider block is never consulted. This means spec.provider.remote.useListenerPortAsContainerPort is effectively ignored and behavior falls back to the Kubernetes field/path, breaking the new remote provider contract.
Useful? React with 👍 / 👎.
| c := &InfraClientImpl{ | ||
| k8sClient: k8sClient, | ||
| remoteService: extensionCfg, | ||
| } |
There was a problem hiding this comment.
The remote infra client passes i.namespace into GenerateGRPCOptions for TLS secret resolution, but namespace is never initialized in the constructor. When certificateRef.namespace is omitted (common same-namespace usage), secret lookup runs against an empty namespace and fails, preventing TLS-configured remote infrastructure connections from starting.
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9027 +/- ##
==========================================
+ Coverage 75.41% 75.44% +0.03%
==========================================
Files 252 256 +4
Lines 41660 42073 +413
==========================================
+ Hits 31417 31744 +327
- Misses 8113 8180 +67
- Partials 2130 2149 +19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // Kubernetes defines the configuration of the Kubernetes provider. This provider retrieves Envoy configuration | ||
| // from a Kubernetes API. | ||
| // +optional | ||
| Kubernetes *EnvoyGatewayKubernetesCustomProvider `json:"kubernetes,omitempty"` |
There was a problem hiding this comment.
I like the split between EnvoyGatewayKubernetesConfiguration and EnvoyGatewayKubernetesInfrastructureConfiguration. It makes sense that custom.infrastructure.remote does not consume Kubernetes infra settings, since the remote infrastructure provider owns its own deployment/HPA/PDB/service details.
The confusing part is discoverability: from the API shape, it’s hard to infer that custom.resource.kubernetes is intended to pair with custom.infrastructure.remote for the “watch Gateway API resources from Kubernetes, but delegate infra provisioning remotely” use case.
Also, the current API shape implies a 2x2 matrix, but we only support two of the four combinations.
How about introducing a remote infrastructure type to provider.kubernetes?
provider:
type: Kubernetes
kubernetes:
watch: ...
client: ...
infrastructure:
type: Remote
remote:
service:
fqdn:
hostname: infra-manager.example.com
port: 50051
We can define a validation rule to prevent the settings in EnvoyGatewayKubernetesInfrastructureConfiguration being configured for provider.kubernetes.infrastructure.type = remote.
cc @envoyproxy/gateway-maintainers
There was a problem hiding this comment.
The goal of this remote infrastructure provider was to have it plug in with other provider types as well, for my specific use-case I am using Kubernetes + Remote but I think it makes sense to leave the API open enough in the future to support arbitrary combinations. Happy to make the change you suggest if we don't value the flexibility!
There was a problem hiding this comment.
Thanks, that makes sense.
My main concern is about the API contract users will infer from the current shape. Since custom.resource and custom.infrastructure look like independent axes, users may reasonably expect all combinations to be supported.
Can we make the currently supported combinations explicit through validation and documentation/examples?
For example, we can document and validate resource.type=Kubernetes + infrastructure.type=Remote as supported now, and reject combinations that we don't support.
That would preserve the extensibility of the API while making the supported behavior clearer to users.
There was a problem hiding this comment.
I can definitely do that. I wanted to gain alignment on this API before putting the effort into creating documentation. I'll start working on the documentation and validations that you outlined.
ae677f4 to
b566cc4
Compare
|
Could we also add CI-backed coverage for the remote infrastructure provider path? Since this PR introduces a new infrastructure provider, I think we should have at least one end-to-end or process-level test that exercises the intended This would give us confidence similar to what we want from the standalone/host-mode test suite, while making sure the newly introduced provider is covered in CI. |
1ab7da9 to
2850d7a
Compare
|
@zhaohuabing I apologize for the delay. I have added what you requested:
I see the PR has grown quite large, what is the project's policy about that? Should I break these commits into smaller PRs? |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2850d7aa29
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if rateLimit.URL == nil { | ||
| return fmt.Errorf("empty ratelimit url settings") | ||
| } | ||
| _, err := url.Parse(*rateLimit.URL) |
There was a problem hiding this comment.
When a Custom provider sets rateLimit.url to a bare host:port value or any URL without an explicit port, this validation succeeds because url.Parse accepts those strings. The xDS translator later calls getRateLimitServiceGrpcHostPort, which parses the same value and panics on ParseUint(u.Port()) when the port is empty, so enabling global rate limit with a bad remote/custom URL can take down translation instead of failing config validation. Please require the form the translator expects, e.g. grpc://<host>:<port>.
Useful? React with 👍 / 👎.
| if isInfraRemote { | ||
| // We won't expect any Envoy replicas, hence we can assume (and document) that the remote provider should | ||
| // guarantee the Envoy addresses are made available if the Envoy data plane is ready to serve traffic. | ||
| gw.Status.Conditions = MergeConditions(gw.Status.Conditions, | ||
| newCondition(string(gwapiv1.GatewayConditionProgrammed), metav1.ConditionTrue, string(gwapiv1.GatewayConditionProgrammed), | ||
| fmt.Sprint(messageFmtProgrammedRemotely), gw.Generation)) |
There was a problem hiding this comment.
For Remote infrastructure providers that do not also create a Kubernetes Service, this remote-specific branch is never reached: with svc == nil and spec.addresses set the function returns earlier with AddressNotUsable, and with no Service-derived addresses updateGatewayProgrammedCondition returns AddressNotAssigned before this block. That makes the new Remote provider unusable for the documented non-Kubernetes substrate case unless it still creates a Kubernetes Service; a Gateway whose remote provider has assigned the requested address remains reported as not programmed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is intentional and documented. The Service is how we propagate metadata back to the main Envoy Gateway process. Building out a new status propagation mechanism would be a lot of work, and probably out of scope of this PR.
2850d7a to
141e2e6
Compare
We encourage contributors to raise an API PR first to reach agreement before implementation, to avoid spending time in the wrong direction. Since you’ve already included the API changes in this PR, I think it’s fine to move forward with everything in one PR. |
6872c51 to
43ca55b
Compare
| // Must be a grpc:// URL with an explicit host and port, e.g. grpc://example.com:50051. | ||
| // The default is the Kubernetes service deployed by the Kubernetes Infrastructure manager. | ||
| // +optional | ||
| URL *string `json:"url"` |
There was a problem hiding this comment.
or can this be moved to a remote specific setting
There was a problem hiding this comment.
I can remove this for now, I wanted to give remote implementations a way to inject their own rate limiter URL as it may not live within the cluster. My own implementation (currently) doesn't support a rate limiter so I didn't even have a great way to test it.
There was a problem hiding this comment.
yeah prefer removing this from the initial PR, to speed up merging this
| rpc DeleteRateLimitInfra(DeleteRateLimitInfraRequest) returns (DeleteRateLimitInfraResponse) {}; | ||
| } | ||
|
|
||
| message CreateOrUpdateProxyInfraRequest { |
There was a problem hiding this comment.
can we make these fields strongly typed
There was a problem hiding this comment.
From the initial conversation we had, #8620 (comment), the idea was to send bytes over and let implementations de-serialize the bytes into IR. Happy to refactor the code to be strongly typed.
There was a problem hiding this comment.
yeah prefer strongly typed fields here, starting with a minimal subset of must haves from the current IR go struct
There was a problem hiding this comment.
Switched to strongly typed IR sent via the gRPC inferface: 5456dce. I only included the fields that my remote infra server uses to materialize the dataplane, but it should be trivial to add more data later on for other use-cases.
Validated by..
1/ unit tests
2/ integration tests
3/ the remote infra e2e test
--- PASS: TestRemoteInfra (33.34s)
--- PASS: TestRemoteInfra/RemoteInfraTCPRoute (12.43s)
--- PASS: TestRemoteInfra/RemoteInfraTCPRoute/Validate_Gateway_status (1.01s)
--- PASS: TestRemoteInfra/RemoteInfraTCPRoute/Remotely_managed_data_plane_serves_traffic (10.38s)
--- PASS: TestRemoteInfra/RemoteInfraTCPRoute/Validate_remotely_managed_data_plane_gets_cleaned_up (1.01s)
PASS
ok github.com/envoyproxy/gateway/test/e2e/remote_infra 33.406s
4/ Our own in house e2e test suite for our remote infra server (looking forward to open-sourcing it!)
55f8c8c to
133feac
Compare
Signed-off-by: Zachary Nixon <nixozach@amazon.com>
…r remote provider Signed-off-by: Zachary Nixon <nixozach@amazon.com>
Signed-off-by: Zachary Nixon <nixozach@amazon.com>
…on generation Signed-off-by: Zachary Nixon <nixozach@amazon.com>
…mmed Signed-off-by: Zachary Nixon <nixozach@amazon.com>
…on to be shared between xds mutator and remote infrastructure Signed-off-by: Zachary Nixon <nixozach@amazon.com>
Signed-off-by: Zachary Nixon <nixozach@amazon.com>
…iting when using remote provider Signed-off-by: Zachary Nixon <nixozach@amazon.com>
Signed-off-by: Zachary Nixon <nixozach@amazon.com>
Signed-off-by: Zachary Nixon <nixozach@amazon.com>
Signed-off-by: Zachary Nixon <nixozach@amazon.com>
Signed-off-by: Zachary Nixon <nixozach@amazon.com>
Signed-off-by: Zachary Nixon <nixozach@amazon.com>
Signed-off-by: Zachary Nixon <nixozach@amazon.com>
Signed-off-by: Zachary Nixon <nixozach@amazon.com>
Signed-off-by: Zachary Nixon <nixozach@amazon.com>
Signed-off-by: Zachary Nixon <nixozach@amazon.com>
Signed-off-by: Zachary Nixon <nixozach@amazon.com>
Signed-off-by: Zachary Nixon <nixozach@amazon.com>
133feac to
5b61000
Compare
| // JSON. It is carried as opaque bytes because the EnvoyProxy CRD schema is | ||
| // large and evolves independently of this contract; providers that need it | ||
| // can unmarshal the JSON into their own representation. | ||
| bytes config = 4; |
There was a problem hiding this comment.
hey is this field needed in the first iteration ? if not, can we rm it
There was a problem hiding this comment.
We're using this config field to populate
1/ Telemetry
2/ Custom bootstrap configuration arguments
3/ Log level
4/ IP Address Type
if you'd like I can add those directly to the IR payload, I chose this approach because the EnvoyProxy object is already available in the public API folder
There was a problem hiding this comment.
yeah lets make this subset typed, thanks
| // UseListenerPortAsContainerPort disables the port shifting feature in the Envoy Proxy. | ||
| // When set to false, if the service port is a privileged port (1-1023), add a constant to the value converting it into an ephemeral port. | ||
| // This allows the container to bind to the port without needing a CAP_NET_BIND_SERVICE capability. | ||
| // The default value is True, which means no port shifting occurs. |
There was a problem hiding this comment.
does this work as mentioned, codex says its unset by default
There was a problem hiding this comment.
It is used here: https://github.com/zac-nixon/gateway/blob/5b61000a8143f2373734e2347291dc2fb16637ba/api/v1alpha1/envoyproxy_helpers.go#L61
hmm.. perhaps the logic is wrong. I can remove it? I will admit the port shifting was a bit weird to me, what do you think, should this be customizable?
There was a problem hiding this comment.
the field was added to undo the default shift logic ( to deal with being unable to bind to 80 and 443 by default), the remote provide is okay with 10080 and 10443 by default, then this field isnt needed
There was a problem hiding this comment.
I remember now, as we expose the direct Envoy listener, it would mean clients would have to connect via port 10080 and 10443 rather than ports 80 and 443.
I added the logic incase other remote strategies wanted the shifting, but I disabled by default here; https://github.com/zac-nixon/gateway/blob/5b61000a8143f2373734e2347291dc2fb16637ba/internal/gatewayapi/listener.go#L1286-L1290
4 participants
What type of PR is this?
/feature
What this PR does / why we need it:
This change allows for users to customize how their Envoy infrastructure is deployed. The concept is similar to how xDS is customized, where Envoy Gateway allows for users to specify a remote process [either via UDS or TCP] that will take charge of provisioning infrastructure. This unlocks the ability to define your Envoy configuration using the Gateway API but to run your Envoy fleet wherever you want.
Some big changes:
In order to get the Gateway address back to the main Envoy Gateway process, I had my extension server create a server with my external address, but with the Envoy Gateway labels.
This allows Envoy Gateway to correctly set my Gateway status:
Scenarios tested:
Things not done, but discussed at contributor meeting:
Which issue(s) this PR fixes:
Fixes #8620
Release Notes: Yes