support also CSI <= 0.3.0

[pohly]

This adds a new function StripSecretsCSI03 that works for messages based on the older versions of the CSI spec. It is in the same package because it shares the same internal implementation. The only difference is the method for determining whether a field is secret, which now just looks at the field name suffix.

While at it, a test for a newer standard converted to an older message also gets added. There was a (low) risk that this might have exposed unknown secrets, but in practice that doesn't happen.

[pohly]

/test pull-sig-storage-csi-lib-utils

[krzyzacy]

it got the webhook but didn't trigger any job

can you close/reopen the PR?

[pohly]

Reopening to trigger testing.

[pohly]

/test pull-sig-storage-csi-lib-utils

[krzyzacy]

sorry, missed kubernetes/test-infra#10309 (comment) in previous review.

[krzyzacy]

/test pull-sig-storage-csi-lib-utils

[pohly]

Yeah, "pull-sig-storage-csi-lib-utils" succeeded! Thanks a lot @krzyzacy.

[msau42]

Is this going to make it difficult to import to 0.3 branches because we would also have to vendor in csi 1.0 spec?

[pohly]

Michelle Au <[email protected]> writes:

Is this going to make it difficult to import to 0.3 branches because we would also have to vendor in csi 1.0 spec?

Good catch. That indeed was a problem. I've updated the branch with a potential solution (copying the V1.0.0 csi.pb.go). Here's a proof-of-concept update for the 0.4 branch of the drivers repo: https://github.com/pohly/drivers/commits/strip-secrets When enabling pruning (which, probably unintentionally, is currently off in the drivers repo), only two files get pulled in: vendor/github.com/kubernetes-csi/csi-lib-utils/ ├── LICENSE └── protosanitizer ├── csi10 │   └── csi.pb.go └── protosanitizer.go

[msau42]

Would it be better to branch this repo and have a 0.3 only version? I worry that by removing the csi spec dependency, then we may upgrade the spec in the future without remembering to upgrade this library first.

[pohly]

Michelle Au <[email protected]> writes:

Would it be better to branch this repo and have a 0.3 only version? I worry that by removing the csi spec dependency, then we may upgrade the spec in the future without remembering to upgrade this library first.

We don't need to update the CSI 1.0 csi.pb.go. The code has to and will keep working even when built with an older copy, because that is exactly the situation that we have with a CSI driver using CSI 1.0.0 and a Kubernetes or CSI sidecar app using something more recent. The "csitest" package was created to test this particular scenario and the test passes.

[pohly]

"Patrick Ohly" <[email protected]> writes:

The "csitest" package was created to test this particular scenario and the test passes.

What we could do when a new CSI spec comes out: - keep protosanitizer/csi10/ unchanged - add protosanitizer/test/csi101 (or whatever the future version is) - add a test case of a message using that spec and convert it with the code compiled against protosanitizer/csi10

[msau42]

IIUC, the function is generic enough to handle any type, so even if a new spec came out with new secret fields, you can still run it through the old function and it would filter out the new fields.

[pohly]

Michelle Au <[email protected]> writes:

IIUC, the function is generic enough to handle any type, so even if a new spec came out with new secret fields, you can still run it through the old function and it would filter out the new fields.

Exactly. That's why it doesn't matter (and must not matter!) which version of the spec is used when building the code. What must not change from now on is the definition of the E_CsiSecret extension, i.e. it's field number and type: https://github.com/container-storage-interface/spec/blob/37e74064635d27c8e33537c863b37ccb1182d4f8/lib/go/csi/csi.pb.go#L4520-L4527 The rest of the csi.pb.go file is irrelevant. Would it be better to make that explicit by copying the definition of that field into protosanitizer.go? Then protosanitizer/csi10/csi.pb.go can be moved to protosanitizer/test/csi10/.

[pohly]

"Patrick Ohly" <[email protected]> writes:

The rest of the csi.pb.go file is irrelevant. Would it be better to make that explicit by copying the definition of that field into protosanitizer.go?

I think it is, see the new commit 1e77671

[msau42]

Thanks!
/lgtm

[saad-ali]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pohly, saad-ali

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 [saad-ali]

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

[saad-ali]

Remember to cherry pick this to release-1.0 branch. And we can cut a new v0.1.1 tag.

[pohly]

@saad-ali Why do we need a release-1.0 branch? We can tag current master as v0.1.1, there's nothing in it that can't be in the next release.

[msau42]

@pohly can you cherry-pick this into the release 0.1 branch? You're correct that we want this in master too, but we may want to include more things in master before we cut the next release

[pohly]

My point is this: we don't need the release 0.1 branch. If you want a release v0.1.1 that is "v0.1.0 + this fix", then just tag 5853414 as v0.1.1 and we are done.

If we ever get into the situation where we want a release that can't be cut from a revision on the master branch, then (and only then) can we start creating a branch and cherry-pick specific changes into that branch. Until then, maintaining release branches is just unnecessary extra work.

[pohly]

The same approach can also be used in all other repos. In external-attacher external-provisioner drivers external-snapshotter, the only difference between master and the release-1.0 is the modification for the canary image name and the hostpathplugin fix for blockvolumes that still needs to be cherry-picked into the release-1.0 branch.

I wrote and tested some new build rules today where it is no longer necessary to change the canary image name, see kubernetes-csi/external-attacher#109.

[msau42]

xref: kubernetes-csi/external-provisioner#82