Add new reserved prefixed parameter keys which are stripped out of parameter list, add deprecation notice for old keys and keep their behavior the same #178

davidz627 · 2018-11-29T23:25:36Z

Add new reserved prefixed parameter keys which are stripped out of parameter list:

csiParameterPrefix = "csi.storage.k8s.io/"
prefixedFsTypeKey = csiParameterPrefix + "FSType"
prefixedProvisionerSecretNameKey      = csiParameterPrefix + "ProvisionerSecretName"
prefixedProvisionerSecretNamespaceKey = csiParameterPrefix + "ProvisionerSecretNamespace"
prefixedControllerPublishSecretNameKey      = csiParameterPrefix + "ControllerPublishSecretName"
prefixedControllerPublishSecretNamespaceKey = csiParameterPrefix + "ControllerPublishSecretNamespace"
prefixedNodeStageSecretNameKey      = csiParameterPrefix + "NodeStageSecretName"
prefixedNodeStageSecretNamespaceKey = csiParameterPrefix + "NodeStageSecretNamespace"
prefixedNodePublishSecretNameKey      = csiParameterPrefix + "NodePublishSecretName"
prefixedNodePublishSecretNamespaceKey = csiParameterPrefix + "NodePublishSecretNamespace"

add deprecation notice for old keys, example:
"fstype" is deprecated and will be removed in a future release, please use "csi.storage.k8s.io/FSType" instead

the behavior will be the same for the old parameter keys, except for the fact that they cannot be specified in conjunction with their respective new prefixed parameter key

/assign @saad-ali @lpabon @jsafrane @msau42 @mkimuram @vladimirvivien

k8s-ci-robot · 2018-11-29T23:25:38Z

@davidz627: GitHub didn't allow me to assign the following users: mkimuram.

Note that only kubernetes-csi members and repo collaborators can be assigned.
For more information please see the contributor guide

In response to this:

all parameters that start with "csi" are interpreted by the external-provisioner as special parameters and they are placed into objects as first class CSI fields. Therefore after they have been "translated" into field they are stripped from the parameters list that gets passed to CreateVolume.

In this vein we add csifstype so that you can specify fstype that the external provisioner will understand and place in to the volume capability, it will then get stripped out of the parameters list.

/assign @saad-ali @lpabon @jsafrane @msau42 @mkimuram @vladimirvivien

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.

davidz627 · 2018-11-29T23:28:18Z

pkg/controller/controller_test.go

