Skip to content

Commit bd889b0

Browse files
bigkraigbigkraig
authored
Merge pull request #437 from kubernetes-sigs/waf-refactor
Refactored WAF code and added caches
2 parents 101ee77 + c6da24f commit bd889b0

File tree

9 files changed

+166
-115
lines changed

9 files changed

+166
-115
lines changed

‎pkg/alb/lb/loadbalancer.go

Lines changed: 33 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ func NewDesiredLoadBalancer(o *NewDesiredLoadBalancerOptions) (newLoadBalancer *
6868
options: options{
6969
desired: opts{
7070
idleTimeout: o.Annotations.ConnectionIdleTimeout,
71-
wafACLID: o.Annotations.WafACLID,
71+
webACLId: o.Annotations.WebACLId,
7272
},
7373
},
7474
lb: lb{
@@ -103,7 +103,7 @@ func NewDesiredLoadBalancer(o *NewDesiredLoadBalancerOptions) (newLoadBalancer *
103103
existinglb.lb.desired = newLoadBalancer.lb.desired
104104
existinglb.tags.desired = newLoadBalancer.tags.desired
105105
existinglb.options.desired.idleTimeout = newLoadBalancer.options.desired.idleTimeout
106-
existinglb.options.desired.wafACLID = newLoadBalancer.options.desired.wafACLID
106+
existinglb.options.desired.webACLId = newLoadBalancer.options.desired.webACLId
107107
if len(o.ExistingLoadBalancer.lb.desired.SecurityGroups) == 0 {
108108
existinglb.options.desired.ports = lsps
109109
existinglb.options.desired.inboundCidrs = o.Annotations.InboundCidrs
@@ -245,13 +245,13 @@ func NewCurrentLoadBalancer(o *NewCurrentLoadBalancerOptions) (newLoadBalancer *
245245
}
246246

247247
// Check WAF
248-
wafResult, err := albwaf.WAFRegionalsvc.GetWebACLSummary(o.LoadBalancer.LoadBalancerArn)
248+
webACLResult, err := albwaf.WAFRegionalsvc.GetWebACLSummary(o.LoadBalancer.LoadBalancerArn)
249249
if err != nil {
250-
return newLoadBalancer, fmt.Errorf("Failed to get associated WAF ACL. Error: %s", err.Error())
250+
return newLoadBalancer, fmt.Errorf("Failed to get associated Web ACL. Error: %s", err.Error())
251251
}
252-
var wafACLID *string
253-
if wafResult != nil {
254-
wafACLID = wafResult.WebACLId
252+
var webACLId *string
253+
if webACLResult != nil {
254+
webACLId = webACLResult.WebACLId
255255
}
256256

257257
newLoadBalancer = &LoadBalancer{
@@ -265,7 +265,7 @@ func NewCurrentLoadBalancer(o *NewCurrentLoadBalancerOptions) (newLoadBalancer *
265265
ports: managedSGPorts,
266266
managedInstanceSG: managedInstanceSG,
267267
idleTimeout: idleTimeout,
268-
wafACLID: wafACLID,
268+
webACLId: webACLId,
269269
},
270270
},
271271
}
@@ -519,11 +519,11 @@ func (l *LoadBalancer) create(rOpts *ReconcileOptions) error {
519519
}
520520
}
521521

522-
if l.options.desired.wafACLID != nil {
523-
_, err = albwaf.WAFRegionalsvc.Associate(l.lb.current.LoadBalancerArn, l.options.desired.wafACLID)
522+
if l.options.desired.webACLId != nil {
523+
_, err = albwaf.WAFRegionalsvc.Associate(l.lb.current.LoadBalancerArn, l.options.desired.webACLId)
524524
if err != nil {
525-
rOpts.Eventf(api.EventTypeWarning, "ERROR", "%s WAF (%s) association failed: %s", *l.lb.current.LoadBalancerName, l.options.desired.wafACLID, err.Error())
526-
l.logger.Errorf("Failed ELBV2 (ALB) WAF (%s) association: %s", l.options.desired.wafACLID, err.Error())
525+
rOpts.Eventf(api.EventTypeWarning, "ERROR", "%s Web ACL (%s) association failed: %s", *l.lb.current.LoadBalancerName, l.options.desired.webACLId, err.Error())
526+
l.logger.Errorf("Failed ELBV2 (ALB) Web ACL (%s) association: %s", l.options.desired.webACLId, err.Error())
527527
return err
528528
}
529529
}
@@ -644,24 +644,24 @@ func (l *LoadBalancer) modify(rOpts *ReconcileOptions) error {
644644
l.logger.Infof("Completed ELBV2 tag modification. Attributes are %s.", log.Prettify(l.attributes.current))
645645
}
646646

647-
if needsMod&wafAssociationModified != 0 {
648-
if l.options.desired.wafACLID != nil {
649-
if _, err := albwaf.WAFRegionalsvc.Associate(l.lb.current.LoadBalancerArn, l.options.desired.wafACLID); err != nil {
650-
rOpts.Eventf(api.EventTypeWarning, "ERROR", "%s Waf (%s) association failed: %s", *l.lb.current.LoadBalancerName, *l.options.desired.wafACLID, err.Error())
651-
l.logger.Errorf("Failed ELBV2 (ALB) Waf (%s) association failed: %s", *l.options.desired.wafACLID, err.Error())
647+
if needsMod&webACLAssociationModified != 0 {
648+
if l.options.desired.webACLId != nil {
649+
if _, err := albwaf.WAFRegionalsvc.Associate(l.lb.current.LoadBalancerArn, l.options.desired.webACLId); err != nil {
650+
rOpts.Eventf(api.EventTypeWarning, "ERROR", "%s Web ACL (%s) association failed: %s", *l.lb.current.LoadBalancerName, *l.options.desired.webACLId, err.Error())
651+
l.logger.Errorf("Failure adding Web ACL (%s) association: %s", *l.options.desired.webACLId, err.Error())
652652
} else {
653-
l.options.current.wafACLID = l.options.desired.wafACLID
654-
rOpts.Eventf(api.EventTypeNormal, "MODIFY", "WAF Association updated to %s", *l.options.desired.wafACLID)
655-
l.logger.Infof("WAF Association updated %s", *l.options.desired.wafACLID)
653+
l.options.current.webACLId = l.options.desired.webACLId
654+
rOpts.Eventf(api.EventTypeNormal, "MODIFY", "Web ACL association updated to %s", *l.options.desired.webACLId)
655+
l.logger.Infof("Web ACL association updated to %s", *l.options.desired.webACLId)
656656
}
657-
} else if l.options.current.wafACLID != nil {
657+
} else if l.options.current.webACLId != nil {
658658
if _, err := albwaf.WAFRegionalsvc.Disassociate(l.lb.current.LoadBalancerArn); err != nil {
659-
rOpts.Eventf(api.EventTypeWarning, "ERROR", "%s Waf disassociation failed: %s", *l.lb.current.LoadBalancerName, err.Error())
660-
l.logger.Errorf("Failed ELBV2 (ALB) Waf disassociation failed: %s", err.Error())
659+
rOpts.Eventf(api.EventTypeWarning, "ERROR", "%s Web ACL disassociation failed: %s", *l.lb.current.LoadBalancerName, err.Error())
660+
l.logger.Errorf("Failure removing Web ACL association: %s", err.Error())
661661
} else {
662-
l.options.current.wafACLID = l.options.desired.wafACLID
663-
rOpts.Eventf(api.EventTypeNormal, "MODIFY", "WAF Disassociated")
664-
l.logger.Infof("WAF Disassociated")
662+
l.options.current.webACLId = l.options.desired.webACLId
663+
rOpts.Eventf(api.EventTypeNormal, "MODIFY", "Web ACL disassociated")
664+
l.logger.Infof("Web ACL disassociated")
665665
}
666666
}
667667
}
@@ -687,7 +687,7 @@ func (l *LoadBalancer) modify(rOpts *ReconcileOptions) error {
687687
func (l *LoadBalancer) delete(rOpts *ReconcileOptions) error {
688688

689689
// we need to disassociate the WAF before deletion
690-
if l.options.current.wafACLID != nil {
690+
if l.options.current.webACLId != nil {
691691
if _, err := albwaf.WAFRegionalsvc.Disassociate(l.lb.current.LoadBalancerArn); err != nil {
692692
rOpts.Eventf(api.EventTypeWarning, "ERROR", "Error disassociating WAF for %s: %s", *l.lb.current.LoadBalancerName, err.Error())
693693
l.logger.Errorf("Failed disassociation of ELBV2 (ALB) WAF: %s.", err.Error())
@@ -827,11 +827,11 @@ func (l *LoadBalancer) needsModification() (loadBalancerChange, bool) {
827827
changes |= attributesModified
828828
}
829829

830-
if dopts.wafACLID != nil && copts.wafACLID == nil ||
831-
dopts.wafACLID == nil && copts.wafACLID != nil ||
832-
(copts.wafACLID != nil && dopts.wafACLID != nil && *copts.wafACLID != *dopts.wafACLID) {
833-
l.logger.Debugf("WAF needs to be changed: (%v != %v)", log.Prettify(copts.wafACLID), log.Prettify(dopts.wafACLID))
834-
changes |= wafAssociationModified
830+
if dopts.webACLId != nil && copts.webACLId == nil ||
831+
dopts.webACLId == nil && copts.webACLId != nil ||
832+
(copts.webACLId != nil && dopts.webACLId != nil && *copts.webACLId != *dopts.webACLId) {
833+
l.logger.Debugf("WAF needs to be changed: (%v != %v)", log.Prettify(copts.webACLId), log.Prettify(dopts.webACLId))
834+
changes |= webACLAssociationModified
835835
}
836836
return changes, true
837837
}
@@ -841,7 +841,7 @@ func (l *LoadBalancer) StripDesiredState() {
841841
l.lb.desired = nil
842842
l.options.desired.ports = nil
843843
l.options.desired.managedSG = nil
844-
l.options.desired.wafACLID = nil
844+
l.options.desired.webACLId = nil
845845
if l.listeners != nil {
846846
l.listeners.StripDesiredState()
847847
}

‎pkg/alb/lb/loadbalancer_test.go

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,8 @@ const (
2222
tag1Value = "value1"
2323
tag2Key = "tag2"
2424
tag2Value = "value2"
25-
wafACL = "current-id-of-waf-acl"
26-
expectedWAFACL = "new-id-of-waf-acl"
25+
webACL = "current-id-of-web-acl"
26+
expectedWebACL = "new-id-of-web-acl"
2727
)
2828

2929
var (
@@ -34,8 +34,8 @@ var (
3434
expectedName string
3535
existing *elbv2.LoadBalancer
3636
lbOpts *NewCurrentLoadBalancerOptions
37-
expectedWaf *string
38-
currentWaf *string
37+
expectedWeb *string
38+
currentWeb *string
3939
)
4040

4141
func init() {
@@ -69,8 +69,8 @@ func init() {
6969
},
7070
}
7171

72-
currentWaf = aws.String(wafACL)
73-
expectedWaf = aws.String(expectedWAFACL)
72+
currentWeb = aws.String(webACL)
73+
expectedWeb = aws.String(expectedWebACL)
7474
lbOpts = &NewCurrentLoadBalancerOptions{
7575
LoadBalancer: existing,
7676
Logger: logr,
@@ -83,7 +83,7 @@ func TestNewDesiredLoadBalancer(t *testing.T) {
8383
anno := &annotations.Annotations{
8484
Scheme: lbScheme,
8585
SecurityGroups: types.AWSStringSlice{aws.String(sg1), aws.String(sg2)},
86-
WafACLID: expectedWaf,
86+
WebACLId: expectedWeb,
8787
}
8888

8989
lbOpts := &NewDesiredLoadBalancerOptions{
@@ -109,8 +109,8 @@ func TestNewDesiredLoadBalancer(t *testing.T) {
109109
t.Errorf("Security group was wrong. Expected: %s | Actual: %s", sg2, *l.lb.desired.SecurityGroups[0])
110110
case key1 != tag1Value:
111111
t.Errorf("Tag was invalid. Expected: %s | Actual: %s", tag1Value, key1)
112-
case *l.options.desired.wafACLID != *expectedWaf:
113-
t.Errorf("WAF ACL ID was invalid. Expected: %s | Actual: %s", *expectedWaf, *l.options.desired.wafACLID)
112+
case *l.options.desired.webACLId != *expectedWeb:
113+
t.Errorf("Web ACL ID was invalid. Expected: %s | Actual: %s", *expectedWeb, *l.options.desired.webACLId)
114114

115115
}
116116
}
@@ -128,9 +128,9 @@ func TestNewDesiredLoadBalancer(t *testing.T) {
128128
// case *l.lb.current.LoadBalancerName != expectedName:
129129
// t.Errorf("Current LB created returned improper LoadBalancerName. Expected: %s | "+
130130
// "Desired: %s", expectedName, *l.lb.current.LoadBalancerName)
131-
// case *l.options.current.wafACLID != *currentWaf:
132-
// t.Errorf("Current LB created returned improper WAF ACL Id. Expected: %s | "+
133-
// "Desired: %s", *currentWaf, *l.options.current.wafACLID)
131+
// case *l.options.current.webACLId != *currentWeb:
132+
// t.Errorf("Current LB created returned improper Web ACL Id. Expected: %s | "+
133+
// "Desired: %s", *currentWeb, *l.options.current.webACLId)
134134
// }
135135
// }
136136

‎pkg/alb/lb/types.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ type opts struct {
4747
idleTimeout *int64
4848
ports portList
4949
inboundCidrs util.Cidrs
50-
wafACLID *string
50+
webACLId *string
5151
managedSG *string
5252
managedInstanceSG *string
5353
}
@@ -63,7 +63,7 @@ const (
6363
managedSecurityGroupsModified
6464
connectionIdleTimeoutModified
6565
ipAddressTypeModified
66-
wafAssociationModified
66+
webACLAssociationModified
6767
)
6868

6969
type ReconcileOptions struct {

‎pkg/annotations/annotations.go

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,8 @@ const (
2828
backendProtocolKey = "alb.ingress.kubernetes.io/backend-protocol"
2929
certificateArnKey = "alb.ingress.kubernetes.io/certificate-arn"
3030
connectionIdleTimeoutKey = "alb.ingress.kubernetes.io/connection-idle-timeout"
31-
wafACLIDKey = "alb.ingress.kubernetes.io/waf-acl-id"
31+
webACLIdKey = "alb.ingress.kubernetes.io/web-acl-id"
32+
webACLIdAltKey = "alb.ingress.kubernetes.io/waf-acl-id"
3233
healthcheckIntervalSecondsKey = "alb.ingress.kubernetes.io/healthcheck-interval-seconds"
3334
healthcheckPathKey = "alb.ingress.kubernetes.io/healthcheck-path"
3435
healthcheckPortKey = "alb.ingress.kubernetes.io/healthcheck-port"
@@ -61,7 +62,7 @@ type Annotations struct {
6162
BackendProtocol *string
6263
CertificateArn *string
6364
ConnectionIdleTimeout *int64
64-
WafACLID *string
65+
WebACLId *string
6566
HealthcheckIntervalSeconds *int64
6667
HealthcheckPath *string
6768
HealthcheckPort *string
@@ -156,7 +157,7 @@ func (vf *ValidatingAnnotationFactory) ParseAnnotations(opts *ParseAnnotationsOp
156157
a.setSuccessCodes(annotations),
157158
a.setTags(annotations),
158159
a.setIgnoreHostHeader(annotations),
159-
a.setWafACLID(annotations, vf.validator),
160+
a.setWebACLId(annotations, vf.validator),
160161
a.setLoadBalancerAttributes(annotations),
161162
a.setTargetGroupAttributes(annotations),
162163
a.setSslPolicy(annotations, vf.validator),
@@ -714,15 +715,20 @@ func (a *Annotations) setIgnoreHostHeader(annotations map[string]string) error {
714715
return nil
715716
}
716717

717-
func (a *Annotations) setWafACLID(annotations map[string]string, validator Validator) error {
718-
if wafACLID, ok := annotations[wafACLIDKey]; ok {
719-
a.WafACLID = aws.String(wafACLID)
720-
if c := cacheLookup(wafACLID); c == nil || c.Expired() {
721-
if err := validator.ValidateWafACLID(a); err != nil {
722-
cache.Set(wafACLID, "error", 1*time.Hour)
718+
func (a *Annotations) setWebACLId(annotations map[string]string, validator Validator) error {
719+
webACLId, ok := annotations[webACLIdKey]
720+
if !ok {
721+
webACLId = annotations[webACLIdAltKey]
722+
}
723+
724+
if webACLId != "" {
725+
a.WebACLId = aws.String(webACLId)
726+
if c := cacheLookup(webACLId); c == nil || c.Expired() {
727+
if err := validator.ValidateWebACLId(a); err != nil {
728+
cache.Set(webACLId, "error", 1*time.Hour)
723729
return err
724730
}
725-
cache.Set(wafACLID, "success", 30*time.Minute)
731+
cache.Set(webACLId, "success", 30*time.Minute)
726732
}
727733
}
728734
return nil

‎pkg/annotations/validation.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ type Validator interface {
2424
ValidateCertARN(a *Annotations) error
2525
ValidateInboundCidrs(a *Annotations) error
2626
ValidateScheme(a *Annotations, ingressNamespace, ingressName string) bool
27-
ValidateWafACLID(a *Annotations) error
27+
ValidateWebACLId(a *Annotations) error
2828
ValidateSslPolicy(a *Annotations) error
2929
}
3030

@@ -106,9 +106,9 @@ func (v ConcreteValidator) ValidateScheme(a *Annotations, ingressNamespace, ingr
106106
return true
107107
}
108108

109-
func (v ConcreteValidator) ValidateWafACLID(a *Annotations) error {
110-
if success, err := albwaf.WAFRegionalsvc.WafAclExists(a.WafACLID); !success {
111-
return fmt.Errorf("waf ACL Id does not exist. Id: %s, error: %s", *a.WafACLID, err.Error())
109+
func (v ConcreteValidator) ValidateWebACLId(a *Annotations) error {
110+
if success, err := albwaf.WAFRegionalsvc.WebACLExists(a.WebACLId); !success {
111+
return fmt.Errorf("Web ACL Id does not exist. Id: %s, error: %s", *a.WebACLId, err.Error())
112112
}
113113
return nil
114114
}

‎pkg/annotations/validation_fake.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ type FakeValidator struct {
77
ValidateCertARNDelegate func() error
88
ValidateInboundCidrsDelegate func() error
99
ValidateSchemeDelegate func() bool
10-
ValidateWafACLIDDelegate func() error
10+
ValidateWebACLIdDelegate func() error
1111
ValidateSslPolicyDelegate func() error
1212
}
1313

@@ -47,9 +47,9 @@ func (fv FakeValidator) ValidateScheme(a *Annotations, ingressNamespace, ingress
4747
return true
4848
}
4949

50-
func (fv FakeValidator) ValidateWafACLID(a *Annotations) error {
51-
if fv.ValidateWafACLIDDelegate != nil {
52-
return fv.ValidateWafACLIDDelegate()
50+
func (fv FakeValidator) ValidateWebACLId(a *Annotations) error {
51+
if fv.ValidateWebACLIdDelegate != nil {
52+
return fv.ValidateWebACLIdDelegate()
5353
}
5454
return nil
5555
}

0 commit comments

Comments
 (0)