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 custom listener ports #745

Merged
merged 12 commits into from
Jun 20, 2023

Conversation

kate-osborn
Copy link
Contributor

Proposed changes

Problem: NKG only supported listener ports 80 and 443. This limitation prevents the conformance tests from running because the conformance tests create a base Gateway that listens on port 8080.

Solution: Allow users to specify any port between 1-65535.

Testing:

  • Added/updated unit tests to cover new code.
  • Manually tested all examples using both 80/443 and 8080/8443.
  • Manually verified that routes can attach to multiple listeners with the same hostname as long as they specify different ports.
  • Ran conformance tests and verified that we can get past setup.

Closes #619

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • 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

@kate-osborn kate-osborn requested a review from a team as a code owner June 12, 2023 20:50
@github-actions github-actions bot added documentation Improvements or additions to documentation enhancement New feature or request labels Jun 12, 2023
Copy link
Contributor

@sjberman sjberman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, can you do a manual test where we have multiple listeners without hostnames, on different ports, and then an HTTPRoute without a section name, and verify that we properly create a server for both ports with the HTTPRoute hostname, and traffic flows through either port?

This was a case I wanted to test when I implemented the empty section name work, but couldn't due to the port restriction.

internal/state/dataplane/configuration.go Show resolved Hide resolved
docs/installation.md Outdated Show resolved Hide resolved
internal/manager/manager.go Outdated Show resolved Hide resolved
internal/state/graph/gateway_listener.go Outdated Show resolved Hide resolved
internal/state/dataplane/configuration.go Outdated Show resolved Hide resolved
internal/state/graph/httproute_test.go Outdated Show resolved Hide resolved
@kate-osborn
Copy link
Contributor Author

Looks good, can you do a manual test where we have multiple listeners without hostnames, on different ports, and then an HTTPRoute without a section name, and verify that we properly create a server for both ports with the HTTPRoute hostname, and traffic flows through either port?

This was a case I wanted to test when I implemented the empty section name work, but couldn't due to the port restriction.

Yep. Verified this works with multiple http & https listeners.

internal/state/dataplane/configuration.go Outdated Show resolved Hide resolved
internal/state/graph/gateway_listener.go Outdated Show resolved Hide resolved
@kate-osborn kate-osborn force-pushed the feat/custom-listener-ports branch 2 times, most recently from 3fd01f6 to 1bd0565 Compare June 16, 2023 17:42
@kate-osborn kate-osborn merged commit 2079725 into nginxinc:main Jun 20, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Custom Listener Ports
3 participants