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

Set gateway Pod IP as GatewayStatus address #638

Merged
merged 7 commits into from
May 17, 2023

Conversation

sjberman
Copy link
Contributor

To satisfy conformance and provide the address of the gateway, we'll now set the pod IP address of the gateway on the GatewayStatus resource. Eventually we will also support other addresses.

Closes #370

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@sjberman sjberman requested a review from a team as a code owner May 11, 2023 16:39
@github-actions github-actions bot added documentation Improvements or additions to documentation enhancement New feature or request labels May 11, 2023
internal/status/gateway.go Show resolved Hide resolved
internal/status/gateway.go Outdated Show resolved Hide resolved
@sjberman
Copy link
Contributor Author

sjberman commented May 16, 2023

@pleshakov I've added the env var to the entrypoint and performed validation. Let me know what you think. I originally was going to mimic the existing validation pattern for arguments, but I changed my mind. For one, I wouldn't expect that we would be supported many env vars, so may not be worth it. Also, if we did want to do it, there's likely a way to refactor the existing argument validation to combine args and env vars, but I figured that's probably a bit too much work for right now, and went with the simpler approach. If we feel that it's worth implementing now though, I can do that.

@pleshakov
Copy link
Contributor

@sjberman

@pleshakov I originally was going to mimic the existing validation pattern for arguments, but I changed my mind. For one, I wouldn't expect that we would be supported many env vars, so may not be worth it. Also, if we did want to do it, there's likely a way to refactor the existing argument validation to combine args and env vars, but I figured that's probably a bit too much work for right now, and went with the simpler approach. If we feel that it's worth implementing now though, I can do that.

This makes sense to me. However, it made me think - do we need to have an env variable at all?

what if we add --external-ips cli arg that will accept the list of IPs (a list, because the Gateway status can include many IPs). By default, we can initialize it to the POD_IP. For example:

        args:
        - --gateway-ctlr-name=k8s-gateway.nginx.org/nginx-gateway-controller
        - --gatewayclass=nginx
        - --external-ips=$(POD_IP)

Could that work better?

@sjberman
Copy link
Contributor Author

it made me think - do we need to have an env variable at all

We do need it; in order to use the k8s downward API to get the pod IP, it has to be an environment variable. I'm also not a huge fan of more CLI args if they can be avoided.

cmd/gateway/setup.go Outdated Show resolved Hide resolved
cmd/gateway/setup.go Outdated Show resolved Hide resolved
cmd/gateway/setup.go Outdated Show resolved Hide resolved
cmd/gateway/setup_test.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label May 16, 2023
internal/status/updater_test.go Outdated Show resolved Hide resolved
cmd/gateway/setup_test.go Outdated Show resolved Hide resolved
cmd/gateway/setup.go Outdated Show resolved Hide resolved
To satisfy conformance and provide the address of the gateway, we'll now set the pod IP address of the gateway on the GatewayStatus resource. Eventually we will also support other addresses.
cmd/gateway/main.go Outdated Show resolved Hide resolved
cmd/gateway/main_test.go Outdated Show resolved Hide resolved
@sjberman sjberman merged commit 2ade9f8 into nginxinc:main May 17, 2023
@sjberman sjberman deleted the feature/gateway-address branch May 17, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Conformance: Set the Address field in Gateway Status
3 participants