Proposal update: Extend service invocation to non-Dapr endpoints #20

[sicoyle]

Opening another PR regarding the closed proposal found here for extending service invocation to non-Dapr endpoints.

This aims to provide clarity on more easily using the secretKeyRef as part of the headers, and provide more examples in the proposal. Opened as a response to Artur's comment here.

Thoughts?

[ItalyPaleAle]

I'm not sure I understand this.

Is authScheme only used for detecting the prefix of the Authorization header? If so, it feels weird to have a single header of all the possible ones (defined in a slice where keys can be arbitrary names) to have a special treatment.

Here's some thoughts:

First: Treat all headers equal. Every header can either be a hardcoded value, or a secreKeyRef. If you need a prefix in the auth header, it should be defined as part of the value/secret.

Second: Although "Authorization" is the most commonly-used auth header, some APIs have different header names, such as "x-api-token". We should support those cases too.

Third: We have a problem when the auth header is changing frequently. For example, bearer tokens are usually short-lived: in many cases, the lifetime is 1hr. Something that is hardcoded, or even a reference to a K8s secret, is not scalable in this case.
Maybe there could be an endpoint in the app that can be invoked to retrieve headers specific for each request?

[sicoyle]

@ItalyPaleAle
Yes, I was thinking of having authScheme as a separate field, and do recognize what you're saying in it maybe not being the best to treat one header different than the rest. However, is it a safe, and a Dapr user friendly experience, to assume that they will include the prefix in the header value?

Yes, although Authorization is the only security tailored header field displayed, others can be accounted for. This would be made easier in treating all headers the same and not adjusting any of them myself with prefixes.

I don't think I'm following 100% on this comment below. What app specifically?

Maybe there could be an endpoint in the app that can be invoked to retrieve headers specific for each request?

[ItalyPaleAle]

On the auth scheme. How about we make prefix a property in each header then?

- name: "x-foo"
  # Note the space at the end (or should we automatically add it?
  prefix: "hello "
  # Could also be a secretRef
  value: "world"

At least this way it's not tied to an item with a given "name".

As for the other thing, let me comment on the main issue

[sicoyle]

how applicable would the prefix be for other header fields though..? and thank you for offering to explain a bit more on the question I had!

[ItalyPaleAle]

Not sure how many more headers can benefit from that, but IMHO it's a lot cleaner than having a fixed property which applies to a property that is in a "dynamic" list.

What I mean is:

• authScheme is a field in the spec, that is defined in the OpenAPI schema. This is a well-known value. I am calling this "fixed" because it's part of the schema.
• The "Authorization" header is defined in a list of key-values, where "Authorization" is an arbitrary string. I am calling this "dynamic" because it's not defined in the schema.

It seems that we're giving some user-defined values, which are not part of the schema, some preferential treatment, defined by a property that is defined in the schema. Maybe it's just me, but this seems it is not very "tidy".

[sicoyle]

went ahead and removed the prefix idea and am putting that as an assumption that it is part of the header value itself. Please lmk if you'd like us to revisit this. @ItalyPaleAle

[ItalyPaleAle]

@sicoyle on the second part of the comment here

The problem I'm pointing out is that values for authorization headers can in many cases be rotated very frequently. For example if you use OAuth2, many times tokens last 60 minutes or less. That makes storing them as K8s secrets and then distributing them across all apps not convenient or practical (some app, which will need write access to k8s secrets, will need to constantly refresh the tokens, then the Dapr Operator has to pick them up, and distribute them to all the apps).

One possibility I was suggesting was offering an alternative way to provide the value. Right now, options are:

- name: "my-header"
  value: "foo"

Could also be a secret ref. Both are "static".

What if we added another option called, for example, provider, which contains the ID of a Dapr app:

- name: "my-header"
  provider: "myapp"

Before making a call to the given endpoint, then, daprd will invoke the app named "myapp" by calling a new method on the App Channel (the same thing we use for service invocation, to send PubSub messages to the app, etc).

The invocation will send the app a request with the URL that is being invoked and the name(s) of the header(s) that needs to be populated. The app will respond with the value of the header.

For example, if the request body contains (as an example JSON):

{
  "endpoint": "https://foo.com/some/path",
  "headers": ["my-header"]
}

The app will respond with the header values:

{
  "headers": {
    "my-header": "foo"
  },
  "ttl": "1h"
}

Then daprd knows to use "foo" as value for "my-header".

The optional "ttl" indicates that the value can be cached within daprd for up to 1hr.

[yaron2]

@sicoyle on the second part of the comment here

The problem I'm pointing out is that values for authorization headers can in many cases be rotated very frequently. For example if you use OAuth2, many times tokens last 60 minutes or less. That makes storing them as K8s secrets and then distributing them across all apps not convenient or practical (some app, which will need write access to k8s secrets, will need to constantly refresh the tokens, then the Dapr Operator has to pick them up, and distribute them to all the apps).

One possibility I was suggesting was offering an alternative way to provide the value. Right now, options are:

- name: "my-header"
  value: "foo"

Could also be a secret ref. Both are "static".

What if we added another option called, for example, provider, which contains the ID of a Dapr app:

- name: "my-header"
  provider: "myapp"

Before making a call to the given endpoint, then, daprd will invoke the app named "myapp" by calling a new method on the App Channel (the same thing we use for service invocation, to send PubSub messages to the app, etc).

