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

feat(gateway-api): Add initial support for tls and grpc routes #238

Merged

Conversation

larivierec
Copy link
Contributor

@larivierec larivierec commented Oct 14, 2023

  • use experimental install crds, for tests
  • add tests for tls and grpc

would close #237

@larivierec
Copy link
Contributor Author

Thanks for enabling ci, I'll fix those linting issues

@larivierec larivierec force-pushed the feature/support-tls-grpc-routes branch 2 times, most recently from 96b73b0 to d22df0a Compare October 16, 2023 20:30
@larivierec
Copy link
Contributor Author

larivierec commented Oct 16, 2023

hi @networkop , i'm going to have to find a better way to test. from what i understand, i'll need an SSL cert in order to do proper integration tests, this might be complicated to do or perhaps i should spoof them with self signed certs?

do you have any ideas?

side note: i fixed the linting and added tls/grpc to the tests controller i was sure i had previously done that but apparently not.

@networkop
Copy link
Collaborator

do you have any ideas?

I think doing tests with self-signed certs should be good enough.

@larivierec
Copy link
Contributor Author

just letting you know, i'm having issues running kind on my arm Mac.
i tried using colima, base lima and kind to spawn a cluster and so far nothing has worked.

i was able to generate the cert locally and create with kubectl

just letting you know i didn't forget, trying to configure my local env on m2 properly.

@larivierec
Copy link
Contributor Author

larivierec commented Oct 31, 2023

hi @networkop

I was finally able to get some very basic integration tests done

Below is the output for k8s-gateway plugin accepting the new tls/grpc routes

(⎈|local:kube-system)➜  ~ k logs excoredns-k8s-gateway-67bb559895-tt52g
[INFO] plugin/k8s_gateway: Building k8s_gateway controller
[INFO] plugin/k8s_gateway: Starting k8s_gateway controller
[INFO] plugin/k8s_gateway: Waiting for controllers to sync
.:1053
[INFO] plugin/reload: Running configuration SHA512 = efa79d1a2c701696de610932242797b9aa250ece9ac2abd7f81f4f6a94bb12cc91e15a43257b1f0683c883af76301567b914ea403db46feebb6b5734f99d5dde
CoreDNS-1.11.1+k8s_gateway-0.3.4
linux/arm64, go1.21.0,
[DEBUG] plugin/k8s_gateway: Request 2426908976122741341.6026106597144163097. has not matched any zones [foo.org.]
[DEBUG] plugin/k8s_gateway: Adding index myservicea.foo.org for ingress ingress-myservicea
[INFO] 127.0.0.1:54209 - 18519 "HINFO IN 2426908976122741341.6026106597144163097. udp 57 false 512" NOERROR qr,rd,ra 57 0.000683459s
[DEBUG] plugin/k8s_gateway: Adding index myservicetls for tlsRoute myservicetls.gw.foo.org
[DEBUG] plugin/k8s_gateway: Adding index myservicegrpc for grpcRoute myservicegrpc.gw.foo.org
[DEBUG] plugin/k8s_gateway: Adding index myservicea for httpRoute myservicea.gw.foo.org
[DEBUG] plugin/k8s_gateway: Adding index myserviced for httpRoute myserviced.gw.foo.org
[DEBUG] plugin/k8s_gateway: Adding index myserviceb for httpRoute myserviceb.gw.foo.org
[DEBUG] plugin/k8s_gateway: Adding index myservicec for httpRoute myservicec.gw.foo.org
[INFO] plugin/k8s_gateway: Invalid FQDN length: abcd0123456789012345678901234567890123456789012345678901234567890
[DEBUG] plugin/k8s_gateway: Adding index annotation-bad.default for service annotation-bad
[INFO] plugin/k8s_gateway: RFC 1123 conformance failed for FQDN: foo_bar
[DEBUG] plugin/k8s_gateway: Adding index annotation-bad-2.default for service annotation-bad-2
[DEBUG] plugin/k8s_gateway: Adding index good.ok for service annotation-good
[DEBUG] plugin/k8s_gateway: Adding index gateway.default for service gateway
[DEBUG] plugin/k8s_gateway: Adding index gateway-one-istio.default for service gateway-one-istio
[DEBUG] plugin/k8s_gateway: Adding index gateway-two-istio.default for service gateway-two-istio
[DEBUG] plugin/k8s_gateway: Adding index ingress-nginx-controller.default for service ingress-nginx-controller
[DEBUG] plugin/k8s_gateway: Adding index nginxinc-nginx-ingress-controller.default for service nginxinc-nginx-ingress-controller
[DEBUG] plugin/k8s_gateway: Adding index test.default for service test
[DEBUG] plugin/k8s_gateway: Adding index cilium-ingress.kube-system for service cilium-ingress
[INFO] plugin/k8s_gateway: Synced all required resources
[DEBUG] plugin/k8s_gateway: Computed Index Keys [myservicetls.gw.foo.org myservicetls.gw]
[DEBUG] plugin/k8s_gateway: Found 0 matching httpRoute objects
[DEBUG] plugin/k8s_gateway: Found 1 matching tlsRoute objects
[DEBUG] plugin/k8s_gateway: Found 1 matching gateway objects
[DEBUG] plugin/k8s_gateway: Computed response addresses [198.51.100.116]
[DEBUG] plugin/k8s_gateway: Computed Index Keys [myservicetls.gw.foo.org myservicetls.gw]
[DEBUG] plugin/k8s_gateway: Found 0 matching httpRoute objects
[DEBUG] plugin/k8s_gateway: Found 1 matching tlsRoute objects
[DEBUG] plugin/k8s_gateway: Found 1 matching gateway objects
[DEBUG] plugin/k8s_gateway: Computed response addresses [198.51.100.116]
[INFO] 192.168.5.15:52253 - 19537 "A IN myservicetls.gw.foo.org. udp 41 false 512" NOERROR qr,aa,rd 80 0.000618458s
[INFO] 192.168.5.15:52253 - 19871 "AAAA IN myservicetls.gw.foo.org. udp 41 false 512" NOERROR qr,aa,rd 177 0.000892792s

