Add standard RequestID and Service fields to every instance objects

geeknoid commented 7 years ago

In order to allow proper multi-tenant behavior, Mixer should deliver the service name to adapters, independent of operator-level config. This avoids one service operator impersonating another.

We also want to include a request id, to assist complex adapters in understanding that a number of individual calls to HandleXXX methods are in fact related. This can help with cross-call caching.

Finally, as part of adding RequestID, we should make sure that all reports associated with a single RequestID should be delivered in a single batch to the adapter's HandleXXX methods. This will make adapters simpler and more robust. In fact, we may wish to sort batches to ensure all reports for the same RequestId appear bunched together in the batch delivered to the adapter.

mandarjog commented 7 years ago

On a related note, we should also add destination_service or (service_id) to the mixer protocol ? It is a fundamental part of the protocol, but it is lost in the current protocol.

douglas-reid commented 7 years ago

To clarify, is RequestID distinct from the http notion (and our attribute supported notion) ? In other words, is this a Mixer Request ID ?

How do we envision RequestID and DedupID interacting?

geeknoid commented 7 years ago

RequestId is intended to capture the whole scope of an operation from the proxy's perspective. All the Check/Report calls made by the proxy on behalf of a single "event" would have the same ID. DedupID is about a single call to Mixer, to provide idempotence for state mutation of that single call. So it's a different context.

Although I'm convinced that the Service field is critical to add, I'm not quite sure RequesId is in fact desirable. As the API management story comes together, we can evaluate whether this is in fact necessary or not.

On Fri, Oct 13, 2017 at 2:07 PM, Douglas Reid notifications@github.com wrote:

To clarify, is RequestID distinct from the http notion (and our attribute supported notion) ? In other words, is this a Mixer Request ID ?

How do we envision RequestID and DedupID interacting?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/istio/mixer/issues/1420#issuecomment-336567640, or mute the thread https://github.com/notifications/unsubscribe-auth/AVucHfdzKmW292e6Lb1JOhyqYnJ2PEtXks5sr9EcgaJpZM4P3xaU .

douglas-reid commented 7 years ago

Should RequestID be set as part of the context passed into adapters ? Seems like we already have a way to do that, and setting in the context is arguably appropriate.

mandarjog commented 7 years ago

What about mesh-id, we need to patch it thru as well.

geeknoid commented 7 years ago

Yep, very true.

Plumbing through the service name provide multi-service multi-tenancy. Plumbing through mesh id gives multi-mesh multi-tenancy support.

Should we combine the mesh id and service name into a single identifier given to the adapter? The point is to uniquely identify the service and just using the service name is insufficient for this. So forcing the mesh id in there unilaterally would avoid potential errors.

On Tue, Oct 17, 2017 at 11:47 AM, mandarjog notifications@github.com wrote:

What about mesh-id, we need to patch it thru as well.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/istio/mixer/issues/1420#issuecomment-337330363, or mute the thread https://github.com/notifications/unsubscribe-auth/AVucHSgyZu0ets99G4L55PcDyBh9h9kcks5stPYygaJpZM4P3xaU .

guptasu commented 7 years ago

@geeknoid so something like ServiceName string = \<meshId>/\<service name> ?

Questions :

Do we have some existing convention of doing such concatinatio in other places inside istio (may be some attributes) ? What is the separator used ?
source.service the correct attribute to fill in the Service field in instance ?
For mesh ID, where does that information live, I am assuming it is not in the request attribute bag and is in some configuration where mixer is deployed.

geeknoid commented 7 years ago

We discussed this and decided that we don't want the mesh id. Mixer is designed to operate within the context of a single mesh. It needs to support multi-tenancy between services, but not multi-tenancy between meshes. So let's just drop all notion of mesh id.

As for service, I think we want destination.service. @mandarjog ?

douglas-reid commented 7 years ago

Can/should we use the IstioService struct that is used elsewhere (Pilot) perhaps?

is the plan context.withValue(ctx, "destination", dest) ? or are we going to modify instances?

geeknoid commented 7 years ago

i'd rather extend instances instead of passing magic undiscoverable values in the context. Do you expect a problem with doing this?

As for IstioService, that's an interesting question. We apply policies against plain service names, so it feels as though this is the only thing we should pass to the adapters. Do you think there's a need for adapters to see more than what operator see in this context? Or do operators see all this stuff, but as distinct attributes?

