From dba015a1c2503128cc6a02a11cb66f801c9e1fa1 Mon Sep 17 00:00:00 2001 From: Oliver King Date: Thu, 2 Nov 2023 22:13:03 -0500 Subject: [PATCH 1/6] everything working --- pkg/controller/controller.go | 7 +- pkg/controller/dns/external_dns.go | 11 +- pkg/controller/dns/external_dns_test.go | 58 ++- pkg/controller/nginx/nginx.go | 28 -- .../nginxingress/nginx_ingress_controller.go | 431 ++++++++++++++++++ pkg/controller/nginxingress/types.go | 11 + pkg/manifests/common.go | 39 +- pkg/manifests/external_dns.go | 7 +- pkg/manifests/nginx.go | 106 ++--- pkg/manifests/types.go | 58 +++ 10 files changed, 597 insertions(+), 159 deletions(-) create mode 100644 pkg/controller/nginxingress/nginx_ingress_controller.go create mode 100644 pkg/controller/nginxingress/types.go create mode 100644 pkg/manifests/types.go diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index a545800c..dc5f4847 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -10,6 +10,7 @@ import ( "os" approutingv1alpha1 "github.com/Azure/aks-app-routing-operator/api/v1alpha1" + "github.com/Azure/aks-app-routing-operator/pkg/controller/nginxingress" "github.com/Azure/aks-app-routing-operator/pkg/webhook" "github.com/go-logr/logr" cfgv1alpha2 "github.com/openservicemesh/osm/pkg/apis/config/v1alpha2" @@ -165,10 +166,14 @@ func setupWebhooks(mgr ctrl.Manager, addWebhooksFn func(mgr ctrl.Manager) error) func setupControllers(mgr ctrl.Manager, conf *config.Config) error { var selfDeploy *appsv1.Deployment = nil // self deploy doesn't work because operator isn't in same resources as child resources - if err := dns.NewExternalDns(mgr, conf, selfDeploy); err != nil { + if err := dns.NewExternalDns(mgr, conf); err != nil { return fmt.Errorf("setting up external dns controller: %w", err) } + if err := nginxingress.NewReconciler(conf, mgr); err != nil { + return fmt.Errorf("setting up nginx ingress controller reconciler: %w", err) + } + nginxConfigs, err := nginx.New(mgr, conf, selfDeploy) if err != nil { return fmt.Errorf("getting nginx configs: %w", err) diff --git a/pkg/controller/dns/external_dns.go b/pkg/controller/dns/external_dns.go index 1a451039..62de497e 100644 --- a/pkg/controller/dns/external_dns.go +++ b/pkg/controller/dns/external_dns.go @@ -8,7 +8,6 @@ import ( "github.com/Azure/aks-app-routing-operator/pkg/controller/controllername" "github.com/Azure/aks-app-routing-operator/pkg/manifests" "github.com/Azure/aks-app-routing-operator/pkg/util" - appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime/schema" ctrl "sigs.k8s.io/controller-runtime" @@ -50,8 +49,8 @@ func addExternalDnsCleaner(manager ctrl.Manager, objs []cleanObj) error { } // NewExternalDns starts all resources required for external dns -func NewExternalDns(manager ctrl.Manager, conf *config.Config, self *appsv1.Deployment) error { - instances := instances(conf, self) +func NewExternalDns(manager ctrl.Manager, conf *config.Config) error { + instances := instances(conf) deployInstances := filterAction(instances, deploy) deployRes := getResources(deployInstances) @@ -67,16 +66,16 @@ func NewExternalDns(manager ctrl.Manager, conf *config.Config, self *appsv1.Depl return nil } -func instances(conf *config.Config, self *appsv1.Deployment) []instance { +func instances(conf *config.Config) []instance { // public publicCfg := publicConfig(conf) publicAction := actionFromConfig(publicCfg) - publicResources := manifests.ExternalDnsResources(conf, self, []*manifests.ExternalDnsConfig{publicCfg}) + publicResources := manifests.ExternalDnsResources(conf, []*manifests.ExternalDnsConfig{publicCfg}) // private privateCfg := privateConfig(conf) privateAction := actionFromConfig(privateCfg) - privateResources := manifests.ExternalDnsResources(conf, self, []*manifests.ExternalDnsConfig{privateCfg}) + privateResources := manifests.ExternalDnsResources(conf, []*manifests.ExternalDnsConfig{privateCfg}) return []instance{ { diff --git a/pkg/controller/dns/external_dns_test.go b/pkg/controller/dns/external_dns_test.go index 85f8f6f3..92c74f22 100644 --- a/pkg/controller/dns/external_dns_test.go +++ b/pkg/controller/dns/external_dns_test.go @@ -11,7 +11,6 @@ import ( "github.com/Azure/aks-app-routing-operator/pkg/util" "github.com/google/uuid" "github.com/stretchr/testify/require" - appsv1 "k8s.io/api/apps/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/rest" @@ -25,9 +24,8 @@ var ( env *envtest.Environment restConfig *rest.Config err error - self *appsv1.Deployment = nil - uid = uuid.New().String() - noZones = config.Config{ + uid = uuid.New().String() + noZones = config.Config{ ClusterUid: uid, PrivateZoneConfig: config.DnsZoneConfig{}, PublicZoneConfig: config.DnsZoneConfig{}, @@ -182,12 +180,12 @@ func TestInstances(t *testing.T) { expected: []instance{ { config: publicConfig(&noZones), - resources: manifests.ExternalDnsResources(&noZones, self, []*manifests.ExternalDnsConfig{publicConfig(&noZones)}), + resources: manifests.ExternalDnsResources(&noZones, []*manifests.ExternalDnsConfig{publicConfig(&noZones)}), action: clean, }, { config: privateConfig(&noZones), - resources: manifests.ExternalDnsResources(&noZones, self, []*manifests.ExternalDnsConfig{privateConfig(&noZones)}), + resources: manifests.ExternalDnsResources(&noZones, []*manifests.ExternalDnsConfig{privateConfig(&noZones)}), action: clean, }, }, @@ -198,12 +196,12 @@ func TestInstances(t *testing.T) { expected: []instance{ { config: publicConfig(&onlyPrivZones), - resources: manifests.ExternalDnsResources(&onlyPrivZones, self, []*manifests.ExternalDnsConfig{publicConfig(&onlyPrivZones)}), + resources: manifests.ExternalDnsResources(&onlyPrivZones, []*manifests.ExternalDnsConfig{publicConfig(&onlyPrivZones)}), action: clean, }, { config: privateConfig(&onlyPrivZones), - resources: manifests.ExternalDnsResources(&onlyPrivZones, self, []*manifests.ExternalDnsConfig{privateConfig(&onlyPrivZones)}), + resources: manifests.ExternalDnsResources(&onlyPrivZones, []*manifests.ExternalDnsConfig{privateConfig(&onlyPrivZones)}), action: deploy, }, }, @@ -214,12 +212,12 @@ func TestInstances(t *testing.T) { expected: []instance{ { config: publicConfig(&onlyPubZones), - resources: manifests.ExternalDnsResources(&onlyPubZones, self, []*manifests.ExternalDnsConfig{publicConfig(&onlyPubZones)}), + resources: manifests.ExternalDnsResources(&onlyPubZones, []*manifests.ExternalDnsConfig{publicConfig(&onlyPubZones)}), action: deploy, }, { config: privateConfig(&onlyPubZones), - resources: manifests.ExternalDnsResources(&onlyPubZones, self, []*manifests.ExternalDnsConfig{privateConfig(&onlyPubZones)}), + resources: manifests.ExternalDnsResources(&onlyPubZones, []*manifests.ExternalDnsConfig{privateConfig(&onlyPubZones)}), action: clean, }, }, @@ -230,12 +228,12 @@ func TestInstances(t *testing.T) { expected: []instance{ { config: publicConfig(&allZones), - resources: manifests.ExternalDnsResources(&allZones, self, []*manifests.ExternalDnsConfig{publicConfig(&allZones)}), + resources: manifests.ExternalDnsResources(&allZones, []*manifests.ExternalDnsConfig{publicConfig(&allZones)}), action: deploy, }, { config: privateConfig(&allZones), - resources: manifests.ExternalDnsResources(&allZones, self, []*manifests.ExternalDnsConfig{privateConfig(&allZones)}), + resources: manifests.ExternalDnsResources(&allZones, []*manifests.ExternalDnsConfig{privateConfig(&allZones)}), action: deploy, }, }, @@ -243,7 +241,7 @@ func TestInstances(t *testing.T) { } for _, test := range tests { - instances := instances(test.conf, self) + instances := instances(test.conf) if !reflect.DeepEqual(instances, test.expected) { t.Error( "For", test.name, @@ -255,9 +253,9 @@ func TestInstances(t *testing.T) { } func TestFilterAction(t *testing.T) { - allClean := instances(&noZones, self) - allDeploy := instances(&allZones, self) - oneDeployOneClean := instances(&onlyPrivZones, self) + allClean := instances(&noZones) + allDeploy := instances(&allZones) + oneDeployOneClean := instances(&onlyPrivZones) tests := []struct { name string @@ -318,7 +316,7 @@ func TestFilterAction(t *testing.T) { } func TestGetResources(t *testing.T) { - instances := instances(&noZones, self) + instances := instances(&noZones) got := getResources(instances) var expected []client.Object for _, instance := range instances { @@ -345,17 +343,17 @@ func TestGetLabels(t *testing.T) { }, { name: "top level and private", - instances: filterAction(instances(&onlyPrivZones, self), deploy), + instances: filterAction(instances(&onlyPrivZones), deploy), expected: util.MergeMaps(manifests.GetTopLevelLabels(), manifests.PrivateProvider.Labels()), }, { name: "top level and public", - instances: filterAction(instances(&onlyPubZones, self), deploy), + instances: filterAction(instances(&onlyPubZones), deploy), expected: util.MergeMaps(manifests.GetTopLevelLabels(), manifests.PublicProvider.Labels()), }, { name: "all labels", - instances: instances(&allZones, self), + instances: instances(&allZones), expected: util.MergeMaps(manifests.GetTopLevelLabels(), manifests.PublicProvider.Labels(), manifests.PrivateProvider.Labels()), }, } @@ -374,36 +372,36 @@ func TestCleanObjs(t *testing.T) { }{ { name: "private dns clean", - instances: instances(&onlyPubZones, self), + instances: instances(&onlyPubZones), expected: []cleanObj{{ - resources: instances(&onlyPubZones, self)[1].resources, + resources: instances(&onlyPubZones)[1].resources, labels: util.MergeMaps(manifests.GetTopLevelLabels(), manifests.PrivateProvider.Labels()), }}, }, { name: "public dns clean", - instances: instances(&onlyPrivZones, self), + instances: instances(&onlyPrivZones), expected: []cleanObj{{ - resources: instances(&onlyPrivZones, self)[0].resources, + resources: instances(&onlyPrivZones)[0].resources, labels: util.MergeMaps(manifests.GetTopLevelLabels(), manifests.PublicProvider.Labels()), }}, }, { name: "all dns clean", - instances: instances(&noZones, self), + instances: instances(&noZones), expected: []cleanObj{ { - resources: instances(&noZones, self)[0].resources, + resources: instances(&noZones)[0].resources, labels: util.MergeMaps(manifests.GetTopLevelLabels(), manifests.PublicProvider.Labels()), }, { - resources: instances(&noZones, self)[1].resources, + resources: instances(&noZones)[1].resources, labels: util.MergeMaps(manifests.GetTopLevelLabels(), manifests.PrivateProvider.Labels()), }}, }, { name: "no dns clean", - instances: instances(&allZones, self), + instances: instances(&allZones), expected: []cleanObj(nil), }, } @@ -462,7 +460,7 @@ func TestAddExternalDnsCleaner(t *testing.T) { err = addExternalDnsCleaner(m, []cleanObj{ { - resources: instances(&noZones, self)[0].resources, + resources: instances(&noZones)[0].resources, labels: util.MergeMaps(manifests.GetTopLevelLabels(), manifests.PublicProvider.Labels()), }}) require.NoError(t, err) @@ -473,7 +471,7 @@ func TestNewExternalDns(t *testing.T) { require.NoError(t, err) conf := &config.Config{NS: "app-routing-system", OperatorDeployment: "operator"} - err = NewExternalDns(m, conf, self) + err = NewExternalDns(m, conf) require.NoError(t, err) } diff --git a/pkg/controller/nginx/nginx.go b/pkg/controller/nginx/nginx.go index b1222542..82c31cc6 100644 --- a/pkg/controller/nginx/nginx.go +++ b/pkg/controller/nginx/nginx.go @@ -4,14 +4,12 @@ import ( "context" "github.com/Azure/aks-app-routing-operator/pkg/config" - "github.com/Azure/aks-app-routing-operator/pkg/controller/ingress" "github.com/Azure/aks-app-routing-operator/pkg/controller/service" "github.com/Azure/aks-app-routing-operator/pkg/manifests" appsv1 "k8s.io/api/apps/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" - "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/manager" ) @@ -68,14 +66,6 @@ func New(m manager.Manager, conf *config.Config, self *appsv1.Deployment) ([]*ma defaultIngConfig: defaultIngConfig, } - if err := n.addIngressClassReconciler(); err != nil { - return nil, err - } - - if err := n.addIngressControllerReconciler(); err != nil { - return nil, err - } - if err := n.addIngressReconciler(); err != nil { return nil, err } @@ -83,24 +73,6 @@ func New(m manager.Manager, conf *config.Config, self *appsv1.Deployment) ([]*ma return n.ingConfigs, nil } -func (n *nginx) addIngressClassReconciler() error { - objs := []client.Object{} - for _, config := range n.ingConfigs { - objs = append(objs, manifests.NginxIngressClass(n.conf, n.self, config)...) - } - - return ingress.NewIngressClassReconciler(n.manager, objs, n.name) -} - -func (n *nginx) addIngressControllerReconciler() error { - objs := []client.Object{} - for _, config := range n.ingConfigs { - objs = append(objs, manifests.NginxIngressControllerResources(n.conf, n.self, config)...) - } - - return ingress.NewIngressControllerReconciler(n.manager, objs, n.name) -} - func (n *nginx) addIngressReconciler() error { return service.NewNginxIngressReconciler(n.manager, n.defaultIngConfig) } diff --git a/pkg/controller/nginxingress/nginx_ingress_controller.go b/pkg/controller/nginxingress/nginx_ingress_controller.go new file mode 100644 index 00000000..3b7fce09 --- /dev/null +++ b/pkg/controller/nginxingress/nginx_ingress_controller.go @@ -0,0 +1,431 @@ +package nginxingress + +import ( + "context" + "errors" + "fmt" + "net/url" + "time" + + approutingv1alpha1 "github.com/Azure/aks-app-routing-operator/api/v1alpha1" + "github.com/Azure/aks-app-routing-operator/pkg/config" + "github.com/Azure/aks-app-routing-operator/pkg/controller/controllername" + "github.com/Azure/aks-app-routing-operator/pkg/controller/metrics" + "github.com/Azure/aks-app-routing-operator/pkg/manifests" + "github.com/Azure/aks-app-routing-operator/pkg/util" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + netv1 "k8s.io/api/networking/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/utils/keymutex" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log" +) + +const ( + controllerClassMaxLen = 250 +) + +var ( + nginxIngressControllerReconcilerName = controllername.New("nginx", "ingress", "controller", "reconciler") + + // collisionCountMu is used to prevent multiple nginxIngressController resources from determining their collisionCount at the same time. We use + // a hashed key mutex because collisions can only occur when the nginxIngressController resources have the same spec.ControllerNamePrefix field. + // This is the field used to key into this mutex. + collisionCountMu = keymutex.NewHashed(6) // 6 is the number of "buckets". It's not too big, not too small +) + +// nginxIngressControllerReconciler reconciles a NginxIngressController object +type nginxIngressControllerReconciler struct { + client client.Client + conf *config.Config +} + +// NewReconciler sets up the controller with the Manager. +func NewReconciler(conf *config.Config, mgr ctrl.Manager) error { + metrics.InitControllerMetrics(nginxIngressControllerReconcilerName) + + reconciler := &nginxIngressControllerReconciler{ + client: mgr.GetClient(), + conf: conf, + } + + if err := nginxIngressControllerReconcilerName.AddToController( + ctrl.NewControllerManagedBy(mgr). + For(&approutingv1alpha1.NginxIngressController{}). + Owns(&appsv1.Deployment{}), + mgr.GetLogger(), + ).Complete(reconciler); err != nil { + return err + } + + return nil +} + +func (n *nginxIngressControllerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, err error) { + lgr := log.FromContext(ctx, "nginxIngressController", req.NamespacedName, "reconciler", "event") + ctx = log.IntoContext(ctx, lgr) + + defer func() { + metrics.HandleControllerReconcileMetrics(nginxIngressControllerReconcilerName, res, err) + }() + + var nginxIngressController approutingv1alpha1.NginxIngressController + if err := n.client.Get(ctx, req.NamespacedName, &nginxIngressController); err != nil { + if apierrors.IsNotFound(err) { // object was deleted + lgr.Info("NginxIngressController not found") + return ctrl.Result{}, nil + } + + lgr.Error(err, "unable to fetch NginxIngressController") + return ctrl.Result{}, err + } + + lockKey := nginxIngressController.Spec.ControllerNamePrefix + collisionCountMu.LockKey(lockKey) + defer collisionCountMu.UnlockKey(lockKey) + if err := n.SetCollisionCount(ctx, &nginxIngressController); err != nil { + lgr.Error(err, "unable to set collision count") + return ctrl.Result{}, fmt.Errorf("setting collision count: %w", err) + } + + resources := n.ManagedResources(&nginxIngressController) + if resources == nil { + return ctrl.Result{}, fmt.Errorf("unable to get managed resources") + } + + managedRes, err := n.ReconcileResource(ctx, &nginxIngressController, resources) + defer func() { + n.updateStatus(&nginxIngressController, resources.Deployment, resources.IngressClass, managedRes) + if statusErr := n.client.Status().Update(ctx, &nginxIngressController); statusErr != nil { + lgr.Error(statusErr, "unable to update NginxIngressController status") + if err == nil { + err = statusErr + } + } + }() + if err != nil { + return ctrl.Result{}, fmt.Errorf("reconciling resource: %w", err) + } + + return ctrl.Result{}, nil +} + +// ReconcileResource reconciles the NginxIngressController resources returning a list of managed resources. +func (n *nginxIngressControllerReconciler) ReconcileResource(ctx context.Context, nic *approutingv1alpha1.NginxIngressController, res *manifests.NginxResources) ([]approutingv1alpha1.ManagedObjectReference, error) { + if nic == nil { + return nil, errors.New("nginxIngressController cannot be nil") + } + + start := time.Now() + lgr := log.FromContext(ctx, "nginxIngressController", nic.GetName()) + ctx = log.IntoContext(ctx, lgr) + lgr.Info("starting to reconcile resource") + defer lgr.Info("finished reconciling resource", "latencySec", time.Since(start).Seconds()) + + var managedResourceRefs []approutingv1alpha1.ManagedObjectReference + for _, obj := range res.Objects() { + // TODO: upsert works pretty well but we want to set annotations exactly on the nginx service, we should use an alternative strategy for that + + if err := util.Upsert(ctx, n.client, obj); err != nil { + lgr.Error(err, "unable to upsert object", "name", obj.GetName(), "kind", obj.GetObjectKind().GroupVersionKind().Kind, "namespace", obj.GetNamespace()) + return nil, fmt.Errorf("upserting object: %w", err) + } + + if managedByUs(obj) { + managedResourceRefs = append(managedResourceRefs, approutingv1alpha1.ManagedObjectReference{ + Name: obj.GetName(), + Namespace: obj.GetNamespace(), + Kind: obj.GetObjectKind().GroupVersionKind().Kind, + APIGroup: obj.GetObjectKind().GroupVersionKind().Group, + }) + + } + } + + return managedResourceRefs, nil +} + +func (n *nginxIngressControllerReconciler) ManagedResources(nic *approutingv1alpha1.NginxIngressController) *manifests.NginxResources { + if nic == nil { + return nil + } + + cc := "webapprouting.kubernetes.azure.com/nginx/" + url.PathEscape(nic.Name) + + // it's impossible for this to happen because we enforce nic.Name to be less than 101 characters + if len(cc) > controllerClassMaxLen { + cc = cc[:controllerClassMaxLen] + } + + nginxIngressCfg := &manifests.NginxIngressConfig{ + ControllerClass: cc, + ResourceName: fmt.Sprintf("%s-%d", nic.Spec.ControllerNamePrefix, nic.Status.CollisionCount), + IcName: nic.Spec.IngressClassName, + ServiceConfig: &manifests.ServiceConfig{ + Annotations: nic.Spec.LoadBalancerAnnotations, + }, + } + + res := manifests.GetNginxResources(n.conf, nginxIngressCfg) + owner := manifests.GetOwnerRefs(nic, true) + for _, obj := range res.Objects() { + if managedByUs(obj) { + obj.SetOwnerReferences(owner) + } + } + + return res +} + +func (n *nginxIngressControllerReconciler) SetCollisionCount(ctx context.Context, nic *approutingv1alpha1.NginxIngressController) error { + lgr := log.FromContext(ctx) + startingCollisionCount := nic.Status.CollisionCount + + // there's a limit to how many times we should try to find the collision count, we don't want to put too much stress on api server + // TODO: we should set a condition when hit + jitter retry interval + for i := 0; i < 10; i++ { + collision, err := n.collides(ctx, nic) + if err != nil { + lgr.Error(err, "unable to determine collision") + return fmt.Errorf("determining collision: %w", err) + } + + if collision == collisionIngressClass { + lgr.Info("ingress class collision") + meta.SetStatusCondition(&nic.Status.Conditions, metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeIngressClassReady, + Status: "Collision", + ObservedGeneration: nic.Generation, + Message: fmt.Sprintf("IngressClass %s already exists in the cluster and isn't owned by this resource. Delete the IngressClass or recreate this resource with a different spec.IngressClass field.", nic.Spec.IngressClassName), + Reason: "IngressClassCollision", + }) + if err := n.client.Status().Update(ctx, nic); err != nil { + lgr.Error(err, "unable to update status") + return fmt.Errorf("updating status with IngressClass collision") + } + + return nil // this isn't an error, it's caused by a race condition involving our webhook + } + + if collision == collisionNone { + break + } + + lgr.Info("reconcilable collision detected, incrementing", "collisionCount", nic.Status.CollisionCount) + nic.Status.CollisionCount++ + + if i == 9 { + return errors.New("too many collisions") + } + } + + if startingCollisionCount != nic.Status.CollisionCount { + lgr.Info("setting new collision count", "collisionCount", nic.Status.CollisionCount, "startingCollisionCount", startingCollisionCount) + if err := n.client.Status().Update(ctx, nic); err != nil { + lgr.Error(err, "unable to update status") + return fmt.Errorf("updating status: %w", err) + } + } + + return nil +} + +func (n *nginxIngressControllerReconciler) collides(ctx context.Context, nic *approutingv1alpha1.NginxIngressController) (collision, error) { + lgr := log.FromContext(ctx) + + res := n.ManagedResources(nic) + if res == nil { + return collisionNone, fmt.Errorf("getting managed objects") + } + + for _, obj := range res.Objects() { + lgr := lgr.WithValues("kind", obj.GetObjectKind().GroupVersionKind().Kind, "name", obj.GetName(), "namespace", obj.GetNamespace()) + + // if we won't own the resource, we don't care about collisions. + // this is most commonly used for namespaces since we shouldn't own + // namespaces + if !managedByUs(obj) { + lgr.Info("skipping collision check because we don't own the resource") + continue + } + + u := &unstructured.Unstructured{} + u.SetGroupVersionKind(obj.GetObjectKind().GroupVersionKind()) + + err := n.client.Get(ctx, client.ObjectKeyFromObject(obj), u) + if err != nil { + if apierrors.IsNotFound(err) { + continue + } + + return collisionNone, fmt.Errorf("getting object: %w", err) + } + + if owner := util.FindOwnerKind(u.GetOwnerReferences(), nic.Kind); owner == nic.Name { + lgr.Info("the nginxIngressController owns this resource") + continue + } + + lgr.Info("collision detected") + if obj.GetObjectKind().GroupVersionKind().Kind == "IngressClass" { + return collisionIngressClass, nil + } + + return collisionOther, nil + } + + lgr.Info("no collisions detected") + return collisionNone, nil +} + +// updateStatus updates the status of the NginxIngressController resource. If a nil controller Deployment or IngressClass is passed, the status is defaulted for those fields if they are not already set. +func (n *nginxIngressControllerReconciler) updateStatus(nic *approutingv1alpha1.NginxIngressController, controllerDeployment *appsv1.Deployment, ic *netv1.IngressClass, managedResourceRefs []approutingv1alpha1.ManagedObjectReference) { + if managedResourceRefs != nil { + nic.Status.ManagedResourceRefs = managedResourceRefs + } + + if ic == nil || ic.CreationTimestamp.IsZero() { + nic.SetCondition(metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeIngressClassReady, + Status: metav1.ConditionUnknown, + Reason: "IngressClassUnknown", + Message: "IngressClass is unknown", + }) + } else { + nic.SetCondition(metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeIngressClassReady, + Status: "True", + Reason: "IngressClassIsReady", + Message: "Ingress Class is up-to-date ", + }) + } + + // default conditions + if controllerDeployment == nil || controllerDeployment.CreationTimestamp.IsZero() { + nic.SetCondition(metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeControllerAvailable, + Status: metav1.ConditionUnknown, + Reason: "ControllerDeploymentUnknown", + Message: "Controller deployment is unknown", + }) + nic.SetCondition(metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeProgressing, + Status: metav1.ConditionUnknown, + Reason: "ControllerDeploymentUnknown", + Message: "Controller deployment is unknown", + }) + } else { + nic.Status.ControllerReadyReplicas = controllerDeployment.Status.ReadyReplicas + nic.Status.ControllerAvailableReplicas = controllerDeployment.Status.AvailableReplicas + nic.Status.ControllerUnavailableReplicas = controllerDeployment.Status.UnavailableReplicas + nic.Status.ControllerReplicas = controllerDeployment.Status.Replicas + + for _, cond := range controllerDeployment.Status.Conditions { + switch cond.Type { + case appsv1.DeploymentProgressing: + n.updateStatusControllerProgressing(nic, cond) + case appsv1.DeploymentAvailable: + n.updateStatusControllerAvailable(nic, cond) + } + } + } + + controllerAvailable := nic.GetCondition(approutingv1alpha1.ConditionTypeControllerAvailable) + icAvailable := nic.GetCondition(approutingv1alpha1.ConditionTypeIngressClassReady) + if controllerAvailable != nil && icAvailable != nil && controllerAvailable.Status == metav1.ConditionTrue && icAvailable.Status == metav1.ConditionTrue { + nic.SetCondition(metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeAvailable, + Status: metav1.ConditionTrue, + Reason: "ControllerIsAvailable", + Message: "Controller Deployment has minimum availability and IngressClass is up-to-date", + }) + } else { + nic.SetCondition(metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeAvailable, + Status: metav1.ConditionFalse, + Reason: "ControllerIsNotAvailable", + Message: "Controller Deployment does not have minimum availability or IngressClass is not up-to-date", + }) + } +} + +func (n *nginxIngressControllerReconciler) updateStatusControllerAvailable(nic *approutingv1alpha1.NginxIngressController, availableCondition appsv1.DeploymentCondition) { + if availableCondition.Type != appsv1.DeploymentAvailable { + return + } + + var cond metav1.Condition + switch availableCondition.Status { + case corev1.ConditionTrue: + cond = metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeControllerAvailable, + Status: metav1.ConditionTrue, + Reason: "ControllerDeploymentAvailable", + Message: "Controller Deployment is available", + } + case corev1.ConditionFalse: + cond = metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeControllerAvailable, + Status: metav1.ConditionFalse, + Reason: "ControllerDeploymentNotAvailable", + Message: "Controller Deployment is not available", + } + default: + cond = metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeControllerAvailable, + Status: metav1.ConditionUnknown, + Reason: "ControllerDeploymentUnknown", + Message: "Controller Deployment is in an unknown state", + } + } + + nic.SetCondition(cond) +} + +func (n *nginxIngressControllerReconciler) updateStatusControllerProgressing(nic *approutingv1alpha1.NginxIngressController, progressingCondition appsv1.DeploymentCondition) { + if progressingCondition.Type != appsv1.DeploymentProgressing { + return + } + + var cond metav1.Condition + switch progressingCondition.Status { + case corev1.ConditionTrue: + cond = metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeProgressing, + Status: metav1.ConditionTrue, + Reason: "ControllerDeploymentProgressing", + Message: "Controller Deployment has successfully progressed", + } + case corev1.ConditionFalse: + cond = metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeProgressing, + Status: metav1.ConditionFalse, + Reason: "ControllerDeploymentNotProgressing", + Message: "Controller Deployment has failed to progress", + } + default: + cond = metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeProgressing, + Status: metav1.ConditionUnknown, + Reason: "ControllerDeploymentProgressingUnknown", + Message: "Controller Deployment progress is unknown", + } + } + + nic.SetCondition(cond) +} + +func managedByUs(obj client.Object) bool { + for k, v := range manifests.GetTopLevelLabels() { + if obj.GetLabels()[k] != v { + return false + } + } + + return true +} diff --git a/pkg/controller/nginxingress/types.go b/pkg/controller/nginxingress/types.go new file mode 100644 index 00000000..0152d692 --- /dev/null +++ b/pkg/controller/nginxingress/types.go @@ -0,0 +1,11 @@ +package nginxingress + +// collision represents the type of collision that occurred when reconciling an nginxIngressController resource. +// This is used to determine the way we should handle the collision. +type collision int + +const ( + collisionNone collision = iota + collisionIngressClass + collisionOther +) diff --git a/pkg/manifests/common.go b/pkg/manifests/common.go index 878e0800..3214ce3d 100644 --- a/pkg/manifests/common.go +++ b/pkg/manifests/common.go @@ -1,30 +1,23 @@ package manifests import ( - appsv1 "k8s.io/api/apps/v1" + "github.com/Azure/aks-app-routing-operator/pkg/util" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" + "sigs.k8s.io/controller-runtime/pkg/client" "github.com/Azure/aks-app-routing-operator/pkg/config" ) const operatorName = "aks-app-routing-operator" -// resourceType is a struct that represents a Kubernetes resource type -type resourceType struct { - Group string - Version string - // Name is the name of the resource type - Name string -} - // GetTopLevelLabels returns labels that every resource App Routing manages have func GetTopLevelLabels() map[string]string { // this is a function to avoid any accidental mutation due to maps being reference types return map[string]string{"app.kubernetes.io/managed-by": operatorName} } -// Checks the first set of labels has the labels of the other passed in sets +// HasTopLevelLabels returns true if the given labels match the top level labels func HasTopLevelLabels(objLabels map[string]string) bool { if len(objLabels) == 0 { return false @@ -42,15 +35,20 @@ func HasTopLevelLabels(objLabels map[string]string) bool { return true } -func getOwnerRefs(deploy *appsv1.Deployment) []metav1.OwnerReference { - if deploy == nil { - return nil - } +// GetOwnerRefs returns the owner references for the given object +func GetOwnerRefs(owner client.Object, controller bool) []metav1.OwnerReference { + gvk := owner.GetObjectKind().GroupVersionKind() + apiVersion := gvk.GroupVersion().String() + kind := gvk.Kind + name := owner.GetName() + uid := owner.GetUID() + return []metav1.OwnerReference{{ - APIVersion: "apps/v1", - Kind: "Deployment", - Name: deploy.Name, - UID: deploy.UID, + APIVersion: apiVersion, + Kind: kind, + Name: name, + UID: uid, + Controller: util.ToPtr(controller), }} } @@ -61,8 +59,9 @@ func namespace(conf *config.Config) *corev1.Namespace { APIVersion: "v1", }, ObjectMeta: metav1.ObjectMeta{ - Name: conf.NS, - Labels: GetTopLevelLabels(), + Name: conf.NS, + // don't set top-level labels,namespace is not managed by operator + Labels: map[string]string{}, Annotations: map[string]string{}, }, } diff --git a/pkg/manifests/external_dns.go b/pkg/manifests/external_dns.go index 33ccd928..fad74d0d 100644 --- a/pkg/manifests/external_dns.go +++ b/pkg/manifests/external_dns.go @@ -83,7 +83,7 @@ type ExternalDnsConfig struct { } // ExternalDnsResources returns Kubernetes objects required for external dns -func ExternalDnsResources(conf *config.Config, self *appsv1.Deployment, externalDnsConfigs []*ExternalDnsConfig) []client.Object { +func ExternalDnsResources(conf *config.Config, externalDnsConfigs []*ExternalDnsConfig) []client.Object { var objs []client.Object // Can safely assume the namespace exists if using kube-system @@ -95,11 +95,6 @@ func ExternalDnsResources(conf *config.Config, self *appsv1.Deployment, external objs = append(objs, externalDnsResourcesFromConfig(conf, dnsConfig)...) } - owners := getOwnerRefs(self) - for _, obj := range objs { - obj.SetOwnerReferences(owners) - } - return objs } diff --git a/pkg/manifests/nginx.go b/pkg/manifests/nginx.go index bf33df47..a9e682f1 100644 --- a/pkg/manifests/nginx.go +++ b/pkg/manifests/nginx.go @@ -7,6 +7,8 @@ import ( "path" "strconv" + "github.com/Azure/aks-app-routing-operator/pkg/config" + "github.com/Azure/aks-app-routing-operator/pkg/util" appsv1 "k8s.io/api/apps/v1" autov1 "k8s.io/api/autoscaling/v1" corev1 "k8s.io/api/core/v1" @@ -16,10 +18,6 @@ import ( "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" - "sigs.k8s.io/controller-runtime/pkg/client" - - "github.com/Azure/aks-app-routing-operator/pkg/config" - "github.com/Azure/aks-app-routing-operator/pkg/util" ) const ( @@ -125,66 +123,49 @@ func (n *NginxIngressConfig) PodLabels() map[string]string { // ServiceConfig defines configuration options for required resources for a Service that goes with an Ingress type ServiceConfig struct { - IsInternal bool - Hostname string + Annotations map[string]string } -// NginxIngressClass returns an IngressClass for the provided configuration -func NginxIngressClass(conf *config.Config, self *appsv1.Deployment, ingressConfig *NginxIngressConfig) []client.Object { - ing := &netv1.IngressClass{ - TypeMeta: metav1.TypeMeta{ - Kind: "IngressClass", - APIVersion: "networking.k8s.io/v1", - }, - ObjectMeta: metav1.ObjectMeta{Name: ingressConfig.IcName, Labels: GetTopLevelLabels()}, - Spec: netv1.IngressClassSpec{ - Controller: ingressConfig.ControllerClass, - }, +func GetNginxResources(conf *config.Config, ingressConfig *NginxIngressConfig) *NginxResources { + res := &NginxResources{ + IngressClass: newNginxIngressControllerIngressClass(conf, ingressConfig), + ServiceAccount: newNginxIngressControllerServiceAccount(conf, ingressConfig), + ClusterRole: newNginxIngressControllerClusterRole(conf, ingressConfig), + Role: newNginxIngressControllerRole(conf, ingressConfig), + ClusterRoleBinding: newNginxIngressControllerClusterRoleBinding(conf, ingressConfig), + RoleBinding: newNginxIngressControllerRoleBinding(conf, ingressConfig), + Service: newNginxIngressControllerService(conf, ingressConfig), + Deployment: newNginxIngressControllerDeployment(conf, ingressConfig), + ConfigMap: newNginxIngressControllerConfigmap(conf, ingressConfig), + HorizontalPodAutoscaler: newNginxIngressControllerHPA(conf, ingressConfig), + PodDisruptionBudget: newNginxIngressControllerPDB(conf, ingressConfig), } - objs := []client.Object{ing} - - owners := getOwnerRefs(self) - for _, obj := range objs { - obj.SetOwnerReferences(owners) + for _, obj := range res.Objects() { l := util.MergeMaps(obj.GetLabels(), nginxLabels) obj.SetLabels(l) } - return objs -} - -// NginxIngressControllerResources returns Kubernetes objects required for the controller -func NginxIngressControllerResources(conf *config.Config, self *appsv1.Deployment, ingressConfig *NginxIngressConfig) []client.Object { - objs := []client.Object{} - - // Can safely assume the namespace exists if using kube-system + // Can safely assume the namespace exists if using kube-system. + // Purposefully do this after applying the labels, namespace isn't an Nginx-specific resource if conf.NS != "kube-system" { - objs = append(objs, namespace(conf)) + res.Namespace = namespace(conf) } - objs = append(objs, - newNginxIngressControllerServiceAccount(conf, ingressConfig), - newNginxIngressControllerClusterRole(conf, ingressConfig), - newNginxIngressControllerClusterRoleBinding(conf, ingressConfig), - newNginxIngressControllerRole(conf, ingressConfig), - newNginxIngressControllerRoleBinding(conf, ingressConfig), - newNginxIngressControllerService(conf, ingressConfig), - newNginxIngressControllerDeployment(conf, ingressConfig), - newNginxIngressControllerConfigmap(conf, ingressConfig), - newNginxIngressControllerPDB(conf, ingressConfig), - newNginxIngressControllerHPA(conf, ingressConfig), - ) - - owners := getOwnerRefs(self) - for _, obj := range objs { - obj.SetOwnerReferences(owners) + return res +} - l := util.MergeMaps(obj.GetLabels(), nginxLabels) - obj.SetLabels(l) +func newNginxIngressControllerIngressClass(conf *config.Config, ingressConfig *NginxIngressConfig) *netv1.IngressClass { + return &netv1.IngressClass{ + TypeMeta: metav1.TypeMeta{ + Kind: "IngressClass", + APIVersion: "networking.k8s.io/v1", + }, + ObjectMeta: metav1.ObjectMeta{Name: ingressConfig.IcName, Labels: GetTopLevelLabels()}, + Spec: netv1.IngressClassSpec{ + Controller: ingressConfig.ControllerClass, + }, } - - return objs } func newNginxIngressControllerServiceAccount(conf *config.Config, ingressConfig *NginxIngressConfig) *corev1.ServiceAccount { @@ -382,28 +363,17 @@ func newNginxIngressControllerRoleBinding(conf *config.Config, ingressConfig *Ng } func newNginxIngressControllerService(conf *config.Config, ingressConfig *NginxIngressConfig) *corev1.Service { - isInternal := false - hostname := "" - if ingressConfig.ServiceConfig != nil { // this should always be nil prior to dynamic provisioning work - isInternal = ingressConfig.ServiceConfig.IsInternal - hostname = ingressConfig.ServiceConfig.Hostname - } - annotations := make(map[string]string) - if isInternal { - annotations["service.beta.kubernetes.io/azure-load-balancer-internal"] = "true" - } - if hostname != "" { - annotations["external-dns.alpha.kubernetes.io/hostname"] = "loadbalancer." + hostname - } - if hostname != "" && isInternal { - annotations["external-dns.alpha.kubernetes.io/internal-hostname"] = "clusterip." + hostname - } - for k, v := range promAnnotations { annotations[k] = v } + if ingressConfig != nil && ingressConfig.ServiceConfig != nil { + for k, v := range ingressConfig.ServiceConfig.Annotations { + annotations[k] = v + } + } + return &corev1.Service{ TypeMeta: metav1.TypeMeta{ Kind: "Service", diff --git a/pkg/manifests/types.go b/pkg/manifests/types.go new file mode 100644 index 00000000..39cf7669 --- /dev/null +++ b/pkg/manifests/types.go @@ -0,0 +1,58 @@ +package manifests + +import ( + appsv1 "k8s.io/api/apps/v1" + autov1 "k8s.io/api/autoscaling/v1" + corev1 "k8s.io/api/core/v1" + netv1 "k8s.io/api/networking/v1" + policyv1 "k8s.io/api/policy/v1" + rbacv1 "k8s.io/api/rbac/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// resourceType is a struct that represents a Kubernetes resource type +type resourceType struct { + Group string + Version string + // Name is the name of the resource type + Name string +} + +// NginxResources is a struct that represents the Kubernetes resources that are created for the Nginx Ingress Controller. When these resources +// are acted upon by client-go, the fields here are updated since they are pointers to the actual resources. +type NginxResources struct { + Namespace *corev1.Namespace + IngressClass *netv1.IngressClass + ServiceAccount *corev1.ServiceAccount + ClusterRole *rbacv1.ClusterRole + Role *rbacv1.Role + ClusterRoleBinding *rbacv1.ClusterRoleBinding + RoleBinding *rbacv1.RoleBinding + Service *corev1.Service + Deployment *appsv1.Deployment + ConfigMap *corev1.ConfigMap + HorizontalPodAutoscaler *autov1.HorizontalPodAutoscaler + PodDisruptionBudget *policyv1.PodDisruptionBudget +} + +func (n *NginxResources) Objects() []client.Object { + objs := []client.Object{ + n.IngressClass, + n.ServiceAccount, + n.ClusterRole, + n.Role, + n.ClusterRoleBinding, + n.RoleBinding, + n.Service, + n.Deployment, + n.ConfigMap, + n.HorizontalPodAutoscaler, + n.PodDisruptionBudget, + } + + if n.Namespace != nil { + objs = append([]client.Object{n.Namespace}, objs...) // put namespace at front, so we can create resources in order + } + + return objs +} From ffe8958356c96aae92bd323dedb7a020468148fc Mon Sep 17 00:00:00 2001 From: Oliver King Date: Fri, 3 Nov 2023 11:01:23 -0500 Subject: [PATCH 2/6] statuses all working nicely --- api/v1alpha1/nginxingresscontroller_types.go | 5 +- ...tes.azure.com_nginxingresscontrollers.yaml | 1 + .../nginxingress/nginx_ingress_controller.go | 163 ++++++++++++------ 3 files changed, 114 insertions(+), 55 deletions(-) diff --git a/api/v1alpha1/nginxingresscontroller_types.go b/api/v1alpha1/nginxingresscontroller_types.go index 1f64b4d7..c20a47c6 100644 --- a/api/v1alpha1/nginxingresscontroller_types.go +++ b/api/v1alpha1/nginxingresscontroller_types.go @@ -13,7 +13,9 @@ func init() { } const ( - maxNameLength = 100 + maxNameLength = 100 + // MaxCollisions is the maximum number of collisions allowed when generating a name for a managed resource. This corresponds to the status.CollisionCount + MaxCollisions = 5 maxControllerNamePrefix = 253 - 10 // 253 is the max length of resource names - 10 to account for the length of the suffix https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-subdomain-names ) @@ -73,6 +75,7 @@ type NginxIngressControllerStatus struct { // Count of hash collisions for the managed resources. The App Routing Operator uses this field // as a collision avoidance mechanism when it needs to create the name for the managed resources. // +optional + // +kubebuilder:validation:Maximum=5 CollisionCount int32 `json:"collisionCount"` // ManagedResourceRefs is a list of references to the managed resources diff --git a/config/crd/bases/approuting.kubernetes.azure.com_nginxingresscontrollers.yaml b/config/crd/bases/approuting.kubernetes.azure.com_nginxingresscontrollers.yaml index e9d38fa3..bcc5f795 100644 --- a/config/crd/bases/approuting.kubernetes.azure.com_nginxingresscontrollers.yaml +++ b/config/crd/bases/approuting.kubernetes.azure.com_nginxingresscontrollers.yaml @@ -80,6 +80,7 @@ spec: App Routing Operator uses this field as a collision avoidance mechanism when it needs to create the name for the managed resources. format: int32 + maximum: 5 type: integer conditions: description: Conditions is an array of current observed conditions diff --git a/pkg/controller/nginxingress/nginx_ingress_controller.go b/pkg/controller/nginxingress/nginx_ingress_controller.go index 3b7fce09..0a111ca9 100644 --- a/pkg/controller/nginxingress/nginx_ingress_controller.go +++ b/pkg/controller/nginxingress/nginx_ingress_controller.go @@ -30,12 +30,18 @@ const ( controllerClassMaxLen = 250 ) +var ( + icCollisionErr = errors.New("collision on the IngressClass") + maxCollisionsErr = errors.New("max collisions reached") +) + var ( nginxIngressControllerReconcilerName = controllername.New("nginx", "ingress", "controller", "reconciler") // collisionCountMu is used to prevent multiple nginxIngressController resources from determining their collisionCount at the same time. We use // a hashed key mutex because collisions can only occur when the nginxIngressController resources have the same spec.ControllerNamePrefix field. - // This is the field used to key into this mutex. + // This is the field used to key into this mutex. Using this mutex prevents a race condition of multiple nginxIngressController resources attempting + // to determine their collisionCount at the same time then both attempting to create and take ownership of the same resources. collisionCountMu = keymutex.NewHashed(6) // 6 is the number of "buckets". It's not too big, not too small ) @@ -67,8 +73,11 @@ func NewReconciler(conf *config.Config, mgr ctrl.Manager) error { } func (n *nginxIngressControllerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, err error) { - lgr := log.FromContext(ctx, "nginxIngressController", req.NamespacedName, "reconciler", "event") + start := time.Now() + lgr := log.FromContext(ctx, "nginxIngressController", req.NamespacedName) ctx = log.IntoContext(ctx, lgr) + lgr.Info("reconciling NginxIngressController") + defer lgr.Info("finished reconciling resource", "latencySec", time.Since(start).String()) defer func() { metrics.HandleControllerReconcileMetrics(nginxIngressControllerReconcilerName, res, err) @@ -84,23 +93,32 @@ func (n *nginxIngressControllerReconciler) Reconcile(ctx context.Context, req ct lgr.Error(err, "unable to fetch NginxIngressController") return ctrl.Result{}, err } + lgr = lgr.WithValues("generation", nginxIngressController.Generation) + ctx = log.IntoContext(ctx, lgr) + + var managedRes []approutingv1alpha1.ManagedObjectReference = nil + var controllerDeployment *appsv1.Deployment = nil + var ingressClass *netv1.IngressClass = nil lockKey := nginxIngressController.Spec.ControllerNamePrefix collisionCountMu.LockKey(lockKey) defer collisionCountMu.UnlockKey(lockKey) - if err := n.SetCollisionCount(ctx, &nginxIngressController); err != nil { - lgr.Error(err, "unable to set collision count") - return ctrl.Result{}, fmt.Errorf("setting collision count: %w", err) - } - resources := n.ManagedResources(&nginxIngressController) - if resources == nil { - return ctrl.Result{}, fmt.Errorf("unable to get managed resources") - } + lgr.Info("determining collision count") + startingCollisionCount := nginxIngressController.Status.CollisionCount + newCollisionCount, collisionCountErr := n.GetCollisionCount(ctx, &nginxIngressController) + if collisionCountErr == nil && newCollisionCount != startingCollisionCount { + nginxIngressController.Status.CollisionCount = newCollisionCount + if err := n.client.Status().Update(ctx, &nginxIngressController); err != nil { + lgr.Error(err, "unable to update collision count status") + return ctrl.Result{}, fmt.Errorf("updating status: %w", err) + } - managedRes, err := n.ReconcileResource(ctx, &nginxIngressController, resources) - defer func() { - n.updateStatus(&nginxIngressController, resources.Deployment, resources.IngressClass, managedRes) + return ctrl.Result{Requeue: true}, nil + } + defer func() { // defer is before checking err so that we can update status even if there is an error + lgr.Info("updating status") + n.updateStatus(ctx, &nginxIngressController, controllerDeployment, ingressClass, managedRes, collisionCountErr) if statusErr := n.client.Status().Update(ctx, &nginxIngressController); statusErr != nil { lgr.Error(statusErr, "unable to update NginxIngressController status") if err == nil { @@ -108,7 +126,28 @@ func (n *nginxIngressControllerReconciler) Reconcile(ctx context.Context, req ct } } }() + if collisionCountErr != nil { + if isUnreconcilableError(collisionCountErr) { + lgr.Info("unreconcilable collision count") + return ctrl.Result{RequeueAfter: time.Minute}, nil // requeue in case cx fix the unreconilable reason + } + + lgr.Error(collisionCountErr, "unable to get determine collision count") + return ctrl.Result{}, fmt.Errorf("determining collision count: %w", collisionCountErr) + } + + lgr.Info("calculating managed resources") + resources := n.ManagedResources(&nginxIngressController) + if resources == nil { + return ctrl.Result{}, fmt.Errorf("unable to get managed resources") + } + controllerDeployment = resources.Deployment + ingressClass = resources.IngressClass + + lgr.Info("reconciling managed resources") + managedRes, err = n.ReconcileResource(ctx, &nginxIngressController, resources) if err != nil { + lgr.Error(err, "unable to reconcile resource") return ctrl.Result{}, fmt.Errorf("reconciling resource: %w", err) } @@ -121,11 +160,7 @@ func (n *nginxIngressControllerReconciler) ReconcileResource(ctx context.Context return nil, errors.New("nginxIngressController cannot be nil") } - start := time.Now() - lgr := log.FromContext(ctx, "nginxIngressController", nic.GetName()) - ctx = log.IntoContext(ctx, lgr) - lgr.Info("starting to reconcile resource") - defer lgr.Info("finished reconciling resource", "latencySec", time.Since(start).Seconds()) + lgr := log.FromContext(ctx) var managedResourceRefs []approutingv1alpha1.ManagedObjectReference for _, obj := range res.Objects() { @@ -136,14 +171,13 @@ func (n *nginxIngressControllerReconciler) ReconcileResource(ctx context.Context return nil, fmt.Errorf("upserting object: %w", err) } - if managedByUs(obj) { + if manifests.HasTopLevelLabels(obj.GetLabels()) { managedResourceRefs = append(managedResourceRefs, approutingv1alpha1.ManagedObjectReference{ Name: obj.GetName(), Namespace: obj.GetNamespace(), Kind: obj.GetObjectKind().GroupVersionKind().Kind, APIGroup: obj.GetObjectKind().GroupVersionKind().Group, }) - } } @@ -157,7 +191,7 @@ func (n *nginxIngressControllerReconciler) ManagedResources(nic *approutingv1alp cc := "webapprouting.kubernetes.azure.com/nginx/" + url.PathEscape(nic.Name) - // it's impossible for this to happen because we enforce nic.Name to be less than 101 characters + // it's impossible for this to happen because we enforce nic.Name to be less than 101 characters in validating webhooks if len(cc) > controllerClassMaxLen { cc = cc[:controllerClassMaxLen] } @@ -174,7 +208,7 @@ func (n *nginxIngressControllerReconciler) ManagedResources(nic *approutingv1alp res := manifests.GetNginxResources(n.conf, nginxIngressCfg) owner := manifests.GetOwnerRefs(nic, true) for _, obj := range res.Objects() { - if managedByUs(obj) { + if manifests.HasTopLevelLabels(obj.GetLabels()) { obj.SetOwnerReferences(owner) } } @@ -182,21 +216,28 @@ func (n *nginxIngressControllerReconciler) ManagedResources(nic *approutingv1alp return res } -func (n *nginxIngressControllerReconciler) SetCollisionCount(ctx context.Context, nic *approutingv1alpha1.NginxIngressController) error { +func (n *nginxIngressControllerReconciler) GetCollisionCount(ctx context.Context, nic *approutingv1alpha1.NginxIngressController) (int32, error) { lgr := log.FromContext(ctx) + + // it's not this fn's job to overwrite the collision count, so we revert any changes we make startingCollisionCount := nic.Status.CollisionCount + defer func() { + nic.Status.CollisionCount = startingCollisionCount + }() - // there's a limit to how many times we should try to find the collision count, we don't want to put too much stress on api server - // TODO: we should set a condition when hit + jitter retry interval - for i := 0; i < 10; i++ { + for { + lgr = lgr.WithValues("collisionCount", nic.Status.CollisionCount) collision, err := n.collides(ctx, nic) if err != nil { lgr.Error(err, "unable to determine collision") - return fmt.Errorf("determining collision: %w", err) + return 0, fmt.Errorf("determining collision: %w", err) } if collision == collisionIngressClass { + // rare since our webhook guards against it, but it's possible lgr.Info("ingress class collision") + return 0, icCollisionErr + meta.SetStatusCondition(&nic.Status.Conditions, metav1.Condition{ Type: approutingv1alpha1.ConditionTypeIngressClassReady, Status: "Collision", @@ -206,33 +247,23 @@ func (n *nginxIngressControllerReconciler) SetCollisionCount(ctx context.Context }) if err := n.client.Status().Update(ctx, nic); err != nil { lgr.Error(err, "unable to update status") - return fmt.Errorf("updating status with IngressClass collision") } - - return nil // this isn't an error, it's caused by a race condition involving our webhook } if collision == collisionNone { break } - lgr.Info("reconcilable collision detected, incrementing", "collisionCount", nic.Status.CollisionCount) - nic.Status.CollisionCount++ - - if i == 9 { - return errors.New("too many collisions") + if nic.Status.CollisionCount == approutingv1alpha1.MaxCollisions { + lgr.Info("max collisions reached") + return 0, maxCollisionsErr } - } - if startingCollisionCount != nic.Status.CollisionCount { - lgr.Info("setting new collision count", "collisionCount", nic.Status.CollisionCount, "startingCollisionCount", startingCollisionCount) - if err := n.client.Status().Update(ctx, nic); err != nil { - lgr.Error(err, "unable to update status") - return fmt.Errorf("updating status: %w", err) - } + lgr.Info("reconcilable collision detected, incrementing") + nic.Status.CollisionCount++ } - return nil + return nic.Status.CollisionCount, nil } func (n *nginxIngressControllerReconciler) collides(ctx context.Context, nic *approutingv1alpha1.NginxIngressController) (collision, error) { @@ -249,14 +280,13 @@ func (n *nginxIngressControllerReconciler) collides(ctx context.Context, nic *ap // if we won't own the resource, we don't care about collisions. // this is most commonly used for namespaces since we shouldn't own // namespaces - if !managedByUs(obj) { + if !manifests.HasTopLevelLabels(obj.GetLabels()) { lgr.Info("skipping collision check because we don't own the resource") continue } u := &unstructured.Unstructured{} u.SetGroupVersionKind(obj.GetObjectKind().GroupVersionKind()) - err := n.client.Get(ctx, client.ObjectKeyFromObject(obj), u) if err != nil { if apierrors.IsNotFound(err) { @@ -273,6 +303,7 @@ func (n *nginxIngressControllerReconciler) collides(ctx context.Context, nic *ap lgr.Info("collision detected") if obj.GetObjectKind().GroupVersionKind().Kind == "IngressClass" { + lgr.Info("collision is with an IngressClass") return collisionIngressClass, nil } @@ -284,12 +315,16 @@ func (n *nginxIngressControllerReconciler) collides(ctx context.Context, nic *ap } // updateStatus updates the status of the NginxIngressController resource. If a nil controller Deployment or IngressClass is passed, the status is defaulted for those fields if they are not already set. -func (n *nginxIngressControllerReconciler) updateStatus(nic *approutingv1alpha1.NginxIngressController, controllerDeployment *appsv1.Deployment, ic *netv1.IngressClass, managedResourceRefs []approutingv1alpha1.ManagedObjectReference) { +func (n *nginxIngressControllerReconciler) updateStatus(ctx context.Context, nic *approutingv1alpha1.NginxIngressController, controllerDeployment *appsv1.Deployment, ic *netv1.IngressClass, managedResourceRefs []approutingv1alpha1.ManagedObjectReference, err error) { + lgr := log.FromContext(ctx) + if managedResourceRefs != nil { + lgr.Info("updating managed resource refs status") nic.Status.ManagedResourceRefs = managedResourceRefs } if ic == nil || ic.CreationTimestamp.IsZero() { + lgr.Info("adding IngressClassUnknown condition") nic.SetCondition(metav1.Condition{ Type: approutingv1alpha1.ConditionTypeIngressClassReady, Status: metav1.ConditionUnknown, @@ -297,6 +332,7 @@ func (n *nginxIngressControllerReconciler) updateStatus(nic *approutingv1alpha1. Message: "IngressClass is unknown", }) } else { + lgr.Info("adding IngressClassIsReady condition") nic.SetCondition(metav1.Condition{ Type: approutingv1alpha1.ConditionTypeIngressClassReady, Status: "True", @@ -307,12 +343,14 @@ func (n *nginxIngressControllerReconciler) updateStatus(nic *approutingv1alpha1. // default conditions if controllerDeployment == nil || controllerDeployment.CreationTimestamp.IsZero() { + lgr.Info("adding ControllerDeploymentUnknown to ControllerAvailable condition") nic.SetCondition(metav1.Condition{ Type: approutingv1alpha1.ConditionTypeControllerAvailable, Status: metav1.ConditionUnknown, Reason: "ControllerDeploymentUnknown", Message: "Controller deployment is unknown", }) + lgr.Info("adding ControllerDeploymentUnknown to Progressing condition") nic.SetCondition(metav1.Condition{ Type: approutingv1alpha1.ConditionTypeProgressing, Status: metav1.ConditionUnknown, @@ -320,6 +358,7 @@ func (n *nginxIngressControllerReconciler) updateStatus(nic *approutingv1alpha1. Message: "Controller deployment is unknown", }) } else { + lgr.Info("updating controller replicas status") nic.Status.ControllerReadyReplicas = controllerDeployment.Status.ReadyReplicas nic.Status.ControllerAvailableReplicas = controllerDeployment.Status.AvailableReplicas nic.Status.ControllerUnavailableReplicas = controllerDeployment.Status.UnavailableReplicas @@ -338,6 +377,7 @@ func (n *nginxIngressControllerReconciler) updateStatus(nic *approutingv1alpha1. controllerAvailable := nic.GetCondition(approutingv1alpha1.ConditionTypeControllerAvailable) icAvailable := nic.GetCondition(approutingv1alpha1.ConditionTypeIngressClassReady) if controllerAvailable != nil && icAvailable != nil && controllerAvailable.Status == metav1.ConditionTrue && icAvailable.Status == metav1.ConditionTrue { + lgr.Info("adding ControllerIsAvailable condition") nic.SetCondition(metav1.Condition{ Type: approutingv1alpha1.ConditionTypeAvailable, Status: metav1.ConditionTrue, @@ -345,6 +385,7 @@ func (n *nginxIngressControllerReconciler) updateStatus(nic *approutingv1alpha1. Message: "Controller Deployment has minimum availability and IngressClass is up-to-date", }) } else { + lgr.Info("adding ControllerIsNotAvailable condition") nic.SetCondition(metav1.Condition{ Type: approutingv1alpha1.ConditionTypeAvailable, Status: metav1.ConditionFalse, @@ -352,6 +393,26 @@ func (n *nginxIngressControllerReconciler) updateStatus(nic *approutingv1alpha1. Message: "Controller Deployment does not have minimum availability or IngressClass is not up-to-date", }) } + + // error checking at end to take precedence over other conditions + if errors.Is(err, icCollisionErr) { + lgr.Info("adding IngressClassCollision condition") + nic.SetCondition(metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeProgressing, + Status: metav1.ConditionFalse, + Reason: "IngressClassCollision", + Message: "IngressClass already exists and is not owned by this controller", + }) + } + if errors.Is(err, maxCollisionsErr) { + lgr.Info("adding TooManyCollisions condition") + nic.SetCondition(metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeProgressing, + Status: metav1.ConditionFalse, + Reason: "TooManyCollisions", + Message: "Too many collisions with existing resources", + }) + } } func (n *nginxIngressControllerReconciler) updateStatusControllerAvailable(nic *approutingv1alpha1.NginxIngressController, availableCondition appsv1.DeploymentCondition) { @@ -420,12 +481,6 @@ func (n *nginxIngressControllerReconciler) updateStatusControllerProgressing(nic nic.SetCondition(cond) } -func managedByUs(obj client.Object) bool { - for k, v := range manifests.GetTopLevelLabels() { - if obj.GetLabels()[k] != v { - return false - } - } - - return true +func isUnreconcilableError(err error) bool { + return errors.Is(err, icCollisionErr) || errors.Is(err, maxCollisionsErr) } From 79375e820b1383217c41d59414552b3a7f2d14af Mon Sep 17 00:00:00 2001 From: Oliver King Date: Fri, 3 Nov 2023 11:22:06 -0500 Subject: [PATCH 3/6] update ginx ingress controller crd --- .../nginxingress/nginx_ingress_controller.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/pkg/controller/nginxingress/nginx_ingress_controller.go b/pkg/controller/nginxingress/nginx_ingress_controller.go index 0a111ca9..47435d20 100644 --- a/pkg/controller/nginxingress/nginx_ingress_controller.go +++ b/pkg/controller/nginxingress/nginx_ingress_controller.go @@ -20,6 +20,7 @@ import ( "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/client-go/tools/record" "k8s.io/utils/keymutex" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -49,6 +50,7 @@ var ( type nginxIngressControllerReconciler struct { client client.Client conf *config.Config + events record.EventRecorder } // NewReconciler sets up the controller with the Manager. @@ -58,6 +60,7 @@ func NewReconciler(conf *config.Config, mgr ctrl.Manager) error { reconciler := &nginxIngressControllerReconciler{ client: mgr.GetClient(), conf: conf, + events: mgr.GetEventRecorderFor("aks-app-routing-operator"), } if err := nginxIngressControllerReconcilerName.AddToController( @@ -129,7 +132,7 @@ func (n *nginxIngressControllerReconciler) Reconcile(ctx context.Context, req ct if collisionCountErr != nil { if isUnreconcilableError(collisionCountErr) { lgr.Info("unreconcilable collision count") - return ctrl.Result{RequeueAfter: time.Minute}, nil // requeue in case cx fix the unreconilable reason + return ctrl.Result{RequeueAfter: time.Minute}, nil // requeue in case cx fix the unreconcilable reason } lgr.Error(collisionCountErr, "unable to get determine collision count") @@ -167,6 +170,9 @@ func (n *nginxIngressControllerReconciler) ReconcileResource(ctx context.Context // TODO: upsert works pretty well but we want to set annotations exactly on the nginx service, we should use an alternative strategy for that if err := util.Upsert(ctx, n.client, obj); err != nil { + // publish an event so cx can see things like their policy is preventing us from creating a resource + n.events.Eventf(nic, corev1.EventTypeWarning, "EnsuringManagedResourcesFailed", "Failed to ensure managed resource %s %s/%s: %s", obj.GetObjectKind().GroupVersionKind().Kind, obj.GetNamespace(), obj.GetName(), err.Error()) + lgr.Error(err, "unable to upsert object", "name", obj.GetName(), "kind", obj.GetObjectKind().GroupVersionKind().Kind, "namespace", obj.GetNamespace()) return nil, fmt.Errorf("upserting object: %w", err) } @@ -403,6 +409,7 @@ func (n *nginxIngressControllerReconciler) updateStatus(ctx context.Context, nic Reason: "IngressClassCollision", Message: "IngressClass already exists and is not owned by this controller", }) + n.events.Event(nic, corev1.EventTypeWarning, "IngressClassCollision", "IngressClass already exists and is not owned by this controller. Change the spec.IngressClassName to an unused IngressClass name in a new NginxIngressController.") } if errors.Is(err, maxCollisionsErr) { lgr.Info("adding TooManyCollisions condition") @@ -412,6 +419,7 @@ func (n *nginxIngressControllerReconciler) updateStatus(ctx context.Context, nic Reason: "TooManyCollisions", Message: "Too many collisions with existing resources", }) + n.events.Event(nic, corev1.EventTypeWarning, "TooManyCollisions", "Too many collisions with existing resources. Change the spec.ControllerNamePrefix to something more unique in a new NginxIngressController.") } } From 4ad512d7e75de09154d457488dc2efe2c2547454 Mon Sep 17 00:00:00 2001 From: Oliver King Date: Fri, 3 Nov 2023 11:28:33 -0500 Subject: [PATCH 4/6] add manifests tests --- pkg/manifests/external_dns_test.go | 2 +- .../fixtures/common/another-namespace.json | 5 +- pkg/manifests/fixtures/common/namespace.json | 5 +- pkg/manifests/fixtures/external_dns/full.json | 113 +-------- .../fixtures/external_dns/no-ownership.json | 5 +- .../fixtures/external_dns/private.json | 63 +---- .../external_dns/short-sync-interval.json | 113 +-------- .../fixtures/nginx/full-ingressclass.json | 25 -- pkg/manifests/fixtures/nginx/full.json | 220 ++++++------------ pkg/manifests/fixtures/nginx/internal.json | 217 ++++++----------- pkg/manifests/fixtures/nginx/kube-system.json | 112 +++++---- .../nginx/no-ownership-ingressclass.json | 17 -- .../fixtures/nginx/no-ownership.json | 112 +++++---- .../nginx/optional-features-disabled.json | 112 +++++---- pkg/manifests/nginx_test.go | 17 +- 15 files changed, 369 insertions(+), 769 deletions(-) delete mode 100644 pkg/manifests/fixtures/nginx/full-ingressclass.json delete mode 100644 pkg/manifests/fixtures/nginx/no-ownership-ingressclass.json diff --git a/pkg/manifests/external_dns_test.go b/pkg/manifests/external_dns_test.go index 198c780c..86c615d5 100644 --- a/pkg/manifests/external_dns_test.go +++ b/pkg/manifests/external_dns_test.go @@ -87,7 +87,7 @@ var ( func TestExternalDnsResources(t *testing.T) { for _, tc := range testCases { - objs := ExternalDnsResources(tc.Conf, tc.Deploy, tc.DnsConfigs) + objs := ExternalDnsResources(tc.Conf, tc.DnsConfigs) fixture := path.Join("fixtures", "external_dns", tc.Name) + ".json" AssertFixture(t, fixture, objs) diff --git a/pkg/manifests/fixtures/common/another-namespace.json b/pkg/manifests/fixtures/common/another-namespace.json index 0c529775..df24a92c 100644 --- a/pkg/manifests/fixtures/common/another-namespace.json +++ b/pkg/manifests/fixtures/common/another-namespace.json @@ -4,10 +4,7 @@ "apiVersion": "v1", "metadata": { "name": "another-test-namespace", - "creationTimestamp": null, - "labels": { - "app.kubernetes.io/managed-by": "aks-app-routing-operator" - } + "creationTimestamp": null }, "spec": {}, "status": {} diff --git a/pkg/manifests/fixtures/common/namespace.json b/pkg/manifests/fixtures/common/namespace.json index 93500d95..59a83115 100644 --- a/pkg/manifests/fixtures/common/namespace.json +++ b/pkg/manifests/fixtures/common/namespace.json @@ -4,10 +4,7 @@ "apiVersion": "v1", "metadata": { "name": "test-namespace", - "creationTimestamp": null, - "labels": { - "app.kubernetes.io/managed-by": "aks-app-routing-operator" - } + "creationTimestamp": null }, "spec": {}, "status": {} diff --git a/pkg/manifests/fixtures/external_dns/full.json b/pkg/manifests/fixtures/external_dns/full.json index d4ef0174..b45a13e3 100644 --- a/pkg/manifests/fixtures/external_dns/full.json +++ b/pkg/manifests/fixtures/external_dns/full.json @@ -4,18 +4,7 @@ "apiVersion": "v1", "metadata": { "name": "test-namespace", - "creationTimestamp": null, - "labels": { - "app.kubernetes.io/managed-by": "aks-app-routing-operator" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + "creationTimestamp": null }, "spec": {}, "status": {} @@ -30,15 +19,7 @@ "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "external-dns" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } } }, { @@ -50,15 +31,7 @@ "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "external-dns" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "rules": [ { @@ -115,15 +88,7 @@ "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "external-dns" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "subjects": [ { @@ -148,15 +113,7 @@ "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "external-dns" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "data": { "azure.json": "{\"cloud\":\"\",\"location\":\"\",\"resourceGroup\":\"test-resource-group-public\",\"subscriptionId\":\"test-subscription-id\",\"tenantId\":\"test-tenant-id\",\"useManagedIdentityExtension\":true,\"userAssignedIdentityID\":\"\"}" @@ -172,15 +129,7 @@ "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "external-dns" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "spec": { "replicas": 1, @@ -333,15 +282,7 @@ "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "external-dns-private" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } } }, { @@ -353,15 +294,7 @@ "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "external-dns-private" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "rules": [ { @@ -418,15 +351,7 @@ "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "external-dns-private" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "subjects": [ { @@ -451,15 +376,7 @@ "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "external-dns-private" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "data": { "azure.json": "{\"cloud\":\"\",\"location\":\"\",\"resourceGroup\":\"test-resource-group-private\",\"subscriptionId\":\"test-subscription-id\",\"tenantId\":\"test-tenant-id\",\"useManagedIdentityExtension\":true,\"userAssignedIdentityID\":\"\"}" @@ -475,15 +392,7 @@ "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "external-dns-private" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "spec": { "replicas": 1, diff --git a/pkg/manifests/fixtures/external_dns/no-ownership.json b/pkg/manifests/fixtures/external_dns/no-ownership.json index 721605f4..45a6f648 100644 --- a/pkg/manifests/fixtures/external_dns/no-ownership.json +++ b/pkg/manifests/fixtures/external_dns/no-ownership.json @@ -4,10 +4,7 @@ "apiVersion": "v1", "metadata": { "name": "test-namespace", - "creationTimestamp": null, - "labels": { - "app.kubernetes.io/managed-by": "aks-app-routing-operator" - } + "creationTimestamp": null }, "spec": {}, "status": {} diff --git a/pkg/manifests/fixtures/external_dns/private.json b/pkg/manifests/fixtures/external_dns/private.json index 34745ef0..b47b0dd5 100644 --- a/pkg/manifests/fixtures/external_dns/private.json +++ b/pkg/manifests/fixtures/external_dns/private.json @@ -4,18 +4,7 @@ "apiVersion": "v1", "metadata": { "name": "test-namespace", - "creationTimestamp": null, - "labels": { - "app.kubernetes.io/managed-by": "aks-app-routing-operator" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + "creationTimestamp": null }, "spec": {}, "status": {} @@ -30,15 +19,7 @@ "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "external-dns-private" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } } }, { @@ -50,15 +31,7 @@ "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "external-dns-private" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "rules": [ { @@ -115,15 +88,7 @@ "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "external-dns-private" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "subjects": [ { @@ -148,15 +113,7 @@ "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "external-dns-private" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "data": { "azure.json": "{\"cloud\":\"\",\"location\":\"\",\"resourceGroup\":\"test-resource-group-private\",\"subscriptionId\":\"test-subscription-id\",\"tenantId\":\"test-tenant-id\",\"useManagedIdentityExtension\":true,\"userAssignedIdentityID\":\"\"}" @@ -172,15 +129,7 @@ "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "external-dns-private" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "spec": { "replicas": 1, diff --git a/pkg/manifests/fixtures/external_dns/short-sync-interval.json b/pkg/manifests/fixtures/external_dns/short-sync-interval.json index 95ae25c6..925a48f5 100644 --- a/pkg/manifests/fixtures/external_dns/short-sync-interval.json +++ b/pkg/manifests/fixtures/external_dns/short-sync-interval.json @@ -4,18 +4,7 @@ "apiVersion": "v1", "metadata": { "name": "test-namespace", - "creationTimestamp": null, - "labels": { - "app.kubernetes.io/managed-by": "aks-app-routing-operator" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + "creationTimestamp": null }, "spec": {}, "status": {} @@ -30,15 +19,7 @@ "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "external-dns" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } } }, { @@ -50,15 +31,7 @@ "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "external-dns" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "rules": [ { @@ -115,15 +88,7 @@ "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "external-dns" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "subjects": [ { @@ -148,15 +113,7 @@ "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "external-dns" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "data": { "azure.json": "{\"cloud\":\"\",\"location\":\"\",\"resourceGroup\":\"test-resource-group-public\",\"subscriptionId\":\"test-subscription-id\",\"tenantId\":\"test-tenant-id\",\"useManagedIdentityExtension\":true,\"userAssignedIdentityID\":\"\"}" @@ -172,15 +129,7 @@ "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "external-dns" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "spec": { "replicas": 1, @@ -333,15 +282,7 @@ "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "external-dns-private" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } } }, { @@ -353,15 +294,7 @@ "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "external-dns-private" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "rules": [ { @@ -418,15 +351,7 @@ "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "external-dns-private" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "subjects": [ { @@ -451,15 +376,7 @@ "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "external-dns-private" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "data": { "azure.json": "{\"cloud\":\"\",\"location\":\"\",\"resourceGroup\":\"test-resource-group-private\",\"subscriptionId\":\"test-subscription-id\",\"tenantId\":\"test-tenant-id\",\"useManagedIdentityExtension\":true,\"userAssignedIdentityID\":\"\"}" @@ -475,15 +392,7 @@ "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "external-dns-private" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "spec": { "replicas": 1, diff --git a/pkg/manifests/fixtures/nginx/full-ingressclass.json b/pkg/manifests/fixtures/nginx/full-ingressclass.json deleted file mode 100644 index a070f8df..00000000 --- a/pkg/manifests/fixtures/nginx/full-ingressclass.json +++ /dev/null @@ -1,25 +0,0 @@ -[ - { - "kind": "IngressClass", - "apiVersion": "networking.k8s.io/v1", - "metadata": { - "name": "webapprouting.kubernetes.azure.com", - "creationTimestamp": null, - "labels": { - "app.kubernetes.io/managed-by": "aks-app-routing-operator", - "app.kubernetes.io/name": "nginx" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] - }, - "spec": { - "controller": "webapprouting.kubernetes.azure.com/nginx" - } - } - ] \ No newline at end of file diff --git a/pkg/manifests/fixtures/nginx/full.json b/pkg/manifests/fixtures/nginx/full.json index ddc2272f..307d0554 100644 --- a/pkg/manifests/fixtures/nginx/full.json +++ b/pkg/manifests/fixtures/nginx/full.json @@ -4,22 +4,25 @@ "apiVersion": "v1", "metadata": { "name": "test-namespace", + "creationTimestamp": null + }, + "spec": {}, + "status": {} + }, + { + "kind": "IngressClass", + "apiVersion": "networking.k8s.io/v1", + "metadata": { + "name": "webapprouting.kubernetes.azure.com", "creationTimestamp": null, "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "nginx" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, - "spec": {}, - "status": {} + "spec": { + "controller": "webapprouting.kubernetes.azure.com/nginx" + } }, { "kind": "ServiceAccount", @@ -32,15 +35,7 @@ "app.kubernetes.io/component": "ingress-controller", "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "nginx" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } } }, { @@ -53,15 +48,7 @@ "app.kubernetes.io/component": "ingress-controller", "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "nginx" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "rules": [ { @@ -181,39 +168,6 @@ } ] }, - { - "kind": "ClusterRoleBinding", - "apiVersion": "rbac.authorization.k8s.io/v1", - "metadata": { - "name": "nginx", - "creationTimestamp": null, - "labels": { - "app.kubernetes.io/component": "ingress-controller", - "app.kubernetes.io/managed-by": "aks-app-routing-operator", - "app.kubernetes.io/name": "nginx" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] - }, - "subjects": [ - { - "kind": "ServiceAccount", - "name": "nginx", - "namespace": "test-namespace" - } - ], - "roleRef": { - "apiGroup": "rbac.authorization.k8s.io", - "kind": "ClusterRole", - "name": "nginx" - } - }, { "kind": "Role", "apiVersion": "rbac.authorization.k8s.io/v1", @@ -225,15 +179,7 @@ "app.kubernetes.io/component": "ingress-controller", "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "nginx" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "rules": [ { @@ -377,6 +323,31 @@ } ] }, + { + "kind": "ClusterRoleBinding", + "apiVersion": "rbac.authorization.k8s.io/v1", + "metadata": { + "name": "nginx", + "creationTimestamp": null, + "labels": { + "app.kubernetes.io/component": "ingress-controller", + "app.kubernetes.io/managed-by": "aks-app-routing-operator", + "app.kubernetes.io/name": "nginx" + } + }, + "subjects": [ + { + "kind": "ServiceAccount", + "name": "nginx", + "namespace": "test-namespace" + } + ], + "roleRef": { + "apiGroup": "rbac.authorization.k8s.io", + "kind": "ClusterRole", + "name": "nginx" + } + }, { "kind": "RoleBinding", "apiVersion": "rbac.authorization.k8s.io/v1", @@ -388,15 +359,7 @@ "app.kubernetes.io/component": "ingress-controller", "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "nginx" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "subjects": [ { @@ -425,16 +388,9 @@ }, "annotations": { "prometheus.io/port": "10254", - "prometheus.io/scrape": "true" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + "prometheus.io/scrape": "true", + "service.beta.kubernetes.io/azure-load-balancer-internal": "true" + } }, "spec": { "ports": [ @@ -475,15 +431,7 @@ "app.kubernetes.io/component": "ingress-controller", "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "nginx" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "spec": { "selector": { @@ -660,15 +608,7 @@ "app.kubernetes.io/component": "ingress-controller", "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "nginx" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "data": { "allow-snippet-annotations": "true", @@ -676,8 +616,8 @@ } }, { - "kind": "PodDisruptionBudget", - "apiVersion": "policy/v1", + "kind": "HorizontalPodAutoscaler", + "apiVersion": "autoscaling/v1", "metadata": { "name": "nginx", "namespace": "test-namespace", @@ -686,34 +626,26 @@ "app.kubernetes.io/component": "ingress-controller", "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "nginx" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "spec": { - "selector": { - "matchLabels": { - "app": "nginx" - } + "scaleTargetRef": { + "kind": "Deployment", + "name": "nginx", + "apiVersion": "apps/v1" }, - "maxUnavailable": 1 + "minReplicas": 2, + "maxReplicas": 100, + "targetCPUUtilizationPercentage": 80 }, "status": { - "disruptionsAllowed": 0, - "currentHealthy": 0, - "desiredHealthy": 0, - "expectedPods": 0 + "currentReplicas": 0, + "desiredReplicas": 0 } }, { - "kind": "HorizontalPodAutoscaler", - "apiVersion": "autoscaling/v1", + "kind": "PodDisruptionBudget", + "apiVersion": "policy/v1", "metadata": { "name": "nginx", "namespace": "test-namespace", @@ -722,29 +654,21 @@ "app.kubernetes.io/component": "ingress-controller", "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "nginx" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "spec": { - "scaleTargetRef": { - "kind": "Deployment", - "name": "nginx", - "apiVersion": "apps/v1" + "selector": { + "matchLabels": { + "app": "nginx" + } }, - "minReplicas": 2, - "maxReplicas": 100, - "targetCPUUtilizationPercentage": 80 + "maxUnavailable": 1 }, "status": { - "currentReplicas": 0, - "desiredReplicas": 0 + "disruptionsAllowed": 0, + "currentHealthy": 0, + "desiredHealthy": 0, + "expectedPods": 0 } } ] \ No newline at end of file diff --git a/pkg/manifests/fixtures/nginx/internal.json b/pkg/manifests/fixtures/nginx/internal.json index f590790b..713d1013 100644 --- a/pkg/manifests/fixtures/nginx/internal.json +++ b/pkg/manifests/fixtures/nginx/internal.json @@ -4,22 +4,25 @@ "apiVersion": "v1", "metadata": { "name": "test-namespace", + "creationTimestamp": null + }, + "spec": {}, + "status": {} + }, + { + "kind": "IngressClass", + "apiVersion": "networking.k8s.io/v1", + "metadata": { + "name": "nginx-private", "creationTimestamp": null, "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "nginx" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, - "spec": {}, - "status": {} + "spec": { + "controller": "test-controller-class" + } }, { "kind": "ServiceAccount", @@ -32,15 +35,7 @@ "app.kubernetes.io/component": "ingress-controller", "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "nginx" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } } }, { @@ -53,15 +48,7 @@ "app.kubernetes.io/component": "ingress-controller", "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "nginx" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "rules": [ { @@ -181,39 +168,6 @@ } ] }, - { - "kind": "ClusterRoleBinding", - "apiVersion": "rbac.authorization.k8s.io/v1", - "metadata": { - "name": "nginx", - "creationTimestamp": null, - "labels": { - "app.kubernetes.io/component": "ingress-controller", - "app.kubernetes.io/managed-by": "aks-app-routing-operator", - "app.kubernetes.io/name": "nginx" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] - }, - "subjects": [ - { - "kind": "ServiceAccount", - "name": "nginx", - "namespace": "test-namespace" - } - ], - "roleRef": { - "apiGroup": "rbac.authorization.k8s.io", - "kind": "ClusterRole", - "name": "nginx" - } - }, { "kind": "Role", "apiVersion": "rbac.authorization.k8s.io/v1", @@ -225,15 +179,7 @@ "app.kubernetes.io/component": "ingress-controller", "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "nginx" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "rules": [ { @@ -377,6 +323,31 @@ } ] }, + { + "kind": "ClusterRoleBinding", + "apiVersion": "rbac.authorization.k8s.io/v1", + "metadata": { + "name": "nginx", + "creationTimestamp": null, + "labels": { + "app.kubernetes.io/component": "ingress-controller", + "app.kubernetes.io/managed-by": "aks-app-routing-operator", + "app.kubernetes.io/name": "nginx" + } + }, + "subjects": [ + { + "kind": "ServiceAccount", + "name": "nginx", + "namespace": "test-namespace" + } + ], + "roleRef": { + "apiGroup": "rbac.authorization.k8s.io", + "kind": "ClusterRole", + "name": "nginx" + } + }, { "kind": "RoleBinding", "apiVersion": "rbac.authorization.k8s.io/v1", @@ -388,15 +359,7 @@ "app.kubernetes.io/component": "ingress-controller", "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "nginx" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "subjects": [ { @@ -426,15 +389,7 @@ "annotations": { "prometheus.io/port": "10254", "prometheus.io/scrape": "true" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "spec": { "ports": [ @@ -475,15 +430,7 @@ "app.kubernetes.io/component": "ingress-controller", "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "nginx" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "spec": { "selector": { @@ -660,15 +607,7 @@ "app.kubernetes.io/component": "ingress-controller", "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "nginx" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "data": { "allow-snippet-annotations": "true", @@ -676,8 +615,8 @@ } }, { - "kind": "PodDisruptionBudget", - "apiVersion": "policy/v1", + "kind": "HorizontalPodAutoscaler", + "apiVersion": "autoscaling/v1", "metadata": { "name": "nginx", "namespace": "test-namespace", @@ -686,34 +625,26 @@ "app.kubernetes.io/component": "ingress-controller", "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "nginx" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "spec": { - "selector": { - "matchLabels": { - "app": "nginx" - } + "scaleTargetRef": { + "kind": "Deployment", + "name": "nginx", + "apiVersion": "apps/v1" }, - "maxUnavailable": 1 + "minReplicas": 2, + "maxReplicas": 100, + "targetCPUUtilizationPercentage": 80 }, "status": { - "disruptionsAllowed": 0, - "currentHealthy": 0, - "desiredHealthy": 0, - "expectedPods": 0 + "currentReplicas": 0, + "desiredReplicas": 0 } }, { - "kind": "HorizontalPodAutoscaler", - "apiVersion": "autoscaling/v1", + "kind": "PodDisruptionBudget", + "apiVersion": "policy/v1", "metadata": { "name": "nginx", "namespace": "test-namespace", @@ -722,29 +653,21 @@ "app.kubernetes.io/component": "ingress-controller", "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "nginx" - }, - "ownerReferences": [ - { - "apiVersion": "apps/v1", - "kind": "Deployment", - "name": "test-operator-deploy", - "uid": "test-operator-deploy-uid" - } - ] + } }, "spec": { - "scaleTargetRef": { - "kind": "Deployment", - "name": "nginx", - "apiVersion": "apps/v1" + "selector": { + "matchLabels": { + "app": "nginx" + } }, - "minReplicas": 2, - "maxReplicas": 100, - "targetCPUUtilizationPercentage": 80 + "maxUnavailable": 1 }, "status": { - "currentReplicas": 0, - "desiredReplicas": 0 + "disruptionsAllowed": 0, + "currentHealthy": 0, + "desiredHealthy": 0, + "expectedPods": 0 } } ] \ No newline at end of file diff --git a/pkg/manifests/fixtures/nginx/kube-system.json b/pkg/manifests/fixtures/nginx/kube-system.json index 5e118a62..1d708d01 100644 --- a/pkg/manifests/fixtures/nginx/kube-system.json +++ b/pkg/manifests/fixtures/nginx/kube-system.json @@ -1,4 +1,19 @@ [ + { + "kind": "IngressClass", + "apiVersion": "networking.k8s.io/v1", + "metadata": { + "name": "webapprouting.kubernetes.azure.com", + "creationTimestamp": null, + "labels": { + "app.kubernetes.io/managed-by": "aks-app-routing-operator", + "app.kubernetes.io/name": "nginx" + } + }, + "spec": { + "controller": "webapprouting.kubernetes.azure.com/nginx" + } + }, { "kind": "ServiceAccount", "apiVersion": "v1", @@ -143,31 +158,6 @@ } ] }, - { - "kind": "ClusterRoleBinding", - "apiVersion": "rbac.authorization.k8s.io/v1", - "metadata": { - "name": "nginx", - "creationTimestamp": null, - "labels": { - "app.kubernetes.io/component": "ingress-controller", - "app.kubernetes.io/managed-by": "aks-app-routing-operator", - "app.kubernetes.io/name": "nginx" - } - }, - "subjects": [ - { - "kind": "ServiceAccount", - "name": "nginx", - "namespace": "kube-system" - } - ], - "roleRef": { - "apiGroup": "rbac.authorization.k8s.io", - "kind": "ClusterRole", - "name": "nginx" - } - }, { "kind": "Role", "apiVersion": "rbac.authorization.k8s.io/v1", @@ -323,6 +313,31 @@ } ] }, + { + "kind": "ClusterRoleBinding", + "apiVersion": "rbac.authorization.k8s.io/v1", + "metadata": { + "name": "nginx", + "creationTimestamp": null, + "labels": { + "app.kubernetes.io/component": "ingress-controller", + "app.kubernetes.io/managed-by": "aks-app-routing-operator", + "app.kubernetes.io/name": "nginx" + } + }, + "subjects": [ + { + "kind": "ServiceAccount", + "name": "nginx", + "namespace": "kube-system" + } + ], + "roleRef": { + "apiGroup": "rbac.authorization.k8s.io", + "kind": "ClusterRole", + "name": "nginx" + } + }, { "kind": "RoleBinding", "apiVersion": "rbac.authorization.k8s.io/v1", @@ -363,7 +378,8 @@ }, "annotations": { "prometheus.io/port": "10254", - "prometheus.io/scrape": "true" + "prometheus.io/scrape": "true", + "service.beta.kubernetes.io/azure-load-balancer-internal": "true" } }, "spec": { @@ -590,8 +606,8 @@ } }, { - "kind": "PodDisruptionBudget", - "apiVersion": "policy/v1", + "kind": "HorizontalPodAutoscaler", + "apiVersion": "autoscaling/v1", "metadata": { "name": "nginx", "namespace": "kube-system", @@ -603,23 +619,23 @@ } }, "spec": { - "selector": { - "matchLabels": { - "app": "nginx" - } + "scaleTargetRef": { + "kind": "Deployment", + "name": "nginx", + "apiVersion": "apps/v1" }, - "maxUnavailable": 1 + "minReplicas": 2, + "maxReplicas": 100, + "targetCPUUtilizationPercentage": 80 }, "status": { - "disruptionsAllowed": 0, - "currentHealthy": 0, - "desiredHealthy": 0, - "expectedPods": 0 + "currentReplicas": 0, + "desiredReplicas": 0 } }, { - "kind": "HorizontalPodAutoscaler", - "apiVersion": "autoscaling/v1", + "kind": "PodDisruptionBudget", + "apiVersion": "policy/v1", "metadata": { "name": "nginx", "namespace": "kube-system", @@ -631,18 +647,18 @@ } }, "spec": { - "scaleTargetRef": { - "kind": "Deployment", - "name": "nginx", - "apiVersion": "apps/v1" + "selector": { + "matchLabels": { + "app": "nginx" + } }, - "minReplicas": 2, - "maxReplicas": 100, - "targetCPUUtilizationPercentage": 80 + "maxUnavailable": 1 }, "status": { - "currentReplicas": 0, - "desiredReplicas": 0 + "disruptionsAllowed": 0, + "currentHealthy": 0, + "desiredHealthy": 0, + "expectedPods": 0 } } ] \ No newline at end of file diff --git a/pkg/manifests/fixtures/nginx/no-ownership-ingressclass.json b/pkg/manifests/fixtures/nginx/no-ownership-ingressclass.json deleted file mode 100644 index 20f1c969..00000000 --- a/pkg/manifests/fixtures/nginx/no-ownership-ingressclass.json +++ /dev/null @@ -1,17 +0,0 @@ -[ - { - "kind": "IngressClass", - "apiVersion": "networking.k8s.io/v1", - "metadata": { - "name": "webapprouting.kubernetes.azure.com", - "creationTimestamp": null, - "labels": { - "app.kubernetes.io/managed-by": "aks-app-routing-operator", - "app.kubernetes.io/name": "nginx" - } - }, - "spec": { - "controller": "webapprouting.kubernetes.azure.com/nginx" - } - } - ] \ No newline at end of file diff --git a/pkg/manifests/fixtures/nginx/no-ownership.json b/pkg/manifests/fixtures/nginx/no-ownership.json index 95f744c3..307d0554 100644 --- a/pkg/manifests/fixtures/nginx/no-ownership.json +++ b/pkg/manifests/fixtures/nginx/no-ownership.json @@ -4,14 +4,25 @@ "apiVersion": "v1", "metadata": { "name": "test-namespace", + "creationTimestamp": null + }, + "spec": {}, + "status": {} + }, + { + "kind": "IngressClass", + "apiVersion": "networking.k8s.io/v1", + "metadata": { + "name": "webapprouting.kubernetes.azure.com", "creationTimestamp": null, "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "nginx" } }, - "spec": {}, - "status": {} + "spec": { + "controller": "webapprouting.kubernetes.azure.com/nginx" + } }, { "kind": "ServiceAccount", @@ -157,31 +168,6 @@ } ] }, - { - "kind": "ClusterRoleBinding", - "apiVersion": "rbac.authorization.k8s.io/v1", - "metadata": { - "name": "nginx", - "creationTimestamp": null, - "labels": { - "app.kubernetes.io/component": "ingress-controller", - "app.kubernetes.io/managed-by": "aks-app-routing-operator", - "app.kubernetes.io/name": "nginx" - } - }, - "subjects": [ - { - "kind": "ServiceAccount", - "name": "nginx", - "namespace": "test-namespace" - } - ], - "roleRef": { - "apiGroup": "rbac.authorization.k8s.io", - "kind": "ClusterRole", - "name": "nginx" - } - }, { "kind": "Role", "apiVersion": "rbac.authorization.k8s.io/v1", @@ -337,6 +323,31 @@ } ] }, + { + "kind": "ClusterRoleBinding", + "apiVersion": "rbac.authorization.k8s.io/v1", + "metadata": { + "name": "nginx", + "creationTimestamp": null, + "labels": { + "app.kubernetes.io/component": "ingress-controller", + "app.kubernetes.io/managed-by": "aks-app-routing-operator", + "app.kubernetes.io/name": "nginx" + } + }, + "subjects": [ + { + "kind": "ServiceAccount", + "name": "nginx", + "namespace": "test-namespace" + } + ], + "roleRef": { + "apiGroup": "rbac.authorization.k8s.io", + "kind": "ClusterRole", + "name": "nginx" + } + }, { "kind": "RoleBinding", "apiVersion": "rbac.authorization.k8s.io/v1", @@ -377,7 +388,8 @@ }, "annotations": { "prometheus.io/port": "10254", - "prometheus.io/scrape": "true" + "prometheus.io/scrape": "true", + "service.beta.kubernetes.io/azure-load-balancer-internal": "true" } }, "spec": { @@ -604,8 +616,8 @@ } }, { - "kind": "PodDisruptionBudget", - "apiVersion": "policy/v1", + "kind": "HorizontalPodAutoscaler", + "apiVersion": "autoscaling/v1", "metadata": { "name": "nginx", "namespace": "test-namespace", @@ -617,23 +629,23 @@ } }, "spec": { - "selector": { - "matchLabels": { - "app": "nginx" - } + "scaleTargetRef": { + "kind": "Deployment", + "name": "nginx", + "apiVersion": "apps/v1" }, - "maxUnavailable": 1 + "minReplicas": 2, + "maxReplicas": 100, + "targetCPUUtilizationPercentage": 80 }, "status": { - "disruptionsAllowed": 0, - "currentHealthy": 0, - "desiredHealthy": 0, - "expectedPods": 0 + "currentReplicas": 0, + "desiredReplicas": 0 } }, { - "kind": "HorizontalPodAutoscaler", - "apiVersion": "autoscaling/v1", + "kind": "PodDisruptionBudget", + "apiVersion": "policy/v1", "metadata": { "name": "nginx", "namespace": "test-namespace", @@ -645,18 +657,18 @@ } }, "spec": { - "scaleTargetRef": { - "kind": "Deployment", - "name": "nginx", - "apiVersion": "apps/v1" + "selector": { + "matchLabels": { + "app": "nginx" + } }, - "minReplicas": 2, - "maxReplicas": 100, - "targetCPUUtilizationPercentage": 80 + "maxUnavailable": 1 }, "status": { - "currentReplicas": 0, - "desiredReplicas": 0 + "disruptionsAllowed": 0, + "currentHealthy": 0, + "desiredHealthy": 0, + "expectedPods": 0 } } ] \ No newline at end of file diff --git a/pkg/manifests/fixtures/nginx/optional-features-disabled.json b/pkg/manifests/fixtures/nginx/optional-features-disabled.json index 7305d3d3..02b5ef7c 100644 --- a/pkg/manifests/fixtures/nginx/optional-features-disabled.json +++ b/pkg/manifests/fixtures/nginx/optional-features-disabled.json @@ -4,14 +4,25 @@ "apiVersion": "v1", "metadata": { "name": "test-namespace", + "creationTimestamp": null + }, + "spec": {}, + "status": {} + }, + { + "kind": "IngressClass", + "apiVersion": "networking.k8s.io/v1", + "metadata": { + "name": "webapprouting.kubernetes.azure.com", "creationTimestamp": null, "labels": { "app.kubernetes.io/managed-by": "aks-app-routing-operator", "app.kubernetes.io/name": "nginx" } }, - "spec": {}, - "status": {} + "spec": { + "controller": "webapprouting.kubernetes.azure.com/nginx" + } }, { "kind": "ServiceAccount", @@ -157,31 +168,6 @@ } ] }, - { - "kind": "ClusterRoleBinding", - "apiVersion": "rbac.authorization.k8s.io/v1", - "metadata": { - "name": "nginx", - "creationTimestamp": null, - "labels": { - "app.kubernetes.io/component": "ingress-controller", - "app.kubernetes.io/managed-by": "aks-app-routing-operator", - "app.kubernetes.io/name": "nginx" - } - }, - "subjects": [ - { - "kind": "ServiceAccount", - "name": "nginx", - "namespace": "test-namespace" - } - ], - "roleRef": { - "apiGroup": "rbac.authorization.k8s.io", - "kind": "ClusterRole", - "name": "nginx" - } - }, { "kind": "Role", "apiVersion": "rbac.authorization.k8s.io/v1", @@ -337,6 +323,31 @@ } ] }, + { + "kind": "ClusterRoleBinding", + "apiVersion": "rbac.authorization.k8s.io/v1", + "metadata": { + "name": "nginx", + "creationTimestamp": null, + "labels": { + "app.kubernetes.io/component": "ingress-controller", + "app.kubernetes.io/managed-by": "aks-app-routing-operator", + "app.kubernetes.io/name": "nginx" + } + }, + "subjects": [ + { + "kind": "ServiceAccount", + "name": "nginx", + "namespace": "test-namespace" + } + ], + "roleRef": { + "apiGroup": "rbac.authorization.k8s.io", + "kind": "ClusterRole", + "name": "nginx" + } + }, { "kind": "RoleBinding", "apiVersion": "rbac.authorization.k8s.io/v1", @@ -377,7 +388,8 @@ }, "annotations": { "prometheus.io/port": "10254", - "prometheus.io/scrape": "true" + "prometheus.io/scrape": "true", + "service.beta.kubernetes.io/azure-load-balancer-internal": "true" } }, "spec": { @@ -603,8 +615,8 @@ } }, { - "kind": "PodDisruptionBudget", - "apiVersion": "policy/v1", + "kind": "HorizontalPodAutoscaler", + "apiVersion": "autoscaling/v1", "metadata": { "name": "nginx", "namespace": "test-namespace", @@ -616,23 +628,23 @@ } }, "spec": { - "selector": { - "matchLabels": { - "app": "nginx" - } + "scaleTargetRef": { + "kind": "Deployment", + "name": "nginx", + "apiVersion": "apps/v1" }, - "maxUnavailable": 1 + "minReplicas": 2, + "maxReplicas": 100, + "targetCPUUtilizationPercentage": 80 }, "status": { - "disruptionsAllowed": 0, - "currentHealthy": 0, - "desiredHealthy": 0, - "expectedPods": 0 + "currentReplicas": 0, + "desiredReplicas": 0 } }, { - "kind": "HorizontalPodAutoscaler", - "apiVersion": "autoscaling/v1", + "kind": "PodDisruptionBudget", + "apiVersion": "policy/v1", "metadata": { "name": "nginx", "namespace": "test-namespace", @@ -644,18 +656,18 @@ } }, "spec": { - "scaleTargetRef": { - "kind": "Deployment", - "name": "nginx", - "apiVersion": "apps/v1" + "selector": { + "matchLabels": { + "app": "nginx" + } }, - "minReplicas": 2, - "maxReplicas": 100, - "targetCPUUtilizationPercentage": 80 + "maxUnavailable": 1 }, "status": { - "currentReplicas": 0, - "desiredReplicas": 0 + "disruptionsAllowed": 0, + "currentHealthy": 0, + "desiredHealthy": 0, + "expectedPods": 0 } } ] \ No newline at end of file diff --git a/pkg/manifests/nginx_test.go b/pkg/manifests/nginx_test.go index 81bd06c6..7019570d 100644 --- a/pkg/manifests/nginx_test.go +++ b/pkg/manifests/nginx_test.go @@ -17,6 +17,11 @@ var ( ControllerClass: "webapprouting.kubernetes.azure.com/nginx", ResourceName: "nginx", IcName: "webapprouting.kubernetes.azure.com", + ServiceConfig: &ServiceConfig{ + Annotations: map[string]string{ + "service.beta.kubernetes.io/azure-load-balancer-internal": "true", + }, + }, } controllerTestCases = []struct { Name string @@ -130,17 +135,9 @@ var ( func TestIngressControllerResources(t *testing.T) { for _, tc := range controllerTestCases { - objs := NginxIngressControllerResources(tc.Conf, tc.Deploy, tc.IngConfig) + objs := GetNginxResources(tc.Conf, tc.IngConfig) fixture := path.Join("fixtures", "nginx", tc.Name) + ".json" - AssertFixture(t, fixture, objs) - } -} - -func TestIngressClassResources(t *testing.T) { - for _, tc := range classTestCases { - objs := NginxIngressClass(tc.Conf, tc.Deploy, tc.IngConfig) - fixture := path.Join("fixtures", "nginx", tc.Name) + "-ingressclass.json" - AssertFixture(t, fixture, objs) + AssertFixture(t, fixture, objs.Objects()) } } From 7ccef06112aefd806fb659f9ebbaeaad5ca3ee04 Mon Sep 17 00:00:00 2001 From: Oliver King Date: Fri, 3 Nov 2023 13:34:56 -0500 Subject: [PATCH 5/6] add status unit tests --- .../nginxingress/nginx_ingress_controller.go | 103 ++-- .../nginx_ingress_controller_test.go | 455 ++++++++++++++++++ pkg/manifests/common_test.go | 74 +++ 3 files changed, 589 insertions(+), 43 deletions(-) create mode 100644 pkg/controller/nginxingress/nginx_ingress_controller_test.go diff --git a/pkg/controller/nginxingress/nginx_ingress_controller.go b/pkg/controller/nginxingress/nginx_ingress_controller.go index 47435d20..56ca7126 100644 --- a/pkg/controller/nginxingress/nginx_ingress_controller.go +++ b/pkg/controller/nginxingress/nginx_ingress_controller.go @@ -121,7 +121,7 @@ func (n *nginxIngressControllerReconciler) Reconcile(ctx context.Context, req ct } defer func() { // defer is before checking err so that we can update status even if there is an error lgr.Info("updating status") - n.updateStatus(ctx, &nginxIngressController, controllerDeployment, ingressClass, managedRes, collisionCountErr) + n.updateStatus(&nginxIngressController, controllerDeployment, ingressClass, managedRes, collisionCountErr) if statusErr := n.client.Status().Update(ctx, &nginxIngressController); statusErr != nil { lgr.Error(statusErr, "unable to update NginxIngressController status") if err == nil { @@ -321,16 +321,42 @@ func (n *nginxIngressControllerReconciler) collides(ctx context.Context, nic *ap } // updateStatus updates the status of the NginxIngressController resource. If a nil controller Deployment or IngressClass is passed, the status is defaulted for those fields if they are not already set. -func (n *nginxIngressControllerReconciler) updateStatus(ctx context.Context, nic *approutingv1alpha1.NginxIngressController, controllerDeployment *appsv1.Deployment, ic *netv1.IngressClass, managedResourceRefs []approutingv1alpha1.ManagedObjectReference, err error) { - lgr := log.FromContext(ctx) +func (n *nginxIngressControllerReconciler) updateStatus(nic *approutingv1alpha1.NginxIngressController, controllerDeployment *appsv1.Deployment, ic *netv1.IngressClass, managedResourceRefs []approutingv1alpha1.ManagedObjectReference, err error) { + n.updateStatusManagedResourceRefs(nic, managedResourceRefs) + + n.updateStatusIngressClass(nic, ic) - if managedResourceRefs != nil { - lgr.Info("updating managed resource refs status") - nic.Status.ManagedResourceRefs = managedResourceRefs + // default conditions + if controllerDeployment == nil || controllerDeployment.CreationTimestamp.IsZero() { + n.updateStatusNilDeployment(nic) + } else { + for _, cond := range controllerDeployment.Status.Conditions { + switch cond.Type { + case appsv1.DeploymentProgressing: + n.updateStatusControllerProgressing(nic, cond) + case appsv1.DeploymentAvailable: + n.updateStatusControllerAvailable(nic, cond) + } + } } + n.updateStatusControllerReplicas(nic, controllerDeployment) + n.updateStatusAvailable(nic) + + // error checking at end to take precedence over other conditions + n.updateStatusFromError(nic, err) +} + +func (n *nginxIngressControllerReconciler) updateStatusManagedResourceRefs(nic *approutingv1alpha1.NginxIngressController, managedResourceRefs []approutingv1alpha1.ManagedObjectReference) { + if managedResourceRefs == nil { + return + } + + nic.Status.ManagedResourceRefs = managedResourceRefs +} + +func (n *nginxIngressControllerReconciler) updateStatusIngressClass(nic *approutingv1alpha1.NginxIngressController, ic *netv1.IngressClass) { if ic == nil || ic.CreationTimestamp.IsZero() { - lgr.Info("adding IngressClassUnknown condition") nic.SetCondition(metav1.Condition{ Type: approutingv1alpha1.ConditionTypeIngressClassReady, Status: metav1.ConditionUnknown, @@ -338,7 +364,6 @@ func (n *nginxIngressControllerReconciler) updateStatus(ctx context.Context, nic Message: "IngressClass is unknown", }) } else { - lgr.Info("adding IngressClassIsReady condition") nic.SetCondition(metav1.Condition{ Type: approutingv1alpha1.ConditionTypeIngressClassReady, Status: "True", @@ -346,44 +371,38 @@ func (n *nginxIngressControllerReconciler) updateStatus(ctx context.Context, nic Message: "Ingress Class is up-to-date ", }) } +} - // default conditions - if controllerDeployment == nil || controllerDeployment.CreationTimestamp.IsZero() { - lgr.Info("adding ControllerDeploymentUnknown to ControllerAvailable condition") - nic.SetCondition(metav1.Condition{ - Type: approutingv1alpha1.ConditionTypeControllerAvailable, - Status: metav1.ConditionUnknown, - Reason: "ControllerDeploymentUnknown", - Message: "Controller deployment is unknown", - }) - lgr.Info("adding ControllerDeploymentUnknown to Progressing condition") - nic.SetCondition(metav1.Condition{ - Type: approutingv1alpha1.ConditionTypeProgressing, - Status: metav1.ConditionUnknown, - Reason: "ControllerDeploymentUnknown", - Message: "Controller deployment is unknown", - }) - } else { - lgr.Info("updating controller replicas status") - nic.Status.ControllerReadyReplicas = controllerDeployment.Status.ReadyReplicas - nic.Status.ControllerAvailableReplicas = controllerDeployment.Status.AvailableReplicas - nic.Status.ControllerUnavailableReplicas = controllerDeployment.Status.UnavailableReplicas - nic.Status.ControllerReplicas = controllerDeployment.Status.Replicas +func (n *nginxIngressControllerReconciler) updateStatusNilDeployment(nic *approutingv1alpha1.NginxIngressController) { + nic.SetCondition(metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeControllerAvailable, + Status: metav1.ConditionUnknown, + Reason: "ControllerDeploymentUnknown", + Message: "Controller deployment is unknown", + }) + nic.SetCondition(metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeProgressing, + Status: metav1.ConditionUnknown, + Reason: "ControllerDeploymentUnknown", + Message: "Controller deployment is unknown", + }) +} - for _, cond := range controllerDeployment.Status.Conditions { - switch cond.Type { - case appsv1.DeploymentProgressing: - n.updateStatusControllerProgressing(nic, cond) - case appsv1.DeploymentAvailable: - n.updateStatusControllerAvailable(nic, cond) - } - } +func (n *nginxIngressControllerReconciler) updateStatusControllerReplicas(nic *approutingv1alpha1.NginxIngressController, deployment *appsv1.Deployment) { + if deployment == nil { + return } + nic.Status.ControllerReadyReplicas = deployment.Status.ReadyReplicas + nic.Status.ControllerAvailableReplicas = deployment.Status.AvailableReplicas + nic.Status.ControllerUnavailableReplicas = deployment.Status.UnavailableReplicas + nic.Status.ControllerReplicas = deployment.Status.Replicas +} + +func (n *nginxIngressControllerReconciler) updateStatusAvailable(nic *approutingv1alpha1.NginxIngressController) { controllerAvailable := nic.GetCondition(approutingv1alpha1.ConditionTypeControllerAvailable) icAvailable := nic.GetCondition(approutingv1alpha1.ConditionTypeIngressClassReady) if controllerAvailable != nil && icAvailable != nil && controllerAvailable.Status == metav1.ConditionTrue && icAvailable.Status == metav1.ConditionTrue { - lgr.Info("adding ControllerIsAvailable condition") nic.SetCondition(metav1.Condition{ Type: approutingv1alpha1.ConditionTypeAvailable, Status: metav1.ConditionTrue, @@ -391,7 +410,6 @@ func (n *nginxIngressControllerReconciler) updateStatus(ctx context.Context, nic Message: "Controller Deployment has minimum availability and IngressClass is up-to-date", }) } else { - lgr.Info("adding ControllerIsNotAvailable condition") nic.SetCondition(metav1.Condition{ Type: approutingv1alpha1.ConditionTypeAvailable, Status: metav1.ConditionFalse, @@ -399,10 +417,10 @@ func (n *nginxIngressControllerReconciler) updateStatus(ctx context.Context, nic Message: "Controller Deployment does not have minimum availability or IngressClass is not up-to-date", }) } +} - // error checking at end to take precedence over other conditions +func (n *nginxIngressControllerReconciler) updateStatusFromError(nic *approutingv1alpha1.NginxIngressController, err error) { if errors.Is(err, icCollisionErr) { - lgr.Info("adding IngressClassCollision condition") nic.SetCondition(metav1.Condition{ Type: approutingv1alpha1.ConditionTypeProgressing, Status: metav1.ConditionFalse, @@ -412,7 +430,6 @@ func (n *nginxIngressControllerReconciler) updateStatus(ctx context.Context, nic n.events.Event(nic, corev1.EventTypeWarning, "IngressClassCollision", "IngressClass already exists and is not owned by this controller. Change the spec.IngressClassName to an unused IngressClass name in a new NginxIngressController.") } if errors.Is(err, maxCollisionsErr) { - lgr.Info("adding TooManyCollisions condition") nic.SetCondition(metav1.Condition{ Type: approutingv1alpha1.ConditionTypeProgressing, Status: metav1.ConditionFalse, diff --git a/pkg/controller/nginxingress/nginx_ingress_controller_test.go b/pkg/controller/nginxingress/nginx_ingress_controller_test.go new file mode 100644 index 00000000..9c42de5c --- /dev/null +++ b/pkg/controller/nginxingress/nginx_ingress_controller_test.go @@ -0,0 +1,455 @@ +package nginxingress + +import ( + "errors" + "fmt" + "testing" + + approutingv1alpha1 "github.com/Azure/aks-app-routing-operator/api/v1alpha1" + "github.com/stretchr/testify/require" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + netv1 "k8s.io/api/networking/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/tools/record" +) + +func TestUpdateStatusManagedResourceRefs(t *testing.T) { + t.Run("nil managed resource refs", func(t *testing.T) { + nic := &approutingv1alpha1.NginxIngressController{} + n := &nginxIngressControllerReconciler{} + n.updateStatusManagedResourceRefs(nic, nil) + + require.Nil(t, nic.Status.ManagedResourceRefs) + }) + + t.Run("managed resource refs", func(t *testing.T) { + nic := &approutingv1alpha1.NginxIngressController{} + n := &nginxIngressControllerReconciler{} + refs := []approutingv1alpha1.ManagedObjectReference{ + { + Name: "name", + Namespace: "namespace", + Kind: "kind", + APIGroup: "group", + }, + } + n.updateStatusManagedResourceRefs(nic, refs) + require.Equal(t, refs, nic.Status.ManagedResourceRefs) + + }) +} + +func TestUpdateStatusIngressClass(t *testing.T) { + t.Run("nil ingress class", func(t *testing.T) { + nic := &approutingv1alpha1.NginxIngressController{} + n := &nginxIngressControllerReconciler{} + n.updateStatusIngressClass(nic, nil) + + got := nic.GetCondition(approutingv1alpha1.ConditionTypeIngressClassReady) + require.NotNil(t, got) + require.Equal(t, metav1.ConditionUnknown, got.Status) + }) + + t.Run(("ingress clas with no creation timestamp"), func(t *testing.T) { + nic := &approutingv1alpha1.NginxIngressController{} + n := &nginxIngressControllerReconciler{} + n.updateStatusIngressClass(nic, &netv1.IngressClass{}) + + got := nic.GetCondition(approutingv1alpha1.ConditionTypeIngressClassReady) + require.NotNil(t, got) + require.Equal(t, metav1.ConditionUnknown, got.Status) + }) + + t.Run("ingress class with creation timestamp", func(t *testing.T) { + nic := &approutingv1alpha1.NginxIngressController{} + n := &nginxIngressControllerReconciler{} + n.updateStatusIngressClass(nic, &netv1.IngressClass{ + ObjectMeta: metav1.ObjectMeta{ + CreationTimestamp: metav1.Now(), + }, + Spec: netv1.IngressClassSpec{}, + }) + + got := nic.GetCondition(approutingv1alpha1.ConditionTypeIngressClassReady) + require.NotNil(t, got) + require.Equal(t, metav1.ConditionTrue, got.Status) + + }) +} + +func TestUpdateStatusNilDeployment(t *testing.T) { + nic := &approutingv1alpha1.NginxIngressController{} + n := &nginxIngressControllerReconciler{} + n.updateStatusNilDeployment(nic) + + controllerAvailable := nic.GetCondition(approutingv1alpha1.ConditionTypeControllerAvailable) + require.NotNil(t, controllerAvailable) + require.Equal(t, metav1.ConditionUnknown, controllerAvailable.Status) + + controllerProgressing := nic.GetCondition(approutingv1alpha1.ConditionTypeProgressing) + require.NotNil(t, controllerProgressing) + require.Equal(t, metav1.ConditionUnknown, controllerProgressing.Status) +} + +func TestUpdateStatusControllerReplicas(t *testing.T) { + t.Run("nil deployment", func(t *testing.T) { + nic := &approutingv1alpha1.NginxIngressController{} + n := &nginxIngressControllerReconciler{} + n.updateStatusControllerReplicas(nic, nil) + require.Equal(t, int32(0), nic.Status.ControllerReplicas) + require.Equal(t, int32(0), nic.Status.ControllerReadyReplicas) + require.Equal(t, int32(0), nic.Status.ControllerAvailableReplicas) + require.Equal(t, int32(0), nic.Status.ControllerUnavailableReplicas) + }) + + t.Run("deployment with replicas", func(t *testing.T) { + nic := &approutingv1alpha1.NginxIngressController{} + n := &nginxIngressControllerReconciler{} + deployment := &appsv1.Deployment{ + Status: appsv1.DeploymentStatus{ + Replicas: 1, + ReadyReplicas: 2, + AvailableReplicas: 1, + UnavailableReplicas: 5, + }, + } + n.updateStatusControllerReplicas(nic, deployment) + require.Equal(t, int32(1), nic.Status.ControllerReplicas) + require.Equal(t, int32(2), nic.Status.ControllerReadyReplicas) + require.Equal(t, int32(1), nic.Status.ControllerAvailableReplicas) + require.Equal(t, int32(5), nic.Status.ControllerUnavailableReplicas) + }) +} + +func TestUpdateStatusAvailable(t *testing.T) { + cases := []struct { + name string + controllerAvailable *metav1.Condition + icAvailable *metav1.Condition + expected *metav1.Condition + }{ + { + name: "controller available, ic available", + controllerAvailable: &metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeControllerAvailable, + Status: metav1.ConditionTrue, + }, + icAvailable: &metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeIngressClassReady, + Status: metav1.ConditionTrue, + }, + expected: &metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeAvailable, + Status: metav1.ConditionTrue, + }, + }, + { + name: "controller not available, ic available", + controllerAvailable: &metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeControllerAvailable, + Status: metav1.ConditionFalse, + }, + icAvailable: &metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeIngressClassReady, + Status: metav1.ConditionTrue, + }, + expected: &metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeAvailable, + Status: metav1.ConditionFalse, + }, + }, + { + name: "controller available, ic not available", + controllerAvailable: &metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeControllerAvailable, + Status: metav1.ConditionTrue, + }, + icAvailable: &metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeIngressClassReady, + Status: metav1.ConditionFalse, + }, + expected: &metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeAvailable, + Status: metav1.ConditionFalse, + }, + }, + { + name: "controller not available, ic not available", + controllerAvailable: &metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeControllerAvailable, + Status: metav1.ConditionFalse, + }, + icAvailable: &metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeIngressClassReady, + Status: metav1.ConditionFalse, + }, + expected: &metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeAvailable, + Status: metav1.ConditionFalse, + }, + }, + { + name: "nil controller condition, ic available", + controllerAvailable: nil, + icAvailable: &metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeIngressClassReady, + Status: metav1.ConditionTrue, + }, + expected: &metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeAvailable, + Status: metav1.ConditionFalse, + }, + }, + { + name: "nil ic condition, controller available", + controllerAvailable: &metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeControllerAvailable, + Status: metav1.ConditionTrue, + }, + icAvailable: nil, + expected: &metav1.Condition{ + Type: approutingv1alpha1.ConditionTypeAvailable, + Status: metav1.ConditionFalse, + }, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + nic := &approutingv1alpha1.NginxIngressController{ + Status: approutingv1alpha1.NginxIngressControllerStatus{ + Conditions: []metav1.Condition{}, + }, + } + + if c.controllerAvailable != nil { + nic.Status.Conditions = append(nic.Status.Conditions, *c.controllerAvailable) + } + if c.icAvailable != nil { + nic.Status.Conditions = append(nic.Status.Conditions, *c.icAvailable) + } + + n := &nginxIngressControllerReconciler{} + n.updateStatusAvailable(nic) + + got := nic.GetCondition(approutingv1alpha1.ConditionTypeAvailable) + if got == nil && c.expected == nil { + return + } + + require.Equal(t, c.expected.Status, got.Status) + }) + } +} + +func TestUpdateStatusFromError(t *testing.T) { + t.Run("nil error", func(t *testing.T) { + recorder := record.NewFakeRecorder(10) + n := &nginxIngressControllerReconciler{ + events: recorder, + } + nic := &approutingv1alpha1.NginxIngressController{} + n.updateStatusFromError(nic, nil) + require.True(t, len(nic.Status.Conditions) == 0) + select { + case <-recorder.Events: + require.Fail(t, "unexpected event") + default: + // no events, we're good + } + }) + + t.Run("non nil, non handled error", func(t *testing.T) { + recorder := record.NewFakeRecorder(10) + n := &nginxIngressControllerReconciler{ + events: recorder, + } + nic := &approutingv1alpha1.NginxIngressController{} + n.updateStatusFromError(nic, errors.New("test error")) + require.True(t, len(nic.Status.Conditions) == 0) + select { + case <-recorder.Events: + require.Fail(t, "unexpected event") + default: + // no events, we're good + } + }) + + t.Run("ingressClass collision error", func(t *testing.T) { + recorder := record.NewFakeRecorder(10) + n := &nginxIngressControllerReconciler{ + events: recorder, + } + nic := &approutingv1alpha1.NginxIngressController{} + nic.Generation = 1 + n.updateStatusFromError(nic, icCollisionErr) + got := nic.GetCondition(approutingv1alpha1.ConditionTypeProgressing) + require.True(t, got.Status == metav1.ConditionFalse) + require.True(t, got.ObservedGeneration == nic.Generation) + + event := <-recorder.Events + require.Equal(t, event, "Warning IngressClassCollision IngressClass already exists and is not owned by this controller. Change the spec.IngressClassName to an unused IngressClass name in a new NginxIngressController.") + }) + + t.Run("max collision error", func(t *testing.T) { + recorder := record.NewFakeRecorder(10) + n := &nginxIngressControllerReconciler{ + events: recorder, + } + nic := &approutingv1alpha1.NginxIngressController{} + nic.Generation = 1 + n.updateStatusFromError(nic, maxCollisionsErr) + got := nic.GetCondition(approutingv1alpha1.ConditionTypeProgressing) + require.True(t, got.Status == metav1.ConditionFalse) + require.True(t, got.ObservedGeneration == nic.Generation) + + event := <-recorder.Events + require.Equal(t, event, "Warning TooManyCollisions Too many collisions with existing resources. Change the spec.ControllerNamePrefix to something more unique in a new NginxIngressController.") + }) +} + +func TestUpdateStatusControllerAvailable(t *testing.T) { + t.Run("non deployment available condition", func(t *testing.T) { + n := &nginxIngressControllerReconciler{} + nic := &approutingv1alpha1.NginxIngressController{} + n.updateStatusControllerAvailable(nic, appsv1.DeploymentCondition{}) + require.True(t, len(nic.Status.Conditions) == 0) + }) + + t.Run("deployment available true condition", func(t *testing.T) { + n := &nginxIngressControllerReconciler{} + nic := &approutingv1alpha1.NginxIngressController{} + nic.Generation = 1 + n.updateStatusControllerAvailable(nic, appsv1.DeploymentCondition{ + Type: appsv1.DeploymentAvailable, + Status: corev1.ConditionTrue, + }) + + got := nic.GetCondition(approutingv1alpha1.ConditionTypeControllerAvailable) + require.True(t, got.Status == metav1.ConditionTrue) + require.True(t, got.ObservedGeneration == nic.Generation) + }) + + t.Run("deployment available false condition", func(t *testing.T) { + n := &nginxIngressControllerReconciler{} + nic := &approutingv1alpha1.NginxIngressController{} + nic.Generation = 2 + n.updateStatusControllerAvailable(nic, appsv1.DeploymentCondition{ + Type: appsv1.DeploymentAvailable, + Status: corev1.ConditionFalse, + }) + + got := nic.GetCondition(approutingv1alpha1.ConditionTypeControllerAvailable) + require.True(t, got.Status == metav1.ConditionFalse) + require.True(t, got.ObservedGeneration == nic.Generation) + }) + + t.Run("deployment available unknown condition", func(t *testing.T) { + n := &nginxIngressControllerReconciler{} + nic := &approutingv1alpha1.NginxIngressController{} + nic.Generation = 3 + n.updateStatusControllerAvailable(nic, appsv1.DeploymentCondition{ + Type: appsv1.DeploymentAvailable, + Status: corev1.ConditionUnknown, + }) + + got := nic.GetCondition(approutingv1alpha1.ConditionTypeControllerAvailable) + require.True(t, got.Status == metav1.ConditionUnknown) + require.True(t, got.ObservedGeneration == nic.Generation) + }) +} + +func TestUpdateStatusControllerProgressing(t *testing.T) { + t.Run("non deployment progressing condition", func(t *testing.T) { + n := &nginxIngressControllerReconciler{} + nic := &approutingv1alpha1.NginxIngressController{} + n.updateStatusControllerProgressing(nic, appsv1.DeploymentCondition{}) + require.True(t, len(nic.Status.Conditions) == 0) + }) + + t.Run("deployment progressing true condition", func(t *testing.T) { + n := &nginxIngressControllerReconciler{} + nic := &approutingv1alpha1.NginxIngressController{} + nic.Generation = 1 + n.updateStatusControllerProgressing(nic, appsv1.DeploymentCondition{ + Type: appsv1.DeploymentProgressing, + Status: corev1.ConditionTrue, + }) + + got := nic.GetCondition(approutingv1alpha1.ConditionTypeProgressing) + require.True(t, got.Status == metav1.ConditionTrue) + require.True(t, got.ObservedGeneration == nic.Generation) + }) + + t.Run("deployment progressing false condition", func(t *testing.T) { + n := &nginxIngressControllerReconciler{} + nic := &approutingv1alpha1.NginxIngressController{} + nic.Generation = 2 + n.updateStatusControllerProgressing(nic, appsv1.DeploymentCondition{ + Type: appsv1.DeploymentProgressing, + Status: corev1.ConditionFalse, + }) + + got := nic.GetCondition(approutingv1alpha1.ConditionTypeProgressing) + require.True(t, got.Status == metav1.ConditionFalse) + require.True(t, got.ObservedGeneration == nic.Generation) + }) + + t.Run("deployment progressing unknown condition", func(t *testing.T) { + n := &nginxIngressControllerReconciler{} + nic := &approutingv1alpha1.NginxIngressController{} + nic.Generation = 3 + n.updateStatusControllerProgressing(nic, appsv1.DeploymentCondition{ + Type: appsv1.DeploymentProgressing, + Status: corev1.ConditionUnknown, + }) + + got := nic.GetCondition(approutingv1alpha1.ConditionTypeProgressing) + require.True(t, got.Status == metav1.ConditionUnknown) + require.True(t, got.ObservedGeneration == nic.Generation) + }) +} + +func TestIsUnreconcilableError(t *testing.T) { + cases := []struct { + name string + err error + want bool + }{ + { + name: "nil error", + err: nil, + want: false, + }, + { + name: "non unreconcilable error", + err: errors.New("some error"), + want: false, + }, + { + name: "unreconcilable error", + err: icCollisionErr, + want: true, + }, + { + name: "another unreconcilable error", + err: maxCollisionsErr, + want: true, + }, + { + name: "wrapped unreconcilable error", + err: fmt.Errorf("wrapped: %w", icCollisionErr), + want: true, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + got := isUnreconcilableError(c.err) + if got != c.want { + t.Errorf("got %v, want %v", got, c.want) + } + }) + } +} diff --git a/pkg/manifests/common_test.go b/pkg/manifests/common_test.go index 53e58ad5..12a4a911 100644 --- a/pkg/manifests/common_test.go +++ b/pkg/manifests/common_test.go @@ -10,6 +10,9 @@ import ( "github.com/Azure/aks-app-routing-operator/pkg/util" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -58,6 +61,77 @@ func TestHasTopLevelLabels(t *testing.T) { } } +func TestGetOwnerRefs(t *testing.T) { + cases := []struct { + Name string + Owner client.Object + Controller bool + Outcome []metav1.OwnerReference + }{ + { + Name: "non-controller", + Owner: &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-namespace", + UID: "test-uid", + }, + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "Namespace", + }, + }, + Controller: false, + Outcome: []metav1.OwnerReference{ + { + APIVersion: "v1", + Kind: "Namespace", + Name: "test-namespace", + UID: "test-uid", + Controller: util.ToPtr(false), + }, + }, + }, + { + Name: "controller", + Owner: &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-deployment", + UID: "test-uid2", + }, + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "Deployment", + }, + }, + Controller: true, + Outcome: []metav1.OwnerReference{ + { + APIVersion: "v1", + Kind: "Deployment", + Name: "test-deployment", + UID: "test-uid2", + Controller: util.ToPtr(true), + }, + }, + }, + } + + for _, c := range cases { + t.Run(c.Name, func(t *testing.T) { + ret := GetOwnerRefs(c.Owner, c.Controller) + + require.Equal(t, len(c.Outcome), len(ret)) + for i, ref := range ret { + require.Equal(t, ref.APIVersion, c.Outcome[i].APIVersion) + require.Equal(t, ref.Kind, c.Outcome[i].Kind) + require.Equal(t, ref.Name, c.Outcome[i].Name) + require.Equal(t, ref.UID, c.Outcome[i].UID) + require.Equal(t, ref.Controller, c.Outcome[i].Controller) + } + }) + } +} + // AssertFixture checks the fixture path and compares it to the provided objects, failing if they are not equal func AssertFixture(t *testing.T, fixturePath string, objs []client.Object) { t.Logf("Testing fixture %s", fixturePath) From 0988d6d8555bc32171e7e29b85f76a483badd7dc Mon Sep 17 00:00:00 2001 From: Oliver King Date: Fri, 3 Nov 2023 14:35:26 -0500 Subject: [PATCH 6/6] add unit tests --- .../nginxingress/nginx_ingress_controller.go | 30 +- .../nginx_ingress_controller_test.go | 295 ++++++++++++++++++ 2 files changed, 306 insertions(+), 19 deletions(-) diff --git a/pkg/controller/nginxingress/nginx_ingress_controller.go b/pkg/controller/nginxingress/nginx_ingress_controller.go index 56ca7126..a5166ef4 100644 --- a/pkg/controller/nginxingress/nginx_ingress_controller.go +++ b/pkg/controller/nginxingress/nginx_ingress_controller.go @@ -17,7 +17,6 @@ import ( corev1 "k8s.io/api/core/v1" netv1 "k8s.io/api/networking/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/client-go/tools/record" @@ -132,7 +131,7 @@ func (n *nginxIngressControllerReconciler) Reconcile(ctx context.Context, req ct if collisionCountErr != nil { if isUnreconcilableError(collisionCountErr) { lgr.Info("unreconcilable collision count") - return ctrl.Result{RequeueAfter: time.Minute}, nil // requeue in case cx fix the unreconcilable reason + return ctrl.Result{RequeueAfter: time.Minute}, nil // requeue in case cx fixes the unreconcilable reason } lgr.Error(collisionCountErr, "unable to get determine collision count") @@ -162,12 +161,15 @@ func (n *nginxIngressControllerReconciler) ReconcileResource(ctx context.Context if nic == nil { return nil, errors.New("nginxIngressController cannot be nil") } + if res == nil { + return nil, errors.New("resources cannot be nil") + } lgr := log.FromContext(ctx) var managedResourceRefs []approutingv1alpha1.ManagedObjectReference for _, obj := range res.Objects() { - // TODO: upsert works pretty well but we want to set annotations exactly on the nginx service, we should use an alternative strategy for that + // TODO: upsert works fine for now but we want to set annotations exactly on the nginx service, we should use an alternative strategy for that in the future if err := util.Upsert(ctx, n.client, obj); err != nil { // publish an event so cx can see things like their policy is preventing us from creating a resource @@ -233,6 +235,12 @@ func (n *nginxIngressControllerReconciler) GetCollisionCount(ctx context.Context for { lgr = lgr.WithValues("collisionCount", nic.Status.CollisionCount) + + if nic.Status.CollisionCount == approutingv1alpha1.MaxCollisions { + lgr.Info("max collisions reached") + return 0, maxCollisionsErr + } + collision, err := n.collides(ctx, nic) if err != nil { lgr.Error(err, "unable to determine collision") @@ -243,28 +251,12 @@ func (n *nginxIngressControllerReconciler) GetCollisionCount(ctx context.Context // rare since our webhook guards against it, but it's possible lgr.Info("ingress class collision") return 0, icCollisionErr - - meta.SetStatusCondition(&nic.Status.Conditions, metav1.Condition{ - Type: approutingv1alpha1.ConditionTypeIngressClassReady, - Status: "Collision", - ObservedGeneration: nic.Generation, - Message: fmt.Sprintf("IngressClass %s already exists in the cluster and isn't owned by this resource. Delete the IngressClass or recreate this resource with a different spec.IngressClass field.", nic.Spec.IngressClassName), - Reason: "IngressClassCollision", - }) - if err := n.client.Status().Update(ctx, nic); err != nil { - lgr.Error(err, "unable to update status") - } } if collision == collisionNone { break } - if nic.Status.CollisionCount == approutingv1alpha1.MaxCollisions { - lgr.Info("max collisions reached") - return 0, maxCollisionsErr - } - lgr.Info("reconcilable collision detected, incrementing") nic.Status.CollisionCount++ } diff --git a/pkg/controller/nginxingress/nginx_ingress_controller_test.go b/pkg/controller/nginxingress/nginx_ingress_controller_test.go index 9c42de5c..44eaf76b 100644 --- a/pkg/controller/nginxingress/nginx_ingress_controller_test.go +++ b/pkg/controller/nginxingress/nginx_ingress_controller_test.go @@ -1,19 +1,314 @@ package nginxingress import ( + "context" "errors" "fmt" + "strings" "testing" approutingv1alpha1 "github.com/Azure/aks-app-routing-operator/api/v1alpha1" + "github.com/Azure/aks-app-routing-operator/pkg/config" + "github.com/Azure/aks-app-routing-operator/pkg/manifests" "github.com/stretchr/testify/require" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" netv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" ) +func TestReconcileResources(t *testing.T) { + t.Run("nil nic", func(t *testing.T) { + n := &nginxIngressControllerReconciler{} + _, err := n.ReconcileResource(context.Background(), nil, nil) + require.ErrorContains(t, err, "nginxIngressController cannot be nil") + }) + + t.Run("nil resources", func(t *testing.T) { + n := &nginxIngressControllerReconciler{} + _, err := n.ReconcileResource(context.Background(), &approutingv1alpha1.NginxIngressController{}, nil) + require.Error(t, err, "resources cannot be nil") + }) + + t.Run("valid resources", func(t *testing.T) { + cl := fake.NewFakeClient() + events := record.NewFakeRecorder(10) + n := &nginxIngressControllerReconciler{ + conf: &config.Config{ + NS: "default", + }, + client: cl, + events: events, + } + + nic := &approutingv1alpha1.NginxIngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nic", + }, + Spec: approutingv1alpha1.NginxIngressControllerSpec{ + IngressClassName: "ingressClassName", + ControllerNamePrefix: "prefix", + }, + } + res := n.ManagedResources(nic) + + managed, err := n.ReconcileResource(context.Background(), nic, res) + require.NoError(t, err) + require.True(t, len(managed) == len(res.Objects())-1, "expected all resources to be returned as managed except the namespace") + + // prove objects were created + for _, obj := range res.Objects() { + require.NoError(t, cl.Get(context.Background(), client.ObjectKeyFromObject(obj), obj)) + } + + // no events + select { + case <-events.Events: + require.Fail(t, "expected no events") + default: + } + }) + + t.Run("invalid resources", func(t *testing.T) { + cl := fake.NewFakeClient() + events := record.NewFakeRecorder(10) + n := &nginxIngressControllerReconciler{ + conf: &config.Config{ + NS: "otherNamespaceThatDoesntExistYet", + }, + client: cl, + events: events, + } + + nic := &approutingv1alpha1.NginxIngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nic", + }, + Spec: approutingv1alpha1.NginxIngressControllerSpec{ + IngressClassName: "ingressClassName", + ControllerNamePrefix: "prefix", + }, + } + res := n.ManagedResources(nic) + res.Deployment = &appsv1.Deployment{} // invalid deployment + + _, err := n.ReconcileResource(context.Background(), nic, res) + require.ErrorContains(t, err, "upserting object: ") + + // prove event was created + e := <-events.Events + require.Equal(t, "Warning EnsuringManagedResourcesFailed Failed to ensure managed resource /: \"\" is invalid: metadata.name: Required value: name is required", e) + }) +} + +func TestManagedResources(t *testing.T) { + t.Run("nil nic", func(t *testing.T) { + n := &nginxIngressControllerReconciler{} + require.Nil(t, n.ManagedResources(nil)) + }) + + t.Run("standard nic", func(t *testing.T) { + n := &nginxIngressControllerReconciler{ + conf: &config.Config{ + NS: "default", + }, + } + nic := &approutingv1alpha1.NginxIngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nic", + }, + Spec: approutingv1alpha1.NginxIngressControllerSpec{ + IngressClassName: "ingressClassName", + ControllerNamePrefix: "prefix", + }, + } + + resources := n.ManagedResources(nic) + require.NotNil(t, resources) + require.Equal(t, nic.Spec.IngressClassName, resources.IngressClass.Name) + require.True(t, strings.HasPrefix(resources.Deployment.Name, nic.Spec.ControllerNamePrefix)) + + // check that we are only putting owner references on managed resources + ownerRef := manifests.GetOwnerRefs(nic, true) + require.Equal(t, ownerRef, resources.Deployment.OwnerReferences) + require.NotEqual(t, ownerRef, resources.Namespace.OwnerReferences) + }) + + t.Run("nic with load balancer annotations", func(t *testing.T) { + n := &nginxIngressControllerReconciler{ + conf: &config.Config{ + NS: "default", + }, + } + nic := &approutingv1alpha1.NginxIngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nic", + }, + Spec: approutingv1alpha1.NginxIngressControllerSpec{ + IngressClassName: "ingressClassName", + ControllerNamePrefix: "prefix", + LoadBalancerAnnotations: map[string]string{ + "foo": "bar", + }, + }, + } + + resources := n.ManagedResources(nic) + require.NotNil(t, resources) + require.Equal(t, nic.Spec.IngressClassName, resources.IngressClass.Name) + require.True(t, strings.HasPrefix(resources.Deployment.Name, nic.Spec.ControllerNamePrefix)) + + // check that we are only putting owner references on managed resources + ownerRef := manifests.GetOwnerRefs(nic, true) + require.Equal(t, ownerRef, resources.Deployment.OwnerReferences) + require.NotEqual(t, ownerRef, resources.Namespace.OwnerReferences) + + // verify load balancer annotations + for k, v := range nic.Spec.LoadBalancerAnnotations { + require.Equal(t, v, resources.Service.Annotations[k]) + } + }) +} + +func TestGetCollisionCount(t *testing.T) { + ctx := context.Background() + cl := fake.NewClientBuilder().Build() + + n := &nginxIngressControllerReconciler{ + client: cl, + conf: &config.Config{ + NS: "default", + }, + } + + // standard collisions + nic := &approutingv1alpha1.NginxIngressController{ + Spec: approutingv1alpha1.NginxIngressControllerSpec{ + ControllerNamePrefix: "sameControllerNamePrefixForAll", + }, + } + for i := 0; i <= approutingv1alpha1.MaxCollisions; i++ { + copy := nic.DeepCopy() + copy.Name = fmt.Sprintf("nic%d", i) + copy.Spec.IngressClassName = fmt.Sprintf("ingressClassName%d", i) + + count, err := n.GetCollisionCount(ctx, copy) + + if i == approutingv1alpha1.MaxCollisions { + require.Equal(t, maxCollisionsErr, err) + continue + } + + require.NoError(t, err) + require.Equal(t, int32(i), count) + + copy.Status.CollisionCount = count + for _, obj := range n.ManagedResources(copy).Objects() { + cl.Create(ctx, obj) + } + } + + // ic collision + collisionIc := &netv1.IngressClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: "collisionIngressClassName", + }, + } + require.NoError(t, cl.Create(ctx, collisionIc)) + nic2 := &approutingv1alpha1.NginxIngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "collisionNic", + }, + Spec: approutingv1alpha1.NginxIngressControllerSpec{ + IngressClassName: collisionIc.Name, + ControllerNamePrefix: "controllerNameUnique", + }, + } + _, err := n.GetCollisionCount(ctx, nic2) + require.Equal(t, icCollisionErr, err) +} + +func TestCollides(t *testing.T) { + ctx := context.Background() + cl := fake.NewClientBuilder().Build() + + collisionNic := &approutingv1alpha1.NginxIngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "collision", + }, + Spec: approutingv1alpha1.NginxIngressControllerSpec{ + IngressClassName: "ingressClassName", + ControllerNamePrefix: "controllerName", + }, + } + + n := &nginxIngressControllerReconciler{ + client: cl, + conf: &config.Config{NS: "default"}, + } + for _, obj := range n.ManagedResources(collisionNic).Objects() { + cl.Create(ctx, obj) + } + + // ic collision + collisionIc := &netv1.IngressClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: "collisionIngressClassName", + }, + } + require.NoError(t, cl.Create(ctx, collisionIc)) + nic := &approutingv1alpha1.NginxIngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nic1", + }, + Spec: approutingv1alpha1.NginxIngressControllerSpec{ + IngressClassName: collisionIc.Name, + ControllerNamePrefix: "controllerName1", + }, + } + got, err := n.collides(ctx, nic) + require.NoError(t, err) + require.Equal(t, collisionIngressClass, got) + + // non ic collision + nic2 := &approutingv1alpha1.NginxIngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nic12", + }, + Spec: collisionNic.Spec, + } + nic2.Spec.IngressClassName = "anotherIngressClassName" + got, err = n.collides(ctx, nic2) + require.NoError(t, err) + require.Equal(t, collisionOther, got) + + // no collision + nic3 := &approutingv1alpha1.NginxIngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nic3", + }, + Spec: approutingv1alpha1.NginxIngressControllerSpec{ + IngressClassName: "anotherIngressClassName", + ControllerNamePrefix: "controllerName3", + }, + } + got, err = n.collides(ctx, nic3) + require.NoError(t, err) + require.Equal(t, collisionNone, got) + + // prove that we check ownership references for collisions and there will be no collision if ownership references match + for _, obj := range n.ManagedResources(collisionNic).Objects() { + cl.Create(ctx, obj) + } + + got, err = n.collides(ctx, nic3) + require.NoError(t, err) + require.Equal(t, collisionNone, got) +} + func TestUpdateStatusManagedResourceRefs(t *testing.T) { t.Run("nil managed resource refs", func(t *testing.T) { nic := &approutingv1alpha1.NginxIngressController{}