Skip to content
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

Support NodePort and LoadBalancer Service Types for GatewayStatus Addresses field #604

Closed
kate-osborn opened this issue May 1, 2023 · 6 comments · Fixed by #1141
Closed
Assignees
Labels
enhancement New feature or request refined Requirements are refined and the issue is ready to be implemented. size/medium Estimated to be completed within a week
Milestone

Comments

@kate-osborn
Copy link
Contributor

kate-osborn commented May 1, 2023

Set the GatewayStatus addresses field based on the type of Service that exposes NKG.

The initial issue for supporting GatewayStatus addresses is limited in scope and will set the GatewayStatus address field to the ClusterIP of the Service, regardless of the Service type.

This issue covers the work to support addresses for LoadBalancer and NodePort Services.

Acceptance

  • When the user gets the GatewayStatus, the IP address of the service that fronts the Gateway is present in the addresses field when the fronting service is either a NodePort or LoadBalancer.
@mpstefan mpstefan added the enhancement New feature or request label Jun 9, 2023
@mpstefan mpstefan added the backlog Currently unprioritized work. May change with user feedback or as the product progresses. label Jul 13, 2023
@ciarams87
Copy link
Member

Note: this is a blocker for external-dns integration (we need the address from the fronting LoadBalancer Service).

@mpstefan mpstefan removed the backlog Currently unprioritized work. May change with user feedback or as the product progresses. label Sep 21, 2023
@mpstefan mpstefan added this to the v1.0.0 milestone Sep 21, 2023
@mpstefan mpstefan added refined Requirements are refined and the issue is ready to be implemented. size/medium Estimated to be completed within a week labels Sep 21, 2023
@sjberman
Copy link
Contributor

sjberman commented Oct 10, 2023

Another note: we set the GatewayStatus to the Pod IP, not the Service's ClusterIP. Should we also support ClusterIP as part of this story? (or maybe not since we'll always use the NodePort or LoadBalancer IP?)

@sjberman sjberman self-assigned this Oct 10, 2023
@brianehlert
Copy link

Another note: we set the GatewayStatus to the Pod IP, not the Service's ClusterIP. Should we also support ClusterIP as part of this story? (or maybe not since we'll always use the NodePort or LoadBalancer IP?)

Service ClusterIP is not appropriate. Since the Gateway is always fronting the service. You got there.

@sjberman sjberman moved this from 🆕 New to 🏗 In Progress in NGINX Gateway Fabric Oct 10, 2023
@sjberman
Copy link
Contributor

Do we want to support setting the address to Hostname if available? (https://github.com/kubernetes-sigs/gateway-api/blob/main/apis/v1beta1/shared_types.go#L719)

Or do we only support IP addresses right now?

@kate-osborn
Copy link
Contributor Author

Do we want to support setting the address to Hostname if available? (https://github.com/kubernetes-sigs/gateway-api/blob/main/apis/v1beta1/shared_types.go#L719)

Or do we only support IP addresses right now?

I think we also want to support Hostname since some LoadBalancers (like AWS NLB) will be exposed with a dns name instead of an IP address.

@sjberman
Copy link
Contributor

Decided to punt on NodePort support for now: #1142

@github-project-automation github-project-automation bot moved this from 👀 In Review to ✅ Done in NGINX Gateway Fabric Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request refined Requirements are refined and the issue is ready to be implemented. size/medium Estimated to be completed within a week
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants