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

proposal: Add IngressClass support for custom resources #592

Closed
7 tasks done
tao12345666333 opened this issue Jul 14, 2021 · 16 comments
Closed
7 tasks done

proposal: Add IngressClass support for custom resources #592

tao12345666333 opened this issue Jul 14, 2021 · 16 comments
Labels
enhancement New feature or request triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Milestone

Comments

@tao12345666333
Copy link
Member

tao12345666333 commented Jul 14, 2021

Currently, our custom resources are all namespace scoped.

If you want to use multiple apisix-ingress-controllers in the namespace at the same time, this is not allowed.

If we can add an ingress class to custom resources, we can let apisix-ingress-controller handle them separately.

Although this is a very low-frequency scene. It is currently being discussed #578

I think we may need to reach a consensus and add more complete information.

EDIT:

We will implement this function and will split it into the following tasks

@tokers
Copy link
Contributor

tokers commented Jul 15, 2021

@tao12345666333 That would be a good way to support the soft isolation, but shall we still use the name ingressClass?

@tao12345666333
Copy link
Member Author

IngressClass is a general concept, I think it is more acceptable to take it.

@Donghui0
Copy link
Contributor

First: Ingress Class has a default value ("apisix"). If you continue to use ingressClass, then only ApisixRoute containing "apisix" will be listened by default. And if you want to listen to all ApisixRoute CRD by default, it seems impossible to achieve? Or put IngressClass To an empty string?
Second: Does ApisixRoute CRD need to add the Spec.IngressClass field?

@tokers
Copy link
Contributor

tokers commented Jul 30, 2021

First: Ingress Class has a default value ("apisix"). If you continue to use ingressClass, then only ApisixRoute containing "apisix" will be listened by default. And if you want to listen to all ApisixRoute CRD by default, it seems impossible to achieve? Or put IngressClass To an empty string?
Second: Does ApisixRoute CRD need to add the Spec.IngressClass field?

A tricky way to support listen all ApisixRoute CRs is reserving a wildcard *, when the configured ingressClass is *, we listen all ApisixRoutes.

I think add spec.IngressClass is a good way.

@gxthrj
Copy link
Contributor

gxthrj commented Jul 30, 2021

First: Ingress Class has a default value ("apisix"). If you continue to use ingressClass, then only ApisixRoute containing "apisix" will be listened by default. And if you want to listen to all ApisixRoute CRD by default, it seems impossible to achieve? Or put IngressClass To an empty string?

The IngressClass can use ingressclass.kubernetes.io/is-default-class to set the default IngressClass.

Second: Does ApisixRoute CRD need to add the Spec.IngressClass field?

Yes, we need to add a field spec.IngressClass.

@tao12345666333
Copy link
Member Author

tao12345666333 commented Jul 30, 2021

+1 for add spec.IngressClass field

@Donghui0
Copy link
Contributor

First: Ingress Class has a default value ("apisix"). If you continue to use ingressClass, then only ApisixRoute containing "apisix" will be listened by default. And if you want to listen to all ApisixRoute CRD by default, it seems impossible to achieve? Or put IngressClass To an empty string?
Second: Does ApisixRoute CRD need to add the Spec.IngressClass field?

A tricky way to support listen all ApisixRoute CRs is reserving a wildcard *, when the configured ingressClass is *, we listen all ApisixRoutes.

I think add spec.IngressClass is a good way.

Consider this scenario: At present, some people have used ApisixRoute CRD, there is no ingressClass for isolation, so the default is to listen to all ApisixRoute CRDs. But when they upgraded to the new ingress-controller version, they found that by default Controller only listen to CRDs containing "apisix". Is this situation unreasonable? Or change the default value of ingressClass to "*"? But this will affect Ingress CRD.

Donghui0 added a commit to Donghui0/apisix-ingress-controller that referenced this issue Jul 30, 2021
@tokers
Copy link
Contributor

tokers commented Aug 1, 2021

First: Ingress Class has a default value ("apisix"). If you continue to use ingressClass, then only ApisixRoute containing "apisix" will be listened by default. And if you want to listen to all ApisixRoute CRD by default, it seems impossible to achieve? Or put IngressClass To an empty string?
Second: Does ApisixRoute CRD need to add the Spec.IngressClass field?

A tricky way to support listen all ApisixRoute CRs is reserving a wildcard *, when the configured ingressClass is *, we listen all ApisixRoutes.
I think add spec.IngressClass is a good way.

