Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Nginx sticky annotations #258

Merged
merged 8 commits into from
Feb 16, 2017
Merged

Nginx sticky annotations #258

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1dbe65e
Initial support for sticky config in annotations
Feb 10, 2017
79e186c
New sticky session configuration
Feb 10, 2017
6809319
Adds support for configuring stickness per Ingress
Feb 10, 2017
a158e5f
Improve the session affinity feature
Feb 12, 2017
0161ae4
Improve in documentation and naming case
Feb 14, 2017
b06ead1
Corrects the affinity test
Feb 15, 2017
e5c9c78
Correct the configuration.md reference to annotations
Feb 16, 2017
4be502e
Adds a new situation in affinity config example
Feb 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 15 additions & 15 deletions core/pkg/ingress/annotations/sessionaffinity/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ const (
// If a cookie with this name exists,
// its value is used as an index into the list of available backends.
annotationAffinityCookieName = "ingress.kubernetes.io/session-cookie-name"
defaultAffinityCookieName = "route"
defaultAffinityCookieName = "INGRESSCOOKIE"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a naming convention we can follow for HTTP headers? ALL CAPS, or first Cap, or X-Blah, would be great if you could dig up some doc or rfc and reference it here, but otherwise just a simple comment about the CAPS is ok

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bprashanth I've got this, but no reference to sticky / cookie session:

https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html

I've seen in there also that it's common to use something like CamelCase (IngressCookie) as an example.

Headers containing X- are going to be deprecated, as the following RFC:

https://tools.ietf.org/html/rfc6648

So we can keep this as all caps by now (as does Google), and reopen this discussion later?

The RFC section that deals with Requests and Responses Headers is the following: https://tools.ietf.org/html/rfc2616#section-5.3

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, whatever makes sense. Something like this will be hard to change without a deprecation notice, because people will probably have clients written to it, but I suppose they can always set the cookie name explicitly back to INGRESSCOOKIE via annotation.
Google uses all caps? for what?

// This is the algorithm used by nginx to generate a value for the session cookie, if
// one isn't supplied and affintiy is set to "cookie".
annotationAffinityCookieHash = "ingress.kubernetes.io/session-cookie-hash"
Expand All @@ -45,24 +45,21 @@ var (
// AffinityConfig describes the per ingress session affinity config
type AffinityConfig struct {
// The type of affinity that will be used
AffinityType string `json:"type"`
CookieAffinityConfig CookieAffinityConfig `json:"cookieconfig"`
AffinityType string `json:"type"`
CookieConfig
}

// CookieAffinityConfig describes the Config of cookie type affinity
type CookieAffinityConfig struct {
// CookieConfig describes the Config of cookie type affinity
type CookieConfig struct {
// The name of the cookie that will be used in case of cookie affinity type.
Name string `json:"name"`
// The hash that will be used to encode the cookie in case of cookie affinity type
Hash string `json:"hash"`
}

type affinity struct {
}

// CookieAffinityParse gets the annotation values related to Cookie Affinity
// It also sets default values when no value or incorrect value is found
func CookieAffinityParse(ing *extensions.Ingress) *CookieAffinityConfig {
func CookieAffinityParse(ing *extensions.Ingress) *CookieConfig {

sn, err := parser.GetStringAnnotation(annotationAffinityCookieName, ing)

Expand All @@ -78,7 +75,7 @@ func CookieAffinityParse(ing *extensions.Ingress) *CookieAffinityConfig {
sh = defaultAffinityCookieHash
}

return &CookieAffinityConfig{
return &CookieConfig{
Name: sn,
Hash: sh,
}
Expand All @@ -89,19 +86,22 @@ func NewParser() parser.IngressAnnotation {
return affinity{}
}

type affinity struct {
}

// ParseAnnotations parses the annotations contained in the ingress
// rule used to configure the affinity directives
func (a affinity) Parse(ing *extensions.Ingress) (interface{}, error) {

var cookieAffinityConfig *CookieAffinityConfig
cookieAffinityConfig = &CookieAffinityConfig{}
var cookieAffinityConfig *CookieConfig
cookieAffinityConfig = &CookieConfig{}

// Check the type of affinity that will be used
at, err := parser.GetStringAnnotation(annotationAffinityType, ing)
if err != nil {
at = ""
}
//cookieAffinityConfig = CookieAffinityParse(ing)

switch at {
case "cookie":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other places we just check for non-nil: https://github.com/kubernetes/kubernetes/blob/master/pkg/api/validation/validation.go#L346, but I'm fine with the way you have it right now.

However, please define the string "cookie" as a const, i.e somewhere, probably types.go, put

type Affinity string

const (
  Cookie Affinity = "cookie"
  Client Affinity = "client"
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bprashanth you mean a new types.go inside the 'sessionaffinity' directory where we can put every constant, or directly in 'core/pkg/ingress/types.go' ?

cookieAffinityConfig = CookieAffinityParse(ing)
Expand All @@ -111,8 +111,8 @@ func (a affinity) Parse(ing *extensions.Ingress) (interface{}, error) {

}
return &AffinityConfig{
AffinityType: at,
CookieAffinityConfig: *cookieAffinityConfig,
AffinityType: at,
CookieConfig: *cookieAffinityConfig,
}, nil

}
4 changes: 2 additions & 2 deletions core/pkg/ingress/annotations/sessionaffinity/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func TestIngressAffinityCookieConfig(t *testing.T) {
data := map[string]string{}
data[annotationAffinityType] = "cookie"
data[annotationAffinityCookieHash] = "sha123"
data[annotationAffinityCookieName] = "route"
data[annotationAffinityCookieName] = "INGRESSCOOKIE"
ing.SetAnnotations(data)

affin, _ := NewParser().Parse(ing)
Expand All @@ -82,7 +82,7 @@ func TestIngressAffinityCookieConfig(t *testing.T) {
t.Errorf("expected md5 as sticky-hash but returned %v", nginxAffinity.CookieAffinityConfig.Hash)
}

if nginxAffinity.CookieAffinityConfig.Name != "route" {
if nginxAffinity.CookieAffinityConfig.Name != "INGRESSCOOKIE" {
t.Errorf("expected route as sticky-name but returned %v", nginxAffinity.CookieAffinityConfig.Name)
}
}
10 changes: 5 additions & 5 deletions core/pkg/ingress/controller/annotations_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ func TestAffinitySession(t *testing.T) {
}{
{map[string]string{annotationAffinityType: "cookie", annotationAffinityCookieHash: "md5", annotationAffinityCookieName: "route"}, "cookie", "md5", "route"},
{map[string]string{annotationAffinityType: "cookie", annotationAffinityCookieHash: "xpto", annotationAffinityCookieName: "route1"}, "cookie", "md5", "route1"},
{map[string]string{annotationAffinityType: "cookie", annotationAffinityCookieHash: "", annotationAffinityCookieName: ""}, "cookie", "md5", "route"},
{map[string]string{annotationAffinityType: "cookie", annotationAffinityCookieHash: "", annotationAffinityCookieName: ""}, "cookie", "md5", "INGRESSCOOKIE"},
{map[string]string{}, "", "", ""},
{nil, "", "", ""},
}
Expand All @@ -209,12 +209,12 @@ func TestAffinitySession(t *testing.T) {
continue
}

if r.CookieAffinityConfig.Hash != foo.hash {
t.Errorf("Returned %v but expected %v for Hash", r.CookieAffinityConfig.Hash, foo.hash)
if r.CookieConfig.Hash != foo.hash {
t.Errorf("Returned %v but expected %v for Hash", r.CookieConfig.Hash, foo.hash)
}

if r.CookieAffinityConfig.Name != foo.name {
t.Errorf("Returned %v but expected %v for Name", r.CookieAffinityConfig.Name, foo.name)
if r.CookieConfig.Name != foo.name {
t.Errorf("Returned %v but expected %v for Name", r.CookieConfig.Name, foo.name)
}
}
}
4 changes: 2 additions & 2 deletions core/pkg/ingress/controller/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -743,8 +743,8 @@ func (ic *GenericController) createUpstreams(data []interface{}) map[string]*ing
if upstreams[name].SessionAffinity.AffinityType == "" {
upstreams[name].SessionAffinity.AffinityType = affinity.AffinityType
if affinity.AffinityType == "cookie" {
upstreams[name].SessionAffinity.CookieSessionAffinity.Name = affinity.CookieAffinityConfig.Name
upstreams[name].SessionAffinity.CookieSessionAffinity.Hash = affinity.CookieAffinityConfig.Hash
upstreams[name].SessionAffinity.CookieSessionAffinity.Name = affinity.CookieConfig.Name
upstreams[name].SessionAffinity.CookieSessionAffinity.Hash = affinity.CookieConfig.Hash
}
}

Expand Down
1 change: 1 addition & 0 deletions examples/affinity/cookie/nginx/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ Using a deployment with only one replica doesn't set the 'sticky' cookie.
Session stickyness is achieved through 3 annotations on the Ingress, as shown in the [example](sticky-ingress.yaml).

|Name|Description|Values|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't showing up as a table, I guess you need a little more formatting: https://help.github.com/articles/organizing-information-with-tables/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, corrected it

| --- | --- | --- |
|ingress.kubernetes.io/affinity|Sets the affinity type|string (in NGINX only ``cookie`` is possible|
|ingress.kubernetes.io/session-cookie-name|Name of the cookie that will be used|string (default to route)|
|ingress.kubernetes.io/session-cookie-hash|Type of hash that will be used in cookie value|sha1/md5/index|
Expand Down