feat: make multiple controllers handle different ApisixRoute CRDs #593

Donghui0 · 2021-07-15T04:03:01Z

Please answer these questions before submitting a pull request

Why submit this pull request?
Bugfix
New feature provided
Improve performance
Backport patches
Related issues
feat: Can it support independent use of ApisixUpstream CRD in controller ? #578
proposal: Add IngressClass support for custom resources #592

New feature or improvement

make multiple controllers handle different ApisixRoute CRDs

Backport patches

Why need to backport?
Source branch
Related commits and pull requests
Target branch

…che#578

codecov-commenter · 2021-07-15T04:08:22Z

Codecov Report

Merging #593 (1ca7df9) into master (6e22838) will not change coverage.
The diff coverage is n/a.

❗ Current head 1ca7df9 differs from pull request most recent head c8456e2. Consider uploading reports for the commit c8456e2 to get more accurate results

@@           Coverage Diff           @@
##           master     #593   +/-   ##
=======================================
  Coverage   41.69%   41.69%           
=======================================
  Files          90       90           
  Lines        7702     7702           
=======================================
  Hits         3211     3211           
  Misses       4127     4127           
  Partials      364      364

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

cmd/ingress/ingress.go

gxthrj · 2021-07-16T02:03:05Z

Here is an issue related

tokers · 2021-08-01T11:47:36Z

cmd/ingress/ingress.go

