cloudevents / spec

CloudEvents Specification
https://cloudevents.io
Apache License 2.0
5.11k stars 586 forks source link

Add unknown value for authtype enum of Auth Context ext #1246

Closed JU-2094 closed 1 year ago

JU-2094 commented 1 year ago

Issue created for this change: https://github.com/cloudevents/spec/issues/1245

Fixes #

For the recently added extension Auth Context /extensions/authcontext.md the attribute of authtype could be not identified reliable and be unknown.

The key principle here is that authtype classification results should be predictable and should not change. However, for some cases the type is preferred to be unknown when we can't determine reliable. The main concern we have is how authtype classifications might change when we are able to classify the request in the future. As a result if we change from unknown to app_user it is a "breaking" change for API consumers. Since their code may build business logic based on authtype results.

In https://github.com/cloudevents/spec/blob/main/cloudevents/documented-extensions.md , this already mentioned:

The attributes defined in this document have no official standing and might be changed, or removed, at any time. As such, inclusion of an attribute in this document does not need to meet the same level of maturity, or popularity, as attributes defined in the CloudEvents specification.

For this reason, the proposal is to add unknown enum value for authtype.

Alternative:

Cons: The #1247 ensures all extensions have at least one REQUIRED field. So clients can determine if an extension is present.

Proposed Changes

Release Note

Add `unknown` value for `authtype` enum of Auth Context extension
JU-2094 commented 1 year ago

In addition with that change, We could add the constraint that that authid OR authtype MUST be present; if we don't want clients to have to check the attributes to see if an extension is present. I would like feedback about this, otherwise I'm ok with the current version of only changing the constraint to optional.

duglin commented 1 year ago

ping @inlined for comment

inlined commented 1 year ago

Spoke internally. I'm surprised that we might not know the authtype, but I accept it as a technical fact. My only request was that we declare some subset of fields of which one must be required so that it simplifies checking wether a given event is using this extension.

duglin commented 1 year ago

@inline - any thoughts on:

We could add the constraint that that authid OR authtype MUST be present; if we don't want clients to have to check the attributes to see if an extension is present.

Or, would it be better to define an "unknown" enum value and keep it REQUIRED? This seems the easiest to know if the extension is being used.

JU-2094 commented 1 year ago

Other extensions have all their attributes as optional (e.g. https://github.com/cloudevents/spec/blob/main/cloudevents/extensions/dataref.md) ,

In case we add this unknown type, some considers as a breaking change if the type changes once is reliably determined. Do you have references/guidelines in this repo for these type of behaviors?

duglin commented 1 year ago

Other extensions have all their attributes as optional...

Funny you mentioned this because I was going to open a PR tomorrow to fix that and try to make it so all extensions have at least one REQUIRED attribute.

re: breaking changes... we state:

The attributes defined in this document have no official standing and might be changed, or removed, at any time. As such, inclusion of an attribute in this document does not need to meet the same level of maturity, or popularity, as attributes defined in the CloudEvents specification.

in https://github.com/cloudevents/spec/blob/main/cloudevents/documented-extensions.md

duglin commented 1 year ago

See https://github.com/cloudevents/spec/pull/1247

JU-2094 commented 1 year ago

Given #1247, and that it is stated that the extension attributes have no official standing and might be changed, or removed, at any time.

I'm good with adding the unknown to the existing authtype enum values. And keep authtype as REQUIRED to keep it consistent with #1247

I'll change the files/commits. Let me know if there's pushback or other comments.

duglin commented 1 year ago

@inlined you ok with the latest?

inlined commented 1 year ago

Sounds good to me

duglin commented 1 year ago

Approved on the 11/16 call