kubernetes-sigs
The boolean "TrueField" introduced for CORS can cause generator issues
When updating gateway-api-rs to Gateway API v1.3.0, the generators failed for the experimental version of HTTPRoute due to a boolean enum in the spec that was introduced for CORS.
properties:
cors:
description: |-
CORS defines a schema for a filter that responds to the
cross-origin request based on HTTP response header.
Support: Extended
properties:
allowCredentials:
description: |-
AllowCredentials indicates whether the actual cross-origin request allows
to include credentials.
The only valid value for the `Access-Control-Allow-Credentials` response
header is true (case-sensitive).
If the credentials are not allowed in cross-origin requests, the gateway
will omit the header `Access-Control-Allow-Credentials` entirely rather
than setting its value to false.
Support: Extended
enum:
- true
type: boolean
More details are available in https://github.com/kube-rs/gateway-api-rs/issues/144.
I suggest we change the type away from boolean as part of #3756.
cc @lianglli @EyalPazz @robscott @youngnick @mlavacca
A string would require a user to write "true" which seems like a regression in user experience and likely source of errors/confusion?
A string would require a user to write
"true"which seems like a regression in user experience and likely source of errors/confusion?
If we decided that we wanted to do a string (which I explicitly avoided recommending above in anticipation of some problems with that), yes we would need to make sure to clearly highlight the change as backwards incompatible in the v1.4.0 release 👍
A string would require a user to write
"true"which seems like a regression in user experience and likely source of errors/confusion?Yes, we would need to make sure to clearly highlight the change as backwards incompatible in the
v1.4.0release 👍
I am not talking about compatibility, I am talking about long term. Representing booleans as a string is quite confusing for a user -- picking a bool to represent a bool was not an accident but an intentional shift from the original GEP because of the confusion this caused; it was originally merged as string!
If you are changing the serialization of the field then you really should be picking a new name for the field. If you change this to a string then any generated client that is on the older version of the API will not be able to decode data from newer clients and will panic.
Changing to a string doesn't really help the issue of the API design here.
Have you considered making an enum string field that has meaningful behavioural descriptions for the values.
Would the following make more sense? (and be able to be expanded in the future with a Disallowed value if that made sense?)
cors:
credentials: Allowed | Omitted (|Disallowed)
@JoelSpeed in 99% of cases I would agree to use an enum like this but in this case, given the API we are configuring is Access-Control-Allow-Credentials: true (and there is no other possible value), IMO anything other than true is actually making things more confusing.
As a user, I know I want the header Access-Control-Allow-Credentials: true to be set. Nothing will be more clear than setting allowCredentials: true in the HTTPRoute IMO.
/api-review
Please include API reviewers on the conversation so we can standardize this situation and be consistent across apis
/assign @liggitt @deads2k @thockin
Booleans in API are not forbidden, but we often end up with multiple booleans which form a sort of non-orthogonal chain, which would have been better represented as an enum.
I don't mean "true", "false", "idunno" (booleans-in-strings are gross), I mean changing the question so that "true" is implied by the answer. E.g. CORSMode: Strict. I honestly don't know if that makes sense here, I'm just clarifying the convention.
To clarify the constraints we're working with here, we're trying to express if/how the Access-Control-Allow-Credentials header is set. The only valid value of that header is true.
I think a variation of @JoelSpeed's idea could work reasonably well:
allowCredentialsHeader: Set | Omitted
My concern with that approach is that it feels a bit less clear than what we have today, although it would likely be easier for tooling like gateway-api-rs to handle.
We also considered using a normal boolean value here where false would be interpreted as unset, but that would be rather unintuitive as allowCredentials: false seems to suggest that we're setting the header to false, which is not valid.
With that said, I tend to agree with @howardjohn that the current approach is the least bad option - you can either leave the field unset, or set it to true, directly matching the behavior of the header we're trying to configure.
The only valid value of that header is true.
Can we guarantee that until the end of time, this statement is true? Would it be a breaking change to add a new value in what is effectively an upstream API? And if they did decide to add a new value, how could this API be designed in a way that we wouldn't have to make a breaking change?
(FWIW I appreciate this is a standard and the likelihood of this changing is close to zero, but, these are the questions I'd be thinking through in any other scenario).
We also considered using a normal boolean value here where false would be interpreted as unset, but that would be rather unintuitive as allowCredentials: false seems to suggest that we're setting the header to false, which is not valid.
In other parts of your API, do you have ways for users to express a specific opinion of "I don't want this"?
Take for example a cluster admin who is setting policy for their cluster, they might write a MutatingAdmissionPolicy that always adds allowCredentials: true to any HTTPRoute that doesn't set it. This might make sense in the general case, but then a particular workload admin comes along and says hey, I don't want this.
In an API where you have a specific way to opt-out, they could set the explicit "nope" value and then the cluster admins policy enforcement could be skipped, but in this API there is no "nope" value, so they wouldn't have the opportunity to do so, and would have to modify the admins policy to add some way to make an exception.
(Again, not sure how much this applies to this API, but in the general case it can be useful for optional fields to distinguish between no opinion and an explicit zero/disable value)
(FWIW I appreciate this is a standard and the likelihood of this changing is close to zero, but, these are the questions I'd be thinking through in any other scenario).
FWIW In the event it does (unlikely, but possible) we can have true | "some string" as an enum as well. Though it would be less ideal than Set | Omitted or similar
We also considered using a normal boolean value here where false would be interpreted as unset, but that would be rather unintuitive as allowCredentials: false seems to suggest that we're setting the header to false, which is not valid.
I don't find it bad if you document that false means the header will not be populated
you can either leave the field unset, or set it to
true
true or "true" ?
If the goal is to have the user specify the literal value, then "true" makes sense (in that any future value would also be a string. If the goal is to have the user specify their intention, then true makes more sense to me.
we can have
true | "some string"as an enum as well
I think in this case you can actually have "true" | "some string" (string) but not true | "some string" (bool and string?). Your type is currently a bool, if you ship it as a bool and later change it to a string, that would be a breaking change.
There seems to at least be some consensus that the current state is problematic, but more discussion will be needed:
/triage needs-information
I'm going to mark this a member of v1.4.0 for now just as a reminder that we should try to have it resolved (one way or another) for that release if we can.
I agree with Joel and Tim that there should be an explicit option to both enable and disable the feature associated with the header Access-Control-Allow-Credentials. Not being able to set a negative value for the field is awkward, and there are issues with the only option for disablement is omission of the field.
The fact that in the HTTP standard the header Access-Control-Allow-Credentials either has value true or is omitted should be an implementation detail to admins using CORS. IMHO, they should be given a boolean switch. FWIW, there is such a switch called allow_credentials in Envoy and allowCredentials in GCP.
Also, I imagine that depending on implementation, having a negative (default or explicit) value of such a switch may have other effects than just omitting the header in responses. For instance, if a client sends a credentialed CORS request despite the fact that credentials are not allowed, the gateway may filter it out or respond with 403 without forwarding it to servers. This is another reason for having allowCredentials with both positive and negative allowed values (and against calling it "allowCredentialsHeader" which focuses on the header only).
Considering this one resolved as the original problem is fixed. Follow up is in #3946.