Repository: kubernetes-sigs/aws-alb-ingress-controller
Pull request #1128 (2020-01-14 03:57:52)

adding cache around WAF API usage
Adding caches for WAF API usage.
The WAF API have a 1 TPS limitation, adding client-side cache to mitigate it.
The default cache timeout is 10 minutes, as long as user didn't manually adjust webACL on ALB from console/CLI, there won't be any cache issue.

k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/aws-alb-ingress-controller/pull/1128#" title="Author self-approved">M00nF1sh</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Faws-alb-ingress-controller).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/aws-alb-ingress-controller/blob/master/OWNERS)~~ [M00nF1sh]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
Jeffwan: /lgtm
k8s-ci-robot: @M00nF1sh: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-aws-alb-ingress-controller-lint | 0d1c36847a819dfd308d3be9ba00802a2e7dbe26 | [link](https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_aws-alb-ingress-controller/1128/pull-aws-alb-ingress-controller-lint/1216932566557265921) | `/test pull-aws-alb-ingress-controller-lint`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=aws-alb-ingress-controller&pr=1128). [Your PR dashboard](https://gubernator.k8s.io/pr/M00nF1sh). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/aws-alb-ingress-controller/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Patch:
diff --git a/internal/alb/lb/loadbalancer.go b/internal/alb/lb/loadbalancer.go
index 717cd1c34..148d8632b 100644
--- a/internal/alb/lb/loadbalancer.go
+++ b/internal/alb/lb/loadbalancer.go
@@ -45,6 +45,7 @@ func NewController(
 	sgAssociationController sg.AssociationController,
 	tagsController tags.Controller) Controller {
 	attrsController := NewAttributesController(cloud)
+	wafController := NewWAFController(cloud)
 
 	return &defaultController{
 		cloud:                   cloud,
@@ -55,6 +56,7 @@ func NewController(
 		sgAssociationController: sgAssociationController,
 		tagsController:          tagsController,
 		attrsController:         attrsController,
+		wafController:           wafController,
 	}
 }
 
@@ -78,6 +80,7 @@ type defaultController struct {
 	sgAssociationController sg.AssociationController
 	tagsController          tags.Controller
 	attrsController         AttributesController
+	wafController           WAFController
 }
 
 var _ Controller = (*defaultController)(nil)
@@ -111,7 +114,7 @@ func (controller *defaultController) Reconcile(ctx context.Context, ingress *ext
 	}
 
 	if controller.store.GetConfig().FeatureGate.Enabled(config.WAF) {
-		if err := controller.reconcileWAF(ctx, lbArn, ingressAnnos.LoadBalancer.WebACLId); err != nil {
+		if err := controller.wafController.Reconcile(ctx, lbArn, ingress); err != nil {
 			return nil, err
 		}
 	}
@@ -252,48 +255,6 @@ func (controller *defaultController) reconcileLBInstance(ctx context.Context, in
 	return nil
 }
 
-func (controller *defaultController) reconcileWAF(ctx context.Context, lbArn string, webACLID *string) error {
-	webACLSummary, err := controller.cloud.GetWebACLSummary(ctx, aws.String(lbArn))
-	if err != nil {
-		return fmt.Errorf("error getting web acl for load balancer %v: %v", lbArn, err)
-	}
-
-	if webACLID != nil {
-		b, err := controller.cloud.WebACLExists(ctx, webACLID)
-		if err != nil {
-			return fmt.Errorf("error fetching web acl %v: %v", aws.StringValue(webACLID), err)
-		}
-		if !b {
-			return fmt.Errorf("web acl %v does not exist", aws.StringValue(webACLID))
-		}
-	}
-
-	switch {
-	case webACLSummary != nil && webACLID == nil:
-		{
-			albctx.GetLogger(ctx).Infof("disassociate WAF on %v", lbArn)
-			if _, err := controller.cloud.DisassociateWAF(ctx, aws.String(lbArn)); err != nil {
-				return fmt.Errorf("failed to disassociate webACL on loadBalancer %v due to %v", lbArn, err)
-			}
-		}
-	case webACLSummary != nil && webACLID != nil && aws.StringValue(webACLSummary.WebACLId) != aws.StringValue(webACLID):
-		{
-			albctx.GetLogger(ctx).Infof("associate WAF on %v to %v", lbArn, aws.StringValue(webACLID))
-			if _, err := controller.cloud.AssociateWAF(ctx, aws.String(lbArn), webACLID); err != nil {
-				return fmt.Errorf("failed to associate webACL on loadBalancer %v due to %v", lbArn, err)
-			}
-		}
-	case webACLSummary == nil && webACLID != nil:
-		{
-			albctx.GetLogger(ctx).Infof("associate WAF on %v to %v", lbArn, aws.StringValue(webACLID))
-			if _, err := controller.cloud.AssociateWAF(ctx, aws.String(lbArn), webACLID); err != nil {
-				return fmt.Errorf("failed to associate webACL on loadBalancer %v due to %v", lbArn, err)
-			}
-		}
-	}
-	return nil
-}
-
 func (controller *defaultController) isLBInstanceNeedRecreation(ctx context.Context, instance *elbv2.LoadBalancer, lbConfig *loadBalancerConfig) bool {
 	if !util.DeepEqual(instance.Scheme, lbConfig.Scheme) {
 		albctx.GetLogger(ctx).Infof("LoadBalancer %s need recreation due to scheme changed(%s => %s)",
diff --git a/internal/alb/lb/waf.go b/internal/alb/lb/waf.go
new file mode 100644
index 000000000..14523cca9
--- /dev/null
+++ b/internal/alb/lb/waf.go
@@ -0,0 +1,96 @@
+package lb
+
+import (
+	"context"
+	"time"
+
+	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/albctx"
+	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/aws"
+	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/ingress/annotations"
+	"github.com/pkg/errors"
+	extensions "k8s.io/api/extensions/v1beta1"
+	"k8s.io/apimachinery/pkg/util/cache"
+)
+
+const (
+	webACLIdForLBCacheMaxSize = 1024
+	webACLIdForLBCacheTTL     = 10 * time.Minute
+)
+
+// WAFController provides functionality to manage ALB's WAF associations.
+type WAFController interface {
+	Reconcile(ctx context.Context, lbArn string, ingress *extensions.Ingress) error
+}
+
+func NewWAFController(cloud aws.CloudAPI) WAFController {
+	return &defaultWAFController{
+		cloud:              cloud,
+		webACLIdForLBCache: cache.NewLRUExpireCache(webACLIdForLBCacheMaxSize),
+	}
+}
+
+type defaultWAFController struct {
+	cloud aws.CloudAPI
+
+	// cache that stores webACLIdForLBCache for LoadBalancerARN.
+	// The cache value is string, while "" represents no webACL.
+	webACLIdForLBCache *cache.LRUExpireCache
+}
+
+func (c *defaultWAFController) Reconcile(ctx context.Context, lbArn string, ing *extensions.Ingress) error {
+	currentWebACLId, err := c.getCurrentWebACLId(ctx, lbArn)
+	if err != nil {
+		return err
+	}
+	desiredWebACLId := c.getDesiredWebACLId(ctx, ing)
+
+	switch {
+	case desiredWebACLId == "" && currentWebACLId != "":
+		albctx.GetLogger(ctx).Infof("disassociate WAF on %v", lbArn)
+		if _, err := c.cloud.DisassociateWAF(ctx, aws.String(lbArn)); err != nil {
+			return errors.Wrapf(err, "failed to disassociate webACL on LoadBalancer %v", lbArn)
+		}
+		c.webACLIdForLBCache.Add(lbArn, desiredWebACLId, webACLIdForLBCacheTTL)
+	case desiredWebACLId != "" && currentWebACLId != "" && desiredWebACLId != currentWebACLId:
+		albctx.GetLogger(ctx).Infof("associate WAF on %v from %v to %v", lbArn, currentWebACLId, desiredWebACLId)
+		if _, err := c.cloud.AssociateWAF(ctx, aws.String(lbArn), aws.String(desiredWebACLId)); err != nil {
+			return errors.Wrapf(err, "failed to associate webACL on LoadBalancer %v", lbArn)
+		}
+		c.webACLIdForLBCache.Add(lbArn, desiredWebACLId, webACLIdForLBCacheTTL)
+	case desiredWebACLId != "" && currentWebACLId == "":
+		albctx.GetLogger(ctx).Infof("associate WAF on %v to %v", lbArn, desiredWebACLId)
+		if _, err := c.cloud.AssociateWAF(ctx, aws.String(lbArn), aws.String(desiredWebACLId)); err != nil {
+			return errors.Wrapf(err, "failed to associate webACL on LoadBalancer %v", lbArn)
+		}
+		c.webACLIdForLBCache.Add(lbArn, desiredWebACLId, webACLIdForLBCacheTTL)
+	}
+	return nil
+}
+
+func (c *defaultWAFController) getDesiredWebACLId(ctx context.Context, ing *extensions.Ingress) string {
+	var webACLId string
+	// support legacy waf-acl-id annotation
+	_ = annotations.LoadStringAnnotation("waf-acl-id", &webACLId, ing.Annotations)
+	_ = annotations.LoadStringAnnotation("web-acl-id", &webACLId, ing.Annotations)
+	return webACLId
+}
+
+func (c *defaultWAFController) getCurrentWebACLId(ctx context.Context, lbArn string) (string, error) {
+	cachedWebACLId, exists := c.webACLIdForLBCache.Get(lbArn)
+	if exists {
+		return cachedWebACLId.(string), nil
+	}
+
+	webACLSummary, err := c.cloud.GetWebACLSummary(ctx, aws.String(lbArn))
+	if err != nil {
+		return "", errors.Wrapf(err, "failed to get web acl for load balancer %v", lbArn)
+	}
+
+	var webACLId string
+	if webACLSummary != nil {
+		webACLId = aws.StringValue(webACLSummary.WebACLId)
+	}
+
+	c.webACLIdForLBCache.Add(lbArn, webACLId, webACLIdForLBCacheTTL)
+	return webACLId, nil
+}
diff --git a/internal/ingress/annotations/loadbalancer/main.go b/internal/ingress/annotations/loadbalancer/main.go
index 3cd028450..084e9a067 100644
--- a/internal/ingress/annotations/loadbalancer/main.go
+++ b/internal/ingress/annotations/loadbalancer/main.go
@@ -39,7 +39,6 @@ type PortData struct {
 type Config struct {
 	Scheme        *string
 	IPAddressType *string
-	WebACLId      *string
 
 	InboundCidrs   []string
 	InboundV6CIDRs []string
@@ -65,13 +64,6 @@ func NewParser(r resolver.Resolver) parser.IngressAnnotation {
 
 // Parse parses the annotations contained in the resource
 func (lb loadBalancer) Parse(ing parser.AnnotationInterface) (interface{}, error) {
-	// support legacy waf-acl-id annotation
-	webACLId, _ := parser.GetStringAnnotation("waf-acl-id", ing)
-	w, err := parser.GetStringAnnotation("web-acl-id", ing)
-	if err == nil {
-		webACLId = w
-	}
-
 	ipAddressType, err := parser.GetStringAnnotation("ip-address-type", ing)
 	if err != nil {
 		ipAddressType = aws.String(DefaultIPAddressType)
@@ -109,7 +101,6 @@ func (lb loadBalancer) Parse(ing parser.AnnotationInterface) (interface{}, error
 	}
 
 	return &Config{
-		WebACLId:      webACLId,
 		Scheme:        scheme,
 		IPAddressType: ipAddressType,
 


Test patch:
diff --git a/internal/alb/lb/waf_test.go b/internal/alb/lb/waf_test.go
new file mode 100644
index 000000000..0dcab91b4
--- /dev/null
+++ b/internal/alb/lb/waf_test.go
@@ -0,0 +1,66 @@
+package lb
+
+import (
+	"context"
+	"testing"
+
+	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/ingress/annotations/parser"
+	"github.com/kubernetes-sigs/aws-alb-ingress-controller/mocks"
+	extensions "k8s.io/api/extensions/v1beta1"
+	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/cache"
+)
+
+func Test_defaultWAFController_getDesiredWebACLId(t *testing.T) {
+	tests := []struct {
+		name string
+		ing  *extensions.Ingress
+		want string
+	}{
+		{
+			name: "ingress without waf settings",
+			ing: &extensions.Ingress{
+				ObjectMeta: v1.ObjectMeta{
+					Name:        "ingress",
+					Annotations: map[string]string{},
+				},
+			},
+			want: "",
+		},
+		{
+			name: "ingress with web-acl-id(waf classic)",
+			ing: &extensions.Ingress{
+				ObjectMeta: v1.ObjectMeta{
+					Name: "ingress",
+					Annotations: map[string]string{
+						parser.AnnotationsPrefix + "/web-acl-id": "my-web-acl-id",
+					},
+				},
+			},
+			want: "my-web-acl-id",
+		},
+		{
+			name: "ingress with waf-acl-id(waf classic)",
+			ing: &extensions.Ingress{
+				ObjectMeta: v1.ObjectMeta{
+					Name: "ingress",
+					Annotations: map[string]string{
+						parser.AnnotationsPrefix + "/waf-acl-id": "my-web-acl-id",
+					},
+				},
+			},
+			want: "my-web-acl-id",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			c := &defaultWAFController{
+				cloud:              &mocks.CloudAPI{},
+				webACLIdForLBCache: cache.NewLRUExpireCache(10),
+			}
+			if got := c.getDesiredWebACLId(context.Background(), tt.ing); got != tt.want {
+				t.Errorf("getDesiredWebACLId() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}