On Thu, Oct 19, 2017 at 3:27 PM, Douglas Reid notifications@github.com wrote:

Can/should we use the IstioService struct that is used elsewhere (Pilot) perhaps?

is the plan context.withValue(ctx, "destination", dest) ? or are we going to modify instances?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/istio/mixer/issues/1420#issuecomment-338055419, or mute the thread https://github.com/notifications/unsubscribe-auth/AVucHT1C-RtISWuOFYDUiR9GG_TKinY8ks5st8y6gaJpZM4P3xaU .

guptasu commented 7 years ago

@douglas-reid IstioService is nicely structured and I think we will be able to fill it up on the mixer side. attributes source.(namespace | domain |name) should be sufficient.

However, IstioService is inside proxy package, so unless we move it to a generic location we cannot use it to pass to adapters. We can define our own similar structure IstioService that we can pass to adapters. Thoughts ?

I was thinking we can modify instance to have a field, but the issue is we cannot keep doing such addition because it will be a breaking change for template developer as we will introduce a reserve keyword. I like the idea of passing non template entries that describe the state of the call via the context object. Thoughts from others on the two question :

should we redefine our IstioService inside mixer package ?
Should we use the context object to pass call specific information and not pollute the instance object ?

guptasu commented 7 years ago

Chatted with @geeknoid and decided to first experiment with passing a proto object as part of context that represent the calls state. We can call it message CallContext and it will contain IstioService and can later be extended for other items to be passed to the Adapters via the context.

These values are filled by Mixer framework and not operator, therefore keeping them separate than Instance kind of make sense. Where everything inside the Instance is based on data provided by the operator in his/her config.

guptasu commented 7 years ago

@geeknoid @mandarjog @douglas-reid Here is an example on how adapter code will use the context to get data about the service and may be other information in the future.

https://github.com/guptasu/mixer/commit/9513f4376079f5c484404a9dc338aff459b7c698

Passing the call specific data via context will give adapters a sense of trust in that information. If there is no objection, I can start defining the proto and make the code change.

douglas-reid commented 7 years ago

a few thoughts:

we should be careful about Get() calls on bags just to add context information. we don't want to dirty attributes just because context is getting updated.
we need to be consistent re: service name and fully-qualified service name.
it would be awesome to reuse IstioService from pilot (vs. defining our own). what are the downsides of trying to use that existing definition?

geeknoid commented 7 years ago

We should be using the patterned described here: https://blog.golang.org/context

In particular, we need a NewContext/FromContext pattern. Adapters will call FromContext to get the payload.

We used to have a custom Context type in our source tree, but I'm unable to find it in the history given my poor git skills. If you've got better skills, you can find context.go in the history and use that as a foundation.

On Mon, Oct 23, 2017 at 5:49 PM, Douglas Reid notifications@github.com wrote:

a few thoughts:

we should be careful about Get() calls on bags just to add context information. we don't want to dirty attributes just because context is getting updated.

we need to be consistent re: service name and fully-qualified service name.

it would be awesome to reuse IstioService from pilot (vs. defining our own). what are the downsides of trying to use that existing definition?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/istio/mixer/issues/1420#issuecomment-338837998, or mute the thread https://github.com/notifications/unsubscribe-auth/AVucHQVV2WL_Fey6JGIVIEcro3SPBaxtks5svTQ0gaJpZM4P3xaU .

mandarjog commented 7 years ago

Mixer uses destination.service as the primary identifier which is IstioService sans labels.
I am not sure what context buys us over adding properly schematized interface with Getters that is reference counted.

If we are passing context anyway for cancellations, We can place this interface in "context" so it will func ArgsFromContext(ctx) Args { }

geeknoid commented 7 years ago

I don't understand your #2 point. Can you restate?

On Tue, Oct 24, 2017 at 10:04 AM, mandarjog notifications@github.com wrote:

Mixer uses destination.service as the primary identifier which is IstioService sans labels.

I am not sure what context buys us over adding properly schematized interface with Getters that is reference counted.

If we are passing context anyway for cancellations, We can place this interface in "context" so it will func ArgsFromContext(ctx) Args { }

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/istio/mixer/issues/1420#issuecomment-339061578, or mute the thread https://github.com/notifications/unsubscribe-auth/AVucHetNXAWhz9t6KaWQqGfr5ZPE1pEpks5svhiigaJpZM4P3xaU .

