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

Google proto timestamps support #510

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Google proto timestamps support #510

wants to merge 1 commit into from

Conversation

guicassolato
Copy link
Collaborator

@guicassolato guicassolato commented Nov 27, 2024

Adds support for selecting values that match google.golang.org/protobuf/types/known/timestamppb.Timestamp type (i.e. { "seconds": Number, "nanos": Number }) from the Authorization JSON. The source values are converted to their corresponding timestamp string representation in RFC3339 format.

Breaking change:

Users that previously trusted on selectors/CEL expressions such as request.time to resolve to { "seconds": Number, "nanos": Number } shall now expect RFC3339 instead.

Users can still dig into the details of a timestamp value in the middle of an expression or for extracting only one of the parts of the timestamp (e.g. request.time.seconds). However, RFC3339 will be used whenever extracting a value known to be a perfect representation of a timestamp in Google's protobuf format, for example, when building dynamic URLs and request parameters for fetching metadata from external sources, when extending properties of identity objects, and dynamic authorization response attributes (e.g. injected HTTP headers, etc) from these values.

Although google.golang.org/protobuf/types/known/timestamppb.Timestamp declares both seconds and nanos as optional. For the conversion to RFC3339 to trigger, at least one of these properties must be present in the source value, and no other property other than these two.

@guicassolato guicassolato self-assigned this Nov 27, 2024
…json - converted to rfc3339

Signed-off-by: Guilherme Cassolato <[email protected]>
@guicassolato guicassolato marked this pull request as ready for review November 28, 2024 12:58
@guicassolato guicassolato requested a review from a team November 28, 2024 12:58
@maleck13
Copy link
Collaborator

@guicassolato not sure what we want to do with this given it is a breaking change? When you mention CEL here is that via AuthConfig or AuthPolicy?

@guicassolato
Copy link
Collaborator Author

@guicassolato not sure what we want to do with this given it is a breaking change?

@maleck13, this was an attempt to "hotfix" specifically for the case of the Timestamp data type, before v1 went out.

I still think it is a somewhat valid approach in spite of the breaking change it would introduce indeed. I struggle to believe users are doing anything useful today with the timestamp as-is. Maybe in OPA, but even so that would be such an edge case.

To have in mind this PR modifies objects like request.time but it doesn't affect the scalars within the object (i.e., request.time.seconds and request.time.nanos continues to work as before), so we're very unlikely to hit an usage of this well-known attribute that could cause any damage.

That said, @alexsnaps has shared some concerns and proposed a more holistic strategy to tackle the issue of data types and well-known attributes. I trust him 100% on this. Unless this other strategy turns out not to be possible for any non-technical reason or we find out the issue is (and will ever be!) in fact exclusive of the Timestamp data type, I'm happy to sink the PR and let @alexsnaps guide the fix.

It is also true that this PR, apart from being limited to timestamps, takes a heuristic approach to detect the data type. In the long-term, this is not great. So another reason.

When you mention CEL here is that via AuthConfig or AuthPolicy?

The change would kind of surface to the AuthPolicy, although there is no actual change in the API, neither in the AuthConfig nor in the AuthPolicy. Just the internals.

@alexsnaps
Copy link
Member

My $.02, I don't think we can get out of our way to introduce a breaking change here. Or say at least not for CEL. Arguably this, in CEL again, is a bug. request.time is documented as Timestamp, but clearly isn't. So I'd fix that. Now wrt to JSON & GJSON... well, "Timestamp" isn't a thing and the closest is an RFC3339 formatted string. I'd be in favor of doing that. But we could also keep the "old" representation in the name of backwards compatibility.

The reason CEL should be consistent is that for one all that's needed is there and spec'ed, but it would align the behavior and data representation across all our component that embed a CEL interpreter, so that both data (as protobuf) and AST could be freely exchanged. Which was actually one of the appeal of CEL when we introduced it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ready For Review
Development

Successfully merging this pull request may close these issues.

3 participants