Consider this scenario: At present, some people have used ApisixRoute CRD, there is no ingressClass for isolation, so the default is to listen to all ApisixRoute CRDs. But when they upgraded to the new ingress-controller version, they found that by default Controller only listen to CRDs containing "apisix". Is this situation unreasonable? Or change the default value of ingressClass to "*"? But this will affect Ingress CRD.

As we already have similar logic about IngressClass resource, we may respect it in other APISIX Ingress Controller CRDs, i.e. the default value of spec.ingrssClass will be decided by the default IngressClass resource.

Donghui0 added a commit to Donghui0/apisix-ingress-controller that referenced this issue Aug 17, 2021
Donghui0 added a commit to Donghui0/apisix-ingress-controller that referenced this issue Aug 17, 2021
@tao12345666333 tao12345666333 added the enhancement New feature or request label Mar 4, 2022
@tao12345666333 tao12345666333 added this to To do in roadmap Mar 4, 2022
Donghui0 added a commit to Donghui0/apisix-ingress-controller that referenced this issue Mar 29, 2022
@github-actions
Copy link

This issue has been marked as stale due to 90 days of inactivity. It will be closed in 30 days if no further activity occurs. If this issue is still relevant, please simply write any comment. Even if closed, you can still revive the issue at any time or discuss it on the dev@apisix.apache.org list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Jun 29, 2022
@tao12345666333 tao12345666333 added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed stale labels Jun 29, 2022
@tangzhenhuang
Copy link

Looking forward to supporting ingressClass, our scenario:
we will have a lot of domain names (several thousand), these domain names belong to different businesses, in order to avoid mutual influence between businesses, we use ingressClass to group domain names, so we are looking forward to CRD ingressClass support

@tao12345666333
Copy link
Member Author

got it. Once we complete this function, it is also very convenient for users to achieve multi-tenant isolation

@Donghui0
Copy link
Contributor

#593 Can it be merged ? Support ingressClass in ApisixRoute resource.

@tao12345666333
Copy link
Member Author

tao12345666333 commented Aug 19, 2022

@Donghui0 thanks for your contributions! Yes, I'd love to merge it!

But there are some conflicts, and I want the feature to just be added to the v2 version of the ApisixRoute

@tao12345666333
Copy link
Member Author

tao12345666333 commented Mar 1, 2023

After discussing with @Donghui0 and @Gallardot , it has been decided that @Donghui0 will be responsible for implementing this feature in ApisixRoute. I will provide a more detailed technical proposal base on #1523 (comment)

Firstly, we will use the IngressClass field for configuration.
All resources will be filtered through the IngressClass field. This is an existing configuration item, so we will need to add this field to custom resources and implement processing logic for this field in custom resources.

Secondly, we need to avoid making breaking changes to users.
The specific scenario is as follows:

  • If users deploy the APISIX Ingress project with default configuration, the apisix-ingress-controller will use a configuration with a value of apisix by default. In this scenario, the IngressClass field has not been configured in the custom resources, so all custom resources will be processed.
  • If users modify the value of ingress-class in the apisix-ingress-controller, they may have created corresponding IngressClass resources.

To handle these custom resources properly, we need to add a mechanism to detect whether the IngressClass field has been set in the custom resource or not.
If it has been set, the controller will use that value to filter the resources.
If not, it will fall back to the default value set in the apisix-ingress-controller configuration.

In this way, we can ensure that both the default configuration and custom configurations with a specified IngressClass value can be processed properly, without causing any destructive changes for the user.

Based on the above description, I believe we can modify the default value to apisix-and-default or apisix-and-all. If a user uses this value, we can assume it is the default installation. Then, apisix-ingress-controller will handle all resources with apisix IngressClass and resources without an IngressClass field.

In addition, we also need to consider the scenario where there are multiple resources with the same IngressClass configuration.

If two controllers have the same ingress-class configuration value, we need to distinguish them based on the controller name. However, I believe this feature can be implemented independently.

@acuteaura
Copy link
Contributor

acuteaura commented Aug 17, 2023

This looks to be mostly done, except for watching (and adding to helm) the actual IngressClass (networking.k8s.io/v1) object for defaulting and possibly claiming by controllername instead of just matching the ingressClassName, no?

Adding the resource might also be desirable for discovery.

@tao12345666333
Copy link
Member Author

https://github.com/apache/apisix-ingress-controller/releases/tag/v1.7.0

V1.7.0 has been published, I will close this one, thanks all!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
roadmap
To do
Development

Successfully merging a pull request may close this issue.

6 participants