mixer: add header operation templates #612

kyessenov · 2018-08-16T16:49:00Z

This is a minimal addition to expose routing features from Mixer.
I've put the combinatorial expression problem out of scope for now, and only added the header template and action name.
With these two, we have a way to propagate adapter attributes out to the client. Header operations are easy to combine, so that is also orthogonal to the check rule combination discussion.

/assign @geeknoid
/assign @mandarjog

Signed-off-by: Kuat Yessenov [email protected]

Signed-off-by: Kuat Yessenov <[email protected]>

geeknoid

You say this is merging is easy, but what's the ordering semantics if multiple adapters request header mutations?

geeknoid · 2018-08-16T20:06:16Z

policy/v1beta1/cfg.proto

+    enum Operation {
+      REPLACE = 0;  // replaces the header with the given name
+      REMOVE = 1;   // removes the header with the given name (the value is ignored)
+      APPEND = 2;   // appends the value to the header value, or sets it if not present


In this context, I don't quite understand the difference in semantics between REPLACE and APPEND. Can you elaborate? Also, the command says "appeands the value to the header value", and I'm not sure what "appends" means here. String concatenation?

HTTP header pairs can be repeated. For example, the following is correct in the protocol:

x-forwarded-for: 10.0.0.1 x-forwarded-for: 10.0.0.2 ...

It is usually up to the client to decide what to do with multiple values for a header. Typically, the values are comma-concatenated, e.g. the client would receive x-forwarded-for: 10.0.0.1, 10.0.0.2. Append operation corresponds to inserting a new pair into the request headers, without regard whether a value is present or not for the same header.

OK, so then what is the semantic of REPLACE and REMOVE given that there can be multiple headers with the same name? Do they remove all headers with the given name? What if you want to remove only the second instance of a particular named header?

Operations are executed one by one. Semantically, there is just one header with the given name, but it can be represented on the wire as several "key: value" pairs. That means removing is really removing all pairs on the wire, yet appending is fast, since you can just add one more pair.

Removing a second value in a list is simply not supported (not in envoy, nor any other proxy I know of). Usually you want to append a header value, or to replace it entirely. Replacing is more intuitive, so I made it the default.

There are a few examples here: https://github.com/istio/istio/blob/master/mixer/test/client/route_directive/route_directive_test.go#L95

Sorry to keep harping on this...

What you say here makes sense. So in this context, each header is thus a name with an array of values. In that context, I think the value field should be a []string instead of just a single string. Thus:

REMOVE. Remove a header with all its value
REPLACE. Replace a header and all its values with the new array of values
APPEND: Appends an array of values to a header's values.

My intent was to optimize for the common case, which is just a single header key-value pair. If you insist we can change the value to be an array type. I am just worried this might confuse the user, since we would need to explain the semantics of HTTP headers again.

In terms of implementation, the two are interchangeable:

REMOVE_ARRAY(key, _) = REMOVE(key, _) REPLACE_ARRAY(key, {"value1", "value2"}) = REPLACE(key, "value1, value2") APPEND_ARRAY(key, {"value1", "value2"}) = APPEND(key, value1); APPEND(key, value2)

I agree the patterns are isomorphic. I just find it confusing that the model is one header to multiple values, but the API just deals in terms of a single value like that's all there is.

Ultimately, it boils down to the syntax operators will use to manipulate this stuff. Do you have an idea how this would look in config?

Here is an example from the doc:

actions: - name: authorizer handler: authorizer instances: [“cookie”] requestHeaderOperations: - name: user-info value: $authorizer.user

geeknoid · 2018-08-16T20:06:50Z

policy/v1beta1/cfg.proto

+      APPEND = 2;   // appends the value to the header value, or sets it if not present
+    }
+
+    // Optional. Header operation type.


Optional? What's the behavior when not present?

Default value is 0, which is REPLACE.

Then let's add a comment about the default behavior when not specified

geeknoid · 2018-08-16T20:07:36Z

policy/v1beta1/cfg.proto

