-
Notifications
You must be signed in to change notification settings - Fork 139
Make endpoint picker connection flags configurable #4105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4105 +/- ##
==========================================
- Coverage 86.53% 86.52% -0.01%
==========================================
Files 131 131
Lines 17796 17833 +37
Branches 74 74
==========================================
+ Hits 15399 15430 +31
- Misses 2197 2201 +4
- Partials 200 202 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Currently, configurable flags can only be set via NGF command-line flags. Each NGF command (for example, controller and endpoint-picker) maintains its own flag values independently. This means that even if the same flag is defined for both commands, changing it in one does not automatically propagate to the other. For example:
If these flags are set differently between the two commands, the values will not stay in sync — each process will only respect its own flag values. However, the value of the controller’s flag determines the container args passed to the endpoint picker. I also tried supporting these flags through values.yaml and updating the NGF Helm deployment to render them as container args. However, this only propagates the values to the controller command — not to the endpoint picker, which reads its own flag values from its command invocation. So effectively, to configure these flags correctly, we need to provide the same flag values to both the controller and endpoint-picker commands. For now, by default we have TLS enabled and skip secure verification on. @ciarams87 is this feasible for users to do? Irregardless they are configurable now, just separately. Do you have any better ideas, i am curious! Chatgpt recommended just using both commands to set the value
|
i'll add more test coverage on monday |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks like the correct approach! Main binary flags -> provisioner -> epp shim flags.
The user cannot configure the EPP shim flags directly, so exposing it in this way makes sense as this is the same way we allow the user to enable the inference extension shim in the first place.
I left a few comments around deduplicating the commands, inverting the enableTLS -> disableTLS, and false flag evaluation.
We should also add this to the Helm chart e.g.:
gwAPIInferenceExtension:
# -, - Enable Gateway API Inference Extension support. Allows for configuring InferencePools to route traffic to AI workloads.
enable: false
# -- Endpoint picker TLS configuration.
endpointPicker:
# -- Disable TLS for endpoint picker communication. By default, TLS is enabled.
# Set to true only for development/testing or when using a service mesh for encryption.
disableTLS: false
# -- Skip TLS certificate verification for endpoint picker.
# REQUIRED: Must be true until Gateway API Inference Extension EPP supports mounting certificates.
# See: https://github.com/kubernetes-sigs/gateway-api-inference-extension/issues/1556
skipSecureVerify: true
Then update charts/nginx-gateway-fabric/templates/deployment.yaml:
{{- if .Values.nginxGateway.gwAPIInferenceExtension.enable }}
- --gateway-api-inference-extension
{{- if .Values.nginxGateway.gwAPIInferenceExtension.endpointPicker.disableTLS }}
- --endpoint-picker-disable-tls
{{- end }}
{{- if not .Values.nginxGateway.gwAPIInferenceExtension.endpointPicker.skipSecureVerify }}
- --endpoint-picker-skip-secure-verify=false
{{- else }}
- --endpoint-picker-skip-secure-verify=true
{{- end }}
{{- end }}
I did add this prior, and did some testing but these flags don't really change the mode in EPP but only get added/updated as flags in the container. If it doesn't like actually change the mode in EPP don't you think it will be a little misleading? I tested this exact scenario Flags enabled in NGF container, set based on value. Update to the flags here only reflected changes in the controller command, and not EPP command. So I don't think we should be setting it here, until obviously we provide the mechanism to do what it says it will do. I am okay with keeping the minimal flag support. Once we finalise this conversation, i can inverse the flags and update the documentation you wanted.
|
Yes exactly, but this is correct: How It Should Work (and does work):
Step 2: Helm template renders controller deployment with these as controller flags:
Step 3: Controller reads its flags and uses them to build the sidecar container command in objects.go:
Step 4: The sidecar container starts with those flags and reads them. You don't need to (and can't) put Helm values directly into the sidecar command because:
Once the flags are configured through Helm, the controller will automatically propagate them to the sidecar (via the code in objects.go) The user can't directly set the epp shim commands - the only option for setting them is exposing them through the controller flags (which is what you have already implemented). Creating the Helm options simply exposes configuring these controller flags through Helm. The values flow: Helm → Controller flags → Controller reads → Provisioner builds sidecar command → Sidecar reads. Does this make sense? |
d84bb72
to
4840163
Compare
@ciarams87 I do understand this workflow, but these settings can only be applied at startup, i guess I was thinking of runtime reconciliation of the flag values. In my testing using deployment updates, they did not get reconciled when updated during run time so I thought maybe that would not be the expectation. I have updated the flags to reflect false values at startup and if settings are updated , flags will be reflected at container level for EPP and NGF deployment now. Let me know if all looks well to you. Flag settings now
|
4840163
to
a883fe6
Compare
@salonichf5 If the flags are updated using a helm upgrade, they should be propagated all the way down as the provisioner will detect the pod spec has changed and will re-deploy the dataplane deployments, so while run time reconciliation isn't the expectation, the flags should still work as intended |
28422ec
to
b3f5327
Compare
Sorry for the back and forth, i guess i wanted to have both settings show up the since they are correlated but I have added all the changes as you asked @ciarams87 |
b3f5327
to
9490f58
Compare
f9e698a
to
1d3cbde
Compare
Proposed changes
Write a clear and concise description that helps reviewers understand the purpose and impact of your changes. Use the
following format:
Problem: User should be able to configure Endpoint picker connection flags
Solution: Adds command line support for endpoint picker flags
Testing: Describe any testing that you did.
Please focus on (optional): If you any specific areas where you would like reviewers to focus their attention or provide
specific feedback, add them here.
Closes #4090
Checklist
Before creating a PR, run through this checklist and mark each as complete.
Release notes
If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.