Merge pull request #494 from kubernetes-sigs/490-fix

Fix a bug which disassociated Web ACLs right after associating them

Gopkg.toml

@@ -39,10 +39,6 @@
   name = "github.com/eapache/channels"
   branch = "master"
 
-[[constraint]]
-  branch = "master"
-  name = "github.com/armon/go-proxyproto"
-
 [[constraint]]
   branch = "master"
   name = "github.com/golang/glog"
@@ -51,27 +47,6 @@
   name = "github.com/imdario/mergo"
   version = "0.2.4"
 
-[[constraint]]
-  branch = "master"
-  name = "github.com/kylelemons/godebug"
-
-[[constraint]]
-  branch = "master"
-  name = "github.com/mitchellh/go-ps"
-
-[[constraint]]
-  branch = "master"
-  name = "github.com/mitchellh/mapstructure"
-
-[[constraint]]
-  name = "github.com/ncabatoff/process-exporter"
-  source = "github.com/aledbf/process-exporter"
-  branch = "master"
-
-[[constraint]]
-  branch = "master"
-  name = "github.com/paultag/sniff"
-
 [[constraint]]
   name = "github.com/pkg/errors"
   version = "0.8.0"
@@ -84,22 +59,10 @@
   name = "github.com/spf13/pflag"
   version = "1.0.0"
 
-[[constraint]]
-  name = "github.com/zakjan/cert-chain-resolver"
-  version = "1.0.1"
-
-[[constraint]]
-  name = "gopkg.in/fsnotify/fsnotify.v1"
-  version = "1.4.7"
-
 [[constraint]]
   name = "gopkg.in/go-playground/pool.v3"
   version = "3.1.1"
 
-[[constraint]]
-  name = "k8s.io/kubernetes"
-  revision = "v1.11.0"
-
 [[constraint]]
   name = "k8s.io/api"
   revision = "kubernetes-1.11.0"
@@ -112,10 +75,6 @@
   name = "k8s.io/client-go"
   revision = "kubernetes-1.11.0"
 
-[[constraint]]
-  name = "k8s.io/apiextensions-apiserver"
-  revision = "kubernetes-1.11.0"
-
 [[constraint]]
   name = "k8s.io/apiserver"
   revision = "kubernetes-1.11.0"

internal/alb/lb/loadbalancer.go

