Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Apr 27, 2023
Merged

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

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e5244c3
feat(secrets): update proposal crd to more easily work with secrets +…
sicoyle Apr 18, 2023
212c561
style: rm unnecessary comment
sicoyle Apr 18, 2023
0cdf513
feat(crd): update resource specifications
sicoyle Apr 18, 2023
a5702f4
fix: address pr feedback
sicoyle Apr 18, 2023
929acd7
docs: add clarity on order
sicoyle Apr 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 41 additions & 13 deletions 0006-B-external-service-invocation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ Cons:

How will this work, technically?

Allow configuration of pieces needed for external service invocation through creation of new CRD titled `ExternalHTTPEndpoint`.
Allow configuration of pieces needed for external service invocation through creation of new CRD titled `HTTPEndpoint`.
It is HTTP specific in it's `Kind`.
This has benefits in being obvious upfront that it supports only `http`,
and makes it to where we do not need `spec.allowed.protocols`.
Expand All @@ -94,30 +94,56 @@ The sample `yaml` file snippet below represents the proposed configuration.

```
apiVersion: dapr.io/v1alpha1
kind: ExternalHTTPEndpoint
kind: HTTPEndpoint
metadata:
name: externalserviceinvocation
name: "github"
spec:
allowed:
- name: github
baseUrl: "github.com"
headers:
- "Accept-Language": "en-US"
metadata:
- name: mymetadata
baseUrl: "http://api.github.com"
authScheme: "token" # or BASIC as default
headers:
- name: "Accept-Language"
value: "en-US"
- name: "Content-Type"
value: "application/json"
- name: "Authorization"
# assumed this is already base64 encoded
# Option #1
secretKeyRef:
name: my-secret
key: mymetadataSecret
name: "my-secret"
key: "mymetadataSecret"
# Option #2
#value: "base64encodedUser:base64encodedPWD" # put in header as is in this case
auth:
secretStore: my-secretstore
secretStore: "my-secretstore"
```

Noteworthy caveat:
If `Authorization` header specified,
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@sicoyle sicoyle Apr 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

Choose a reason for hiding this comment

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

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".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

then we would need validation on `authScheme` to become mandatory to know what to use for the prefix.
This allows for headers to match with the existing HTTP header schema,
thus leading to a better user experience that is straightforward to use.
The term `authScheme` comes from the HTTP headers syntax found [here](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Authorization).

Implementation for external service invocation will sit alongside the existing service invocation building block implementation with API changes to support external invocation.

User facing changes include overriding the URL when calling Dapr for service invocation.
Users will use the existing service invocation API, but instead of using an app ID,
they can use an external URL and optionally overwrite fields at the time of invocation.

To summarize, there would be two ways of working with external service invocations:
1. The URL format programatically.
This allows for convenience and includes a single HTTP call.
2. HTTPEndpoint resource creation declaratively,
where the `HTTPEndpoint.Name` would be used as the AppId in the existing service invocation URL.

#### Examples

1. URL format overwritten:
`http://localhost:${daprPort}/v1.0/invoke/http://api.github.com/method/`

2. HTTPEndpoint resource creation declaratively using the HTTPEndpoint resource definition above.
`http://localhost:${daprPort}/v1.0/invoke/github/method/`


### Feature lifecycle outline

Expand All @@ -139,6 +165,8 @@ N/A

* Compabitility requirements
This feature will need to be fully compatible with existing service invocation API.
In the case that a user tries to add an `HTTPEndpoint` with the same name as an AppId in the same namespace,
then the `HTTPEndpoint` will fail to create as names must be unique.

* Metrics
Existing service invocation tracing and metrics capabilities when calling external enpoints will be fully functional.
Expand Down