diff --git a/pkg/reconciler/route/reconcile_resources.go b/pkg/reconciler/route/reconcile_resources.go index 8cb6e8067273..185816588058 100644 --- a/pkg/reconciler/route/reconcile_resources.go +++ b/pkg/reconciler/route/reconcile_resources.go @@ -20,6 +20,7 @@ import ( "context" "encoding/json" "fmt" + "sort" "time" "github.com/google/go-cmp/cmp" @@ -43,69 +44,146 @@ import ( v1 "knative.dev/serving/pkg/apis/serving/v1" "knative.dev/serving/pkg/reconciler/route/config" "knative.dev/serving/pkg/reconciler/route/resources" - "knative.dev/serving/pkg/reconciler/route/resources/names" + resourcenames "knative.dev/serving/pkg/reconciler/route/resources/names" "knative.dev/serving/pkg/reconciler/route/traffic" ) -func (c *Reconciler) reconcileIngress( +func (c *Reconciler) reconcileIngresses( ctx context.Context, r *v1.Route, tc *traffic.Config, tls []netv1alpha1.IngressTLS, ingressClass string, acmeChallenges ...netv1alpha1.HTTP01Challenge, -) (*netv1alpha1.Ingress, *traffic.Rollout, error) { +) ([]*netv1alpha1.Ingress, *traffic.Rollout, error) { recorder := controller.GetEventRecorder(ctx) - var effectiveRO *traffic.Rollout - ingress, err := c.ingressLister.Ingresses(r.Namespace).Get(names.Ingress(r)) - if apierrs.IsNotFound(err) { - desired, err := resources.MakeIngress(ctx, r, tc, tls, ingressClass, acmeChallenges...) - if err != nil { - return nil, nil, err + // Phase 1: Build the default ingress with rollout. + desiredNames := resources.DesiredIngressNames(r, tc) + + // Collect existing ingresses and check readiness. + existingIngresses := map[string]*netv1alpha1.Ingress{} + allExistingReady := true + hasExisting := false + + for name := range desiredNames { + existing, err := c.ingressLister.Ingresses(r.Namespace).Get(name) + if apierrs.IsNotFound(err) { + allExistingReady = false + continue + } else if err != nil { + return nil, nil, fmt.Errorf("failed to get Ingress %q: %w", name, err) } - ingress, err = c.netclient.NetworkingV1alpha1().Ingresses(desired.Namespace).Create(ctx, desired, metav1.CreateOptions{}) - if err != nil { - recorder.Eventf(r, corev1.EventTypeWarning, "CreationFailed", "Failed to create Ingress: %v", err) - return nil, nil, fmt.Errorf("failed to create Ingress: %w", err) + hasExisting = true + existingIngresses[name] = existing + if !existing.IsReady() { + allExistingReady = false } + } - recorder.Eventf(r, corev1.EventTypeNormal, "Created", "Created Ingress %q", ingress.GetName()) - return ingress, tc.BuildRollout(), nil - } else if err != nil { - return nil, nil, err + // Read previous rollout only from the default ingress. + // Rollout state is a concern of the default ingress only. + defaultName := resourcenames.TaggedIngress(r, traffic.DefaultTarget) + var prevRO *traffic.Rollout + if existingDefault, ok := existingIngresses[defaultName]; ok { + prevRO = deserializeRollout(ctx, existingDefault.Annotations[networking.RolloutAnnotationKey]) + } + + // Compute the effective rollout. + var effectiveRO *traffic.Rollout + if !hasExisting { + effectiveRO = tc.BuildRollout() } else { - // Ingress exists. We need to compute the rollout spec diff. - effectiveRO = c.reconcileRollout(ctx, r, tc, ingress) - desired, err := resources.MakeIngressWithRollout(ctx, r, tc, effectiveRO, - tls, ingressClass, acmeChallenges...) + effectiveRO = c.reconcileRolloutFromIngresses(ctx, r, tc, prevRO, allExistingReady) + } + + // Build default ingress with the effective rollout. + defaultIng, err := resources.MakeDefaultIngressWithRollout(ctx, r, tc, effectiveRO, tls, ingressClass, acmeChallenges...) + if err != nil { + return nil, nil, err + } + desired := []*netv1alpha1.Ingress{defaultIng} + + // Phase 2: Build per-tag ingresses (no rollout annotation). + var tagNames []string + for tag := range tc.Targets { + if tag != traffic.DefaultTarget { + tagNames = append(tagNames, tag) + } + } + sort.Strings(tagNames) + + for _, tag := range tagNames { + tagIng, err := resources.MakeRouteTagIngress(ctx, r, tc, tag, tls, ingressClass, acmeChallenges...) if err != nil { return nil, nil, err } + desired = append(desired, tagIng) + } - if !equality.Semantic.DeepEqual(ingress.Spec, desired.Spec) || - !equality.Semantic.DeepEqual(ingress.Annotations, desired.Annotations) || - !equality.Semantic.DeepEqual(ingress.Labels, desired.Labels) { - // It is notable that one reason for differences here may be defaulting. - // When that is the case, the Update will end up being a nop because the - // webhook will bring them into alignment and no new reconciliation will occur. - // Also, compare annotation and label in case ingress.Class or parent route's labels - // is updated. - - // Don't modify the informers copy. - origin := ingress.DeepCopy() - origin.Spec = desired.Spec - origin.Annotations = desired.Annotations - origin.Labels = desired.Labels - - updated, err := c.netclient.NetworkingV1alpha1().Ingresses(origin.Namespace).Update( - ctx, origin, metav1.UpdateOptions{}) + // Phase 3: Create or update each desired ingress, then clean up orphans. + var result []*netv1alpha1.Ingress + for _, d := range desired { + existing, ok := existingIngresses[d.Name] + if !ok { + created, err := c.netclient.NetworkingV1alpha1().Ingresses(d.Namespace).Create(ctx, d, metav1.CreateOptions{}) if err != nil { - return nil, nil, fmt.Errorf("failed to update Ingress: %w", err) + recorder.Eventf(r, corev1.EventTypeWarning, "CreationFailed", "Failed to create Ingress: %v", err) + return nil, nil, fmt.Errorf("failed to create Ingress: %w", err) + } + recorder.Eventf(r, corev1.EventTypeNormal, "Created", "Created Ingress %q (tag: %s)", created.GetName(), ingressTagForEvent(d.Labels)) + result = append(result, created) + } else { + if !equality.Semantic.DeepEqual(existing.Spec, d.Spec) || + !equality.Semantic.DeepEqual(existing.Annotations, d.Annotations) || + !equality.Semantic.DeepEqual(existing.Labels, d.Labels) { + origin := existing.DeepCopy() + origin.Spec = d.Spec + origin.Annotations = d.Annotations + origin.Labels = d.Labels + + updated, err := c.netclient.NetworkingV1alpha1().Ingresses(origin.Namespace).Update( + ctx, origin, metav1.UpdateOptions{}) + if err != nil { + return nil, nil, fmt.Errorf("failed to update Ingress: %w", err) + } + result = append(result, updated) + } else { + result = append(result, existing) } - return updated, effectiveRO, nil } } - return ingress, effectiveRO, err + // Delete orphaned ingresses (tags that no longer exist). + if err := c.deleteOrphanedIngresses(ctx, r, desiredNames); err != nil { + return nil, nil, err + } + + return result, effectiveRO, nil +} + +func (c *Reconciler) deleteOrphanedIngresses(ctx context.Context, r *v1.Route, desiredNames sets.Set[string]) error { + routeLabelSelector := labels.SelectorFromSet(labels.Set{serving.RouteLabelKey: r.Name}) + allIngresses, err := c.ingressLister.Ingresses(r.Namespace).List(routeLabelSelector) + if err != nil { + return fmt.Errorf("failed to fetch existing ingresses: %w", err) + } + + recorder := controller.GetEventRecorder(ctx) + for _, ing := range allIngresses { + if desiredNames.Has(ing.Name) { + continue + } + if !metav1.IsControlledBy(ing, r) { + continue + } + if err := c.netclient.NetworkingV1alpha1().Ingresses(r.Namespace).Delete( + ctx, ing.Name, metav1.DeleteOptions{}); err != nil && !apierrs.IsNotFound(err) { + recorder.Eventf(r, corev1.EventTypeWarning, "DeleteFailed", + "Failed to delete orphaned Ingress %q: %v", ing.Name, err) + return fmt.Errorf("failed to delete orphaned Ingress: %w", err) + } + recorder.Eventf(r, corev1.EventTypeNormal, "Deleted", "Deleted orphaned Ingress %q (tag: %s)", ing.Name, ingressTagForEvent(ing.Labels)) + } + return nil } func (c *Reconciler) deleteOrphanedServices(ctx context.Context, r *v1.Route, activeServices []resources.ServicePair) error { @@ -176,10 +254,10 @@ func (c *Reconciler) reconcilePlaceholderServices(ctx context.Context, route *v1 "Failed to create placeholder service %q: %v", desiredService.Name, err) return nil, fmt.Errorf("failed to create placeholder service: %w", err) } - logger.Info("Created service ", desiredService.Name) + logger.Infof("Created service %q", desiredService.Name) recorder.Eventf(route, corev1.EventTypeNormal, "Created", "Created placeholder service %q", desiredService.Name) } else if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get Service %q: %w", desiredService.Name, err) } else if !metav1.IsControlledBy(service, route) { // Surface an error in the route's status, and return an error. route.Status.MarkServiceNotOwned(desiredService.Name) @@ -215,13 +293,24 @@ func (c *Reconciler) reconcilePlaceholderServices(ctx context.Context, route *v1 return services, nil } -func (c *Reconciler) updatePlaceholderServices(ctx context.Context, route *v1.Route, pairs []resources.ServicePair, ingress *netv1alpha1.Ingress) error { +func (c *Reconciler) updatePlaceholderServices(ctx context.Context, route *v1.Route, pairs []resources.ServicePair, ingresses []*netv1alpha1.Ingress) error { logger := logging.FromContext(ctx) ns := route.Namespace + ingressByTag := make(map[string]*netv1alpha1.Ingress, len(ingresses)) + for _, ing := range ingresses { + tag := ing.Labels[networking.TagLabelKey] + ingressByTag[tag] = ing + } + eg, egCtx := errgroup.WithContext(ctx) for _, from := range pairs { eg.Go(func() error { + ingress, ok := ingressByTag[from.Tag] + if !ok { + logger.Warnw("No ingress found for tag, skipping placeholder update", zap.String("tag", from.Tag)) + return nil + } to, err := resources.MakeK8sService(egCtx, route, from.Tag, ingress, resources.IsClusterLocalService(from.Service)) if err != nil { // Loadbalancer not ready, no need to update. @@ -328,9 +417,16 @@ func deserializeRollout(ctx context.Context, ro string) *traffic.Rollout { return r } -func (c *Reconciler) reconcileRollout( +func ingressTagForEvent(labels map[string]string) string { + if tag := labels[networking.TagLabelKey]; tag != traffic.DefaultTarget { + return tag + } + return "default" +} + +func (c *Reconciler) reconcileRolloutFromIngresses( ctx context.Context, r *v1.Route, tc *traffic.Config, - ingress *netv1alpha1.Ingress, + prevRO *traffic.Rollout, allIngressesReady bool, ) *traffic.Rollout { cfg := config.FromContext(ctx) @@ -349,18 +445,18 @@ func (c *Reconciler) reconcileRollout( logger := logging.FromContext(ctx).Desugar().With( zap.Int("durationSecs", rd)) logger.Debug("Rollout is enabled. Stepping from previous state.") - // Get the previous rollout state from the annotation. - // If it's corrupt, inexistent, or otherwise incorrect, - // the prevRO will be just nil rollout. - prevRO := deserializeRollout(ctx, - ingress.Annotations[networking.RolloutAnnotationKey]) + + // prevRO was read from the default ingress annotation. + if prevRO == nil || len(prevRO.Configurations) == 0 { + prevRO = nil + } // And recompute the rollout state. now := c.clock.Now().UnixNano() - // Now check if the ingress status changed from not ready to ready. + // Now check if all ingresses transitioned from not ready to ready. rtView := r.Status.GetCondition(v1.RouteConditionIngressReady) - if prevRO != nil && ingress.IsReady() && !rtView.IsTrue() { + if prevRO != nil && allIngressesReady && !rtView.IsTrue() { logger.Debug("Observing Ingress not-ready to ready switch condition for rollout") prevRO.ObserveReady(ctx, now, float64(rd)) } diff --git a/pkg/reconciler/route/reconcile_resources_test.go b/pkg/reconciler/route/reconcile_resources_test.go index 9628476c1b7f..bca945971c63 100644 --- a/pkg/reconciler/route/reconcile_resources_test.go +++ b/pkg/reconciler/route/reconcile_resources_test.go @@ -53,7 +53,7 @@ func TestReconcileIngressInsert(t *testing.T) { r := Route("test-ns", "test-route") tc, tls := testIngressParams(t, r) - _, ro, err := reconciler.reconcileIngress(updateContext(ctx, 0), r, tc, tls, "foo-ingress") + _, ro, err := reconciler.reconcileIngresses(updateContext(ctx, 0), r, tc, tls, "foo-ingress") if err != nil { t.Error("Unexpected error:", err) } @@ -110,7 +110,7 @@ func TestReconcileIngressUpdateReenqueueRollout(t *testing.T) { duration = d } ctx := updateContext(baseCtx, rd) - _, ro, err := reconciler.reconcileIngress(ctx, r, tc, tls, "foo-ingress-class") + _, ro, err := reconciler.reconcileIngresses(ctx, r, tc, tls, "foo-ingress-class") if err != nil { t.Fatal("Unexpected error:", err) } @@ -119,13 +119,13 @@ func TestReconcileIngressUpdateReenqueueRollout(t *testing.T) { t.Errorf("Complex initial rollout mismatch: diff(-want,+got):\n%s", cmp.Diff(want, got)) } + addAllRouteIngressesToIndexer(ctx, t, r) ing := getRouteIngressFromClient(ctx, t, r) - fakeingressinformer.Get(ctx).Informer().GetIndexer().Add(ing) // Now we have initial version. Let's make a rollout. tc.Targets[traffic.DefaultTarget][1].RevisionName = "miercoles" - _, updRO, err := reconciler.reconcileIngress(ctx, r, tc, tls, "foo-ingress-class") + _, updRO, err := reconciler.reconcileIngresses(ctx, r, tc, tls, "foo-ingress-class") if err != nil { t.Error("Unexpected error:", err) } @@ -146,6 +146,7 @@ func TestReconcileIngressUpdateReenqueueRollout(t *testing.T) { t.Fatal("Rollout was bogus and impossible to deserialize") } + // Default ingress annotation now stores the full rollout (all tags). // Verify the same rollout was returned by the ReconcileResources. if !cmp.Equal(ro, updRO) { t.Errorf("Returned and Annotation Rollouts differ: diff(-ret,+ann):\n%s", cmp.Diff(updRO, ro)) @@ -157,10 +158,13 @@ func TestReconcileIngressUpdateReenqueueRollout(t *testing.T) { } // OK. So now let's observe ready to start the rollout. For that we need to make ingress ready. - cp := updated.DeepCopy() - cp.Status.MarkLoadBalancerReady(nil, nil) - cp.Status.MarkNetworkConfigured() - fakeingressinformer.Get(ctx).Informer().GetIndexer().Add(cp) + // Mark ALL per-tag ingresses as ready so allIngressesReady is true. + allIngresses := getRouteIngressesFromClient(ctx, t, r) + for i := range allIngresses { + allIngresses[i].Status.MarkLoadBalancerReady(nil, nil) + allIngresses[i].Status.MarkNetworkConfigured() + fakeingressinformer.Get(ctx).Informer().GetIndexer().Add(&allIngresses[i]) + } // For comparisons. ing = updated @@ -175,7 +179,7 @@ func TestReconcileIngressUpdateReenqueueRollout(t *testing.T) { stepSize := math.Max(1, math.Round((allocatedTraffic-1)/steps)) // we round the step size. stepDuration := time.Duration(int(totalDuration * float64(time.Second) / steps)) - _, updRO, err = reconciler.reconcileIngress(ctx, r, tc, tls, "foo-ingress") + _, updRO, err = reconciler.reconcileIngresses(ctx, r, tc, tls, "foo-ingress") if err != nil { t.Error("Unexpected error:", err) } @@ -202,6 +206,7 @@ func TestReconcileIngressUpdateReenqueueRollout(t *testing.T) { t.Errorf("StepDuration = %d, want: %d", got, want) } + // Default ingress annotation now stores the full rollout (all tags). // Verify the same rollout was returned by the ReconcileResources. if !cmp.Equal(ro, updRO) { t.Errorf("Returned and Annotation Rollouts after re-enqueue differ: diff(-ret,+ann):\n%s", @@ -272,7 +277,7 @@ func TestReconcileIngressUpdateReenqueueRolloutAnnotation(t *testing.T) { duration = d } ctx := updateContext(baseCtx, 311 /*rolloutDuration*/) // This should be ignored, since we set annotation. - _, ro, err := reconciler.reconcileIngress(ctx, r, tc, tls, "foo-ingress-class") + _, ro, err := reconciler.reconcileIngresses(ctx, r, tc, tls, "foo-ingress-class") if err != nil { t.Fatal("Unexpected error:", err) } @@ -281,13 +286,13 @@ func TestReconcileIngressUpdateReenqueueRolloutAnnotation(t *testing.T) { t.Errorf("Complex initial rollout mismatch: diff(-want,+got):\n%s", cmp.Diff(want, got)) } + addAllRouteIngressesToIndexer(ctx, t, r) ing := getRouteIngressFromClient(ctx, t, r) - fakeingressinformer.Get(ctx).Informer().GetIndexer().Add(ing) // Now we have initial version. Let's make a rollout. tc.Targets[traffic.DefaultTarget][1].RevisionName = "miercoles" - _, updRO, err := reconciler.reconcileIngress(ctx, r, tc, tls, "foo-ingress-class") + _, updRO, err := reconciler.reconcileIngresses(ctx, r, tc, tls, "foo-ingress-class") if err != nil { t.Error("Unexpected error:", err) } @@ -308,6 +313,7 @@ func TestReconcileIngressUpdateReenqueueRolloutAnnotation(t *testing.T) { t.Fatal("Rollout was bogus and impossible to deserialize") } + // Default ingress annotation now stores the full rollout (all tags). // Verify the same rollout was returned by the ReconcileResources. if !cmp.Equal(ro, updRO) { t.Errorf("Returned and Annotation Rollouts differ: diff(-ret,+ann):\n%s", cmp.Diff(updRO, ro)) @@ -319,10 +325,13 @@ func TestReconcileIngressUpdateReenqueueRolloutAnnotation(t *testing.T) { } // OK. So now let's observe ready to start the rollout. For that we need to make ingress ready. - cp := updated.DeepCopy() - cp.Status.MarkLoadBalancerReady(nil, nil) - cp.Status.MarkNetworkConfigured() - fakeingressinformer.Get(ctx).Informer().GetIndexer().Add(cp) + // Mark ALL per-tag ingresses as ready so allIngressesReady is true. + allIngresses := getRouteIngressesFromClient(ctx, t, r) + for i := range allIngresses { + allIngresses[i].Status.MarkLoadBalancerReady(nil, nil) + allIngresses[i].Status.MarkNetworkConfigured() + fakeingressinformer.Get(ctx).Informer().GetIndexer().Add(&allIngresses[i]) + } // For comparisons. ing = updated @@ -337,7 +346,7 @@ func TestReconcileIngressUpdateReenqueueRolloutAnnotation(t *testing.T) { stepSize := math.Max(1, math.Round((allocatedTraffic-1)/steps)) // we round the step size. stepDuration := time.Duration(int(totalDuration * float64(time.Second) / steps)) - _, updRO, err = reconciler.reconcileIngress(ctx, r, tc, tls, "foo-ingress") + _, updRO, err = reconciler.reconcileIngresses(ctx, r, tc, tls, "foo-ingress") if err != nil { t.Error("Unexpected error:", err) } @@ -364,6 +373,7 @@ func TestReconcileIngressUpdateReenqueueRolloutAnnotation(t *testing.T) { t.Errorf("StepDuration = %d, want: %d", got, want) } + // Default ingress annotation now stores the full rollout (all tags). // Verify the same rollout was returned by the ReconcileResources. if !cmp.Equal(ro, updRO) { t.Errorf("Returned and Annotation Rollouts after re-enqueue differ: diff(-ret,+ann):\n%s", @@ -394,17 +404,17 @@ func TestReconcileIngressUpdateNoRollout(t *testing.T) { r := Route("test-ns", "test-route") tc, tls := testIngressParams(t, r) - if _, _, err := reconciler.reconcileIngress(ctx, r, tc, tls, "foo-ingress"); err != nil { + if _, _, err := reconciler.reconcileIngresses(ctx, r, tc, tls, "foo-ingress"); err != nil { t.Error("Unexpected error:", err) } + addAllRouteIngressesToIndexer(ctx, t, r) initial := getRouteIngressFromClient(ctx, t, r) - fakeingressinformer.Get(ctx).Informer().GetIndexer().Add(initial) tc, tls = testIngressParams(t, r, func(tc *traffic.Config) { tc.Targets[traffic.DefaultTarget][0].RevisionName = "revision2" }) - if _, _, err := reconciler.reconcileIngress(ctx, r, tc, tls, "foo-ingress"); err != nil { + if _, _, err := reconciler.reconcileIngresses(ctx, r, tc, tls, "foo-ingress"); err != nil { t.Error("Unexpected error:", err) } @@ -441,12 +451,12 @@ func TestReconcileIngressUpdate(t *testing.T) { r := Route("test-ns", "test-route") tc, tls := testIngressParams(t, r) - if _, _, err := reconciler.reconcileIngress(updateContext(ctx, 0), r, tc, tls, "foo-ingress"); err != nil { + if _, _, err := reconciler.reconcileIngresses(updateContext(ctx, 0), r, tc, tls, "foo-ingress"); err != nil { t.Error("Unexpected error:", err) } + addAllRouteIngressesToIndexer(ctx, t, r) initial := getRouteIngressFromClient(ctx, t, r) - fakeingressinformer.Get(ctx).Informer().GetIndexer().Add(initial) tc, tls = testIngressParams(t, r, func(tc *traffic.Config) { tc.Targets[traffic.DefaultTarget][0].TrafficTarget.Percent = ptr.Int64(50) @@ -457,7 +467,7 @@ func TestReconcileIngressUpdate(t *testing.T) { }, }) }) - if _, _, err := reconciler.reconcileIngress(updateContext(ctx, 0), r, tc, tls, "foo-ingress"); err != nil { + if _, _, err := reconciler.reconcileIngresses(updateContext(ctx, 0), r, tc, tls, "foo-ingress"); err != nil { t.Error("Unexpected error:", err) } @@ -503,15 +513,16 @@ func TestReconcileIngressRolloutDeserializeFail(t *testing.T) { r := Route("test-ns", "test-route-"+tc.name) traffic, tls := testIngressParams(t, r) - ing, err := resources.MakeIngress(ctx, r, traffic, tls, "foo-ingress") + ings, err := resources.MakeIngress(ctx, r, traffic, tls, "foo-ingress") if err != nil { t.Fatal("Error creating ingress:", err) } + ing := ings[0] ing.Annotations[networking.RolloutAnnotationKey] = tc.val ingClient := fakenetworkingclient.Get(ctx).NetworkingV1alpha1().Ingresses(ing.Namespace) ingClient.Create(ctx, ing, metav1.CreateOptions{}) fakeingressinformer.Get(ctx).Informer().GetIndexer().Add(ing) - if _, _, err := reconciler.reconcileIngress(ctx, r, traffic, tls, "foo-ingress"); err != nil { + if _, _, err := reconciler.reconcileIngresses(ctx, r, traffic, tls, "foo-ingress"); err != nil { t.Error("Unexpected error:", err) } ing, err = ingClient.Get(ctx, ing.Name, metav1.GetOptions{}) @@ -519,8 +530,10 @@ func TestReconcileIngressRolloutDeserializeFail(t *testing.T) { t.Fatal("Could not get the ingress:", err) } // In all cases we want to ignore the previous one, since it's bogus. + // Default ingress annotation now stores the full rollout (all tags). want := func() string { - d, _ := json.Marshal(traffic.BuildRollout()) + ro := traffic.BuildRollout() + d, _ := json.Marshal(ro) return string(d) }() if got := ing.Annotations[networking.RolloutAnnotationKey]; got != want { @@ -597,18 +610,21 @@ func TestReconcileIngressClassAnnotation(t *testing.T) { r := Route("test-ns", "test-route") tc, tls := testIngressParams(t, r) - if _, _, err := reconciler.reconcileIngress(updateContext(ctx, 0), r, tc, tls, "foo-ingress"); err != nil { + if _, _, err := reconciler.reconcileIngresses(updateContext(ctx, 0), r, tc, tls, "foo-ingress"); err != nil { t.Error("Unexpected error:", err) } - updated := getRouteIngressFromClient(ctx, t, r) - fakeingressinformer.Get(ctx).Informer().GetIndexer().Add(updated) + // Add all per-tag ingresses to the informer indexer. + allIngresses := getRouteIngressesFromClient(ctx, t, r) + for i := range allIngresses { + fakeingressinformer.Get(ctx).Informer().GetIndexer().Add(&allIngresses[i]) + } - if _, _, err := reconciler.reconcileIngress(updateContext(ctx, 0), r, tc, tls, expClass); err != nil { + if _, _, err := reconciler.reconcileIngresses(updateContext(ctx, 0), r, tc, tls, expClass); err != nil { t.Error("Unexpected error:", err) } - updated = getRouteIngressFromClient(ctx, t, r) + updated := getRouteIngressFromClient(ctx, t, r) updatedClass := networking.GetIngressClass(updated.Annotations) if expClass != updatedClass { t.Errorf("Unexpected annotation got %q want %q", expClass, updatedClass) diff --git a/pkg/reconciler/route/resources/ingress.go b/pkg/reconciler/route/resources/ingress.go index ba593ebe421d..4beda5fa4440 100644 --- a/pkg/reconciler/route/resources/ingress.go +++ b/pkg/reconciler/route/resources/ingress.go @@ -19,6 +19,9 @@ package resources import ( "context" "encoding/json" + "errors" + "maps" + "slices" "sort" "github.com/davecgh/go-spew/spew" @@ -30,6 +33,7 @@ import ( "knative.dev/networking/pkg/apis/networking" netv1alpha1 "knative.dev/networking/pkg/apis/networking/v1alpha1" + netcfg "knative.dev/networking/pkg/config" netheader "knative.dev/networking/pkg/http/header" "knative.dev/pkg/kmeta" "knative.dev/pkg/logging" @@ -52,8 +56,11 @@ func MakeIngressTLS(cert *netv1alpha1.Certificate, hostNames []string) netv1alph } } -// MakeIngress creates Ingress to set up routing rules. Such Ingress specifies -// which Hosts that it applies to, as well as the routing rules. +// MakeIngress creates the desired per-tag Ingress resources for the Route, +// deriving the rollout from the current traffic configuration. +// +// The provided traffic.Config's Targets map must contain the default target +// (traffic.DefaultTarget), as this is essential for routing configuration. func MakeIngress( ctx context.Context, r *servingv1.Route, @@ -61,16 +68,67 @@ func MakeIngress( tls []netv1alpha1.IngressTLS, ingressClass string, acmeChallenges ...netv1alpha1.HTTP01Challenge, -) (*netv1alpha1.Ingress, error) { - return MakeIngressWithRollout( - // If no rollout is specified, we just build the default one. - ctx, r, tc, tc.BuildRollout(), tls, ingressClass, acmeChallenges...) +) ([]*netv1alpha1.Ingress, error) { + return makeIngresses(ctx, r, tc, tc.BuildRollout(), tls, ingressClass, acmeChallenges...) +} + +// DesiredIngressNames returns the set of ingress names that should exist +// for the given traffic configuration. Always includes the default ingress. +func DesiredIngressNames(r kmeta.Accessor, tc *traffic.Config) sets.Set[string] { + result := sets.New(names.TaggedIngress(r, traffic.DefaultTarget)) + for tag := range tc.Targets { + if tag != traffic.DefaultTarget { + result.Insert(names.TaggedIngress(r, tag)) + } + } + return result +} + +// makeIngresses builds the full set of per-tag KIngress objects from the given +// parameters using the provided rollout. +func makeIngresses( + ctx context.Context, + r *servingv1.Route, + tc *traffic.Config, + ro *traffic.Rollout, + tls []netv1alpha1.IngressTLS, + ingressClass string, + acmeChallenges ...netv1alpha1.HTTP01Challenge, +) ([]*netv1alpha1.Ingress, error) { + // Get sorted tag names for deterministic ordering. + tagNames := slices.Sorted(maps.Keys(tc.Targets)) + + featuresConfig := config.FromContextOrDefaults(ctx).Features + networkConfig := config.FromContextOrDefaults(ctx).Network + + httpOption, err := servingnetworking.GetHTTPOption(ctx, networkConfig, r.GetAnnotations()) + if err != nil { + return nil, err + } + + acmePathsByHost := buildACMEPathsByHost(acmeChallenges) + + ingresses := make([]*netv1alpha1.Ingress, 0, len(tagNames)) + for _, tagName := range tagNames { + ing, err := buildTagIngress(ctx, r, tc, ro, tagName, tls, ingressClass, + featuresConfig, networkConfig, httpOption, acmePathsByHost) + if err != nil { + return nil, err + } + ingresses = append(ingresses, ing) + } + + return ingresses, nil } -// MakeIngressWithRollout builds a KIngress object from the given parameters. -// When building the ingress the builder will take into the account -// the desired rollout state to split the traffic. -func MakeIngressWithRollout( +// MakeDefaultIngressWithRollout creates the default Ingress for the Route, +// incorporating the specified rollout configuration for gradual traffic shifting. +// Rollout is only meaningful for the default ingress. +// +// When TagHeaderBasedRouting is enabled, the provided Rollout must contain +// configurations for all traffic targets (not just the default), because +// the default ingress includes tag-header-based routing paths for other tags. +func MakeDefaultIngressWithRollout( ctx context.Context, r *servingv1.Route, tc *traffic.Config, @@ -79,28 +137,112 @@ func MakeIngressWithRollout( ingressClass string, acmeChallenges ...netv1alpha1.HTTP01Challenge, ) (*netv1alpha1.Ingress, error) { - spec, err := makeIngressSpec(ctx, r, tls, tc, ro, acmeChallenges...) + featuresConfig := config.FromContextOrDefaults(ctx).Features + networkConfig := config.FromContextOrDefaults(ctx).Network + + httpOption, err := servingnetworking.GetHTTPOption(ctx, config.FromContext(ctx).Network, r.GetAnnotations()) + if err != nil { + return nil, err + } + + acmePathsByHost := buildACMEPathsByHost(acmeChallenges) + + return buildTagIngress(ctx, r, tc, ro, traffic.DefaultTarget, tls, ingressClass, + featuresConfig, networkConfig, httpOption, acmePathsByHost) +} + +// MakeRouteTagIngress creates an Ingress for a specific traffic tag. +// Tag ingresses handle routing for named traffic targets (e.g., "canary", "latest"). +// Rollout configuration is derived internally via tc.BuildRollout(); +// gradual rollout (multi-step traffic shifting) is primarily a concern of the +// default ingress. +func MakeRouteTagIngress( + ctx context.Context, + r *servingv1.Route, + tc *traffic.Config, + tagName string, + tls []netv1alpha1.IngressTLS, + ingressClass string, + acmeChallenges ...netv1alpha1.HTTP01Challenge, +) (*netv1alpha1.Ingress, error) { + if tagName == traffic.DefaultTarget { + return nil, errors.New("cannot create per-tag ingress for default target: use MakeDefaultIngressWithRollout instead") + } + + featuresConfig := config.FromContextOrDefaults(ctx).Features + networkConfig := config.FromContextOrDefaults(ctx).Network + + httpOption, err := servingnetworking.GetHTTPOption(ctx, config.FromContext(ctx).Network, r.GetAnnotations()) + if err != nil { + return nil, err + } + + ro := tc.BuildRollout() + acmePathsByHost := buildACMEPathsByHost(acmeChallenges) + + return buildTagIngress(ctx, r, tc, ro, tagName, tls, ingressClass, + featuresConfig, networkConfig, httpOption, acmePathsByHost) +} + +// buildTagIngress builds a single Ingress for the given traffic tag. +func buildTagIngress( + ctx context.Context, + r *servingv1.Route, + tc *traffic.Config, + ro *traffic.Rollout, + tagName string, + tls []netv1alpha1.IngressTLS, + ingressClass string, + featuresConfig *apicfg.Features, + networkConfig *netcfg.Config, + httpOption netv1alpha1.HTTPOption, + acmePathsByHost map[string][]netv1alpha1.HTTPIngressPath, +) (*netv1alpha1.Ingress, error) { + spec, err := makeIngressSpecForTag(ctx, r, tls, tc, ro, tagName, featuresConfig, networkConfig, httpOption, acmePathsByHost) if err != nil { return nil, err } + + labels := map[string]string{ + serving.RouteLabelKey: r.Name, + serving.RouteNamespaceLabelKey: r.Namespace, + } + if tagName != traffic.DefaultTarget { + labels[networking.TagLabelKey] = tagName + } + + // Rollout annotation is only set on the default ingress, as rollout state + // (gradual traffic shifting) is a concern of the default ingress only. + baseAnnotations := map[string]string{ + networking.IngressClassAnnotationKey: ingressClass, + } + if tagName == traffic.DefaultTarget { + baseAnnotations[networking.RolloutAnnotationKey] = serializeRollout(ctx, ro) + } + return &netv1alpha1.Ingress{ ObjectMeta: metav1.ObjectMeta{ - Name: names.Ingress(r), - Namespace: r.Namespace, - Labels: kmeta.UnionMaps(r.Labels, map[string]string{ - serving.RouteLabelKey: r.Name, - serving.RouteNamespaceLabelKey: r.Namespace, - }), - Annotations: kmeta.FilterMap(kmeta.UnionMaps(map[string]string{ - networking.IngressClassAnnotationKey: ingressClass, - networking.RolloutAnnotationKey: serializeRollout(ctx, ro), - }, r.GetAnnotations()), ExcludedAnnotations.Has), + Name: names.TaggedIngress(r, tagName), + Namespace: r.Namespace, + Labels: kmeta.UnionMaps(r.Labels, labels), + Annotations: kmeta.FilterMap(kmeta.UnionMaps(baseAnnotations, r.GetAnnotations()), ExcludedAnnotations.Has), OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(r)}, }, Spec: spec, }, nil } +// buildACMEPathsByHost creates a lookup map from host to ACME challenge paths. +func buildACMEPathsByHost(acmeChallenges []netv1alpha1.HTTP01Challenge) map[string][]netv1alpha1.HTTPIngressPath { + acmePathsByHost := make(map[string][]netv1alpha1.HTTPIngressPath) + for _, challenge := range acmeChallenges { + host := challenge.URL.Host + acmePath := MakeACMEIngressPath(challenge) + acmePathsByHost[host] = append(acmePathsByHost[host], acmePath) + } + return acmePathsByHost +} + func serializeRollout(ctx context.Context, r *traffic.Rollout) string { sr, err := json.Marshal(r) if err != nil { @@ -112,114 +254,115 @@ func serializeRollout(ctx context.Context, r *traffic.Rollout) string { return string(sr) } -// makeIngressSpec builds a new IngressSpec from inputs. -func makeIngressSpec( +// makeIngressSpecForTag builds a new IngressSpec for a single traffic tag. +func makeIngressSpecForTag( ctx context.Context, r *servingv1.Route, tls []netv1alpha1.IngressTLS, tc *traffic.Config, ro *traffic.Rollout, - acmeChallenges ...netv1alpha1.HTTP01Challenge, + tagName string, + featuresConfig *apicfg.Features, + networkConfig *netcfg.Config, + httpOption netv1alpha1.HTTPOption, + acmePathsByHost map[string][]netv1alpha1.HTTPIngressPath, ) (netv1alpha1.IngressSpec, error) { - // Domain should have been specified in route status - // before calling this func. - names := make([]string, 0, len(tc.Targets)) - for name := range tc.Targets { - names = append(names, name) + visibilities := []netv1alpha1.IngressVisibility{netv1alpha1.IngressVisibilityClusterLocal} + // If this is a public target (or not being marked as cluster-local), we also make public rule. + if v, ok := tc.Visibility[tagName]; !ok || v == netv1alpha1.IngressVisibilityExternalIP { + visibilities = append(visibilities, netv1alpha1.IngressVisibilityExternalIP) } - // Sort the names to give things a deterministic ordering. - sort.Strings(names) - // The routes are matching rule based on domain name to traffic split targets. - rules := make([]netv1alpha1.IngressRule, 0, len(names)) - featuresConfig := config.FromContextOrDefaults(ctx).Features - networkConfig := config.FromContextOrDefaults(ctx).Network + var rules []netv1alpha1.IngressRule + for _, visibility := range visibilities { + tagDomains, err := domains.GetDomainsForVisibility(ctx, tagName, r, visibility) + if err != nil { + return netv1alpha1.IngressSpec{}, err + } + domainRules := makeIngressRules(tagDomains, r.Namespace, + visibility, tc.Targets[tagName], ro.RolloutsByTag(tagName), networkConfig.SystemInternalTLSEnabled()) - // Build ACME lookup map: host -> []HTTPIngressPath - acmePathsByHost := make(map[string][]netv1alpha1.HTTPIngressPath) - for _, challenge := range acmeChallenges { - host := challenge.URL.Host - acmePath := MakeACMEIngressPath(challenge) - acmePathsByHost[host] = append(acmePathsByHost[host], acmePath) - } + // Apply tag header routing and ACME merging to each rule + for i := range domainRules { + rule := &domainRules[i] - for _, name := range names { - visibilities := []netv1alpha1.IngressVisibility{netv1alpha1.IngressVisibilityClusterLocal} - // If this is a public target (or not being marked as cluster-local), we also make public rule. - if v, ok := tc.Visibility[name]; !ok || v == netv1alpha1.IngressVisibilityExternalIP { - visibilities = append(visibilities, netv1alpha1.IngressVisibilityExternalIP) - } - for _, visibility := range visibilities { - domains, err := domains.GetDomainsForVisibility(ctx, name, r, visibility) - if err != nil { - return netv1alpha1.IngressSpec{}, err - } - domainRules := makeIngressRules(domains, r.Namespace, - visibility, tc.Targets[name], ro.RolloutsByTag(name), networkConfig.SystemInternalTLSEnabled()) + if featuresConfig.TagHeaderBasedRouting == apicfg.Enabled { + if rule.HTTP.Paths[0].AppendHeaders == nil { + rule.HTTP.Paths[0].AppendHeaders = make(map[string]string, 1) + } - // Apply tag header routing and ACME merging to each rule - for i := range domainRules { - rule := &domainRules[i] + if tagName == traffic.DefaultTarget { + rule.HTTP.Paths[0].AppendHeaders[netheader.DefaultRouteKey] = "true" - if featuresConfig.TagHeaderBasedRouting == apicfg.Enabled { - if rule.HTTP.Paths[0].AppendHeaders == nil { - rule.HTTP.Paths[0].AppendHeaders = make(map[string]string, 1) + // Add ingress paths for requests with the tag header. + // Collect non-default tag names (sorted). + var otherTags []string + for name := range tc.Targets { + if name != traffic.DefaultTarget { + otherTags = append(otherTags, name) + } } - - if name == traffic.DefaultTarget { - // To provide information if a request is routed via the "default route" or not, - // the header "Knative-Serving-Default-Route: true" is appended here. - // If the header has "true" and there is a "Knative-Serving-Tag" header, - // then the request is having the undefined tag header, - // which will be observed in queue-proxy. - rule.HTTP.Paths[0].AppendHeaders[netheader.DefaultRouteKey] = "true" - - // Add ingress paths for a request with the tag header. - // If a request has one of the `names` (tag name), specified as the - // Knative-Serving-Tag header, except for the DefaultTarget, - // the request will be routed to that target. - // corresponding to the tag name. - // Since names are sorted `DefaultTarget == ""` is the first one, - // so just pass the subslice. - rule.HTTP.Paths = append( - makeTagBasedRoutingIngressPaths(r.Namespace, tc, ro, networkConfig.SystemInternalTLSEnabled(), names[1:]), rule.HTTP.Paths...) - } else { - // If a request is routed by a tag-attached hostname instead of the tag header, - // the request may not have the tag header "Knative-Serving-Tag", - // even though the ingress path used in the case is also originated - // from the same Knative route with the ingress path for the tag based routing. - // - // To prevent such inconsistency, - // the tag header is appended with the tag corresponding to the tag-attached hostname - rule.HTTP.Paths[0].AppendHeaders[netheader.RouteTagKey] = name + sort.Strings(otherTags) + if len(otherTags) > 0 { + tagPaths := makeTagBasedRoutingIngressPaths( + r.Namespace, tc, ro, networkConfig.SystemInternalTLSEnabled(), otherTags) + rule.HTTP.Paths = append(tagPaths, rule.HTTP.Paths...) } + } else { + rule.HTTP.Paths[0].AppendHeaders[netheader.RouteTagKey] = tagName } + } - // Merge ACME paths for ExternalIP single-host rules - // Note: ACME challenges without matching traffic rules are silently ignored - if visibility == netv1alpha1.IngressVisibilityExternalIP && len(rule.Hosts) == 1 { - if acmePaths, exists := acmePathsByHost[rule.Hosts[0]]; exists { - rule.HTTP.Paths = append(acmePaths, rule.HTTP.Paths...) - } + // Merge ACME paths for ExternalIP single-host rules + // Note: ACME challenges without matching traffic rules are silently ignored + if visibility == netv1alpha1.IngressVisibilityExternalIP && len(rule.Hosts) == 1 { + if acmePaths, exists := acmePathsByHost[rule.Hosts[0]]; exists { + rule.HTTP.Paths = append(acmePaths, rule.HTTP.Paths...) } } - - rules = append(rules, domainRules...) } - } - httpOption, err := servingnetworking.GetHTTPOption(ctx, config.FromContext(ctx).Network, r.GetAnnotations()) - if err != nil { - return netv1alpha1.IngressSpec{}, err + rules = append(rules, domainRules...) } + // Filter TLS entries to only include those relevant to this tag's domains. + tagTLS := filterTLSForRules(tls, rules) + return netv1alpha1.IngressSpec{ Rules: rules, - TLS: tls, + TLS: tagTLS, HTTPOption: httpOption, }, nil } +// filterTLSForRules returns the subset of TLS entries whose hosts match +// hosts found in the given ingress rules. +func filterTLSForRules(tls []netv1alpha1.IngressTLS, rules []netv1alpha1.IngressRule) []netv1alpha1.IngressTLS { + // Collect all hosts from rules + ruleHosts := sets.New[string]() + for _, rule := range rules { + ruleHosts.Insert(rule.Hosts...) + } + + var filtered []netv1alpha1.IngressTLS + for _, t := range tls { + var matchingHosts []string + for _, h := range t.Hosts { + if ruleHosts.Has(h) { + matchingHosts = append(matchingHosts, h) + } + } + if len(matchingHosts) > 0 { + filtered = append(filtered, netv1alpha1.IngressTLS{ + Hosts: matchingHosts, + SecretName: t.SecretName, + SecretNamespace: t.SecretNamespace, + }) + } + } + return filtered +} + // MakeACMEIngressPath converts an ACME challenge into an HTTPIngressPath. func MakeACMEIngressPath(challenge netv1alpha1.HTTP01Challenge) netv1alpha1.HTTPIngressPath { return netv1alpha1.HTTPIngressPath{ @@ -267,7 +410,6 @@ func makeIngressRuleForHosts(hosts []string, visibility netv1alpha1.IngressVisib } } -// `names` must not include `""` — the DefaultTarget. func makeTagBasedRoutingIngressPaths(ns string, tc *traffic.Config, ro *traffic.Rollout, encryption bool, names []string) []netv1alpha1.HTTPIngressPath { paths := make([]netv1alpha1.HTTPIngressPath, 0, len(names)) diff --git a/pkg/reconciler/route/resources/ingress_test.go b/pkg/reconciler/route/resources/ingress_test.go index 2bfd12db0ff8..49b84bb71a83 100644 --- a/pkg/reconciler/route/resources/ingress_test.go +++ b/pkg/reconciler/route/resources/ingress_test.go @@ -62,7 +62,15 @@ func TestMakeIngressCorrectMetadata(t *testing.T) { ingressClass = "ng-ingress" passdownIngressClass = "ok-ingress" ) - targets := map[string]traffic.RevisionTargets{} + targets := map[string]traffic.RevisionTargets{ + traffic.DefaultTarget: {{ + TrafficTarget: v1.TrafficTarget{ + ConfigurationName: "config", + RevisionName: "v1", + Percent: ptr.Int64(100), + }, + }}, + } r := Route(ns, "test-route", WithRouteLabel(map[string]string{ serving.RouteLabelKey: "try-to-override", serving.RouteNamespaceLabelKey: "try-to-override", @@ -87,13 +95,17 @@ func TestMakeIngressCorrectMetadata(t *testing.T) { }, OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(r)}, } - ia, err := MakeIngress(testContext(), r, &traffic.Config{Targets: targets}, nil, ingressClass) + ingresses, err := MakeIngress(testContext(), r, &traffic.Config{Targets: targets}, nil, ingressClass) if err != nil { t.Error("Unexpected error", err) } - if !cmp.Equal(expected, ia.ObjectMeta) { - t.Error("Unexpected metadata (-want, +got):", cmp.Diff(expected, ia.ObjectMeta)) + if len(ingresses) == 0 { + t.Fatal("Expected at least one ingress, got none") + } + + if !cmp.Equal(expected, ingresses[0].ObjectMeta) { + t.Error("Unexpected metadata (-want, +got):", cmp.Diff(expected, ingresses[0].ObjectMeta)) } } @@ -129,7 +141,20 @@ func TestMakeIngressWithTaggedRollout(t *testing.T) { "test-annotation": "bar", }), WithRouteUID("1234-5678"), WithURL) - wantMeta := metav1.ObjectMeta{ + ingresses, err := MakeIngress(testContext(), r, cfg, nil, ingressClass) + if err != nil { + t.Error("Unexpected error", err) + } + + if len(ingresses) != 2 { + t.Fatalf("Expected 2 ingresses, got %d", len(ingresses)) + } + + // Ingresses are sorted by tag name: "" (default) comes first, then "tagged" + defaultIng := ingresses[0] + taggedIng := ingresses[1] + + wantDefaultMeta := metav1.ObjectMeta{ Name: "test-route", Namespace: ns, Labels: map[string]string{ @@ -144,13 +169,27 @@ func TestMakeIngressWithTaggedRollout(t *testing.T) { }, OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(r)}, } - ing, err := MakeIngress(testContext(), r, cfg, nil, ingressClass) - if err != nil { - t.Error("Unexpected error", err) + if !cmp.Equal(wantDefaultMeta, defaultIng.ObjectMeta) { + t.Error("Unexpected default ingress metadata (-want, +got):", cmp.Diff(wantDefaultMeta, defaultIng.ObjectMeta)) } - if !cmp.Equal(wantMeta, ing.ObjectMeta) { - t.Error("Unexpected metadata (-want, +got):", cmp.Diff(wantMeta, ing.ObjectMeta)) + wantTaggedMeta := metav1.ObjectMeta{ + Name: "test-route-tagged", + Namespace: ns, + Labels: map[string]string{ + serving.RouteLabelKey: "test-route", + serving.RouteNamespaceLabelKey: ns, + "test-label": "foo", + networking.TagLabelKey: "tagged", + }, + Annotations: map[string]string{ + networking.IngressClassAnnotationKey: ingressClass, + "test-annotation": "bar", + }, + OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(r)}, + } + if !cmp.Equal(wantTaggedMeta, taggedIng.ObjectMeta) { + t.Error("Unexpected tagged ingress metadata (-want, +got):", cmp.Diff(wantTaggedMeta, taggedIng.ObjectMeta)) } } @@ -235,7 +274,22 @@ func TestMakeIngressWithActualRollout(t *testing.T) { networking.IngressClassAnnotationKey: ingressClass, }), WithRouteUID("1234-5678"), WithURL) - wantMeta := metav1.ObjectMeta{ + ingresses, err := makeIngresses(testContext(), r, cfg, ro, nil /*tls*/, ingressClass) + if err != nil { + t.Error("Unexpected error", err) + } + + if len(ingresses) != 2 { + t.Fatalf("Expected 2 ingresses, got %d", len(ingresses)) + } + + // Ingresses are sorted by tag name: "" (default) first, then "hammer" + defaultIng := ingresses[0] + hammerIng := ingresses[1] + + // Check default ingress metadata + // Default ingress now includes full rollout (all configurations including tagged ones). + wantDefaultMeta := metav1.ObjectMeta{ Name: "test-route", Namespace: ns, Labels: map[string]string{ @@ -248,14 +302,325 @@ func TestMakeIngressWithActualRollout(t *testing.T) { }, OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(r)}, } - ing, err := MakeIngressWithRollout(testContext(), r, cfg, ro, nil /*tls*/, ingressClass) + if !cmp.Equal(wantDefaultMeta, defaultIng.ObjectMeta) { + t.Error("Unexpected default ingress metadata (-want, +got):", cmp.Diff(wantDefaultMeta, defaultIng.ObjectMeta)) + } + + wantDefaultRules := []netv1alpha1.IngressRule{{ + Hosts: []string{ + "test-route." + ns, + "test-route." + ns + ".svc", + pkgnet.GetServiceHostname("test-route", ns), + }, + HTTP: &netv1alpha1.HTTPIngressRuleValue{ + Paths: []netv1alpha1.HTTPIngressPath{{ + Splits: []netv1alpha1.IngressBackendSplit{{ + IngressBackend: netv1alpha1.IngressBackend{ + ServiceNamespace: ns, + ServiceName: "rune-01911", + ServicePort: intstr.FromInt(80), + }, + Percent: 1, + AppendHeaders: map[string]string{ + "Knative-Serving-Revision": "rune-01911", + "Knative-Serving-Namespace": ns, + }, + }, { + IngressBackend: netv1alpha1.IngressBackend{ + ServiceNamespace: ns, + ServiceName: "valhalla-01981", + ServicePort: intstr.FromInt(80), + }, + Percent: 41, + AppendHeaders: map[string]string{ + "Knative-Serving-Revision": "valhalla-01981", + "Knative-Serving-Namespace": ns, + }, + }, { + IngressBackend: netv1alpha1.IngressBackend{ + ServiceNamespace: ns, + ServiceName: "valhalla-01982", + ServicePort: intstr.FromInt(80), + }, + Percent: 68, + AppendHeaders: map[string]string{ + "Knative-Serving-Revision": "valhalla-01982", + "Knative-Serving-Namespace": ns, + }, + }}, + }}, + }, + Visibility: netv1alpha1.IngressVisibilityClusterLocal, + }, { + Hosts: []string{ + "test-route." + ns + ".example.com", + }, + HTTP: &netv1alpha1.HTTPIngressRuleValue{ + Paths: []netv1alpha1.HTTPIngressPath{{ + Splits: []netv1alpha1.IngressBackendSplit{{ + IngressBackend: netv1alpha1.IngressBackend{ + ServiceNamespace: ns, + ServiceName: "rune-01911", + ServicePort: intstr.FromInt(80), + }, + Percent: 1, + AppendHeaders: map[string]string{ + "Knative-Serving-Revision": "rune-01911", + "Knative-Serving-Namespace": ns, + }, + }, { + IngressBackend: netv1alpha1.IngressBackend{ + ServiceNamespace: ns, + ServiceName: "valhalla-01981", + ServicePort: intstr.FromInt(80), + }, + Percent: 41, + AppendHeaders: map[string]string{ + "Knative-Serving-Revision": "valhalla-01981", + "Knative-Serving-Namespace": ns, + }, + }, { + IngressBackend: netv1alpha1.IngressBackend{ + ServiceNamespace: ns, + ServiceName: "valhalla-01982", + ServicePort: intstr.FromInt(80), + }, + Percent: 68, + AppendHeaders: map[string]string{ + "Knative-Serving-Revision": "valhalla-01982", + "Knative-Serving-Namespace": ns, + }, + }}, + }}, + }, + Visibility: netv1alpha1.IngressVisibilityExternalIP, + }} + if got, want := defaultIng.Spec.Rules, wantDefaultRules; !cmp.Equal(got, want) { + t.Errorf("Default ingress rules mismatch: diff(-want,+got)\n%s", cmp.Diff(want, got)) + } + + wantHammerRules := []netv1alpha1.IngressRule{{ + Hosts: []string{ + "hammer-test-route." + ns, + "hammer-test-route." + ns + ".svc", + pkgnet.GetServiceHostname("hammer-test-route", ns), + }, + HTTP: &netv1alpha1.HTTPIngressRuleValue{ + Paths: []netv1alpha1.HTTPIngressPath{{ + Splits: []netv1alpha1.IngressBackendSplit{{ + IngressBackend: netv1alpha1.IngressBackend{ + ServiceNamespace: ns, + ServiceName: "thor-02018", + ServicePort: intstr.FromInt(80), + }, + Percent: 60, + AppendHeaders: map[string]string{ + "Knative-Serving-Revision": "thor-02018", + "Knative-Serving-Namespace": ns, + }, + }, { + IngressBackend: netv1alpha1.IngressBackend{ + ServiceNamespace: ns, + ServiceName: "thor-02019", + ServicePort: intstr.FromInt(80), + }, + Percent: 15, + AppendHeaders: map[string]string{ + "Knative-Serving-Revision": "thor-02019", + "Knative-Serving-Namespace": ns, + }, + }, { + IngressBackend: netv1alpha1.IngressBackend{ + ServiceNamespace: ns, + ServiceName: "thor-02020", + ServicePort: intstr.FromInt(80), + }, + Percent: 5, + AppendHeaders: map[string]string{ + "Knative-Serving-Revision": "thor-02020", + "Knative-Serving-Namespace": ns, + }, + }, { + IngressBackend: netv1alpha1.IngressBackend{ + ServiceNamespace: ns, + ServiceName: "thor-beta", + ServicePort: intstr.FromInt(80), + }, + Percent: 20, + AppendHeaders: map[string]string{ + "Knative-Serving-Revision": "thor-beta", + "Knative-Serving-Namespace": ns, + }, + }}, + }}, + }, + Visibility: netv1alpha1.IngressVisibilityClusterLocal, + }, { + Hosts: []string{ + "hammer-test-route." + ns + ".example.com", + }, + HTTP: &netv1alpha1.HTTPIngressRuleValue{ + Paths: []netv1alpha1.HTTPIngressPath{{ + Splits: []netv1alpha1.IngressBackendSplit{{ + IngressBackend: netv1alpha1.IngressBackend{ + ServiceNamespace: ns, + ServiceName: "thor-02018", + ServicePort: intstr.FromInt(80), + }, + Percent: 60, + AppendHeaders: map[string]string{ + "Knative-Serving-Revision": "thor-02018", + "Knative-Serving-Namespace": ns, + }, + }, { + IngressBackend: netv1alpha1.IngressBackend{ + ServiceNamespace: ns, + ServiceName: "thor-02019", + ServicePort: intstr.FromInt(80), + }, + Percent: 15, + AppendHeaders: map[string]string{ + "Knative-Serving-Revision": "thor-02019", + "Knative-Serving-Namespace": ns, + }, + }, { + IngressBackend: netv1alpha1.IngressBackend{ + ServiceNamespace: ns, + ServiceName: "thor-02020", + ServicePort: intstr.FromInt(80), + }, + Percent: 5, + AppendHeaders: map[string]string{ + "Knative-Serving-Revision": "thor-02020", + "Knative-Serving-Namespace": ns, + }, + }, { + IngressBackend: netv1alpha1.IngressBackend{ + ServiceNamespace: ns, + ServiceName: "thor-beta", + ServicePort: intstr.FromInt(80), + }, + Percent: 20, + AppendHeaders: map[string]string{ + "Knative-Serving-Revision": "thor-beta", + "Knative-Serving-Namespace": ns, + }, + }}, + }}, + }, + Visibility: netv1alpha1.IngressVisibilityExternalIP, + }} + if got, want := hammerIng.Spec.Rules, wantHammerRules; !cmp.Equal(got, want) { + t.Errorf("Hammer ingress rules mismatch: diff(-want,+got)\n%s", cmp.Diff(want, got)) + } +} + +func TestMakeDefaultIngressWithRollout(t *testing.T) { + const ingressClass = "ng-ingress" + ro := &traffic.Rollout{ + Configurations: []*traffic.ConfigurationRollout{{ + ConfigurationName: "rune", + Percent: 1, + Revisions: []traffic.RevisionRollout{{ + RevisionName: "rune-01911", + Percent: 1, + }}, + }, { + ConfigurationName: "valhalla", + Percent: 99, + Revisions: []traffic.RevisionRollout{{ + RevisionName: "valhalla-01981", + Percent: 41, + }, { + RevisionName: "valhalla-01982", + Percent: 68, + }}, + }, { + ConfigurationName: "rune", + Percent: 1, + Revisions: []traffic.RevisionRollout{{ + RevisionName: "rune-01911", + Percent: 1, + }}, + }, { + ConfigurationName: "thor", + Tag: "hammer", + Percent: 80, + Revisions: []traffic.RevisionRollout{{ + RevisionName: "thor-02018", + Percent: 60, + }, { + RevisionName: "thor-02019", + Percent: 15, + }, { + RevisionName: "thor-02020", + Percent: 5, + }}, + }}, + } + cfg := &traffic.Config{ + Targets: map[string]traffic.RevisionTargets{ + "hammer": {{ + TrafficTarget: v1.TrafficTarget{ + ConfigurationName: "thor", + LatestRevision: ptr.Bool(true), + Percent: ptr.Int64(80), + RevisionName: "thor-02020", + }, + }, { + TrafficTarget: v1.TrafficTarget{ + ConfigurationName: "thor", + LatestRevision: ptr.Bool(false), + Percent: ptr.Int64(20), + RevisionName: "thor-beta", + }, + }}, + traffic.DefaultTarget: {{ + TrafficTarget: v1.TrafficTarget{ + ConfigurationName: "rune", + LatestRevision: ptr.Bool(true), + Percent: ptr.Int64(1), + RevisionName: "rune-01911", + }, + }, { + TrafficTarget: v1.TrafficTarget{ + ConfigurationName: "valhalla", + LatestRevision: ptr.Bool(true), + Percent: ptr.Int64(99), + RevisionName: "valhalla-01982", + }, + }}, + }, + } + r := Route(ns, "test-route", WithRouteAnnotation(map[string]string{ + networking.IngressClassAnnotationKey: ingressClass, + }), WithRouteUID("1234-5678"), WithURL) + + ing, err := MakeDefaultIngressWithRollout(testContext(), r, cfg, ro, nil /*tls*/, ingressClass) if err != nil { - t.Error("Unexpected error", err) + t.Fatal("Unexpected error", err) } + // Check default ingress metadata + // Default ingress now includes full rollout (all configurations including tagged ones). + wantMeta := metav1.ObjectMeta{ + Name: "test-route", + Namespace: ns, + Labels: map[string]string{ + serving.RouteLabelKey: "test-route", + serving.RouteNamespaceLabelKey: ns, + }, + Annotations: map[string]string{ + networking.IngressClassAnnotationKey: ingressClass, + networking.RolloutAnnotationKey: serializeRollout(context.Background(), ro), + }, + OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(r)}, + } if !cmp.Equal(wantMeta, ing.ObjectMeta) { - t.Error("Unexpected metadata (-want, +got):", cmp.Diff(wantMeta, ing.ObjectMeta)) + t.Error("Unexpected default ingress metadata (-want, +got):", cmp.Diff(wantMeta, ing.ObjectMeta)) } + + // Check default ingress rules (rollout splits for valhalla) wantRules := []netv1alpha1.IngressRule{{ Hosts: []string{ "test-route." + ns, @@ -343,8 +708,83 @@ func TestMakeIngressWithActualRollout(t *testing.T) { }}, }}, }, - Visibility: netv1alpha1.IngressVisibilityExternalIP, - }, { + Visibility: netv1alpha1.IngressVisibilityExternalIP, + }} + if got, want := ing.Spec.Rules, wantRules; !cmp.Equal(got, want) { + t.Errorf("Default ingress rules mismatch: diff(-want,+got)\n%s", cmp.Diff(want, got)) + } +} + +func TestMakeRouteTagIngress(t *testing.T) { + const ingressClass = "ng-ingress" + cfg := &traffic.Config{ + Targets: map[string]traffic.RevisionTargets{ + "hammer": {{ + TrafficTarget: v1.TrafficTarget{ + ConfigurationName: "thor", + LatestRevision: ptr.Bool(true), + Percent: ptr.Int64(80), + RevisionName: "thor-02020", + }, + }, { + TrafficTarget: v1.TrafficTarget{ + ConfigurationName: "thor", + LatestRevision: ptr.Bool(false), + Percent: ptr.Int64(20), + RevisionName: "thor-beta", + }, + }}, + traffic.DefaultTarget: {{ + TrafficTarget: v1.TrafficTarget{ + ConfigurationName: "rune", + LatestRevision: ptr.Bool(true), + Percent: ptr.Int64(1), + RevisionName: "rune-01911", + }, + }, { + TrafficTarget: v1.TrafficTarget{ + ConfigurationName: "valhalla", + LatestRevision: ptr.Bool(true), + Percent: ptr.Int64(99), + RevisionName: "valhalla-01982", + }, + }}, + }, + } + r := Route(ns, "test-route", WithRouteAnnotation(map[string]string{ + networking.IngressClassAnnotationKey: ingressClass, + }), WithRouteUID("1234-5678"), WithURL) + + ing, err := MakeRouteTagIngress(testContext(), r, cfg, "hammer", nil /*tls*/, ingressClass) + if err != nil { + t.Fatal("Unexpected error", err) + } + + // Check hammer tag ingress metadata. + // Tag ingresses no longer carry rollout annotations; rollout state is only + // stored on the default ingress. + wantMeta := metav1.ObjectMeta{ + Name: "test-route-hammer", + Namespace: ns, + Labels: map[string]string{ + serving.RouteLabelKey: "test-route", + serving.RouteNamespaceLabelKey: ns, + networking.TagLabelKey: "hammer", + }, + Annotations: map[string]string{ + networking.IngressClassAnnotationKey: ingressClass, + }, + OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(r)}, + } + if !cmp.Equal(wantMeta, ing.ObjectMeta) { + t.Error("Unexpected hammer ingress metadata (-want, +got):", cmp.Diff(wantMeta, ing.ObjectMeta)) + } + + // Check hammer tag ingress rules. + // Since BuildRollout produces only 1 revision for thor (len < 2), makeBaseIngressPath + // uses the target directly rather than rollout splits. So thor-02020 gets 80% and + // thor-beta gets 20% as simple splits (no multi-revision rollout expansion). + wantRules := []netv1alpha1.IngressRule{{ Hosts: []string{ "hammer-test-route." + ns, "hammer-test-route." + ns + ".svc", @@ -353,34 +793,12 @@ func TestMakeIngressWithActualRollout(t *testing.T) { HTTP: &netv1alpha1.HTTPIngressRuleValue{ Paths: []netv1alpha1.HTTPIngressPath{{ Splits: []netv1alpha1.IngressBackendSplit{{ - IngressBackend: netv1alpha1.IngressBackend{ - ServiceNamespace: ns, - ServiceName: "thor-02018", - ServicePort: intstr.FromInt(80), - }, - Percent: 60, - AppendHeaders: map[string]string{ - "Knative-Serving-Revision": "thor-02018", - "Knative-Serving-Namespace": ns, - }, - }, { - IngressBackend: netv1alpha1.IngressBackend{ - ServiceNamespace: ns, - ServiceName: "thor-02019", - ServicePort: intstr.FromInt(80), - }, - Percent: 15, - AppendHeaders: map[string]string{ - "Knative-Serving-Revision": "thor-02019", - "Knative-Serving-Namespace": ns, - }, - }, { IngressBackend: netv1alpha1.IngressBackend{ ServiceNamespace: ns, ServiceName: "thor-02020", ServicePort: intstr.FromInt(80), }, - Percent: 5, + Percent: 80, AppendHeaders: map[string]string{ "Knative-Serving-Revision": "thor-02020", "Knative-Serving-Namespace": ns, @@ -407,34 +825,12 @@ func TestMakeIngressWithActualRollout(t *testing.T) { HTTP: &netv1alpha1.HTTPIngressRuleValue{ Paths: []netv1alpha1.HTTPIngressPath{{ Splits: []netv1alpha1.IngressBackendSplit{{ - IngressBackend: netv1alpha1.IngressBackend{ - ServiceNamespace: ns, - ServiceName: "thor-02018", - ServicePort: intstr.FromInt(80), - }, - Percent: 60, - AppendHeaders: map[string]string{ - "Knative-Serving-Revision": "thor-02018", - "Knative-Serving-Namespace": ns, - }, - }, { - IngressBackend: netv1alpha1.IngressBackend{ - ServiceNamespace: ns, - ServiceName: "thor-02019", - ServicePort: intstr.FromInt(80), - }, - Percent: 15, - AppendHeaders: map[string]string{ - "Knative-Serving-Revision": "thor-02019", - "Knative-Serving-Namespace": ns, - }, - }, { IngressBackend: netv1alpha1.IngressBackend{ ServiceNamespace: ns, ServiceName: "thor-02020", ServicePort: intstr.FromInt(80), }, - Percent: 5, + Percent: 80, AppendHeaders: map[string]string{ "Knative-Serving-Revision": "thor-02020", "Knative-Serving-Namespace": ns, @@ -456,21 +852,118 @@ func TestMakeIngressWithActualRollout(t *testing.T) { Visibility: netv1alpha1.IngressVisibilityExternalIP, }} if got, want := ing.Spec.Rules, wantRules; !cmp.Equal(got, want) { - t.Errorf("Rules mismatch: diff(-want,+got)\n%s", cmp.Diff(want, got)) + t.Errorf("Hammer ingress rules mismatch: diff(-want,+got)\n%s", cmp.Diff(want, got)) + } +} + +func TestMakeRouteTagIngressRejectsDefaultTarget(t *testing.T) { + const ingressClass = "ng-ingress" + cfg := &traffic.Config{ + Targets: map[string]traffic.RevisionTargets{ + traffic.DefaultTarget: {{ + TrafficTarget: v1.TrafficTarget{ + ConfigurationName: "config", + RevisionName: "v1", + Percent: ptr.Int64(100), + }, + }}, + }, + } + r := Route(ns, "test-route", WithRouteAnnotation(map[string]string{ + networking.IngressClassAnnotationKey: ingressClass, + }), WithRouteUID("1234-5678"), WithURL) + + _, err := MakeRouteTagIngress(testContext(), r, cfg, traffic.DefaultTarget, nil /*tls*/, ingressClass) + if err == nil { + t.Fatal("Expected error when calling MakeRouteTagIngress with DefaultTarget, got nil") + } + if !strings.Contains(err.Error(), "cannot create per-tag ingress for default target") { + t.Errorf("Unexpected error message: %v", err) + } +} + +func TestMakeDefaultIngressWithRolloutMetadata(t *testing.T) { + const ingressClass = "ng-ingress" + + cfg := &traffic.Config{ + Targets: map[string]traffic.RevisionTargets{ + "tagged": {{ + TrafficTarget: v1.TrafficTarget{ + ConfigurationName: "thor", + LatestRevision: ptr.Bool(true), + Percent: ptr.Int64(100), + RevisionName: "thor-02020", + }, + }}, + traffic.DefaultTarget: {{ + TrafficTarget: v1.TrafficTarget{ + ConfigurationName: "valhalla", + LatestRevision: ptr.Bool(true), + Percent: ptr.Int64(100), + RevisionName: "valhalla-01982", + }, + }}, + }, + } + r := Route(ns, "test-route", WithRouteLabel(map[string]string{ + serving.RouteLabelKey: "try-to-override", + serving.RouteNamespaceLabelKey: "try-to-override", + "test-label": "foo", + }), WithRouteAnnotation(map[string]string{ + networking.IngressClassAnnotationKey: ingressClass, + "test-annotation": "bar", + }), WithRouteUID("1234-5678"), WithURL) + + // Use MakeIngress to derive the rollout (same as TestMakeIngressWithTaggedRollout) + ro := cfg.BuildRollout() + + ing, err := MakeDefaultIngressWithRollout(testContext(), r, cfg, ro, nil, ingressClass) + if err != nil { + t.Fatal("Unexpected error", err) + } + + wantMeta := metav1.ObjectMeta{ + Name: "test-route", + Namespace: ns, + Labels: map[string]string{ + serving.RouteLabelKey: "test-route", + serving.RouteNamespaceLabelKey: ns, + "test-label": "foo", + }, + Annotations: map[string]string{ + networking.IngressClassAnnotationKey: ingressClass, + networking.RolloutAnnotationKey: serializeRollout(context.Background(), ro), + "test-annotation": "bar", + }, + OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(r)}, + } + if !cmp.Equal(wantMeta, ing.ObjectMeta) { + t.Error("Unexpected default ingress metadata (-want, +got):", cmp.Diff(wantMeta, ing.ObjectMeta)) } } func TestIngressNoKubectlAnnotation(t *testing.T) { - targets := map[string]traffic.RevisionTargets{} + targets := map[string]traffic.RevisionTargets{ + traffic.DefaultTarget: {{ + TrafficTarget: v1.TrafficTarget{ + ConfigurationName: "config", + RevisionName: "v1", + Percent: ptr.Int64(100), + }, + }}, + } r := Route(ns, testRouteName, WithRouteAnnotation(map[string]string{ networking.IngressClassAnnotationKey: testIngressClass, corev1.LastAppliedConfigAnnotation: testAnnotationValue, }), WithRouteUID("1234-5678"), WithURL) - ing, err := MakeIngress(testContext(), r, &traffic.Config{Targets: targets}, nil, testIngressClass) + ingresses, err := MakeIngress(testContext(), r, &traffic.Config{Targets: targets}, nil, testIngressClass) if err != nil { t.Error("Unexpected error", err) } - if v, ok := ing.Annotations[corev1.LastAppliedConfigAnnotation]; ok { + if len(ingresses) == 0 { + t.Fatal("Expected at least one ingress, got none") + } + if v, ok := ingresses[0].Annotations[corev1.LastAppliedConfigAnnotation]; ok { t.Errorf("Annotation %s = %q, want empty", corev1.LastAppliedConfigAnnotation, v) } } @@ -586,14 +1079,18 @@ func TestMakeIngressSpecCorrectRules(t *testing.T) { }} tc := &traffic.Config{Targets: targets} - ro := tc.BuildRollout() - ci, err := makeIngressSpec(testContext(), r, nil /*tls*/, tc, ro) + ingresses, err := MakeIngress(testContext(), r, tc, nil /*tls*/, "" /*ingressClass*/) if err != nil { t.Error("Unexpected error", err) } - if !cmp.Equal(expected, ci.Rules) { - t.Error("Unexpected rules (-want, +got):", cmp.Diff(expected, ci.Rules)) + var allRules []netv1alpha1.IngressRule + for _, ing := range ingresses { + allRules = append(allRules, ing.Spec.Rules...) + } + + if !cmp.Equal(expected, allRules) { + t.Error("Unexpected rules (-want, +got):", cmp.Diff(expected, allRules)) } } @@ -661,15 +1158,18 @@ func TestMakeIngressSpecCorrectRuleVisibility(t *testing.T) { Targets: c.targets, Visibility: c.serviceVisibility, } - ro := tc.BuildRollout() - ci, err := makeIngressSpec(testContext(), c.route, nil /*tls*/, tc, ro) + ingresses, err := MakeIngress(testContext(), c.route, tc, nil /*tls*/, "" /*ingressClass*/) if err != nil { t.Error("Unexpected error", err) } - if len(c.expectedVisibility) != len(ci.Rules) { - t.Errorf("|rules| = %d, want: %d", len(ci.Rules), len(c.expectedVisibility)) + var allRules []netv1alpha1.IngressRule + for _, ing := range ingresses { + allRules = append(allRules, ing.Spec.Rules...) } - for _, rule := range ci.Rules { + if len(c.expectedVisibility) != len(allRules) { + t.Errorf("|rules| = %d, want: %d", len(allRules), len(c.expectedVisibility)) + } + for _, rule := range allRules { visibility := rule.Visibility if !cmp.Equal(c.expectedVisibility[visibility], rule.Hosts) { t.Errorf("Hosts for visibility[%s] = %v, want: %v", visibility, rule.Hosts, c.expectedVisibility) @@ -699,7 +1199,10 @@ func TestMakeIngressSpecCorrectRulesWithTagBasedRouting(t *testing.T) { r := Route(ns, "test-route", WithURL) - expected := []netv1alpha1.IngressRule{{ + // With per-tag ingresses, the default ingress has default-route rules + // and the v1 ingress has v1-tag rules. Each ingress's rules get the + // tag-based routing header appended independently. + expectedDefaultRules := []netv1alpha1.IngressRule{{ Hosts: []string{ "test-route." + ns, "test-route." + ns + ".svc", @@ -708,9 +1211,7 @@ func TestMakeIngressSpecCorrectRulesWithTagBasedRouting(t *testing.T) { HTTP: &netv1alpha1.HTTPIngressRuleValue{ Paths: []netv1alpha1.HTTPIngressPath{{ Headers: map[string]netv1alpha1.HeaderMatch{ - netheader.RouteTagKey: { - Exact: "v1", - }, + netheader.RouteTagKey: {Exact: "v1"}, }, Splits: []netv1alpha1.IngressBackendSplit{{ IngressBackend: netv1alpha1.IngressBackend{ @@ -750,9 +1251,7 @@ func TestMakeIngressSpecCorrectRulesWithTagBasedRouting(t *testing.T) { HTTP: &netv1alpha1.HTTPIngressRuleValue{ Paths: []netv1alpha1.HTTPIngressPath{{ Headers: map[string]netv1alpha1.HeaderMatch{ - netheader.RouteTagKey: { - Exact: "v1", - }, + netheader.RouteTagKey: {Exact: "v1"}, }, Splits: []netv1alpha1.IngressBackendSplit{{ IngressBackend: netv1alpha1.IngressBackend{ @@ -785,7 +1284,9 @@ func TestMakeIngressSpecCorrectRulesWithTagBasedRouting(t *testing.T) { }}, }, Visibility: netv1alpha1.IngressVisibilityExternalIP, - }, { + }} + + expectedV1Rules := []netv1alpha1.IngressRule{{ Hosts: []string{ "v1-test-route." + ns, "v1-test-route." + ns + ".svc", @@ -841,14 +1342,21 @@ func TestMakeIngressSpecCorrectRulesWithTagBasedRouting(t *testing.T) { config.FromContext(ctx).Features.TagHeaderBasedRouting = apicfg.Enabled tc := &traffic.Config{Targets: targets} - ro := tc.BuildRollout() - ci, err := makeIngressSpec(ctx, r, nil /*tls*/, tc, ro) + ingresses, err := MakeIngress(ctx, r, tc, nil /*tls*/, "" /*ingressClass*/) if err != nil { t.Error("Unexpected error", err) } - if !cmp.Equal(expected, ci.Rules) { - t.Error("Unexpected rules (-want, +got):", cmp.Diff(expected, ci.Rules)) + if len(ingresses) != 2 { + t.Fatalf("Expected 2 ingresses, got %d", len(ingresses)) + } + + // Ingresses sorted by tag: "" (default) first, then "v1" + if !cmp.Equal(expectedDefaultRules, ingresses[0].Spec.Rules) { + t.Error("Unexpected default ingress rules (-want, +got):", cmp.Diff(expectedDefaultRules, ingresses[0].Spec.Rules)) + } + if !cmp.Equal(expectedV1Rules, ingresses[1].Spec.Rules) { + t.Error("Unexpected v1 ingress rules (-want, +got):", cmp.Diff(expectedV1Rules, ingresses[1].Spec.Rules)) } } @@ -1029,44 +1537,50 @@ func TestMakeIngressRuleTwoTargets(t *testing.T) { } func TestMakeIngressWithTLS(t *testing.T) { - targets := map[string]traffic.RevisionTargets{} + targets := map[string]traffic.RevisionTargets{ + traffic.DefaultTarget: {{ + TrafficTarget: v1.TrafficTarget{ + ConfigurationName: "config", + RevisionName: "v1", + Percent: ptr.Int64(100), + }, + }}, + } ingressClass := "foo-ingress" r := Route(ns, "test-route", WithRouteUID("1234-5678"), WithURL) tls := []netv1alpha1.IngressTLS{{ - Hosts: []string{"*.default.domain.com"}, + Hosts: []string{"test-route." + ns + ".example.com"}, SecretName: "secret", }} - expected := &netv1alpha1.Ingress{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-route", - Namespace: ns, - Annotations: map[string]string{ - networking.IngressClassAnnotationKey: ingressClass, - networking.RolloutAnnotationKey: emptyRollout, - }, - Labels: map[string]string{ - serving.RouteLabelKey: "test-route", - serving.RouteNamespaceLabelKey: ns, - }, - OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(r)}, - }, - Spec: netv1alpha1.IngressSpec{ - Rules: []netv1alpha1.IngressRule{}, - TLS: tls, - }, - } - got, err := MakeIngress(testContext(), r, &traffic.Config{Targets: targets}, tls, ingressClass) + ingresses, err := MakeIngress(testContext(), r, &traffic.Config{Targets: targets}, tls, ingressClass) if err != nil { t.Error("Unexpected error:", err) } - if diff := cmp.Diff(expected, got); diff != "" { - t.Error("Unexpected metadata (-want, +got):", diff) + if len(ingresses) == 0 { + t.Fatal("Expected at least one ingress, got none") + } + + got := ingresses[0] + // Check that TLS is present in the ingress spec + if len(got.Spec.TLS) == 0 { + t.Error("Expected TLS entries in ingress spec, got none") + } + if diff := cmp.Diff(tls, got.Spec.TLS); diff != "" { + t.Error("Unexpected TLS (-want, +got):", diff) } } func TestMakeIngressWithHTTPOption(t *testing.T) { - targets := map[string]traffic.RevisionTargets{} + targets := map[string]traffic.RevisionTargets{ + traffic.DefaultTarget: {{ + TrafficTarget: v1.TrafficTarget{ + ConfigurationName: "config", + RevisionName: "v1", + Percent: ptr.Int64(100), + }, + }}, + } ingressClass := "foo-ingress" tls := []netv1alpha1.IngressTLS{{ Hosts: []string{"*.default.domain.com"}, @@ -1102,7 +1616,7 @@ func TestMakeIngressWithHTTPOption(t *testing.T) { if tc.wantError { return } - if diff := cmp.Diff(tc.wantOption, got.Spec.HTTPOption); diff != "" { + if diff := cmp.Diff(tc.wantOption, got[0].Spec.HTTPOption); diff != "" { t.Error("Unexpected Ingress (-want, +got):", diff) } }) @@ -1220,14 +1734,18 @@ func TestMakeIngressWithActivatorCA(t *testing.T) { }} tc := &traffic.Config{Targets: targets} - ro := tc.BuildRollout() - ci, err := makeIngressSpec(testContextWithActivatorCA(), r, nil /*tls*/, tc, ro) + ingresses, err := MakeIngress(testContextWithActivatorCA(), r, tc, nil /*tls*/, "" /*ingressClass*/) if err != nil { t.Error("Unexpected error", err) } - if !cmp.Equal(expected, ci.Rules) { - t.Error("Unexpected rules (-want, +got):", cmp.Diff(expected, ci.Rules)) + var allRules []netv1alpha1.IngressRule + for _, ing := range ingresses { + allRules = append(allRules, ing.Spec.Rules...) + } + + if !cmp.Equal(expected, allRules) { + t.Error("Unexpected rules (-want, +got):", cmp.Diff(expected, allRules)) } } @@ -1350,15 +1868,19 @@ func TestMakeIngressACMEChallenges(t *testing.T) { tc := &traffic.Config{ Targets: targets, } - ro := tc.BuildRollout() - ci, err := makeIngressSpec(testContext(), r, nil /*tls*/, tc, ro, acmeChallenge) + ingresses, err := MakeIngress(testContext(), r, tc, nil /*tls*/, "" /*ingressClass*/, acmeChallenge) if err != nil { t.Error("Unexpected error", err) } - if !cmp.Equal(expected, ci.Rules) { - t.Error("Unexpected rules (-want, +got):", cmp.Diff(expected, ci.Rules)) + var allRules []netv1alpha1.IngressRule + for _, ing := range ingresses { + allRules = append(allRules, ing.Spec.Rules...) + } + + if !cmp.Equal(expected, allRules) { + t.Error("Unexpected rules (-want, +got):", cmp.Diff(expected, allRules)) } } @@ -1440,22 +1962,23 @@ func TestMakeIngressACMEChallengesWithTrafficTags(t *testing.T) { tc := &traffic.Config{ Targets: targets, } - ro := tc.BuildRollout() - ci, err := makeIngressSpec(testContext(), r, nil /*tls*/, tc, ro, acmeChallenges...) + ingresses, err := MakeIngress(testContext(), r, tc, nil /*tls*/, "" /*ingressClass*/, acmeChallenges...) if err != nil { t.Fatal("Unexpected error", err) } // Verify each ACME challenge path appears in exactly one rule acmePathsSeen := make(map[string]int) // path -> count across all rules - for _, rule := range ci.Rules { - if rule.Visibility != netv1alpha1.IngressVisibilityExternalIP || rule.HTTP == nil { - continue - } - for _, path := range rule.HTTP.Paths { - if path.Path != "" && len(path.Path) > 0 { - acmePathsSeen[path.Path]++ + for _, ing := range ingresses { + for _, rule := range ing.Spec.Rules { + if rule.Visibility != netv1alpha1.IngressVisibilityExternalIP || rule.HTTP == nil { + continue + } + for _, path := range rule.HTTP.Paths { + if path.Path != "" && len(path.Path) > 0 { + acmePathsSeen[path.Path]++ + } } } } @@ -1540,6 +2063,44 @@ func TestMakeIngressFailToGenerateTagHost(t *testing.T) { } } +func TestDesiredIngressNames(t *testing.T) { + r := Route(ns, testRouteName) + + tests := []struct { + name string + targets map[string]traffic.RevisionTargets + expected sets.Set[string] + }{{ + name: "default and named tags", + targets: map[string]traffic.RevisionTargets{ + traffic.DefaultTarget: {}, + "canary": {}, + "blue": {}, + }, + expected: sets.New(testRouteName, testRouteName+"-canary", testRouteName+"-blue"), + }, { + name: "empty targets", + targets: map[string]traffic.RevisionTargets{}, + expected: sets.New(testRouteName), + }, { + name: "default target only", + targets: map[string]traffic.RevisionTargets{ + traffic.DefaultTarget: {}, + }, + expected: sets.New(testRouteName), + }} + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tc := &traffic.Config{Targets: tt.targets} + got := DesiredIngressNames(r, tc) + if !got.Equal(tt.expected) { + t.Errorf("DesiredIngressNames() = %v, want %v", sets.List(got), sets.List(tt.expected)) + } + }) + } +} + func testContext() context.Context { ctx := context.Background() cfg := testConfig() diff --git a/pkg/reconciler/route/resources/names/names.go b/pkg/reconciler/route/resources/names/names.go index 32e4abe4c441..5fd20ae48e28 100644 --- a/pkg/reconciler/route/resources/names/names.go +++ b/pkg/reconciler/route/resources/names/names.go @@ -37,6 +37,16 @@ func Ingress(route kmeta.Accessor) string { return kmeta.ChildName(route.GetName(), "") } +// TaggedIngress returns the name for the per-tag Ingress +// child resource for the given Route and tag name. +// For the default (untagged) target, it returns the same name as Ingress(). +func TaggedIngress(route kmeta.Accessor, tag string) string { + if tag == "" { + return Ingress(route) + } + return kmeta.ChildName(route.GetName(), "-"+tag) +} + // Certificate returns the name for the Certificate // child resource for the given Route. func Certificate(route kmeta.Accessor) string { diff --git a/pkg/reconciler/route/route.go b/pkg/reconciler/route/route.go index d6be181c957e..d49097f0af10 100644 --- a/pkg/reconciler/route/route.go +++ b/pkg/reconciler/route/route.go @@ -161,21 +161,42 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, r *v1.Route) pkgreconcil r.Status.MarkTLSNotEnabled(v1.TLSNotEnabledForClusterLocalMessage) } - // Reconcile ingress and its children resources. - ingress, effectiveRO, err := c.reconcileIngress(ctx, r, traffic, tls, ingressClassForRoute(ctx, r), acmeChallenges...) + // Reconcile per-tag ingresses and their children resources. + ingresses, effectiveRO, err := c.reconcileIngresses(ctx, r, traffic, tls, ingressClassForRoute(ctx, r), acmeChallenges...) if err != nil { return err } roInProgress := !effectiveRO.Done() - if ingress.GetObjectMeta().GetGeneration() != ingress.Status.ObservedGeneration { + allConfigured := true + for _, ing := range ingresses { + if ing.GetObjectMeta().GetGeneration() != ing.Status.ObservedGeneration { + allConfigured = false + break + } + } + if !allConfigured { r.Status.MarkIngressNotConfigured() } else if !roInProgress { - r.Status.PropagateIngressStatus(ingress.Status) + // Propagate status: all ingresses must be ready for the route to be ready. + // Use the first non-ready ingress status, or the first ingress if all ready. + // Note: ingresses is guaranteed non-empty because DesiredIngressNames + // always includes the default ingress. + propagated := false + for _, ing := range ingresses { + if !ing.IsReady() { + r.Status.PropagateIngressStatus(ing.Status) + propagated = true + break + } + } + if !propagated { + r.Status.PropagateIngressStatus(ingresses[0].Status) + } } logger.Info("Updating placeholder k8s services with ingress information") - if err := c.updatePlaceholderServices(ctx, r, services, ingress); err != nil { + if err := c.updatePlaceholderServices(ctx, r, services, ingresses); err != nil { return err } diff --git a/pkg/reconciler/route/route_test.go b/pkg/reconciler/route/route_test.go index 7cc392ea3f39..626cff723739 100644 --- a/pkg/reconciler/route/route_test.go +++ b/pkg/reconciler/route/route_test.go @@ -141,6 +141,16 @@ func newTestSetup(t *testing.T, opts ...reconcilerOption) ( } func getRouteIngressFromClient(ctx context.Context, t *testing.T, route *v1.Route) *v1alpha1.Ingress { + t.Helper() + // Get the default ingress directly by name (route name = default ingress name). + ingress, err := fakenetworkingclient.Get(ctx).NetworkingV1alpha1().Ingresses(route.Namespace).Get(ctx, route.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Ingress.Get(%s/%s) = %v", route.Namespace, route.Name, err) + } + return ingress +} + +func getRouteIngressesFromClient(ctx context.Context, t *testing.T, route *v1.Route) []v1alpha1.Ingress { t.Helper() opts := metav1.ListOptions{ LabelSelector: labels.SelectorFromSet(labels.Set{ @@ -150,14 +160,17 @@ func getRouteIngressFromClient(ctx context.Context, t *testing.T, route *v1.Rout } ingresses, err := fakenetworkingclient.Get(ctx).NetworkingV1alpha1().Ingresses(route.Namespace).List(ctx, opts) if err != nil { - t.Errorf("Ingress.Get(%v) = %v", opts, err) + t.Fatalf("Ingress.List(%v) = %v", opts, err) } + return ingresses.Items +} - if len(ingresses.Items) != 1 { - t.Fatalf("Ingress.Get(%v), expect 1 instance, but got %d", opts, len(ingresses.Items)) +func addAllRouteIngressesToIndexer(ctx context.Context, t *testing.T, route *v1.Route) { + t.Helper() + allIngresses := getRouteIngressesFromClient(ctx, t, route) + for i := range allIngresses { + fakeingressinformer.Get(ctx).Informer().GetIndexer().Add(&allIngresses[i]) } - - return &ingresses.Items[0] } func addRouteToInformers(ctx context.Context, t *testing.T, route *v1.Route) { @@ -171,11 +184,7 @@ func addRouteToInformers(ctx context.Context, t *testing.T, route *v1.Route) { } fakerouteinformer.Get(ctx).Informer().GetIndexer().Add(route) - if ci := getRouteIngressFromClient(ctx, t, route); ci != nil { - fakeingressinformer.Get(ctx).Informer().GetIndexer().Add(ci) - } - ingress := getRouteIngressFromClient(ctx, t, route) - fakeingressinformer.Get(ctx).Informer().GetIndexer().Add(ingress) + addAllRouteIngressesToIndexer(ctx, t, route) } // Test the only revision in the route is in Reserve (inactive) serving status. @@ -219,7 +228,6 @@ func TestCreateRouteForOneReserveRevision(t *testing.T) { domain := strings.Join([]string{route.Name, route.Namespace, defaultDomainSuffix}, ".") expectedSpec := v1alpha1.IngressSpec{ HTTPOption: v1alpha1.HTTPOptionEnabled, - TLS: []v1alpha1.IngressTLS{}, Rules: []v1alpha1.IngressRule{{ Hosts: []string{ "test-route.test", @@ -341,7 +349,6 @@ func TestCreateRouteWithMultipleTargets(t *testing.T) { domain := strings.Join([]string{route.Name, route.Namespace, defaultDomainSuffix}, ".") expectedSpec := v1alpha1.IngressSpec{ HTTPOption: v1alpha1.HTTPOptionEnabled, - TLS: []v1alpha1.IngressTLS{}, Rules: []v1alpha1.IngressRule{{ Hosts: []string{ "test-route.test", @@ -459,7 +466,6 @@ func TestCreateRouteWithOneTargetReserve(t *testing.T) { domain := strings.Join([]string{route.Name, route.Namespace, defaultDomainSuffix}, ".") expectedSpec := v1alpha1.IngressSpec{ HTTPOption: v1alpha1.HTTPOptionEnabled, - TLS: []v1alpha1.IngressTLS{}, Rules: []v1alpha1.IngressRule{{ Hosts: []string{ "test-route.test", @@ -591,7 +597,6 @@ func TestCreateRouteWithDuplicateTargets(t *testing.T) { domain := strings.Join([]string{route.Name, route.Namespace, defaultDomainSuffix}, ".") expectedSpec := v1alpha1.IngressSpec{ HTTPOption: v1alpha1.HTTPOptionEnabled, - TLS: []v1alpha1.IngressTLS{}, Rules: []v1alpha1.IngressRule{{ Hosts: []string{ "test-route.test", @@ -658,94 +663,6 @@ func TestCreateRouteWithDuplicateTargets(t *testing.T) { }}, }, Visibility: v1alpha1.IngressVisibilityExternalIP, - }, { - Hosts: []string{ - "test-revision-1-test-route.test", - "test-revision-1-test-route.test.svc", - pkgnet.GetServiceHostname("test-revision-1-test-route", "test"), - }, - HTTP: &v1alpha1.HTTPIngressRuleValue{ - Paths: []v1alpha1.HTTPIngressPath{{ - Splits: []v1alpha1.IngressBackendSplit{{ - IngressBackend: v1alpha1.IngressBackend{ - ServiceNamespace: testNamespace, - ServiceName: "test-rev", - ServicePort: intstr.FromInt(80), - }, - Percent: 100, - AppendHeaders: map[string]string{ - "Knative-Serving-Revision": rev.Name, - "Knative-Serving-Namespace": testNamespace, - }, - }}, - }}, - }, - Visibility: v1alpha1.IngressVisibilityClusterLocal, - }, { - Hosts: []string{ - "test-revision-1-test-route.test.test-domain.dev", - }, - HTTP: &v1alpha1.HTTPIngressRuleValue{ - Paths: []v1alpha1.HTTPIngressPath{{ - Splits: []v1alpha1.IngressBackendSplit{{ - IngressBackend: v1alpha1.IngressBackend{ - ServiceNamespace: testNamespace, - ServiceName: "test-rev", - ServicePort: intstr.FromInt(80), - }, - Percent: 100, - AppendHeaders: map[string]string{ - "Knative-Serving-Revision": rev.Name, - "Knative-Serving-Namespace": testNamespace, - }, - }}, - }}, - }, - Visibility: v1alpha1.IngressVisibilityExternalIP, - }, { - Hosts: []string{ - "test-revision-2-test-route.test", - "test-revision-2-test-route.test.svc", - pkgnet.GetServiceHostname("test-revision-2-test-route", "test"), - }, - HTTP: &v1alpha1.HTTPIngressRuleValue{ - Paths: []v1alpha1.HTTPIngressPath{{ - Splits: []v1alpha1.IngressBackendSplit{{ - IngressBackend: v1alpha1.IngressBackend{ - ServiceNamespace: testNamespace, - ServiceName: "test-rev", - ServicePort: intstr.FromInt(80), - }, - Percent: 100, - AppendHeaders: map[string]string{ - "Knative-Serving-Revision": rev.Name, - "Knative-Serving-Namespace": testNamespace, - }, - }}, - }}, - }, - Visibility: v1alpha1.IngressVisibilityClusterLocal, - }, { - Hosts: []string{ - "test-revision-2-test-route.test.test-domain.dev", - }, - HTTP: &v1alpha1.HTTPIngressRuleValue{ - Paths: []v1alpha1.HTTPIngressPath{{ - Splits: []v1alpha1.IngressBackendSplit{{ - IngressBackend: v1alpha1.IngressBackend{ - ServiceNamespace: testNamespace, - ServiceName: "test-rev", - ServicePort: intstr.FromInt(80), - }, - Percent: 100, - AppendHeaders: map[string]string{ - "Knative-Serving-Revision": rev.Name, - "Knative-Serving-Namespace": testNamespace, - }, - }}, - }}, - }, - Visibility: v1alpha1.IngressVisibilityExternalIP, }}, } @@ -797,7 +714,6 @@ func TestCreateRouteWithNamedTargets(t *testing.T) { domain := strings.Join([]string{route.Name, route.Namespace, defaultDomainSuffix}, ".") expectedSpec := v1alpha1.IngressSpec{ HTTPOption: v1alpha1.HTTPOptionEnabled, - TLS: []v1alpha1.IngressTLS{}, Rules: []v1alpha1.IngressRule{{ Hosts: []string{ "test-route.test", @@ -862,90 +778,6 @@ func TestCreateRouteWithNamedTargets(t *testing.T) { }}, }, Visibility: v1alpha1.IngressVisibilityExternalIP, - }, { - Hosts: []string{ - "bar-test-route.test", - "bar-test-route.test.svc", - pkgnet.GetServiceHostname("bar-test-route", "test"), - }, - HTTP: &v1alpha1.HTTPIngressRuleValue{ - Paths: []v1alpha1.HTTPIngressPath{{ - Splits: []v1alpha1.IngressBackendSplit{{ - IngressBackend: v1alpha1.IngressBackend{ - ServiceNamespace: testNamespace, - ServiceName: cfgrev.Name, - ServicePort: intstr.FromInt(80), - }, - Percent: 100, - AppendHeaders: map[string]string{ - "Knative-Serving-Revision": cfgrev.Name, - "Knative-Serving-Namespace": testNamespace, - }, - }}, - }}, - }, - Visibility: v1alpha1.IngressVisibilityClusterLocal, - }, { - Hosts: []string{"bar-test-route.test.test-domain.dev"}, - HTTP: &v1alpha1.HTTPIngressRuleValue{ - Paths: []v1alpha1.HTTPIngressPath{{ - Splits: []v1alpha1.IngressBackendSplit{{ - IngressBackend: v1alpha1.IngressBackend{ - ServiceNamespace: testNamespace, - ServiceName: cfgrev.Name, - ServicePort: intstr.FromInt(80), - }, - Percent: 100, - AppendHeaders: map[string]string{ - "Knative-Serving-Revision": cfgrev.Name, - "Knative-Serving-Namespace": testNamespace, - }, - }}, - }}, - }, - Visibility: v1alpha1.IngressVisibilityExternalIP, - }, { - Hosts: []string{ - "foo-test-route.test", - "foo-test-route.test.svc", - pkgnet.GetServiceHostname("foo-test-route", "test"), - }, - HTTP: &v1alpha1.HTTPIngressRuleValue{ - Paths: []v1alpha1.HTTPIngressPath{{ - Splits: []v1alpha1.IngressBackendSplit{{ - IngressBackend: v1alpha1.IngressBackend{ - ServiceNamespace: testNamespace, - ServiceName: rev.Name, - ServicePort: intstr.FromInt(80), - }, - Percent: 100, - AppendHeaders: map[string]string{ - "Knative-Serving-Revision": rev.Name, - "Knative-Serving-Namespace": testNamespace, - }, - }}, - }}, - }, - Visibility: v1alpha1.IngressVisibilityClusterLocal, - }, { - Hosts: []string{"foo-test-route.test.test-domain.dev"}, - HTTP: &v1alpha1.HTTPIngressRuleValue{ - Paths: []v1alpha1.HTTPIngressPath{{ - Splits: []v1alpha1.IngressBackendSplit{{ - IngressBackend: v1alpha1.IngressBackend{ - ServiceNamespace: testNamespace, - ServiceName: rev.Name, - ServicePort: intstr.FromInt(80), - }, - Percent: 100, - AppendHeaders: map[string]string{ - "Knative-Serving-Revision": rev.Name, - "Knative-Serving-Namespace": testNamespace, - }, - }}, - }}, - }, - Visibility: v1alpha1.IngressVisibilityExternalIP, }}, } @@ -1006,7 +838,6 @@ func TestCreateRouteWithNamedTargetsAndTagBasedRouting(t *testing.T) { domain := strings.Join([]string{route.Name, route.Namespace, defaultDomainSuffix}, ".") expectedSpec := v1alpha1.IngressSpec{ HTTPOption: v1alpha1.HTTPOptionEnabled, - TLS: []v1alpha1.IngressTLS{}, Rules: []v1alpha1.IngressRule{{ Hosts: []string{ "test-route.test", @@ -1016,117 +847,36 @@ func TestCreateRouteWithNamedTargetsAndTagBasedRouting(t *testing.T) { HTTP: &v1alpha1.HTTPIngressRuleValue{ Paths: []v1alpha1.HTTPIngressPath{{ Headers: map[string]v1alpha1.HeaderMatch{ - netheader.RouteTagKey: { - Exact: "bar", - }, - }, - Splits: []v1alpha1.IngressBackendSplit{ - { - IngressBackend: v1alpha1.IngressBackend{ - ServiceNamespace: "test", - ServiceName: "p-deadbeef", - ServicePort: intstr.IntOrString{IntVal: 80}, - }, - Percent: 100, - AppendHeaders: map[string]string{ - "Knative-Serving-Namespace": "test", - "Knative-Serving-Revision": "p-deadbeef", - }, - }, - }, - }, { - Headers: map[string]v1alpha1.HeaderMatch{ - netheader.RouteTagKey: { - Exact: "foo", - }, - }, - Splits: []v1alpha1.IngressBackendSplit{ - { - IngressBackend: v1alpha1.IngressBackend{ - ServiceNamespace: "test", - ServiceName: "test-rev", - ServicePort: intstr.IntOrString{IntVal: 80}, - }, - Percent: 100, - AppendHeaders: map[string]string{ - "Knative-Serving-Namespace": "test", - "Knative-Serving-Revision": "test-rev", - }, - }, - }, - }, { - AppendHeaders: map[string]string{ - netheader.DefaultRouteKey: "true", + netheader.RouteTagKey: {Exact: "bar"}, }, Splits: []v1alpha1.IngressBackendSplit{{ IngressBackend: v1alpha1.IngressBackend{ ServiceNamespace: testNamespace, - ServiceName: rev.Name, + ServiceName: cfgrev.Name, ServicePort: intstr.FromInt(80), }, - Percent: 50, + Percent: 100, AppendHeaders: map[string]string{ - "Knative-Serving-Revision": rev.Name, + "Knative-Serving-Revision": cfgrev.Name, "Knative-Serving-Namespace": testNamespace, }, - }, { + }}, + }, { + Headers: map[string]v1alpha1.HeaderMatch{ + netheader.RouteTagKey: {Exact: "foo"}, + }, + Splits: []v1alpha1.IngressBackendSplit{{ IngressBackend: v1alpha1.IngressBackend{ ServiceNamespace: testNamespace, - ServiceName: cfgrev.Name, + ServiceName: rev.Name, ServicePort: intstr.FromInt(80), }, - Percent: 50, + Percent: 100, AppendHeaders: map[string]string{ - "Knative-Serving-Revision": cfgrev.Name, + "Knative-Serving-Revision": rev.Name, "Knative-Serving-Namespace": testNamespace, }, }}, - }}, - }, - Visibility: v1alpha1.IngressVisibilityClusterLocal, - }, { - Hosts: []string{domain}, - HTTP: &v1alpha1.HTTPIngressRuleValue{ - Paths: []v1alpha1.HTTPIngressPath{{ - Headers: map[string]v1alpha1.HeaderMatch{ - netheader.RouteTagKey: { - Exact: "bar", - }, - }, - Splits: []v1alpha1.IngressBackendSplit{ - { - IngressBackend: v1alpha1.IngressBackend{ - ServiceNamespace: "test", - ServiceName: "p-deadbeef", - ServicePort: intstr.IntOrString{IntVal: 80}, - }, - Percent: 100, - AppendHeaders: map[string]string{ - "Knative-Serving-Namespace": "test", - "Knative-Serving-Revision": "p-deadbeef", - }, - }, - }, - }, { - Headers: map[string]v1alpha1.HeaderMatch{ - netheader.RouteTagKey: { - Exact: "foo", - }, - }, - Splits: []v1alpha1.IngressBackendSplit{ - { - IngressBackend: v1alpha1.IngressBackend{ - ServiceNamespace: "test", - ServiceName: "test-rev", - ServicePort: intstr.IntOrString{IntVal: 80}, - }, - Percent: 100, - AppendHeaders: map[string]string{ - "Knative-Serving-Namespace": "test", - "Knative-Serving-Revision": "test-rev", - }, - }, - }, }, { AppendHeaders: map[string]string{ netheader.DefaultRouteKey: "true", @@ -1156,15 +906,14 @@ func TestCreateRouteWithNamedTargetsAndTagBasedRouting(t *testing.T) { }}, }}, }, - Visibility: v1alpha1.IngressVisibilityExternalIP, + Visibility: v1alpha1.IngressVisibilityClusterLocal, }, { - Hosts: []string{ - "bar-test-route.test", - "bar-test-route.test.svc", - pkgnet.GetServiceHostname("bar-test-route", "test"), - }, + Hosts: []string{domain}, HTTP: &v1alpha1.HTTPIngressRuleValue{ Paths: []v1alpha1.HTTPIngressPath{{ + Headers: map[string]v1alpha1.HeaderMatch{ + netheader.RouteTagKey: {Exact: "bar"}, + }, Splits: []v1alpha1.IngressBackendSplit{{ IngressBackend: v1alpha1.IngressBackend{ ServiceNamespace: testNamespace, @@ -1177,81 +926,49 @@ func TestCreateRouteWithNamedTargetsAndTagBasedRouting(t *testing.T) { "Knative-Serving-Namespace": testNamespace, }, }}, - AppendHeaders: map[string]string{ - netheader.RouteTagKey: "bar", + }, { + Headers: map[string]v1alpha1.HeaderMatch{ + netheader.RouteTagKey: {Exact: "foo"}, }, - }}, - }, - Visibility: v1alpha1.IngressVisibilityClusterLocal, - }, { - Hosts: []string{ - "bar-test-route.test.test-domain.dev", - }, - HTTP: &v1alpha1.HTTPIngressRuleValue{ - Paths: []v1alpha1.HTTPIngressPath{{ Splits: []v1alpha1.IngressBackendSplit{{ IngressBackend: v1alpha1.IngressBackend{ ServiceNamespace: testNamespace, - ServiceName: cfgrev.Name, + ServiceName: rev.Name, ServicePort: intstr.FromInt(80), }, Percent: 100, AppendHeaders: map[string]string{ - "Knative-Serving-Revision": cfgrev.Name, + "Knative-Serving-Revision": rev.Name, "Knative-Serving-Namespace": testNamespace, }, }}, + }, { AppendHeaders: map[string]string{ - netheader.RouteTagKey: "bar", + netheader.DefaultRouteKey: "true", }, - }}, - }, - Visibility: v1alpha1.IngressVisibilityExternalIP, - }, { - Hosts: []string{ - "foo-test-route.test", - "foo-test-route.test.svc", - pkgnet.GetServiceHostname("foo-test-route", "test"), - }, - HTTP: &v1alpha1.HTTPIngressRuleValue{ - Paths: []v1alpha1.HTTPIngressPath{{ Splits: []v1alpha1.IngressBackendSplit{{ IngressBackend: v1alpha1.IngressBackend{ ServiceNamespace: testNamespace, ServiceName: rev.Name, ServicePort: intstr.FromInt(80), }, - Percent: 100, + Percent: 50, AppendHeaders: map[string]string{ "Knative-Serving-Revision": rev.Name, "Knative-Serving-Namespace": testNamespace, }, - }}, - AppendHeaders: map[string]string{ - netheader.RouteTagKey: "foo", - }, - }}, - }, - Visibility: v1alpha1.IngressVisibilityClusterLocal, - }, { - Hosts: []string{"foo-test-route.test.test-domain.dev"}, - HTTP: &v1alpha1.HTTPIngressRuleValue{ - Paths: []v1alpha1.HTTPIngressPath{{ - Splits: []v1alpha1.IngressBackendSplit{{ + }, { IngressBackend: v1alpha1.IngressBackend{ ServiceNamespace: testNamespace, - ServiceName: rev.Name, + ServiceName: cfgrev.Name, ServicePort: intstr.FromInt(80), }, - Percent: 100, + Percent: 50, AppendHeaders: map[string]string{ - "Knative-Serving-Revision": rev.Name, + "Knative-Serving-Revision": cfgrev.Name, "Knative-Serving-Namespace": testNamespace, }, }}, - AppendHeaders: map[string]string{ - netheader.RouteTagKey: "foo", - }, }}, }, Visibility: v1alpha1.IngressVisibilityExternalIP, diff --git a/pkg/reconciler/route/table_test.go b/pkg/reconciler/route/table_test.go index 96031d136792..79d4e7feb93b 100644 --- a/pkg/reconciler/route/table_test.go +++ b/pkg/reconciler/route/table_test.go @@ -67,6 +67,13 @@ const testIngressClass = "ingress-class-foo" var fakeCurTime = time.Unix(1e9, 0) +func createdIngressEvent(name, tag string) string { + if tag == traffic.DefaultTarget { + tag = "default" + } + return Eventf(corev1.EventTypeNormal, "Created", "Created Ingress %q (tag: %s)", name, tag) +} + type key int const ( @@ -187,7 +194,7 @@ func TestReconcile(t *testing.T) { }}, WantEvents: []string{ Eventf(corev1.EventTypeNormal, "Created", "Created placeholder service %q", "becomes-ready"), - Eventf(corev1.EventTypeNormal, "Created", "Created Ingress %q", "becomes-ready"), + createdIngressEvent("becomes-ready", traffic.DefaultTarget), }, Key: "default/becomes-ready", }, { @@ -230,7 +237,7 @@ func TestReconcile(t *testing.T) { Percent: ptr.Int64(100), LatestRevision: ptr.Bool(true), }), - WithPropagatedStatus(simpleIngress(Route("default", "ingress-failed"), &traffic.Config{}, + WithPropagatedStatus(simpleIngress(Route("default", "ingress-failed"), emptyTrafficConfig(), WithLoadbalancerFailed("TestFailure", "failure")).Status), ), }}, @@ -283,7 +290,7 @@ func TestReconcile(t *testing.T) { }}, WantEvents: []string{ Eventf(corev1.EventTypeNormal, "Created", "Created placeholder service %q", "becomes-ready"), - Eventf(corev1.EventTypeNormal, "Created", "Created Ingress %q", "becomes-ready"), + createdIngressEvent("becomes-ready", traffic.DefaultTarget), }, Key: "default/becomes-ready", }, { @@ -340,7 +347,7 @@ func TestReconcile(t *testing.T) { }}, WantEvents: []string{ Eventf(corev1.EventTypeNormal, "Created", "Created placeholder service %q", "becomes-ready"), - Eventf(corev1.EventTypeNormal, "Created", "Created Ingress %q", "becomes-ready"), + createdIngressEvent("becomes-ready", traffic.DefaultTarget), }, Key: "default/becomes-ready", }, { @@ -1657,7 +1664,7 @@ func TestReconcile(t *testing.T) { }}, WantEvents: []string{ Eventf(corev1.EventTypeNormal, "Created", "Created placeholder service %q", "named-traffic-split"), - Eventf(corev1.EventTypeNormal, "Created", "Created Ingress %q", "named-traffic-split"), + createdIngressEvent("named-traffic-split", traffic.DefaultTarget), }, Key: "default/named-traffic-split", }, { @@ -1678,8 +1685,8 @@ func TestReconcile(t *testing.T) { WithConfigGeneration(1), WithLatestCreated("gray-00001"), WithLatestReady("gray-00001")), rev("default", "gray", 1, MarkRevisionReady, WithRevName("gray-00001")), }, - WantCreates: []runtime.Object{ - simpleIngress( + WantCreates: append( + simpleIngresses( Route("default", "same-revision-targets", WithURL, WithRouteGeneration(1), WithSpecTraffic( v1.TrafficTarget{ @@ -1766,7 +1773,7 @@ func TestReconcile(t *testing.T) { }), WithRouteUID("1-2"), WithRouteFinalizer), "gray", ), - }, + ), WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: Route("default", "same-revision-targets", WithRouteGeneration(1), WithRouteGeneration(1), WithRouteObservedGeneration, WithSpecTraffic( @@ -1807,7 +1814,9 @@ func TestReconcile(t *testing.T) { Eventf(corev1.EventTypeNormal, "Created", "Created placeholder service %q", "same-revision-targets"), Eventf(corev1.EventTypeNormal, "Created", "Created placeholder service %q", "also-gray-same-revision-targets"), Eventf(corev1.EventTypeNormal, "Created", "Created placeholder service %q", "gray-same-revision-targets"), - Eventf(corev1.EventTypeNormal, "Created", "Created Ingress %q", "same-revision-targets"), + createdIngressEvent("same-revision-targets", traffic.DefaultTarget), + createdIngressEvent("same-revision-targets-also-gray", "also-gray"), + createdIngressEvent("same-revision-targets-gray", "gray"), }, Key: "default/same-revision-targets", }, { @@ -2395,7 +2404,7 @@ func TestReconcileEnableExternalDomainTLS(t *testing.T) { }}, WantEvents: []string{ Eventf(corev1.EventTypeNormal, "Created", "Created placeholder service %q", "becomes-ready"), - Eventf(corev1.EventTypeNormal, "Created", "Created Ingress %q", "becomes-ready"), + createdIngressEvent("becomes-ready", traffic.DefaultTarget), }, Key: "default/becomes-ready", }, { @@ -2447,7 +2456,7 @@ func TestReconcileEnableExternalDomainTLS(t *testing.T) { WantEvents: []string{ Eventf(corev1.EventTypeNormal, "Created", "Created placeholder service %q", "becomes-ready"), Eventf(corev1.EventTypeNormal, "Created", "Created Certificate %s/%s", "default", "route-12-34"), - Eventf(corev1.EventTypeNormal, "Created", "Created Ingress %q", "becomes-ready"), + createdIngressEvent("becomes-ready", traffic.DefaultTarget), }, Key: "default/becomes-ready", }, { @@ -2504,7 +2513,7 @@ func TestReconcileEnableExternalDomainTLS(t *testing.T) { }}, WantEvents: []string{ Eventf(corev1.EventTypeNormal, "Created", "Created placeholder service %q", "becomes-ready"), - Eventf(corev1.EventTypeNormal, "Created", "Created Ingress %q", "becomes-ready"), + createdIngressEvent("becomes-ready", traffic.DefaultTarget), }, Key: "default/becomes-ready", }, { @@ -2586,7 +2595,7 @@ func TestReconcileEnableExternalDomainTLS(t *testing.T) { Eventf(corev1.EventTypeNormal, "Created", "Created placeholder service %q", "becomes-ready"), Eventf(corev1.EventTypeNormal, "Updated", "Updated Spec for Certificate %s/%s", "default", "route-12-34"), Eventf(corev1.EventTypeNormal, "Deleted", "Deleted orphaned Knative Certificate %s/%s", "default", "route-12-34"), - Eventf(corev1.EventTypeNormal, "Created", "Created Ingress %q", "becomes-ready"), + createdIngressEvent("becomes-ready", traffic.DefaultTarget), }, WantDeletes: []clientgotesting.DeleteActionImpl{{ // This certificate's DNS name is not the host name needed by the input Route. @@ -2716,7 +2725,7 @@ func TestReconcileEnableExternalDomainTLS(t *testing.T) { Eventf(corev1.EventTypeNormal, "Created", "Created placeholder service %q", "becomes-ready"), Eventf(corev1.EventTypeNormal, "Updated", "Updated Spec for Certificate %s/%s", "default", "route-12-34"), Eventf(corev1.EventTypeNormal, "Deleted", "Deleted orphaned Knative Certificate %s/%s", "default", "route-12-34-unused"), - Eventf(corev1.EventTypeNormal, "Created", "Created Ingress %q", "becomes-ready"), + createdIngressEvent("becomes-ready", traffic.DefaultTarget), }, WantDeletes: []clientgotesting.DeleteActionImpl{{ // This certificate's DNS name is not the host name needed by the input Route. @@ -2821,7 +2830,7 @@ func TestReconcileEnableExternalDomainTLS(t *testing.T) { }, WantEvents: []string{ Eventf(corev1.EventTypeNormal, "Created", "Created placeholder service %q", "becomes-ready"), - Eventf(corev1.EventTypeNormal, "Created", "Created Ingress %q", "becomes-ready"), + createdIngressEvent("becomes-ready", traffic.DefaultTarget), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: Route("default", "becomes-ready", WithConfigTarget("config"), @@ -3115,7 +3124,7 @@ func TestReconcileEnableExternalDomainTLS(t *testing.T) { Eventf(corev1.EventTypeNormal, "Created", "Created placeholder service %q", "becomes-ready"), Eventf(corev1.EventTypeNormal, "Updated", "Updated Spec for Certificate %s/%s", "default", "route-12-34"), Eventf(corev1.EventTypeNormal, "Deleted", "Deleted orphaned Knative Certificate %s/%s", "default", "route-12-34"), - Eventf(corev1.EventTypeNormal, "Created", "Created Ingress %q", "becomes-ready"), + createdIngressEvent("becomes-ready", traffic.DefaultTarget), }, WantDeletes: []clientgotesting.DeleteActionImpl{{ // This certificate's DNS name is not the host name needed by the input Route. @@ -3455,7 +3464,7 @@ func simplePlaceholderK8sService(ctx context.Context, r *v1.Route, targetName st } func simpleK8sService(r *v1.Route, so ...K8sServiceOption) *corev1.Service { - return k8sServiceWithIngress(r, simpleIngress(r, &traffic.Config{}, withReadyIngress), so...) + return k8sServiceWithIngress(r, simpleIngress(r, emptyTrafficConfig(), withReadyIngress), so...) } func k8sServiceWithIngress(r *v1.Route, ing *netv1alpha1.Ingress, so ...K8sServiceOption) *corev1.Service { @@ -3489,13 +3498,28 @@ func k8sEndpointsWithIngress(r *v1.Route, ing *netv1alpha1.Ingress) *corev1.Endp return pair.Endpoints } +func emptyTrafficConfig() *traffic.Config { + return &traffic.Config{ + Targets: map[string]traffic.RevisionTargets{ + traffic.DefaultTarget: {}, + }, + } +} + func simpleIngress(r *v1.Route, tc *traffic.Config, io ...IngressOption) *netv1alpha1.Ingress { return ingressWithTLS(r, tc, nil /*tls*/, nil /*challenges*/, io...) } +func simpleIngresses(r *v1.Route, tc *traffic.Config, io ...IngressOption) []runtime.Object { + return ingressesWithTLS(r, tc, nil /*tls*/, nil /*challenges*/, io...) +} + +// ingressWithTLS returns the default-tag ingress from MakeIngress. +// For multi-tag scenarios, use ingressesWithTLS instead. func ingressWithTLS(r *v1.Route, tc *traffic.Config, tls []netv1alpha1.IngressTLS, challenges []netv1alpha1.HTTP01Challenge, io ...IngressOption) *netv1alpha1.Ingress { - ingress, _ := resources.MakeIngress(getContext(), r, tc, tls, testIngressClass, challenges...) + ingresses, _ := resources.MakeIngress(getContext(), r, tc, tls, testIngressClass, challenges...) + ingress := ingresses[0] for _, opt := range io { opt(ingress) } @@ -3503,10 +3527,25 @@ func ingressWithTLS(r *v1.Route, tc *traffic.Config, tls []netv1alpha1.IngressTL return ingress } +func ingressesWithTLS(r *v1.Route, tc *traffic.Config, tls []netv1alpha1.IngressTLS, challenges []netv1alpha1.HTTP01Challenge, io ...IngressOption) []runtime.Object { + ingresses, _ := resources.MakeIngress(getContext(), r, tc, tls, testIngressClass, challenges...) + + result := make([]runtime.Object, 0, len(ingresses)) + for _, ingress := range ingresses { + for _, opt := range io { + opt(ingress) + } + result = append(result, ingress) + } + + return result +} + +// ingressWithRollout returns the default-tag ingress with the provided rollout. func ingressWithRollout(r *v1.Route, tc *traffic.Config, ro *traffic.Rollout, io ...IngressOption) *netv1alpha1.Ingress { - ingress, _ := resources.MakeIngressWithRollout(getContext(), r, tc, ro, nil /*tls*/, testIngressClass) - for _, o := range io { - o(ingress) + ingress, _ := resources.MakeDefaultIngressWithRollout(getContext(), r, tc, ro, nil /*tls*/, testIngressClass) + for _, opt := range io { + opt(ingress) } return ingress } diff --git a/test/e2e/externaldomaintls/auto_tls_test.go b/test/e2e/externaldomaintls/auto_tls_test.go index 1493ade7c7c7..39190232390c 100644 --- a/test/e2e/externaldomaintls/auto_tls_test.go +++ b/test/e2e/externaldomaintls/auto_tls_test.go @@ -33,6 +33,7 @@ import ( "knative.dev/networking/pkg/apis/networking" "knative.dev/pkg/ptr" "knative.dev/pkg/test/spoof" + serving "knative.dev/serving/pkg/apis/serving" servingv1 "knative.dev/serving/pkg/apis/serving/v1" routenames "knative.dev/serving/pkg/reconciler/route/resources/names" rtesting "knative.dev/serving/pkg/testing/v1" @@ -138,14 +139,23 @@ func testExternalDomainTLS(t *testing.T) { t.Fatalf("Traffic for route: %s is not HTTPS: %v", names.Route, err) } - ing, err := clients.NetworkingClient.Ingresses.Get(context.Background(), routenames.Ingress(objects.Route), metav1.GetOptions{}) + // List all ingresses for this route, including per-tag ingresses, + // so we can load TLS certificates from each one. + ingList, err := clients.NetworkingClient.Ingresses.List(context.Background(), metav1.ListOptions{ + LabelSelector: serving.RouteLabelKey + "=" + objects.Route.Name, + }) if err != nil { - t.Fatal("Failed to get ingress:", err) + t.Fatal("Failed to list ingresses:", err) } - for _, tls := range ing.Spec.TLS { - // Each new cert has to be added to the root pool so we can make requests. - if !rootCAs.AppendCertsFromPEM(test.PemDataFromSecret(context.Background(), t.Logf, clients, tls.SecretNamespace, tls.SecretName)) { - t.Fatal("Failed to add the certificate to the root CA") + if len(ingList.Items) == 0 { + t.Fatalf("No ingresses found for route %q", objects.Route.Name) + } + for _, ing := range ingList.Items { + for _, tls := range ing.Spec.TLS { + // Each new cert has to be added to the root pool so we can make requests. + if !rootCAs.AppendCertsFromPEM(test.PemDataFromSecret(context.Background(), t.Logf, clients, tls.SecretNamespace, tls.SecretName)) { + t.Fatalf("Failed to add certificate to root CA from ingress %q (secret %s/%s)", ing.Name, tls.SecretNamespace, tls.SecretName) + } } } diff --git a/vendor/knative.dev/networking/pkg/apis/networking/register.go b/vendor/knative.dev/networking/pkg/apis/networking/register.go index 054f2a053607..ad4b0ee42259 100644 --- a/vendor/knative.dev/networking/pkg/apis/networking/register.go +++ b/vendor/knative.dev/networking/pkg/apis/networking/register.go @@ -40,6 +40,10 @@ const ( // RolloutAnnotationKey is the annotation key for storing // the rollout state in the Annotations of the Kingress or Route.Status. RolloutAnnotationKey = GroupName + "/rollout" + + // TagLabelKey is the label key attached to per-tag Ingress resources + // to indicate which traffic tag the Ingress corresponds to. + TagLabelKey = GroupName + "/tag" ) const (