@@ -13,6 +13,7 @@ import (
 	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/k8s"
 
 	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/aws/albrgt"
+	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/aws/albwafregional"
 
 	extensions "k8s.io/api/extensions/v1beta1"
 
@@ -23,7 +24,6 @@ import (
 	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/alb/tg"
 	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/aws/albec2"
 	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/aws/albelbv2"
-	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/aws/albwaf"
 	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/ingress/controller/store"
 	"github.com/kubernetes-sigs/aws-alb-ingress-controller/pkg/util/log"
 	util "github.com/kubernetes-sigs/aws-alb-ingress-controller/pkg/util/types"
@@ -216,7 +216,7 @@ func NewCurrentLoadBalancer(o *NewCurrentLoadBalancerOptions) (newLoadBalancer *
 	}
 
 	// Check WAF
-	webACLResult, err := albwaf.WAFRegionalsvc.GetWebACLSummary(o.LoadBalancer.LoadBalancerArn)
+	webACLResult, err := albwafregional.WAFRegionalsvc.GetWebACLSummary(o.LoadBalancer.LoadBalancerArn)
 	if err != nil {
 		return newLoadBalancer, fmt.Errorf("Failed to get associated Web ACL. Error: %s", err.Error())
 	}
@@ -461,7 +461,7 @@ func (l *LoadBalancer) create(rOpts *ReconcileOptions) error {
 	}
 
 	if l.options.desired.webACLId != nil {
-		_, err = albwaf.WAFRegionalsvc.Associate(l.lb.current.LoadBalancerArn, l.options.desired.webACLId)
+		_, err = albwafregional.WAFRegionalsvc.Associate(l.lb.current.LoadBalancerArn, l.options.desired.webACLId)
 		if err != nil {
 			rOpts.Eventf(api.EventTypeWarning, "ERROR", "%s Web ACL (%s) association failed: %s", *l.lb.current.LoadBalancerName, l.options.desired.webACLId, err.Error())
 			l.logger.Errorf("Failed setting Web ACL (%s) association: %s", l.options.desired.webACLId, err.Error())
@@ -567,8 +567,8 @@ func (l *LoadBalancer) modify(rOpts *ReconcileOptions) error {
 		}
 
 		if needsMod&webACLAssociationModified != 0 && l.options.desired.webACLId != nil {
-			l.logger.Infof("Associating %v Web ACL.", l.options.desired.webACLId)
-			if _, err := albwaf.WAFRegionalsvc.Associate(l.lb.current.LoadBalancerArn, l.options.desired.webACLId); err != nil {
+			l.logger.Infof("Associating %v Web ACL.", *l.options.desired.webACLId)
+			if _, err := albwafregional.WAFRegionalsvc.Associate(l.lb.current.LoadBalancerArn, l.options.desired.webACLId); err != nil {
 				rOpts.Eventf(api.EventTypeWarning, "ERROR", "%s Web ACL (%s) association failed: %s", *l.lb.current.LoadBalancerName, *l.options.desired.webACLId, err.Error())
 				return fmt.Errorf("Failed associating Web ACL: %s", err.Error())
 			} else {
@@ -577,9 +577,9 @@ func (l *LoadBalancer) modify(rOpts *ReconcileOptions) error {
 			}
 		}
 
-		if needsMod&webACLAssociationModified != 0 && l.options.current.webACLId != nil {
+		if needsMod&webACLAssociationModified != 0 && l.options.desired.webACLId == nil {
 			l.logger.Infof("Disassociating Web ACL.")
-			if _, err := albwaf.WAFRegionalsvc.Disassociate(l.lb.current.LoadBalancerArn); err != nil {
+			if _, err := albwafregional.WAFRegionalsvc.Disassociate(l.lb.current.LoadBalancerArn); err != nil {
 				rOpts.Eventf(api.EventTypeWarning, "ERROR", "%s Web ACL disassociation failed: %s", *l.lb.current.LoadBalancerName, err.Error())
 				return fmt.Errorf("Failed removing Web ACL association: %s", err.Error())
 			} else {
@@ -611,7 +611,7 @@ func (l *LoadBalancer) delete(rOpts *ReconcileOptions) error {
 
 	// we need to disassociate the WAF before deletion
 	if l.options.current.webACLId != nil {
-		if _, err := albwaf.WAFRegionalsvc.Disassociate(l.lb.current.LoadBalancerArn); err != nil {
+		if _, err := albwafregional.WAFRegionalsvc.Disassociate(l.lb.current.LoadBalancerArn); err != nil {
 			rOpts.Eventf(api.EventTypeWarning, "ERROR", "Error disassociating Web ACL for %s: %s", *l.lb.current.LoadBalancerName, err.Error())
 			return fmt.Errorf("Failed disassociation of ELBV2 Web ACL: %s.", err.Error())
 		}

internal/aws/albwaf/waf.go → internal/aws/albwafregional/wafregional.go

@@ -1,4 +1,4 @@
-package albwaf
+package albwafregional
 
 import (
 	"time"

internal/ingress/annotations/loadbalancer/main.go

@@ -23,11 +23,10 @@ import (
 	"sort"
 	"strings"
 
-	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/aws/albwaf"
-
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/aws/albec2"
 	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/aws/albelbv2"
+	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/aws/albwafregional"
 	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/ingress/annotations/parser"
 	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/ingress/errors"
 	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/ingress/resolver"
@@ -67,13 +66,27 @@ 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)
+	webACLId, err := parser.GetStringAnnotation("waf-acl-id", ing)
 	if err == nil {
-		if success, err := albwaf.WAFRegionalsvc.WebACLExists(w); !success {
-			return nil, fmt.Errorf("Web ACL Id does not exist. Id: %s, error: %s", *w, err.Error())
+		b, err := albwafregional.WAFRegionalsvc.WebACLExists(webACLId)
+		if err != nil {
+			return nil, fmt.Errorf("Web ACL Id does not exist. Id: %s, error: %s", *webACLId, err.Error())
 		}
+		if b == false {
+			return nil, fmt.Errorf("Web ACL Id does not exist. Id: %s", *webACLId)
+		}
+	}
+
+	w, err := parser.GetStringAnnotation("web-acl-id", ing)
+	if err == nil {
 		webACLId = w
+		b, err := albwafregional.WAFRegionalsvc.WebACLExists(webACLId)
+		if err != nil {
+			return nil, fmt.Errorf("Web ACL Id does not exist. Id: %s, error: %s", *webACLId, err.Error())
+		}
+		if b == false {
+			return nil, fmt.Errorf("Web ACL Id does not exist. Id: %s", *webACLId)
+		}
 	}
 
 	ipAddressType, err := parser.GetStringAnnotation("ip-address-type", ing)

internal/ingress/controller/alb.go

@@ -41,7 +41,7 @@ import (
 	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/aws/albiam"
 	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/aws/albrgt"
 	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/aws/albsession"
-	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/aws/albwaf"
+	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/aws/albwafregional"
 	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/ingress"
 	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/ingress/annotations/class"
 	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/ingress/controller/config"
@@ -72,7 +72,7 @@ func NewALBController(config *config.Configuration, mc metric.Collector) *ALBCon
 	albacm.NewACM(sess)
 	albiam.NewIAM(sess)
 	albrgt.NewRGT(sess, config.ClusterName)
-	albwaf.NewWAFRegional(sess)
+	albwafregional.NewWAFRegional(sess)
 
 	if config.ALBNamePrefix == "" {
 		config.ALBNamePrefix = generateAlbNamePrefix(config.ClusterName)