API Review PR #199

BlaineEXE · 2025-12-05T17:56:03Z

This PR is not intended to be merged. I have merely copied the current WIP API that is proposed in the v1alpha2 KEP PR to solicit more technical feedback.

This PR will not be merged directly, but feedback will be taken and made in other PRs.

k8s-ci-robot · 2025-12-05T17:56:05Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

netlify · 2025-12-05T17:56:08Z

✅ Deploy Preview for container-object-storage-interface ready!

Name	Link
🔨 Latest commit	`914c1d4`
🔍 Latest deploy log	https://app.netlify.com/projects/container-object-storage-interface/deploys/69331cb5f6a0ad0008a2983c
😎 Deploy Preview	https://deploy-preview-199--container-object-storage-interface.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

k8s-ci-robot · 2025-12-05T17:56:09Z

[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

Needs approval from an approver in each of these files:

~~OWNERS~~ [BlaineEXE]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

JoelSpeed

I stopped at a point here as I think a lot of what I've said I'm now repeating for fields further on the review. I'd recommend implementing Kube-api-linter here to help with reminders on best practices (using omitempty everywhere, setting min/max lengths consistently)

Please review my feedback and apply anywhere it seems repeated across the API surface in this PR

JoelSpeed · 2025-12-08T11:44:36Z

client2/apis/objectstorage/v1alpha2/bucket_types.go

+// +kubebuilder:validation:XValidation:message="protocols list is immutable",rule="has(oldSelf.protocols) == has(self.protocols)"
+// +kubebuilder:validation:XValidation:message="existingBucketID is immutable",rule="has(oldSelf.existingBucketID) == has(self.existingBucketID)"
+type BucketSpec struct {
+	// driverName is the name of the driver that fulfills requests for this Bucket.


Have you considered what valid data for this field looks like? What is the possible format of a driver name? Must it conform to any pattern (e.g. must it be a valid K8s resource name?)

This should probably be in line with other Kubernetes names, following RFC1123.

JoelSpeed · 2025-12-08T11:44:54Z

client2/apis/objectstorage/v1alpha2/bucket_types.go

+type BucketSpec struct {
+	// driverName is the name of the driver that fulfills requests for this Bucket.
+	// +required
+	// +kubebuilder:validation:MinLength=1


What about a maximum length?

JoelSpeed · 2025-12-08T11:46:41Z

client2/apis/objectstorage/v1alpha2/bucket_types.go

+	// Possible values:
+	//  - Retain: keep both the Bucket object and the backend bucket
+	//  - Delete: delete both the Bucket object and the backend bucket


I'd recommend trying kubectl explain on this field. I don't think this will render as it is written here.

I generally recommend full sentences over bullets for this reason.

Suggested change

// Possible values:

// - Retain: keep both the Bucket object and the backend bucket

// - Delete: delete both the Bucket object and the backend bucket

// Valid values are Retain and Delete.

// When set to Retain, both the Bucket object and the backend bucket will be kept upon deletion.

// When set to Delete, both the Bucket object and the backend bucket will be removed upon deletion.

JoelSpeed · 2025-12-08T11:50:34Z

client2/apis/objectstorage/v1alpha2/bucket_types.go

+	// parameters is an opaque map of driver-specific configuration items passed to the driver that
+	// fulfills requests for this Bucket.


What is a driver here? How would I know what valid options are? Is this CLI args, or a config file?

What happens if a bucket needs more flexibility for configuration than string to string allows?

Those are open options - how they are used it's up to the driver's implementation details, and should be documented in the specific driver's documentation. If one needs to have more flexibility here, they can use JSON/YAML in strings or reference config maps or secrets through that field.

So the driver will effectively come up with a schema here and will be given a raw blob of data? Or how are you passing the data to the driver? Do you have any more context on this?

I'm wondering if a map here is the right thing to do, vs just being a raw blob of unstructured data

This reminds me of the early days of cluster API, we found raw data to be awkward and eventually switch to references to concrete CRs. Have you seen this pattern before? Or considered it here?

It would mean that each driver has its own config type that can be properly structured, can be evolved over time and provide proper validations

One of the goals of COSI is to do our best to balance familiarity with StorageClass/PV/PVC concepts against new designs that allow better usability. This is a case where COSI has borrowed the same opaque map concept from StorageClass.

My gut feel here is that this is still sufficient for block and file storage and that it will probably be slightly more beneficial to keep similar semantics. However, I will make a note to ask in the sig-storage group if there have been discussions to change this and solicit feedback.

This reminds me of the early days of cluster API, we found raw data to be awkward and eventually switch to references to concrete CRs. Have you seen this pattern before? Or considered it here?

Yes, and I don't like this design from user's perspective. Balancing few different vendors and CAPI itself is painful, especially if you want to keep up to date. I'd like to keep it simple, using k/v pairs as here.

I spoke to sig-storage folks about this topic today, and they reported that the opaque parameter map has worked well for some time for StorageClasses, PVCs, and CSI. Unless additional info comes up here, the takeaway is that COSI and sig-storage prefer to keep this as-is to rely on user/driver/admin familiarity from CSI.

I consider this resolved, but I'll opt to leave this thread open because I think it will be a good reference in the future. Thanks again, Joel.

JoelSpeed · 2025-12-08T11:53:49Z

client2/apis/objectstorage/v1alpha2/bucket_types.go

+	// protocols lists object store protocols that the provisioned Bucket must support.
+	// If specified, COSI will verify that each item is advertised as supported by the driver.


What are the valid protocol types? Should include the valid options here to that users can see from the docs what the valid options are

Can this be set through // +kubebuilder:validation:Enum, right? Do we need this separately in the comment though?

Can this be set through // +kubebuilder:validation:Enum, right?

Yes, though I thought this was already done?

Most end user documentation is generated only from the godoc. It doesn't include the enum or other validations from the schema. Which means that when a user reads the docs for the field (e.g. kubectl explain), they would have no idea what the valid options are unless we tell them in the godoc

JoelSpeed · 2025-12-08T12:21:16Z

client2/apis/objectstorage/v1alpha2/bucketaccess_types.go

+	// +kubebuilder:validation:MinLength=1
+	// +kubebuilder:validation:MaxLength=253


DNS 1123 subdomain feedback? Bucket here is a kube object or an external object?

JoelSpeed · 2025-12-08T12:21:50Z

client2/apis/objectstorage/v1alpha2/bucketaccess_types.go

+	// +kubebuilder:validation:MinLength=1
+	// +kubebuilder:validation:MaxLength=253


DNS 1123 subdomain feedback here

JoelSpeed · 2025-12-08T12:23:25Z

client2/apis/objectstorage/v1alpha2/protocols.go

+// Non-relevant vars will not be present, even when marked "Required".
+// Vars are used as data keys in BucketAccess Secrets.
+// Vars must be all-caps and must begin with `COSI_`.
+type CosiEnvVar string


Why represent all of these as env vars in your API vs something more structured?

Oof. This is a question. 😅

In v1a1, we had this as a structured type that was rendered into a JSON blob in the Secret that users use to consume the info. In the best case, an application would read the file directly. In usual cases, an init container with jq would process the file into something that the main container would use.

However, we got feedback from a few users that there are scenarios where this just wasn't possible, and users instead wanted individual Secret fields for each item.

These are fields that must remain secret, so we can't encode them into a status. We have to use a secret.

So this is our best attempt to have some reliable structure for end users within Secret data fields.

If you have other suggestions, we will be happy to consider them. I can't say I love this approach, but it's the best we've been able to come up with given our constraints.

JoelSpeed · 2025-12-08T12:24:39Z

client2/apis/objectstorage/v1alpha2/bucket_types.go

+	// BucketAccess Secret. e.g., COSI_S3_ENDPOINT, COSI_AZURE_STORAGE_ACCOUNT.
+	// This should not contain any sensitive information.
+	// +optional
+	BucketInfo map[string]string `json:"bucketInfo,omitempty"`


Might be better as a list of named objects, easier to apply validation and allow SSA style writes

bucketInfo: - property: Blah value: something

JoelSpeed · 2025-12-08T12:29:40Z

client2/apis/objectstorage/v1alpha2/bucketaccessclass_types.go

+
+	// featureOptions can be used to adjust various COSI access provisioning behaviors.
+	// +optional
+	FeatureOptions BucketAccessFeatureOptions `json:"featureOptions,omitempty"`


Feature options feels a bit odd here, why not make this more about bucket access. Also, try to avoid the bool here

bucketAccess: <- you can probably come up with something better? disallowedModes: ["S3", ... ] concurrency: Singular | Multi

BlaineEXE · 2025-12-09T17:43:34Z

Thanks, Joel, for the very thorough review so far and the help understanding the suggestions. I am beginning my work to resolve the issues by finishing the open issue to run kube-api-linter and address those issues. After that, I'll keep addressing the minor points bit by bit, and I will begin discuss the bigger questions like ReadyToUse/TimestampedError/Conditions with sig-storage leadership later this week.

placeholder commit for purposes of reviewing API in PR

914c1d4

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 5, 2025

k8s-ci-robot requested a review from saad-ali December 5, 2025 17:56

k8s-ci-robot requested a review from shanduur December 5, 2025 17:56

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 5, 2025

BlaineEXE changed the title ~~placeholder commit for purposes of reviewing API in PR~~ API Review PR (placeholder) Dec 5, 2025

BlaineEXE changed the title ~~API Review PR (placeholder)~~ API Review PR Dec 5, 2025

JoelSpeed reviewed Dec 8, 2025

View reviewed changes

BlaineEXE mentioned this pull request Dec 9, 2025

add kube-api-linter and resolve API issues #201

Merged

		// parameters is an opaque map of driver-specific configuration items passed to the driver that
		// fulfills requests for this Bucket.

		// protocols lists object store protocols that the provisioned Bucket must support.
		// If specified, COSI will verify that each item is advertised as supported by the driver.

		// +kubebuilder:validation:MinLength=1
		// +kubebuilder:validation:MaxLength=253

API Review PR #199

Are you sure you want to change the base?

API Review PR #199

Conversation

BlaineEXE commented Dec 5, 2025

Uh oh!

k8s-ci-robot commented Dec 5, 2025

Uh oh!

netlify bot commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for container-object-storage-interface ready!

Uh oh!

k8s-ci-robot commented Dec 5, 2025

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BlaineEXE commented Dec 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

netlify bot commented Dec 5, 2025 •

edited

Loading