Skip to content
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.

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

Well-known Attributes #17

Merged
merged 3 commits into from
Aug 24, 2023
Merged

Well-known Attributes #17

merged 3 commits into from
Aug 24, 2023

Conversation

guicassolato
Copy link
Contributor

No description provided.

@guicassolato guicassolato self-assigned this Jun 13, 2023
@guicassolato guicassolato added RFC Request For Comments status/discussing Requires further discussions target/current labels Jun 13, 2023
@guicassolato guicassolato requested a review from eguzki June 14, 2023 07:54
@guicassolato guicassolato force-pushed the rfc/well-known-attributes branch 16 times, most recently from 97de81f to 7642902 Compare June 14, 2023 10:57
@guicassolato guicassolato marked this pull request as ready for review June 14, 2023 10:59
@guicassolato guicassolato force-pushed the rfc/well-known-attributes branch 6 times, most recently from 36069ec to 2941f42 Compare June 14, 2023 11:08
@eguzki
Copy link
Contributor

eguzki commented Jun 15, 2023

Overall, looking super good.

Kuadrant's well known attributes are another value added by Kuadrant to be leveraged by users.

@guicassolato guicassolato requested a review from a team August 21, 2023 10:37
Copy link
Collaborator

@maleck13 maleck13 left a comment

Choose a reason for hiding this comment

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

some small comments:

We should follow up to ensure the sections marked as These attributes need confirmation if they are all actually available to be requested by the Wasm-shim. are confirmed.

@alexsnaps
Copy link
Member

alexsnaps commented Aug 21, 2023

some small comments:

We should follow up to ensure the sections marked as These attributes need confirmation if they are all actually available to be requested by the Wasm-shim. are confirmed.

Just to make it clear to all, these aren't spec'ed anywhere afaik, so that every other host might have a different set of those available...

@maleck13
Copy link
Collaborator

The wasm ones? Should we consider removing those or not adding the to the public facing docs? @guicassolato what if the public facing docs communicated only things like the request , auth and rate limit attributes?

@alexsnaps
Copy link
Member

Some notes here from me, feel free to ignore for now, but I think we'll have to account for these:

  • Currently the Rust SDK makes no difference between the property being null or actually being "not found". This might be an issue for us if we want to support multiple hosts....
  • A property's value are just pub type Bytes = Vec<u8>... if there is, amongst different hosts, different types to be returned for a given property, how would we deal with these? Minor issue, if it ever happens even, but raises another question...
  • Currently, Envoy's RL protocol "only" lets us pass String pairs thru the descriptors. Further more, Rust only allows valid UTF-8 strings... So we'd probably need to "stringify" the different types... Integers can be integer literals in Limitador's expression language. Timestamps could "simply" be ISO 8601 formatted strings. Which leaves us with more complex data structures... Depending on what we'd want to support as operators and whether these need to be evaluated in Limitador, we'll have to either support literals for at least Arrays and Maps, but we'd be left with Metadata & Node (that's if I don't miss anything)... Again, if these are to be evaluated in Limitador and NOT with the wasm-shim.
  • Finally, I'm unsure what someone would use the wasm.* attributes for? If there is no actual use for them today, should we just avoid adding them now?

@maleck13
Copy link
Collaborator

@alexsnaps I am a fan on not making those wasm ones available. Or at least not documenting publicly for now. I can imagine you can get a long way with more complex use cases with the request , auth and rl properties @guicassolato WDYT

@guicassolato
Copy link
Contributor Author

Should we consider removing those or not adding the to the public facing docs?

I'm okay with omitting the Wasm attributes. What about the Proxy configuration ones? Do we need them?

@alexsnaps
Copy link
Member

What about the Proxy configuration ones? Do we need them?

I see you applied the same logic there, right? i.e. remove them until we actually have a use case that requires them?

@@ -547,11 +547,13 @@ Another experience learned from Authorino's Authorization JSON selectors is that
# Unresolved questions
[unresolved-questions]: #unresolved-questions

1. Can we make more attributes that are currently available to only one of the components common to both?
1. How to deal with the differences regarding the availability and data types of the attributes across clients/hosts?
Copy link
Member

Choose a reason for hiding this comment

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

Thinking... should we only support (nested) Maps and "flatten" object structures down to these? So we'd have Strings, Numbers (i64?), arrays and nested maps... Would that be good enough... for now?

Copy link
Member

Choose a reason for hiding this comment

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

Maps would only ever be Stringly key'able probably...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we only support (nested) Maps and "flatten" object structures down to these? So we'd have Strings, Numbers (i64?), arrays and nested maps... Would that be good enough... for now?

I think so.


I think we only (mainly?) have this problem in the Rust-based apps, and to retrieve the Metadata attributes, which, by transition, extends the problem to the Auth attributes as well, correct?

Specifically the metadata attribute, in the Envoy environment, is a Map of String → Struct, where Struct is itself another Map of String → Value and Value can be pretty much anything.

The main use case we have for metadata in the Rust-based chain of apps (i.e. wasm shim, Limitador) is essentially of the auth service passing data to the RL service. Then, there are the edge (?) cases of users who want to inject sources of metadata in the filter chain themselves (e.g. from a Lua filter, etc).

The kind of thing that one may want to export from the auth service to RL is hard to predict, but I would guess most of them is related to attributes of the identity/principal –– or what we will later abstract as auth.identity.* in the RLP (thus achieving consistency with the AuthPolicy).

I have no recollection of identity objects ever defined as Map structs other than by only having stringy keys, i.e. auth.identity: Map<String → Any> in virtually 100% of the cases. And the values themselves are rarely of complex types – some Arrays and nested Maps no more than a 2-level deep.

E.g.,

  • auth.identity.username: String
  • auth.identity.verified_email: Boolean
  • auth.identity.scopes: Array<String>
  • auth.identity.access: Map<String → Map<String → Array<String>>>

Then we have other auth attributes (auth.(metadata|authorization|response.callbacks)) that are guaranteed to be Map<String → Any>. These are really open and the Any type becomes a problem.

Amongst all these known attributes, some could be used in conditions to be evaluated by the wasm shim, but, given the currently supported operators, not all of them.

As @alexsnaps pointed out, what is passed to Limitador in the RateLimitRequest is always String → String, although Integer literals can be already distinguished and (I'm guessing) Booleans should be easy to add support to if we want. Limitador also has its own set of operators, even though Kuadrant only needs == now. Still, this imposes a second level of constraints on the data types.

All this so far to explain (and agree) that the problem lies in the more complex data structures indeed.

Given, however:

  • the use cases we know to exist,
  • the operators we currently support,
  • the protocol limitations we have, and
  • the possibility the user almost always has to re-state the problem/implement the solution in a slight different way (e.g., in a AuthPolicy, in a Lua filter), to reduce it to an instance where only simple data types are employed,

...I think we do not need to support complex data structures. We can try to detect Numbers, maybe Booleans, but the rest can be always String.

A null value can be an empty string. Some meaning is lost in the process, but it's not the end of the world IMO (for this particular case). An Array is a String whose first character is a [ and the last is a ]; similarly, a Map starts with { and ends with }.

The user should have this in mind when defining conditions, qualifiers, or pretty much anything else that derives from Well-known attributes.

@guicassolato guicassolato merged commit 9620231 into main Aug 24, 2023
@guicassolato guicassolato deleted the rfc/well-known-attributes branch August 24, 2023 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC Request For Comments status/discussing Requires further discussions target/current
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants