From 8ba34e27224d1f7a56f0e416086c43b6877c7e65 Mon Sep 17 00:00:00 2001 From: wweiwei-li <79778352+wweiwei-li@users.noreply.github.com> Date: Thu, 21 Nov 2024 13:05:29 -0800 Subject: [PATCH] enable http and https listener attributes (#3948) --- docs/guide/ingress/annotations.md | 9 ++ pkg/deploy/elbv2/listener_manager.go | 4 +- pkg/ingress/model_build_listener.go | 29 +++++-- pkg/ingress/model_build_listener_test.go | 84 +++++++++++++++++++ .../model_build_load_balancer_attributes.go | 28 +++++-- ...del_build_load_balancer_attributes_test.go | 5 +- test/e2e/ingress/vanilla_ingress_test.go | 62 ++++++++++++++ 7 files changed, 201 insertions(+), 20 deletions(-) diff --git a/docs/guide/ingress/annotations.md b/docs/guide/ingress/annotations.md index a47e534050..2dc2947ede 100644 --- a/docs/guide/ingress/annotations.md +++ b/docs/guide/ingress/annotations.md @@ -60,6 +60,7 @@ You can add annotations to kubernetes Ingress and Service objects to customize t | [alb.ingress.kubernetes.io/target-node-labels](#target-node-labels) | stringMap |N/A| Ingress,Service | N/A | | [alb.ingress.kubernetes.io/mutual-authentication](#mutual-authentication) | json |N/A| Ingress | Exclusive | | [alb.ingress.kubernetes.io/multi-cluster-target-group](#multi-cluster-target-group) | boolean |N/A| Ingress, Service | N/A | +| [alb.ingress.kubernetes.io/listener-attributes.${Protocol}-${Port}](#listener-attributes) | stringMap |N/A| Ingress |Merge| ## IngressGroup IngressGroup feature enables you to group multiple Ingress resources together. @@ -903,6 +904,14 @@ Custom attributes to LoadBalancers and TargetGroups can be controlled with follo alb.ingress.kubernetes.io/multi-cluster-target-group: "true" ``` +- `alb.ingress.kubernetes.io/listener-attributes.${Protocol}-${Port}` specifies Listener Attributes which should be applied to listener. + + !!!example + - Server header enablement attribute + ``` + alb.ingress.kubernetes.io/listener-attributes.HTTP-80: routing.http.response.server.enabled=true + ``` + ## Resource Tags The AWS Load Balancer Controller automatically applies following tags to the AWS resources (ALB/TargetGroups/SecurityGroups/Listener/ListenerRule) it creates: diff --git a/pkg/deploy/elbv2/listener_manager.go b/pkg/deploy/elbv2/listener_manager.go index 1aa4a6b70f..a9db440779 100644 --- a/pkg/deploy/elbv2/listener_manager.go +++ b/pkg/deploy/elbv2/listener_manager.go @@ -23,8 +23,8 @@ import ( ) var PROTOCOLS_SUPPORTING_LISTENER_ATTRIBUTES = map[elbv2model.Protocol]bool{ - elbv2model.ProtocolHTTP: false, - elbv2model.ProtocolHTTPS: false, + elbv2model.ProtocolHTTP: true, + elbv2model.ProtocolHTTPS: true, elbv2model.ProtocolTCP: true, elbv2model.ProtocolUDP: false, elbv2model.ProtocolTLS: false, diff --git a/pkg/ingress/model_build_listener.go b/pkg/ingress/model_build_listener.go index 31757773e5..80f848ba3d 100644 --- a/pkg/ingress/model_build_listener.go +++ b/pkg/ingress/model_build_listener.go @@ -426,6 +426,14 @@ func (t *defaultModelBuildTask) fetchTrustStoreArnFromName(ctx context.Context, func (t *defaultModelBuildTask) buildIngressGroupListenerAttributes(ctx context.Context, ingList []ClassifiedIngress, listenerProtocol elbv2model.Protocol, port int32) ([]elbv2model.ListenerAttribute, error) { rawIngGrouplistenerAttributes := make(map[string]string) + ingClassAttributes := make(map[string]string) + if len(ingList) > 0 { + var err error + ingClassAttributes, err = t.buildIngressClassListenerAttributes(ingList[0].IngClassConfig, listenerProtocol, port) + if err != nil { + return nil, err + } + } for _, ing := range ingList { ingAttributes, err := t.buildIngressListenerAttributes(ctx, ing.Ing.Annotations, port, listenerProtocol) if err != nil { @@ -435,18 +443,23 @@ func (t *defaultModelBuildTask) buildIngressGroupListenerAttributes(ctx context. attributeKey := attribute.Key attributeValue := attribute.Value if existingAttributeValue, exists := rawIngGrouplistenerAttributes[attributeKey]; exists && existingAttributeValue != attributeValue { - return nil, errors.Errorf("conflicting attributes %v: %v | %v", attributeKey, existingAttributeValue, attributeValue) + if ingClassValue, exists := ingClassAttributes[attributeKey]; exists { + // Conflict is resolved by ingClassAttributes, show a warning + t.logger.Info("listener attribute conflict resolved by ingress class", + "attributeKey", attributeKey, + "existingValue", existingAttributeValue, + "newValue", attributeValue, + "ingClassValue", ingClassValue) + } else { + // Conflict is not resolved by ingClassAttributes, return an error + return nil, errors.Errorf("conflicting listener attributes %v: %v | %v for ingress %s/%s", + attributeKey, existingAttributeValue, attributeValue, ing.Ing.Namespace, ing.Ing.Name) + } } rawIngGrouplistenerAttributes[attributeKey] = attributeValue } } - if len(ingList) > 0 { - ingClassAttributes, err := t.buildIngressClassListenerAttributes(ingList[0].IngClassConfig, listenerProtocol, port) - if err != nil { - return nil, err - } - rawIngGrouplistenerAttributes = algorithm.MergeStringMap(ingClassAttributes, rawIngGrouplistenerAttributes) - } + rawIngGrouplistenerAttributes = algorithm.MergeStringMap(ingClassAttributes, rawIngGrouplistenerAttributes) attributes := make([]elbv2model.ListenerAttribute, 0, len(rawIngGrouplistenerAttributes)) for attrKey, attrValue := range rawIngGrouplistenerAttributes { attributes = append(attributes, elbv2model.ListenerAttribute{ diff --git a/pkg/ingress/model_build_listener_test.go b/pkg/ingress/model_build_listener_test.go index ca856b6d9a..9514b8f6e1 100644 --- a/pkg/ingress/model_build_listener_test.go +++ b/pkg/ingress/model_build_listener_test.go @@ -7,6 +7,7 @@ import ( "github.com/stretchr/testify/assert" networking "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + elbv2api "sigs.k8s.io/aws-load-balancer-controller/apis/elbv2/v1beta1" "sigs.k8s.io/aws-load-balancer-controller/pkg/annotations" "sigs.k8s.io/aws-load-balancer-controller/pkg/model/elbv2" elbv2model "sigs.k8s.io/aws-load-balancer-controller/pkg/model/elbv2" @@ -272,6 +273,89 @@ func Test_buildListenerAttributes(t *testing.T) { }, }, }, + { + name: "Ignore conflicting value when the key is specified by ingress class param", + fields: fields{ + ingGroup: Group{ + ID: GroupID{Name: "explicit-group"}, + Members: []ClassifiedIngress{ + { + Ing: &networking.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "awesome-ns", + Name: "ing-6", + Annotations: map[string]string{ + "alb.ingress.kubernetes.io/listen-ports": `[{"HTTP": 80}]`, + "alb.ingress.kubernetes.io/listener-attributes.HTTP-80": "attrKey1=attrValue1", + }, + }, + }, + IngClassConfig: ClassConfiguration{ + IngClassParams: &elbv2api.IngressClassParams{ + ObjectMeta: metav1.ObjectMeta{ + Name: "awesome-class", + }, + Spec: elbv2api.IngressClassParamsSpec{ + Listeners: []elbv2api.Listener{ + { + Protocol: "HTTP", + Port: 80, + ListenerAttributes: []elbv2api.Attribute{ + { + Key: "attrKey1", + Value: "attrValue1", + }, + }, + }, + }, + }, + }, + }, + }, + { + Ing: &networking.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "awesome-ns", + Name: "ing-7", + Annotations: map[string]string{ + "alb.ingress.kubernetes.io/listen-ports": `[{"HTTP": 80}]`, + "alb.ingress.kubernetes.io/listener-attributes.HTTP-80": "attrKey1=attrValue2", + }, + }, + }, + IngClassConfig: ClassConfiguration{ + IngClassParams: &elbv2api.IngressClassParams{ + ObjectMeta: metav1.ObjectMeta{ + Name: "awesome-class", + }, + Spec: elbv2api.IngressClassParamsSpec{ + Listeners: []elbv2api.Listener{ + { + Protocol: "HTTP", + Port: 80, + ListenerAttributes: []elbv2api.Attribute{ + { + Key: "attrKey1", + Value: "attrValue1", + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + wantErr: false, + wantValue: []elbv2model.ListenerAttribute{ + { + Key: "attrKey1", + Value: "attrValue1", + }, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/ingress/model_build_load_balancer_attributes.go b/pkg/ingress/model_build_load_balancer_attributes.go index e63e286bbe..af67c49239 100644 --- a/pkg/ingress/model_build_load_balancer_attributes.go +++ b/pkg/ingress/model_build_load_balancer_attributes.go @@ -9,6 +9,14 @@ import ( // buildIngressGroupLoadBalancerAttributes builds the LB attributes for a group of Ingresses. func (t *defaultModelBuildTask) buildIngressGroupLoadBalancerAttributes(ingList []ClassifiedIngress) (map[string]string, error) { ingGroupAttributes := make(map[string]string) + ingClassAttributes := make(map[string]string) + if len(ingList) > 0 { + var err error + ingClassAttributes, err = t.buildIngressClassLoadBalancerAttributes(ingList[0].IngClassConfig) + if err != nil { + return nil, err + } + } for _, ing := range ingList { ingAttributes, err := t.buildIngressLoadBalancerAttributes(ing) if err != nil { @@ -18,18 +26,22 @@ func (t *defaultModelBuildTask) buildIngressGroupLoadBalancerAttributes(ingList for attrKey, attrValue := range ingAttributes { existingAttrValue, exists := ingGroupAttributes[attrKey] if exists && existingAttrValue != attrValue { - return nil, errors.Errorf("conflicting attributes %v: %v | %v", attrKey, existingAttrValue, attrValue) + if ingClassValue, exists := ingClassAttributes[attrKey]; exists { + // Conflict is resolved by ingClassAttributes, show a warning + t.logger.Info("load balancer attribute conflict resolved by ingress class", + "attributeKey", attrKey, + "existingValue", existingAttrValue, + "newValue", attrValue, + "ingClassValue", ingClassValue) + } else { + // Conflict is not resolved by ingClassAttributes, return an error + return nil, errors.Errorf("conflicting load balancer attributes %v: %v | %v", attrKey, existingAttrValue, attrValue) + } } ingGroupAttributes[attrKey] = attrValue } } - if len(ingList) > 0 { - ingClassAttributes, err := t.buildIngressClassLoadBalancerAttributes(ingList[0].IngClassConfig) - if err != nil { - return nil, err - } - return algorithm.MergeStringMap(ingClassAttributes, ingGroupAttributes), nil - } + ingGroupAttributes = algorithm.MergeStringMap(ingClassAttributes, ingGroupAttributes) return ingGroupAttributes, nil } diff --git a/pkg/ingress/model_build_load_balancer_attributes_test.go b/pkg/ingress/model_build_load_balancer_attributes_test.go index bb42e50e3f..4c623bd8ee 100644 --- a/pkg/ingress/model_build_load_balancer_attributes_test.go +++ b/pkg/ingress/model_build_load_balancer_attributes_test.go @@ -2,13 +2,14 @@ package ingress import ( "fmt" + "testing" + "github.com/pkg/errors" "github.com/stretchr/testify/assert" networking "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" elbv2api "sigs.k8s.io/aws-load-balancer-controller/apis/elbv2/v1beta1" "sigs.k8s.io/aws-load-balancer-controller/pkg/annotations" - "testing" ) func Test_defaultModelBuildTask_buildIngressGroupLoadBalancerAttributes(t *testing.T) { @@ -82,7 +83,7 @@ func Test_defaultModelBuildTask_buildIngressGroupLoadBalancerAttributes(t *testi }, }, }, - wantErr: errors.New("conflicting attributes deletion_protection.enabled: true | false"), + wantErr: errors.New("conflicting load balancer attributes deletion_protection.enabled: true | false"), }, { name: "non-empty annotation attributes from single Ingress, non-empty IngressClass attributes - has overlap attributes", diff --git a/test/e2e/ingress/vanilla_ingress_test.go b/test/e2e/ingress/vanilla_ingress_test.go index f5366889de..a79a3e673d 100644 --- a/test/e2e/ingress/vanilla_ingress_test.go +++ b/test/e2e/ingress/vanilla_ingress_test.go @@ -11,6 +11,7 @@ import ( "github.com/gavv/httpexpect/v2" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" networking "k8s.io/api/networking/v1" apierrs "k8s.io/apimachinery/pkg/api/errors" @@ -777,6 +778,56 @@ var _ = Describe("vanilla ingress tests", func() { } }) }) + + Context("with `alb.ingress.kubernetes.io/listener-attributes.{Protocol}-{Port}` variant settings", func() { + It("with 'alb.ingress.kubernetes.io/listener-attributes.{Protocol}-{Port}' annotation explicitly specified, one ALB shall be created and functional", func() { + appBuilder := manifest.NewFixedResponseServiceBuilder() + ingBuilder := manifest.NewIngressBuilder() + dp, svc := appBuilder.Build(sandboxNS.Name, "app", tf.Options.TestImageRegistry) + ingBackend := networking.IngressBackend{ + Service: &networking.IngressServiceBackend{ + Name: svc.Name, + Port: networking.ServiceBackendPort{ + Number: 80, + }, + }, + } + annotation := map[string]string{ + "kubernetes.io/ingress.class": "alb", + "alb.ingress.kubernetes.io/scheme": "internet-facing", + "alb.ingress.kubernetes.io/listen-ports": `[{"HTTP": 80}]`, + "alb.ingress.kubernetes.io/listener-attributes.HTTP-80": "routing.http.response.server.enabled=false", + } + if tf.Options.IPFamily == "IPv6" { + annotation["alb.ingress.kubernetes.io/ip-address-type"] = "dualstack" + annotation["alb.ingress.kubernetes.io/target-type"] = "ip" + } + ing := ingBuilder. + AddHTTPRoute("", networking.HTTPIngressPath{Path: "/path", PathType: &exact, Backend: ingBackend}). + WithAnnotations(annotation).Build(sandboxNS.Name, "ing") + resStack := fixture.NewK8SResourceStack(tf, dp, svc, ing) + err := resStack.Setup(ctx) + Expect(err).NotTo(HaveOccurred()) + + defer resStack.TearDown(ctx) + + lbARN, lbDNS := ExpectOneLBProvisionedForIngress(ctx, tf, ing) + sdkListeners, err := tf.LBManager.GetLoadBalancerListeners(ctx, lbARN) + + Eventually(func() bool { + return verifyListenerAttributes(ctx, tf, *sdkListeners[0].ListenerArn, map[string]string{ + "routing.http.response.server.enabled": "false", + }) == nil + }, utils.PollTimeoutShort, utils.PollIntervalMedium).Should(BeTrue()) + + // test traffic + ExpectLBDNSBeAvailable(ctx, tf, lbARN, lbDNS) + httpExp := httpexpect.New(tf.LoggerReporter, fmt.Sprintf("http://%v", lbDNS)) + httpExp.GET("/path").Expect(). + Status(http.StatusOK). + Body().Equal("Hello World!") + }) + }) }) // ExpectOneLBProvisionedForIngress expects one LoadBalancer provisioned for Ingress. @@ -820,3 +871,14 @@ func ExpectLBDNSBeAvailable(ctx context.Context, tf *framework.Framework, lbARN Expect(err).NotTo(HaveOccurred()) tf.Logger.Info("dns becomes available", "dns", lbDNS) } + +func verifyListenerAttributes(ctx context.Context, f *framework.Framework, lsARN string, expectedAttrs map[string]string) error { + lsAttrs, err := f.LBManager.GetListenerAttributes(ctx, lsARN) + Expect(err).NotTo(HaveOccurred()) + for _, attr := range lsAttrs { + if val, ok := expectedAttrs[awssdk.ToString(attr.Key)]; ok && val != awssdk.ToString(attr.Value) { + return errors.Errorf("Attribute %v, expected %v, actual %v", awssdk.ToString(attr.Key), val, awssdk.ToString(attr.Value)) + } + } + return nil +}