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 multiple gateways #599

Closed
wants to merge 5 commits into from
Closed

Conversation

pleshakov
Copy link
Contributor

@pleshakov pleshakov commented Apr 27, 2023

This is a prototype to support multiple Gateways via single a NKG deployment primarily done to unblock running conformance tests. See #305

Note: This is a prototype where number one priority was to get things done and prove the idea. So little thinking was devoted to the design, etc.

This PR:

  • Supports multiple Gateways. For each Gateway, NKG deploys a ClusterIP Service and reports the IP of that Service in the status of the Gateway.
  • Adds a way to run conformance tests. The tests are from v0.6.2 release of Gateway API.

What is needed to merge to main:

  • Proper design and implementation of the feature.
  • Currently this new feature replaces the existing behavior of supporting only one Gateway. That behavior should be preserved, remain default and the only recommended way. And the new feature should be enabled via a feature flag. Also, because the new feature creates ClusterIP Services (rather than LoadBalancer or NodePort), it is note really intended to be used in production. We can in the future make it useful for production -- by supporting customization of created Services -- but this can be done much later.
  • Improve the files in tests folder, add documentation.

After merging to main:

  • Add running conformance tests in the pipeline.

How to run it locally:

(1) Build NKG image:

make PREFIX=nginx-kubernetes-gateway TAG=edge container

(2) Build tester image

cd tests
make build

(3) Create cluster

make create-kind-cluster

(4) Load NKG image to Kind cluster:

make load-image-to-kind

(5) Install NKG:

make install-nkg

(6) Double check NGK is running:

kubectl get pods -n nginx-gateway
NAME                            READY   STATUS    RESTARTS   AGE
nginx-gateway-f87859d99-6842w   2/2     Running   0          26s

(7) Run tests:

. . .
=== RUN   TestConformance/GatewayObservedGenerationBump
    suite.go:258: Applying tests/gateway-observed-generation-bump.yaml
    apply.go:211: Creating gateway-observed-generation-bump Gateway
=== RUN   TestConformance/GatewayObservedGenerationBump/observedGeneration_should_increment
    helpers.go:198: Gateway gateway-conformance-infra/gateway-observed-generation-bump expected observedGeneration to be updated to 1 for all conditions, only 0/1 were updated. stale conditions are: Accepted (generation 0)
    helpers.go:198: Gateway gateway-conformance-infra/gateway-observed-generation-bump expected observedGeneration to be updated to 1 for all conditions, only 0/1 were updated. stale conditions are: Accepted (generation 0)
    helpers.go:222: Gateways and Pods in gateway-conformance-infra namespaces ready
    helpers.go:198: Gateway gateway-conformance-infra/gateway-observed-generation-bump expected observedGeneration to be updated to 2 for all conditions, only 0/2 were updated. stale conditions are: Accepted (generation 1), Programmed (generation 1)
    helpers.go:198: Gateway gateway-conformance-infra/gateway-observed-generation-bump expected observedGeneration to be updated to 2 for all conditions, only 0/2 were updated. stale conditions are: Accepted (generation 1), Programmed (generation 1)
    helpers.go:222: Gateways and Pods in gateway-conformance-infra namespaces ready
=== NAME  TestConformance/GatewayObservedGenerationBump
    apply.go:219: Deleting gateway-observed-generation-bump Gateway
. . .
PASS
ok      github.com/nginxinc/nginx-kubernetes-gateway/tests      13.085s

Note: the only test we can pass now is GatewayObservedGenerationBump (maybe a few more - my search was not too thorough), so in conformance_test.go it is the only one enabled , the rest is skipped.

Below are the Gateways that were created during the test run. Note that the NKG reported their IP addresses, which are ClusterIPs of the corresponding provisioned (by NKG) Services:

NAMESPACE                   NAME                               CLASS   ADDRESS         PROGRAMMED   AGE
gateway-conformance-infra   all-namespaces                     nginx   10.96.185.231   True         11s
gateway-conformance-infra   backend-namespaces                 nginx   10.96.84.224    True         11s
gateway-conformance-infra   gateway-observed-generation-bump   nginx   10.96.54.54     True         3s
gateway-conformance-infra   same-namespace                     nginx   10.96.134.183   True         11s

@github-actions github-actions bot added dependencies Pull requests that update a dependency file enhancement New feature or request labels Apr 27, 2023
ssl_certificate {{ $s.SSL.Certificate }};
ssl_certificate_key {{ $s.SSL.CertificateKey }};

if ($ssl_server_name != $host) {
return 421;
}
{{ else }}
listen {{ $s.Port }};
Copy link
Contributor

Choose a reason for hiding this comment

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

Why add the else block here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

before, for non-SSL servers, NKG didn't generate any listen directive. For that case, NGINX uses the default listen *:80.

in this PR, we start using different ports (different than 80 and 443), so omitting the listen directive non-SSL no longer works, so that's why I added an explicit listen directive here.

@kate-osborn
Copy link
Contributor

@pleshakov creative approach to handling multiple gateways. I'm not sure it's worth the effort and additional complexity to add this for testing purposes only. However, if there's a strong use case for supporting multiple gateways outside of testing, I think this solution is worth exploring.

@brianehlert
Copy link

@pleshakov creative approach to handling multiple gateways. I'm not sure it's worth the effort and additional complexity to add this for testing purposes only. However, if there's a strong use case for supporting multiple gateways outside of testing, I think this solution is worth exploring.

I definitely would not do something for testing and exercise it on a regular basis and not claim it as being supported. Unless it is a total hack or something is manipulated outside of the system processes itself (such as some test script injects some tweak outside of using the API as an example)
If it works for testing, follows all the normal system workflow and processes, then it should be considered a supported use case / scenario.

@kate-osborn
Copy link
Contributor

@brianehlert good to know.

I think the question, then, is whether we want to support multiple gateways. Is there a use case for it when we only have one data plane?

@brianehlert
Copy link

brianehlert commented May 2, 2023

@brianehlert good to know.

I think the question, then, is whether we want to support multiple gateways. Is there a use case for it when we only have one data plane?

I think it all depends on perspective. But the immediate answer is 'we don't yet know'
It could be argued that a single data plane could be tenant-ized through multiple Gateway objects.
I think that if we can introduce the capability in a very light weight and unobtrusive way, then we can find out as the folks using the project will find the edges for us and show us that it matters. Otherwise, we can leave the capability as it is and not mature it.
Thoughts?

@pleshakov
Copy link
Contributor Author

closing this draft PR. support for multiple Gateways will be implemented in #634

@pleshakov pleshakov closed this May 17, 2023
@pleshakov pleshakov deleted the feature/multiple-gateways branch August 4, 2023 13:36
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 enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants