add header-value annotation

[minherz]

This PR extends the existing canary-by-header annotation.

What this PR does / why we need it:
This PR introduce more flexibility into the existing definition of the canary routing rules. The current implementation requires sending a header with a fixed value in order to trigger a canary routing which requires dedicated changes in the code of the application. This is less convenient when the application already has a header which can be used given its value represents a partition of users applicable to the canary testing (e.g. tenant id in the multi-tenant application).

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:
This subject was already discussed in this message thread. Given we agree on implementation, I am going to add tests to the new annotation.

[minherz]

@ElvinEfendi , please have a look. I would like to better understand the suggestion you wrote in the message thread. Do you propose to have a single annotation that defines either header, cookie or weight or you like to have more complex condition like two headers or a header and cookie?

[clandry94]

Could you please update the e2e tests to reflect the use-cases of these changes? https://github.com/kubernetes/ingress-nginx/blob/master/test/e2e/annotations/canary.go

[minherz]

@ElvinEfendi , @clandry94 what possible scenarios of canary traffic policies you think may be required?
my list is following:

• header(s) expression evaluated to true (expressions are exists and equal to value)
• cookie(s) expression evaluated to true (expressions are exists and equal to value)
• coin tossing (a.k.a. weight)
  All three can be combined while the number of of headers and cookies has to be limited (e.g. to 1).
  Another question is whether preparing a lua expression and execute it via

expr = loadstring(traffic_shaping_policy.predicate)
if expr() then
  return true
else
  return false
end

or evaluate the policy basing on data structure like it is now?
The latter will not work well given we want dynamic number of entries or predicate that includes ORed and ANDed results.

[minherz]

@clandry94, can you advise on possible reasoning for e2e tests failures? I see there failures for other modules beside canary. i am not experienced with the tests output. i cannot see the response headers or body, hence cannot identify the source of the problem.

[clandry94]

@minherz You can add FOCUS="Annotations\s-\scanary" make e2e-test when running the tests to only have the canaries e2e tests run. If it still times out, CI will also run them

[minherz]

it seems that all "my" e2e tests pass. i have a problem to run it locally on my Windows 10 environment because Docker for Windows conflicts with minikube due different hypervisor platforms and ingress-nginx build scripts fail to run without minikube (even build). I will try to organize a Linux VM to run everything on it in a hope it works out of the box. However, e2e failures seem unrelated to my change.

[minherz]

@ElvinEfendi, @clandry94 I would like to propose the following annotation to replace the existing:

Define the annotation nginx.ingress.kubernetes.io/canary-by-policy so the name of the structure in Lua will make more sense. The annotation value will be a expression in a form:

<predicate> ::= <condition>*(<sp><bool_op><sp><condition>)
<bool_op> ::= "or" / "||" / "and" / "&&"
<condition> ::= <field><osp>"="<osp><value>
<sp> ::= SPACE / TAB
<osp> ::= *<sp>
<field> ::= "weight" / <cookie> / <header>
<value> ::= 1*UCHAR / """1*UCHAR"""
<cookie> ::= "@"<name>
<header> ::= "#"<name>
<name> ::= "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA

I don't know if there is a simple parsing package in go. If there is none, a simple one can be implemented here. The idea is to parse the predicate and run in a loop in Lua script on each condition.
Since basic one will be either a single header or cookie or weight and sometimes a combination of two or three, the above should be sufficient. I will open a separate issue to describe it if you are OK.

[ElvinEfendi]

@minherz regarding to supporting more complex expressions let's not address it just yet. I'm looking into introducing Lua code sandboxing into ingress-nginx for other purposes that can also be used here.

Also let's let the users use the existing canary feature and provide more feedback - I'd rather avoid implementing something that won't be used widely.

[aledbf]

Also let's let the users use the existing canary feature and provide more feedback - I'd rather avoid implementing something that won't be used widely.

💯 that.

[minherz]

@minherz regarding to supporting more complex expressions let's not address it just yet. I'm looking into introducing Lua code sandboxing into ingress-nginx for other purposes that can also be used here.

Also let's let the users use the existing canary feature and provide more feedback - I'd rather avoid implementing something that won't be used widely.

Then I misinterpret the suggestion you wrote in the mail thread:

It would be even better if you implement a more general version of it: something like introducing a canary-by-predicate annotation which can have values such as "header(my-header) > 99"
or ``"header(my-header) == custom_value"or"cookie(my-cookie) > 99" etc
Do you suggest to replace the existing multiple annotations with a single one but having the same functionality?

