From 78c5f917b8c3e235a0ef22461403bf980ae652c0 Mon Sep 17 00:00:00 2001 From: Mikhail Berezovskiy Date: Thu, 7 Dec 2023 15:23:12 -0500 Subject: [PATCH 1/2] update validations and status on route reconciler --- pkg/controllers/gateway_controller.go | 2 +- pkg/controllers/route_controller.go | 261 ++++++++++++++---- pkg/controllers/route_controller_test.go | 6 +- pkg/deploy/lattice/service_network_manager.go | 3 +- pkg/model/core/grpcroute.go | 7 + pkg/model/core/httproute.go | 7 + pkg/model/core/route.go | 1 + 7 files changed, 237 insertions(+), 50 deletions(-) diff --git a/pkg/controllers/gateway_controller.go b/pkg/controllers/gateway_controller.go index 505d5e3e..73af335e 100644 --- a/pkg/controllers/gateway_controller.go +++ b/pkg/controllers/gateway_controller.go @@ -191,7 +191,7 @@ func (r *gatewayReconciler) reconcileDelete(ctx context.Context, gw *gwv1beta1.G } if httpGw.Name == gw.Name && httpGw.Namespace == gw.Namespace { - return fmt.Errorf("Cannot delete gateway %s/%s - found referencing route %s/%s", + return fmt.Errorf("cannot delete gateway %s/%s - found referencing route %s/%s", gw.Namespace, gw.Name, route.Namespace(), route.Name()) } } diff --git a/pkg/controllers/route_controller.go b/pkg/controllers/route_controller.go index 4a52af6e..d43abb42 100644 --- a/pkg/controllers/route_controller.go +++ b/pkg/controllers/route_controller.go @@ -20,30 +20,26 @@ import ( "context" "fmt" - "github.com/aws/aws-application-networking-k8s/pkg/aws/services" - "github.com/aws/aws-application-networking-k8s/pkg/utils" - "github.com/aws/aws-application-networking-k8s/pkg/utils/gwlog" - "github.com/pkg/errors" - corev1 "k8s.io/api/core/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/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" - gwv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" - gwv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1" - - "sigs.k8s.io/external-dns/endpoint" - "sigs.k8s.io/controller-runtime/pkg/builder" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/predicate" + "sigs.k8s.io/external-dns/endpoint" gwv1 "sigs.k8s.io/gateway-api/apis/v1" + gwv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" + gwv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1" anv1alpha1 "github.com/aws/aws-application-networking-k8s/pkg/apis/applicationnetworking/v1alpha1" "github.com/aws/aws-application-networking-k8s/pkg/aws" + "github.com/aws/aws-application-networking-k8s/pkg/aws/services" "github.com/aws/aws-application-networking-k8s/pkg/config" "github.com/aws/aws-application-networking-k8s/pkg/controllers/eventhandlers" "github.com/aws/aws-application-networking-k8s/pkg/deploy" @@ -52,6 +48,9 @@ import ( "github.com/aws/aws-application-networking-k8s/pkg/k8s" "github.com/aws/aws-application-networking-k8s/pkg/model/core" lattice_runtime "github.com/aws/aws-application-networking-k8s/pkg/runtime" + "github.com/aws/aws-application-networking-k8s/pkg/utils" + k8sutils "github.com/aws/aws-application-networking-k8s/pkg/utils" + "github.com/aws/aws-application-networking-k8s/pkg/utils/gwlog" ) var routeTypeToFinalizer = map[core.RouteType]string{ @@ -152,6 +151,10 @@ func RegisterAllRouteControllers( func (r *routeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { r.log.Infow("reconcile", "name", req.Name) recErr := r.reconcile(ctx, req) + if errors.Is(recErr, ErrValidation) { + r.log.Infof("non-retryable error: %s", recErr) + return ctrl.Result{}, nil // validation errors are not retryable, operator need to update spec + } if recErr != nil { r.log.Infow("reconcile error", "name", req.Name, "message", recErr.Error()) } @@ -317,6 +320,10 @@ func (r *routeReconciler) reconcileUpsert(ctx context.Context, req ctrl.Request, r.eventRecorder.Event(route.K8sObject(), corev1.EventTypeWarning, k8s.RouteEventReasonFailedAddFinalizer, fmt.Sprintf("Failed add finalizer due to %s", err)) } + if err := r.validateRoute(ctx, route); err != nil { + return fmt.Errorf("upsert: %w", err) + } + backendRefIPFamiliesErr := r.validateBackendRefsIpFamilies(ctx, route) if backendRefIPFamiliesErr != nil { @@ -361,7 +368,7 @@ func (r *routeReconciler) reconcileUpsert(ctx context.Context, req ctrl.Request, r.eventRecorder.Event(route.K8sObject(), corev1.EventTypeNormal, k8s.RouteEventReasonDeploySucceed, "Adding/Updating reconcile Done!") - svcName := utils.LatticeServiceName(route.Name(), route.Namespace()) + svcName := k8sutils.LatticeServiceName(route.Name(), route.Namespace()) svc, err := r.cloud.Lattice().FindService(ctx, svcName) if err != nil && !services.IsNotFoundError(err) { return err @@ -372,7 +379,7 @@ func (r *routeReconciler) reconcileUpsert(ctx context.Context, req ctrl.Request, return errors.New(lattice.LATTICE_RETRY) } - if err := r.updateRouteStatus(ctx, *svc.DnsEntry.DomainName, route); err != nil { + if err := r.updateRouteAnnotation(ctx, *svc.DnsEntry.DomainName, route); err != nil { return err } @@ -380,7 +387,7 @@ func (r *routeReconciler) reconcileUpsert(ctx context.Context, req ctrl.Request, return nil } -func (r *routeReconciler) updateRouteStatus(ctx context.Context, dns string, route core.Route) error { +func (r *routeReconciler) updateRouteAnnotation(ctx context.Context, dns string, route core.Route) error { r.log.Debugf("Updating route %s-%s with DNS %s", route.Name(), route.Namespace(), dns) routeOld := route.DeepCopy() @@ -392,39 +399,6 @@ func (r *routeReconciler) updateRouteStatus(ctx context.Context, dns string, rou if err := r.client.Patch(ctx, route.K8sObject(), client.MergeFrom(routeOld.K8sObject())); err != nil { return fmt.Errorf("failed to update route status due to err %w", err) } - routeOld = route.DeepCopy() - - route.Status().UpdateParentRefs(route.Spec().ParentRefs()[0], config.LatticeGatewayControllerName) - - // Update listener Status - if err := updateRouteListenerStatus(ctx, r.client, route); err != nil { - route.Status().UpdateRouteCondition(metav1.Condition{ - Type: string(gwv1beta1.RouteConditionAccepted), - Status: metav1.ConditionFalse, - ObservedGeneration: route.K8sObject().GetGeneration(), - Reason: string(gwv1.RouteReasonNoMatchingParent), - Message: fmt.Sprintf("Could not match gateway %s: %s", route.Spec().ParentRefs()[0].Name, err), - }) - } else { - route.Status().UpdateRouteCondition(metav1.Condition{ - Type: string(gwv1beta1.RouteConditionAccepted), - Status: metav1.ConditionTrue, - ObservedGeneration: route.K8sObject().GetGeneration(), - Reason: string(gwv1beta1.RouteReasonAccepted), - Message: fmt.Sprintf("DNS Name: %s", dns), - }) - route.Status().UpdateRouteCondition(metav1.Condition{ - Type: string(gwv1beta1.RouteConditionResolvedRefs), - Status: metav1.ConditionTrue, - ObservedGeneration: route.K8sObject().GetGeneration(), - Reason: string(gwv1beta1.RouteReasonResolvedRefs), - Message: fmt.Sprintf("DNS Name: %s", dns), - }) - } - - if err := r.client.Status().Patch(ctx, route.K8sObject(), client.MergeFrom(routeOld.K8sObject())); err != nil { - return fmt.Errorf("failed to update route status due to err %w", err) - } r.log.Debugf("Successfully updated route %s-%s with DNS %s", route.Name(), route.Namespace(), dns) return nil @@ -456,3 +430,196 @@ func (r *routeReconciler) validateBackendRefsIpFamilies(ctx context.Context, rou return nil } + +var ( + ErrValidation = errors.New("validation") + ErrParentRefsNotFound = errors.New("parentRefs are not found") + ErrRouteGKNotSupported = errors.New("route GroupKind is not supported") +) + +// Validation for route spec. Will validate and update route status. Returns error if not valid. +// Validation rules are suppose to be compliant to Gateway API Spec. +// +// https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io%2fv1.RouteConditionType +// +// There are 3 condition types: Accepted, PartiallyInvalid, ResolvedRefs. +// We dont support PartiallyInvalid for now and reject entire route if there is at least one invalid field. +// Accepted type is related to parentRefs, and ResolvedRefs to backendRefs. These 2 are validated independently. +func (r *routeReconciler) validateRoute(ctx context.Context, route core.Route) error { + ps, err := r.validateRouteParentRefs(ctx, route) + if err != nil { + return err + } + + cnd, err := r.validateBackedRefs(ctx, route) + if err != nil { + return err + } + + // we need to update each parentRef with backendRef status + psbr := make([]gwv1.RouteParentStatus, len(ps)) + for i, rps := range ps { + meta.SetStatusCondition(&rps.Conditions, cnd) + psbr[i] = rps + } + + route.Status().SetParents(psbr) + + err = r.client.Status().Update(ctx, route.K8sObject()) + if err != nil { + return fmt.Errorf("validate route: %w", err) + } + + if r.hasNotAcceptedCondition(route) { + return fmt.Errorf("%w: route has validation errors, see status", ErrValidation) + } + + return nil +} + +// checks if route has at least single condition with status = false +func (r *routeReconciler) hasNotAcceptedCondition(route core.Route) bool { + rps := route.Status().Parents() + for _, ps := range rps { + for _, cnd := range ps.Conditions { + if cnd.Status != metav1.ConditionTrue { + return true + } + } + } + return false +} + +// find Gateway by Route and parentRef, returns nil if not found +func (r *routeReconciler) findRouteParentGw(ctx context.Context, route core.Route, parentRef gwv1beta1.ParentReference) (*gwv1beta1.Gateway, error) { + ns := route.Namespace() + if parentRef.Namespace != nil && *parentRef.Namespace != "" { + ns = string(*parentRef.Namespace) + } + gwName := types.NamespacedName{ + Namespace: ns, + Name: string(parentRef.Name), + } + gw := &gwv1beta1.Gateway{} + err := r.client.Get(ctx, gwName, gw) + if err != nil { + return nil, client.IgnoreNotFound(err) + } + return gw, nil +} + +// Validation rules for route parentRefs +// +// Will ignore status update when: +// - parentRef does not exists, includes when parentRef Kind is not Gateway +// +// If parent GW exists will check: +// - NoMatchingParent: parentRef sectionName and port matches Listener name and port +// - TODO: NoMatchingListenerHostname: listener hostname matches one of route hostnames +// - TODO: NotAllowedByListeners: listener allowedRoutes contains route GroupKind +func (r *routeReconciler) validateRouteParentRefs(ctx context.Context, route core.Route) ([]gwv1beta1.RouteParentStatus, error) { + if len(route.Spec().ParentRefs()) == 0 { + return nil, ErrParentRefsNotFound + } + + parentStatuses := []gwv1beta1.RouteParentStatus{} + for _, parentRef := range route.Spec().ParentRefs() { + gw, err := r.findRouteParentGw(ctx, route, parentRef) + if err != nil { + return nil, err + } + if gw == nil { + continue // ignore status update if gw not found + } + + noMatchingParent := true + for _, listener := range gw.Spec.Listeners { + if parentRef.Port != nil && *parentRef.Port != listener.Port { + continue + } + if parentRef.SectionName != nil && *parentRef.SectionName != listener.Name { + continue + } + noMatchingParent = false + } + + parentStatus := gwv1beta1.RouteParentStatus{ + ParentRef: parentRef, + ControllerName: "application-networking.k8s.aws/gateway-api-controller", + Conditions: []metav1.Condition{}, + } + + var cnd metav1.Condition + switch { + case noMatchingParent: + cnd = r.newCondition(route, gwv1beta1.RouteConditionAccepted, gwv1.RouteReasonNoMatchingParent, "") + default: + cnd = r.newCondition(route, gwv1beta1.RouteConditionAccepted, gwv1beta1.RouteReasonAccepted, "") + } + meta.SetStatusCondition(&parentStatus.Conditions, cnd) + parentStatuses = append(parentStatuses, parentStatus) + } + + return parentStatuses, nil +} + +// set of valid Kinds for Route Backend References +var validBackendKinds = utils.NewSet("Service", "ServiceImport") + +// validate route's backed references, will return non-accepted +// condition if at least one backendRef not in a valid state +func (r *routeReconciler) validateBackedRefs(ctx context.Context, route core.Route) (metav1.Condition, error) { + var empty metav1.Condition + for _, rule := range route.Spec().Rules() { + for _, ref := range rule.BackendRefs() { + kind := "Service" + if ref.Kind() != nil { + kind = string(*ref.Kind()) + } + if !validBackendKinds.Contains(kind) { + return r.newCondition(route, gwv1beta1.RouteConditionResolvedRefs, gwv1beta1.RouteReasonInvalidKind, kind), nil + } + + namespace := route.Namespace() + if ref.Namespace() != nil { + namespace = string(*ref.Namespace()) + } + objKey := types.NamespacedName{ + Namespace: namespace, + Name: string(ref.Name()), + } + var obj client.Object + + switch kind { + case "Service": + obj = &corev1.Service{} + case "ServiceImport": + obj = &anv1alpha1.ServiceImport{} + default: + return empty, fmt.Errorf("invalid backed end ref kind, must be validated before, kind=%s", kind) + } + err := r.client.Get(ctx, objKey, obj) + if err != nil { + if apierrors.IsNotFound(err) { + msg := fmt.Sprintf("backendRef name: %s", ref.Name()) + return r.newCondition(route, gwv1beta1.RouteConditionResolvedRefs, gwv1beta1.RouteReasonBackendNotFound, msg), nil + } + } + } + } + return r.newCondition(route, gwv1beta1.RouteConditionResolvedRefs, gwv1beta1.RouteReasonResolvedRefs, ""), nil +} + +func (r *routeReconciler) newCondition(route core.Route, t gwv1beta1.RouteConditionType, reason gwv1beta1.RouteConditionReason, msg string) metav1.Condition { + status := metav1.ConditionTrue + if reason != gwv1beta1.RouteReasonAccepted && reason != gwv1beta1.RouteReasonResolvedRefs { + status = metav1.ConditionFalse + } + return metav1.Condition{ + Type: string(t), + Status: status, + ObservedGeneration: route.K8sObject().GetGeneration(), + Reason: string(reason), + Message: msg, + } +} diff --git a/pkg/controllers/route_controller_test.go b/pkg/controllers/route_controller_test.go index 209c2268..bf4ab52a 100644 --- a/pkg/controllers/route_controller_test.go +++ b/pkg/controllers/route_controller_test.go @@ -42,7 +42,11 @@ func TestRouteReconciler_ReconcileCreates(t *testing.T) { gwv1beta1.AddToScheme(k8sScheme) addOptionalCRDs(k8sScheme) - k8sClient := testclient.NewClientBuilder().WithScheme(k8sScheme).Build() + k8sClient := testclient. + NewClientBuilder(). + WithScheme(k8sScheme). + WithStatusSubresource(&gwv1beta1.HTTPRoute{}). + Build() gwClass := &gwv1beta1.GatewayClass{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/deploy/lattice/service_network_manager.go b/pkg/deploy/lattice/service_network_manager.go index ff74093a..8930544b 100644 --- a/pkg/deploy/lattice/service_network_manager.go +++ b/pkg/deploy/lattice/service_network_manager.go @@ -193,7 +193,8 @@ func (m *defaultServiceNetworkManager) CreateOrUpdate(ctx context.Context, servi return model.ServiceNetworkStatus{}, err } if snva != nil { - m.log.Debugf("ServiceNetwork %s already has VPC association %s", serviceNetwork.Spec.Name, snva.Arn) + m.log.Debugf("ServiceNetwork %s already has VPC association %s", + serviceNetwork.Spec.Name, aws.StringValue(snva.Arn)) return model.ServiceNetworkStatus{ServiceNetworkARN: serviceNetworkArn, ServiceNetworkID: serviceNetworkId}, nil } } diff --git a/pkg/model/core/grpcroute.go b/pkg/model/core/grpcroute.go index 25fb169a..9710223e 100644 --- a/pkg/model/core/grpcroute.go +++ b/pkg/model/core/grpcroute.go @@ -79,6 +79,13 @@ func (r *GRPCRoute) Inner() *gwv1alpha2.GRPCRoute { return &r.r } +func (r *GRPCRoute) GK() metav1.GroupKind { + return metav1.GroupKind{ + Group: gwv1beta1.GroupName, + Kind: "GRPCRoute", + } +} + type GRPCRouteSpec struct { s gwv1alpha2.GRPCRouteSpec } diff --git a/pkg/model/core/httproute.go b/pkg/model/core/httproute.go index 723be086..5c1439ab 100644 --- a/pkg/model/core/httproute.go +++ b/pkg/model/core/httproute.go @@ -78,6 +78,13 @@ func (r *HTTPRoute) Inner() *gwv1beta1.HTTPRoute { return &r.r } +func (r *HTTPRoute) GK() metav1.GroupKind { + return metav1.GroupKind{ + Group: gwv1beta1.GroupName, + Kind: "HTTPRoute", + } +} + type HTTPRouteSpec struct { s gwv1beta1.HTTPRouteSpec } diff --git a/pkg/model/core/route.go b/pkg/model/core/route.go index 95814aaf..155d5db8 100644 --- a/pkg/model/core/route.go +++ b/pkg/model/core/route.go @@ -22,6 +22,7 @@ type Route interface { DeletionTimestamp() *metav1.Time DeepCopy() Route K8sObject() client.Object + GK() metav1.GroupKind } func NewRoute(object client.Object) (Route, error) { From 853fb38c6487a2b30c4a9dc1e7842feb16850984 Mon Sep 17 00:00:00 2001 From: Mikhail Berezovskiy Date: Thu, 7 Dec 2023 17:13:25 -0500 Subject: [PATCH 2/2] ignore validation error in route reconciler, minor PR comments --- pkg/controllers/route_controller.go | 24 ++++++++++++------------ pkg/model/core/grpcroute.go | 2 +- pkg/model/core/httproute.go | 2 +- pkg/model/core/route.go | 2 +- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/pkg/controllers/route_controller.go b/pkg/controllers/route_controller.go index d43abb42..af0ef1b9 100644 --- a/pkg/controllers/route_controller.go +++ b/pkg/controllers/route_controller.go @@ -151,10 +151,6 @@ func RegisterAllRouteControllers( func (r *routeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { r.log.Infow("reconcile", "name", req.Name) recErr := r.reconcile(ctx, req) - if errors.Is(recErr, ErrValidation) { - r.log.Infof("non-retryable error: %s", recErr) - return ctrl.Result{}, nil // validation errors are not retryable, operator need to update spec - } if recErr != nil { r.log.Infow("reconcile error", "name", req.Name, "message", recErr.Error()) } @@ -321,7 +317,11 @@ func (r *routeReconciler) reconcileUpsert(ctx context.Context, req ctrl.Request, } if err := r.validateRoute(ctx, route); err != nil { - return fmt.Errorf("upsert: %w", err) + // TODO: we suppose to stop reconciliation here, but that will create problem when + // we delete Service and we suppose to delete TargetGroup, this validation will + // throw error if Service is not found. For now just update route status and log + // error. + r.log.Infof("route: %s: %s", route.Name(), err) } backendRefIPFamiliesErr := r.validateBackendRefsIpFamilies(ctx, route) @@ -446,24 +446,24 @@ var ( // We dont support PartiallyInvalid for now and reject entire route if there is at least one invalid field. // Accepted type is related to parentRefs, and ResolvedRefs to backendRefs. These 2 are validated independently. func (r *routeReconciler) validateRoute(ctx context.Context, route core.Route) error { - ps, err := r.validateRouteParentRefs(ctx, route) + parentRefsAccepted, err := r.validateRouteParentRefs(ctx, route) if err != nil { return err } - cnd, err := r.validateBackedRefs(ctx, route) + resolvedRefsCnd, err := r.validateBackedRefs(ctx, route) if err != nil { return err } // we need to update each parentRef with backendRef status - psbr := make([]gwv1.RouteParentStatus, len(ps)) - for i, rps := range ps { - meta.SetStatusCondition(&rps.Conditions, cnd) - psbr[i] = rps + parentRefsAcceptedResolvedRefs := make([]gwv1.RouteParentStatus, len(parentRefsAccepted)) + for i, rps := range parentRefsAccepted { + meta.SetStatusCondition(&rps.Conditions, resolvedRefsCnd) + parentRefsAcceptedResolvedRefs[i] = rps } - route.Status().SetParents(psbr) + route.Status().SetParents(parentRefsAcceptedResolvedRefs) err = r.client.Status().Update(ctx, route.K8sObject()) if err != nil { diff --git a/pkg/model/core/grpcroute.go b/pkg/model/core/grpcroute.go index 9710223e..8d76a879 100644 --- a/pkg/model/core/grpcroute.go +++ b/pkg/model/core/grpcroute.go @@ -79,7 +79,7 @@ func (r *GRPCRoute) Inner() *gwv1alpha2.GRPCRoute { return &r.r } -func (r *GRPCRoute) GK() metav1.GroupKind { +func (r *GRPCRoute) GroupKind() metav1.GroupKind { return metav1.GroupKind{ Group: gwv1beta1.GroupName, Kind: "GRPCRoute", diff --git a/pkg/model/core/httproute.go b/pkg/model/core/httproute.go index 5c1439ab..397fdb65 100644 --- a/pkg/model/core/httproute.go +++ b/pkg/model/core/httproute.go @@ -78,7 +78,7 @@ func (r *HTTPRoute) Inner() *gwv1beta1.HTTPRoute { return &r.r } -func (r *HTTPRoute) GK() metav1.GroupKind { +func (r *HTTPRoute) GroupKind() metav1.GroupKind { return metav1.GroupKind{ Group: gwv1beta1.GroupName, Kind: "HTTPRoute", diff --git a/pkg/model/core/route.go b/pkg/model/core/route.go index 155d5db8..7ae6d711 100644 --- a/pkg/model/core/route.go +++ b/pkg/model/core/route.go @@ -22,7 +22,7 @@ type Route interface { DeletionTimestamp() *metav1.Time DeepCopy() Route K8sObject() client.Object - GK() metav1.GroupKind + GroupKind() metav1.GroupKind } func NewRoute(object client.Object) (Route, error) {