Support overlay upgrades + wait for OEC to reach terminal state (#1682)

* reconcile CNI inline while building appgw config

* remove err const

* fix issue in bubbling up error

* reconcile once

* don't update if subnet matches

* subnet change should cause re-creation

* remove unused

* move to func

Author: akshaysngupta

cmd/appgw-ingress/main.go

@@ -6,7 +6,6 @@
 package main
 
 import (
-	"context"
 	"flag"
 	"fmt"
 	"os"
@@ -140,16 +139,6 @@ func main() {
 		AppGwName:      env.AppGwName,
 	}
 
-	// create a new agic controller
-	appGwIngressController := controller.NewAppGwIngressController(azClient, appGwIdentifier, k8sContext, recorder, metricStore, agicPod, env.HostedOnUnderlay)
-
-	// initialize the http server and start it
-	httpServer := httpserver.NewHTTPServer(
-		appGwIngressController,
-		metricStore,
-		env.HTTPServicePort)
-	httpServer.Start()
-
 	klog.V(3).Infof("Application Gateway Details: Subscription=\"%s\" Resource Group=\"%s\" Name=\"%s\"", env.SubscriptionID, env.ResourceGroupName, env.AppGwName)
 
 	var authorizer autorest.Authorizer
@@ -215,12 +204,17 @@ func main() {
 		klog.Fatal(errorLine)
 	}
 
-	if err := cni.ReconcileCNI(context.Background(), azClient, ctrlClient, env.AGICPodNamespace, cpConfig, appGw, env.AddonMode); err != nil {
-		if agicPod != nil {
-			recorder.Event(agicPod, v1.EventTypeWarning, events.ReasonFailedCNIConfiguration, err.Error())
-		}
-		klog.Warning(err)
-	}
+	cniReconciler := cni.NewReconciler(azClient, ctrlClient, recorder, cpConfig, appGw, agicPod, env.AGICPodNamespace, env.AddonMode)
+
+	// create a new agic controller
+	appGwIngressController := controller.NewAppGwIngressController(azClient, appGwIdentifier, k8sContext, recorder, metricStore, cniReconciler, agicPod, env.HostedOnUnderlay)
+
+	// initialize the http server and start it
+	httpServer := httpserver.NewHTTPServer(
+		appGwIngressController,
+		metricStore,
+		env.HTTPServicePort)
+	httpServer.Start()
 
 	if err := appGwIngressController.Start(env); err != nil {
 		errorLine := fmt.Sprint("Could not start AGIC: ", err)

cmd/appgw-ingress/utils.go

@@ -39,7 +39,7 @@ func validateNamespaces(namespaces []string, kubeClient *kubernetes.Clientset) e
 			controllererrors.ErrorNoSuchNamespace,
 			"error creating informers; Namespaces do not exist or Ingress Controller has no access to: %v", strings.Join(nonExistent, ","),
 		)
-		klog.Errorf(err.Error())
+		klog.Error(err.Error())
 		return err
 	}
 	return nil

pkg/azure/client.go

@@ -248,9 +248,6 @@ func (az *azClient) ApplyRouteTable(subnetID string, routeTableID string) error
 
 	// if route table is not found, then simply add a log and return no error. routeTable will always be initialized.
 	if routeTable.Response.StatusCode == 404 {
-		klog.V(3).Infof("Error getting route table '%s' (this is relevant for AKS clusters using 'Kubenet' network plugin): %s",
-			routeTableID,
-			err.Error())
 		return nil
 	}
 

pkg/cni/cni.go

@@ -6,6 +6,8 @@ import (
 	"github.com/Azure/application-gateway-kubernetes-ingress/pkg/azure"
 	n "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-03-01/network"
 	"github.com/pkg/errors"
+	v1 "k8s.io/api/core/v1"
+	"k8s.io/client-go/tools/record"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -14,30 +16,45 @@ import (
 type Reconciler struct {
 	armClient azure.AzClient
 	client    client.Client
+	cpConfig  *azure.CloudProviderConfig
+	appGw     n.ApplicationGateway
 	namespace string
 	addonMode bool
+
+	reconciledKubenetCNI bool
+	reconciledOverlayCNI bool
 }
 
-func ReconcileCNI(ctx context.Context, armClient azure.AzClient, client client.Client, namespace string, cpConfig *azure.CloudProviderConfig, appGw n.ApplicationGateway, addonMode bool) error {
-	r := &Reconciler{
-		armClient: armClient,
-		client:    client,
-		namespace: namespace,
-		addonMode: addonMode,
+func NewReconciler(armClient azure.AzClient,
+	client client.Client,
+	recorder record.EventRecorder,
+	cpConfig *azure.CloudProviderConfig,
+	appGw n.ApplicationGateway,
+	agicPod *v1.Pod,
+	namespace string,
+	addonMode bool) *Reconciler {
+	return &Reconciler{
+		armClient:            armClient,
+		client:               client,
+		cpConfig:             cpConfig,
+		appGw:                appGw,
+		namespace:            namespace,
+		addonMode:            addonMode,
+		reconciledKubenetCNI: false,
+		reconciledOverlayCNI: false,
 	}
-
-	return r.Reconcile(ctx, cpConfig, appGw)
 }
 
-func (r *Reconciler) Reconcile(ctx context.Context, cpConfig *azure.CloudProviderConfig, appGw n.ApplicationGateway) error {
-	subnetID := *(*appGw.GatewayIPConfigurations)[0].Subnet.ID
+func (r *Reconciler) Reconcile(ctx context.Context) error {
+	subnetID := *(*r.appGw.GatewayIPConfigurations)[0].Subnet.ID
+
+	if err := r.reconcileKubenetCniIfNeeded(r.cpConfig, subnetID); err != nil {
+		return errors.Wrap(err, "failed to reconcile kubenet CNI")
+	}
 
 	if err := r.reconcileOverlayCniIfNeeded(ctx, subnetID); err != nil {
 		return errors.Wrap(err, "failed to reconcile overlay CNI")
 	}
 
-	if err := r.reconcileKubenetCniIfNeeded(cpConfig, subnetID); err != nil {
-		return errors.Wrap(err, "failed to reconcile kubenet CNI")
-	}
 	return nil
 }

pkg/cni/kubenet.go

@@ -6,6 +6,10 @@ import (
 )
 
 func (r *Reconciler) reconcileKubenetCniIfNeeded(cpConfig *azure.CloudProviderConfig, subnetID string) error {
+	if r.reconciledKubenetCNI {
+		return nil
+	}
+
 	if cpConfig == nil || cpConfig.RouteTableName == "" {
 		return nil
 	}
@@ -17,5 +21,6 @@ func (r *Reconciler) reconcileKubenetCniIfNeeded(cpConfig *azure.CloudProviderCo
 			routeTableID)
 	}
 
+	r.reconciledKubenetCNI = true
 	return nil
 }

pkg/cni/kubenet_test.go

@@ -5,6 +5,10 @@ import (
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	"github.com/pkg/errors"
+	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/tools/record"
 	ctrl_client "sigs.k8s.io/controller-runtime/pkg/client"
 	ctrl_client_fake "sigs.k8s.io/controller-runtime/pkg/client/fake"
 
@@ -16,9 +20,12 @@ import (
 )
 
 var _ = Describe("Kubenet CNI", func() {
-	var ctx = context.TODO()
+	var ctx context.Context
+	var cancel context.CancelFunc
 	var azClient *azure.FakeAzClient
 	var k8sClient ctrl_client.Client
+	var recorder *record.FakeRecorder
+	var agicPod *v1.Pod
 	var appGw = n.ApplicationGateway{
 		ApplicationGatewayPropertiesFormat: &n.ApplicationGatewayPropertiesFormat{
 			GatewayIPConfigurations: &[]n.ApplicationGatewayIPConfiguration{
@@ -32,12 +39,32 @@ var _ = Describe("Kubenet CNI", func() {
 			},
 		},
 	}
+	var reconciler *cni.Reconciler
 
 	BeforeEach(func() {
+		ctx, cancel = context.WithCancel(context.Background())
 		azClient = azure.NewFakeAzClient()
+		recorder = record.NewFakeRecorder(100)
+		recorder.IncludeObject = true
+		agicPod = &v1.Pod{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "agic",
+				Namespace: "kube-system",
+			},
+			TypeMeta: metav1.TypeMeta{
+				Kind:       "Pod",
+				APIVersion: "v1",
+			},
+		}
 
 		scheme, _ := k8s.NewScheme()
 		k8sClient = ctrl_client_fake.NewClientBuilder().WithScheme(scheme).Build()
+
+		reconciler = cni.NewReconciler(azClient, k8sClient, recorder, &azure.CloudProviderConfig{
+			SubscriptionID:          "test-sub",
+			RouteTableResourceGroup: "test-rg",
+			RouteTableName:          "test-rt",
+		}, appGw, agicPod, "test", false)
 	})
 
 	Context("reconcileKubenetCniIfNeeded", func() {
@@ -48,12 +75,8 @@ var _ = Describe("Kubenet CNI", func() {
 				return nil
 			}
 
-			err := cni.ReconcileCNI(ctx, azClient, k8sClient, "test", &azure.CloudProviderConfig{
-				SubscriptionID:          "test-sub",
-				RouteTableResourceGroup: "test-rg",
-				RouteTableName:          "test-rt",
-			}, appGw, false)
-			Expect(err).To(BeNil())
+			reconciler.Reconcile(ctx)
+			Eventually(recorder.Events).ShouldNot(Receive())
 		})
 
 		It("should return nil if RouteTableName is empty", func() {
@@ -62,10 +85,26 @@ var _ = Describe("Kubenet CNI", func() {
 				return nil
 			}
 
-			err := cni.ReconcileCNI(ctx, azClient, k8sClient, "test", &azure.CloudProviderConfig{
-				RouteTableName: "",
-			}, appGw, false)
-			Expect(err).To(BeNil())
+			reconciler = cni.NewReconciler(azClient, k8sClient, recorder, &azure.CloudProviderConfig{
+				SubscriptionID: "test-sub",
+			}, appGw, agicPod, "test", false)
+
+			err := reconciler.Reconcile(ctx)
+			Expect(err).ToNot(HaveOccurred())
 		})
+
+		It("should create event if ApplyRouteTable fails", func() {
+			azClient.ApplyRouteTableFunc = func(subnetID string, routeTableID string) error {
+				return errors.New("failed to apply route table")
+			}
+
+			err := reconciler.Reconcile(ctx)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("failed to apply route table"))
+		})
+	})
+
+	AfterEach(func() {
+		cancel()
 	})
 })