[minherz]

@ElvinEfendi can I mark the conversations as resolved?
One more question, some e2e tests seem to fail due timeout. it doesn't look like my change is the reason. what do i do with that?
@aledbf, what does the label blocked-paths means?

[minherz]

@ElvinEfendi and @aledbf , i have create a PR that implements the suggestion to replace current multiple canary annotations with one that follows the same routing logic.
Please, have a look and let me know if you prefer to replace this PR with the new one or we can move forward with this one.
Thank you.

P.S. the new PR doesn't have e2e tests yet. Before submitting it, I will edit existing tests to reflect the new format and structure.

[minherz]

ping @aledbf, @ElvinEfendi,
i don't know about you but i, personally, hate hanged PRs. it is to you to decide but either reject it or let's move forward with it.

[minherz]

ping @aledbf, @ElvinEfendi,@clandry94
fellows, is there any way to make a progress with this PR?
@ElvinEfendi, would you prefer to replace this PR with this change?

[aledbf]

would you prefer to replace this PR with this change?

Please no. Annotations are just strings, no to store json or yaml definitions.

[aledbf]

what does the label blocked-paths means?

Nothing in particular, is just a safety measure to avoid just one approve/lgtm command that will trigger the merge of the PR.

i don't know about you but i, personally, hate hanged PRs. it is to you to decide but either reject it or let's move forward with it.

@minherz apologies for the delay in the review of this PR. For this feature, I prefer to defer the first lgtm to @ElvinEfendi/@clandry94

[minherz]

@minherz apologies for the delay in the review of this PR. For this feature, I prefer to defer the first lgtm to @ElvinEfendi/@clandry94

ok, guys (@ElvinEfendi/@clandry94) I am waiting for your feedback about next steps with this PR and with the suggestion.

[minherz]

I think this PR looks good - as I mentioned earlier we should stick to simplest possible solution for now.
Please include some readme and rebase with master. (also squash commits into one)

Done

[ElvinEfendi]

+2,235 −667

@minherz something must have went wrong with rebasing - there are a lot of unrelated changes.

[minherz]

+2,235 −667

@minherz something must have went wrong with rebasing - there are a lot of unrelated changes.

I synced master in my fork and then merged it into my work branch which I used for this PR. At the end I rebased it back by 8 commits but git produced 53. Reviewing the commits they all seem to happened between the change i made by Jan 5th and today.

[ElvinEfendi]

@minherz after you pull remote master you should rebase your branch with that, not merge

[minherz]

@ElvinEfendi it should be better now 😃

[ElvinEfendi]

@minherz it's indeed better! Test failure seems to be legit.

[ElvinEfendi]

@minherz please apply following patch:

diff --git a/rootfs/etc/nginx/lua/balancer.lua b/rootfs/etc/nginx/lua/balancer.lua
index 05650716d..f3b96f9d1 100644
--- a/rootfs/etc/nginx/lua/balancer.lua
+++ b/rootfs/etc/nginx/lua/balancer.lua
@@ -160,7 +160,7 @@ local function route_to_alternative_balancer(balancer)
   local target_header = util.replace_special_char(traffic_shaping_policy.header, "-", "_")
   local header = ngx.var["http_" .. target_header]
   if header then
-    if traffic_shaping_policy.headerValue then
+    if traffic_shaping_policy.headerValue and #traffic_shaping_policy.headerValue > 0 then
       if traffic_shaping_policy.headerValue == header then
         return true
       end

that will fix the bug that causes test to fail

[ElvinEfendi]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

• OWNERS [ElvinEfendi]

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

[ElvinEfendi]

/hold cancel

[ElvinEfendi]

@aledbf how do you cancel do-not-merge/blocked-paths?

[minherz]

the idea indeed was to encode a canary policy via multi-string yaml. what is wrong with that? it is simple and straightforward.

Nothing. Just annotations are not designed for that. This is another example why CRDs are a better fit this use cases.

@aledbf @clandry94 would it be possible to use annotation to define a configMap that will describe the canary policy? the policy can be updated either each time the annotation is processed or each time the configMap resource is updated.

[Sarelbotavia]

can we use multiple values in canary-by-header-value?
there is an intention to support this in the future?
or there is some workaround?

[longwuyuan]

I think we can safely say that currently you can use only one value.
No thought has been given to support multiple values.
There is o known workaround to use multiple values and expect match for any one of those values because of this

ingress-nginx/internal/ingress/annotations/canary/main.go

Line 27 in 1eeabe9

type canary struct {