You can see that it properly matches the IP of gateway-istio-one

(⎈|local:kube-system)➜  home-cluster git:(main) ✗ k -n default get svc
NAME                                TYPE           CLUSTER-IP      EXTERNAL-IP      PORT(S)                                      AGE
kubernetes                          ClusterIP      10.43.0.1       <none>           443/TCP                                      2d14h
istiod                              ClusterIP      10.43.18.104    <none>           15010/TCP,15012/TCP,443/TCP,15014/TCP        19m
backend                             ClusterIP      10.43.204.227   <none>           80/TCP                                       19m
annotation-bad                      LoadBalancer   10.43.249.147   198.51.100.5     80:31092/TCP                                 19m
annotation-bad-2                    LoadBalancer   10.43.16.121    198.51.100.148   80:32460/TCP                                 19m
annotation-good                     LoadBalancer   10.43.196.200   198.51.100.99    80:30094/TCP                                 19m
gateway                             LoadBalancer   10.43.7.176     198.51.100.111   15021:32018/TCP,80:30444/TCP,443:31129/TCP   19m
ingress-nginx-controller            LoadBalancer   10.43.102.214   198.51.100.53    80:32344/TCP,443:30830/TCP                   19m
nginxinc-nginx-ingress-controller   LoadBalancer   10.43.95.164    198.51.100.82    80:32342/TCP,443:30857/TCP                   19m
test                                LoadBalancer   10.43.199.34    198.51.100.66    80:31382/TCP                                 19m
gateway-one-istio                   LoadBalancer   10.43.59.247    198.51.100.116   15021:32366/TCP,80:32636/TCP                 17m
gateway-two-istio                   LoadBalancer   10.43.36.109    198.51.100.200   15021:30421/TCP,80:32122/TCP                 17m
colima:/Users/christopher$ nslookup myservicetls.gw.foo.org 10.43.150.78
Server:		10.43.150.78
Address:	10.43.150.78:53

Name:	myservicetls.gw.foo.org
Address: 198.51.100.116

I had to heavily modify the tiltfiles and had to seperate the stacks into single-stack (ipv4) and dual-stack(ipv6) because I was unable to test ipv6 with Colima on M2 Pro.

I took the liberty of updating the Tiltfiles with latest chart versions.

  • Removed metallb in favor of cilium with l2
  • Started integration with cert-manager (i did not test to see if the tls routes and grpc routes actually respond only that they are added to k8s_gateway)
  • Updating watched resources
  • Used nerdctl instead of docker_restart
  • i kept the makefile intact and left the docker step inside each tiltfiles.

I'll be committing all these changes

@larivierec larivierec marked this pull request as ready for review October 31, 2023 16:27
@larivierec larivierec force-pushed the feature/support-tls-grpc-routes branch from 4261eb2 to d088b80 Compare October 31, 2023 16:59
@larivierec
Copy link
Contributor Author

larivierec commented Oct 31, 2023

also, 1.0.0 of gateway-apis was just released, should probably consider adding v1 afterwards in a seperate PR.
kubectl apply -f https://github.com/kubernetes-sigs/gateway-api/releases/download/v1.0.0/experimental-install.yaml

i was able to bump at least the CRDs to 1.0.0 without impact.
if you have v1beta1 http routes, they are translated to v1 if you have the 1.0.0 crds
EDIT: i squashed the commits

@larivierec larivierec force-pushed the feature/support-tls-grpc-routes branch 2 times, most recently from 20694b4 to 5f0ee52 Compare November 1, 2023 12:29
@networkop
Copy link
Collaborator

