Use the gateway as the loadbalancer for all types of services#817
Open
RyaliNvidia wants to merge 15 commits intomainfrom
Open
Use the gateway as the loadbalancer for all types of services#817RyaliNvidia wants to merge 15 commits intomainfrom
RyaliNvidia wants to merge 15 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR removes ingress-nginx and ingress manifests, shifts exposure to an Envoy-based osmo-gateway, updates Helm values and runtime configs to reference Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant K8s as Kubernetes
participant Envoy as osmo-gateway (Envoy)
participant App as Application Services
User->>K8s: port-forward svc/osmo-gateway or access LoadBalancer
K8s->>Envoy: request delivered to osmo-gateway service
Envoy->>App: route request to API/UI/backend services
App-->>Envoy: response
Envoy-->>K8s: response returned via Service
K8s-->>User: response delivered
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
RyaliNvidia
force-pushed
the
ryali/loadbalancer
branch
from
7bd1ff3 to
4296359
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #817 +/- ##
==========================================
- Coverage 42.82% 42.65% -0.18%
==========================================
Files 203 203
Lines 27123 27123
Branches 7759 7759
==========================================
- Hits 11616 11568 -48
- Misses 15398 15443 +45
- Partials 109 112 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
deployments/charts/quick-start/templates/NOTES.txt (1)
24-25: Use release namespace in the port-forward command.Hardcoding
-n osmomakes NOTES inaccurate when installed into another namespace. Prefer-n {{ .Release.Namespace }}.Proposed tweak
- kubectl port-forward svc/osmo-gateway 80:80 -n osmo + kubectl port-forward svc/osmo-gateway 80:80 -n {{ .Release.Namespace }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/charts/quick-start/templates/NOTES.txt` around lines 24 - 25, Update the NOTES text that hardcodes the namespace in the port-forward command: replace the literal "-n osmo" instance in the line containing "kubectl port-forward svc/osmo-gateway 80:80 -n osmo" with the Helm release namespace template "-n {{ .Release.Namespace }}" so the NOTES.txt uses the actual release namespace at install time.deployments/charts/quick-start/values.yaml (2)
621-634: Consider using FQDN for gateway hostname in init containers.The init container uses the short hostname
osmo-gateway(line 630). While this should work when pods are in the same namespace, using the FQDNosmo-gateway.osmo.svc.cluster.localwould be more robust and consistent with other references in this file (e.g., line 590).Proposed change for consistency
until nc -z osmo-gateway 80; do + until nc -z osmo-gateway.osmo.svc.cluster.local 80; do echo "Waiting for osmo-gateway..."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/charts/quick-start/values.yaml` around lines 621 - 634, The init container "wait-for-gateway" under initContainers uses the short hostname "osmo-gateway" in its command; update that hostname to the cluster FQDN "osmo-gateway.osmo.svc.cluster.local" so the nc check is robust across namespaces and consistent with other references — modify the command sequence in the wait-for-gateway init container to use the FQDN instead of the short name.
672-685: Same consideration as backend listener: use FQDN for consistency.See previous comment about using FQDN in init containers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/charts/quick-start/values.yaml` around lines 672 - 685, The init container wait-for-gateway uses a short service name ("osmo-gateway") in the nc check which should be replaced with the cluster FQDN for consistency with the backend listener; update the command in initContainers -> name: wait-for-gateway (the lines invoking nc -z osmo-gateway 80) to use the service FQDN (e.g., osmo-gateway.<namespace>.svc.cluster.local) or the Helm-templated full service name (.Release.Namespace or your chart's fullname helper) so DNS resolution is reliable across namespaces and matches the other init containers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deployments/charts/service/values.yaml`:
- Line 1234: This change flips the defaults for oauth2Proxy.enabled and
authz.enabled to false which silently disables authentication/authorization for
existing deployments; either revert these default values back to true in
values.yaml or, if you intend the breaking change, update the PR/CHANGELOG and
upgrade docs to add a clear migration note instructing users to set
gateway.oauth2Proxy.enabled: true and gateway.authz.enabled: true, and mention
the conditional rendering points in templates/gateway.yaml (blocks around lines
referencing the gateway oauth2Proxy and authz conditionals) so operators know
where to re-enable these features during upgrade.
In `@deployments/charts/web-ui/templates/NOTES.txt`:
- Line 5: The NOTES.txt contains a hardcoded kubectl target "svc/osmo-gateway"
which can be wrong when the gateway/service name is overridden; update the
kubectl port-forward line to use the chart's templated UI service name (the same
template/metadata used on Line 2) instead of the literal "svc/osmo-gateway" so
the NOTES always matches deployed resources—replace the hardcoded
"svc/osmo-gateway" reference in NOTES.txt with the chart template helper or
value used to render the UI Service name (the same helper/template that
generates the service metadata).
In `@deployments/charts/web-ui/values.yaml`:
- Line 85: The default apiHostname value "osmo-gateway:80" in values.yaml forces
NEXT_PUBLIC_OSMO_API_HOSTNAME and makes getApiHostname() always use an internal
Kubernetes DNS name; change the chart default to an empty string or remove the
apiHostname default so NEXT_PUBLIC_OSMO_API_HOSTNAME is unset by default
(allowing getApiHostname() to fall back to window.location.host), and document
in values.yaml that operators should set apiHostname only for
internal/browser-resolvable hostnames; refer to the apiHostname key and the use
of NEXT_PUBLIC_OSMO_API_HOSTNAME / getApiHostname() when making this change.
---
Nitpick comments:
In `@deployments/charts/quick-start/templates/NOTES.txt`:
- Around line 24-25: Update the NOTES text that hardcodes the namespace in the
port-forward command: replace the literal "-n osmo" instance in the line
containing "kubectl port-forward svc/osmo-gateway 80:80 -n osmo" with the Helm
release namespace template "-n {{ .Release.Namespace }}" so the NOTES.txt uses
the actual release namespace at install time.
In `@deployments/charts/quick-start/values.yaml`:
- Around line 621-634: The init container "wait-for-gateway" under
initContainers uses the short hostname "osmo-gateway" in its command; update
that hostname to the cluster FQDN "osmo-gateway.osmo.svc.cluster.local" so the
nc check is robust across namespaces and consistent with other references —
modify the command sequence in the wait-for-gateway init container to use the
FQDN instead of the short name.
- Around line 672-685: The init container wait-for-gateway uses a short service
name ("osmo-gateway") in the nc check which should be replaced with the cluster
FQDN for consistency with the backend listener; update the command in
initContainers -> name: wait-for-gateway (the lines invoking nc -z osmo-gateway
80) to use the service FQDN (e.g., osmo-gateway.<namespace>.svc.cluster.local)
or the Helm-templated full service name (.Release.Namespace or your chart's
fullname helper) so DNS resolution is reliable across namespaces and matches the
other init containers.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 192bc8a6-7d79-43a0-8952-fcc38be3d11f
📒 Files selected for processing (20)
deployments/charts/quick-start/Chart.yamldeployments/charts/quick-start/templates/NOTES.txtdeployments/charts/quick-start/values.yamldeployments/charts/router/templates/NOTES.txtdeployments/charts/router/templates/router-service.yamldeployments/charts/router/values.yamldeployments/charts/service/templates/_gateway-envoy-config.tpldeployments/charts/service/templates/agent-service.yamldeployments/charts/service/templates/api-service.yamldeployments/charts/service/templates/gateway.yamldeployments/charts/service/templates/logger-service.yamldeployments/charts/service/values.yamldeployments/charts/web-ui/templates/NOTES.txtdeployments/charts/web-ui/templates/ui.yamldeployments/charts/web-ui/values.yamlrun/minimal/backend_operator_values.yamlrun/minimal/osmo_values.yamlrun/minimal/router_values.yamlrun/minimal/ui_values.yamlrun/start_service_kind.py
💤 Files with no reviewable changes (8)
- deployments/charts/quick-start/Chart.yaml
- run/start_service_kind.py
- deployments/charts/router/values.yaml
- deployments/charts/router/templates/router-service.yaml
- deployments/charts/service/templates/logger-service.yaml
- deployments/charts/service/templates/agent-service.yaml
- deployments/charts/service/templates/api-service.yaml
- deployments/charts/web-ui/templates/ui.yaml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deployments/charts/service/templates/gateway.yaml (1)
21-23:⚠️ Potential issue | 🟡 MinorDefault values have inconsistent configuration:
oauth2Proxy.enabled: truebutenvoy.hostnameis empty.The validation logic at lines 21-23 is correct—it enforces that
gateway.envoy.hostnamemust be provided whengateway.oauth2Proxy.enabledis true (required for OAuth2 redirect URL). The helm-lint pipeline failure occurs because the defaultvalues.yamlenables oauth2Proxy but leaves the hostname empty.Fix by either:
- Setting a default
gateway.envoy.hostnamevalue invalues.yaml, or- Setting
gateway.oauth2Proxy.enabled: falseby default, or- Creating a custom values file for the helm-lint CI test that provides a hostname when testing with oauth2Proxy enabled
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/charts/service/templates/gateway.yaml` around lines 21 - 23, The default Helm values enable oauth2Proxy but leave gateway.envoy.hostname empty, which triggers the validation in the template (check using gateway.oauth2Proxy.enabled and gateway.envoy.hostname). Fix by updating the defaults so they are consistent: either set a sensible default for gateway.envoy.hostname in values.yaml, or change gateway.oauth2Proxy.enabled to false in values.yaml, or add a CI/test values file that sets gateway.envoy.hostname when running helm-lint; update the values.yaml or CI test configuration accordingly and ensure the Helm template condition (gateway.oauth2Proxy.enabled / gateway.envoy.hostname) no longer fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@deployments/charts/service/templates/gateway.yaml`:
- Around line 21-23: The default Helm values enable oauth2Proxy but leave
gateway.envoy.hostname empty, which triggers the validation in the template
(check using gateway.oauth2Proxy.enabled and gateway.envoy.hostname). Fix by
updating the defaults so they are consistent: either set a sensible default for
gateway.envoy.hostname in values.yaml, or change gateway.oauth2Proxy.enabled to
false in values.yaml, or add a CI/test values file that sets
gateway.envoy.hostname when running helm-lint; update the values.yaml or CI test
configuration accordingly and ensure the Helm template condition
(gateway.oauth2Proxy.enabled / gateway.envoy.hostname) no longer fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fe163d35-f5ad-474a-b947-c6fbbf5494fd
📒 Files selected for processing (4)
deployments/charts/service/templates/gateway.yamldeployments/charts/service/values.yamldeployments/charts/web-ui/templates/NOTES.txtdeployments/charts/web-ui/values.yaml
✅ Files skipped from review due to trivial changes (1)
- deployments/charts/web-ui/templates/NOTES.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- deployments/charts/web-ui/values.yaml
- deployments/charts/service/values.yaml
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/deployment_guide/getting_started/deploy_service.rst (1)
552-558: 🛠️ Refactor suggestion | 🟠 MajorDocument TLS/HTTPS configuration requirements.
The post-deployment instructions reference accessing the UI at
https://osmo.example.com(Line 558), and the service verification shows port 443/TCP. However, the deployment guide'sosmo_values.yamlexample does not include any TLS/SSL certificate configuration for the gateway.Users will need guidance on:
- How to configure TLS certificates for the gateway
- Whether the gateway supports automatic HTTPS redirect
- How to configure
gateway.envoy.service.httpsPortand related TLS settingsConsider adding a subsection or note explaining TLS configuration, or updating the example to show HTTP access (port 80) if TLS is optional for initial deployment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment_guide/getting_started/deploy_service.rst` around lines 552 - 558, Add a TLS/HTTPS subsection to the deployment guide clarifying how to configure certificates for the gateway: document where to add TLS certs/keys in the example osmo_values.yaml (or reference secret creation), show how to set gateway.envoy.service.httpsPort and related TLS settings (e.g., cert secret name, key paths, tls mode), state whether the osmo-gateway-external supports automatic HTTP→HTTPS redirect and how to enable it, and alternatively show the example using HTTP (port 80) if TLS is optional for initial verification; update the post-deployment note that verifies UI access to reference these configuration options so readers know how to reach https://osmo.example.com securely or via HTTP for testing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/deployment_guide/appendix/deploy_local.rst`:
- Around line 285-297: The docs currently instruct users to run "kubectl
port-forward svc/osmo-gateway 8080:80" and visit "http://quick-start.osmo:8080",
which conflicts with the quick-start chart NOTES.txt that recommends "kubectl
port-forward svc/osmo-gateway 80:80" and using "osmo login
http://quick-start.osmo"; update the text to either (A) align with the chart by
changing the port-forward command to "kubectl port-forward svc/osmo-gateway
80:80", update the /etc/hosts example and final URL to "http://quick-start.osmo"
(no :8080), and mention the recommended "osmo login http://quick-start.osmo", or
(B) if you intentionally prefer local port 8080, add a brief justification
sentence explaining why 8080 is used and keep the current command and URL;
ensure the commands referenced in the doc (kubectl port-forward svc/osmo-gateway
8080:80 / 80:80 and the URL examples) are updated consistently.
In `@docs/deployment_guide/getting_started/deploy_service.rst`:
- Around line 532-539: The documented service list shows osmo-gateway-external
as 443/TCP but the example osmo_values.yaml does not set
gateway.envoy.service.httpsPort, so update the docs to match the actual default:
either add gateway.envoy.service.httpsPort: 443 to the osmo_values.yaml example
(so the external service is 443/TCP) or change the osmo-gateway-external line in
the service list to 80/TCP to reflect the default when
gateway.envoy.service.httpsPort is unset; reference the gateway template
behavior (gateway.envoy.service.httpsPort) and the osmo-gateway-external entry
when applying the change.
---
Outside diff comments:
In `@docs/deployment_guide/getting_started/deploy_service.rst`:
- Around line 552-558: Add a TLS/HTTPS subsection to the deployment guide
clarifying how to configure certificates for the gateway: document where to add
TLS certs/keys in the example osmo_values.yaml (or reference secret creation),
show how to set gateway.envoy.service.httpsPort and related TLS settings (e.g.,
cert secret name, key paths, tls mode), state whether the osmo-gateway-external
supports automatic HTTP→HTTPS redirect and how to enable it, and alternatively
show the example using HTTP (port 80) if TLS is optional for initial
verification; update the post-deployment note that verifies UI access to
reference these configuration options so readers know how to reach
https://osmo.example.com securely or via HTTP for testing.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 658d2804-12bb-4e43-958a-e5599c4bd7eb
📒 Files selected for processing (5)
docs/deployment_guide/appendix/deploy_local.rstdocs/deployment_guide/appendix/deploy_minimal.rstdocs/deployment_guide/getting_started/deploy_service.rstdocs/deployment_guide/getting_started/infrastructure_setup.rstdocs/deployment_guide/requirements/networking.rst
✅ Files skipped from review due to trivial changes (3)
- docs/deployment_guide/getting_started/infrastructure_setup.rst
- docs/deployment_guide/requirements/networking.rst
- docs/deployment_guide/appendix/deploy_minimal.rst
|
📖 Docs preview: https://d3in15bfzp49i0.cloudfront.net/817/index.html |
elookpotts-nvidia
previously approved these changes
Apr 8, 2026
RyaliNvidia
had a problem deploying
to
internal-ci
GitHub Actions
Failure
RyaliNvidia
had a problem deploying
to
internal-ci
GitHub Actions
Failure
RyaliNvidia
had a problem deploying
to
internal-ci
GitHub Actions
Failure
RyaliNvidia
had a problem deploying
to
internal-ci
GitHub Actions
Failure
RyaliNvidia
had a problem deploying
to
internal-ci
GitHub Actions
Failure
RyaliNvidia
had a problem deploying
to
internal-ci
GitHub Actions
Failure
RyaliNvidia
had a problem deploying
to
internal-ci
GitHub Actions
Failure
RyaliNvidia
had a problem deploying
to
internal-ci
GitHub Actions
Failure
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #None
Checklist
Summary by CodeRabbit
New Features
Chores
Documentation