Attach Detach Controller CRD validations #68406
Conversation
k8s-ci-robot
added
release-note
k8s-ci-robot
added
the
sig/api-machinery
|
Thanks for the pull request. The minimum required is that we check the types of all the fields defined in the go types to ensure we can decode into the go types successfully. @lavalamp requested the CRD be defined in a yaml file in the original API PR, and I agree with that request. Reviewing and tracking changes to API type definitions made in code like this is difficult and error prone. Additionally, that yaml file should be in a package covered by API review and approval /assign @lavalamp |
|
/cc @saad-ali |
|
@liggitt -- Thanks for your comments!
I believe we have done that -- but let's check (again) once we've moved the CRD definition into yaml.
OK. Sorry, we didn't see that. Could you point us to that request? We try to change the PR so that it loads the CRD definition from a file.
I am not sure what that exactly means. Could you please add more details on what that means for our PR? Thanks! /cc @mariantalla |
|
@hoegaarden: GitHub didn't allow me to request PR reviews from the following users: mariantalla. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In 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/test-infra repository. |
#67803 (comment) and again at #68159 (comment)
The api definitions should be in a folder with an OWNERS file that looks something like: options:
no_parent_owners: true
reviewers:
- api-reviewers
- … any sig-storage reviewers desired
approvers:
- api-approvers |
|
@LiGgit @hoegaarden - since the related issue is in the 1.12 milestone, can you make sure the labels match? Otherwise this will hang in CI as we're in code freeze.
|
There was a problem hiding this comment.
thanks for the PR! @hoegaarden
added some minor styling and copy edit comments.
|
|
||
| if err == nil { | ||
| glog.Infof("CSIDrivers CRD created successfully: %#v", | ||
| res) |
There was a problem hiding this comment.
please move this on the previous line. [1]
| res) | ||
| } else if apierrors.IsAlreadyExists(err) { | ||
| glog.Warningf("CSIDrivers CRD already exists: %#v, err: %#v", | ||
| res, err) |
There was a problem hiding this comment.
[1]
also, just %v is fine for error
| res, err) | ||
| } else { | ||
| glog.Errorf("failed to create CSIDrivers CRD: %#v, err: %#v", | ||
| res, err) |
There was a problem hiding this comment.
[1]
also, just %v is fine for error
| t.Fatal("Expected to find validator for 'spec.attachRequired'") | ||
| } | ||
| if attachRequired.Type != "boolean" { | ||
| t.Fatalf("Expected 'spec.attachRequired' to have a type validator for boolean, got %s", attachRequired.Type) |
| t.Fatal("Expected to find validator for 'csiDrivers'") | ||
| } | ||
| if drivers.Type != "array" { | ||
| t.Fatalf("Expected 'csiDrivers' to have a type validator for array, got %s", drivers.Type) |
|
|
||
| driver := schema.Properties["driver"] | ||
| if driver.Type != "string" { | ||
| t.Fatalf("Expected 'csiDrivers[].driver' to have a type validator for string, got %s", driver.Type) |
|
|
||
| nodeID := schema.Properties["nodeID"] | ||
| if nodeID.Type != "string" { | ||
| t.Fatalf("Expected 'csiDrivers[].nodeID' to have a type validator for string, got %s", nodeID.Type) |
|
|
||
| topologyKeys := schema.Properties["topologyKeys"] | ||
| if topologyKeys.Type != "array" { | ||
| t.Fatalf("Expected 'csiDrivers[].topologyKeys' to have a type validator for array, got %s", topologyKeys.Type) |
| } | ||
| topologyKeysSchema := topologyKeys.Items.Schema | ||
| if topologyKeysSchema.Type != "string" { | ||
| t.Fatalf("Expected 'csiDrivers[].topologyKeys[]' to have a type validator for string, got %s", topologyKeysSchema.Type) |
| "attachRequired": apiextensionsv1beta1.JSONSchemaProps{ | ||
| Description: "Indicates this CSI volume driver requires an attach operation," + | ||
| " and that Kubernetes should call attach and wait for any attach operation to" + | ||
| " complete before proceeding to mounting.", |
There was a problem hiding this comment.
complete before proceeding to mounting -> complete before proceeding to mount
?
|
/kind bug also, you can run |
k8s-ci-robot
added
kind/bug
|
/milestone v 1.12 @hoegaarden please verify and rebase. merging this would make me so happy. :) |
|
@guineveresaenger: The provided milestone is not valid for this repository. Milestones in this repository: [ Use In 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/test-infra repository. |
k8s-ci-robot
added
the
priority/critical-urgent
|
@guineveresaenger |
|
/milestone v1.12 (missed that earlier) @hoegaarden ack - thank you! |
|
Thanks for working on this @hoegaarden. Ping me on slack when it is ready to review.
A separate yaml file would be nice to have, but for the sake of release blocker bug, not a requirement (no user visible impact) -- if it is too much work, we can split the file out in the next release, if needed. |
Specifically test: - nested elements - arrays
k8s-ci-robot
removed
the
needs-rebase
|
/test pull-kubernetes-bazel-build |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hoegaarden, saad-ali If they are not already assigned, you can assign the PR to them by writing 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:
Approvers can indicate their approval by writing |
| // CSIDriver generates a CRD captureing information about a Container Storage Interface (CSI) | ||
| // volume driver deployed on the cluster. | ||
| func CSIDriver() *apiextensionsv1beta1.CustomResourceDefinition { | ||
| return &apiextensionsv1beta1.CustomResourceDefinition{ |
There was a problem hiding this comment.
The source of this really needs to be YAML. I don't care for the moment if it's embedded in a go string or stored in a file. But it must be YAML for review.
Actually I think you need to go one step further: generate the YAML from a types.go file and check that in. Humans really need to review that source.
|
Followed up with @lavalamp @verult @msau42 offline. We decided that for both #68260 and #68490 we are just going to remove automatic CRD installation by Attach/Detach controller. For alpha, user will have to manually install the CRD. In the next release we will come up with a proper mechanism for installing CRDs Details here: #68519 (comment) |
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #68260
Special notes for your reviewer:
We are not sure if those validations are narrow enough -- we don't set any of the fields as mandatory, we do no specify any REs, min/max lengths, ...
Release note:
/assign @kubernetes/sig-storage-pr-reviews