From 0b4dcb06c2700717589584781a219817759736f7 Mon Sep 17 00:00:00 2001 From: Benjamin Pineau Date: Thu, 25 Oct 2018 14:28:45 +0200 Subject: [PATCH 1/4] BackendConfig support for session affinity Allow user provided session affinity type and ttl. Sending e2e/fuzz tests on a distinct PR. --- pkg/apis/backendconfig/v1beta1/types.go | 12 +- .../v1beta1/zz_generated.deepcopy.go | 9 ++ .../v1beta1/zz_generated.openapi.go | 12 ++ pkg/backends/features/affinity.go | 50 ++++++++ pkg/backends/features/affinity_test.go | 119 ++++++++++++++++++ pkg/backends/syncer.go | 1 + 6 files changed, 198 insertions(+), 5 deletions(-) create mode 100644 pkg/backends/features/affinity.go create mode 100644 pkg/backends/features/affinity_test.go diff --git a/pkg/apis/backendconfig/v1beta1/types.go b/pkg/apis/backendconfig/v1beta1/types.go index e0cab555ce..64639ab660 100644 --- a/pkg/apis/backendconfig/v1beta1/types.go +++ b/pkg/apis/backendconfig/v1beta1/types.go @@ -36,11 +36,13 @@ type BackendConfig struct { // BackendConfigSpec is the spec for a BackendConfig resource // +k8s:openapi-gen=true type BackendConfigSpec struct { - Iap *IAPConfig `json:"iap,omitempty"` - Cdn *CDNConfig `json:"cdn,omitempty"` - SecurityPolicy *SecurityPolicyConfig `json:"securityPolicy,omitempty"` - TimeoutSec *int64 `json:"timeoutSec,omitempty"` - ConnectionDraining *ConnectionDrainingConfig `json:"connectionDraining,omitempty"` + Iap *IAPConfig `json:"iap,omitempty"` + Cdn *CDNConfig `json:"cdn,omitempty"` + SecurityPolicy *SecurityPolicyConfig `json:"securityPolicy,omitempty"` + TimeoutSec *int64 `json:"timeoutSec,omitempty"` + ConnectionDraining *ConnectionDrainingConfig `json:"connectionDraining,omitempty"` + AffinityCookieTtlSec *int64 `json:"affinityCookieTtlSec,omitempty"` + SessionAffinity string `json:"sessionAffinity,omitempty"` } // BackendConfigStatus is the status for a BackendConfig resource diff --git a/pkg/apis/backendconfig/v1beta1/zz_generated.deepcopy.go b/pkg/apis/backendconfig/v1beta1/zz_generated.deepcopy.go index 43f33f0c95..c4dcebef76 100644 --- a/pkg/apis/backendconfig/v1beta1/zz_generated.deepcopy.go +++ b/pkg/apis/backendconfig/v1beta1/zz_generated.deepcopy.go @@ -133,6 +133,15 @@ func (in *BackendConfigSpec) DeepCopyInto(out *BackendConfigSpec) { (*in).DeepCopyInto(*out) } } + if in.AffinityCookieTtlSec != nil { + in, out := &in.AffinityCookieTtlSec, &out.AffinityCookieTtlSec + if *in == nil { + *out = nil + } else { + *out = new(int64) + **out = **in + } + } return } diff --git a/pkg/apis/backendconfig/v1beta1/zz_generated.openapi.go b/pkg/apis/backendconfig/v1beta1/zz_generated.openapi.go index 0d9a3a05f7..70087cad2d 100644 --- a/pkg/apis/backendconfig/v1beta1/zz_generated.openapi.go +++ b/pkg/apis/backendconfig/v1beta1/zz_generated.openapi.go @@ -99,6 +99,18 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA Ref: ref("k8s.io/ingress-gce/pkg/apis/backendconfig/v1beta1.ConnectionDrainingConfig"), }, }, + "affinityCookieTtlSec": { + SchemaProps: spec.SchemaProps{ + Type: []string{"integer"}, + Format: "int64", + }, + }, + "sessionAffinity": { + SchemaProps: spec.SchemaProps{ + Type: []string{"string"}, + Format: "", + }, + }, }, }, }, diff --git a/pkg/backends/features/affinity.go b/pkg/backends/features/affinity.go new file mode 100644 index 0000000000..c3d7ebc3de --- /dev/null +++ b/pkg/backends/features/affinity.go @@ -0,0 +1,50 @@ +/* +Copyright 2018 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package features + +import ( + "strings" + + "github.com/golang/glog" + "k8s.io/ingress-gce/pkg/composite" + "k8s.io/ingress-gce/pkg/utils" +) + +// EnsureAffinity reads the sessionAffinity and AffinityCookieTtlSec configuration +// specified in the ServicePort.BackendConfig and applies it to the BackendService. +// It returns true if there were existing settings on the BackendService +// that were overwritten. +func EnsureAffinity(sp utils.ServicePort, be *composite.BackendService) bool { + changed := false + + // Should we check if specified SessionAffinity is among the currently GCP supported values? + // For now let's forward as is to GCP, to inherit API error message (and evolutions). + // Same for TTL (should be in 0-86400 range). + if sp.BackendConfig.Spec.SessionAffinity != "" && strings.Compare(sp.BackendConfig.Spec.SessionAffinity, be.SessionAffinity) != 0 { + be.SessionAffinity = sp.BackendConfig.Spec.SessionAffinity + glog.V(2).Infof("Updated SessionAffinity settings for service %v/%v.", sp.ID.Service.Namespace, sp.ID.Service.Name) + changed = true + } + + if sp.BackendConfig.Spec.AffinityCookieTtlSec != nil && *sp.BackendConfig.Spec.AffinityCookieTtlSec != be.AffinityCookieTtlSec { + be.AffinityCookieTtlSec = *sp.BackendConfig.Spec.AffinityCookieTtlSec + glog.V(2).Infof("Updated AffinityCookieTtlSec settings for service %v/%v.", sp.ID.Service.Namespace, sp.ID.Service.Name) + changed = true + } + + return changed +} diff --git a/pkg/backends/features/affinity_test.go b/pkg/backends/features/affinity_test.go new file mode 100644 index 0000000000..3722fdd8eb --- /dev/null +++ b/pkg/backends/features/affinity_test.go @@ -0,0 +1,119 @@ +/* +Copyright 2015 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package features + +import ( + "testing" + + backendconfigv1beta1 "k8s.io/ingress-gce/pkg/apis/backendconfig/v1beta1" + "k8s.io/ingress-gce/pkg/composite" + "k8s.io/ingress-gce/pkg/utils" +) + +var testTTL = int64(10) + +func TestEnsureAffinity(t *testing.T) { + testCases := []struct { + desc string + sp utils.ServicePort + be *composite.BackendService + updateExpected bool + }{ + { + desc: "affinity settings missing from spec, no update needed", + sp: utils.ServicePort{ + BackendConfig: &backendconfigv1beta1.BackendConfig{ + Spec: backendconfigv1beta1.BackendConfigSpec{}, + }, + }, + be: &composite.BackendService{ + SessionAffinity: "GENERATED_COOKIE", + AffinityCookieTtlSec: 10, + }, + updateExpected: false, + }, + { + desc: "sessionaffinity setting differing, update needed", + sp: utils.ServicePort{ + BackendConfig: &backendconfigv1beta1.BackendConfig{ + Spec: backendconfigv1beta1.BackendConfigSpec{ + SessionAffinity: "CLIENT_IP", + }, + }, + }, + be: &composite.BackendService{ + SessionAffinity: "NONE", + }, + updateExpected: true, + }, + { + desc: "affinity ttl setting differing, update needed", + sp: utils.ServicePort{ + BackendConfig: &backendconfigv1beta1.BackendConfig{ + Spec: backendconfigv1beta1.BackendConfigSpec{ + AffinityCookieTtlSec: &testTTL, + }, + }, + }, + be: &composite.BackendService{ + AffinityCookieTtlSec: 20, + }, + updateExpected: true, + }, + { + desc: "sessionaffinity and ttl settings differing, update needed", + sp: utils.ServicePort{ + BackendConfig: &backendconfigv1beta1.BackendConfig{ + Spec: backendconfigv1beta1.BackendConfigSpec{ + SessionAffinity: "CLIENT_IP", + AffinityCookieTtlSec: &testTTL, + }, + }, + }, + be: &composite.BackendService{ + SessionAffinity: "NONE", + AffinityCookieTtlSec: 20, + }, + updateExpected: true, + }, + { + desc: "affinity settings identical, no updated needed", + sp: utils.ServicePort{ + BackendConfig: &backendconfigv1beta1.BackendConfig{ + Spec: backendconfigv1beta1.BackendConfigSpec{ + SessionAffinity: "CLIENT_IP", + AffinityCookieTtlSec: &testTTL, + }, + }, + }, + be: &composite.BackendService{ + SessionAffinity: "CLIENT_IP", + AffinityCookieTtlSec: testTTL, + }, + updateExpected: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + result := EnsureAffinity(tc.sp, tc.be) + if result != tc.updateExpected { + t.Errorf("%v: expected %v but got %v", tc.desc, tc.updateExpected, result) + } + }) + } +} diff --git a/pkg/backends/syncer.go b/pkg/backends/syncer.go index 810813c6a0..6a451e8a34 100644 --- a/pkg/backends/syncer.go +++ b/pkg/backends/syncer.go @@ -114,6 +114,7 @@ func (s *backendSyncer) ensureBackendService(sp utils.ServicePort) error { needUpdate = features.EnsureIAP(sp, be) || needUpdate needUpdate = features.EnsureTimeout(sp, be) || needUpdate needUpdate = features.EnsureDraining(sp, be) || needUpdate + needUpdate = features.EnsureAffinity(sp, be) || needUpdate } if needUpdate { From 37eb6aa51514f4faf9e2b783c6edbd2038116fe1 Mon Sep 17 00:00:00 2001 From: Benjamin Pineau Date: Thu, 25 Oct 2018 22:08:27 +0200 Subject: [PATCH 2/4] Group affinity settings + validation --- pkg/apis/backendconfig/v1beta1/types.go | 20 ++++++---- .../v1beta1/zz_generated.deepcopy.go | 33 ++++++++++++++-- .../v1beta1/zz_generated.openapi.go | 33 +++++++++++----- pkg/backendconfig/validation.go | 39 ++++++++++++++++++- pkg/backends/features/affinity.go | 17 ++++---- pkg/backends/features/affinity_test.go | 20 +++++++--- 6 files changed, 128 insertions(+), 34 deletions(-) diff --git a/pkg/apis/backendconfig/v1beta1/types.go b/pkg/apis/backendconfig/v1beta1/types.go index 64639ab660..ceac78fc36 100644 --- a/pkg/apis/backendconfig/v1beta1/types.go +++ b/pkg/apis/backendconfig/v1beta1/types.go @@ -36,13 +36,12 @@ type BackendConfig struct { // BackendConfigSpec is the spec for a BackendConfig resource // +k8s:openapi-gen=true type BackendConfigSpec struct { - Iap *IAPConfig `json:"iap,omitempty"` - Cdn *CDNConfig `json:"cdn,omitempty"` - SecurityPolicy *SecurityPolicyConfig `json:"securityPolicy,omitempty"` - TimeoutSec *int64 `json:"timeoutSec,omitempty"` - ConnectionDraining *ConnectionDrainingConfig `json:"connectionDraining,omitempty"` - AffinityCookieTtlSec *int64 `json:"affinityCookieTtlSec,omitempty"` - SessionAffinity string `json:"sessionAffinity,omitempty"` + Iap *IAPConfig `json:"iap,omitempty"` + Cdn *CDNConfig `json:"cdn,omitempty"` + SecurityPolicy *SecurityPolicyConfig `json:"securityPolicy,omitempty"` + TimeoutSec *int64 `json:"timeoutSec,omitempty"` + ConnectionDraining *ConnectionDrainingConfig `json:"connectionDraining,omitempty"` + SessionAffinity *SessionAffinityConfig `json:"sessionAffinity,omitempty"` } // BackendConfigStatus is the status for a BackendConfig resource @@ -119,3 +118,10 @@ type ConnectionDrainingConfig struct { // Draining timeout in seconds. DrainingTimeoutSec *int64 `json:"drainingTimeoutSec,omitempty"` } + +// SessionAffinityConfig contains configuration for stickyness parameters. +// +k8s:openapi-gen=true +type SessionAffinityConfig struct { + AffinityType string `json:"affinityType,omitempty"` + AffinityCookieTtlSec *int64 `json:"affinityCookieTtlSec,omitempty"` +} diff --git a/pkg/apis/backendconfig/v1beta1/zz_generated.deepcopy.go b/pkg/apis/backendconfig/v1beta1/zz_generated.deepcopy.go index c4dcebef76..673238d840 100644 --- a/pkg/apis/backendconfig/v1beta1/zz_generated.deepcopy.go +++ b/pkg/apis/backendconfig/v1beta1/zz_generated.deepcopy.go @@ -133,13 +133,13 @@ func (in *BackendConfigSpec) DeepCopyInto(out *BackendConfigSpec) { (*in).DeepCopyInto(*out) } } - if in.AffinityCookieTtlSec != nil { - in, out := &in.AffinityCookieTtlSec, &out.AffinityCookieTtlSec + if in.SessionAffinity != nil { + in, out := &in.SessionAffinity, &out.SessionAffinity if *in == nil { *out = nil } else { - *out = new(int64) - **out = **in + *out = new(SessionAffinityConfig) + (*in).DeepCopyInto(*out) } } return @@ -303,3 +303,28 @@ func (in *SecurityPolicyConfig) DeepCopy() *SecurityPolicyConfig { in.DeepCopyInto(out) return out } + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *SessionAffinityConfig) DeepCopyInto(out *SessionAffinityConfig) { + *out = *in + if in.AffinityCookieTtlSec != nil { + in, out := &in.AffinityCookieTtlSec, &out.AffinityCookieTtlSec + if *in == nil { + *out = nil + } else { + *out = new(int64) + **out = **in + } + } + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new SessionAffinityConfig. +func (in *SessionAffinityConfig) DeepCopy() *SessionAffinityConfig { + if in == nil { + return nil + } + out := new(SessionAffinityConfig) + in.DeepCopyInto(out) + return out +} diff --git a/pkg/apis/backendconfig/v1beta1/zz_generated.openapi.go b/pkg/apis/backendconfig/v1beta1/zz_generated.openapi.go index 70087cad2d..ea4551b2f8 100644 --- a/pkg/apis/backendconfig/v1beta1/zz_generated.openapi.go +++ b/pkg/apis/backendconfig/v1beta1/zz_generated.openapi.go @@ -99,23 +99,16 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA Ref: ref("k8s.io/ingress-gce/pkg/apis/backendconfig/v1beta1.ConnectionDrainingConfig"), }, }, - "affinityCookieTtlSec": { - SchemaProps: spec.SchemaProps{ - Type: []string{"integer"}, - Format: "int64", - }, - }, "sessionAffinity": { SchemaProps: spec.SchemaProps{ - Type: []string{"string"}, - Format: "", + Ref: ref("k8s.io/ingress-gce/pkg/apis/backendconfig/v1beta1.SessionAffinityConfig"), }, }, }, }, }, Dependencies: []string{ - "k8s.io/ingress-gce/pkg/apis/backendconfig/v1beta1.CDNConfig", "k8s.io/ingress-gce/pkg/apis/backendconfig/v1beta1.ConnectionDrainingConfig", "k8s.io/ingress-gce/pkg/apis/backendconfig/v1beta1.IAPConfig", "k8s.io/ingress-gce/pkg/apis/backendconfig/v1beta1.SecurityPolicyConfig"}, + "k8s.io/ingress-gce/pkg/apis/backendconfig/v1beta1.CDNConfig", "k8s.io/ingress-gce/pkg/apis/backendconfig/v1beta1.ConnectionDrainingConfig", "k8s.io/ingress-gce/pkg/apis/backendconfig/v1beta1.IAPConfig", "k8s.io/ingress-gce/pkg/apis/backendconfig/v1beta1.SecurityPolicyConfig", "k8s.io/ingress-gce/pkg/apis/backendconfig/v1beta1.SessionAffinityConfig"}, }, "k8s.io/ingress-gce/pkg/apis/backendconfig/v1beta1.CDNConfig": { Schema: spec.Schema{ @@ -271,5 +264,27 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA }, Dependencies: []string{}, }, + "k8s.io/ingress-gce/pkg/apis/backendconfig/v1beta1.SessionAffinityConfig": { + Schema: spec.Schema{ + SchemaProps: spec.SchemaProps{ + Description: "SessionAffinityConfig contains configuration for stickyness parameters.", + Properties: map[string]spec.Schema{ + "affinityType": { + SchemaProps: spec.SchemaProps{ + Type: []string{"string"}, + Format: "", + }, + }, + "affinityCookieTtlSec": { + SchemaProps: spec.SchemaProps{ + Type: []string{"integer"}, + Format: "int64", + }, + }, + }, + }, + }, + Dependencies: []string{}, + }, } } diff --git a/pkg/backendconfig/validation.go b/pkg/backendconfig/validation.go index 4c2da79391..11fd8de759 100644 --- a/pkg/backendconfig/validation.go +++ b/pkg/backendconfig/validation.go @@ -29,11 +29,26 @@ const ( OAuthClientSecretKey = "client_secret" ) +var supportedAffinities = map[string]bool{ + "NONE": true, + "CLIENT_IP": true, + "GENERATED_COOKIE": true, +} + func Validate(kubeClient kubernetes.Interface, beConfig *backendconfigv1beta1.BackendConfig) error { if beConfig == nil { return nil } - return validateIAP(kubeClient, beConfig) + + if err := validateIAP(kubeClient, beConfig); err != nil { + return err + } + + if err := validateSessionAffinity(kubeClient, beConfig); err != nil { + return err + } + + return nil } // TODO(rramkumar): Return errors as constants so that the unit tests can distinguish @@ -67,3 +82,25 @@ func validateIAP(kubeClient kubernetes.Interface, beConfig *backendconfigv1beta1 } return nil } + +func validateSessionAffinity(kubeClient kubernetes.Interface, beConfig *backendconfigv1beta1.BackendConfig) error { + if beConfig.Spec.SessionAffinity == nil { + return nil + } + + if beConfig.Spec.SessionAffinity.AffinityType != "" { + if _, ok := supportedAffinities[beConfig.Spec.SessionAffinity.AffinityType]; !ok { + return fmt.Errorf("unsupported AffinityType: %s, sould be one of NONE, CLIENT_IP, or GENERATED_COOKIE", + beConfig.Spec.SessionAffinity.AffinityType) + } + } + + if beConfig.Spec.SessionAffinity.AffinityCookieTtlSec != nil { + if *beConfig.Spec.SessionAffinity.AffinityCookieTtlSec < 0 || *beConfig.Spec.SessionAffinity.AffinityCookieTtlSec > 86400 { + return fmt.Errorf("unsupported AffinityCookieTtlSec: %d, should be comprised between 0 and 86400", + *beConfig.Spec.SessionAffinity.AffinityCookieTtlSec) + } + } + + return nil +} diff --git a/pkg/backends/features/affinity.go b/pkg/backends/features/affinity.go index c3d7ebc3de..eff256744c 100644 --- a/pkg/backends/features/affinity.go +++ b/pkg/backends/features/affinity.go @@ -31,17 +31,20 @@ import ( func EnsureAffinity(sp utils.ServicePort, be *composite.BackendService) bool { changed := false - // Should we check if specified SessionAffinity is among the currently GCP supported values? - // For now let's forward as is to GCP, to inherit API error message (and evolutions). - // Same for TTL (should be in 0-86400 range). - if sp.BackendConfig.Spec.SessionAffinity != "" && strings.Compare(sp.BackendConfig.Spec.SessionAffinity, be.SessionAffinity) != 0 { - be.SessionAffinity = sp.BackendConfig.Spec.SessionAffinity + if sp.BackendConfig.Spec.SessionAffinity == nil { + return false + } + + if sp.BackendConfig.Spec.SessionAffinity.AffinityType != "" && + strings.Compare(sp.BackendConfig.Spec.SessionAffinity.AffinityType, be.SessionAffinity) != 0 { + be.SessionAffinity = sp.BackendConfig.Spec.SessionAffinity.AffinityType glog.V(2).Infof("Updated SessionAffinity settings for service %v/%v.", sp.ID.Service.Namespace, sp.ID.Service.Name) changed = true } - if sp.BackendConfig.Spec.AffinityCookieTtlSec != nil && *sp.BackendConfig.Spec.AffinityCookieTtlSec != be.AffinityCookieTtlSec { - be.AffinityCookieTtlSec = *sp.BackendConfig.Spec.AffinityCookieTtlSec + if sp.BackendConfig.Spec.SessionAffinity.AffinityCookieTtlSec != nil && + *sp.BackendConfig.Spec.SessionAffinity.AffinityCookieTtlSec != be.AffinityCookieTtlSec { + be.AffinityCookieTtlSec = *sp.BackendConfig.Spec.SessionAffinity.AffinityCookieTtlSec glog.V(2).Infof("Updated AffinityCookieTtlSec settings for service %v/%v.", sp.ID.Service.Namespace, sp.ID.Service.Name) changed = true } diff --git a/pkg/backends/features/affinity_test.go b/pkg/backends/features/affinity_test.go index 3722fdd8eb..ba34ec2343 100644 --- a/pkg/backends/features/affinity_test.go +++ b/pkg/backends/features/affinity_test.go @@ -51,7 +51,9 @@ func TestEnsureAffinity(t *testing.T) { sp: utils.ServicePort{ BackendConfig: &backendconfigv1beta1.BackendConfig{ Spec: backendconfigv1beta1.BackendConfigSpec{ - SessionAffinity: "CLIENT_IP", + SessionAffinity: &backendconfigv1beta1.SessionAffinityConfig{ + AffinityType: "CLIENT_IP", + }, }, }, }, @@ -65,7 +67,9 @@ func TestEnsureAffinity(t *testing.T) { sp: utils.ServicePort{ BackendConfig: &backendconfigv1beta1.BackendConfig{ Spec: backendconfigv1beta1.BackendConfigSpec{ - AffinityCookieTtlSec: &testTTL, + SessionAffinity: &backendconfigv1beta1.SessionAffinityConfig{ + AffinityCookieTtlSec: &testTTL, + }, }, }, }, @@ -79,8 +83,10 @@ func TestEnsureAffinity(t *testing.T) { sp: utils.ServicePort{ BackendConfig: &backendconfigv1beta1.BackendConfig{ Spec: backendconfigv1beta1.BackendConfigSpec{ - SessionAffinity: "CLIENT_IP", - AffinityCookieTtlSec: &testTTL, + SessionAffinity: &backendconfigv1beta1.SessionAffinityConfig{ + AffinityType: "CLIENT_IP", + AffinityCookieTtlSec: &testTTL, + }, }, }, }, @@ -95,8 +101,10 @@ func TestEnsureAffinity(t *testing.T) { sp: utils.ServicePort{ BackendConfig: &backendconfigv1beta1.BackendConfig{ Spec: backendconfigv1beta1.BackendConfigSpec{ - SessionAffinity: "CLIENT_IP", - AffinityCookieTtlSec: &testTTL, + SessionAffinity: &backendconfigv1beta1.SessionAffinityConfig{ + AffinityType: "CLIENT_IP", + AffinityCookieTtlSec: &testTTL, + }, }, }, }, From be69edef012c6cc39b265de5fd1a0de56f14a739 Mon Sep 17 00:00:00 2001 From: Benjamin Pineau Date: Thu, 25 Oct 2018 22:26:46 +0200 Subject: [PATCH 3/4] Make DrainingTimeoutSec a plain int64 --- pkg/apis/backendconfig/v1beta1/types.go | 2 +- .../backendconfig/v1beta1/zz_generated.deepcopy.go | 11 +---------- pkg/backends/features/draining.go | 4 +--- pkg/backends/features/draining_test.go | 8 +++----- 4 files changed, 6 insertions(+), 19 deletions(-) diff --git a/pkg/apis/backendconfig/v1beta1/types.go b/pkg/apis/backendconfig/v1beta1/types.go index ceac78fc36..1e52eac12b 100644 --- a/pkg/apis/backendconfig/v1beta1/types.go +++ b/pkg/apis/backendconfig/v1beta1/types.go @@ -116,7 +116,7 @@ type SecurityPolicyConfig struct { // +k8s:openapi-gen=true type ConnectionDrainingConfig struct { // Draining timeout in seconds. - DrainingTimeoutSec *int64 `json:"drainingTimeoutSec,omitempty"` + DrainingTimeoutSec int64 `json:"drainingTimeoutSec,omitempty"` } // SessionAffinityConfig contains configuration for stickyness parameters. diff --git a/pkg/apis/backendconfig/v1beta1/zz_generated.deepcopy.go b/pkg/apis/backendconfig/v1beta1/zz_generated.deepcopy.go index 673238d840..eacd389b93 100644 --- a/pkg/apis/backendconfig/v1beta1/zz_generated.deepcopy.go +++ b/pkg/apis/backendconfig/v1beta1/zz_generated.deepcopy.go @@ -130,7 +130,7 @@ func (in *BackendConfigSpec) DeepCopyInto(out *BackendConfigSpec) { *out = nil } else { *out = new(ConnectionDrainingConfig) - (*in).DeepCopyInto(*out) + **out = **in } } if in.SessionAffinity != nil { @@ -225,15 +225,6 @@ func (in *CacheKeyPolicy) DeepCopy() *CacheKeyPolicy { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ConnectionDrainingConfig) DeepCopyInto(out *ConnectionDrainingConfig) { *out = *in - if in.DrainingTimeoutSec != nil { - in, out := &in.DrainingTimeoutSec, &out.DrainingTimeoutSec - if *in == nil { - *out = nil - } else { - *out = new(int64) - **out = **in - } - } return } diff --git a/pkg/backends/features/draining.go b/pkg/backends/features/draining.go index b023c66dea..a61e9525f3 100644 --- a/pkg/backends/features/draining.go +++ b/pkg/backends/features/draining.go @@ -47,7 +47,5 @@ func EnsureDraining(sp utils.ServicePort, be *composite.BackendService) bool { // to be made to actually persist the changes. func applyDrainingSettings(sp utils.ServicePort, be *composite.BackendService) { be.ConnectionDraining = &composite.ConnectionDraining{} - if sp.BackendConfig.Spec.ConnectionDraining.DrainingTimeoutSec != nil { - be.ConnectionDraining.DrainingTimeoutSec = *sp.BackendConfig.Spec.ConnectionDraining.DrainingTimeoutSec - } + be.ConnectionDraining.DrainingTimeoutSec = sp.BackendConfig.Spec.ConnectionDraining.DrainingTimeoutSec } diff --git a/pkg/backends/features/draining_test.go b/pkg/backends/features/draining_test.go index 563811acb1..c71eda1ed6 100644 --- a/pkg/backends/features/draining_test.go +++ b/pkg/backends/features/draining_test.go @@ -24,8 +24,6 @@ import ( "k8s.io/ingress-gce/pkg/utils" ) -var testPort = int64(111) - func TestEnsureDraining(t *testing.T) { testCases := []struct { desc string @@ -39,7 +37,7 @@ func TestEnsureDraining(t *testing.T) { BackendConfig: &backendconfigv1beta1.BackendConfig{ Spec: backendconfigv1beta1.BackendConfigSpec{ ConnectionDraining: &backendconfigv1beta1.ConnectionDrainingConfig{ - DrainingTimeoutSec: &testPort, + DrainingTimeoutSec: 111, }, }, }, @@ -69,7 +67,7 @@ func TestEnsureDraining(t *testing.T) { BackendConfig: &backendconfigv1beta1.BackendConfig{ Spec: backendconfigv1beta1.BackendConfigSpec{ ConnectionDraining: &backendconfigv1beta1.ConnectionDrainingConfig{ - DrainingTimeoutSec: &testPort, + DrainingTimeoutSec: 111, }, }, }, @@ -87,7 +85,7 @@ func TestEnsureDraining(t *testing.T) { BackendConfig: &backendconfigv1beta1.BackendConfig{ Spec: backendconfigv1beta1.BackendConfigSpec{ ConnectionDraining: &backendconfigv1beta1.ConnectionDrainingConfig{ - DrainingTimeoutSec: &testPort, + DrainingTimeoutSec: 111, }, }, }, From 8bcada0411872987ee57151cf7541476a5d46668 Mon Sep 17 00:00:00 2001 From: Benjamin Pineau Date: Fri, 26 Oct 2018 00:53:49 +0200 Subject: [PATCH 4/4] validateSessionAffinity unit test + review feedback --- pkg/backendconfig/validation.go | 4 +- pkg/backendconfig/validation_test.go | 80 ++++++++++++++++++++++++++++ pkg/backends/features/affinity.go | 28 +++++----- 3 files changed, 96 insertions(+), 16 deletions(-) diff --git a/pkg/backendconfig/validation.go b/pkg/backendconfig/validation.go index 11fd8de759..d6fde86b97 100644 --- a/pkg/backendconfig/validation.go +++ b/pkg/backendconfig/validation.go @@ -90,14 +90,14 @@ func validateSessionAffinity(kubeClient kubernetes.Interface, beConfig *backendc if beConfig.Spec.SessionAffinity.AffinityType != "" { if _, ok := supportedAffinities[beConfig.Spec.SessionAffinity.AffinityType]; !ok { - return fmt.Errorf("unsupported AffinityType: %s, sould be one of NONE, CLIENT_IP, or GENERATED_COOKIE", + return fmt.Errorf("unsupported AffinityType: %s, should be one of NONE, CLIENT_IP, or GENERATED_COOKIE", beConfig.Spec.SessionAffinity.AffinityType) } } if beConfig.Spec.SessionAffinity.AffinityCookieTtlSec != nil { if *beConfig.Spec.SessionAffinity.AffinityCookieTtlSec < 0 || *beConfig.Spec.SessionAffinity.AffinityCookieTtlSec > 86400 { - return fmt.Errorf("unsupported AffinityCookieTtlSec: %d, should be comprised between 0 and 86400", + return fmt.Errorf("unsupported AffinityCookieTtlSec: %d, should be between 0 and 86400", *beConfig.Spec.SessionAffinity.AffinityCookieTtlSec) } } diff --git a/pkg/backendconfig/validation_test.go b/pkg/backendconfig/validation_test.go index e8d92f64bf..196ff61c0c 100644 --- a/pkg/backendconfig/validation_test.go +++ b/pkg/backendconfig/validation_test.go @@ -27,6 +27,9 @@ import ( ) var ( + goodTTL int64 = 86400 + badTTL int64 = 86400 + 1 + defaultBeConfig = &backendconfigv1beta1.BackendConfig{ ObjectMeta: meta_v1.ObjectMeta{ Namespace: "default", @@ -159,3 +162,80 @@ func TestValidateIAP(t *testing.T) { } } } + +func TestValidateSessionAffinity(t *testing.T) { + testCases := []struct { + desc string + beConfig *backendconfigv1beta1.BackendConfig + expectError bool + }{ + + { + desc: "unsupported affinity type", + beConfig: &backendconfigv1beta1.BackendConfig{ + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "default", + }, + Spec: backendconfigv1beta1.BackendConfigSpec{ + SessionAffinity: &backendconfigv1beta1.SessionAffinityConfig{ + AffinityType: "WRONG_TYPE", + }, + }, + }, + expectError: true, + }, + { + desc: "supported affinity type", + beConfig: &backendconfigv1beta1.BackendConfig{ + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "default", + }, + Spec: backendconfigv1beta1.BackendConfigSpec{ + SessionAffinity: &backendconfigv1beta1.SessionAffinityConfig{ + AffinityType: "CLIENT_IP", + }, + }, + }, + expectError: false, + }, + { + desc: "unsupported ttl value", + beConfig: &backendconfigv1beta1.BackendConfig{ + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "default", + }, + Spec: backendconfigv1beta1.BackendConfigSpec{ + SessionAffinity: &backendconfigv1beta1.SessionAffinityConfig{ + AffinityCookieTtlSec: &badTTL, + }, + }, + }, + expectError: true, + }, + { + desc: "supported ttl value", + beConfig: &backendconfigv1beta1.BackendConfig{ + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "default", + }, + Spec: backendconfigv1beta1.BackendConfigSpec{ + SessionAffinity: &backendconfigv1beta1.SessionAffinityConfig{ + AffinityCookieTtlSec: &goodTTL, + }, + }, + }, + expectError: false, + }, + } + + for _, testCase := range testCases { + kubeClient := fake.NewSimpleClientset() + err := Validate(kubeClient, testCase.beConfig) + if testCase.expectError && err == nil { + t.Errorf("%v: Expected error but got nil", testCase.desc) + } + if !testCase.expectError && err != nil { + t.Errorf("%v: Did not expect error but got: %v", testCase.desc, err) + } + } +} diff --git a/pkg/backends/features/affinity.go b/pkg/backends/features/affinity.go index eff256744c..21c20636f5 100644 --- a/pkg/backends/features/affinity.go +++ b/pkg/backends/features/affinity.go @@ -17,7 +17,7 @@ limitations under the License. package features import ( - "strings" + "reflect" "github.com/golang/glog" "k8s.io/ingress-gce/pkg/composite" @@ -29,25 +29,25 @@ import ( // It returns true if there were existing settings on the BackendService // that were overwritten. func EnsureAffinity(sp utils.ServicePort, be *composite.BackendService) bool { - changed := false - if sp.BackendConfig.Spec.SessionAffinity == nil { return false } - - if sp.BackendConfig.Spec.SessionAffinity.AffinityType != "" && - strings.Compare(sp.BackendConfig.Spec.SessionAffinity.AffinityType, be.SessionAffinity) != 0 { - be.SessionAffinity = sp.BackendConfig.Spec.SessionAffinity.AffinityType + beTemp := &composite.BackendService{} + applyAffinitySettings(sp, beTemp) + if !reflect.DeepEqual(beTemp.AffinityCookieTtlSec, be.AffinityCookieTtlSec) || beTemp.SessionAffinity != be.SessionAffinity { + applyAffinitySettings(sp, be) glog.V(2).Infof("Updated SessionAffinity settings for service %v/%v.", sp.ID.Service.Namespace, sp.ID.Service.Name) - changed = true + return true } + return false +} - if sp.BackendConfig.Spec.SessionAffinity.AffinityCookieTtlSec != nil && - *sp.BackendConfig.Spec.SessionAffinity.AffinityCookieTtlSec != be.AffinityCookieTtlSec { +// applyAffinitySettings applies the session affinity settings specified in the +// BackendConfig to the passed in composite.BackendService. A GCE API call still +// needs to be made to actually persist the changes. +func applyAffinitySettings(sp utils.ServicePort, be *composite.BackendService) { + be.SessionAffinity = sp.BackendConfig.Spec.SessionAffinity.AffinityType + if sp.BackendConfig.Spec.SessionAffinity.AffinityCookieTtlSec != nil { be.AffinityCookieTtlSec = *sp.BackendConfig.Spec.SessionAffinity.AffinityCookieTtlSec - glog.V(2).Infof("Updated AffinityCookieTtlSec settings for service %v/%v.", sp.ID.Service.Namespace, sp.ID.Service.Name) - changed = true } - - return changed }