-
Couldn't load subscription status.
- Fork 346
feat: support for rel and namespace deprecation #2504
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
base: main
Are you sure you want to change the base?
feat: support for rel and namespace deprecation #2504
Conversation
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (56.75%) is below the target coverage (75.00%). You can increase the head coverage or adjust the target coverage.
Additional details and impacted files@@ Coverage Diff @@
## main #2504 +/- ##
===========================================
- Coverage 77.62% 56.75% -20.87%
===========================================
Files 440 399 -41
Lines 54097 50483 -3614
===========================================
- Hits 41986 28645 -13341
- Misses 9490 19529 +10039
+ Partials 2621 2309 -312 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| err, | ||
| codes.Aborted, | ||
| spiceerrors.ForReason( | ||
| v1.ErrorReason_ERROR_REASON_SCHEMA_TYPE_ERROR, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a new error reason into the API and use it here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just raising that into the api repo, once that gets in, i will switch to that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here: authzed/api#143
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a TODO here
| return err | ||
| } | ||
| reader := ds.SnapshotReader(headRevision) | ||
| _, relDef, err := namespace.ReadNamespaceAndRelation(ctx, resource.ObjectType, update.Relationship.Relation, reader) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rather than reading the namespace again, let's do this check in the existing validation block, just as an additional check if enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
migrated the check to the relationship validation block to remove unnecessary lookups, also this now uses the typesystem for the read, also added a test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a method into the type system to retrieve the deprecation status of a namespace and/or relation and then use that? Its good to have it in a single place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| return err | ||
| } | ||
|
|
||
| if relDef.Deprecation != nil && relDef.Deprecation.DeprecationType != corev1.DeprecationType_DEPRECATED_TYPE_UNSPECIFIED { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can add deprecation to the type system and just read it from there
|
Just thinking about the UX a bit (we don't have to solve it in this PR - just thinking ahead!) it would be nice to be able to do something like: which would normally fail validation as a duplicated relation name |
Not sure... I kind of prefer: |
b5137fc to
1ea5e0f
Compare
this would require extending the support to the type reference, would look into that, or address do that in a follow up |
1ea5e0f to
9ce1eed
Compare
Followup is fine |
I added it just a while ago 😅, have added tests too, if anything looks sideways, would revert back to previous commit, or if any other suggestion is taken into consideration regarding the type ref approach for relation deprecation, let me know. |
9ce1eed to
f20b462
Compare
internal/relationships/validation.go
Outdated
| for _, allowed := range relDef.TypeInformation.AllowedDirectRelations { | ||
| // check namespace | ||
| if allowed.Namespace != rel.Subject.ObjectType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this iterates over the allowed relation types for a relation to validate the deprecation type, it now supports relation viewer: user | @deprecated(warn, "documents can no longer be public") user:*, also added tests for it.
| for _, allowed := range rel.TypeInformation.GetAllowedDirectRelations() { | ||
| // check namespace match |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could've used ts.GetFullRecursiveSubjectTypesForRelation() but that too would've took O(N) time and would want a *core.AllowedRelation at other steps as it gives the string types only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah
f20b462 to
b00779f
Compare
internal/relationships/validation.go
Outdated
| for _, update := range updates { | ||
| if update.Operation == tuple.UpdateOperationTouch || update.Operation == tuple.UpdateOperationCreate { | ||
| relsToCheck = append(relsToCheck, update.Relationship) | ||
| } | ||
| } | ||
| if err := CheckDeprecationsOnRelationships(ctx, relsToCheck, ts); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a little check here as we would not want an attempted write with DELETE operation to a deprecated relation or namespace to fail
internal/relationships/validation.go
Outdated
| func checkForDeprecatedRelationsAndObjects(ctx context.Context, rel tuple.Relationship, ts *schema.TypeSystem) error { | ||
| // Validate if the resource relation is deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this now uses the typesystem with relevant functions
7419d6f to
3a5f4d4
Compare
internal/relationships/validation.go
Outdated
| switch relDep.DeprecationType { | ||
| case core.DeprecationType_DEPRECATED_TYPE_WARNING: | ||
| log.Warn(). | ||
| Str("namespace", rel.Resource.ObjectType). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
namespace -> resource_type
internal/relationships/validation.go
Outdated
| } | ||
|
|
||
| // Validate if the resource namespace is deprecated | ||
| resDep, ok, err := ts.GetDeprecationForNamespace(ctx, rel.Resource.ObjectType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetDeprecationForObjectType
internal/relationships/validation.go
Outdated
| return err | ||
| } | ||
| if ok { | ||
| switch subDep.DeprecationType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make this "warning vs error" into a helper method and use here and the other two locations
internal/relationships/validation.go
Outdated
|
|
||
| // check deprecation for allowed relation types | ||
| dep, ok, err := ts.GetDeprecationForAllowedRelation( | ||
| ctx, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be worth having a method GetDeprecationForRelationship and have it return an error/warning message and what kind it is, if any
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but a single relationship might have multiple deprecations (a warning type deprecation at the resource or relation level, and an error type deprecation at the subject level) and it might be necessary to preserve all of them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these methods would now be invoked in CheckRelationshipDeprecation, does this sound about right?
| }`, | ||
| "document:foo#editor@testuser:tom", | ||
| core.RelationTupleUpdate_CREATE, | ||
| "the relation document#editor is deprecated: deprecated, migrate away", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drop the the, its cleaner :) - relation ....
| err, | ||
| codes.Aborted, | ||
| spiceerrors.ForReason( | ||
| v1.ErrorReason_ERROR_REASON_SCHEMA_TYPE_ERROR, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a TODO here
internal/services/shared/errors.go
Outdated
|
|
||
| func NewDeprecationError(namespace, relation, comments string) DeprecationError { | ||
| switch { | ||
| case relation == "*": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the string method in the type system to describe the relation: you need to handle wildcards, expiration and caveats, and all should be done in there
| }, | ||
| { | ||
| "deprecated relation same subject type with wildcard", | ||
| `use deprecation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add tests for deprecation of, say, a subject type without expiration but the one with expiration is still valid (and vice versa) and caveats as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added tests of these scenarios including the log and err messages, example "resource_type user with caveat somecaveat and expiration is deprecated:comments here"
pkg/schema/typesystem.go
Outdated
| } | ||
|
|
||
| // GetValidatedDefinition looks up and returns a definition struct, if it has been validated. | ||
| func (ts *TypeSystem) GetDeprecationForNamespace(ctx context.Context, definition string) (*core.Deprecation, bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add these methods to the type system tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
pkg/schemadsl/generator/generator.go
Outdated
| sg.markNewScope() | ||
|
|
||
| for _, relation := range namespace.Relation { | ||
| if relation.Deprecation != nil && relation.Deprecation.DeprecationType != core.DeprecationType_DEPRECATED_TYPE_UNSPECIFIED { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra into a helper method
proto/internal/core/v1/core.proto
Outdated
| /** | ||
| * deprecation contains the required deprecation for the relation. | ||
| */ | ||
| Deprecation required_deprecation = 8; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be marked as required. The required above means that a caveat (or expiration) must be specified when using the type as part of a subject, which doesn't apply here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed
3a5f4d4 to
47cda83
Compare
| func (ts *TypeSystem) CheckRelationshipDeprecation(ctx context.Context, relationship tuple.Relationship) error { | ||
| // Validate if the resource relation is deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method is now invoked during validation of the relationship being written, and is responsible for checking any applicable deprecations on the resource, relation, or subject
pkg/schema/typesystem.go
Outdated
| func (ts *TypeSystem) GetDeprecationForAllowedRelation( | ||
| ctx context.Context, | ||
| resourceNamespace string, | ||
| resourceRelation string, | ||
| subjectNamespace string, | ||
| subjectRelation string, | ||
| isWildcard bool, | ||
| hasCaveat bool, | ||
| hasExpiration bool, | ||
| ) (*core.Deprecation, bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this now checks whether the allowed relation reference has a caveat/expiration or both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth changing this to take in a traits map
bb31fc5 to
f0bc6ff
Compare
b9c44e0 to
21a1c1c
Compare
internal/relationships/validation.go
Outdated
| } | ||
| } | ||
|
|
||
| ts := schema.NewTypeSystem(schema.ResolverForDatastoreReader(reader)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's not reconstruct the type system, but rather pass it in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially tried to pass reader to the CheckDeprecationsOnRelationships function, then construct a type system there so that it would be centralised for other calls, but this would trigger the hedgingProxy to panic at a point where HeadRevision() is called twice, so circled back to this. (Neither ValidateRelationshipsForCreateOrTouch nor ValidateRelationships gets an argument of type system from upstream)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed a bit
| len(relationship.OptionalCaveat.Context.GetFields()) > 0 | ||
| } | ||
|
|
||
| func CheckDeprecationsOnRelationships( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doc comment
| }`, | ||
| "platform:foo#auditor@testuser:*", | ||
| core.RelationTupleUpdate_CREATE, | ||
| "wildcard allowed type testuser:* is deprecated: no wildcard please", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add the same test with writing the non-wildcard and ensure it does not return an error/warning
| ) | ||
| } | ||
|
|
||
| func NewDeprecationError(err error) DeprecationError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doc comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
object type, relation, or allowed subject type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also: can we add a test such that if an object type is deprecated, and you attempt to write with it as a subject, it also fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think that is already there, something like this?
pkg/namespace/builder.go
Outdated
| } | ||
|
|
||
| // AllowedRelationWithDeprecationCaveatAndExpiration creates a relation reference to an allowed relation. | ||
| func AllowedRelationWithDeprecationCaveatAndExpiration(namespaceName string, relationName string, withCaveat *core.AllowedCaveat, deprecation *core.Deprecation) *core.AllowedRelation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be easier to add a WithDeprecation that takes in an AllowedRelation, adds it, and returns a new one
pkg/schema/typesystem.go
Outdated
| return d, prevalidated, nil | ||
| } | ||
|
|
||
| func handleDeprecation(objectType, relation string, dep *core.Deprecation, logMessage string, caveatName string, hasExpiration bool) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more descriptive name than handle...
pkg/schema/typesystem.go
Outdated
| func handleDeprecation(objectType, relation string, dep *core.Deprecation, logMessage string, caveatName string, hasExpiration bool) error { | ||
| extra := "" | ||
| if caveatName != "" { | ||
| extra += fmt.Sprintf(" with caveat `%s`", caveatName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switch from fmt.Sprintf to string concat
| hasCaveat: false, | ||
| hasExpiration: false, | ||
| }, | ||
| expectedDeprecation: ns.Deprecation(core.DeprecationType_DEPRECATED_TYPE_WARNING, "wildcard"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sure to add the get deprecation accessors to the existing type system test accessor suite: https://github.com/authzed/spicedb/blob/main/pkg/schema/definition_test.go#L506
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
pkg/schemadsl/compiler/translator.go
Outdated
|
|
||
| ns := namespace.Namespace(nspath, relationsAndPermissions...) | ||
| var ns *core.NamespaceDefinition | ||
| if slices.Contains(tctx.enabledFlags, "deprecation") && slices.Contains(tctx.allowedFlags, "deprecation") && (deprecationForNamespace != &core.Deprecation{}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if deprecation is not enabled, we should make sure to raise an error during parsing
pkg/schemadsl/compiler/translator.go
Outdated
| } | ||
|
|
||
| // Add the deprecation, if any. | ||
| if slices.Contains(tctx.enabledFlags, "deprecation") && slices.Contains(tctx.allowedFlags, "deprecation") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this check into translateDeprecation and have it return an error if not enabled but specified
21a1c1c to
7f1a1d8
Compare
|
Rebased and updated |
internal/relationships/validation.go
Outdated
| } | ||
|
|
||
| // check if the relation is deprecated for create or touch operations | ||
| var relsToCheck []tuple.Relationship |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
relsToCheck := make([]tuple.Relationship, 0, len(updates))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah would improve perf, done
internal/relationships/validation.go
Outdated
| len(relationship.OptionalCaveat.Context.GetFields()) > 0 | ||
| } | ||
|
|
||
| // CheckDeprecationsOnRelationships checks the provided relationships for any deprecations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// CheckDeprecationsOnRelationships checks the provided relationships for any deprecations, returning an error if applicable
| ) | ||
| } | ||
|
|
||
| func NewDeprecationError(err error) DeprecationError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
object type, relation, or allowed subject type
| ) | ||
| } | ||
|
|
||
| func NewDeprecationError(err error) DeprecationError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also: can we add a test such that if an object type is deprecated, and you attempt to write with it as a subject, it also fails?
| require.True(t, traits.AllowsCaveats) | ||
| require.True(t, traits.AllowsExpiration) | ||
|
|
||
| _, err = vts.PossibleTraitsForSubject("unknown", "user") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add calls to the deprecation methods in the other tests here, and make sure they all return false/nil/empty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
pkg/schema/typesystem.go
Outdated
| func (ts *TypeSystem) GetDeprecationForAllowedRelation( | ||
| ctx context.Context, | ||
| resourceNamespace string, | ||
| resourceRelation string, | ||
| subjectNamespace string, | ||
| subjectRelation string, | ||
| isWildcard bool, | ||
| hasCaveat bool, | ||
| hasExpiration bool, | ||
| ) (*core.Deprecation, bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth changing this to take in a traits map
| case *core.AllowedRelation_Relation: | ||
| if isWildcard || w.Relation != subjectRelation { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a default here that returns a spiceerrors
pkg/schemadsl/compiler/translator.go
Outdated
| if len(relationsAndPermissions) == 0 { | ||
| ns := namespace.Namespace(nspath) | ||
| var ns *core.NamespaceDefinition | ||
| if slices.Contains(tctx.enabledFlags, "deprecation") && slices.Contains(tctx.allowedFlags, "deprecation") && (deprecationForNamespace != &core.Deprecation{}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move if slices.Contains(tctx.enabledFlags, "deprecation") && slices.Contains(tctx.allowedFlags, "deprecation") && (deprecationForNamespace != &core.Deprecation{}) into a helper method
| } | ||
|
|
||
| // CheckRelationshipDeprecation performs a check over the deprecated relationship's resource, relation, subject and allowed relation, if any | ||
| func (ts *TypeSystem) CheckRelationshipDeprecation(ctx context.Context, relationship tuple.Relationship) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A related call to this might also be useful in Validate() (in this package) if the deprecation is an error state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Validate function is also invoked during WriteSchema, so if we call this in Validate, any attempt to write a schema with this feature containing deprecated resources/relations/subjects would result in an error at schema write time.
7f1a1d8 to
1b3efa7
Compare
|
checking on this |
1b3efa7 to
5b6d6ac
Compare
|
@kartikaysaxena Sorry, I thought this merged. Can you rebase and resolve conflicts? |
0da1a6a to
c9be2e2
Compare
Signed-off-by: Kartikay <[email protected]>
b4f8353 to
d0c7c28
Compare
Signed-off-by: Kartikay <[email protected]> m docs
d0c7c28 to
3408bfa
Compare
|
@kartikaysaxena Checking in on this; I'd like to get it in but it has conflicts |
This PR adds support for relation and namespace level deprecation in schemas.
@deprecated(...)directive to indicate they are deprecated.--enable-experimental-deprecationalong with use directive.Example:
Note
Each relation has a object
Deprecationassigned to them containing an enumDeprecationTypeand optional comments, if unspecified DeprecationType defaults toDEPRECATED_TYPE_UNSPECIFIEDNote
Attempted write with a
DELETEoperation to a deprecated relation/namespace doesn't fail(related #2465)