nice, thanks @larivierec , let me know when the PR is ready for review

@larivierec
Copy link
Contributor Author

larivierec commented Nov 1, 2023

nice, thanks @larivierec , let me know when the PR is ready for review

Oh, yeah @networkop you can go ahead.
I changed the status of this PR to ready I don't know if GH notififes you.

@larivierec larivierec force-pushed the feature/support-tls-grpc-routes branch from 5f0ee52 to a538b2c Compare November 2, 2023 14:03
@networkop
Copy link
Collaborator

sorry @larivierec , been a bit busy. will try to review this week

@larivierec
Copy link
Contributor Author

No rush :)

@larivierec larivierec changed the title Add initial support for tls and grpc routes feat(gateway-api): Add initial support for tls and grpc routes Nov 10, 2023
README.md Outdated

### Steps

1. In `Tiltfile.single`, ensure that you have commented out the `docker` portion and uncommented the `nerdctl` portion.
Copy link
Collaborator

@networkop networkop Nov 17, 2023

Choose a reason for hiding this comment

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

So I understand the only purpose Tiltfile.single is for lima VM development? Why do you need to uncomment something then? Is there another purpose for this tiltfile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You have a good point actually.

Since the .single is already aimed towards Lima I could simply remove the docker commands and use nerdctl directly.

I'll do that yeah

Copy link
Collaborator

Choose a reason for hiding this comment

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

perfect. so there's one file for linux and one file for mac

Copy link
Collaborator

Choose a reason for hiding this comment

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

can you just update the readme and maybe rename the file to something reflecting that it's for mac or nerdctl.
otherwise single is a bit confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, i opted for Tiltfile.nerdctl finally, it made more sense because nerdctl any os and the tiltfile isn't restricted to M powered macs

Tiltfile Show resolved Hide resolved
Copy link
Collaborator

@networkop networkop left a comment

Choose a reason for hiding this comment

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

hey @larivierec , thanks for the massive PR and apologies for the delay in reviewing.
I've left a few comments, let me know what you think

@networkop networkop mentioned this pull request Nov 17, 2023
Copy link
Contributor Author

@larivierec larivierec left a comment

Choose a reason for hiding this comment

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

i reviewed your comments, see my replies if you can :) thanks

Tiltfile Show resolved Hide resolved
examples/install-clusterwide.yml Show resolved Hide resolved
gateway.go Show resolved Hide resolved
gateway_test.go Show resolved Hide resolved
kubernetes.go Show resolved Hide resolved
@larivierec
Copy link
Contributor Author

i'll have another review for Gateway APIs v1.0.0 update after this one gets merged.
won't be too complicated to change.

@networkop
Copy link
Collaborator

@larivierec so apart from tiltfile updates this PR is good to be merged.
Let's keep the GwAPI 1.0 updates to its own PR.

@larivierec
Copy link
Contributor Author

Awesome, do you want me to rename the file to something else maybe?

Tiltfile.mac

@networkop
Copy link
Collaborator

yep. Tiltfile.mac sounds good.
and btw, since you've mentioned that Cilium now implements the GwAPI spec, would it make sense to drop Istio (from tiltfile)?

@larivierec
Copy link
Contributor Author

yep. Tiltfile.mac sounds good. and btw, since you've mentioned that Cilium now implements the GwAPI spec, would it make sense to drop Istio (from tiltfile)?

Ok for the tiltfile.
No I don't think so, from my perspective, it adds value as it shows the this plugin works with istio as well :)

I'll go ahead and make the changes and squash everything nicely.

    - use nerdctl and cilium for integration tests
    - update tiltfile dependencies
    - update Golang dependencies
    - seperate into 2 stacks
    - update CRDs for Gateway-API to 1.0.0 GA
    - update helm chart values for updates
    - add unit tests for TLSRoute, GRPCRoute
    - keep same kind registry as default (for makefile)
    - update README.md - 0.8.1+ minimum for CRDs + dev notes
    - fix variable GOATCH to GOARCH in both Tiltiles
    - add nerdctl tiltfile when we are using containerd

Signed-off-by: Christopher Larivière <lariviere.c@gmail.com>
@larivierec larivierec force-pushed the feature/support-tls-grpc-routes branch from 9d82ef9 to 2a53d47 Compare November 18, 2023 16:39
@larivierec
Copy link
Contributor Author

everything should be good, let me know if I need to do something else.
thanks for the review 👍

@networkop networkop merged commit fd587ff into ori-edge:master Nov 18, 2023
2 checks passed
@networkop
Copy link
Collaborator

awesome 🎉
thanks for all the great work @larivierec

@larivierec larivierec deleted the feature/support-tls-grpc-routes branch November 18, 2023 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Support other Gateway API route types
2 participants