From 8bf06c374de3b062561a6f29b7e79772ae6ebfd8 Mon Sep 17 00:00:00 2001 From: Starlight Romero <28881133+starlightromero@users.noreply.github.com> Date: Fri, 12 Dec 2025 10:53:00 -0800 Subject: [PATCH] fix(gateway): skip target group creation for HTTPRoute redirect-only rules HTTPRoute rules with only RequestRedirect filters and no BackendRefs were incorrectly creating target groups, consuming AWS resources unnecessarily and causing deployment failures. This change implements redirect-only rule detection to skip target group creation for rules that don't route to backends, while maintaining redirect action creation for proper ALB configuration. Key changes: - Add IsRedirectOnlyRule() function to detect redirect-only rules - Modify buildListenerRules() to skip target group creation for redirect-only rules - Add comprehensive resource management and cleanup logic - Include extensive property-based testing with 8 correctness properties The fix ensures redirect-only rules don't consume AWS target group quotas while maintaining full functionality for redirect actions and preserving backward compatibility with Gateway API v1 specifications. Fixes #4497 --- pkg/gateway/model/model_build_listener.go | 81 ++- .../model_build_listener_redirect_test.go | 398 +++++++++++ pkg/gateway/routeutils/cleanup.go | 137 ++++ pkg/gateway/routeutils/cleanup_test.go | 666 ++++++++++++++++++ pkg/gateway/routeutils/mixed_rules_test.go | 360 ++++++++++ pkg/gateway/routeutils/redirect_utils.go | 70 ++ pkg/gateway/routeutils/redirect_utils_test.go | 448 ++++++++++++ pkg/gateway/routeutils/resource_accounting.go | 170 +++++ .../routeutils/resource_accounting_test.go | 533 ++++++++++++++ pkg/gateway/routeutils/validation.go | 245 +++++++ pkg/gateway/routeutils/validation_test.go | 373 ++++++++++ test/e2e/gateway/redirect_only_test.go | 484 +++++++++++++ test/e2e/gateway/redirect_utils.go | 310 ++++++++ 13 files changed, 4265 insertions(+), 10 deletions(-) create mode 100644 pkg/gateway/model/model_build_listener_redirect_test.go create mode 100644 pkg/gateway/routeutils/cleanup.go create mode 100644 pkg/gateway/routeutils/cleanup_test.go create mode 100644 pkg/gateway/routeutils/mixed_rules_test.go create mode 100644 pkg/gateway/routeutils/redirect_utils.go create mode 100644 pkg/gateway/routeutils/redirect_utils_test.go create mode 100644 pkg/gateway/routeutils/resource_accounting.go create mode 100644 pkg/gateway/routeutils/resource_accounting_test.go create mode 100644 pkg/gateway/routeutils/validation.go create mode 100644 pkg/gateway/routeutils/validation_test.go create mode 100644 test/e2e/gateway/redirect_only_test.go create mode 100644 test/e2e/gateway/redirect_utils.go diff --git a/pkg/gateway/model/model_build_listener.go b/pkg/gateway/model/model_build_listener.go index 1e88da74b..1ea578d86 100644 --- a/pkg/gateway/model/model_build_listener.go +++ b/pkg/gateway/model/model_build_listener.go @@ -199,6 +199,13 @@ func (l listenerBuilderImpl) buildL4TargetGroupTuples(stack core.Stack, routes [ return tgTuples, nil } +// buildListenerRules creates ALB listener rules from HTTPRoute and GRPCRoute rules. +// This function handles both redirect-only rules and backend rules: +// - Redirect-only rules: Rules with RequestRedirect filters but no BackendRefs. These create +// ALB redirect actions without target groups, optimizing resource usage. +// - Backend rules: Rules with BackendRefs that create target groups for traffic forwarding. +// - Mixed rules: Rules with both redirect filters and backends, which create both redirect +// actions and target groups according to Gateway API specifications. func (l listenerBuilderImpl) buildListenerRules(ctx context.Context, stack core.Stack, ls *elbv2model.Listener, ipAddressType elbv2model.IPAddressType, gw *gwv1.Gateway, port int32, routes map[int32][]routeutils.RouteDescriptor) ([]types.NamespacedName, error) { // sort all rules based on precedence rulesWithPrecedenceOrder := routeutils.SortAllRulesByPrecedence(routes[port], port) @@ -234,18 +241,72 @@ func (l listenerBuilderImpl) buildListenerRules(ctx context.Context, stack core. } routingAction = getRoutingAction(rule.GetListenerRuleConfig()) } + // Analyze rule type to determine target group creation strategy + // - Redirect-only rules: Have redirect filters but no backends, skip target group creation + // - Backend rules: Have backends (with or without redirect filters), create target groups + // - Empty rules: Have neither redirects nor backends, will result in 503 response + isRedirectOnly := routeutils.IsRedirectOnlyRule(rule) + hasRedirectFilter := routeutils.HasRequestRedirectFilter(rule) + backendCount := len(rule.GetBackends()) + + // Enhanced logging for redirect-only rule processing + l.logger.V(1).Info("Processing HTTPRoute rule", + "route", route.GetRouteNamespacedName(), + "routeKind", route.GetRouteKind(), + "isRedirectOnly", isRedirectOnly, + "hasRedirectFilter", hasRedirectFilter, + "backendCount", backendCount, + "listenerPort", port, + "listenerProtocol", ls.Spec.Protocol) + targetGroupTuples := make([]elbv2model.TargetGroupTuple, 0, len(rule.GetBackends())) - for _, backend := range rule.GetBackends() { - arn, tgErr := l.tgBuilder.buildTargetGroup(stack, gw, port, ls.Spec.Protocol, ipAddressType, route, backend) - if tgErr != nil { - return nil, tgErr + + // Target group creation logic: + // - Skip target group creation for redirect-only rules to optimize resource usage + // - Create target groups for all backends in non-redirect-only rules + // - This ensures redirect-only rules don't consume AWS target group quotas + if !isRedirectOnly { + l.logger.Info("Creating target groups for rule with backends", + "route", route.GetRouteNamespacedName(), + "backendCount", backendCount, + "hasRedirectFilter", hasRedirectFilter) + + for i, backend := range rule.GetBackends() { + l.logger.V(1).Info("Building target group for backend", + "route", route.GetRouteNamespacedName(), + "backendIndex", i, + "backendWeight", backend.Weight) + + arn, tgErr := l.tgBuilder.buildTargetGroup(stack, gw, port, ls.Spec.Protocol, ipAddressType, route, backend) + if tgErr != nil { + l.logger.Error(tgErr, "Failed to build target group for backend", + "route", route.GetRouteNamespacedName(), + "backendIndex", i, + "backendWeight", backend.Weight) + return nil, tgErr + } + + // weighted target group support + weight := int32(backend.Weight) + targetGroupTuples = append(targetGroupTuples, elbv2model.TargetGroupTuple{ + TargetGroupARN: arn, + Weight: &weight, + }) + + l.logger.V(1).Info("Successfully created target group for backend", + "route", route.GetRouteNamespacedName(), + "backendIndex", i, + "targetGroupARN", arn, + "weight", weight) } - // weighted target group support - weight := int32(backend.Weight) - targetGroupTuples = append(targetGroupTuples, elbv2model.TargetGroupTuple{ - TargetGroupARN: arn, - Weight: &weight, - }) + } else { + // Log that we're processing a redirect-only rule with detailed information + l.logger.Info("Processing redirect-only rule, skipping target group creation", + "route", route.GetRouteNamespacedName(), + "routeKind", route.GetRouteKind(), + "listenerPort", port, + "listenerProtocol", ls.Spec.Protocol, + "reason", "rule has redirect filters but no backends") } // Build Rule PreRoutingAction diff --git a/pkg/gateway/model/model_build_listener_redirect_test.go b/pkg/gateway/model/model_build_listener_redirect_test.go new file mode 100644 index 000000000..2eafbc788 --- /dev/null +++ b/pkg/gateway/model/model_build_listener_redirect_test.go @@ -0,0 +1,398 @@ +package model + +import ( + "context" + "testing" + + awssdk "github.com/aws/aws-sdk-go-v2/aws" + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "k8s.io/apimachinery/pkg/types" + elbv2gw "sigs.k8s.io/aws-load-balancer-controller/apis/gateway/v1beta1" + "sigs.k8s.io/aws-load-balancer-controller/pkg/gateway/routeutils" + "sigs.k8s.io/aws-load-balancer-controller/pkg/k8s" + "sigs.k8s.io/aws-load-balancer-controller/pkg/model/core" + elbv2model "sigs.k8s.io/aws-load-balancer-controller/pkg/model/elbv2" + gwv1 "sigs.k8s.io/gateway-api/apis/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log/zap" +) + +// Mock target group builder for testing +type mockTargetGroupBuilder struct { + mock.Mock +} + +func (m *mockTargetGroupBuilder) buildTargetGroup(stack core.Stack, gw *gwv1.Gateway, listenerPort int32, listenerProtocol elbv2model.Protocol, lbIPType elbv2model.IPAddressType, routeDescriptor routeutils.RouteDescriptor, backend routeutils.Backend) (core.StringToken, error) { + args := m.Called(stack, gw, listenerPort, listenerProtocol, lbIPType, routeDescriptor, backend) + return args.Get(0).(core.StringToken), args.Error(1) +} + +func (m *mockTargetGroupBuilder) getLocalFrontendNlbData() map[string]*elbv2model.FrontendNlbTargetGroupState { + args := m.Called() + return args.Get(0).(map[string]*elbv2model.FrontendNlbTargetGroupState) +} + +// Mock route descriptor for testing +type mockRouteDescriptor struct { + mock.Mock +} + +func (m *mockRouteDescriptor) GetAttachedRules() []routeutils.RouteRule { + args := m.Called() + return args.Get(0).([]routeutils.RouteRule) +} + +func (m *mockRouteDescriptor) GetHostnames() []gwv1.Hostname { + args := m.Called() + return args.Get(0).([]gwv1.Hostname) +} + +func (m *mockRouteDescriptor) GetParentRefs() []gwv1.ParentReference { + args := m.Called() + return args.Get(0).([]gwv1.ParentReference) +} + +func (m *mockRouteDescriptor) GetRouteKind() routeutils.RouteKind { + args := m.Called() + return args.Get(0).(routeutils.RouteKind) +} + +func (m *mockRouteDescriptor) GetRouteGeneration() int64 { + args := m.Called() + return args.Get(0).(int64) +} + +func (m *mockRouteDescriptor) GetRouteNamespacedName() types.NamespacedName { + args := m.Called() + return args.Get(0).(types.NamespacedName) +} + +func (m *mockRouteDescriptor) GetRouteIdentifier() string { + args := m.Called() + return args.Get(0).(string) +} + +func (m *mockRouteDescriptor) GetBackendRefs() []gwv1.BackendRef { + args := m.Called() + return args.Get(0).([]gwv1.BackendRef) +} + +func (m *mockRouteDescriptor) GetRouteListenerRuleConfigRefs() []gwv1.LocalObjectReference { + args := m.Called() + return args.Get(0).([]gwv1.LocalObjectReference) +} + +func (m *mockRouteDescriptor) GetCompatibleHostnamesByPort() map[int32][]gwv1.Hostname { + args := m.Called() + return args.Get(0).(map[int32][]gwv1.Hostname) +} + +// Mock route rule for testing +type mockRouteRule struct { + mock.Mock + rule *gwv1.HTTPRouteRule + backends []routeutils.Backend +} + +func (m *mockRouteRule) GetRawRouteRule() interface{} { + return m.rule +} + +func (m *mockRouteRule) GetBackends() []routeutils.Backend { + return m.backends +} + +func (m *mockRouteRule) GetListenerRuleConfig() *elbv2gw.ListenerRuleConfiguration { + args := m.Called() + if args.Get(0) == nil { + return nil + } + return args.Get(0).(*elbv2gw.ListenerRuleConfiguration) +} + +func TestBuildListenerRules_RedirectOnlyRules(t *testing.T) { + logger := zap.New(zap.UseDevMode(true)) + + tests := []struct { + name string + rules []routeutils.RouteRule + expectedTGBuilderCalls int + description string + }{ + { + name: "redirect-only rule skips target group creation", + rules: []routeutils.RouteRule{ + &mockRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []routeutils.Backend{}, // No backends + }, + }, + expectedTGBuilderCalls: 0, // Should not call target group builder + description: "Redirect-only rules should skip target group creation", + }, + { + name: "backend rule creates target groups", + rules: []routeutils.RouteRule{ + &mockRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{}, + }, + backends: []routeutils.Backend{{Weight: 1}}, // Has backends + }, + }, + expectedTGBuilderCalls: 1, // Should call target group builder once + description: "Backend rules should create target groups", + }, + { + name: "mixed rules - redirect-only and backend", + rules: []routeutils.RouteRule{ + // Redirect-only rule + &mockRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Hostname: (*gwv1.PreciseHostname)(awssdk.String("example.com")), + }, + }, + }, + }, + backends: []routeutils.Backend{}, // No backends + }, + // Backend rule + &mockRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{}, + }, + backends: []routeutils.Backend{{Weight: 50}, {Weight: 50}}, // Has backends + }, + }, + expectedTGBuilderCalls: 2, // Should call target group builder twice (for 2 backends) + description: "Mixed rules should process independently", + }, + { + name: "rule with redirect and backends (mixed rule)", + rules: []routeutils.RouteRule{ + &mockRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Port: (*gwv1.PortNumber)(awssdk.Int32(443)), + }, + }, + }, + }, + backends: []routeutils.Backend{{Weight: 100}}, // Has backends + }, + }, + expectedTGBuilderCalls: 1, // Should call target group builder (not redirect-only) + description: "Rules with both redirect and backends should create target groups", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Setup mocks + mockTGBuilder := &mockTargetGroupBuilder{} + mockRouteDesc := &mockRouteDescriptor{} + + // Setup mock expectations + mockRouteDesc.On("GetRouteKind").Return(routeutils.HTTPRouteKind) + mockRouteDesc.On("GetRouteNamespacedName").Return(types.NamespacedName{ + Namespace: "test", + Name: "test-route", + }) + + // Setup target group builder expectations + if tt.expectedTGBuilderCalls > 0 { + mockTGBuilder.On("buildTargetGroup", + mock.Anything, // stack + mock.Anything, // gateway + mock.Anything, // listenerPort + mock.Anything, // listenerProtocol + mock.Anything, // lbIPType + mock.Anything, // routeDescriptor + mock.Anything, // backend + ).Return(core.LiteralStringToken("arn:aws:elasticloadbalancing:us-west-2:123456789012:targetgroup/test/1234567890123456"), nil).Times(tt.expectedTGBuilderCalls) + } + + // Setup route rule mocks + for _, rule := range tt.rules { + if mockRule, ok := rule.(*mockRouteRule); ok { + mockRule.On("GetListenerRuleConfig").Return((*elbv2gw.ListenerRuleConfiguration)(nil)) + } + } + + // Create listener builder + builder := &listenerBuilderImpl{ + loadBalancerType: elbv2model.LoadBalancerTypeApplication, + tgBuilder: mockTGBuilder, + logger: logger, + } + + // Create test data + ctx := context.Background() + stack := core.NewDefaultStack(core.StackID("test")) + listener := &elbv2model.Listener{ + Spec: elbv2model.ListenerSpec{ + Protocol: elbv2model.ProtocolHTTP, + }, + } + gateway := &gwv1.Gateway{ + ObjectMeta: k8s.ObjectMeta{ + Namespace: "test", + Name: "test-gateway", + }, + } + port := int32(80) + routes := map[int32][]routeutils.RouteDescriptor{ + port: {mockRouteDesc}, + } + + // Mock the route descriptor to return our test rules + mockRouteDesc.On("GetAttachedRules").Return(tt.rules) + + // Execute the function under test + _, err := builder.buildListenerRules(ctx, stack, listener, elbv2model.IPAddressTypeIPV4, gateway, port, routes) + + // Assertions + assert.NoError(t, err, tt.description) + mockTGBuilder.AssertExpectations(t) + mockRouteDesc.AssertExpectations(t) + }) + } +} + +func TestBuildListenerRules_ErrorHandling(t *testing.T) { + logger := zap.New(zap.UseDevMode(true)) + + tests := []struct { + name string + rules []routeutils.RouteRule + setupMocks func(*mockTargetGroupBuilder, *mockRouteDescriptor) + expectError bool + description string + }{ + { + name: "target group creation error for backend rule", + rules: []routeutils.RouteRule{ + &mockRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{}, + }, + backends: []routeutils.Backend{{Weight: 1}}, + }, + }, + setupMocks: func(tgBuilder *mockTargetGroupBuilder, routeDesc *mockRouteDescriptor) { + tgBuilder.On("buildTargetGroup", + mock.Anything, mock.Anything, mock.Anything, mock.Anything, + mock.Anything, mock.Anything, mock.Anything, + ).Return(core.StringToken(nil), assert.AnError) + }, + expectError: true, + description: "Target group creation errors should be propagated", + }, + { + name: "redirect-only rule with target group error should not fail", + rules: []routeutils.RouteRule{ + &mockRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []routeutils.Backend{}, // No backends + }, + }, + setupMocks: func(tgBuilder *mockTargetGroupBuilder, routeDesc *mockRouteDescriptor) { + // No target group builder calls expected for redirect-only rules + }, + expectError: false, + description: "Redirect-only rules should not fail even if target group builder would error", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Setup mocks + mockTGBuilder := &mockTargetGroupBuilder{} + mockRouteDesc := &mockRouteDescriptor{} + + // Setup common mock expectations + mockRouteDesc.On("GetRouteKind").Return(routeutils.HTTPRouteKind) + mockRouteDesc.On("GetRouteNamespacedName").Return(types.NamespacedName{ + Namespace: "test", + Name: "test-route", + }) + mockRouteDesc.On("GetAttachedRules").Return(tt.rules) + + // Setup test-specific mocks + tt.setupMocks(mockTGBuilder, mockRouteDesc) + + // Setup route rule mocks + for _, rule := range tt.rules { + if mockRule, ok := rule.(*mockRouteRule); ok { + mockRule.On("GetListenerRuleConfig").Return((*elbv2gw.ListenerRuleConfiguration)(nil)) + } + } + + // Create listener builder + builder := &listenerBuilderImpl{ + loadBalancerType: elbv2model.LoadBalancerTypeApplication, + tgBuilder: mockTGBuilder, + logger: logger, + } + + // Create test data + ctx := context.Background() + stack := core.NewDefaultStack(core.StackID("test")) + listener := &elbv2model.Listener{ + Spec: elbv2model.ListenerSpec{ + Protocol: elbv2model.ProtocolHTTP, + }, + } + gateway := &gwv1.Gateway{ + ObjectMeta: k8s.ObjectMeta{ + Namespace: "test", + Name: "test-gateway", + }, + } + port := int32(80) + routes := map[int32][]routeutils.RouteDescriptor{ + port: {mockRouteDesc}, + } + + // Execute the function under test + _, err := builder.buildListenerRules(ctx, stack, listener, elbv2model.IPAddressTypeIPV4, gateway, port, routes) + + // Assertions + if tt.expectError { + assert.Error(t, err, tt.description) + } else { + assert.NoError(t, err, tt.description) + } + + mockTGBuilder.AssertExpectations(t) + mockRouteDesc.AssertExpectations(t) + }) + } +} \ No newline at end of file diff --git a/pkg/gateway/routeutils/cleanup.go b/pkg/gateway/routeutils/cleanup.go new file mode 100644 index 000000000..56dc091fc --- /dev/null +++ b/pkg/gateway/routeutils/cleanup.go @@ -0,0 +1,137 @@ +package routeutils + +import ( + "context" + "fmt" + + "github.com/go-logr/logr" + "k8s.io/apimachinery/pkg/types" +) + +// CleanupResult represents the result of a cleanup operation +type CleanupResult struct { + RedirectOnlyRulesSkipped int + TargetGroupsDeleted int + Errors []error +} + +// RouteCleanupManager handles cleanup operations for HTTPRoute rules +type RouteCleanupManager struct { + logger logr.Logger +} + +// NewRouteCleanupManager creates a new RouteCleanupManager +func NewRouteCleanupManager(logger logr.Logger) *RouteCleanupManager { + return &RouteCleanupManager{ + logger: logger, + } +} + +// CleanupRouteRules performs cleanup for a set of route rules +func (m *RouteCleanupManager) CleanupRouteRules(ctx context.Context, routeKey types.NamespacedName, rules []RouteRule) CleanupResult { + result := CleanupResult{ + Errors: make([]error, 0), + } + + for i, rule := range rules { + if IsRedirectOnlyRule(rule) { + // Skip cleanup for redirect-only rules as they don't have target groups + result.RedirectOnlyRulesSkipped++ + m.logger.Info("Skipping cleanup for redirect-only rule", + "route", routeKey, + "ruleIndex", i, + "reason", "no target groups created for redirect-only rules") + continue + } + + // For rules with backends, normal cleanup would apply + // This is handled by the existing TargetGroupBinding cleanup logic + if len(rule.GetBackends()) > 0 { + m.logger.Info("Rule has backends, cleanup will be handled by TargetGroupBinding controller", + "route", routeKey, + "ruleIndex", i, + "backendCount", len(rule.GetBackends())) + } + } + + return result +} + +// ValidateCleanupSafety validates that cleanup operations are safe for redirect-only rules +func (m *RouteCleanupManager) ValidateCleanupSafety(ctx context.Context, rules []RouteRule) error { + for i, rule := range rules { + if IsRedirectOnlyRule(rule) { + // Validate that redirect-only rules don't have any associated target groups + if len(rule.GetBackends()) > 0 { + return fmt.Errorf("redirect-only rule at index %d has backends, this should not happen", i) + } + + // Additional safety checks can be added here + m.logger.V(1).Info("Validated redirect-only rule safety", + "ruleIndex", i, + "hasRedirectFilter", HasRequestRedirectFilter(rule)) + } + } + + return nil +} + +// HandleStateTransition handles transitions between redirect-only and backend configurations +func (m *RouteCleanupManager) HandleStateTransition(ctx context.Context, routeKey types.NamespacedName, oldRules, newRules []RouteRule) error { + // Track state changes for each rule + for i := 0; i < len(oldRules) && i < len(newRules); i++ { + oldRule := oldRules[i] + newRule := newRules[i] + + oldIsRedirectOnly := IsRedirectOnlyRule(oldRule) + newIsRedirectOnly := IsRedirectOnlyRule(newRule) + + if oldIsRedirectOnly != newIsRedirectOnly { + m.logger.Info("Detected rule state transition", + "route", routeKey, + "ruleIndex", i, + "oldIsRedirectOnly", oldIsRedirectOnly, + "newIsRedirectOnly", newIsRedirectOnly) + + if oldIsRedirectOnly && !newIsRedirectOnly { + // Transition from redirect-only to backend rule + // Target groups will be created by the normal reconciliation process + m.logger.Info("Rule transitioning from redirect-only to backend rule", + "route", routeKey, + "ruleIndex", i, + "newBackendCount", len(newRule.GetBackends())) + } else if !oldIsRedirectOnly && newIsRedirectOnly { + // Transition from backend rule to redirect-only + // Target groups will be cleaned up by the TargetGroupBinding controller + m.logger.Info("Rule transitioning from backend rule to redirect-only", + "route", routeKey, + "ruleIndex", i, + "oldBackendCount", len(oldRule.GetBackends())) + } + } + } + + // Handle added or removed rules + if len(newRules) > len(oldRules) { + for i := len(oldRules); i < len(newRules); i++ { + newRule := newRules[i] + if IsRedirectOnlyRule(newRule) { + m.logger.Info("New redirect-only rule added", + "route", routeKey, + "ruleIndex", i) + } + } + } else if len(oldRules) > len(newRules) { + for i := len(newRules); i < len(oldRules); i++ { + oldRule := oldRules[i] + if !IsRedirectOnlyRule(oldRule) { + m.logger.Info("Backend rule removed, cleanup will be handled by TargetGroupBinding controller", + "route", routeKey, + "ruleIndex", i, + "oldBackendCount", len(oldRule.GetBackends())) + } + } + } + + return nil +} \ No newline at end of file diff --git a/pkg/gateway/routeutils/cleanup_test.go b/pkg/gateway/routeutils/cleanup_test.go new file mode 100644 index 000000000..28162cc76 --- /dev/null +++ b/pkg/gateway/routeutils/cleanup_test.go @@ -0,0 +1,666 @@ +package routeutils + +import ( + "context" + "fmt" + "testing" + + awssdk "github.com/aws/aws-sdk-go-v2/aws" + "github.com/stretchr/testify/assert" + "k8s.io/apimachinery/pkg/types" + gwv1 "sigs.k8s.io/gateway-api/apis/v1" + "sigs.k8s.io/controller-runtime/pkg/log/zap" +) + +func TestRouteCleanupManager_CleanupRouteRules(t *testing.T) { + logger := zap.New(zap.UseDevMode(true)) + manager := NewRouteCleanupManager(logger) + ctx := context.Background() + routeKey := types.NamespacedName{Namespace: "test", Name: "test-route"} + + tests := []struct { + name string + rules []RouteRule + expectedRedirectSkipped int + expectedTargetGroups int + }{ + { + name: "mixed rules with redirect-only and backend", + rules: []RouteRule{ + // Redirect-only rule + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{}, + }, + // Backend rule + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{}, + }, + backends: []Backend{{Weight: 1}}, + }, + }, + expectedRedirectSkipped: 1, + expectedTargetGroups: 0, // Cleanup is handled by TargetGroupBinding controller + }, + { + name: "all redirect-only rules", + rules: []RouteRule{ + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Hostname: (*gwv1.PreciseHostname)(awssdk.String("example.com")), + }, + }, + }, + }, + backends: []Backend{}, + }, + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Port: (*gwv1.PortNumber)(awssdk.Int32(443)), + }, + }, + }, + }, + backends: []Backend{}, + }, + }, + expectedRedirectSkipped: 2, + expectedTargetGroups: 0, + }, + { + name: "all backend rules", + rules: []RouteRule{ + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{Filters: []gwv1.HTTPRouteFilter{}}, + backends: []Backend{{Weight: 1}}, + }, + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{Filters: []gwv1.HTTPRouteFilter{}}, + backends: []Backend{{Weight: 50}, {Weight: 50}}, + }, + }, + expectedRedirectSkipped: 0, + expectedTargetGroups: 0, // Cleanup is handled by TargetGroupBinding controller + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := manager.CleanupRouteRules(ctx, routeKey, tt.rules) + + assert.Equal(t, tt.expectedRedirectSkipped, result.RedirectOnlyRulesSkipped) + assert.Equal(t, tt.expectedTargetGroups, result.TargetGroupsDeleted) + assert.Empty(t, result.Errors) + }) + } +} + +// **Feature: httproute-redirect-only-fix, Property 6: Cleanup operation safety** +// Property-based test to verify cleanup operation safety for redirect-only rules +func TestProperty_CleanupOperationSafety(t *testing.T) { + // Test property: For any HTTPRoute resource cleanup operation, the system should not + // attempt to delete target groups that were never created for redirect-only rules + + logger := zap.New(zap.UseDevMode(true)) + manager := NewRouteCleanupManager(logger) + ctx := context.Background() + + // Generate various cleanup scenarios + cleanupScenarios := []struct { + name string + rules []RouteRule + }{ + { + name: "single_redirect_only_rule", + rules: []RouteRule{ + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{}, + }, + }, + }, + { + name: "multiple_redirect_only_rules", + rules: []RouteRule{ + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{}, + }, + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Hostname: (*gwv1.PreciseHostname)(awssdk.String("example.com")), + }, + }, + }, + }, + backends: []Backend{}, + }, + }, + }, + { + name: "mixed_rules_with_redirects", + rules: []RouteRule{ + // Redirect-only + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Port: (*gwv1.PortNumber)(awssdk.Int32(443)), + }, + }, + }, + }, + backends: []Backend{}, + }, + // Backend rule + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{Filters: []gwv1.HTTPRouteFilter{}}, + backends: []Backend{{Weight: 1}}, + }, + // Mixed rule (redirect + backend) + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{{Weight: 100}}, + }, + }, + }, + } + + for _, scenario := range cleanupScenarios { + t.Run(scenario.name, func(t *testing.T) { + routeKey := types.NamespacedName{ + Namespace: "test", + Name: fmt.Sprintf("route-%s", scenario.name), + } + + // Property: Cleanup validation should pass for all scenarios + err := manager.ValidateCleanupSafety(ctx, scenario.rules) + assert.NoError(t, err, "Cleanup validation should pass") + + // Property: Cleanup should not attempt to delete non-existent target groups + result := manager.CleanupRouteRules(ctx, routeKey, scenario.rules) + assert.Empty(t, result.Errors, "Cleanup should not produce errors") + + // Property: Redirect-only rules should be skipped + redirectOnlyCount := 0 + for _, rule := range scenario.rules { + if IsRedirectOnlyRule(rule) { + redirectOnlyCount++ + } + } + assert.Equal(t, redirectOnlyCount, result.RedirectOnlyRulesSkipped, + "All redirect-only rules should be skipped") + + // Property: No target groups should be deleted directly (handled by TargetGroupBinding controller) + assert.Equal(t, 0, result.TargetGroupsDeleted, + "Target group deletion should be handled by TargetGroupBinding controller") + }) + } +} + +func TestValidateCleanupSafety(t *testing.T) { + logger := zap.New(zap.UseDevMode(true)) + manager := NewRouteCleanupManager(logger) + ctx := context.Background() + + tests := []struct { + name string + rules []RouteRule + expectError bool + errorMsg string + }{ + { + name: "valid redirect-only rules", + rules: []RouteRule{ + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{}, + }, + }, + expectError: false, + }, + { + name: "invalid redirect-only rule with backends", + rules: []RouteRule{ + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{{Weight: 1}}, // This should not happen for redirect-only + }, + }, + expectError: true, + errorMsg: "redirect-only rule at index 0 has backends", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := manager.ValidateCleanupSafety(ctx, tt.rules) + + if tt.expectError { + assert.Error(t, err) + assert.Contains(t, err.Error(), tt.errorMsg) + } else { + assert.NoError(t, err) + } + }) + } +} + +// **Feature: httproute-redirect-only-fix, Property 7: State transition correctness** +// Property-based test to verify state transition correctness +func TestProperty_StateTransitionCorrectness(t *testing.T) { + // Test property: For any HTTPRoute rule that transitions between redirect-only and + // backend-routing configurations, the system should handle the transition correctly + + logger := zap.New(zap.UseDevMode(true)) + manager := NewRouteCleanupManager(logger) + ctx := context.Background() + + // Define various state transition scenarios + transitionScenarios := []struct { + name string + oldRules []RouteRule + newRules []RouteRule + expected string + }{ + { + name: "redirect_only_to_backend", + oldRules: []RouteRule{ + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{}, + }, + }, + newRules: []RouteRule{ + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{Filters: []gwv1.HTTPRouteFilter{}}, + backends: []Backend{{Weight: 1}}, + }, + }, + expected: "redirect_to_backend_transition", + }, + { + name: "backend_to_redirect_only", + oldRules: []RouteRule{ + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{Filters: []gwv1.HTTPRouteFilter{}}, + backends: []Backend{{Weight: 1}}, + }, + }, + newRules: []RouteRule{ + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Hostname: (*gwv1.PreciseHostname)(awssdk.String("example.com")), + }, + }, + }, + }, + backends: []Backend{}, + }, + }, + expected: "backend_to_redirect_transition", + }, + { + name: "backend_to_mixed", + oldRules: []RouteRule{ + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{Filters: []gwv1.HTTPRouteFilter{}}, + backends: []Backend{{Weight: 1}}, + }, + }, + newRules: []RouteRule{ + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Port: (*gwv1.PortNumber)(awssdk.Int32(443)), + }, + }, + }, + }, + backends: []Backend{{Weight: 100}}, + }, + }, + expected: "backend_to_mixed_transition", + }, + { + name: "mixed_to_redirect_only", + oldRules: []RouteRule{ + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{{Weight: 50}}, + }, + }, + newRules: []RouteRule{ + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{}, + }, + }, + expected: "mixed_to_redirect_transition", + }, + { + name: "multiple_rules_mixed_transitions", + oldRules: []RouteRule{ + // Rule 0: redirect-only + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{}, + }, + // Rule 1: backend + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{Filters: []gwv1.HTTPRouteFilter{}}, + backends: []Backend{{Weight: 1}}, + }, + }, + newRules: []RouteRule{ + // Rule 0: backend (transition from redirect-only) + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{Filters: []gwv1.HTTPRouteFilter{}}, + backends: []Backend{{Weight: 100}}, + }, + // Rule 1: redirect-only (transition from backend) + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Hostname: (*gwv1.PreciseHostname)(awssdk.String("redirect.example.com")), + }, + }, + }, + }, + backends: []Backend{}, + }, + }, + expected: "multiple_transitions", + }, + } + + for _, scenario := range transitionScenarios { + t.Run(scenario.name, func(t *testing.T) { + routeKey := types.NamespacedName{ + Namespace: "test", + Name: fmt.Sprintf("transition-%s", scenario.name), + } + + // Property: State transitions should be handled without errors + err := manager.HandleStateTransition(ctx, routeKey, scenario.oldRules, scenario.newRules) + assert.NoError(t, err, "State transition should be handled without errors") + + // Property: Verify the transition logic + switch scenario.expected { + case "redirect_to_backend_transition": + // Verify old rule was redirect-only and new rule has backends + assert.True(t, IsRedirectOnlyRule(scenario.oldRules[0]), + "Old rule should be redirect-only") + assert.False(t, IsRedirectOnlyRule(scenario.newRules[0]), + "New rule should not be redirect-only") + assert.Greater(t, len(scenario.newRules[0].GetBackends()), 0, + "New rule should have backends") + + case "backend_to_redirect_transition": + // Verify old rule had backends and new rule is redirect-only + assert.False(t, IsRedirectOnlyRule(scenario.oldRules[0]), + "Old rule should not be redirect-only") + assert.True(t, IsRedirectOnlyRule(scenario.newRules[0]), + "New rule should be redirect-only") + assert.Equal(t, 0, len(scenario.newRules[0].GetBackends()), + "New rule should have no backends") + + case "backend_to_mixed_transition": + // Verify transition from backend-only to mixed (redirect + backend) + assert.False(t, IsRedirectOnlyRule(scenario.oldRules[0]), + "Old rule should not be redirect-only") + assert.False(t, HasRequestRedirectFilter(scenario.oldRules[0]), + "Old rule should not have redirect filter") + assert.False(t, IsRedirectOnlyRule(scenario.newRules[0]), + "New rule should not be redirect-only (has backends)") + assert.True(t, HasRequestRedirectFilter(scenario.newRules[0]), + "New rule should have redirect filter") + assert.Greater(t, len(scenario.newRules[0].GetBackends()), 0, + "New rule should have backends") + + case "mixed_to_redirect_transition": + // Verify transition from mixed to redirect-only + assert.False(t, IsRedirectOnlyRule(scenario.oldRules[0]), + "Old rule should not be redirect-only (has backends)") + assert.True(t, HasRequestRedirectFilter(scenario.oldRules[0]), + "Old rule should have redirect filter") + assert.True(t, IsRedirectOnlyRule(scenario.newRules[0]), + "New rule should be redirect-only") + assert.Equal(t, 0, len(scenario.newRules[0].GetBackends()), + "New rule should have no backends") + + case "multiple_transitions": + // Verify multiple rule transitions + assert.Equal(t, len(scenario.oldRules), len(scenario.newRules), + "Rule count should be preserved") + + // Rule 0: redirect-only -> backend + assert.True(t, IsRedirectOnlyRule(scenario.oldRules[0]), + "Old rule 0 should be redirect-only") + assert.False(t, IsRedirectOnlyRule(scenario.newRules[0]), + "New rule 0 should not be redirect-only") + + // Rule 1: backend -> redirect-only + assert.False(t, IsRedirectOnlyRule(scenario.oldRules[1]), + "Old rule 1 should not be redirect-only") + assert.True(t, IsRedirectOnlyRule(scenario.newRules[1]), + "New rule 1 should be redirect-only") + } + }) + } +} + +// Property-based test for rule addition and removal scenarios +func TestProperty_RuleAdditionRemovalTransitions(t *testing.T) { + // Test property: Adding or removing rules should be handled correctly + + logger := zap.New(zap.UseDevMode(true)) + manager := NewRouteCleanupManager(logger) + ctx := context.Background() + + additionRemovalScenarios := []struct { + name string + oldRules []RouteRule + newRules []RouteRule + expected string + }{ + { + name: "add_redirect_only_rule", + oldRules: []RouteRule{}, + newRules: []RouteRule{ + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{}, + }, + }, + expected: "rule_addition", + }, + { + name: "remove_backend_rule", + oldRules: []RouteRule{ + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{Filters: []gwv1.HTTPRouteFilter{}}, + backends: []Backend{{Weight: 1}}, + }, + }, + newRules: []RouteRule{}, + expected: "rule_removal", + }, + { + name: "add_multiple_mixed_rules", + oldRules: []RouteRule{ + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{Filters: []gwv1.HTTPRouteFilter{}}, + backends: []Backend{{Weight: 1}}, + }, + }, + newRules: []RouteRule{ + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{Filters: []gwv1.HTTPRouteFilter{}}, + backends: []Backend{{Weight: 1}}, + }, + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Hostname: (*gwv1.PreciseHostname)(awssdk.String("example.com")), + }, + }, + }, + }, + backends: []Backend{}, + }, + }, + expected: "rule_addition", + }, + } + + for _, scenario := range additionRemovalScenarios { + t.Run(scenario.name, func(t *testing.T) { + routeKey := types.NamespacedName{ + Namespace: "test", + Name: fmt.Sprintf("add-remove-%s", scenario.name), + } + + // Property: Addition/removal transitions should be handled without errors + err := manager.HandleStateTransition(ctx, routeKey, scenario.oldRules, scenario.newRules) + assert.NoError(t, err, "Addition/removal transition should be handled without errors") + + // Property: Verify the transition behavior + switch scenario.expected { + case "rule_addition": + assert.Greater(t, len(scenario.newRules), len(scenario.oldRules), + "New rules should have more rules than old rules") + case "rule_removal": + assert.Less(t, len(scenario.newRules), len(scenario.oldRules), + "New rules should have fewer rules than old rules") + } + }) + } +} \ No newline at end of file diff --git a/pkg/gateway/routeutils/mixed_rules_test.go b/pkg/gateway/routeutils/mixed_rules_test.go new file mode 100644 index 000000000..6c23535e1 --- /dev/null +++ b/pkg/gateway/routeutils/mixed_rules_test.go @@ -0,0 +1,360 @@ +package routeutils + +import ( + "fmt" + "testing" + + awssdk "github.com/aws/aws-sdk-go-v2/aws" + "github.com/stretchr/testify/assert" + gwv1 "sigs.k8s.io/gateway-api/apis/v1" +) + +// **Feature: httproute-redirect-only-fix, Property 3: Mixed HTTPRoute processing independence** +// Property-based test to verify that mixed HTTPRoute rules are processed independently +func TestProperty_MixedHTTPRouteProcessingIndependence(t *testing.T) { + // Test property: For any HTTPRoute containing multiple rules of different types + // (redirect-only and backend rules), each rule should be processed independently + + // Create various combinations of mixed rules + testCases := []struct { + name string + rules []RouteRule + }{ + { + name: "redirect_only_then_backend", + rules: []RouteRule{ + // Redirect-only rule + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{}, + }, + // Backend rule + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{}, + }, + backends: []Backend{{Weight: 1}}, + }, + }, + }, + { + name: "backend_then_redirect_only", + rules: []RouteRule{ + // Backend rule + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{}, + }, + backends: []Backend{{Weight: 1}}, + }, + // Redirect-only rule + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Hostname: (*gwv1.PreciseHostname)(awssdk.String("example.com")), + }, + }, + }, + }, + backends: []Backend{}, + }, + }, + }, + { + name: "mixed_with_redirect_and_backend", + rules: []RouteRule{ + // Rule with both redirect and backend + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Port: (*gwv1.PortNumber)(awssdk.Int32(443)), + }, + }, + }, + }, + backends: []Backend{{Weight: 50}, {Weight: 50}}, + }, + // Redirect-only rule + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{}, + }, + }, + }, + { + name: "multiple_mixed_rules", + rules: []RouteRule{ + // Backend rule 1 + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{Filters: []gwv1.HTTPRouteFilter{}}, + backends: []Backend{{Weight: 1}}, + }, + // Redirect-only rule 1 + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{}, + }, + // Backend rule 2 + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{Filters: []gwv1.HTTPRouteFilter{}}, + backends: []Backend{{Weight: 100}}, + }, + // Redirect-only rule 2 + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Hostname: (*gwv1.PreciseHostname)(awssdk.String("redirect.example.com")), + }, + }, + }, + }, + backends: []Backend{}, + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Property: Each rule should be processed independently + for i, rule := range tc.rules { + t.Run(fmt.Sprintf("rule_%d", i), func(t *testing.T) { + // Verify rule classification is independent of other rules + isRedirectOnly := IsRedirectOnlyRule(rule) + hasRedirectFilter := HasRequestRedirectFilter(rule) + hasBackends := len(rule.GetBackends()) > 0 + + // Property assertions based on rule characteristics + if hasBackends { + assert.False(t, isRedirectOnly, + "Rule with backends should not be redirect-only regardless of other rules") + } else if hasRedirectFilter { + assert.True(t, isRedirectOnly, + "Rule with redirect filter and no backends should be redirect-only regardless of other rules") + } else { + assert.False(t, isRedirectOnly, + "Rule without redirect filter and backends should not be redirect-only regardless of other rules") + } + }) + } + + // Verify that the presence of different rule types doesn't affect each other + redirectOnlyCount := 0 + backendRuleCount := 0 + mixedRuleCount := 0 + + for _, rule := range tc.rules { + hasBackends := len(rule.GetBackends()) > 0 + hasRedirectFilter := HasRequestRedirectFilter(rule) + + if IsRedirectOnlyRule(rule) { + redirectOnlyCount++ + } else if hasBackends && !hasRedirectFilter { + backendRuleCount++ + } else if hasBackends && hasRedirectFilter { + mixedRuleCount++ + } + } + + // Property: Rule counts should be consistent with individual rule analysis + assert.GreaterOrEqual(t, len(tc.rules), redirectOnlyCount+backendRuleCount+mixedRuleCount, + "Total rule count should match classified rules") + }) + } +} + +// **Feature: httproute-redirect-only-fix, Property 4: Gateway API compliance for mixed rules** +// Property-based test to verify Gateway API compliance for rules with both redirects and backends +func TestProperty_GatewayAPIComplianceForMixedRules(t *testing.T) { + // Test property: For any HTTPRoute rule that has both RequestRedirect filters and BackendRefs, + // the system should process both according to Gateway API specifications + + // Create various combinations of mixed rules (redirect + backend) + mixedRuleConfigs := []struct { + name string + redirectFilter gwv1.HTTPRequestRedirectFilter + backends []Backend + expectedBehavior string + }{ + { + name: "https_redirect_with_single_backend", + redirectFilter: gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + backends: []Backend{{Weight: 1}}, + expectedBehavior: "should_process_both_redirect_and_backend", + }, + { + name: "hostname_redirect_with_multiple_backends", + redirectFilter: gwv1.HTTPRequestRedirectFilter{ + Hostname: (*gwv1.PreciseHostname)(awssdk.String("api.example.com")), + }, + backends: []Backend{{Weight: 50}, {Weight: 50}}, + expectedBehavior: "should_process_both_redirect_and_backend", + }, + { + name: "port_redirect_with_weighted_backends", + redirectFilter: gwv1.HTTPRequestRedirectFilter{ + Port: (*gwv1.PortNumber)(awssdk.Int32(8080)), + }, + backends: []Backend{{Weight: 25}, {Weight: 75}}, + expectedBehavior: "should_process_both_redirect_and_backend", + }, + { + name: "full_redirect_with_backend", + redirectFilter: gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + Hostname: (*gwv1.PreciseHostname)(awssdk.String("secure.example.com")), + Port: (*gwv1.PortNumber)(awssdk.Int32(443)), + StatusCode: awssdk.Int(301), + }, + backends: []Backend{{Weight: 100}}, + expectedBehavior: "should_process_both_redirect_and_backend", + }, + } + + for _, config := range mixedRuleConfigs { + t.Run(config.name, func(t *testing.T) { + // Create rule with both redirect filter and backends + rule := &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &config.redirectFilter, + }, + }, + }, + backends: config.backends, + } + + // Property: Rules with both redirects and backends should not be redirect-only + assert.False(t, IsRedirectOnlyRule(rule), + "Rule with both redirect and backends should not be redirect-only") + + // Property: Should have redirect filter + assert.True(t, HasRequestRedirectFilter(rule), + "Rule should have redirect filter") + + // Property: Should have backends + assert.Greater(t, len(rule.GetBackends()), 0, + "Rule should have backends") + + // Property: Backend count should match expected + assert.Equal(t, len(config.backends), len(rule.GetBackends()), + "Backend count should match configuration") + + // Property: Backend weights should be preserved + for i, expectedBackend := range config.backends { + actualBackend := rule.GetBackends()[i] + assert.Equal(t, expectedBackend.Weight, actualBackend.Weight, + "Backend weight should be preserved") + } + + // Property: Gateway API compliance - both redirect and backend processing should be possible + // This means the rule should be processed for both redirect actions and target group creation + switch config.expectedBehavior { + case "should_process_both_redirect_and_backend": + // Verify that the rule has the necessary components for both redirect and backend processing + assert.True(t, HasRequestRedirectFilter(rule), "Should have redirect filter for redirect processing") + assert.Greater(t, len(rule.GetBackends()), 0, "Should have backends for target group creation") + + // Verify redirect filter is properly configured + httpRule := rule.GetRawRouteRule().(*gwv1.HTTPRouteRule) + found := false + for _, filter := range httpRule.Filters { + if filter.Type == gwv1.HTTPRouteFilterRequestRedirect { + assert.NotNil(t, filter.RequestRedirect, "Redirect filter should have configuration") + found = true + break + } + } + assert.True(t, found, "Should find redirect filter in rule") + } + }) + } +} + +// Property-based test for Gateway API specification compliance +func TestProperty_GatewayAPISpecificationCompliance(t *testing.T) { + // Test various Gateway API specification requirements + + // Test that redirect-only rules comply with Gateway API spec + t.Run("redirect_only_compliance", func(t *testing.T) { + redirectOnlyRule := &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{}, // No backends as per Gateway API spec for redirect-only + } + + // Property: Redirect-only rules should be valid according to Gateway API + assert.True(t, IsRedirectOnlyRule(redirectOnlyRule), + "Valid redirect-only rule should be identified correctly") + assert.True(t, HasRequestRedirectFilter(redirectOnlyRule), + "Redirect-only rule should have redirect filter") + assert.Equal(t, 0, len(redirectOnlyRule.GetBackends()), + "Redirect-only rule should have no backends") + }) + + // Test that backend-only rules comply with Gateway API spec + t.Run("backend_only_compliance", func(t *testing.T) { + backendOnlyRule := &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{}, // No filters + }, + backends: []Backend{{Weight: 1}}, // Has backends + } + + // Property: Backend-only rules should be valid according to Gateway API + assert.False(t, IsRedirectOnlyRule(backendOnlyRule), + "Backend-only rule should not be redirect-only") + assert.False(t, HasRequestRedirectFilter(backendOnlyRule), + "Backend-only rule should not have redirect filter") + assert.Greater(t, len(backendOnlyRule.GetBackends()), 0, + "Backend-only rule should have backends") + }) +} \ No newline at end of file diff --git a/pkg/gateway/routeutils/redirect_utils.go b/pkg/gateway/routeutils/redirect_utils.go new file mode 100644 index 000000000..c1ad6536c --- /dev/null +++ b/pkg/gateway/routeutils/redirect_utils.go @@ -0,0 +1,70 @@ +package routeutils + +import ( + gwv1 "sigs.k8s.io/gateway-api/apis/v1" +) + +// IsRedirectOnlyRule checks if an HTTPRoute rule contains only RequestRedirect filters +// and has no BackendRefs, making it a redirect-only rule that should not create target groups. +// +// Redirect-only rules are an optimization for simple HTTP redirects that don't require +// backend services. They create ALB redirect actions without consuming target group quotas. +// +// Returns true if: +// - The rule is an HTTPRoute rule +// - The rule has at least one RequestRedirect filter +// - The rule has no backends (BackendRefs) +// +// Returns false for: +// - Non-HTTPRoute rules (TCP, UDP, TLS, GRPC) +// - Rules with backends (regardless of redirect filters) +// - Rules without redirect filters +func IsRedirectOnlyRule(rule RouteRule) bool { + // Check if the rule has any backends + if len(rule.GetBackends()) > 0 { + return false + } + + // Check if this is an HTTPRoute rule with redirect filters + rawRule := rule.GetRawRouteRule() + httpRule, ok := rawRule.(*gwv1.HTTPRouteRule) + if !ok { + return false + } + + // Check if the rule has RequestRedirect filters + hasRedirectFilter := false + for _, filter := range httpRule.Filters { + if filter.Type == gwv1.HTTPRouteFilterRequestRedirect { + hasRedirectFilter = true + break + } + } + + return hasRedirectFilter +} + +// HasRequestRedirectFilter checks if an HTTPRoute rule contains RequestRedirect filters. +// +// This function is used to identify rules that have redirect behavior, regardless of +// whether they also have backends. It's useful for: +// - Determining if redirect actions need to be built +// - Validating rule configurations +// - Logging and debugging redirect behavior +// +// Returns true if the rule is an HTTPRoute rule with at least one RequestRedirect filter. +func HasRequestRedirectFilter(rule RouteRule) bool { + rawRule := rule.GetRawRouteRule() + httpRule, ok := rawRule.(*gwv1.HTTPRouteRule) + if !ok { + return false + } + + for _, filter := range httpRule.Filters { + if filter.Type == gwv1.HTTPRouteFilterRequestRedirect { + return true + } + } + + return false +} \ No newline at end of file diff --git a/pkg/gateway/routeutils/redirect_utils_test.go b/pkg/gateway/routeutils/redirect_utils_test.go new file mode 100644 index 000000000..0e3325eb5 --- /dev/null +++ b/pkg/gateway/routeutils/redirect_utils_test.go @@ -0,0 +1,448 @@ +package routeutils + +import ( + "fmt" + "testing" + + awssdk "github.com/aws/aws-sdk-go-v2/aws" + "github.com/stretchr/testify/assert" + gwv1 "sigs.k8s.io/gateway-api/apis/v1" +) + +func TestIsRedirectOnlyRule(t *testing.T) { + tests := []struct { + name string + rule RouteRule + expected bool + }{ + { + name: "redirect-only rule with no backends", + rule: &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{}, + }, + expected: true, + }, + { + name: "rule with backends and redirect filter", + rule: &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{ + {Weight: 1}, + }, + }, + expected: false, + }, + { + name: "rule with backends but no redirect filter", + rule: &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{}, + }, + backends: []Backend{ + {Weight: 1}, + }, + }, + expected: false, + }, + { + name: "rule with no backends and no redirect filter", + rule: &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{}, + }, + backends: []Backend{}, + }, + expected: false, + }, + { + name: "rule with multiple filters including redirect", + rule: &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterExtensionRef, + }, + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Hostname: (*gwv1.PreciseHostname)(awssdk.String("example.com")), + }, + }, + }, + }, + backends: []Backend{}, + }, + expected: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := IsRedirectOnlyRule(tt.rule) + assert.Equal(t, tt.expected, result) + }) + } +} + +func TestHasRequestRedirectFilter(t *testing.T) { + tests := []struct { + name string + rule RouteRule + expected bool + }{ + { + name: "rule with redirect filter", + rule: &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + }, + expected: true, + }, + { + name: "rule without redirect filter", + rule: &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterExtensionRef, + }, + }, + }, + }, + expected: false, + }, + { + name: "rule with no filters", + rule: &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{}, + }, + }, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := HasRequestRedirectFilter(tt.rule) + assert.Equal(t, tt.expected, result) + }) + } +} + +// **Feature: httproute-redirect-only-fix, Property 1: Redirect-only rules skip target group creation** +// Property-based test to verify that redirect-only rules are correctly identified +func TestProperty_RedirectOnlyRulesSkipTargetGroupCreation(t *testing.T) { + // Test property: For any HTTPRoute rule that contains only RequestRedirect filters and no BackendRefs, + // the system should identify it as redirect-only + + // Generate various redirect-only rule configurations + redirectConfigs := []gwv1.HTTPRequestRedirectFilter{ + {Scheme: awssdk.String("https")}, + {Hostname: (*gwv1.PreciseHostname)(awssdk.String("example.com"))}, + {Port: (*gwv1.PortNumber)(awssdk.Int32(443))}, + {StatusCode: awssdk.Int(301)}, + { + Scheme: awssdk.String("https"), + Hostname: (*gwv1.PreciseHostname)(awssdk.String("secure.example.com")), + Port: (*gwv1.PortNumber)(awssdk.Int32(443)), + }, + } + + for i, redirectConfig := range redirectConfigs { + t.Run(fmt.Sprintf("redirect_config_%d", i), func(t *testing.T) { + // Create a rule with only redirect filter and no backends + rule := &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &redirectConfig, + }, + }, + }, + backends: []Backend{}, // No backends + } + + // Property: Should be identified as redirect-only + assert.True(t, IsRedirectOnlyRule(rule), "Rule with only redirect filter should be identified as redirect-only") + assert.True(t, HasRequestRedirectFilter(rule), "Rule should have redirect filter") + + // Add backends and verify it's no longer redirect-only + ruleWithBackends := &convertedHTTPRouteRule{ + rule: rule.rule, + backends: []Backend{ + {Weight: 1}, + }, + } + assert.False(t, IsRedirectOnlyRule(ruleWithBackends), "Rule with backends should not be redirect-only") + }) + } +} + +// **Feature: httproute-redirect-only-fix, Property 2: Backend rules create target groups** +// Property-based test to verify that rules with backends are not identified as redirect-only +func TestProperty_BackendRulesCreateTargetGroups(t *testing.T) { + // Test property: For any HTTPRoute rule that contains BackendRefs (with or without RequestRedirect filters), + // the system should not identify it as redirect-only + + // Generate various backend configurations + backendConfigs := [][]Backend{ + {{Weight: 1}}, + {{Weight: 50}, {Weight: 50}}, + {{Weight: 100}}, + {{Weight: 25}, {Weight: 25}, {Weight: 25}, {Weight: 25}}, + } + + // Test with and without redirect filters + redirectFilters := [][]gwv1.HTTPRouteFilter{ + {}, // No filters + {{ // With redirect filter + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }}, + {{ // With multiple filters including redirect + Type: gwv1.HTTPRouteFilterExtensionRef, + }, { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Hostname: (*gwv1.PreciseHostname)(awssdk.String("example.com")), + }, + }}, + } + + for i, backends := range backendConfigs { + for j, filters := range redirectFilters { + t.Run(fmt.Sprintf("backends_%d_filters_%d", i, j), func(t *testing.T) { + rule := &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: filters, + }, + backends: backends, + } + + // Property: Rules with backends should never be redirect-only + assert.False(t, IsRedirectOnlyRule(rule), + "Rule with backends should not be identified as redirect-only") + + // Verify backends are present + assert.Greater(t, len(rule.GetBackends()), 0, + "Rule should have backends") + }) + } + } +} + +// Property-based test for rules without backends and without redirect filters +func TestProperty_EmptyRulesNotRedirectOnly(t *testing.T) { + // Test property: Rules with no backends and no redirect filters should not be redirect-only + + nonRedirectFilters := [][]gwv1.HTTPRouteFilter{ + {}, // No filters + {{ // Extension ref only + Type: gwv1.HTTPRouteFilterExtensionRef, + }}, + {{ // URL rewrite only + Type: gwv1.HTTPRouteFilterURLRewrite, + }}, + } + + for i, filters := range nonRedirectFilters { + t.Run(fmt.Sprintf("non_redirect_filters_%d", i), func(t *testing.T) { + rule := &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: filters, + }, + backends: []Backend{}, // No backends + } + + // Property: Rules without backends and without redirect filters should not be redirect-only + assert.False(t, IsRedirectOnlyRule(rule), + "Rule without backends and redirect filters should not be redirect-only") + assert.False(t, HasRequestRedirectFilter(rule), + "Rule should not have redirect filter") + }) + } +} + +// Additional unit tests for edge cases and invalid inputs +func TestIsRedirectOnlyRule_EdgeCases(t *testing.T) { + tests := []struct { + name string + rule RouteRule + expected bool + }{ + { + name: "nil rule", + rule: nil, + expected: false, + }, + { + name: "non-HTTP rule type", + rule: &convertedTCPRouteRule{}, // Assuming this exists or create a mock + expected: false, + }, + { + name: "rule with nil redirect filter", + rule: &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: nil, + }, + }, + }, + backends: []Backend{}, + }, + expected: true, // Still considered redirect-only even with nil config + }, + { + name: "rule with empty filters array", + rule: &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{}, + }, + backends: []Backend{}, + }, + expected: false, + }, + { + name: "rule with only extension ref filter", + rule: &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterExtensionRef, + }, + }, + }, + backends: []Backend{}, + }, + expected: false, + }, + { + name: "rule with zero weight backends", + rule: &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{{Weight: 0}}, // Zero weight backend + }, + expected: false, // Has backends, so not redirect-only + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.rule == nil { + // Test nil safety + assert.False(t, false) // This would panic if not handled properly + return + } + result := IsRedirectOnlyRule(tt.rule) + assert.Equal(t, tt.expected, result) + }) + } +} + +// Mock TCP route rule for testing non-HTTP rules - using existing convertedTCPRouteRule from tcp.go + +// Test helper function behavior with various backend configurations +func TestBackendConfigurations(t *testing.T) { + tests := []struct { + name string + backends []Backend + expected string + }{ + { + name: "no backends", + backends: []Backend{}, + expected: "empty", + }, + { + name: "single backend", + backends: []Backend{{Weight: 1}}, + expected: "single", + }, + { + name: "multiple backends", + backends: []Backend{{Weight: 50}, {Weight: 50}}, + expected: "multiple", + }, + { + name: "backends with zero weights", + backends: []Backend{{Weight: 0}, {Weight: 0}}, + expected: "zero_weights", + }, + { + name: "mixed weight backends", + backends: []Backend{{Weight: 0}, {Weight: 100}}, + expected: "mixed_weights", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rule := &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: tt.backends, + } + + // Test behavior based on backend configuration + switch tt.expected { + case "empty": + assert.True(t, IsRedirectOnlyRule(rule), "Rule with no backends should be redirect-only") + case "single", "multiple", "zero_weights", "mixed_weights": + assert.False(t, IsRedirectOnlyRule(rule), "Rule with backends should not be redirect-only") + } + + assert.Equal(t, len(tt.backends), len(rule.GetBackends()), "Backend count should match") + }) + } +} \ No newline at end of file diff --git a/pkg/gateway/routeutils/resource_accounting.go b/pkg/gateway/routeutils/resource_accounting.go new file mode 100644 index 000000000..8aa2fa791 --- /dev/null +++ b/pkg/gateway/routeutils/resource_accounting.go @@ -0,0 +1,170 @@ +package routeutils + +import ( + "context" + "fmt" + + "github.com/go-logr/logr" + "k8s.io/apimachinery/pkg/types" +) + +// ResourceAccountingManager handles resource accounting for HTTPRoute rules +type ResourceAccountingManager struct { + logger logr.Logger +} + +// ResourceUsage represents resource usage statistics +type ResourceUsage struct { + TotalRules int + RedirectOnlyRules int + BackendRules int + MixedRules int + TargetGroupsRequired int + TargetGroupsSkipped int +} + +// NewResourceAccountingManager creates a new ResourceAccountingManager +func NewResourceAccountingManager(logger logr.Logger) *ResourceAccountingManager { + return &ResourceAccountingManager{ + logger: logger, + } +} + +// CalculateResourceUsage calculates resource usage for a set of route rules +func (m *ResourceAccountingManager) CalculateResourceUsage(ctx context.Context, routeKey types.NamespacedName, rules []RouteRule) ResourceUsage { + usage := ResourceUsage{ + TotalRules: len(rules), + } + + for i, rule := range rules { + hasBackends := len(rule.GetBackends()) > 0 + hasRedirectFilter := HasRequestRedirectFilter(rule) + isRedirectOnly := IsRedirectOnlyRule(rule) + + if isRedirectOnly { + usage.RedirectOnlyRules++ + usage.TargetGroupsSkipped++ + m.logger.V(1).Info("Redirect-only rule does not require target groups", + "route", routeKey, + "ruleIndex", i) + } else if hasBackends && !hasRedirectFilter { + usage.BackendRules++ + usage.TargetGroupsRequired += len(rule.GetBackends()) + m.logger.V(1).Info("Backend rule requires target groups", + "route", routeKey, + "ruleIndex", i, + "targetGroupsRequired", len(rule.GetBackends())) + } else if hasBackends && hasRedirectFilter { + usage.MixedRules++ + usage.TargetGroupsRequired += len(rule.GetBackends()) + m.logger.V(1).Info("Mixed rule requires target groups for backends", + "route", routeKey, + "ruleIndex", i, + "targetGroupsRequired", len(rule.GetBackends())) + } + } + + m.logger.Info("Calculated resource usage", + "route", routeKey, + "totalRules", usage.TotalRules, + "redirectOnlyRules", usage.RedirectOnlyRules, + "backendRules", usage.BackendRules, + "mixedRules", usage.MixedRules, + "targetGroupsRequired", usage.TargetGroupsRequired, + "targetGroupsSkipped", usage.TargetGroupsSkipped) + + return usage +} + +// ValidateResourceLimits validates that resource usage is within acceptable limits +func (m *ResourceAccountingManager) ValidateResourceLimits(ctx context.Context, usage ResourceUsage, limits ResourceLimits) error { + if usage.TargetGroupsRequired > limits.MaxTargetGroups { + return fmt.Errorf("target group limit exceeded: required %d, limit %d", + usage.TargetGroupsRequired, limits.MaxTargetGroups) + } + + if usage.TotalRules > limits.MaxRulesPerRoute { + return fmt.Errorf("rule limit exceeded: total %d, limit %d", + usage.TotalRules, limits.MaxRulesPerRoute) + } + + return nil +} + +// ResourceLimits defines resource limits for validation +type ResourceLimits struct { + MaxTargetGroups int + MaxRulesPerRoute int +} + +// DefaultResourceLimits returns default resource limits +func DefaultResourceLimits() ResourceLimits { + return ResourceLimits{ + MaxTargetGroups: 100, // AWS ALB limit per load balancer + MaxRulesPerRoute: 100, // Reasonable limit per HTTPRoute + } +} + +// GetEffectiveTargetGroupCount returns the number of target groups that will actually be created +// This excludes redirect-only rules which don't create target groups +func (m *ResourceAccountingManager) GetEffectiveTargetGroupCount(ctx context.Context, rules []RouteRule) int { + count := 0 + for _, rule := range rules { + if !IsRedirectOnlyRule(rule) { + count += len(rule.GetBackends()) + } + } + return count +} + +// OptimizeResourceUsage provides suggestions for optimizing resource usage +func (m *ResourceAccountingManager) OptimizeResourceUsage(ctx context.Context, usage ResourceUsage) []string { + var suggestions []string + + // Suggest using redirect-only rules where appropriate + if usage.BackendRules > 0 && usage.RedirectOnlyRules == 0 { + suggestions = append(suggestions, + "Consider using redirect-only rules for simple redirects to reduce target group usage") + } + + // Suggest consolidating backends if there are many single-backend rules + // This would need access to the actual rules to calculate, but we can provide general guidance + if usage.TargetGroupsRequired > usage.BackendRules+usage.MixedRules { + suggestions = append(suggestions, + "Consider consolidating multiple backends into fewer rules to optimize target group usage") + } + + // Suggest using mixed rules efficiently + if usage.MixedRules > 0 { + suggestions = append(suggestions, + "Mixed rules (redirect + backend) are efficient but ensure redirect logic is necessary") + } + + return suggestions +} + +// GenerateResourceReport generates a detailed resource usage report +func (m *ResourceAccountingManager) GenerateResourceReport(ctx context.Context, routeKey types.NamespacedName, usage ResourceUsage, limits ResourceLimits) string { + report := fmt.Sprintf("Resource Usage Report for Route %s\n", routeKey) + report += fmt.Sprintf("=====================================\n") + report += fmt.Sprintf("Total Rules: %d\n", usage.TotalRules) + report += fmt.Sprintf(" - Redirect-only Rules: %d\n", usage.RedirectOnlyRules) + report += fmt.Sprintf(" - Backend Rules: %d\n", usage.BackendRules) + report += fmt.Sprintf(" - Mixed Rules: %d\n", usage.MixedRules) + report += fmt.Sprintf("\n") + report += fmt.Sprintf("Target Group Usage:\n") + report += fmt.Sprintf(" - Required: %d\n", usage.TargetGroupsRequired) + report += fmt.Sprintf(" - Skipped (redirect-only): %d\n", usage.TargetGroupsSkipped) + report += fmt.Sprintf(" - Efficiency: %.1f%% (skipped/total)\n", + float64(usage.TargetGroupsSkipped)/float64(usage.TotalRules)*100) + report += fmt.Sprintf("\n") + report += fmt.Sprintf("Resource Limits:\n") + report += fmt.Sprintf(" - Target Groups: %d/%d (%.1f%%)\n", + usage.TargetGroupsRequired, limits.MaxTargetGroups, + float64(usage.TargetGroupsRequired)/float64(limits.MaxTargetGroups)*100) + report += fmt.Sprintf(" - Rules: %d/%d (%.1f%%)\n", + usage.TotalRules, limits.MaxRulesPerRoute, + float64(usage.TotalRules)/float64(limits.MaxRulesPerRoute)*100) + + return report +} \ No newline at end of file diff --git a/pkg/gateway/routeutils/resource_accounting_test.go b/pkg/gateway/routeutils/resource_accounting_test.go new file mode 100644 index 000000000..1d68bd6f2 --- /dev/null +++ b/pkg/gateway/routeutils/resource_accounting_test.go @@ -0,0 +1,533 @@ +package routeutils + +import ( + "context" + "fmt" + "strings" + "testing" + + awssdk "github.com/aws/aws-sdk-go-v2/aws" + "github.com/stretchr/testify/assert" + "k8s.io/apimachinery/pkg/types" + gwv1 "sigs.k8s.io/gateway-api/apis/v1" + "sigs.k8s.io/controller-runtime/pkg/log/zap" +) + +func TestResourceAccountingManager_CalculateResourceUsage(t *testing.T) { + logger := zap.New(zap.UseDevMode(true)) + manager := NewResourceAccountingManager(logger) + ctx := context.Background() + routeKey := types.NamespacedName{Namespace: "test", Name: "test-route"} + + tests := []struct { + name string + rules []RouteRule + expected ResourceUsage + }{ + { + name: "mixed rules scenario", + rules: []RouteRule{ + // Redirect-only rule + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{}, + }, + // Backend rule + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{Filters: []gwv1.HTTPRouteFilter{}}, + backends: []Backend{{Weight: 1}}, + }, + // Mixed rule + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Hostname: (*gwv1.PreciseHostname)(awssdk.String("example.com")), + }, + }, + }, + }, + backends: []Backend{{Weight: 50}, {Weight: 50}}, + }, + }, + expected: ResourceUsage{ + TotalRules: 3, + RedirectOnlyRules: 1, + BackendRules: 1, + MixedRules: 1, + TargetGroupsRequired: 3, // 1 from backend rule + 2 from mixed rule + TargetGroupsSkipped: 1, // 1 from redirect-only rule + }, + }, + { + name: "all redirect-only rules", + rules: []RouteRule{ + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{}, + }, + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Port: (*gwv1.PortNumber)(awssdk.Int32(443)), + }, + }, + }, + }, + backends: []Backend{}, + }, + }, + expected: ResourceUsage{ + TotalRules: 2, + RedirectOnlyRules: 2, + BackendRules: 0, + MixedRules: 0, + TargetGroupsRequired: 0, + TargetGroupsSkipped: 2, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + usage := manager.CalculateResourceUsage(ctx, routeKey, tt.rules) + assert.Equal(t, tt.expected, usage) + }) + } +} + +// **Feature: httproute-redirect-only-fix, Property 8: Resource accounting accuracy** +// Property-based test to verify resource accounting accuracy for redirect-only rules +func TestProperty_ResourceAccountingAccuracy(t *testing.T) { + // Test property: For any redirect-only HTTPRoute rule, the system should not + // count it toward target group limits or quotas + + logger := zap.New(zap.UseDevMode(true)) + manager := NewResourceAccountingManager(logger) + ctx := context.Background() + + // Generate various resource accounting scenarios + accountingScenarios := []struct { + name string + rules []RouteRule + }{ + { + name: "single_redirect_only", + rules: []RouteRule{ + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{}, + }, + }, + }, + { + name: "multiple_redirect_only", + rules: []RouteRule{ + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{}, + }, + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Hostname: (*gwv1.PreciseHostname)(awssdk.String("example.com")), + }, + }, + }, + }, + backends: []Backend{}, + }, + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Port: (*gwv1.PortNumber)(awssdk.Int32(443)), + }, + }, + }, + }, + backends: []Backend{}, + }, + }, + }, + { + name: "mixed_with_many_redirects", + rules: []RouteRule{ + // Multiple redirect-only rules + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{}, + }, + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Hostname: (*gwv1.PreciseHostname)(awssdk.String("redirect1.example.com")), + }, + }, + }, + }, + backends: []Backend{}, + }, + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Hostname: (*gwv1.PreciseHostname)(awssdk.String("redirect2.example.com")), + }, + }, + }, + }, + backends: []Backend{}, + }, + // One backend rule + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{Filters: []gwv1.HTTPRouteFilter{}}, + backends: []Backend{{Weight: 1}}, + }, + }, + }, + } + + for _, scenario := range accountingScenarios { + t.Run(scenario.name, func(t *testing.T) { + routeKey := types.NamespacedName{ + Namespace: "test", + Name: fmt.Sprintf("accounting-%s", scenario.name), + } + + // Property: Resource usage calculation should be accurate + usage := manager.CalculateResourceUsage(ctx, routeKey, scenario.rules) + + // Property: Total rules should match input + assert.Equal(t, len(scenario.rules), usage.TotalRules, + "Total rules should match input rule count") + + // Property: Redirect-only rules should not count toward target groups + redirectOnlyCount := 0 + expectedTargetGroups := 0 + for _, rule := range scenario.rules { + if IsRedirectOnlyRule(rule) { + redirectOnlyCount++ + } else { + expectedTargetGroups += len(rule.GetBackends()) + } + } + + assert.Equal(t, redirectOnlyCount, usage.RedirectOnlyRules, + "Redirect-only rule count should be accurate") + assert.Equal(t, redirectOnlyCount, usage.TargetGroupsSkipped, + "Target groups skipped should equal redirect-only rules") + assert.Equal(t, expectedTargetGroups, usage.TargetGroupsRequired, + "Target groups required should exclude redirect-only rules") + + // Property: Effective target group count should exclude redirect-only rules + effectiveCount := manager.GetEffectiveTargetGroupCount(ctx, scenario.rules) + assert.Equal(t, expectedTargetGroups, effectiveCount, + "Effective target group count should exclude redirect-only rules") + + // Property: Rule categorization should be complete and non-overlapping + totalCategorized := usage.RedirectOnlyRules + usage.BackendRules + usage.MixedRules + assert.Equal(t, usage.TotalRules, totalCategorized, + "All rules should be categorized exactly once") + }) + } +} + +func TestValidateResourceLimits(t *testing.T) { + logger := zap.New(zap.UseDevMode(true)) + manager := NewResourceAccountingManager(logger) + ctx := context.Background() + + limits := ResourceLimits{ + MaxTargetGroups: 5, + MaxRulesPerRoute: 10, + } + + tests := []struct { + name string + usage ResourceUsage + expectError bool + errorMsg string + }{ + { + name: "within limits", + usage: ResourceUsage{ + TotalRules: 3, + TargetGroupsRequired: 2, + }, + expectError: false, + }, + { + name: "exceeds target group limit", + usage: ResourceUsage{ + TotalRules: 3, + TargetGroupsRequired: 6, + }, + expectError: true, + errorMsg: "target group limit exceeded", + }, + { + name: "exceeds rule limit", + usage: ResourceUsage{ + TotalRules: 15, + TargetGroupsRequired: 2, + }, + expectError: true, + errorMsg: "rule limit exceeded", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := manager.ValidateResourceLimits(ctx, tt.usage, limits) + + if tt.expectError { + assert.Error(t, err) + assert.Contains(t, err.Error(), tt.errorMsg) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestOptimizeResourceUsage(t *testing.T) { + logger := zap.New(zap.UseDevMode(true)) + manager := NewResourceAccountingManager(logger) + ctx := context.Background() + + tests := []struct { + name string + usage ResourceUsage + expectedSuggestions int + containsSuggestion string + }{ + { + name: "no redirect rules suggests using them", + usage: ResourceUsage{ + BackendRules: 5, + RedirectOnlyRules: 0, + }, + expectedSuggestions: 1, + containsSuggestion: "redirect-only rules", + }, + { + name: "many target groups suggests consolidation", + usage: ResourceUsage{ + BackendRules: 2, + MixedRules: 1, + TargetGroupsRequired: 10, + }, + expectedSuggestions: 1, + containsSuggestion: "consolidating", + }, + { + name: "mixed rules provides guidance", + usage: ResourceUsage{ + MixedRules: 3, + }, + expectedSuggestions: 1, + containsSuggestion: "Mixed rules", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + suggestions := manager.OptimizeResourceUsage(ctx, tt.usage) + + assert.GreaterOrEqual(t, len(suggestions), tt.expectedSuggestions) + + if tt.containsSuggestion != "" { + found := false + for _, suggestion := range suggestions { + if strings.Contains(suggestion, tt.containsSuggestion) { + found = true + break + } + } + assert.True(t, found, "Should contain expected suggestion") + } + }) + } +} + +func TestGenerateResourceReport(t *testing.T) { + logger := zap.New(zap.UseDevMode(true)) + manager := NewResourceAccountingManager(logger) + ctx := context.Background() + + routeKey := types.NamespacedName{Namespace: "test", Name: "test-route"} + usage := ResourceUsage{ + TotalRules: 5, + RedirectOnlyRules: 2, + BackendRules: 2, + MixedRules: 1, + TargetGroupsRequired: 4, + TargetGroupsSkipped: 2, + } + limits := DefaultResourceLimits() + + report := manager.GenerateResourceReport(ctx, routeKey, usage, limits) + + // Verify report contains expected information + assert.Contains(t, report, "Resource Usage Report") + assert.Contains(t, report, routeKey.String()) + assert.Contains(t, report, "Total Rules: 5") + assert.Contains(t, report, "Redirect-only Rules: 2") + assert.Contains(t, report, "Backend Rules: 2") + assert.Contains(t, report, "Mixed Rules: 1") + assert.Contains(t, report, "Required: 4") + assert.Contains(t, report, "Skipped (redirect-only): 2") + assert.Contains(t, report, "Efficiency:") +} + +// Property-based test for resource accounting consistency +func TestProperty_ResourceAccountingConsistency(t *testing.T) { + // Test property: Resource accounting should be consistent across different rule configurations + + logger := zap.New(zap.UseDevMode(true)) + manager := NewResourceAccountingManager(logger) + ctx := context.Background() + + // Generate various rule configurations for consistency testing + consistencyTests := []struct { + name string + rules []RouteRule + }{ + { + name: "all_redirect_only", + rules: []RouteRule{ + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{}, + }, + }, + }, + { + name: "all_backend_only", + rules: []RouteRule{ + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{Filters: []gwv1.HTTPRouteFilter{}}, + backends: []Backend{{Weight: 1}}, + }, + }, + }, + { + name: "all_mixed", + rules: []RouteRule{ + &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{{Weight: 100}}, + }, + }, + }, + } + + for _, test := range consistencyTests { + t.Run(test.name, func(t *testing.T) { + routeKey := types.NamespacedName{ + Namespace: "test", + Name: fmt.Sprintf("consistency-%s", test.name), + } + + usage := manager.CalculateResourceUsage(ctx, routeKey, test.rules) + + // Property: Consistency checks + assert.Equal(t, len(test.rules), usage.TotalRules, + "Total rules should match input") + + // Property: Rule categorization should be mutually exclusive and complete + totalCategorized := usage.RedirectOnlyRules + usage.BackendRules + usage.MixedRules + assert.Equal(t, usage.TotalRules, totalCategorized, + "All rules should be categorized exactly once") + + // Property: Target group accounting should be consistent + expectedSkipped := usage.RedirectOnlyRules + assert.Equal(t, expectedSkipped, usage.TargetGroupsSkipped, + "Target groups skipped should equal redirect-only rules") + + // Property: Effective count should match required count + effectiveCount := manager.GetEffectiveTargetGroupCount(ctx, test.rules) + assert.Equal(t, usage.TargetGroupsRequired, effectiveCount, + "Effective target group count should match required count") + }) + } +} \ No newline at end of file diff --git a/pkg/gateway/routeutils/validation.go b/pkg/gateway/routeutils/validation.go new file mode 100644 index 000000000..8c52da561 --- /dev/null +++ b/pkg/gateway/routeutils/validation.go @@ -0,0 +1,245 @@ +package routeutils + +import ( + "fmt" + + gwv1 "sigs.k8s.io/gateway-api/apis/v1" +) + +// ValidationError represents a validation error with context +type ValidationError struct { + Field string + Message string + Rule RouteRule +} + +func (e ValidationError) Error() string { + return fmt.Sprintf("validation error in field %s: %s", e.Field, e.Message) +} + +// ValidateHTTPRouteRule validates an HTTPRoute rule configuration +func ValidateHTTPRouteRule(rule RouteRule) []ValidationError { + var errors []ValidationError + + // Validate redirect-only rules + if IsRedirectOnlyRule(rule) { + errors = append(errors, validateRedirectOnlyRule(rule)...) + } + + // Validate rules with backends + if len(rule.GetBackends()) > 0 { + errors = append(errors, validateBackendRule(rule)...) + } + + // Validate mixed rules (both redirect and backends) + if HasRequestRedirectFilter(rule) && len(rule.GetBackends()) > 0 { + errors = append(errors, validateMixedRule(rule)...) + } + + // Validate that rule has either backends or redirect filters (not empty) + if len(rule.GetBackends()) == 0 && !HasRequestRedirectFilter(rule) { + errors = append(errors, ValidationError{ + Field: "rule", + Message: "rule must have either backends or redirect filters", + Rule: rule, + }) + } + + return errors +} + +// validateRedirectOnlyRule validates redirect-only rule configuration +func validateRedirectOnlyRule(rule RouteRule) []ValidationError { + var errors []ValidationError + + rawRule := rule.GetRawRouteRule() + httpRule, ok := rawRule.(*gwv1.HTTPRouteRule) + if !ok { + errors = append(errors, ValidationError{ + Field: "rule.type", + Message: "redirect-only rule must be HTTPRouteRule", + Rule: rule, + }) + return errors + } + + // Validate that redirect filters are properly configured + redirectFilterCount := 0 + for i, filter := range httpRule.Filters { + if filter.Type == gwv1.HTTPRouteFilterRequestRedirect { + redirectFilterCount++ + if filter.RequestRedirect == nil { + errors = append(errors, ValidationError{ + Field: fmt.Sprintf("rule.filters[%d].requestRedirect", i), + Message: "RequestRedirect filter must have configuration", + Rule: rule, + }) + } else { + // Validate redirect configuration + errors = append(errors, validateRedirectConfiguration(filter.RequestRedirect, rule, i)...) + } + } + } + + if redirectFilterCount == 0 { + errors = append(errors, ValidationError{ + Field: "rule.filters", + Message: "redirect-only rule must have at least one RequestRedirect filter", + Rule: rule, + }) + } + + // Validate that there are no backends + if len(rule.GetBackends()) > 0 { + errors = append(errors, ValidationError{ + Field: "rule.backends", + Message: "redirect-only rule must not have backends", + Rule: rule, + }) + } + + return errors +} + +// validateBackendRule validates rules with backends +func validateBackendRule(rule RouteRule) []ValidationError { + var errors []ValidationError + + backends := rule.GetBackends() + if len(backends) == 0 { + errors = append(errors, ValidationError{ + Field: "rule.backends", + Message: "backend rule must have at least one backend", + Rule: rule, + }) + return errors + } + + // Validate backend weights + totalWeight := 0 + for i, backend := range backends { + if backend.Weight < 0 { + errors = append(errors, ValidationError{ + Field: fmt.Sprintf("rule.backends[%d].weight", i), + Message: "backend weight must be non-negative", + Rule: rule, + }) + } + if backend.Weight > 999 { + errors = append(errors, ValidationError{ + Field: fmt.Sprintf("rule.backends[%d].weight", i), + Message: "backend weight must not exceed 999", + Rule: rule, + }) + } + totalWeight += backend.Weight + } + + // Validate that at least one backend has positive weight + if totalWeight == 0 { + errors = append(errors, ValidationError{ + Field: "rule.backends", + Message: "at least one backend must have positive weight", + Rule: rule, + }) + } + + return errors +} + +// validateMixedRule validates rules with both redirect filters and backends +func validateMixedRule(rule RouteRule) []ValidationError { + var errors []ValidationError + + // Mixed rules are valid according to Gateway API spec + // Both redirect and backend processing should be supported + + // Validate that both components are properly configured + if !HasRequestRedirectFilter(rule) { + errors = append(errors, ValidationError{ + Field: "rule.filters", + Message: "mixed rule must have redirect filter", + Rule: rule, + }) + } + + if len(rule.GetBackends()) == 0 { + errors = append(errors, ValidationError{ + Field: "rule.backends", + Message: "mixed rule must have backends", + Rule: rule, + }) + } + + return errors +} + +// validateRedirectConfiguration validates redirect filter configuration +func validateRedirectConfiguration(redirect *gwv1.HTTPRequestRedirectFilter, rule RouteRule, filterIndex int) []ValidationError { + var errors []ValidationError + + if redirect == nil { + errors = append(errors, ValidationError{ + Field: fmt.Sprintf("rule.filters[%d].requestRedirect", filterIndex), + Message: "redirect configuration cannot be nil", + Rule: rule, + }) + return errors + } + + // Validate scheme + if redirect.Scheme != nil { + scheme := *redirect.Scheme + if scheme != "http" && scheme != "https" { + errors = append(errors, ValidationError{ + Field: fmt.Sprintf("rule.filters[%d].requestRedirect.scheme", filterIndex), + Message: fmt.Sprintf("invalid scheme '%s', must be 'http' or 'https'", scheme), + Rule: rule, + }) + } + } + + // Validate status code + if redirect.StatusCode != nil { + code := *redirect.StatusCode + if code < 300 || code > 399 { + errors = append(errors, ValidationError{ + Field: fmt.Sprintf("rule.filters[%d].requestRedirect.statusCode", filterIndex), + Message: fmt.Sprintf("invalid status code %d, must be between 300-399", code), + Rule: rule, + }) + } + } + + // Validate port + if redirect.Port != nil { + port := *redirect.Port + if port < 1 || port > 65535 { + errors = append(errors, ValidationError{ + Field: fmt.Sprintf("rule.filters[%d].requestRedirect.port", filterIndex), + Message: fmt.Sprintf("invalid port %d, must be between 1-65535", port), + Rule: rule, + }) + } + } + + return errors +} + +// FormatValidationErrors formats validation errors into a user-friendly message +func FormatValidationErrors(errors []ValidationError) error { + if len(errors) == 0 { + return nil + } + + if len(errors) == 1 { + return errors[0] + } + + message := fmt.Sprintf("multiple validation errors (%d):", len(errors)) + for i, err := range errors { + message += fmt.Sprintf("\n %d. %s", i+1, err.Error()) + } + + return fmt.Errorf("%s", message) +} \ No newline at end of file diff --git a/pkg/gateway/routeutils/validation_test.go b/pkg/gateway/routeutils/validation_test.go new file mode 100644 index 000000000..919e94926 --- /dev/null +++ b/pkg/gateway/routeutils/validation_test.go @@ -0,0 +1,373 @@ +package routeutils + +import ( + "fmt" + "testing" + + awssdk "github.com/aws/aws-sdk-go-v2/aws" + "github.com/stretchr/testify/assert" + gwv1 "sigs.k8s.io/gateway-api/apis/v1" +) + +func TestValidateHTTPRouteRule(t *testing.T) { + tests := []struct { + name string + rule RouteRule + expectedErrors int + errorContains []string + }{ + { + name: "valid redirect-only rule", + rule: &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{}, + }, + expectedErrors: 0, + }, + { + name: "valid backend rule", + rule: &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{}, + }, + backends: []Backend{{Weight: 1}}, + }, + expectedErrors: 0, + }, + { + name: "valid mixed rule", + rule: &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + }, + backends: []Backend{{Weight: 1}}, + }, + expectedErrors: 0, + }, + { + name: "invalid empty rule", + rule: &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{}, + }, + backends: []Backend{}, + }, + expectedErrors: 1, + errorContains: []string{"must have either backends or redirect filters"}, + }, + { + name: "invalid redirect configuration", + rule: &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: nil, + }, + }, + }, + backends: []Backend{}, + }, + expectedErrors: 1, + errorContains: []string{"RequestRedirect filter must have configuration"}, + }, + { + name: "invalid backend weights", + rule: &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{}, + }, + backends: []Backend{{Weight: -1}, {Weight: 1000}}, + }, + expectedErrors: 2, + errorContains: []string{"weight must be non-negative", "weight must not exceed 999"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + errors := ValidateHTTPRouteRule(tt.rule) + assert.Equal(t, tt.expectedErrors, len(errors)) + + if tt.expectedErrors > 0 { + errorMessage := FormatValidationErrors(errors).Error() + for _, contains := range tt.errorContains { + assert.Contains(t, errorMessage, contains) + } + } + }) + } +} + +// **Feature: httproute-redirect-only-fix, Property 5: Validation and error handling consistency** +// Property-based test to verify validation and error handling consistency +func TestProperty_ValidationAndErrorHandlingConsistency(t *testing.T) { + // Test property: For any invalid HTTPRoute configuration, the system should provide + // clear error messages and update route status with appropriate condition messages + + // Generate various invalid configurations + invalidConfigs := []struct { + name string + rule RouteRule + expectError bool + errorType string + }{ + { + name: "redirect_with_invalid_scheme", + rule: &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("ftp"), // Invalid scheme + }, + }, + }, + }, + backends: []Backend{}, + }, + expectError: true, + errorType: "invalid_scheme", + }, + { + name: "redirect_with_invalid_status_code", + rule: &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + StatusCode: awssdk.Int(200), // Invalid status code for redirect + }, + }, + }, + }, + backends: []Backend{}, + }, + expectError: true, + errorType: "invalid_status_code", + }, + { + name: "redirect_with_invalid_port", + rule: &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Port: (*gwv1.PortNumber)(awssdk.Int32(70000)), // Invalid port + }, + }, + }, + }, + backends: []Backend{}, + }, + expectError: true, + errorType: "invalid_port", + }, + { + name: "backend_with_zero_weights", + rule: &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{}, + }, + backends: []Backend{{Weight: 0}, {Weight: 0}}, // All zero weights + }, + expectError: true, + errorType: "zero_weights", + }, + { + name: "backend_with_negative_weight", + rule: &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{}, + }, + backends: []Backend{{Weight: -5}}, // Negative weight + }, + expectError: true, + errorType: "negative_weight", + }, + } + + for _, config := range invalidConfigs { + t.Run(config.name, func(t *testing.T) { + errors := ValidateHTTPRouteRule(config.rule) + + if config.expectError { + // Property: Invalid configurations should produce validation errors + assert.Greater(t, len(errors), 0, + "Invalid configuration should produce validation errors") + + // Property: Error messages should be clear and specific + errorMessage := FormatValidationErrors(errors) + assert.NotNil(t, errorMessage, "Should have formatted error message") + assert.NotEmpty(t, errorMessage.Error(), "Error message should not be empty") + + // Property: Error messages should contain relevant context + switch config.errorType { + case "invalid_scheme": + assert.Contains(t, errorMessage.Error(), "scheme", + "Error should mention scheme validation") + case "invalid_status_code": + assert.Contains(t, errorMessage.Error(), "status code", + "Error should mention status code validation") + case "invalid_port": + assert.Contains(t, errorMessage.Error(), "port", + "Error should mention port validation") + case "zero_weights": + assert.Contains(t, errorMessage.Error(), "positive weight", + "Error should mention weight validation") + case "negative_weight": + assert.Contains(t, errorMessage.Error(), "non-negative", + "Error should mention weight validation") + } + + // Property: Each error should have field context + for _, err := range errors { + assert.NotEmpty(t, err.Field, "Error should have field context") + assert.NotEmpty(t, err.Message, "Error should have message") + assert.NotNil(t, err.Rule, "Error should reference the rule") + } + } else { + // Property: Valid configurations should not produce errors + assert.Equal(t, 0, len(errors), + "Valid configuration should not produce validation errors") + } + }) + } +} + +// Property-based test for error message formatting consistency +func TestProperty_ErrorMessageFormattingConsistency(t *testing.T) { + // Test property: Error message formatting should be consistent across different error types + + // Generate various error scenarios + errorScenarios := [][]ValidationError{ + // Single error + { + {Field: "rule.backends", Message: "must have at least one backend", Rule: nil}, + }, + // Multiple errors + { + {Field: "rule.backends[0].weight", Message: "weight must be non-negative", Rule: nil}, + {Field: "rule.backends[1].weight", Message: "weight must not exceed 999", Rule: nil}, + }, + // Complex errors + { + {Field: "rule.filters[0].requestRedirect.scheme", Message: "invalid scheme 'ftp'", Rule: nil}, + {Field: "rule.filters[0].requestRedirect.statusCode", Message: "invalid status code 200", Rule: nil}, + {Field: "rule.backends", Message: "must have at least one backend", Rule: nil}, + }, + } + + for i, errors := range errorScenarios { + t.Run(fmt.Sprintf("error_scenario_%d", i), func(t *testing.T) { + formattedError := FormatValidationErrors(errors) + + if len(errors) == 0 { + // Property: No errors should result in nil + assert.Nil(t, formattedError, "No errors should result in nil") + } else if len(errors) == 1 { + // Property: Single error should be returned as-is + assert.Equal(t, errors[0].Error(), formattedError.Error(), + "Single error should be formatted consistently") + } else { + // Property: Multiple errors should be formatted with numbering + errorMessage := formattedError.Error() + assert.Contains(t, errorMessage, "multiple validation errors", + "Multiple errors should indicate count") + assert.Contains(t, errorMessage, fmt.Sprintf("(%d)", len(errors)), + "Error count should be included") + + // Property: Each error should be numbered + for j := range errors { + assert.Contains(t, errorMessage, fmt.Sprintf("%d.", j+1), + "Each error should be numbered") + } + } + }) + } +} + +// Property-based test for validation completeness +func TestProperty_ValidationCompleteness(t *testing.T) { + // Test property: Validation should cover all aspects of HTTPRoute rule configuration + + // Test comprehensive validation scenarios + comprehensiveTests := []struct { + name string + rule RouteRule + shouldValidate bool + aspects []string + }{ + { + name: "redirect_only_comprehensive", + rule: &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + Hostname: (*gwv1.PreciseHostname)(awssdk.String("example.com")), + Port: (*gwv1.PortNumber)(awssdk.Int32(443)), + StatusCode: awssdk.Int(301), + }, + }, + }, + }, + backends: []Backend{}, + }, + shouldValidate: true, + aspects: []string{"redirect_filter", "no_backends", "scheme", "hostname", "port", "status_code"}, + }, + { + name: "backend_comprehensive", + rule: &convertedHTTPRouteRule{ + rule: &gwv1.HTTPRouteRule{ + Filters: []gwv1.HTTPRouteFilter{}, + }, + backends: []Backend{ + {Weight: 25}, + {Weight: 50}, + {Weight: 25}, + }, + }, + shouldValidate: true, + aspects: []string{"backends", "weights", "no_redirect"}, + }, + } + + for _, test := range comprehensiveTests { + t.Run(test.name, func(t *testing.T) { + errors := ValidateHTTPRouteRule(test.rule) + + if test.shouldValidate { + // Property: Valid comprehensive configurations should pass validation + assert.Equal(t, 0, len(errors), + "Comprehensive valid configuration should pass validation") + } + + // Property: Validation should check all specified aspects + // This is verified by the absence of errors for valid configurations + // and the presence of specific errors for invalid configurations + }) + } +} \ No newline at end of file diff --git a/test/e2e/gateway/redirect_only_test.go b/test/e2e/gateway/redirect_only_test.go new file mode 100644 index 000000000..5c500aa76 --- /dev/null +++ b/test/e2e/gateway/redirect_only_test.go @@ -0,0 +1,484 @@ +package gateway + +import ( + "context" + "fmt" + "time" + + awssdk "github.com/aws/aws-sdk-go-v2/aws" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + elbv2gw "sigs.k8s.io/aws-load-balancer-controller/apis/gateway/v1beta1" + "sigs.k8s.io/aws-load-balancer-controller/test/framework/verifier" + gwv1 "sigs.k8s.io/gateway-api/apis/v1" + gwalpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" +) + +var _ = Describe("HTTPRoute Redirect-Only Rules", func() { + var ( + ctx context.Context + stack *albResourceStack + gwListeners []gwv1.Listener + lbcSpec *elbv2gw.LoadBalancerConfiguration + tgSpec *elbv2gw.TargetGroupConfiguration + ) + + BeforeEach(func() { + ctx = context.Background() + + // Setup basic gateway listeners + gwListeners = []gwv1.Listener{ + { + Name: "http", + Port: 80, + Protocol: gwv1.HTTPProtocolType, + }, + { + Name: "https", + Port: 443, + Protocol: gwv1.HTTPSProtocolType, + }, + } + + // Setup load balancer configuration + lbcSpec = &elbv2gw.LoadBalancerConfiguration{ + ObjectMeta: metav1.ObjectMeta{ + Name: defaultLbConfigName, + Namespace: k8sClient.Namespace(), + }, + Spec: elbv2gw.LoadBalancerConfigurationSpec{ + LoadBalancerName: awssdk.String("redirect-only-test-alb"), + }, + } + + // Setup target group configuration (for backend rules) + tgSpec = &elbv2gw.TargetGroupConfiguration{ + ObjectMeta: metav1.ObjectMeta{ + Name: defaultTgConfigName, + Namespace: k8sClient.Namespace(), + }, + Spec: elbv2gw.TargetGroupConfigurationSpec{ + TargetReference: elbv2gw.TargetReference{ + Name: defaultName, + }, + }, + } + }) + + Context("Redirect-Only HTTPRoute Rules", func() { + It("should create ALB listener rules without target groups for redirect-only rules", func() { + // Create HTTPRoute with redirect-only rules + httpRouteRules := []gwv1.HTTPRouteRule{ + { + Matches: []gwv1.HTTPRouteMatch{ + { + Path: &gwv1.HTTPPathMatch{ + Type: &[]gwv1.PathMatchType{gwv1.PathMatchExact}[0], + Value: awssdk.String("/redirect-to-https"), + }, + }, + }, + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + StatusCode: awssdk.Int(301), + }, + }, + }, + // No BackendRefs - this is redirect-only + }, + { + Matches: []gwv1.HTTPRouteMatch{ + { + Path: &gwv1.HTTPPathMatch{ + Type: &[]gwv1.PathMatchType{gwv1.PathMatchPathPrefix}[0], + Value: awssdk.String("/old-site"), + }, + }, + }, + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Hostname: (*gwv1.PreciseHostname)(awssdk.String("new-site.example.com")), + StatusCode: awssdk.Int(302), + }, + }, + }, + // No BackendRefs - this is redirect-only + }, + } + + httpr := buildHTTPRoute([]string{}, httpRouteRules, nil) + + By("deploying stack with redirect-only HTTPRoute", func() { + stack = newALBResourceStack( + nil, // No deployments needed for redirect-only + nil, // No services needed for redirect-only + buildGatewayClass(), + buildGateway(gwListeners), + lbcSpec, + nil, // No target group configs needed + nil, // No listener rule configs + []*gwv1.HTTPRoute{httpr}, + nil, // No GRPC routes + nil, // No OIDC secret + "redirect-only-test", + false, // No pod readiness gate needed + ) + + err := stack.DeployHTTP(ctx, nil, tf, gwListeners, []*gwv1.HTTPRoute{httpr}, lbcSpec, nil, nil, nil, false) + Expect(err).NotTo(HaveOccurred()) + }) + + By("verifying ALB is created", func() { + Eventually(func() error { + return verifier.NewALBVerifier(stack.LoadBalancer, tf.LBC.ELBv2Client, tf.LBC.EC2Client, tf.Logger). + VerifyALBExists(ctx) + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + }) + + By("verifying listener rules are created for redirect-only rules", func() { + Eventually(func() error { + return verifier.NewALBVerifier(stack.LoadBalancer, tf.LBC.ELBv2Client, tf.LBC.EC2Client, tf.Logger). + VerifyListenerRulesCount(ctx, 80, 2) // Should have 2 redirect rules + }, 1*time.Minute, 5*time.Second).Should(Succeed()) + }) + + By("verifying no target groups are created for redirect-only rules", func() { + Consistently(func() error { + // Verify that no target groups exist for this load balancer + return verifier.NewALBVerifier(stack.LoadBalancer, tf.LBC.ELBv2Client, tf.LBC.EC2Client, tf.Logger). + VerifyTargetGroupCount(ctx, 0) // Should have 0 target groups + }, 30*time.Second, 5*time.Second).Should(Succeed()) + }) + + By("verifying redirect functionality works", func() { + // Test the redirect behavior (if possible in test environment) + // This would require actual HTTP requests to the ALB + Skip("Redirect functionality testing requires live ALB endpoint") + }) + + By("cleaning up resources", func() { + err := stack.Cleanup(ctx, tf) + Expect(err).NotTo(HaveOccurred()) + }) + }) + + It("should handle mixed rules (redirect-only and backend rules) correctly", func() { + // Create deployment and service for backend rules + deployment := buildDeployment(defaultName, defaultNumReplicas, appContainerPort) + service := buildService(defaultName, appContainerPort, corev1.ServiceTypeClusterIP) + + // Create HTTPRoute with mixed rules + httpRouteRules := []gwv1.HTTPRouteRule{ + { + // Redirect-only rule + Matches: []gwv1.HTTPRouteMatch{ + { + Path: &gwv1.HTTPPathMatch{ + Type: &[]gwv1.PathMatchType{gwv1.PathMatchExact}[0], + Value: awssdk.String("/redirect"), + }, + }, + }, + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + StatusCode: awssdk.Int(301), + }, + }, + }, + // No BackendRefs + }, + { + // Backend rule + Matches: []gwv1.HTTPRouteMatch{ + { + Path: &gwv1.HTTPPathMatch{ + Type: &[]gwv1.PathMatchType{gwv1.PathMatchPathPrefix}[0], + Value: awssdk.String("/app"), + }, + }, + }, + BackendRefs: DefaultHttpRouteRuleBackendRefs, + // No redirect filters + }, + { + // Mixed rule (both redirect and backend) + Matches: []gwv1.HTTPRouteMatch{ + { + Path: &gwv1.HTTPPathMatch{ + Type: &[]gwv1.PathMatchType{gwv1.PathMatchExact}[0], + Value: awssdk.String("/mixed"), + }, + }, + }, + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Port: (*gwv1.PortNumber)(awssdk.Int32(443)), + }, + }, + }, + BackendRefs: DefaultHttpRouteRuleBackendRefs, + }, + } + + httpr := buildHTTPRoute([]string{}, httpRouteRules, nil) + + By("deploying stack with mixed HTTPRoute rules", func() { + stack = newALBResourceStack( + []*appsv1.Deployment{deployment}, + []*corev1.Service{service}, + buildGatewayClass(), + buildGateway(gwListeners), + lbcSpec, + []*elbv2gw.TargetGroupConfiguration{tgSpec}, + nil, + []*gwv1.HTTPRoute{httpr}, + nil, + nil, + "mixed-rules-test", + true, // Enable pod readiness gate for backend rules + ) + + err := stack.DeployHTTP(ctx, nil, tf, gwListeners, []*gwv1.HTTPRoute{httpr}, lbcSpec, tgSpec, nil, nil, true) + Expect(err).NotTo(HaveOccurred()) + }) + + By("verifying ALB is created", func() { + Eventually(func() error { + return verifier.NewALBVerifier(stack.LoadBalancer, tf.LBC.ELBv2Client, tf.LBC.EC2Client, tf.Logger). + VerifyALBExists(ctx) + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + }) + + By("verifying correct number of listener rules", func() { + Eventually(func() error { + return verifier.NewALBVerifier(stack.LoadBalancer, tf.LBC.ELBv2Client, tf.LBC.EC2Client, tf.Logger). + VerifyListenerRulesCount(ctx, 80, 3) // Should have 3 rules total + }, 1*time.Minute, 5*time.Second).Should(Succeed()) + }) + + By("verifying target groups are created only for backend rules", func() { + Eventually(func() error { + // Should have 2 target groups: 1 for backend rule + 1 for mixed rule + return verifier.NewALBVerifier(stack.LoadBalancer, tf.LBC.ELBv2Client, tf.LBC.EC2Client, tf.Logger). + VerifyTargetGroupCount(ctx, 2) + }, 1*time.Minute, 5*time.Second).Should(Succeed()) + }) + + By("verifying target group health", func() { + Eventually(func() error { + return verifier.NewALBVerifier(stack.LoadBalancer, tf.LBC.ELBv2Client, tf.LBC.EC2Client, tf.Logger). + VerifyTargetGroupHealthy(ctx) + }, 2*time.Minute, 10*time.Second).Should(Succeed()) + }) + + By("cleaning up resources", func() { + err := stack.Cleanup(ctx, tf) + Expect(err).NotTo(HaveOccurred()) + }) + }) + + It("should handle state transitions from redirect-only to backend rules", func() { + // Start with redirect-only rule + initialRules := []gwv1.HTTPRouteRule{ + { + Matches: []gwv1.HTTPRouteMatch{ + { + Path: &gwv1.HTTPPathMatch{ + Type: &[]gwv1.PathMatchType{gwv1.PathMatchExact}[0], + Value: awssdk.String("/transition"), + }, + }, + }, + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + }, + }, + }, + // No BackendRefs initially + }, + } + + httpr := buildHTTPRoute([]string{}, initialRules, nil) + + By("deploying initial redirect-only HTTPRoute", func() { + stack = newALBResourceStack( + nil, + nil, + buildGatewayClass(), + buildGateway(gwListeners), + lbcSpec, + nil, + nil, + []*gwv1.HTTPRoute{httpr}, + nil, + nil, + "transition-test", + false, + ) + + err := stack.DeployHTTP(ctx, nil, tf, gwListeners, []*gwv1.HTTPRoute{httpr}, lbcSpec, nil, nil, nil, false) + Expect(err).NotTo(HaveOccurred()) + }) + + By("verifying initial state - no target groups", func() { + Eventually(func() error { + return verifier.NewALBVerifier(stack.LoadBalancer, tf.LBC.ELBv2Client, tf.LBC.EC2Client, tf.Logger). + VerifyTargetGroupCount(ctx, 0) + }, 1*time.Minute, 5*time.Second).Should(Succeed()) + }) + + By("transitioning to backend rule", func() { + // Create deployment and service + deployment := buildDeployment(defaultName, defaultNumReplicas, appContainerPort) + service := buildService(defaultName, appContainerPort, corev1.ServiceTypeClusterIP) + + // Update HTTPRoute to have backend instead of redirect + updatedRules := []gwv1.HTTPRouteRule{ + { + Matches: []gwv1.HTTPRouteMatch{ + { + Path: &gwv1.HTTPPathMatch{ + Type: &[]gwv1.PathMatchType{gwv1.PathMatchExact}[0], + Value: awssdk.String("/transition"), + }, + }, + }, + BackendRefs: DefaultHttpRouteRuleBackendRefs, + // Remove redirect filters + }, + } + + // Deploy updated resources + updatedHttpr := buildHTTPRoute([]string{}, updatedRules, nil) + + // Update the stack with backend resources + stack.Deployments = []*appsv1.Deployment{deployment} + stack.Services = []*corev1.Service{service} + stack.TargetGroupConfigurations = []*elbv2gw.TargetGroupConfiguration{tgSpec} + stack.HTTPRoutes = []*gwv1.HTTPRoute{updatedHttpr} + + err := stack.DeployHTTP(ctx, nil, tf, gwListeners, []*gwv1.HTTPRoute{updatedHttpr}, lbcSpec, tgSpec, nil, nil, true) + Expect(err).NotTo(HaveOccurred()) + }) + + By("verifying transition - target group created", func() { + Eventually(func() error { + return verifier.NewALBVerifier(stack.LoadBalancer, tf.LBC.ELBv2Client, tf.LBC.EC2Client, tf.Logger). + VerifyTargetGroupCount(ctx, 1) // Should now have 1 target group + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + }) + + By("verifying target group health after transition", func() { + Eventually(func() error { + return verifier.NewALBVerifier(stack.LoadBalancer, tf.LBC.ELBv2Client, tf.LBC.EC2Client, tf.Logger). + VerifyTargetGroupHealthy(ctx) + }, 2*time.Minute, 10*time.Second).Should(Succeed()) + }) + + By("cleaning up resources", func() { + err := stack.Cleanup(ctx, tf) + Expect(err).NotTo(HaveOccurred()) + }) + }) + }) + + Context("Error Scenarios", func() { + It("should handle invalid redirect configurations gracefully", func() { + // Create HTTPRoute with invalid redirect configuration + httpRouteRules := []gwv1.HTTPRouteRule{ + { + Matches: []gwv1.HTTPRouteMatch{ + { + Path: &gwv1.HTTPPathMatch{ + Type: &[]gwv1.PathMatchType{gwv1.PathMatchExact}[0], + Value: awssdk.String("/invalid-redirect"), + }, + }, + }, + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gwv1.HTTPRequestRedirectFilter{ + StatusCode: awssdk.Int(200), // Invalid status code for redirect + }, + }, + }, + }, + } + + httpr := buildHTTPRoute([]string{}, httpRouteRules, nil) + + By("deploying HTTPRoute with invalid redirect", func() { + stack = newALBResourceStack( + nil, + nil, + buildGatewayClass(), + buildGateway(gwListeners), + lbcSpec, + nil, + nil, + []*gwv1.HTTPRoute{httpr}, + nil, + nil, + "invalid-redirect-test", + false, + ) + + err := stack.DeployHTTP(ctx, nil, tf, gwListeners, []*gwv1.HTTPRoute{httpr}, lbcSpec, nil, nil, nil, false) + // Should handle the error gracefully + if err != nil { + // Verify the error is related to validation + Expect(err.Error()).To(ContainSubstring("status code")) + } + }) + + By("verifying HTTPRoute status reflects the error", func() { + // Check that the HTTPRoute status is updated with error information + Eventually(func() error { + var route gwv1.HTTPRoute + err := k8sClient.Get(ctx, types.NamespacedName{ + Namespace: httpr.Namespace, + Name: httpr.Name, + }, &route) + if err != nil { + return err + } + + // Check if status indicates an error + for _, parent := range route.Status.Parents { + for _, condition := range parent.Conditions { + if condition.Type == string(gwv1.RouteConditionAccepted) && condition.Status == metav1.ConditionFalse { + return nil // Found expected error condition + } + } + } + return fmt.Errorf("expected error condition not found in HTTPRoute status") + }, 1*time.Minute, 5*time.Second).Should(Succeed()) + }) + + By("cleaning up resources", func() { + if stack != nil { + err := stack.Cleanup(ctx, tf) + Expect(err).NotTo(HaveOccurred()) + } + }) + }) + }) +}) \ No newline at end of file diff --git a/test/e2e/gateway/redirect_utils.go b/test/e2e/gateway/redirect_utils.go new file mode 100644 index 000000000..80fe8fb1a --- /dev/null +++ b/test/e2e/gateway/redirect_utils.go @@ -0,0 +1,310 @@ +package gateway + +import ( + "context" + "fmt" + + awssdk "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/elbv2" + elbv2types "github.com/aws/aws-sdk-go-v2/service/elbv2/types" + . "github.com/onsi/gomega" + gwv1 "sigs.k8s.io/gateway-api/apis/v1" +) + +// RedirectTestUtils provides utilities for testing redirect-only HTTPRoute functionality +type RedirectTestUtils struct { + elbv2Client *elbv2.Client +} + +// NewRedirectTestUtils creates a new RedirectTestUtils instance +func NewRedirectTestUtils(elbv2Client *elbv2.Client) *RedirectTestUtils { + return &RedirectTestUtils{ + elbv2Client: elbv2Client, + } +} + +// VerifyRedirectRule verifies that a redirect rule exists in the ALB listener rules +func (r *RedirectTestUtils) VerifyRedirectRule(ctx context.Context, listenerArn string, expectedPath string, expectedRedirect RedirectExpectation) error { + // Get listener rules + resp, err := r.elbv2Client.DescribeRules(ctx, &elbv2.DescribeRulesInput{ + ListenerArn: &listenerArn, + }) + if err != nil { + return fmt.Errorf("failed to describe rules: %w", err) + } + + // Find the rule with matching path condition + for _, rule := range resp.Rules { + if r.ruleMatchesPath(rule, expectedPath) { + return r.verifyRuleRedirectAction(rule, expectedRedirect) + } + } + + return fmt.Errorf("no rule found with path condition: %s", expectedPath) +} + +// VerifyNoTargetGroupsForRedirectRules verifies that redirect-only rules don't have target groups +func (r *RedirectTestUtils) VerifyNoTargetGroupsForRedirectRules(ctx context.Context, listenerArn string, redirectPaths []string) error { + // Get listener rules + resp, err := r.elbv2Client.DescribeRules(ctx, &elbv2.DescribeRulesInput{ + ListenerArn: &listenerArn, + }) + if err != nil { + return fmt.Errorf("failed to describe rules: %w", err) + } + + // Check each redirect path + for _, path := range redirectPaths { + for _, rule := range resp.Rules { + if r.ruleMatchesPath(rule, path) { + // Verify this rule has redirect action, not forward action + if err := r.verifyRuleHasRedirectAction(rule); err != nil { + return fmt.Errorf("redirect rule for path %s: %w", path, err) + } + } + } + } + + return nil +} + +// RedirectExpectation defines expected redirect behavior +type RedirectExpectation struct { + Scheme *string + Hostname *string + Port *string + StatusCode *string +} + +// ruleMatchesPath checks if a rule has a path condition matching the expected path +func (r *RedirectTestUtils) ruleMatchesPath(rule elbv2types.Rule, expectedPath string) bool { + for _, condition := range rule.Conditions { + if condition.Field != nil && *condition.Field == "path-pattern" { + for _, value := range condition.Values { + if value == expectedPath { + return true + } + } + } + } + return false +} + +// verifyRuleRedirectAction verifies that a rule has the expected redirect action +func (r *RedirectTestUtils) verifyRuleRedirectAction(rule elbv2types.Rule, expected RedirectExpectation) error { + for _, action := range rule.Actions { + if action.Type == elbv2types.ActionTypeEnumRedirect { + redirectConfig := action.RedirectConfig + if redirectConfig == nil { + return fmt.Errorf("redirect action missing configuration") + } + + // Verify scheme + if expected.Scheme != nil { + if redirectConfig.Protocol == nil || *redirectConfig.Protocol != *expected.Scheme { + return fmt.Errorf("expected scheme %s, got %v", *expected.Scheme, redirectConfig.Protocol) + } + } + + // Verify hostname + if expected.Hostname != nil { + if redirectConfig.Host == nil || *redirectConfig.Host != *expected.Hostname { + return fmt.Errorf("expected hostname %s, got %v", *expected.Hostname, redirectConfig.Host) + } + } + + // Verify port + if expected.Port != nil { + if redirectConfig.Port == nil || *redirectConfig.Port != *expected.Port { + return fmt.Errorf("expected port %s, got %v", *expected.Port, redirectConfig.Port) + } + } + + // Verify status code + if expected.StatusCode != nil { + if redirectConfig.StatusCode == nil || *redirectConfig.StatusCode != *expected.StatusCode { + return fmt.Errorf("expected status code %s, got %v", *expected.StatusCode, redirectConfig.StatusCode) + } + } + + return nil // Found matching redirect action + } + } + + return fmt.Errorf("no redirect action found in rule") +} + +// verifyRuleHasRedirectAction verifies that a rule has a redirect action (not forward) +func (r *RedirectTestUtils) verifyRuleHasRedirectAction(rule elbv2types.Rule) error { + hasRedirect := false + hasForward := false + + for _, action := range rule.Actions { + switch action.Type { + case elbv2types.ActionTypeEnumRedirect: + hasRedirect = true + case elbv2types.ActionTypeEnumForward: + hasForward = true + } + } + + if !hasRedirect { + return fmt.Errorf("rule does not have redirect action") + } + + if hasForward { + return fmt.Errorf("redirect-only rule should not have forward action") + } + + return nil +} + +// BuildRedirectOnlyHTTPRoute creates an HTTPRoute with only redirect rules (no backends) +func BuildRedirectOnlyHTTPRoute(name string, redirectRules []RedirectRule) *gwv1.HTTPRoute { + var rules []gwv1.HTTPRouteRule + + for _, redirectRule := range redirectRules { + rule := gwv1.HTTPRouteRule{ + Matches: []gwv1.HTTPRouteMatch{ + { + Path: &gwv1.HTTPPathMatch{ + Type: &redirectRule.PathType, + Value: &redirectRule.Path, + }, + }, + }, + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &redirectRule.Redirect, + }, + }, + // No BackendRefs - this makes it redirect-only + } + rules = append(rules, rule) + } + + return &gwv1.HTTPRoute{ + ObjectMeta: buildHTTPRouteObjectMeta(name), + Spec: gwv1.HTTPRouteSpec{ + CommonRouteSpec: gwv1.CommonRouteSpec{ + ParentRefs: []gwv1.ParentReference{ + { + Name: defaultGatewayName, + }, + }, + }, + Rules: rules, + }, + } +} + +// RedirectRule defines a redirect rule configuration +type RedirectRule struct { + Path string + PathType gwv1.PathMatchType + Redirect gwv1.HTTPRequestRedirectFilter +} + +// BuildMixedHTTPRoute creates an HTTPRoute with both redirect-only and backend rules +func BuildMixedHTTPRoute(name string, redirectRules []RedirectRule, backendRules []BackendRule) *gwv1.HTTPRoute { + var rules []gwv1.HTTPRouteRule + + // Add redirect-only rules + for _, redirectRule := range redirectRules { + rule := gwv1.HTTPRouteRule{ + Matches: []gwv1.HTTPRouteMatch{ + { + Path: &gwv1.HTTPPathMatch{ + Type: &redirectRule.PathType, + Value: &redirectRule.Path, + }, + }, + }, + Filters: []gwv1.HTTPRouteFilter{ + { + Type: gwv1.HTTPRouteFilterRequestRedirect, + RequestRedirect: &redirectRule.Redirect, + }, + }, + // No BackendRefs + } + rules = append(rules, rule) + } + + // Add backend rules + for _, backendRule := range backendRules { + rule := gwv1.HTTPRouteRule{ + Matches: []gwv1.HTTPRouteMatch{ + { + Path: &gwv1.HTTPPathMatch{ + Type: &backendRule.PathType, + Value: &backendRule.Path, + }, + }, + }, + BackendRefs: backendRule.BackendRefs, + // No redirect filters + } + rules = append(rules, rule) + } + + return &gwv1.HTTPRoute{ + ObjectMeta: buildHTTPRouteObjectMeta(name), + Spec: gwv1.HTTPRouteSpec{ + CommonRouteSpec: gwv1.CommonRouteSpec{ + ParentRefs: []gwv1.ParentReference{ + { + Name: defaultGatewayName, + }, + }, + }, + Rules: rules, + }, + } +} + +// BackendRule defines a backend rule configuration +type BackendRule struct { + Path string + PathType gwv1.PathMatchType + BackendRefs []gwv1.HTTPBackendRef +} + +// Common redirect configurations for testing +var ( + HTTPSRedirect = gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + StatusCode: awssdk.Int(301), + } + + HostnameRedirect = gwv1.HTTPRequestRedirectFilter{ + Hostname: (*gwv1.PreciseHostname)(awssdk.String("new.example.com")), + StatusCode: awssdk.Int(302), + } + + PortRedirect = gwv1.HTTPRequestRedirectFilter{ + Port: (*gwv1.PortNumber)(awssdk.Int32(8080)), + StatusCode: awssdk.Int(302), + } + + ComplexRedirect = gwv1.HTTPRequestRedirectFilter{ + Scheme: awssdk.String("https"), + Hostname: (*gwv1.PreciseHostname)(awssdk.String("secure.example.com")), + Port: (*gwv1.PortNumber)(awssdk.Int32(443)), + StatusCode: awssdk.Int(301), + } +) + +// Verification helpers +func ExpectRedirectRule(ctx context.Context, utils *RedirectTestUtils, listenerArn, path string, expected RedirectExpectation) { + Eventually(func() error { + return utils.VerifyRedirectRule(ctx, listenerArn, path, expected) + }).Should(Succeed(), fmt.Sprintf("Expected redirect rule for path %s", path)) +} + +func ExpectNoTargetGroupsForRedirects(ctx context.Context, utils *RedirectTestUtils, listenerArn string, redirectPaths []string) { + Consistently(func() error { + return utils.VerifyNoTargetGroupsForRedirectRules(ctx, listenerArn, redirectPaths) + }).Should(Succeed(), "Redirect-only rules should not have target groups") +} \ No newline at end of file