Hi! While investigating #595, I noticed a related bug in isControllerCapabilitySupported that affects any driver that exposes zero controller capabilities.
What happened?
pkg/sanity/controller.go#L88-L107:
caps, err := c.ControllerGetCapabilities(
context.Background(),
&csi.ControllerGetCapabilitiesRequest{})
Expect(err).NotTo(HaveOccurred())
Expect(caps).NotTo(BeNil())
Expect(caps.GetCapabilities()).NotTo(BeNil())
for _, cap := range caps.GetCapabilities() {
...
}
ControllerGetCapabilitiesResponse.capabilities is a repeated protobuf field. In proto3, an empty repeated field and an unset repeated field are encoded identically on the wire. Both produce zero bytes for that field. After the response is decoded in Go, a driver that returns zero controller capabilities may be observed by the client as a nil slice, regardless of whether the server set Capabilities to nil or to []*ControllerServiceCapability{}.
This makes Expect(caps.GetCapabilities()).NotTo(BeNil()) too strict. A nil slice does not indicate a failed RPC or an invalid response. It can simply mean the driver returned no controller capabilities. The loop immediately below already handles this case correctly, since ranging over a nil slice performs zero iterations.
What did you expect to happen?
A driver that registers a Controller service but advertises zero controller capabilities should pass isControllerCapabilitySupported (it would correctly return false for any cap type) rather than fail the suite in BeforeEach.
How to reproduce it
- Implement a CSI driver that registers a
ControllerServer whose ControllerGetCapabilities returns &csi.ControllerGetCapabilitiesResponse{} (or &csi.ControllerGetCapabilitiesResponse{Capabilities: []*csi.ControllerServiceCapability{}}, which serialize identically).
- Run
sanity.GinkgoTest against it.
- Any spec that calls
isControllerCapabilitySupported fails at the Expect(caps.GetCapabilities()).NotTo(BeNil()) assertion.
Anything else we need to know?
Proposed fix
Remove the line:
Expect(caps.GetCapabilities()).NotTo(BeNil())
The helper would still validate that the RPC succeeded and that the response itself is non-nil. The for-loop directly below already handles a nil slice correctly.
Environment
csi-test version: v5.4.0
- CSI spec version: 1.x
- Go version: 1.23
I would be happy to also drive the fix for this!
Hi! While investigating #595, I noticed a related bug in
isControllerCapabilitySupportedthat affects any driver that exposes zero controller capabilities.What happened?
pkg/sanity/controller.go#L88-L107:ControllerGetCapabilitiesResponse.capabilitiesis a repeated protobuf field. In proto3, an empty repeated field and an unset repeated field are encoded identically on the wire. Both produce zero bytes for that field. After the response is decoded in Go, a driver that returns zero controller capabilities may be observed by the client as anilslice, regardless of whether the server setCapabilitiestonilor to[]*ControllerServiceCapability{}.This makes
Expect(caps.GetCapabilities()).NotTo(BeNil())too strict. Anilslice does not indicate a failed RPC or an invalid response. It can simply mean the driver returned no controller capabilities. The loop immediately below already handles this case correctly, since ranging over anilslice performs zero iterations.What did you expect to happen?
A driver that registers a Controller service but advertises zero controller capabilities should pass
isControllerCapabilitySupported(it would correctly returnfalsefor any cap type) rather than fail the suite inBeforeEach.How to reproduce it
ControllerServerwhoseControllerGetCapabilitiesreturns&csi.ControllerGetCapabilitiesResponse{}(or&csi.ControllerGetCapabilitiesResponse{Capabilities: []*csi.ControllerServiceCapability{}}, which serialize identically).sanity.GinkgoTestagainst it.isControllerCapabilitySupportedfails at theExpect(caps.GetCapabilities()).NotTo(BeNil())assertion.Anything else we need to know?
Proposed fix
Remove the line:
The helper would still validate that the RPC succeeded and that the response itself is non-nil. The for-loop directly below already handles a
nilslice correctly.Environment
csi-testversion:v5.4.0I would be happy to also drive the fix for this!