mandarjog commented 7 years ago

in #2 I am arguing against putting every piece of information individually in the context using magic keys. We should create a proper interface / struct and have only one magic key that provides all the information that we want to pass along.

geeknoid commented 7 years ago

Yes, then we agree. That's what I implied by introducing the NewContext and FromContext pattern. That's how our old context.go code worked.

On Tue, Oct 24, 2017 at 11:39 AM, mandarjog notifications@github.com wrote:

in #2 I am arguing against putting every piece of information individually in the context using magic keys. We should create a proper interface / struct and have only one magic key that provides all the information that we want to pass along.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/istio/mixer/issues/1420#issuecomment-339090142, or mute the thread https://github.com/notifications/unsubscribe-auth/AVucHQyE0gZUQT8M8DWCQFhMoqiQQ_KSks5svi7vgaJpZM4P3xaU .

guptasu commented 7 years ago

@mandarjog @geeknoid that is my understanding too. Therefore in the sample PR I have CallContext as the only interface (will be proto message) that is tied to single magic key "callcontext". All the data will be inside that wrapper object.

@geeknoid thanks for NewContext and FromContext pointer. It is a good pattern, no casting needed. I have updated the example to demonstrate that patter. The code location is not right, I need to find the right package for NewContext and FromContext code.

We can discuss naming/coding details on the PR, but at a high level we agree to:

use context instead of updating the Instance object ?
There will be a single magic key to fetch data from context and that data will be a proto message that we can extend in the future to add more information that we think the adapters would need.

geeknoid commented 7 years ago

That looks right.

Couple things though:

Probably should remove references to userIP in the code
Gotta make sure we're delivering a unique context with each call into adapters so that there's no accidental interference.

On Tue, Oct 24, 2017 at 12:09 PM, Sunny Gupta notifications@github.com wrote:

@mandarjog https://github.com/mandarjog @geeknoid https://github.com/geeknoid that is my understanding too. Therefore in the sample PR I have CallContext https://github.com/guptasu/mixer/commit/9513f4376079f5c484404a9dc338aff459b7c698#diff-3e8fbeb8245f151867bb976b7bc026bcR108 as the only interface (will be proto message) that is tied to single magic key "callcontext". All the data will be inside that wrapper object.

@geeknoid https://github.com/geeknoid thanks for NewContext and FromContext pointer. It is a good pattern, no casting needed. I have updated the example https://github.com/guptasu/mixer/commit/9f2b0a86c3a40bae340b8acb22f442dcbbdf49d8 to demonstrate that patter. The code location is not right, I need to find the right package for NewContext and FromContext code.

We can discuss naming/coding details on the PR, but at a high level we agree to:

use context instead of updating the Instance object ?

There will be a single magic key to fetch data from context and that data will be a proto message that we can extend in the future to add more information that we think the adapters would need.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/istio/mixer/issues/1420#issuecomment-339098963, or mute the thread https://github.com/notifications/unsubscribe-auth/AVucHQUyhRlyoErsI96i0Teeg9a2ARc4ks5svjXzgaJpZM4P3xaU .

guptasu commented 7 years ago

@geeknoid oops. userIP was a copy paste error. Okie, I will define the proto now and start the work to push data via the context.

Thanks for the discussion everyone.

guptasu commented 7 years ago

This is interesting, so if we send some set of attribute data (service name etc.) to the adapters via the context, it would mean that those attributes are used during the request execution and will be part of tainted attribute set. I think this is problematic because adapters might never use that information but our cache key will have that attribute as part of it. Is there a way to fetch data from attribute bag without tainting the attribute? Well, even if we do that, we will never know if operator actually used the passed in attribute data and we will not know if it should be in the cache key or not.

Two options that come to my mind and both have some obvious issues:

Data that we pass in context must be a key part of the request and it is okay if the attributes read as a result always become part of the cache key. So for example we should not use request.time to fill in data in the context object but it is okay to user destination.service.
We make the data passed in via context an interface and the Go implementation does the lookup from the attribute bag whenever the adapter invokes a function on the interface to read the data. But this does not work for grpcAdapters.

I think option #1 is the only option we have. Any other brilliant ideas ? @geeknoid @douglas-reid @mandarjog @ozevren

geeknoid commented 7 years ago

I think #1 is reasonable.

istio / old_mixer_repo

Add standard RequestID and Service fields to every instance objects #1420