Merge pull request #3324 from Shopify/fix-sticky-session

Fix sticky session
kubernetes · Oct 30, 2018 · 01df84d · 01df84d
2 parents bec97b3 + 9e639f9
commit 01df84d
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 54 deletions.
diff --git a/rootfs/etc/nginx/lua/balancer/sticky.lua b/rootfs/etc/nginx/lua/balancer/sticky.lua
@@ -50,22 +50,17 @@ local function set_cookie(self, value)
   end
 end
 
-local function pick_random(instance)
-  local index = math.random(instance.npoints)
-  return instance:next(index)
-end
-
 function _M.balance(self)
   local cookie, err = ck:new()
   if not cookie then
-    ngx.log(ngx.ERR, err)
-    return pick_random(self.instance)
+    ngx.log(ngx.ERR, "error while initializing cookie: " .. tostring(err))
+    return
   end
 
   local key = cookie:get(self.cookie_name)
   if not key then
-    local tmp_endpoint_string = pick_random(self.instance)
-    key = encrypted_endpoint_string(self, tmp_endpoint_string)
+    local random_str = string.format("%s.%s", ngx.now(), ngx.worker.pid())
+    key = encrypted_endpoint_string(self, random_str)
     set_cookie(self, key)
   end
 

diff --git a/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua b/rootfs/etc/nginx/lua/test/balancer/sticky_test.lua
@@ -116,7 +116,8 @@ describe("Sticky", function()
           local cookie_instance = {
             set = function(self, payload)
               assert.equal(payload.key, test_backend.sessionAffinityConfig.cookieSessionAffinity.name)
-              assert.equal(payload.value, util[test_backend_hash_fn .. "_digest"](test_backend_endpoint))
+              local expected_len = #util[test_backend_hash_fn .. "_digest"]("anything")
+              assert.equal(#payload.value, expected_len)
               assert.equal(payload.path, ngx.var.location_path)
               assert.equal(payload.domain, nil)
               assert.equal(payload.httponly, true)

diff --git a/test/e2e/annotations/affinity.go b/test/e2e/annotations/affinity.go
@@ -16,22 +16,27 @@ limitations under the License.
 
 package annotations
 
-/*
 import (
+	"fmt"
+	"net/http"
+	"strings"
+
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
-)
+	"github.com/parnurzeal/gorequest"
 
+	"k8s.io/api/extensions/v1beta1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/intstr"
+
+	"k8s.io/ingress-nginx/test/e2e/framework"
+)
 
-// TODO(elvinefendi) merge this with Affinity tests in test/e2e/lua/dynamic_configuration.go
-var _ = framework.IngressNginxDescribe("Annotations - Affinity", func() {
+var _ = framework.IngressNginxDescribe("Annotations - Affinity/Sticky Sessions", func() {
 	f := framework.NewDefaultFramework("affinity")
 
 	BeforeEach(func() {
-		err := f.DisableDynamicConfiguration()
-		Expect(err).NotTo(HaveOccurred())
-
-		err = f.NewEchoDeploymentWithReplicas(2)
+		err := f.NewEchoDeploymentWithReplicas(2)
 		Expect(err).NotTo(HaveOccurred())
 	})
 
@@ -53,7 +58,7 @@ var _ = framework.IngressNginxDescribe("Annotations - Affinity", func() {
 
 		err = f.WaitForNginxServer(host,
 			func(server string) bool {
-				return strings.Contains(server, "proxy_pass http://sticky-"+f.IngressController.Namespace+"-http-svc-80;")
+				return strings.Contains(server, fmt.Sprintf("server_name %s ;", host))
 			})
 		Expect(err).NotTo(HaveOccurred())
 
@@ -67,36 +72,6 @@ var _ = framework.IngressNginxDescribe("Annotations - Affinity", func() {
 		Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring("SERVERID="))
 	})
 
-	It("should redirect to '/something' with enabled affinity", func() {
-		host := "example.com"
-		annotations := map[string]string{
-			"nginx.ingress.kubernetes.io/affinity":            "cookie",
-			"nginx.ingress.kubernetes.io/session-cookie-name": "SERVERID",
-		}
-
-		ing := framework.NewSingleIngress(host, "/", host, f.IngressController.Namespace, "http-svc", 80, &annotations)
-		_, err := f.EnsureIngress(ing)
-
-		Expect(err).NotTo(HaveOccurred())
-		Expect(ing).NotTo(BeNil())
-
-		err = f.WaitForNginxServer(host,
-			func(server string) bool {
-				return strings.Contains(server, "proxy_pass http://sticky-"+f.IngressController.Namespace+"-http-svc-80;")
-			})
-		Expect(err).NotTo(HaveOccurred())
-
-		resp, body, errs := gorequest.New().
-			Get(f.IngressController.HTTPURL).
-			Set("Host", host).
-			End()
-
-		Expect(len(errs)).Should(BeNumerically("==", 0))
-		Expect(resp.StatusCode).Should(Equal(http.StatusOK))
-		Expect(body).Should(ContainSubstring(fmt.Sprintf("request_uri=http://%v:8080/", host)))
-		Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring("SERVERID="))
-	})
-
 	It("should set the path to /something on the generated cookie", func() {
 		host := "example.com"
 		annotations := map[string]string{
@@ -112,7 +87,7 @@ var _ = framework.IngressNginxDescribe("Annotations - Affinity", func() {
 
 		err = f.WaitForNginxServer(host,
 			func(server string) bool {
-				return strings.Contains(server, "proxy_pass http://sticky-"+f.IngressController.Namespace+"-http-svc-80;")
+				return strings.Contains(server, fmt.Sprintf("server_name %s ;", host))
 			})
 		Expect(err).NotTo(HaveOccurred())
 
@@ -126,7 +101,7 @@ var _ = framework.IngressNginxDescribe("Annotations - Affinity", func() {
 		Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring("Path=/something"))
 	})
 
-	It("should set the path to / on the generated cookie if there's more than one rule referring to the same backend", func() {
+	It("does not set the path to / on the generated cookie if there's more than one rule referring to the same backend", func() {
 		host := "example.com"
 		annotations := map[string]string{
 			"nginx.ingress.kubernetes.io/affinity":            "cookie",
@@ -173,7 +148,7 @@ var _ = framework.IngressNginxDescribe("Annotations - Affinity", func() {
 
 		err = f.WaitForNginxServer(host,
 			func(server string) bool {
-				return strings.Contains(server, "proxy_pass http://sticky-"+f.IngressController.Namespace+"-http-svc-80;")
+				return strings.Contains(server, fmt.Sprintf("server_name %s ;", host))
 			})
 		Expect(err).NotTo(HaveOccurred())
 
@@ -184,7 +159,15 @@ var _ = framework.IngressNginxDescribe("Annotations - Affinity", func() {
 
 		Expect(len(errs)).Should(BeNumerically("==", 0))
 		Expect(resp.StatusCode).Should(Equal(http.StatusOK))
-		Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring("Path=/;"))
+		Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring("Path=/something;"))
+
+		resp, _, errs = gorequest.New().
+			Get(f.IngressController.HTTPURL+"/somewhereelese").
+			Set("Host", host).
+			End()
+
+		Expect(len(errs)).Should(BeNumerically("==", 0))
+		Expect(resp.StatusCode).Should(Equal(http.StatusOK))
+		Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring("Path=/somewhereelese;"))
 	})
 })
-*/