@@ -143,8 +143,7 @@ the apisix cluster and others are created`,
 	cmd.PersistentFlags().StringVar(&cfg.Kubernetes.Kubeconfig, "kubeconfig", "", "Kubernetes configuration file (by default in-cluster configuration will be used)")
 	cmd.PersistentFlags().DurationVar(&cfg.Kubernetes.ResyncInterval.Duration, "resync-interval", time.Minute, "the controller resync (with Kubernetes) interval, the minimum resync interval is 30s")
 	cmd.PersistentFlags().StringSliceVar(&cfg.Kubernetes.AppNamespaces, "app-namespace", []string{config.NamespaceAll}, "namespaces that controller will watch for resources")
-	cmd.PersistentFlags().StringVar(&cfg.Kubernetes.IngressClass, "ingress-class", config.IngressClass, "the class of an Ingress object is set using the field IngressClassName in Kubernetes clusters version v1.18.0 or higher or the annotation \"kubernetes.io/ingress.class\" (deprecated)")
-	cmd.PersistentFlags().StringVar(&cfg.Kubernetes.ApisixRouteClass, "apisix-route-class", config.ApisixRouteClass, "the class of an ApisixRoute object is set using the field ApisixRouteClassName in Kubernetes")


This is a big broken change, we need some discussions in the mailing list.

gxthrj · 2021-09-01T06:27:20Z

pkg/kube/apisix/apis/config/v1/types.go

@@ -39,7 +39,8 @@ type ApisixRoute struct {

 // ApisixRouteSpec is the spec definition for ApisixRouteSpec.
 type ApisixRouteSpec struct {
-	Rules []Rule `json:"rules,omitempty" yaml:"rules,omitempty"`
+	IngressClass string `json:"ingressClass,omitempty" yaml:"ingressClass,omitempty"`


use IngressClassName instead of ingressClass
ref to: https://kubernetes.io/zh/docs/concepts/services-networking/ingress/#deprecated-annotation

tokers · 2021-09-02T10:42:14Z

pkg/ingress/apisix_route.go

+	} else if ar.GroupVersion() == kube.ApisixRouteV2beta1 {
+		class = ar.V2beta1().Spec.IngressClassName
+	} else {
+		class = c.controller.cfg.Kubernetes.IngressClass


Actually the else won't be entered, once the logic here was executed, then there are some programming faults occurred, we may just:

Panic, or

log the error and just return false.

tao12345666333 · 2022-03-04T16:31:13Z

Thanks for your contribution,
Please resolve the conflict if you have time,
let's move forward.

…che#578

…troller into feat-routeclass

tao12345666333 · 2023-02-17T06:15:18Z

Yesterday I had an online meeting with @Donghui0 and @Gallardot.
After discussion, we will use this PR instead of #1523, and @Donghui0 will complete this PR.

I've just resolved the code merge conflict in this PR.

For some basic technical solutions can refer to #1523 (comment)

I will also submit a more complete description at #592 later

Thank you all!

tao12345666333 · 2023-03-03T09:11:03Z

@Donghui0 Please see #592 (comment)
I have added more details.

tao12345666333

And please add e2e test cases to cover this change. Thanks

tao12345666333 · 2023-03-09T06:26:36Z

pkg/kube/apisix/apis/config/v2beta3/types.go

+	IngressClassName string              `json:"ingressClassName,omitempty" yaml:"ingressClassName,omitempty"`
+	HTTP             []ApisixRouteHTTP   `json:"http,omitempty" yaml:"http,omitempty"`
+	Stream           []ApisixRouteStream `json:"stream,omitempty" yaml:"stream,omitempty"`


We don't need to change the v2beta3 resource.

tao12345666333 · 2023-03-09T06:26:50Z

pkg/providers/apisix/apisix_route.go

+	case config.ApisixV2beta3:
+		icn = ar.V2beta3().Spec.IngressClassName


samples/deploy/crd/v1/ApisixRoute.yaml

tao12345666333

Please resolve the conflicts, thanks

tao12345666333 · 2023-03-17T06:53:30Z

go.mod

+require (
+	github.com/go-task/slim-sprig v0.0.0-20210107165309-348f09dbbbc0 // indirect
+	github.com/google/pprof v0.0.0-20210407192527-94a9f03dee38 // indirect
+)
+


you don't need to change this one. right?

tao12345666333 · 2023-03-20T01:25:24Z

CI fails, I think @AlinsRan can help. PTAL

Gallardot

LGTM

AlinsRan · 2023-03-22T07:34:52Z

go.mod

@@ -67,6 +67,8 @@ require (
 	github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
 	github.com/modern-go/reflect2 v1.0.2 // indirect
 	github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
+	github.com/onsi/ginkgo/v2 v2.9.0 // indirect
+	github.com/onsi/gomega v1.27.1 // indirect


These two dependencies are redundant and will only be used in e2e. Let me remove them.

feat: make multiple controllers handle different ApisixRoute CRDs apa…

b2c23fd

…che#578

tokers reviewed Jul 15, 2021

View reviewed changes

cmd/ingress/ingress.go Outdated Show resolved Hide resolved

feat: Add IngressClass support for custom resources. apache#592

aba19cc

Donghui0 requested a review from tokers July 30, 2021 09:23

tokers reviewed Aug 1, 2021

View reviewed changes

fix variable name

16edb2d

Donghui0 closed this Aug 17, 2021

Donghui0 reopened this Aug 18, 2021

Donghui0 force-pushed the feat-routeclass branch from dfff454 to 16edb2d Compare August 18, 2021 07:54

gxthrj reviewed Sep 1, 2021

View reviewed changes

use IngressClassName instead of ingressClass

7799f25

tokers reviewed Sep 2, 2021

View reviewed changes

fix version logic

731ac47

tokers approved these changes Sep 2, 2021

View reviewed changes

tao12345666333 added this to In progress in roadmap Mar 4, 2022

Donghui0 added 8 commits March 29, 2022 19:43

feat: make multiple controllers handle different ApisixRoute CRDs apa…

eff3156

…che#578

feat: Add IngressClass support for custom resources. apache#592

7515f64

fix variable name

ed4f0e7

use IngressClassName instead of ingressClass

d0d773f

fix version logic

eedc377

fix: merge conflict

7defa15

fix: merge

89edb94

Merge branch 'apache:master' into feat-routeclass

528884a

tao12345666333 mentioned this pull request Jun 19, 2022

request help: How to let multiple APISIX Ingress process resources independently #1089

Closed

Donghui0 added 2 commits June 28, 2022 16:07

Merge branch 'apache:master' into feat-routeclass

1990930

fix: v2 version

c75dcc4

Donghui0 mentioned this pull request Aug 11, 2022

proposal: Add IngressClass support for custom resources #592

Closed

7 tasks

Merge branch 'master' of https://github.com/apache/apisix-ingress-con…

7477a1d

…troller into feat-routeclass

tao12345666333 added the area/controller label Feb 17, 2023

tao12345666333 changed the title ~~feat: make multiple controllers handle different ApisixRoute CRDs #578~~ feat: make multiple controllers handle different ApisixRoute CRDs Feb 20, 2023

tao12345666333 added this to the v1.7.0 milestone Feb 20, 2023

tao12345666333 requested review from tokers and removed request for tokers February 24, 2023 09:17

fix: ApisixRoute support IngressClass

b7b7d34

tao12345666333 reviewed Mar 9, 2023

View reviewed changes

fix: remove v2beta3 crd change

b7bde68

lingsamuel requested changes Mar 10, 2023

View reviewed changes

samples/deploy/crd/v1/ApisixRoute.yaml Outdated Show resolved Hide resolved

fix: remove v2beta3 ingressClass prop

086d872

lingsamuel approved these changes Mar 10, 2023

View reviewed changes

tao12345666333 reviewed Mar 17, 2023

View reviewed changes

Donghui0 added 2 commits March 17, 2023 16:07

fix: merge conflict

f57c047

fix: e2e test

1c418f2

tao12345666333 assigned AlinsRan Mar 20, 2023

Gallardot approved these changes Mar 20, 2023

View reviewed changes

AlinsRan force-pushed the feat-routeclass branch from e6cfb2e to 1c418f2 Compare March 22, 2023 07:25

Merge remote-tracking branch 'upstream/master' into feat-routeclass

19e9220

AlinsRan reviewed Mar 22, 2023

View reviewed changes

update go.mod go.sum

08350e7

AlinsRan force-pushed the feat-routeclass branch from 81381b4 to 08350e7 Compare March 22, 2023 07:35

fix e2e apisixroute config

c8456e2

tao12345666333 merged commit c6dd810 into apache:master Mar 22, 2023
53 checks passed

Donghui0 deleted the feat-routeclass branch April 3, 2023 08:24

tao12345666333 moved this from In progress to Done in roadmap Jun 27, 2023

csotiriou mentioned this pull request Apr 4, 2024

docs: is IngressClassName supported in ApisixRoute custom resource? #2201

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: make multiple controllers handle different ApisixRoute CRDs #593

feat: make multiple controllers handle different ApisixRoute CRDs #593

Donghui0 commented Jul 15, 2021 •

edited

codecov-commenter commented Jul 15, 2021 •

edited

gxthrj commented Jul 16, 2021

tokers Aug 1, 2021

gxthrj Sep 1, 2021

tokers Sep 2, 2021

tao12345666333 commented Mar 4, 2022

tao12345666333 commented Feb 17, 2023

tao12345666333 commented Mar 3, 2023

tao12345666333 left a comment

tao12345666333 Mar 9, 2023

tao12345666333 Mar 9, 2023

tao12345666333 left a comment

tao12345666333 Mar 17, 2023

tao12345666333 commented Mar 20, 2023

Gallardot left a comment

AlinsRan Mar 22, 2023

		case config.ApisixV2beta3:
		icn = ar.V2beta3().Spec.IngressClassName

feat: make multiple controllers handle different ApisixRoute CRDs #593

feat: make multiple controllers handle different ApisixRoute CRDs #593

Conversation

Donghui0 commented Jul 15, 2021 • edited

New feature or improvement

Backport patches

codecov-commenter commented Jul 15, 2021 • edited

Codecov Report

gxthrj commented Jul 16, 2021

tokers Aug 1, 2021

Choose a reason for hiding this comment

gxthrj Sep 1, 2021

Choose a reason for hiding this comment

tokers Sep 2, 2021

Choose a reason for hiding this comment

tao12345666333 commented Mar 4, 2022

tao12345666333 commented Feb 17, 2023

tao12345666333 commented Mar 3, 2023

tao12345666333 left a comment

Choose a reason for hiding this comment

tao12345666333 Mar 9, 2023

Choose a reason for hiding this comment

tao12345666333 Mar 9, 2023

Choose a reason for hiding this comment

tao12345666333 left a comment

Choose a reason for hiding this comment

tao12345666333 Mar 17, 2023

Choose a reason for hiding this comment

tao12345666333 commented Mar 20, 2023

Gallardot left a comment

Choose a reason for hiding this comment

AlinsRan Mar 22, 2023

Choose a reason for hiding this comment

Donghui0 commented Jul 15, 2021 •

edited

codecov-commenter commented Jul 15, 2021 •

edited