Allow Nested Namespaces in Cedar Schemas#105
Allow Nested Namespaces in Cedar Schemas#105chaluli wants to merge 3 commits intocedar-policy:mainfrom
Conversation
Signed-off-by: Charlie Murphy <mutmoth@amazon.com>
chaluli
changed the title
cdisselkoen
left a comment
cdisselkoen
left a comment
There was a problem hiding this comment.
This RFC seems reasonable to me.
| } | ||
| ``` | ||
|
|
||
| While the current namespaces make it possible to distinguish between the similarly named `aws::iam::User` and `google::identity::User` and `aws::s3::Bucket` and `google::storage::Bucket`. It is unintuitive that the sub-namespaces cannot be nested in a single namespaces for `aws` and `google`, respectively and similarly that one cannot refer to `iam::User` inside `aws::s3` without fully qualifying the name as `aws::iam::User`. |
There was a problem hiding this comment.
tiny nit: this paragraph probably belongs in the Motivation section
| } | ||
| ``` | ||
|
|
||
| Today, this is a currently valid schema. In which `Foo::MyThing` is a common type referring to `Bar::Thing` (definition 1). If we were to do relative name resolution based on namespaces with equal path prefixes, then `Foo::MyThing` would instead be a common type referring to `Foo::Bar::Thing` (definition 2), which would be backwards compatible. |
There was a problem hiding this comment.
at the end of this paragraph do you mean "backwards incompatible"?
There was a problem hiding this comment.
I got confused by this.
There was a problem hiding this comment.
If we follow the algorithm below, I'd expect Bar::Thing still refers to (1) because the names of scope Foo only contain MyThing. Am I missing anything?
Co-authored-by: John Kastner <130772734+john-h-kastner-aws@users.noreply.github.com>
|
|
||
| Today, this is a currently valid schema. In which `Foo::MyThing` is a common type referring to `Bar::Thing` (definition 1). If we were to do relative name resolution based on namespaces with equal path prefixes, then `Foo::MyThing` would instead be a common type referring to `Foo::Bar::Thing` (definition 2), which would be backwards compatible. | ||
|
|
||
| However, this RFC proposes that for nested namespaces, most users would expect definition 2 would be used. This RFC proposes that for structurally nested namespaces, definitions defined within the namespace declaration shadow namespaces outside of the namespace declaration. |
There was a problem hiding this comment.
One concern: Will it be confusing that the schemas above and below this paragraph are not equivalent? Users might expect them to be equivalent?
There was a problem hiding this comment.
Perhaps. But I already think the choice of name resolution in cedar is confusing. In that, when I first looked into schemas, I expected the one above to behave as the one below.
There was a problem hiding this comment.
I guess the question is, are things more confusing after this RFC than the status quo. The argument on the side of this RFC would be that now things behave more intuitively in the case that you write nested namespaces. The argument against this RFC would be that having both of these schemas legal, but not equivalent, is more confusing than simply disallowing the latter and forcing everyone to write more namespaces explicitly.
|
|
||
| ## Drawbacks | ||
|
|
||
| While this change is likely to improve how one writes schemas, we should support nested schemas in a backwards compatible way. This means that, how we perform name resolution may be tricky in order to support backwards compatibility. This may either (1) limit how much users can exploit nested namespaces to use relative namespace identifies when defining entities and types or (2) increase the complexity of name resolution which may slow down parsing of Cedar Schemas. |
There was a problem hiding this comment.
Can you elaborate on this? What specific corner cases in the current name resolution system could cause backwards compatibility issues when this feature is implemented?
There was a problem hiding this comment.
A basic example of what problems can occur is that if we support relative name resolution for nested namespaces is found in the paragraph on lines 195-198, it would only work for namespaces that are structurally nested. Current "nested" namespaces (e.g., ones in which the path is a prefix of another) do not have the same resolution structure.
| ## Detailed design | ||
|
|
||
| This RFC proposes updating the parser and name resolution algorithm for Cedar Schemas to enable nested namespaces with relative namespace resolution (for nested namespaces). Consider the example from above (with AWS and Google identity and storage entity types). This RFC proposes that both schemas are accepted and once parsed result in equal Schemas. |
There was a problem hiding this comment.
Does this RFC propose any changes to name resolution in the JSON schema format? or any changes to what defines a valid schema in the JSON format?
There was a problem hiding this comment.
It does not. But I would be open to changing this RFC to propose the JSON schemas be recursively definable structures that match the changes to human readable schema format. But, I think Json isn't really human readable and as such would not benefit as much from the purpose of this RFC.
There was a problem hiding this comment.
I agree with this, but the RFC should say so explicitly. It should also state whether we preserve the property that all valid schemas in one format can be translated into a valid schema in the other format (I assume we do).
| entity Thing; // (1) | ||
| } | ||
|
|
||
| namespace Foo { |
There was a problem hiding this comment.
Will we allow multiple instances of the same namespace declaration? I guess we would have to in order to be backwards compatible
There was a problem hiding this comment.
Today, you cannot define the same namespace multiple times in the same schema fragment. See cedar-policy/cedar#1086
| ``` | ||
|
|
||
|
|
||
| ### Why relative name resolution for only structurally nested namespaces and not any namespace with the same path prefix? |
There was a problem hiding this comment.
Do we need to consider the interaction with reopening namespace?
in one fragment
namespace Foo {
entity A;
namespace Bar {
entity B;
}
}
and then in another fragment
namespace Foo {
namespace Bar {
type C = A;
}
}
and then what if we start allowing you to reopen a namespace inside the same fragment (don't think you can do that now, but could be wrong)
|
|
||
| Today, this is a currently valid schema. In which `Foo::MyThing` is a common type referring to `Bar::Thing` (definition 1). If we were to do relative name resolution based on namespaces with equal path prefixes, then `Foo::MyThing` would instead be a common type referring to `Foo::Bar::Thing` (definition 2), which would be backwards compatible. | ||
|
|
||
| However, this RFC proposes that for nested namespaces, most users would expect definition 2 would be used. This RFC proposes that for structurally nested namespaces, definitions defined within the namespace declaration shadow namespaces outside of the namespace declaration. |
There was a problem hiding this comment.
You could be more explicit here by indicating what option (1 or 2) would MyThing resolve to.
| } | ||
| ``` | ||
|
|
||
| Similarly, for the below schema, this RFC proposes that `Foo::Bar::Baz::MyUser` be a common type that references definition 2, because it's nested namespace scope is the closest enclosing scope to the common type declaration. |
There was a problem hiding this comment.
Is there anything special about this case? It seems that as a general rule you just use "closest scope" as resolution.
|
|
||
| The primary alternative is not accepting this rfc and instead keeping the status quo. I believe the current status quo is a worse user experience when writing a Cedar schema but would not encounter the drawbacks stated above. | ||
|
|
||
| An alternative to defaulting to use names (resolving to the name within the closest containing namespace), we could instead add explicit path identifiers for the current or containing scopes, e.g., `:this::User` and `:super::User` to help disambiguate relative versus global paths. |
There was a problem hiding this comment.
Can we elaborate more on the namespace-based resolution which would be backwards incompatible? Is there any reason this would be the right choice long term?
6 participants
Rendered