+func runProvisionTest(t *testing.T, k string, tc provisioningTestcase, requestedBytes int64) {
+	t.Logf("Running test: %v", k)
+
+	mockController, driver, identityServer, controllerServer, csiConn, err := createMockServer(t)


this refactor was so that we have a new server for each test, so that mock expectations did not carry through from test to test causing hard to debug cascading failures

davidz627 · 2018-11-29T23:28:40Z

pkg/controller/controller_test.go

+	} else {
+		// Setup regular mock call expectations.
+		provisionMockServerSetupExpectations(identityServer, controllerServer)
+		if !tc.expectErr {


I also added that we should not expect create volume when test is supposed to error

saad-ali · 2018-11-30T00:20:10Z

I support this change. This reserves StorageClass parameters with keys starting with csi... for use by the CSI external-provisioner and prevents them from being passed through to the CSI driver in the CreateVolume call.

mkimuram · 2018-11-30T16:20:09Z

@davidz627

Is my understanding right that the issue is caused by that csi external provisioner passes all the storage class parameters to csi driver's CreateVolume, as a result, parameters that are not specific to csi driver, such as fsType and secrets for csi, is passed to csi driver?

As spec describes as below, I agree that csi plugins should check if invalid parameters are not passed like this, and then above behavior will make csi driver regard such parameters as invalid.

  // Plugin specific parameters passed in as opaque key-value pairs.
  // This field is OPTIONAL. The Plugin is responsible for parsing and
  // validating these parameters. COs will treat these as opaque.

To solve this issue, we also have an option to strip fsType in addition to parameters that is prefixed with csi, instead of introducing new csiFsType. Pros and Cons will be as below, so if we don't need to prioritize compatibility, implementation of this PR will be better.

Pros:

Compatible with csi v0 driver on how to specify fstype in storage class parameters
Compatible with in-tree driver or non csi external provisioner on how to specify fstype in storage class parameters

Cons:

Introduce an irregular case for handling csi's storage class parameters (Assuming that this PR intends to make a rule that storage class parameters prefixed with "csi" are not for each csi driver in k8s.)

pkg/controller/controller.go

davidz627 · 2018-11-30T23:48:16Z

@saad-ali @lpabon @jsafrane @msau42 @mkimuram @vladimirvivien
new version based on morning CSI standup discussion pushed

verult · 2018-12-01T00:12:05Z

/assign

pkg/controller/controller.go

davidz627 · 2018-12-01T00:36:28Z

@saad-ali resolved. The error messages I made more generic because we can't tell if the user used the deprecated or new parameters, unless we want to add more cases :S

verult · 2018-12-01T00:07:36Z

pkg/controller/controller.go

@@ -82,6 +110,40 @@ const (
 	snapshotAPIGroup = snapapi.GroupName // "snapshot.storage.k8s.io"
 )

+var (


What about a function

generateCSIParams(actor, key string, deprecated bool)

and

noActor = "" provisionerActor = "Provisioner" controllerPublishActor = "ControllerPublish" ... secretNameKey = "SecretName" fsTypeKey = "FSType" ... actors = []string{provisionerActor, controllerPublishActor, ...} keys = []string{secretNameKey, fsTypeKey, ...}

how would we associate actors with keys? This ends up just shifting the variable definition into a different type of struct I think. It's unclear what params we're trying to generate here, whats the output of this function?

Sorry the func should be called generateCSIParamKeys and returns an actual storageclass parameter key. The logic would be the same as what you have right now, but encapsulated in a function, so it's easier to read and you don't need a giant list of variables.

I'm trying to see if there's a way to not have a hard-coded list of all the various combinations.

verult · 2018-12-01T00:35:03Z

pkg/controller/controller.go

+		namespaceTemplate = t
+		numNamespace += 1
+	}
+


What if you had a helper function

getKeyFromParams(actor, key string) (val string, isDeprecated bool, err error)

that

Returns the val corresponding to either the deprecated or the existing key if only 1 of them exists (and set isDeprecated accordingly)

Returns an error if both deprecated and existing keys exist

Then the check below becomes something like:

if secretNameIsDeprecated != secretNamespaceIsDeprecated { throwError() } else if secretNameIsDeprecated { ... } else { // !secretNameIsDeprecated && !secretNamespaceIsDeprecated ... }

when passing in a key we already know whether its deprecated or not as we define that so it doesnt make sense to me to have that as a return. Also given one deprecated theres no way to associate that with the "non-deprecated" version of that key without a struct that binds both together (or some hardcoding in this function) which serves the same purpose.

Tried to prototype this piece of logic and yeah, it doesn't look much prettier. OK to keep everything as is!

could you LGTM?

verult · 2018-12-01T00:38:28Z

pkg/controller/controller.go

@@ -702,8 +805,52 @@ func (p *csiProvisioner) volumeHandleToId(handle string) string {
 	return handle
 }

-// getSecretReference returns a reference to the secret specified in the given nameKey and namespaceKey parameters, or an error if the parameters are not specified correctly.
-// if neither the name or namespace parameter are set, a nil reference and no error is returned.
+func getSecretNameAndNamespaceTemplate(secret secretParamsType, storageClassParams map[string]string) (nameTemplate, namespaceTemplate string, err error) {


Going off the suggestions from above, this would take in actor string instead of secret secretParamsType

this just shifts association of deprecatedness/actors from the structs that I created into hardcoded cases in the functions

saad-ali

/approve

LGTM

Will let @verult or @msau42 do final lgtm

k8s-ci-robot · 2018-12-01T00:47:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidz627, 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

pkg/controller/controller.go

verult

Couple nits but LGTM otherwise!

verult · 2018-12-03T21:46:17Z

pkg/controller/controller.go

+	secretNameKey                string
+	secretNamespaceKey           string
+}
+
 const (


nit: Move new consts to a separate file?

refactor in future

pkg/controller/controller.go

pkg/controller/controller_test.go

pkg/controller/controller.go

…rameter list, add deprecation notice for old keys and keep their behavior the same

jingxu97 · 2018-12-04T17:40:18Z

/lgtm

k8s-ci-robot · 2018-12-04T17:40:26Z

@jingxu97: changing LGTM is restricted to assignees, and only kubernetes-csi/external-provisioner repo collaborators may be assigned issues.

In response to this:

/lgtm

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.

verult · 2018-12-04T18:30:36Z

/lgtm

k8s-ci-robot assigned jsafrane, lpabon, msau42, saad-ali and vladimirvivien Nov 29, 2018

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 29, 2018

k8s-ci-robot requested review from childsb and saad-ali November 29, 2018 23:25

davidz627 changed the title ~~Add csifstype paramater, strip all parameters that start with CSI~~ [BREAKING CHANGE] Add csifstype parameter, strip all parameters that start with CSI Nov 29, 2018

davidz627 commented Nov 29, 2018

View reviewed changes

davidz627 mentioned this pull request Nov 29, 2018

Fix volumes e2e test to check fsType kubernetes/kubernetes#70823

Merged

davidz627 force-pushed the feature/stripCSI branch from 2b1cf72 to c9af2e3 Compare November 29, 2018 23:38

davidz627 force-pushed the feature/stripCSI branch from c9af2e3 to bf160c0 Compare November 30, 2018 22:34

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 30, 2018

davidz627 changed the title ~~[BREAKING CHANGE] Add csifstype parameter, strip all parameters that start with CSI~~ Add new reserved prefixed parameter keys which are stripped out of parameter list, add deprecation notice for old keys and keep their behavior the same Nov 30, 2018

davidz627 force-pushed the feature/stripCSI branch from bf160c0 to ed46e81 Compare November 30, 2018 23:20

msau42 reviewed Nov 30, 2018

View reviewed changes

pkg/controller/controller.go Outdated Show resolved Hide resolved

davidz627 force-pushed the feature/stripCSI branch from ed46e81 to 05716f9 Compare November 30, 2018 23:41

davidz627 force-pushed the feature/stripCSI branch from 05716f9 to 0ac9db9 Compare November 30, 2018 23:59

k8s-ci-robot assigned verult Dec 1, 2018

saad-ali requested changes Dec 1, 2018

View reviewed changes

pkg/controller/controller.go Show resolved Hide resolved

pkg/controller/controller.go Outdated Show resolved Hide resolved

pkg/controller/controller.go Outdated Show resolved Hide resolved

pkg/controller/controller.go Outdated Show resolved Hide resolved

davidz627 force-pushed the feature/stripCSI branch from 0ac9db9 to 2b53902 Compare December 1, 2018 00:35

verult reviewed Dec 1, 2018

View reviewed changes

saad-ali approved these changes Dec 1, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 1, 2018

davidz627 force-pushed the feature/stripCSI branch from 2b53902 to b29e000 Compare December 1, 2018 00:48

xing-yang mentioned this pull request Dec 1, 2018

Add new reserved prefixed parameter keys kubernetes-csi/external-snapshotter#78

Merged

jingxu97 reviewed Dec 4, 2018

View reviewed changes

pkg/controller/controller.go Outdated Show resolved Hide resolved

verult reviewed Dec 4, 2018

View reviewed changes

jingxu97 reviewed Dec 4, 2018

View reviewed changes

pkg/controller/controller.go Show resolved Hide resolved

jingxu97 reviewed Dec 4, 2018

View reviewed changes

pkg/controller/controller.go Outdated Show resolved Hide resolved

davidz627 force-pushed the feature/stripCSI branch from b29e000 to 452a401 Compare December 4, 2018 00:54

Add new reserved prefixed parameter keys which are stripped out of pa…

26305f2

…rameter list, add deprecation notice for old keys and keep their behavior the same

davidz627 force-pushed the feature/stripCSI branch from 452a401 to 26305f2 Compare December 4, 2018 01:01

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 4, 2018

k8s-ci-robot merged commit b52f481 into kubernetes-csi:master Dec 4, 2018

davidz627 deleted the feature/stripCSI branch December 4, 2018 18:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new reserved prefixed parameter keys which are stripped out of parameter list, add deprecation notice for old keys and keep their behavior the same #178

Add new reserved prefixed parameter keys which are stripped out of parameter list, add deprecation notice for old keys and keep their behavior the same #178

davidz627 commented Nov 29, 2018 •

edited

Loading

k8s-ci-robot commented Nov 29, 2018

davidz627 Nov 29, 2018 •

edited

Loading

davidz627 Nov 29, 2018

saad-ali commented Nov 30, 2018

mkimuram commented Nov 30, 2018

davidz627 commented Nov 30, 2018

verult commented Dec 1, 2018

davidz627 commented Dec 1, 2018

verult Dec 1, 2018

davidz627 Dec 1, 2018

verult Dec 1, 2018

verult Dec 1, 2018

verult Dec 1, 2018

davidz627 Dec 1, 2018

verult Dec 1, 2018

davidz627 Dec 3, 2018

verult Dec 1, 2018

davidz627 Dec 1, 2018

saad-ali left a comment

k8s-ci-robot commented Dec 1, 2018

verult left a comment

verult Dec 3, 2018

davidz627 Dec 4, 2018

jingxu97 commented Dec 4, 2018

k8s-ci-robot commented Dec 4, 2018

verult commented Dec 4, 2018

Add new reserved prefixed parameter keys which are stripped out of parameter list, add deprecation notice for old keys and keep their behavior the same #178

Add new reserved prefixed parameter keys which are stripped out of parameter list, add deprecation notice for old keys and keep their behavior the same #178

Conversation

davidz627 commented Nov 29, 2018 • edited Loading

k8s-ci-robot commented Nov 29, 2018

davidz627 Nov 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-ali commented Nov 30, 2018

mkimuram commented Nov 30, 2018

davidz627 commented Nov 30, 2018

verult commented Dec 1, 2018

davidz627 commented Dec 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-ali left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 1, 2018

verult left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingxu97 commented Dec 4, 2018

k8s-ci-robot commented Dec 4, 2018

verult commented Dec 4, 2018

davidz627 commented Nov 29, 2018 •

edited

Loading

davidz627 Nov 29, 2018 •

edited

Loading