You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2021/09/15 01:47:53 UTC
[GitHub] [apisix-ingress-controller] tokers commented on a change in pull request #680: feat: add full compare when ingress startup
tokers commented on a change in pull request #680:
URL: https://github.com/apache/apisix-ingress-controller/pull/680#discussion_r708768142
##########
File path: utils/kind-with-registry.sh
##########
@@ -54,7 +54,7 @@ fi
echo "Registry Host: ${reg_host}"
# create a cluster with the local registry enabled in containerd
-cat <<EOF | kind create cluster --name "${KIND_CLUSTER_NAME}" --config=-
+cat <<EOF | kind create cluster --image kindest/node:v1.21.2 --name "${KIND_CLUSTER_NAME}" --config=-
Review comment:
Version should be set as configurable.
##########
File path: pkg/ingress/compare.go
##########
@@ -0,0 +1,271 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package ingress
+
+import (
+ "context"
+ "sync"
+ "time"
+
+ "C"
+ v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+ apisix "github.com/apache/apisix-ingress-controller/pkg/types/apisix/v1"
+)
+
+// CompareResources use to compare the object IDs in resources and APISIX
+// Find out the rest of objects in APISIX
+// AND remove them.
+func (c *Controller) CompareResources() {
+ var (
+ routeMapK8S = new(sync.Map)
+ streamRouteMapK8S = new(sync.Map)
+ upstreamMapK8S = new(sync.Map)
+ sslMapK8S = new(sync.Map)
+ consumerMapK8S = new(sync.Map)
+
+ routeMapA6 = new(sync.Map)
+ streamRouteMapA6 = new(sync.Map)
+ upstreamMapA6 = new(sync.Map)
+ sslMapA6 = new(sync.Map)
+ consumerMapA6 = new(sync.Map)
+ )
+ // todo if watchingNamespace == nil
+ if c.watchingNamespace == nil {
+ opts := v1.ListOptions{}
+ // list all apisixroute resources in all namespaces
+ nsList, err := c.kubeClient.Client.CoreV1().Namespaces().List(context.TODO(), opts)
+ if err != nil {
+ panic(err)
Review comment:
Panic here is not so friendly. We may just retry the comparison, as it's meaningless for the Ingress controller to skip the comparison and go ahead if the communication with Kubernetes is broken.
##########
File path: pkg/ingress/compare.go
##########
@@ -0,0 +1,271 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package ingress
+
+import (
+ "context"
+ "sync"
+ "time"
+
+ "C"
+ v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+ apisix "github.com/apache/apisix-ingress-controller/pkg/types/apisix/v1"
+)
+
+// CompareResources use to compare the object IDs in resources and APISIX
+// Find out the rest of objects in APISIX
+// AND remove them.
+func (c *Controller) CompareResources() {
+ var (
+ routeMapK8S = new(sync.Map)
+ streamRouteMapK8S = new(sync.Map)
+ upstreamMapK8S = new(sync.Map)
+ sslMapK8S = new(sync.Map)
+ consumerMapK8S = new(sync.Map)
+
+ routeMapA6 = new(sync.Map)
+ streamRouteMapA6 = new(sync.Map)
+ upstreamMapA6 = new(sync.Map)
+ sslMapA6 = new(sync.Map)
+ consumerMapA6 = new(sync.Map)
+ )
+ // todo if watchingNamespace == nil
+ if c.watchingNamespace == nil {
+ opts := v1.ListOptions{}
+ // list all apisixroute resources in all namespaces
+ nsList, err := c.kubeClient.Client.CoreV1().Namespaces().List(context.TODO(), opts)
+ if err != nil {
+ panic(err)
+ } else {
+ wns := make(map[string]struct{}, len(nsList.Items))
+ for _, v := range nsList.Items {
+ wns[v.Name] = struct{}{}
+ }
+ c.watchingNamespace = wns
+ }
+ }
+ for ns := range c.watchingNamespace {
+ // ApisixRoute
+ opts := v1.ListOptions{}
+ retRoutes, err := c.kubeClient.APISIXClient.ApisixV2beta1().ApisixRoutes(ns).List(context.TODO(), opts)
+ if err != nil {
+ panic(err)
+ } else {
+ for _, r := range retRoutes.Items {
+ tc, err := c.translator.TranslateRouteV2beta1NotStrictly(&r)
+ if err != nil {
+ panic(err)
+ } else {
+ // routes
+ for _, route := range tc.Routes {
+ routeMapK8S.Store(route.ID, route.ID)
+ }
+ // streamRoutes
+ for _, stRoute := range tc.StreamRoutes {
+ streamRouteMapK8S.Store(stRoute.ID, stRoute.ID)
+ }
+ // upstreams
+ for _, upstream := range tc.Upstreams {
+ upstreamMapK8S.Store(upstream.ID, upstream.ID)
+ }
+ // ssl
+ for _, ssl := range tc.SSL {
+ sslMapK8S.Store(ssl.ID, ssl.ID)
+ }
+ }
+ }
+ }
+ // todo ApisixUpstream
+ // ApisixUpstream should be synced with ApisixRoute resource
+
+ // ApisixSSL
+ retSSL, err := c.kubeClient.APISIXClient.ApisixV1().ApisixTlses(ns).List(context.TODO(), opts)
+ if err != nil {
+ panic(err)
+ } else {
+ for _, s := range retSSL.Items {
+ ssl, err := c.translator.TranslateSSL(&s)
+ if err != nil {
+ panic(err)
+ } else {
+ sslMapK8S.Store(ssl.ID, ssl.ID)
+ }
+ }
+ }
+ // ApisixConsumer
+ retConsumer, err := c.kubeClient.APISIXClient.ApisixV2alpha1().ApisixConsumers(ns).List(context.TODO(), opts)
+ if err != nil {
+ panic(err)
+ } else {
+ for _, con := range retConsumer.Items {
+ consumer, err := c.translator.TranslateApisixConsumer(&con)
+ if err != nil {
+ panic(err)
+ } else {
+ consumerMapK8S.Store(consumer.Username, consumer.Username)
+ }
+ }
+ }
+ }
+
+ // 2.get all cache routes
+ c.listRouteCache(routeMapA6)
Review comment:
Errors that occurred during the communication with Kubernetes will cause the program to be panicked but were ignored during the communication with Apache APISIX, it's wrong.
##########
File path: pkg/ingress/compare.go
##########
@@ -0,0 +1,271 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package ingress
+
+import (
+ "context"
+ "sync"
+ "time"
+
+ "C"
+ v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+ apisix "github.com/apache/apisix-ingress-controller/pkg/types/apisix/v1"
+)
+
+// CompareResources use to compare the object IDs in resources and APISIX
+// Find out the rest of objects in APISIX
+// AND remove them.
+func (c *Controller) CompareResources() {
+ var (
+ routeMapK8S = new(sync.Map)
+ streamRouteMapK8S = new(sync.Map)
+ upstreamMapK8S = new(sync.Map)
+ sslMapK8S = new(sync.Map)
+ consumerMapK8S = new(sync.Map)
+
+ routeMapA6 = new(sync.Map)
+ streamRouteMapA6 = new(sync.Map)
+ upstreamMapA6 = new(sync.Map)
+ sslMapA6 = new(sync.Map)
+ consumerMapA6 = new(sync.Map)
+ )
+ // todo if watchingNamespace == nil
+ if c.watchingNamespace == nil {
+ opts := v1.ListOptions{}
+ // list all apisixroute resources in all namespaces
+ nsList, err := c.kubeClient.Client.CoreV1().Namespaces().List(context.TODO(), opts)
+ if err != nil {
+ panic(err)
+ } else {
+ wns := make(map[string]struct{}, len(nsList.Items))
+ for _, v := range nsList.Items {
+ wns[v.Name] = struct{}{}
+ }
+ c.watchingNamespace = wns
+ }
+ }
+ for ns := range c.watchingNamespace {
+ // ApisixRoute
+ opts := v1.ListOptions{}
+ retRoutes, err := c.kubeClient.APISIXClient.ApisixV2beta1().ApisixRoutes(ns).List(context.TODO(), opts)
+ if err != nil {
+ panic(err)
+ } else {
+ for _, r := range retRoutes.Items {
+ tc, err := c.translator.TranslateRouteV2beta1NotStrictly(&r)
+ if err != nil {
+ panic(err)
+ } else {
+ // routes
+ for _, route := range tc.Routes {
+ routeMapK8S.Store(route.ID, route.ID)
+ }
+ // streamRoutes
+ for _, stRoute := range tc.StreamRoutes {
+ streamRouteMapK8S.Store(stRoute.ID, stRoute.ID)
+ }
+ // upstreams
+ for _, upstream := range tc.Upstreams {
+ upstreamMapK8S.Store(upstream.ID, upstream.ID)
+ }
+ // ssl
+ for _, ssl := range tc.SSL {
+ sslMapK8S.Store(ssl.ID, ssl.ID)
+ }
+ }
+ }
+ }
+ // todo ApisixUpstream
+ // ApisixUpstream should be synced with ApisixRoute resource
+
+ // ApisixSSL
+ retSSL, err := c.kubeClient.APISIXClient.ApisixV1().ApisixTlses(ns).List(context.TODO(), opts)
+ if err != nil {
+ panic(err)
+ } else {
+ for _, s := range retSSL.Items {
+ ssl, err := c.translator.TranslateSSL(&s)
+ if err != nil {
+ panic(err)
+ } else {
+ sslMapK8S.Store(ssl.ID, ssl.ID)
+ }
+ }
+ }
+ // ApisixConsumer
+ retConsumer, err := c.kubeClient.APISIXClient.ApisixV2alpha1().ApisixConsumers(ns).List(context.TODO(), opts)
+ if err != nil {
+ panic(err)
+ } else {
+ for _, con := range retConsumer.Items {
+ consumer, err := c.translator.TranslateApisixConsumer(&con)
+ if err != nil {
+ panic(err)
+ } else {
+ consumerMapK8S.Store(consumer.Username, consumer.Username)
+ }
+ }
+ }
+ }
+
+ // 2.get all cache routes
+ c.listRouteCache(routeMapA6)
+ c.listStreamRouteCache(streamRouteMapA6)
+ c.listUpstreamCache(upstreamMapA6)
+ c.listSSLCache(sslMapA6)
+ c.listConsumerCache(consumerMapA6)
+ // 3.compare
+ routeReult := findRedundant(routeMapA6, routeMapK8S)
+ streamRouteReult := findRedundant(streamRouteMapA6, streamRouteMapK8S)
+ upstreamReult := findRedundant(upstreamMapA6, upstreamMapK8S)
+ sslReult := findRedundant(sslMapA6, sslMapK8S)
+ consuemrReult := findRedundant(consumerMapA6, consumerMapK8S)
+ // 4.remove from APISIX
+ c.removeRouteFromA6(routeReult)
+ c.removeStreamRouteFromA6(streamRouteReult)
+ c.removeSSLFromA6(sslReult)
+ c.removeConsumerFromA6(consuemrReult)
+ time.Sleep(5 * time.Second)
Review comment:
Don't reply on a FIXED WINDOW. Use poll mechanism.
##########
File path: pkg/ingress/compare.go
##########
@@ -0,0 +1,271 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package ingress
+
+import (
+ "context"
+ "sync"
+ "time"
+
+ "C"
+ v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+ apisix "github.com/apache/apisix-ingress-controller/pkg/types/apisix/v1"
+)
+
+// CompareResources use to compare the object IDs in resources and APISIX
+// Find out the rest of objects in APISIX
+// AND remove them.
+func (c *Controller) CompareResources() {
+ var (
+ routeMapK8S = new(sync.Map)
+ streamRouteMapK8S = new(sync.Map)
+ upstreamMapK8S = new(sync.Map)
+ sslMapK8S = new(sync.Map)
+ consumerMapK8S = new(sync.Map)
+
+ routeMapA6 = new(sync.Map)
+ streamRouteMapA6 = new(sync.Map)
+ upstreamMapA6 = new(sync.Map)
+ sslMapA6 = new(sync.Map)
+ consumerMapA6 = new(sync.Map)
+ )
+ // todo if watchingNamespace == nil
+ if c.watchingNamespace == nil {
+ opts := v1.ListOptions{}
+ // list all apisixroute resources in all namespaces
+ nsList, err := c.kubeClient.Client.CoreV1().Namespaces().List(context.TODO(), opts)
Review comment:
We should not use `context.TODO()`, use the context passed in `c.run`, using `context.TODO()` is not controllable, for instance, you're running ingress controller on local and the process of communication with kubernetes is stuck, you can't quit the program by just click <Ctrl-C>.
##########
File path: pkg/ingress/compare.go
##########
@@ -0,0 +1,271 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package ingress
+
+import (
+ "context"
+ "sync"
+ "time"
+
+ "C"
+ v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+ apisix "github.com/apache/apisix-ingress-controller/pkg/types/apisix/v1"
+)
+
+// CompareResources use to compare the object IDs in resources and APISIX
+// Find out the rest of objects in APISIX
+// AND remove them.
Review comment:
I'm afraid removing users' data that doesn't create by the APISIX Ingress controller is a **quite dangerous** behavior, people will be confused by this behavior and complain about it.
What we can do is warning users the consequence, we don't have permissions to remove users' data, again, TOO DANGEROUS.
##########
File path: pkg/ingress/compare.go
##########
@@ -0,0 +1,271 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package ingress
+
+import (
+ "context"
+ "sync"
+ "time"
+
+ "C"
+ v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+ apisix "github.com/apache/apisix-ingress-controller/pkg/types/apisix/v1"
+)
+
+// CompareResources use to compare the object IDs in resources and APISIX
+// Find out the rest of objects in APISIX
+// AND remove them.
+func (c *Controller) CompareResources() {
+ var (
+ routeMapK8S = new(sync.Map)
+ streamRouteMapK8S = new(sync.Map)
+ upstreamMapK8S = new(sync.Map)
+ sslMapK8S = new(sync.Map)
+ consumerMapK8S = new(sync.Map)
+
+ routeMapA6 = new(sync.Map)
+ streamRouteMapA6 = new(sync.Map)
+ upstreamMapA6 = new(sync.Map)
+ sslMapA6 = new(sync.Map)
+ consumerMapA6 = new(sync.Map)
+ )
+ // todo if watchingNamespace == nil
+ if c.watchingNamespace == nil {
+ opts := v1.ListOptions{}
+ // list all apisixroute resources in all namespaces
+ nsList, err := c.kubeClient.Client.CoreV1().Namespaces().List(context.TODO(), opts)
+ if err != nil {
+ panic(err)
+ } else {
+ wns := make(map[string]struct{}, len(nsList.Items))
+ for _, v := range nsList.Items {
+ wns[v.Name] = struct{}{}
+ }
+ c.watchingNamespace = wns
+ }
+ }
+ for ns := range c.watchingNamespace {
+ // ApisixRoute
+ opts := v1.ListOptions{}
+ retRoutes, err := c.kubeClient.APISIXClient.ApisixV2beta1().ApisixRoutes(ns).List(context.TODO(), opts)
+ if err != nil {
+ panic(err)
+ } else {
+ for _, r := range retRoutes.Items {
+ tc, err := c.translator.TranslateRouteV2beta1NotStrictly(&r)
+ if err != nil {
+ panic(err)
Review comment:
Ditto.
##########
File path: pkg/ingress/compare.go
##########
@@ -0,0 +1,271 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package ingress
+
+import (
+ "context"
+ "sync"
+ "time"
+
+ "C"
+ v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+ apisix "github.com/apache/apisix-ingress-controller/pkg/types/apisix/v1"
+)
+
+// CompareResources use to compare the object IDs in resources and APISIX
+// Find out the rest of objects in APISIX
+// AND remove them.
+func (c *Controller) CompareResources() {
+ var (
+ routeMapK8S = new(sync.Map)
+ streamRouteMapK8S = new(sync.Map)
+ upstreamMapK8S = new(sync.Map)
+ sslMapK8S = new(sync.Map)
+ consumerMapK8S = new(sync.Map)
+
+ routeMapA6 = new(sync.Map)
+ streamRouteMapA6 = new(sync.Map)
+ upstreamMapA6 = new(sync.Map)
+ sslMapA6 = new(sync.Map)
+ consumerMapA6 = new(sync.Map)
+ )
+ // todo if watchingNamespace == nil
+ if c.watchingNamespace == nil {
+ opts := v1.ListOptions{}
+ // list all apisixroute resources in all namespaces
+ nsList, err := c.kubeClient.Client.CoreV1().Namespaces().List(context.TODO(), opts)
+ if err != nil {
+ panic(err)
+ } else {
+ wns := make(map[string]struct{}, len(nsList.Items))
+ for _, v := range nsList.Items {
+ wns[v.Name] = struct{}{}
+ }
+ c.watchingNamespace = wns
+ }
+ }
+ for ns := range c.watchingNamespace {
+ // ApisixRoute
+ opts := v1.ListOptions{}
+ retRoutes, err := c.kubeClient.APISIXClient.ApisixV2beta1().ApisixRoutes(ns).List(context.TODO(), opts)
+ if err != nil {
+ panic(err)
Review comment:
Ditto. We should not panic the program here.
##########
File path: pkg/ingress/compare.go
##########
@@ -0,0 +1,271 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package ingress
+
+import (
+ "context"
+ "sync"
+ "time"
+
+ "C"
+ v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+ apisix "github.com/apache/apisix-ingress-controller/pkg/types/apisix/v1"
+)
+
+// CompareResources use to compare the object IDs in resources and APISIX
+// Find out the rest of objects in APISIX
+// AND remove them.
+func (c *Controller) CompareResources() {
+ var (
+ routeMapK8S = new(sync.Map)
+ streamRouteMapK8S = new(sync.Map)
+ upstreamMapK8S = new(sync.Map)
+ sslMapK8S = new(sync.Map)
+ consumerMapK8S = new(sync.Map)
+
+ routeMapA6 = new(sync.Map)
+ streamRouteMapA6 = new(sync.Map)
+ upstreamMapA6 = new(sync.Map)
+ sslMapA6 = new(sync.Map)
+ consumerMapA6 = new(sync.Map)
Review comment:
I didn't see any scenarios that multiple goroutines use these objects, so why to use `sync.Map` here, it's overengineering.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org