@@ -146,6 +174,9 @@ message Action {
  // Referenced instances are evaluated by resolving the attributes/literals for all the fields.
  // The constructed objects are then passed to the `handler` referenced within this action.
  repeated string instances = 3;
+
+  // Optional. A handle to refer to the results of the action application.


Remove "application"

Signed-off-by: Kuat Yessenov <[email protected]>

kyessenov · 2018-08-16T20:18:44Z

@geeknoid I think it is reasonable to expect no overlap between header operations for multiple rules for the same header keys. We can under-specify what happens if two rules apply operations on the same key. Mixer runtime may only need to guarantee a deterministic ordering of rule evaluation.

Signed-off-by: Kuat Yessenov <[email protected]>

geeknoid · 2018-08-16T23:14:13Z

OK, so if you want to add two values, you need to repeat a whole bunch of stuff. If the value field is an array, it's a trivial thing instead. So to me, it seems like a win to make the value field an array. Am I missing something?

…

On Thu, Aug 16, 2018 at 3:44 PM Kuat ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In policy/v1beta1/cfg.proto <#612 (comment)>: > @@ -124,6 +124,34 @@ message Rule { // Optional. The actions that will be executed when match evaluates to `true`. repeated Action actions = 2; + + // A template for an HTTP header manipulation. + message HeaderOperationTemplate { + + // Required. Header name. + string name = 1; + + // Optional. Header value. + string value = 2; + + // Header operation type. + enum Operation { + REPLACE = 0; // replaces the header with the given name + REMOVE = 1; // removes the header with the given name (the value is ignored) + APPEND = 2; // appends the value to the header value, or sets it if not present Here is an example from the doc: actions: - name: authorizer handler: authorizer instances: [“cookie”]requestHeaderOperations: - name: user-info value: $denier.user — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#612 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AVucHaz6SsOAiMfGKQAVuVI1k2Yino7fks5uRfXDgaJpZM4WAN6_> .

Signed-off-by: Kuat Yessenov <[email protected]>

kyessenov · 2018-08-17T00:01:13Z

Fair enough. Here is an updated example:

actions:
    - name: authorizer
      handler: authorizer
      instances: [“cookie”]
requestHeaderOperations:
    - name: user-info
      values: 
      - $authorizer.user

geeknoid · 2018-08-17T00:08:28Z

Thanks.

/lgtm
/approve

istio-testing · 2018-08-17T00:08:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: geeknoid

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~~ [geeknoid]

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

add header operation templates

7a3dc78

Signed-off-by: Kuat Yessenov <[email protected]>

istio-testing assigned geeknoid and mandarjog Aug 16, 2018

istio-testing requested review from costinm and sebastienvas August 16, 2018 16:49

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Aug 16, 2018

geeknoid reviewed Aug 16, 2018

View reviewed changes

comment

6a8dc8a

Signed-off-by: Kuat Yessenov <[email protected]>

kyessenov added 2 commits August 16, 2018 15:27

add comment

9330ece

Signed-off-by: Kuat Yessenov <[email protected]>

add comment

12fa1a5

Signed-off-by: Kuat Yessenov <[email protected]>

use repeated

efab8b2

Signed-off-by: Kuat Yessenov <[email protected]>

istio-testing added the lgtm label Aug 17, 2018

istio-testing added the approved label Aug 17, 2018

geeknoid approved these changes Aug 17, 2018

View reviewed changes

istio-testing merged commit 864a733 into istio:master Aug 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mixer: add header operation templates #612

mixer: add header operation templates #612

kyessenov commented Aug 16, 2018

geeknoid left a comment

geeknoid Aug 16, 2018

kyessenov Aug 16, 2018 •

edited

Loading

geeknoid Aug 16, 2018

kyessenov Aug 16, 2018

geeknoid Aug 16, 2018

kyessenov Aug 16, 2018 •

edited

Loading

geeknoid Aug 16, 2018

kyessenov Aug 16, 2018 •

edited

Loading

geeknoid Aug 16, 2018

kyessenov Aug 16, 2018

geeknoid Aug 16, 2018

kyessenov Aug 16, 2018

geeknoid Aug 16, 2018

kyessenov Aug 16, 2018

kyessenov commented Aug 16, 2018 •

edited

Loading

geeknoid commented Aug 16, 2018 via email

kyessenov commented Aug 17, 2018 •

edited

Loading

geeknoid commented Aug 17, 2018

istio-testing commented Aug 17, 2018

mixer: add header operation templates #612

mixer: add header operation templates #612

Conversation

kyessenov commented Aug 16, 2018

geeknoid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyessenov Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyessenov Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyessenov Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyessenov commented Aug 16, 2018 • edited Loading

geeknoid commented Aug 16, 2018 via email

kyessenov commented Aug 17, 2018 • edited Loading

geeknoid commented Aug 17, 2018

istio-testing commented Aug 17, 2018

kyessenov Aug 16, 2018 •

edited

Loading

kyessenov Aug 16, 2018 •

edited

Loading

kyessenov Aug 16, 2018 •

edited

Loading

kyessenov commented Aug 16, 2018 •

edited

Loading

kyessenov commented Aug 17, 2018 •

edited

Loading