internal/dag: break out rate limit descriptor structs (projectcontour…

…#3597) Refactors the DAG model of rate limit actions to have one struct per type of rate limit action, rather than one flat struct with fields for all the possible action types. Signed-off-by: Steve Kriss <krisss@vmware.com>
stevesloka · Apr 20, 2021 · 38bb8b6 · 38bb8b6
1 parent bc94389
commit 38bb8b6
Show file tree

Hide file tree

Showing 5 changed files with 84 additions and 32 deletions.
diff --git a/internal/dag/dag.go b/internal/dag/dag.go
@@ -356,20 +356,38 @@ type GlobalRateLimitPolicy struct {
 	Descriptors []*RateLimitDescriptor
 }
 
+// RateLimitDescriptor is a list of rate limit descriptor entries.
 type RateLimitDescriptor struct {
 	Entries []RateLimitDescriptorEntry
 }
 
+// RateLimitDescriptorEntry is an entry in a rate limit descriptor.
+// Exactly one field should be non-nil.
 type RateLimitDescriptorEntry struct {
-	GenericKeyKey   string
-	GenericKeyValue string
+	GenericKey    *GenericKeyDescriptorEntry
+	HeaderMatch   *HeaderMatchDescriptorEntry
+	RemoteAddress *RemoteAddressDescriptorEntry
+}
 
-	HeaderMatchHeaderName    string
-	HeaderMatchDescriptorKey string
+// GenericKeyDescriptorEntry  configures a descriptor entry
+// that has a static key & value.
+type GenericKeyDescriptorEntry struct {
+	Key   string
+	Value string
+}
 
-	RemoteAddress bool
+// HeaderMatchDescriptorEntry configures a descriptor entry
+// that's populated only if the specified header is present
+// on the request.
+type HeaderMatchDescriptorEntry struct {
+	HeaderName string
+	Key        string
 }
 
+// RemoteAddressDescriptorEntry configures a descriptor entry
+// that contains the remote address (i.e. client IP).
+type RemoteAddressDescriptorEntry struct{}
+
 // CORSPolicy allows setting the CORS policy
 type CORSPolicy struct {
 	// Specifies whether the resource allows credentials.

diff --git a/internal/dag/policy.go b/internal/dag/policy.go
@@ -534,25 +534,29 @@ func globalRateLimitPolicy(in *contour_api_v1.GlobalRateLimitPolicy) (*GlobalRat
 				set++
 
 				rld.Entries = append(rld.Entries, RateLimitDescriptorEntry{
-					GenericKeyKey:   entry.GenericKey.Key,
-					GenericKeyValue: entry.GenericKey.Value,
+					GenericKey: &GenericKeyDescriptorEntry{
+						Key:   entry.GenericKey.Key,
+						Value: entry.GenericKey.Value,
+					},
 				})
 			}
 
 			if entry.RequestHeader != nil {
 				set++
 
 				rld.Entries = append(rld.Entries, RateLimitDescriptorEntry{
-					HeaderMatchHeaderName:    entry.RequestHeader.HeaderName,
-					HeaderMatchDescriptorKey: entry.RequestHeader.DescriptorKey,
+					HeaderMatch: &HeaderMatchDescriptorEntry{
+						HeaderName: entry.RequestHeader.HeaderName,
+						Key:        entry.RequestHeader.DescriptorKey,
+					},
 				})
 			}
 
 			if entry.RemoteAddress != nil {
 				set++
 
 				rld.Entries = append(rld.Entries, RateLimitDescriptorEntry{
-					RemoteAddress: true,
+					RemoteAddress: &RemoteAddressDescriptorEntry{},
 				})
 			}
 

diff --git a/internal/dag/policy_test.go b/internal/dag/policy_test.go
@@ -687,26 +687,32 @@ func TestRateLimitPolicy(t *testing.T) {
 						{
 							Entries: []RateLimitDescriptorEntry{
 								{
-									GenericKeyKey:   "generic-key-key",
-									GenericKeyValue: "generic-key-value",
+									GenericKey: &GenericKeyDescriptorEntry{
+										Key:   "generic-key-key",
+										Value: "generic-key-value",
+									},
 								},
 								{
-									RemoteAddress: true,
+									RemoteAddress: &RemoteAddressDescriptorEntry{},
 								},
 								{
-									HeaderMatchHeaderName:    "X-Header",
-									HeaderMatchDescriptorKey: "request-header-key",
+									HeaderMatch: &HeaderMatchDescriptorEntry{
+										HeaderName: "X-Header",
+										Key:        "request-header-key",
+									},
 								},
 							},
 						},
 						{
 							Entries: []RateLimitDescriptorEntry{
 								{
-									RemoteAddress: true,
+									RemoteAddress: &RemoteAddressDescriptorEntry{},
 								},
 								{
-									GenericKeyKey:   "generic-key-key-2",
-									GenericKeyValue: "generic-key-value-2",
+									GenericKey: &GenericKeyDescriptorEntry{
+										Key:   "generic-key-key-2",
+										Value: "generic-key-value-2",
+									},
 								},
 							},
 						},
@@ -773,7 +779,9 @@ func TestRateLimitPolicy(t *testing.T) {
 					Descriptors: []*RateLimitDescriptor{
 						{
 							Entries: []RateLimitDescriptorEntry{
-								{RemoteAddress: true},
+								{
+									RemoteAddress: &RemoteAddressDescriptorEntry{},
+								},
 							},
 						},
 					},

diff --git a/internal/envoy/v3/ratelimit.go b/internal/envoy/v3/ratelimit.go
@@ -75,25 +75,25 @@ func GlobalRateLimits(descriptors []*dag.RateLimitDescriptor) []*envoy_route_v3.
 
 		for _, entry := range descriptor.Entries {
 			switch {
-			case entry.GenericKeyValue != "":
+			case entry.GenericKey != nil:
 				rl.Actions = append(rl.Actions, &envoy_route_v3.RateLimit_Action{
 					ActionSpecifier: &envoy_route_v3.RateLimit_Action_GenericKey_{
 						GenericKey: &envoy_route_v3.RateLimit_Action_GenericKey{
-							DescriptorKey:   entry.GenericKeyKey,
-							DescriptorValue: entry.GenericKeyValue,
+							DescriptorKey:   entry.GenericKey.Key,
+							DescriptorValue: entry.GenericKey.Value,
 						},
 					},
 				})
-			case entry.HeaderMatchHeaderName != "":
+			case entry.HeaderMatch != nil:
 				rl.Actions = append(rl.Actions, &envoy_route_v3.RateLimit_Action{
 					ActionSpecifier: &envoy_route_v3.RateLimit_Action_RequestHeaders_{
 						RequestHeaders: &envoy_route_v3.RateLimit_Action_RequestHeaders{
-							HeaderName:    entry.HeaderMatchHeaderName,
-							DescriptorKey: entry.HeaderMatchDescriptorKey,
+							HeaderName:    entry.HeaderMatch.HeaderName,
+							DescriptorKey: entry.HeaderMatch.Key,
 						},
 					},
 				})
-			case entry.RemoteAddress:
+			case entry.RemoteAddress != nil:
 				rl.Actions = append(rl.Actions, &envoy_route_v3.RateLimit_Action{
 					ActionSpecifier: &envoy_route_v3.RateLimit_Action_RemoteAddress_{
 						RemoteAddress: &envoy_route_v3.RateLimit_Action_RemoteAddress{},

diff --git a/internal/envoy/v3/ratelimit_test.go b/internal/envoy/v3/ratelimit_test.go
@@ -105,16 +105,38 @@ func TestGlobalRateLimits(t *testing.T) {
 			descriptors: []*dag.RateLimitDescriptor{
 				{
 					Entries: []dag.RateLimitDescriptorEntry{
-						{RemoteAddress: true},
-						{GenericKeyValue: "generic-key-val"},
-						{GenericKeyKey: "generic-key-custom-key", GenericKeyValue: "generic-key-val"},
+						{
+							RemoteAddress: &dag.RemoteAddressDescriptorEntry{},
+						},
+						{
+							GenericKey: &dag.GenericKeyDescriptorEntry{
+								Value: "generic-key-val",
+							},
+						},
+						{
+							GenericKey: &dag.GenericKeyDescriptorEntry{
+								Key:   "generic-key-custom-key",
+								Value: "generic-key-val",
+							},
+						},
 					},
 				},
 				{
 					Entries: []dag.RateLimitDescriptorEntry{
-						{HeaderMatchHeaderName: "X-Header-1", HeaderMatchDescriptorKey: "foo"},
-						{RemoteAddress: true},
-						{GenericKeyValue: "generic-key-val-2"},
+						{
+							HeaderMatch: &dag.HeaderMatchDescriptorEntry{
+								HeaderName: "X-Header-1",
+								Key:        "foo",
+							},
+						},
+						{
+							RemoteAddress: &dag.RemoteAddressDescriptorEntry{},
+						},
+						{
+							GenericKey: &dag.GenericKeyDescriptorEntry{
+								Value: "generic-key-val-2",
+							},
+						},
 					},
 				},
 			},