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.

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

Add canary annotation and alternative backends for traffic shaping #3341

Merged
merged 1 commit into from
Nov 6, 2018

Conversation

clandry94
Copy link

@clandry94 clandry94 commented Nov 1, 2018

What this PR does / why we need it:

Adds support for multi-service routing per ingress by means of alternative backends. It also implements an interface to create these alternative backends through the form of canary annotations.

Some terms

AlternativeBackend - An alternative backend is a normal ingress-nginx backend with the condition that it is not backed through the typical 1:1 backend-server relationship. Instead, alternative backends share a server with another real backend.

Primary Backend - This is the same as a backend. I prefix with the word primary to have a clearer distinction between it and a alternative backend.

How it works

Canary

As described in the linked issue above, the following annotation

nginx.ingress.kubernetes.io/canary: true

Combined with a combination of the below annotations, we can define the behaviour of the canary.

The canary will define a canary-weight. If only weight is specified, then n% of traffic will be shifted to the canary backend. Additionally, if canary-by-cookie or canary-by-header are set, they will shift all traffic which match the cookie/header set with a value of always to the canary backend. If no value is set for the header, it will follow the rules of canary-weight.

nginx.ingress.kubernetes.io/canary-by-cookie: <cookie name>
nginx.ingress.kubernetes.io/canary-by-header: <header name>
nginx.ingress.kubernetes.io/canary-weight: <0 to 100>

After the spec is applied, the ingress in question has it's backends marked as alternative so that the merging code will know what to do with them.

AlternativeBackends

When ingresses are synced, the list of backends and servers is built up. After these lists are finished being built in getBackendServers(), but before they are sorted and returned to the sync function, we check if the current ingress has a canary enabled. If so, we pass a reference of the ingress, annotations, upstreams, and servers to the mergeVirtualBackends function. It works as such:

  1. For each path that matches a alternative backend, find the server that matches its path and is not alternative.
  2. If found, embed a alternative backend into the primary backend. If no matching path is found, then delete the alternative upstream as it is invalid and has no server to back it.

When alternative backends are matched to a primary backend, they will have access to metadata that describes the primary backend as well as rules to shape traffic. These rules come from the canary ingress configuration.

Which issue this PR fixes

This PR is based on this proposal https://docs.google.com/document/d/1qKTyLBLuKIYE6d6BsFXRM7zYB-2MUk6qJjtBL1KCz78/edit#heading=h.gv8zkanxal65 by @ElvinEfendi and from #2560

Closes #2419

Special notes for your reviewer:

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 1, 2018
@ElvinEfendi
Copy link
Member

ElvinEfendi commented Nov 1, 2018

@clandry94 can you include some documentation showing an example usage and also noting the existing limitations (i.e single alternative backend support so far)

Also standard annotation documentation at https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/annotations/

@clandry94 clandry94 force-pushed the canary_upstream branch 6 times, most recently from c18e2b5 to 8aeb4ca Compare November 1, 2018 21:10
merged := false

// find matching paths
for _, server := range servers {
Copy link
Member

Choose a reason for hiding this comment

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

Why don't you retrieve the server directly using

server := servers[rule.Host]

?

return balancers[backend_name]

local balancer = balancers[backend_name]

Copy link
Member

Choose a reason for hiding this comment

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

If balancer is nil we should short-circuit and return here. This is the case when all endpoints are unavailable or there's no endpoint configured.

Currently as you can see in the e2e tests, you will get an error like

018/11/02 07:56:39 [error] 228#228: *112 lua entry thread aborted: runtime error: /etc/nginx/lua/balancer.lua:126: attempt to index local 'balancer' (a nil value)
stack traceback:
coroutine 0:
	/etc/nginx/lua/balancer.lua: in function 'route_to_alternative_balancer'
	/etc/nginx/lua/balancer.lua:167: in function 'get_balancer'
	/etc/nginx/lua/balancer.lua:184: in function 'rewrite'
	rewrite_by_lua(nginx.conf:422):2: in function <rewrite_by_lua(nginx.conf:422):1>, client: 10.244.2.1, server: unavailable.svc.com, request: "GET / HTTP/1.1", host: "unavailable.svc.com"
2018/11/02 07:56:39 [error] 228#228: *112 failed to run log_by_lua*: /etc/nginx/lua/balancer.lua:126: attempt to index local 'balancer' (a nil value)
stack traceback:
	/etc/nginx/lua/balancer.lua:126: in function 'route_to_alternative_balancer'
	/etc/nginx/lua/balancer.lua:167: in function 'get_balancer'
	/etc/nginx/lua/balancer.lua:212: in function 'log'

Copy link
Author

Choose a reason for hiding this comment

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

Fixed 👍

@clandry94 clandry94 force-pushed the canary_upstream branch 6 times, most recently from 1de1e8b to 4579866 Compare November 6, 2018 14:57
@clandry94
Copy link
Author

/assign @aledbf

test/e2e/e2e.go Outdated
// _ "k8s.io/ingress-nginx/test/e2e/servicebackend"
// _ "k8s.io/ingress-nginx/test/e2e/settings"
// _ "k8s.io/ingress-nginx/test/e2e/ssl"
// _ "k8s.io/ingress-nginx/test/e2e/status"
Copy link
Member

Choose a reason for hiding this comment

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

I assume this change is from your testing.

Copy link
Member

Choose a reason for hiding this comment

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

@clandry94 you can use FOCUS="Annotations\s-\scanary" make e2e-test to avoid commenting the code.

Adds the ability to create alternative backends. Alternative backends enable
traffic shaping by sharing a single location but routing to different
backends depending on the TrafficShapingPolicy defined by AlternativeBackends.

When the list of upstreams and servers are retrieved, we then call
mergeAlternativeBackends which iterates through the paths of every ingress
and checks if the backend supporting the path is a AlternativeBackend. If
so, we then iterate through the map of servers and find the real backend
that the AlternativeBackend should fall under. Once found, the
AlternativeBackend is embedded in the list of VirtualBackends for the real
backend.

If no matching real backend for a AlternativeBackend is found, then the
AlternativeBackend is deleted as it cannot be backed by any server.
@ElvinEfendi
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 6, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: clandry94, ElvinEfendi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 6, 2018
@k8s-ci-robot k8s-ci-robot merged commit 17cad51 into kubernetes:master Nov 6, 2018
@ElvinEfendi ElvinEfendi deleted the canary_upstream branch November 6, 2018 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

traffic split by request header or cookie
4 participants