From 768dbaa68d3b5a77e0203db1ad8bd02ea6bd3344 Mon Sep 17 00:00:00 2001 From: freddygv Date: Fri, 11 Sep 2020 18:34:03 -0600 Subject: [PATCH] Add session flag to cookie config --- agent/proxycfg/testing.go | 5 + agent/structs/config_entry_discoverychain.go | 14 +- .../config_entry_discoverychain_test.go | 25 ++- agent/structs/config_entry_test.go | 150 ++++++++++++++++++ agent/xds/routes.go | 11 +- agent/xds/routes_test.go | 73 +++++++++ ...t-proxy-lb-in-resolver.envoy-1-12-x.golden | 6 + ...t-proxy-lb-in-resolver.envoy-1-13-x.golden | 6 + ...t-proxy-lb-in-resolver.envoy-1-14-x.golden | 6 + ...t-proxy-lb-in-resolver.envoy-1-15-x.golden | 6 + ...ingress-lb-in-resolver.envoy-1-12-x.golden | 6 + ...ingress-lb-in-resolver.envoy-1-13-x.golden | 6 + ...ingress-lb-in-resolver.envoy-1-14-x.golden | 6 + ...ingress-lb-in-resolver.envoy-1-15-x.golden | 6 + api/config_entry_discoverychain.go | 5 +- api/config_entry_discoverychain_test.go | 8 + api/config_entry_test.go | 106 +++++++++++++ command/config/write/config_write_test.go | 45 ++++++ .../agent/config-entries/service-resolver.mdx | 4 +- 19 files changed, 486 insertions(+), 8 deletions(-) diff --git a/agent/proxycfg/testing.go b/agent/proxycfg/testing.go index 0514b8984c..1c01744935 100644 --- a/agent/proxycfg/testing.go +++ b/agent/proxycfg/testing.go @@ -1286,6 +1286,11 @@ func setupTestVariationConfigEntriesAndSnapshot( FieldValue: "chocolate-chip", Terminal: true, }, + { + Field: "cookie", + FieldValue: "chocolate-chip", + CookieConfig: &structs.CookieConfig{Session: true}, + }, { Field: "header", FieldValue: "x-user-id", diff --git a/agent/structs/config_entry_discoverychain.go b/agent/structs/config_entry_discoverychain.go index b977f89871..0317f144a4 100644 --- a/agent/structs/config_entry_discoverychain.go +++ b/agent/structs/config_entry_discoverychain.go @@ -882,8 +882,13 @@ func (e *ServiceResolverConfigEntry) Validate() error { if hp.FieldValue != "" && hp.Field == "" { return fmt.Errorf("Bad LoadBalancer HashPolicy[%d]: FieldValue requires a Field to apply to", i) } - if hp.CookieConfig != nil && hp.Field != HashPolicyCookie { - return fmt.Errorf("Bad LoadBalancer HashPolicy[%d]: cookie_config provided for %q", i, hp.Field) + if hp.CookieConfig != nil { + if hp.Field != HashPolicyCookie { + return fmt.Errorf("Bad LoadBalancer HashPolicy[%d]: cookie_config provided for %q", i, hp.Field) + } + if hp.CookieConfig.Session && hp.CookieConfig.TTL != 0*time.Second { + return fmt.Errorf("Bad LoadBalancer HashPolicy[%d]: a session cookie cannot have an associated TTL", i) + } } } } @@ -1087,7 +1092,10 @@ type HashPolicy struct { // CookieConfig contains configuration for the "cookie" hash policy type. // This is specified to have Envoy generate a cookie for a client on its first request. type CookieConfig struct { - // TTL for generated cookies + // Generates a session cookie with no expiration. + Session bool `json:",omitempty"` + + // TTL for generated cookies. Cannot be specified for session cookies. TTL time.Duration `json:",omitempty"` // The path to set for the cookie diff --git a/agent/structs/config_entry_discoverychain_test.go b/agent/structs/config_entry_discoverychain_test.go index 3e5d99f9e9..09ca4eaa66 100644 --- a/agent/structs/config_entry_discoverychain_test.go +++ b/agent/structs/config_entry_discoverychain_test.go @@ -656,7 +656,7 @@ func TestServiceResolverConfigEntry_LoadBalancer(t *testing.T) { validateErr: `HashPolicies specified for non-hash-based Policy`, }, { - name: "empty policy with hash policy", + name: "cookie config with header policy", entry: &ServiceResolverConfigEntry{ Kind: ServiceResolver, Name: "test", @@ -677,7 +677,28 @@ func TestServiceResolverConfigEntry_LoadBalancer(t *testing.T) { validateErr: `cookie_config provided for "header"`, }, { - name: "empty policy with hash policy", + name: "cannot generate session cookie with ttl", + entry: &ServiceResolverConfigEntry{ + Kind: ServiceResolver, + Name: "test", + LoadBalancer: &LoadBalancer{ + Policy: LBPolicyMaglev, + HashPolicies: []HashPolicy{ + { + Field: HashPolicyCookie, + FieldValue: "good-cookie", + CookieConfig: &CookieConfig{ + Session: true, + TTL: 10 * time.Second, + }, + }, + }, + }, + }, + validateErr: `a session cookie cannot have an associated TTL`, + }, + { + name: "valid cookie policy", entry: &ServiceResolverConfigEntry{ Kind: ServiceResolver, Name: "test", diff --git a/agent/structs/config_entry_test.go b/agent/structs/config_entry_test.go index 34ccfbaf46..0b056390fd 100644 --- a/agent/structs/config_entry_test.go +++ b/agent/structs/config_entry_test.go @@ -531,6 +531,156 @@ func TestDecodeConfigEntry(t *testing.T) { Name: "main", }, }, + { + name: "service-resolver: envoy hash lb kitchen sink", + snake: ` + kind = "service-resolver" + name = "main" + load_balancer = { + policy = "ring_hash" + ring_hash_config = { + minimum_ring_size = 1 + maximum_ring_size = 2 + } + hash_policies = [ + { + field = "cookie" + field_value = "good-cookie" + cookie_config = { + ttl = "1s" + path = "/oven" + } + terminal = true + }, + { + field = "cookie" + field_value = "less-good-cookie" + cookie_config = { + session = true + path = "/toaster" + } + terminal = true + }, + { + field = "header" + field_value = "x-user-id" + }, + { + source_ip = true + } + ] + } + `, + camel: ` + Kind = "service-resolver" + Name = "main" + LoadBalancer = { + Policy = "ring_hash" + RingHashConfig = { + MinimumRingSize = 1 + MaximumRingSize = 2 + } + HashPolicies = [ + { + Field = "cookie" + FieldValue = "good-cookie" + CookieConfig = { + TTL = "1s" + Path = "/oven" + } + Terminal = true + }, + { + Field = "cookie" + FieldValue = "less-good-cookie" + CookieConfig = { + Session = true + Path = "/toaster" + } + Terminal = true + }, + { + Field = "header" + FieldValue = "x-user-id" + }, + { + SourceIP = true + } + ] + } + `, + expect: &ServiceResolverConfigEntry{ + Kind: "service-resolver", + Name: "main", + LoadBalancer: &LoadBalancer{ + Policy: LBPolicyRingHash, + RingHashConfig: &RingHashConfig{ + MinimumRingSize: 1, + MaximumRingSize: 2, + }, + HashPolicies: []HashPolicy{ + { + Field: HashPolicyCookie, + FieldValue: "good-cookie", + CookieConfig: &CookieConfig{ + TTL: 1 * time.Second, + Path: "/oven", + }, + Terminal: true, + }, + { + Field: HashPolicyCookie, + FieldValue: "less-good-cookie", + CookieConfig: &CookieConfig{ + Session: true, + Path: "/toaster", + }, + Terminal: true, + }, + { + Field: HashPolicyHeader, + FieldValue: "x-user-id", + }, + { + SourceIP: true, + }, + }, + }, + }, + }, + { + name: "service-resolver: envoy least request kitchen sink", + snake: ` + kind = "service-resolver" + name = "main" + load_balancer = { + policy = "least_request" + least_request_config = { + choice_count = 2 + } + } + `, + camel: ` + Kind = "service-resolver" + Name = "main" + LoadBalancer = { + Policy = "least_request" + LeastRequestConfig = { + ChoiceCount = 2 + } + } + `, + expect: &ServiceResolverConfigEntry{ + Kind: "service-resolver", + Name: "main", + LoadBalancer: &LoadBalancer{ + Policy: LBPolicyLeastRequest, + LeastRequestConfig: &LeastRequestConfig{ + ChoiceCount: 2, + }, + }, + }, + }, { name: "ingress-gateway: kitchen sink", snake: ` diff --git a/agent/xds/routes.go b/agent/xds/routes.go index ad20f5f189..c50528d0a3 100644 --- a/agent/xds/routes.go +++ b/agent/xds/routes.go @@ -5,6 +5,7 @@ import ( "fmt" "net" "strings" + "time" envoy "github.com/envoyproxy/go-control-plane/envoy/api/v2" envoyroute "github.com/envoyproxy/go-control-plane/envoy/api/v2/route" @@ -614,8 +615,16 @@ func injectLBToRouteAction(lb *structs.LoadBalancer, action *envoyroute.RouteAct Name: policy.FieldValue, } if policy.CookieConfig != nil { - cookie.Ttl = ptypes.DurationProto(policy.CookieConfig.TTL) cookie.Path = policy.CookieConfig.Path + + if policy.CookieConfig.TTL != 0*time.Second { + cookie.Ttl = ptypes.DurationProto(policy.CookieConfig.TTL) + } + + // Envoy will generate a session cookie if the ttl is present and zero. + if policy.CookieConfig.Session { + cookie.Ttl = ptypes.DurationProto(0 * time.Second) + } } result = append(result, &envoyroute.RouteAction_HashPolicy{ PolicySpecifier: &envoyroute.RouteAction_HashPolicy_Cookie_{ diff --git a/agent/xds/routes_test.go b/agent/xds/routes_test.go index 43d2857aea..bb20fc5379 100644 --- a/agent/xds/routes_test.go +++ b/agent/xds/routes_test.go @@ -376,6 +376,62 @@ func TestEnvoyLBConfig_InjectToRouteAction(t *testing.T) { }, }, }, + { + name: "non-zero session ttl gets zeroed out", + lb: &structs.LoadBalancer{ + Policy: structs.LBPolicyMaglev, + HashPolicies: []structs.HashPolicy{ + { + Field: structs.HashPolicyCookie, + FieldValue: "oatmeal", + CookieConfig: &structs.CookieConfig{ + TTL: 10 * time.Second, + Session: true, + }, + }, + }, + }, + expected: envoyroute.RouteAction{ + HashPolicy: []*envoyroute.RouteAction_HashPolicy{ + { + PolicySpecifier: &envoyroute.RouteAction_HashPolicy_Cookie_{ + Cookie: &envoyroute.RouteAction_HashPolicy_Cookie{ + Name: "oatmeal", + Ttl: ptypes.DurationProto(0 * time.Second), + }, + }, + }, + }, + }, + }, + { + name: "zero value ttl omitted if not session cookie", + lb: &structs.LoadBalancer{ + Policy: structs.LBPolicyMaglev, + HashPolicies: []structs.HashPolicy{ + { + Field: structs.HashPolicyCookie, + FieldValue: "oatmeal", + CookieConfig: &structs.CookieConfig{ + Path: "/oven", + }, + }, + }, + }, + expected: envoyroute.RouteAction{ + HashPolicy: []*envoyroute.RouteAction_HashPolicy{ + { + PolicySpecifier: &envoyroute.RouteAction_HashPolicy_Cookie_{ + Cookie: &envoyroute.RouteAction_HashPolicy_Cookie{ + Name: "oatmeal", + Path: "/oven", + Ttl: nil, + }, + }, + }, + }, + }, + }, { name: "source addr", lb: &structs.LoadBalancer{ @@ -417,6 +473,14 @@ func TestEnvoyLBConfig_InjectToRouteAction(t *testing.T) { Path: "/oven", }, }, + { + Field: structs.HashPolicyCookie, + FieldValue: "chocolate-chip", + CookieConfig: &structs.CookieConfig{ + Session: true, + Path: "/oven", + }, + }, { Field: structs.HashPolicyHeader, FieldValue: "special-header", @@ -443,6 +507,15 @@ func TestEnvoyLBConfig_InjectToRouteAction(t *testing.T) { }, }, }, + { + PolicySpecifier: &envoyroute.RouteAction_HashPolicy_Cookie_{ + Cookie: &envoyroute.RouteAction_HashPolicy_Cookie{ + Name: "chocolate-chip", + Ttl: ptypes.DurationProto(0 * time.Second), + Path: "/oven", + }, + }, + }, { PolicySpecifier: &envoyroute.RouteAction_HashPolicy_Header_{ Header: &envoyroute.RouteAction_HashPolicy_Header{ diff --git a/agent/xds/testdata/routes/connect-proxy-lb-in-resolver.envoy-1-12-x.golden b/agent/xds/testdata/routes/connect-proxy-lb-in-resolver.envoy-1-12-x.golden index 6fa9287ba7..608140c669 100644 --- a/agent/xds/testdata/routes/connect-proxy-lb-in-resolver.envoy-1-12-x.golden +++ b/agent/xds/testdata/routes/connect-proxy-lb-in-resolver.envoy-1-12-x.golden @@ -36,6 +36,12 @@ }, "terminal": true }, + { + "cookie": { + "name": "chocolate-chip", + "ttl": "0s" + } + }, { "header": { "headerName": "x-user-id" diff --git a/agent/xds/testdata/routes/connect-proxy-lb-in-resolver.envoy-1-13-x.golden b/agent/xds/testdata/routes/connect-proxy-lb-in-resolver.envoy-1-13-x.golden index 6fa9287ba7..608140c669 100644 --- a/agent/xds/testdata/routes/connect-proxy-lb-in-resolver.envoy-1-13-x.golden +++ b/agent/xds/testdata/routes/connect-proxy-lb-in-resolver.envoy-1-13-x.golden @@ -36,6 +36,12 @@ }, "terminal": true }, + { + "cookie": { + "name": "chocolate-chip", + "ttl": "0s" + } + }, { "header": { "headerName": "x-user-id" diff --git a/agent/xds/testdata/routes/connect-proxy-lb-in-resolver.envoy-1-14-x.golden b/agent/xds/testdata/routes/connect-proxy-lb-in-resolver.envoy-1-14-x.golden index 6fa9287ba7..608140c669 100644 --- a/agent/xds/testdata/routes/connect-proxy-lb-in-resolver.envoy-1-14-x.golden +++ b/agent/xds/testdata/routes/connect-proxy-lb-in-resolver.envoy-1-14-x.golden @@ -36,6 +36,12 @@ }, "terminal": true }, + { + "cookie": { + "name": "chocolate-chip", + "ttl": "0s" + } + }, { "header": { "headerName": "x-user-id" diff --git a/agent/xds/testdata/routes/connect-proxy-lb-in-resolver.envoy-1-15-x.golden b/agent/xds/testdata/routes/connect-proxy-lb-in-resolver.envoy-1-15-x.golden index 6fa9287ba7..608140c669 100644 --- a/agent/xds/testdata/routes/connect-proxy-lb-in-resolver.envoy-1-15-x.golden +++ b/agent/xds/testdata/routes/connect-proxy-lb-in-resolver.envoy-1-15-x.golden @@ -36,6 +36,12 @@ }, "terminal": true }, + { + "cookie": { + "name": "chocolate-chip", + "ttl": "0s" + } + }, { "header": { "headerName": "x-user-id" diff --git a/agent/xds/testdata/routes/ingress-lb-in-resolver.envoy-1-12-x.golden b/agent/xds/testdata/routes/ingress-lb-in-resolver.envoy-1-12-x.golden index 1dce540e7a..fa9217491d 100644 --- a/agent/xds/testdata/routes/ingress-lb-in-resolver.envoy-1-12-x.golden +++ b/agent/xds/testdata/routes/ingress-lb-in-resolver.envoy-1-12-x.golden @@ -37,6 +37,12 @@ }, "terminal": true }, + { + "cookie": { + "name": "chocolate-chip", + "ttl": "0s" + } + }, { "header": { "headerName": "x-user-id" diff --git a/agent/xds/testdata/routes/ingress-lb-in-resolver.envoy-1-13-x.golden b/agent/xds/testdata/routes/ingress-lb-in-resolver.envoy-1-13-x.golden index 1dce540e7a..fa9217491d 100644 --- a/agent/xds/testdata/routes/ingress-lb-in-resolver.envoy-1-13-x.golden +++ b/agent/xds/testdata/routes/ingress-lb-in-resolver.envoy-1-13-x.golden @@ -37,6 +37,12 @@ }, "terminal": true }, + { + "cookie": { + "name": "chocolate-chip", + "ttl": "0s" + } + }, { "header": { "headerName": "x-user-id" diff --git a/agent/xds/testdata/routes/ingress-lb-in-resolver.envoy-1-14-x.golden b/agent/xds/testdata/routes/ingress-lb-in-resolver.envoy-1-14-x.golden index 1dce540e7a..fa9217491d 100644 --- a/agent/xds/testdata/routes/ingress-lb-in-resolver.envoy-1-14-x.golden +++ b/agent/xds/testdata/routes/ingress-lb-in-resolver.envoy-1-14-x.golden @@ -37,6 +37,12 @@ }, "terminal": true }, + { + "cookie": { + "name": "chocolate-chip", + "ttl": "0s" + } + }, { "header": { "headerName": "x-user-id" diff --git a/agent/xds/testdata/routes/ingress-lb-in-resolver.envoy-1-15-x.golden b/agent/xds/testdata/routes/ingress-lb-in-resolver.envoy-1-15-x.golden index 1dce540e7a..fa9217491d 100644 --- a/agent/xds/testdata/routes/ingress-lb-in-resolver.envoy-1-15-x.golden +++ b/agent/xds/testdata/routes/ingress-lb-in-resolver.envoy-1-15-x.golden @@ -37,6 +37,12 @@ }, "terminal": true }, + { + "cookie": { + "name": "chocolate-chip", + "ttl": "0s" + } + }, { "header": { "headerName": "x-user-id" diff --git a/api/config_entry_discoverychain.go b/api/config_entry_discoverychain.go index d7a02f839a..5b989e1054 100644 --- a/api/config_entry_discoverychain.go +++ b/api/config_entry_discoverychain.go @@ -269,7 +269,10 @@ type HashPolicy struct { // CookieConfig contains configuration for the "cookie" hash policy type. // This is specified to have Envoy generate a cookie for a client on its first request. type CookieConfig struct { - // TTL for generated cookies + // Generates a session cookie with no expiration. + Session bool `json:",omitempty"` + + // TTL for generated cookies. Cannot be specified for session cookies. TTL time.Duration `json:",omitempty"` // The path to set for the cookie diff --git a/api/config_entry_discoverychain_test.go b/api/config_entry_discoverychain_test.go index abd34dabc8..c5e6d82d9d 100644 --- a/api/config_entry_discoverychain_test.go +++ b/api/config_entry_discoverychain_test.go @@ -326,6 +326,14 @@ func TestAPI_ConfigEntry_ServiceResolver_LoadBalancer(t *testing.T) { TTL: 20 * time.Millisecond, }, }, + { + Field: "cookie", + FieldValue: "sugar", + CookieConfig: &CookieConfig{ + Session: true, + Path: "/tin", + }, + }, { SourceIP: true, }, diff --git a/api/config_entry_test.go b/api/config_entry_test.go index aa384bb9f1..3bb2239586 100644 --- a/api/config_entry_test.go +++ b/api/config_entry_test.go @@ -613,6 +613,112 @@ func TestDecodeConfigEntry(t *testing.T) { Name: "main", }, }, + { + name: "service-resolver: envoy hash lb kitchen sink", + body: ` + { + "Kind": "service-resolver", + "Name": "main", + "LoadBalancer": { + "Policy": "ring_hash", + "RingHashConfig": { + "MinimumRingSize": 1, + "MaximumRingSize": 2 + }, + "HashPolicies": [ + { + "Field": "cookie", + "FieldValue": "good-cookie", + "CookieConfig": { + "TTL": "1s", + "Path": "/oven" + }, + "Terminal": true + }, + { + "Field": "cookie", + "FieldValue": "less-good-cookie", + "CookieConfig": { + "Session": true, + "Path": "/toaster" + }, + "Terminal": true + }, + { + "Field": "header", + "FieldValue": "x-user-id" + }, + { + "SourceIP": true + } + ] + } + } + `, + expect: &ServiceResolverConfigEntry{ + Kind: "service-resolver", + Name: "main", + LoadBalancer: &LoadBalancer{ + Policy: "ring_hash", + RingHashConfig: &RingHashConfig{ + MinimumRingSize: 1, + MaximumRingSize: 2, + }, + HashPolicies: []HashPolicy{ + { + Field: "cookie", + FieldValue: "good-cookie", + CookieConfig: &CookieConfig{ + TTL: 1 * time.Second, + Path: "/oven", + }, + Terminal: true, + }, + { + Field: "cookie", + FieldValue: "less-good-cookie", + CookieConfig: &CookieConfig{ + Session: true, + Path: "/toaster", + }, + Terminal: true, + }, + { + Field: "header", + FieldValue: "x-user-id", + }, + { + SourceIP: true, + }, + }, + }, + }, + }, + { + name: "service-resolver: envoy least request kitchen sink", + body: ` + { + "Kind": "service-resolver", + "Name": "main", + "LoadBalancer": { + "Policy": "least_request", + "LeastRequestConfig": { + "ChoiceCount": 2 + } + } + } + `, + expect: &ServiceResolverConfigEntry{ + Kind: "service-resolver", + Name: "main", + LoadBalancer: &LoadBalancer{ + Policy: "least_request", + LeastRequestConfig: &LeastRequestConfig{ + ChoiceCount: 2, + }, + }, + }, + }, { name: "ingress-gateway", body: ` diff --git a/command/config/write/config_write_test.go b/command/config/write/config_write_test.go index 601d232a52..e4d9e27883 100644 --- a/command/config/write/config_write_test.go +++ b/command/config/write/config_write_test.go @@ -1187,6 +1187,15 @@ func TestParseConfigEntry(t *testing.T) { } terminal = true }, + { + field = "cookie" + field_value = "less-good-cookie" + cookie_config = { + session = true + path = "/toaster" + } + terminal = true + }, { field = "header" field_value = "x-user-id" @@ -1216,6 +1225,15 @@ func TestParseConfigEntry(t *testing.T) { } Terminal = true }, + { + Field = "cookie" + FieldValue = "less-good-cookie" + CookieConfig = { + Session = true + Path = "/toaster" + } + Terminal = true + }, { Field = "header" FieldValue = "x-user-id" @@ -1246,6 +1264,15 @@ func TestParseConfigEntry(t *testing.T) { }, "terminal": true }, + { + "field": "cookie", + "field_value": "less-good-cookie", + "cookie_config": { + "session": true, + "path": "/toaster" + }, + "terminal": true + }, { "field": "header", "field_value": "x-user-id" @@ -1277,6 +1304,15 @@ func TestParseConfigEntry(t *testing.T) { }, "Terminal": true }, + { + "Field": "cookie", + "FieldValue": "less-good-cookie", + "CookieConfig": { + "Session": true, + "Path": "/toaster" + }, + "Terminal": true + }, { "Field": "header", "FieldValue": "x-user-id" @@ -1307,6 +1343,15 @@ func TestParseConfigEntry(t *testing.T) { }, Terminal: true, }, + { + Field: structs.HashPolicyCookie, + FieldValue: "less-good-cookie", + CookieConfig: &api.CookieConfig{ + Session: true, + Path: "/toaster", + }, + Terminal: true, + }, { Field: structs.HashPolicyHeader, FieldValue: "x-user-id", diff --git a/website/pages/docs/agent/config-entries/service-resolver.mdx b/website/pages/docs/agent/config-entries/service-resolver.mdx index 5317494419..115fd966c3 100644 --- a/website/pages/docs/agent/config-entries/service-resolver.mdx +++ b/website/pages/docs/agent/config-entries/service-resolver.mdx @@ -206,7 +206,9 @@ LoadBalancer = { - `CookieConfig` `(CookieConfig)` - Additional configuration for the "cookie" hash policy type. This is specified to have Envoy generate a cookie for a client on its first request. - - `TTL` `(duration: 0s)` - TTL for generated cookies. + - `Session` `(bool: false)` - Generates a session cookie with no expiration. + + - `TTL` `(duration: 0s)` - TTL for generated cookies. Cannot be specified for session cookies. - `Path` `(string: "")` - The path to set for the cookie.