The invocation will send the app a request with the URL that is being invoked and the name(s) of the header(s) that needs to be populated. The app will respond with the value of the header.

For example, if the request body contains (as an example JSON):

{
  "endpoint": "https://foo.com/some/path",
  "headers": ["my-header"]
}

The app will respond with the header values:

{
  "headers": {
    "my-header": "foo"
  },
  "ttl": "1h"
}

Then daprd knows to use "foo" as value for "my-header".

The optional "ttl" indicates that the value can be cached within daprd for up to 1hr.

This might be something to look at for next iterations but largely irrelevant for the current iteration. Rotation mechanisms should not be a concern. Users can always override or insert headers as part of their HTTP call to Dapr and can thus integrate natively with whatever mechanism they have going or might have used/been using independently of Dapr.

[artursouza]

@sicoyle on the second part of the comment here
The problem I'm pointing out is that values for authorization headers can in many cases be rotated very frequently. For example if you use OAuth2, many times tokens last 60 minutes or less. That makes storing them as K8s secrets and then distributing them across all apps not convenient or practical (some app, which will need write access to k8s secrets, will need to constantly refresh the tokens, then the Dapr Operator has to pick them up, and distribute them to all the apps).
One possibility I was suggesting was offering an alternative way to provide the value. Right now, options are:

- name: "my-header"
  value: "foo"

Could also be a secret ref. Both are "static".
What if we added another option called, for example, provider, which contains the ID of a Dapr app:

- name: "my-header"
  provider: "myapp"

Before making a call to the given endpoint, then, daprd will invoke the app named "myapp" by calling a new method on the App Channel (the same thing we use for service invocation, to send PubSub messages to the app, etc).
The invocation will send the app a request with the URL that is being invoked and the name(s) of the header(s) that needs to be populated. The app will respond with the value of the header.
For example, if the request body contains (as an example JSON):

{
  "endpoint": "https://foo.com/some/path",
  "headers": ["my-header"]
}

The app will respond with the header values:

{
  "headers": {
    "my-header": "foo"
  },
  "ttl": "1h"
}

Then daprd knows to use "foo" as value for "my-header".
The optional "ttl" indicates that the value can be cached within daprd for up to 1hr.

This might be something to look at for next iterations but largely irrelevant for the current iteration. Rotation mechanisms should not be a concern. Users can always override or insert headers as part of their HTTP call to Dapr and can thus integrate natively with whatever mechanism they have going or might have used/been using independently of Dapr.

I think we should add a solution like this in a future iteration for this feature. The provider should not be attached to any header since the callback result would dictate which headers need to be set. Regardless, the current scope is for "static" values - which works for many scenarios (but not all).

[ItalyPaleAle]

I think we should add a solution like this in a future iteration for this feature.

It's ok if this isn't implemented in 1.11 but I think it's still important that the design be finalized here taking into account where we want to be when the feature is complete. Not taking into account what we want in the future can put us in a situation where we are making choices now that limit what we can do in the future. This is especially concerning in this case since we're introducing a new CRD, which are harder to make changes to (unless we make a "v1alph12").

To quote @yaron2's comment from this repo:

1. Proposals are separated from milestones. This can go anywhere from 1.11 to 1.14 and beyond.
2. Because proposals are separated from milestones, there is no requirement for every feature listed in a proposal to fit within a single milestone. Specifying the wanted interfaces serves as a decision record we can look back to and then derive milestone work for. How that actually works in practice remains to be seen as this is a new process for us.

Let me know how I can help finalizing the design.

[mukundansundar]

Regardless, the current scope is for "static" values - which works for many scenarios (but not all).

Why is this out of scope as it would help in coming up with a design that would be extensible enough to later not require breaking changes if any.

[yaron2]

This is especially concerning in this case since we're introducing a new CRD, which are harder to make changes to (unless we make a "v1alph12")

For the changes discussed above to support value rotations on a CRD, adding something like the suggested provider field would not require a v1alpha2 CRD for 2 reasons:

1. This change does not constitue a breaking change, its an addition of a new field
2. The CRD itself is v1alpha1 and will be labeled as an alpha feature, meaning we can have breaking changes (technically all Dapr CRDs have an apiVersion of v1alpha1, which should have been changed to v1 a long time ago).

For the provider suggestion in particular, there are many holes in the design right now, mainly being that providing an app id in the header for Dapr to call back to isn't applicable, as the app channel is always used to communicate with a single app and its associated id. What more, since its the app that is calling Dapr to externally invoke a service, it seems very awkward and backwards to have Dapr call back to the app to get a header value while the app is still waiting for Dapr to complete the call. In single threaded applications this will lead to a deadlock.

Finally, as mentioned before, users have the option of overriding header values per call and thus are free to use any rotation mechanisms they would use outside of Dapr to fetch the updated values and use it in the svc invocation call - whether it is internal or external, and can also use the Secrets API to fetch any sensitive header values when needed before the invocation call.

Supporting rotations for various fields in Dapr CRDs in a generic way is a good idea and should be discussed separately, to be solved broadly. Today we don't support rotations for stable components CRDs which are far more critical than any optional header value rotations in this alpha CRD. As all other changes requested have been made, we can approve and merge this. We can open as many follow up PRs / issues as needed to discuss the provider setting or any other suggestions for future iterations of this API.