rework BucketAccessClass feature options #222
Conversation
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
the
do-not-merge/work-in-progress
✅ Deploy Preview for container-object-storage-interface ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
approved
BlaineEXE
changed the title
Rework BucketAccessClass feature options based on input from API review. Signed-off-by: Blaine Gardner <[email protected]>
BlaineEXE
force-pushed
the
api-rework-baclass-feature-options
branch
from
6a745fa to
7c3e8ba
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BlaineEXE, shanduur The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| // multiBucketAccess specifies whether a BucketAccess using this class can reference multiple | ||
| // BucketClaims. When unspecified, multi-bucket access is allowed. | ||
| // +optional | ||
| DisallowMultiBucketAccess *bool `json:"disallowMultiBucketAccess,omitempty"` | ||
| MultiBucketAccess MultiBucketAccess `json:"multiBucketAccess,omitempty"` | ||
| } |
There was a problem hiding this comment.
From meeting with Xing:
- Usually, "single" would be the default here (more restrictive) and less restrictive option is optional.
- Alternately, maybe this should be a required input. Having it be explicit could be more clear.
@JoelSpeed do you have recommendations around this API change?
There was a problem hiding this comment.
I think in this case, I would initially say lean towards defaulting this to the more secure option, that would be SingleBucket in this case. What that gives us, is, if we ever decided MultipleBuckets were a sensible new default, it would mean that all existing instances would retain the SingleBucket default option, and we would only affect new instances.
However, there's been a recent discussion around some defaulting where we think we got the default wrong. One issue we typically have with default values in APIs is that we generally don't know if the value was defaulted, or set by a user. This means, if someone disagrees with your default, and would prefer MultipleBuckets be a default on their cluster, it's impossible for them to reliably apply their own default, without overwriting deliberate choices of SingleBucket.
What you could do, is to have the default applied by the controller at read time, and not write it back to the API. The controller would see this value is empty, and apply the SingleBucket behaviour.
This allows cluster admins to apply their own defaults if they wish to do so, it also allows folks who care to explicitly choose SingleBucket if they wanted to, and we will know they deliberately chose it, or folks can just chose MultipleBuckets.
The risk here is that if you were to choose to change the default later, some folks may not have expected that the value changes underneath them between releases, and in this case, that would be a security bad.
In OpenShift we typically call this out with the following message:
When omitted, this means no opinion and the platform is left to choose a reasonable default, which is subject to change over time. The current default is `___`.
This latter option may be a good choice if you're extremely confident we will never change the default
4 participants
Rework BucketAccessClass feature options based on input from API review.
Review comment: #199 (comment)
Joel and Xing both seem to have trouble with the "feature options" wording, so I opted to try moving this to the top level, like PVCs.
Once we agree on the API here, I will update the rest of COSI's internal code and the KEP to match