VirtualServer respecting kubernetes.io/ingress.class

[defect]

Proposed changes

This is an attempt at implementing #863

I apologize for taking so long to open this PR. I actually wrote this quite a few weeks ago, deployed to our cluster, and then kinda forgot about it. The good news is that we've been having it running in production for a while now. We're still not heavy on using VirtualServers but I've deployed a few, making sure they end up on the correct ingress instances.

I do need some pointers and/or help with the python test suite though. I haven't quite grok'd it enough to get a decent test for this set up. Any help is appreciated.

Checklist

• 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 master
• I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

[lucacome]

Hi @defect thanks for the PR! We'll review it and get back to you.

[lucacome]

Hi @defect sorry for the delay.

The code looks good and I've tested that it's working, but we were thinking about introducing a new field ingressClassName instead of using annotations like:

apiVersion: k8s.nginx.org/v1
kind: VirtualServer
metadata:
  name: cafe
spec:
  ingressClassName: prod
  host: cafe.example.com

Our goal with VirtualServer is to avoid using any annotations, but rather add new fields to the spec of the resources.

Would you be interested in implementing a class field? Otherwise, we will implement this feature soon 🙂

[defect]

Yeah, that makes sense to me. I suspect it's to align with the changes in 1.18? Is there a wider plan on how to attack #924 ?

I'd be happy to work on moving to a class field, i'll try to dig around a bit this weekend.

In the meantime, I'll try to keep my fork up-to-date with master if anyone else needs to use the annotation on VirtualServers.

[lucacome]

We don't have a wider plan at the moment, but we will support it eventually.

I've also noticed that we probably have a bug in our current implementation: when a user changes the class of an existing Ingress resource, the configuration doesn't get deleted. I think it should be deleted and that it should be the behavior for VS/VSR as well.
What do you think? Does it make sense?

[lucacome]

Hi @defect,

I just wanted to check if you had a chance to look into this or if you need any assistance. Let me know 🙂

Cheers.

[lucacome]

Closing this as it was implemented in #994 with the ingressClassName field