Improve API docs and validation based on API review #208
Conversation
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BlaineEXE 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 |
k8s-ci-robot
added
the
approved
BlaineEXE
force-pushed
the
improve-api-docs
branch
from
3bb6d46 to
9dde7a8
Compare
k8s-ci-robot
added
the
size/L
| // DriverName represents the name of a driver. | ||
| // Must be 63 characters or less, beginning and ending with an alphanumeric character | ||
| // ([a-z0-9A-Z]) with dashes (-), dots (.), and alphanumerics between. | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=63 | ||
| // +kubebuilder:validation:Pattern=`^[a-zA-Z0-9]([a-zA-Z0-9\-\.]{0,61}[a-zA-Z0-9])?$` | ||
| type DriverName string | ||
|
|
||
| // DriverResourceID represents a unique identifier for a driver bucket or access resource. | ||
| // To prevent misuse, a driver resource ID must be at most 2048 characters and consist only of | ||
| // alphanumeric characters ([a-z0-9A-Z]), dashes (-), dots (.), and underscores (_). | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=2048 | ||
| // +kubebuilder:validation:Pattern=`^[a-zA-Z0-9._-]+$` | ||
| type DriverResourceID string |
There was a problem hiding this comment.
driverName appears ~4 times, and bucketID/accountID ~4 times.
Initially, I extracted them to their own types for reuse, but I didn't want to have to update all code and tests to use cast to types (e.g., string(bucketID)).
We could always do this, but it seemed somewhat more straightforward to copy-paste the validation and stuff 4 times (not that many) rather than define a separate type.
BlaineEXE
force-pushed
the
improve-api-docs
branch
2 times, most recently
from
434d010 to
6ad6c9a
Compare
| // +optional | ||
| // +listType=set | ||
| // +kubebuilder:validation:XValidation:message="protocols list is immutable",rule="self == oldSelf" | ||
| Protocols []ObjectProtocol `json:"protocols,omitempty"` |
There was a problem hiding this comment.
@JoelSpeed you recommended MinProperties for maps. Is there an equivalent recommendation for MinItems for lists, or are maps more susceptible to concerns?
There was a problem hiding this comment.
I'd recommend a minimum on both maps and lists yes, in fact, any type
What you want to avoid is someone persisting the empty object when semantically there is no difference between unset and empty, so in this case, adding a MinItems=1 would be advised
KAL has minlength which should point out where these are missing for you
| // bucketID is the unique identifier for the backend bucket known to the driver. | ||
| // Must be at most 2048 characters and consist only of alphanumeric characters ([a-z0-9A-Z]), | ||
| // dashes (-), dots (.), and underscores (_). | ||
| // +optional | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=2048 | ||
| // +kubebuilder:validation:Pattern=`^[a-zA-Z0-9._-]+$` | ||
| // +kubebuilder:validation:XValidation:message="boundBucketName is immutable once set",rule="self == oldSelf" | ||
| BucketID string `json:"bucketID,omitempty"` |
There was a problem hiding this comment.
For bucket ID's, I'm trying to find the middle groud between a strong formal definition that might conflict with some edge cases between S3/Azure/GCS bucket naming -- versus totally unvalidated input.
Azure has the longest bucket ID limit at 1024 chars, and doubling that to 2048 seemed somewhat reasonable in case drivers want to add extra text based on other things.
Among the 3, the common character set is alphanums, dot, dash, and underscore. I opted to not get too complex here. Limiting chars to these should hinder any code injection type attacks a lot without risking unexpected compatibility in corner cases.
WDYT @JoelSpeed ? @shanduur ?
There was a problem hiding this comment.
Are any of these case sensitive? Should we limit to lowercase alphanums? Otherwise that seems reasonable. Have you looked beyond the existing platforms to other future platforms at all?
There was a problem hiding this comment.
From what I can tell, most backends only want lower case chars for what directly correlates to Bucket/BucketAccess identifiers. However, I could easily foresee a driver wanting to do use compound identifiers for COSI that include a case-sensitive component.
My guess about case-sensitivity comes from the Azure spec which says paths are case sensitive. Being paths, they would also include the / character. I will add a / char to the allowed set, but please let me know if you foresee any issues allowing that char. I am aware that we should likely avoid & and ; to prevent implicit embedding of arbitrary chars like . Backslash \ (e.g., \n) for a similar purpose. I don't know of any concern forward slash / would bring.
We don't have any future platforms in our mind at the moment beyond the Swift object store standard which we have had (and expect) near-zero interest in. A quick search suggests that Swift's rules are comparable to S3, so if we were to add Swift in the future, I don't anticipate needing a change to support it.
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
BlaineEXE
force-pushed
the
improve-api-docs
branch
from
6ad6c9a to
0e22e7d
Compare
|
/retest required |
|
@BlaineEXE: The Use DetailsIn response to this:
Instructions 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. |
BlaineEXE
force-pushed
the
improve-api-docs
branch
from
0e22e7d to
a4d0828
Compare
k8s-ci-robot
added
size/XL
BlaineEXE
changed the title
From API review, make sure all APIs have documentation that allows users to better interact with the API using `kubectl explain`. Ensure all possible input and output values are noted with text explaining what values mean. Also ensure all API fields have validation that prevents abuse. Signed-off-by: Blaine Gardner <blaine.gardner@ibm.com>
BlaineEXE
force-pushed
the
improve-api-docs
branch
from
a4d0828 to
34aab12
Compare
3 participants
From API review, make sure all APIs have documentation that allows users
to better interact with the API using
kubectl explain. Ensure allpossible input and output values are noted with text explaining what
values mean.
Also ensure all API fields have validation that prevents abuse.
Relates to #199
Resolves #168
If merged, we should follow up to ensure returned
bucket_idandaccount_idfrom RPC calls are validated to match the regex decided upon here.