ori-edge / k8s_gateway

A CoreDNS plugin to resolve all types of external Kubernetes resources
Apache License 2.0
316 stars 65 forks source link

Support for Nginx VirtualServer CRs #62

Closed Kaezon closed 2 years ago

Kaezon commented 2 years ago

I've been looking into nginxinc's VirtualServer CRs as a replacement for my usual Ingress objects. One of the things I need in order to keep things running smoothly is k8s_gateway support for these objects, or perhaps CRs in general.

I'm looking at adding the feature myself, but I wanted to start a conversation to see if this would be in scope for the project and talk about implementation details.

networkop commented 2 years ago

Hey @Kaezon , this seems like a nice contribution to me. Would be cool to add it in.

Kaezon commented 2 years ago

Took me a little while to get my development environment working. The test yamls needed a little touch-up, and Tilt is new to me, but I think I've got it now.

Kaezon commented 2 years ago

I thought this was going to be easy for all of 2 minutes XD I did not realize how little documentation was available for the kubernetes-client package, and working with CRs using k8s.io/client-go/kubernetes.Interface isn't really straightforward.

I'm playing with the idea of importing nginx-ingress configuration/v1 directly, but I'm not sure if that would be acceptable or not. Working on understanding how the nginx-ingress project manages their own resources so I can query them properly here.

Kaezon commented 2 years ago

I've managed to get a test client querying VirtualServer Hosts by importing the nginx ingress ClientSet. I'm going to try integrating it with k8s_gateway's Gateway implementation next.

networkop commented 2 years ago

great!, I'm planning to finish the GatewayAPI/HTTPRoute implementation by end of this week.

Kaezon commented 2 years ago

Having an issue with my watcher implementation for some reason. It doesn't panic and die, but it's definitely not able to watch VirtualServers. Have you run into an issue like this before @networkop ? E0111 18:57:31.541474 182 reflector.go:138] pkg/mod/k8s.io/client-go@v0.22.0/tools/cache/reflector.go:167: Failed to watch *v1.VirtualServer: failed to list *v1.VirtualServer: v1.ListOptions is not suitable for converting to "meta.k8s.io/v1" in scheme "pkg/runtime/scheme.go:100"

Edit: Seems to be a notRegisteredError from k8s apimachinery package

Edit 2: I found this older issue filed against k8s apiextensions-apiserver which makes it seem like maybe the nginx types aren't registered correctly somewhere, maybe with the ClientSet?

Edit 3: Ah, yeah. I found this functionality in the ingress' repo. I guess the way I am instantiating the ingress' ClientSet isn't building out the scheme correctly.

Nginxinc's Watch function My watcher implementation My controller implementation

Kaezon commented 2 years ago

Fixed that issue and now it looks like DNS is able to register and return entries for VirtualServer CR's correctly.

Kaezon commented 2 years ago

Tests are updated. Will look to see what docs might need to be updated tomorrow.

networkop commented 2 years ago

@Kaezon can you check the https://github.com/ori-edge/k8s_gateway/pull/65 ? I want to make sure it makes sense to you and is aligned with what you do before I merge.

Kaezon commented 2 years ago

Sure thing

Kaezon commented 2 years ago

Next up: resolving merge conflicts now that #65 is merged :P

networkop commented 2 years ago

heh, yeah , I couldn't help but rename some of the funcs along the way.

Kaezon commented 2 years ago

@networkop I'm not sure what priority VirtualServers should have in orderedResources I had given it least priority. Do you have any opinion on where they should be?

networkop commented 2 years ago

the order only matters when there's an overlap. my thinking was to order them from least common to most common, i.e. if you're using VirtualServers and you happen to have an ingress with the same hostname, then you most likely would prefer VS over Ingress. So I'd put it anywhere above ingress.

networkop commented 2 years ago

closed in #69