New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding credentials support for k8s core CSI #60118

Merged
merged 3 commits into from Feb 24, 2018

Conversation

@sbezverk
Contributor

sbezverk commented Feb 21, 2018

PR implements changes proposed in: kubernetes/community#1816

CSI now allows credentials to be specified on CreateVolume/DeleteVolume, ControllerPublishVolume/ControllerUnpublishVolume, and NodePublishVolume/NodeUnpublishVolume operations
@sbezverk

This comment has been minimized.

Contributor

sbezverk commented Feb 21, 2018

/sig storage
/kind feature

@sbezverk

This comment has been minimized.

Contributor

sbezverk commented Feb 21, 2018

@vladimirvivien @saad-ali Please review.
This implementation adds two new objects and modifies code only for NodePublish and NodeUnpublish, since these operations are executed by csi_mounter.go and csi_client.go
Changes for ControllerCreate/Delete/Publish/Unpublish are outside of k8s core and will be done after merging this change.

@david-mcmahon david-mcmahon removed their request for review Feb 21, 2018

@saad-ali saad-ali self-assigned this Feb 22, 2018

@saad-ali saad-ali requested review from saad-ali and removed request for lavalamp, nikhiljindal and wojtek-t Feb 22, 2018

@sbezverk

This comment has been minimized.

Contributor

sbezverk commented Feb 22, 2018

@saad-ali addressed changes in the proposal, PTAL.

// ControllerPublishVolume and ControllerUnpublishVolume calls.
// This may be empty if no secret is required. If the secret object contains
// more than one secret, all secrets are passed.
// +optional

This comment has been minimized.

@saad-ali

saad-ali Feb 22, 2018

Member

nit: Update comment

  // ControllerPublishSecretRef is a reference to the secret object containing
  // sensitive information to pass to the CSI driver to complete the CSI
  // ControllerPublishVolume and ControllerUnpublishVolume calls.
  // This field is optional, and  may be empty if no secret is required. If the
  // secret object contains more than one secret, all secrets are passed.
  // +optional

This comment has been minimized.

@sbezverk

sbezverk Feb 22, 2018

Contributor

done

// NodeStageSecretRef is a reference to the secret object containing sensitive
// information to pass to the CSI driver to complete the CSI NodeStageVolume
// and NodeStageVolume and NodeUnstageVolume calls.
// This secret will be fetched by the kubelet.

This comment has been minimized.

@saad-ali

saad-ali Feb 22, 2018

Member

You can remove this line.

// NodePublishSecretRef is a reference to the secret object containing
// sensitive information to pass to the CSI driver to complete the CSI
// NodePublishVolume and NodeUnpublishVolume calls.
// This secret will be fetched by the kubelet.

This comment has been minimized.

@saad-ali

saad-ali Feb 22, 2018

Member

You can remove this line.

This comment has been minimized.

@sbezverk

sbezverk Feb 22, 2018

Contributor

done

func getCredentialsFromSecret(k8s kubernetes.Interface, sn, ns string) map[string]string {
credentials := map[string]string{}
if len(sn) == 0 || len(ns) == 0 {

This comment has been minimized.

@saad-ali

saad-ali Feb 22, 2018

Member

I think empty namespace maybe a valid case (default namespace?).

This comment has been minimized.

@sbezverk

sbezverk Feb 22, 2018

Contributor

done, in reality it will not be ever empty as I take it from pod's spec. Removed ns check.

@@ -195,7 +195,12 @@ func (c *csiMountMgr) SetUpAt(dir string, fsGroup *int64) error {
if len(fsType) == 0 {
fsType = defaultFSType
}
nodePublishCredentials := map[string]string{}

This comment has been minimized.

@saad-ali

saad-ali Feb 22, 2018

Member

Not your code, but, can you add a commit to remove probe related code in this PR too:

...
 	if err := csi.NodeProbe(ctx, csiVersion); err != nil {
...

and all related comments, e.g.:

// TODO (vladimirvivien) use this method to probe controller using CSI.NodeProbe() call

This comment has been minimized.

@sbezverk

sbezverk Feb 23, 2018

Contributor

done

nodePublishCredentials := map[string]string{}
if c.spec.PersistentVolume.Spec.CSI.NodePublishSecretRef != nil {
sn := c.spec.PersistentVolume.Spec.CSI.NodePublishSecretRef.Name

This comment has been minimized.

@saad-ali

saad-ali Feb 22, 2018

Member

Just use csiSource.NodePublishSecretRef.Name everywhere instead.

This comment has been minimized.

@sbezverk

sbezverk Feb 22, 2018

Contributor

done

@@ -195,7 +195,12 @@ func (c *csiMountMgr) SetUpAt(dir string, fsGroup *int64) error {
if len(fsType) == 0 {
fsType = defaultFSType
}
nodePublishCredentials := map[string]string{}
if c.spec.PersistentVolume.Spec.CSI.NodePublishSecretRef != nil {

This comment has been minimized.

@saad-ali

saad-ali Feb 22, 2018

Member

I'm not 100% sure, but I think an empty namespace is valid. So maybe just check if NodePublishSecretRef.Name != nil

This comment has been minimized.

@sbezverk

sbezverk Feb 22, 2018

Contributor

Here I check for the presence of Secret Object, Name is string, it could be not nil but "". If you do not mind I would like to keep this check for object not nil and secret name validation in getCredentialsFromSecret

@sbezverk

This comment has been minimized.

Contributor

sbezverk commented Feb 23, 2018

@saad-ali addressed your comments, PTAL.

@saad-ali

This comment has been minimized.

Member

saad-ali commented Feb 23, 2018

I forgot one more set of changes: The Kubernetes API server's node authorizer must be updated to allow kubelet to access the secrets referenced by CSIPersistentVolumeSource.

@saad-ali

This comment has been minimized.

Member

saad-ali commented Feb 23, 2018

I forgot one more set of changes: The Kubernetes API server's node authorizer must be updated to allow kubelet to access the secrets referenced by CSIPersistentVolumeSource.

Actually I think this might be handled by VisitPVSecretNames.
@liggitt to confirm and review.

/assign @liggitt

@saad-ali

This comment has been minimized.

Member

saad-ali commented Feb 23, 2018

verify gofmt is failing, please run gofmt

@@ -111,6 +111,32 @@ func VisitPVSecretNames(pv *api.PersistentVolume, visitor Visitor) bool {
if source.StorageOS.SecretRef != nil && !visitor(source.StorageOS.SecretRef.Namespace, source.StorageOS.SecretRef.Name) {
return false
}
case source.CSI != nil:
if source.CSI.ControllerPublishSecretRef != nil {
if len(source.CSI.ControllerPublishSecretRef.Name) == 0 || len(source.CSI.ControllerPublishSecretRef.Namespace) == 0 {

This comment has been minimized.

@liggitt

liggitt Feb 23, 2018

Member

api validation should be added to prevent these from being empty... no need to recheck here (or in the other blocks)

This comment has been minimized.

@sbezverk

sbezverk Feb 23, 2018

Contributor

@liggitt could you please point me where the validation you mentioned should be added.

This comment has been minimized.

@liggitt

liggitt Feb 23, 2018

Member

func validateCSIPersistentVolumeSource(csi *core.CSIPersistentVolumeSource, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}
if !utilfeature.DefaultFeatureGate.Enabled(features.CSIPersistentVolume) {
allErrs = append(allErrs, field.Forbidden(fldPath, "CSIPersistentVolume disabled by feature-gate"))
}
if len(csi.Driver) == 0 {
allErrs = append(allErrs, field.Required(fldPath.Child("driver"), ""))
}
if len(csi.Driver) > 63 {
allErrs = append(allErrs, field.TooLong(fldPath.Child("driver"), csi.Driver, 63))
}
if !csiDriverNameRexp.MatchString(csi.Driver) {
allErrs = append(allErrs, field.Invalid(fldPath.Child("driver"), csi.Driver, validation.RegexError(csiDriverNameRexpErrMsg, csiDriverNameRexpFmt, "csi-hostpath")))
}
if len(csi.VolumeHandle) == 0 {
allErrs = append(allErrs, field.Required(fldPath.Child("volumeHandle"), ""))
}
return allErrs
}

@sbezverk

This comment has been minimized.

Contributor

sbezverk commented Feb 23, 2018

/test pull-kubernetes-unit

@@ -1452,6 +1452,33 @@ func validateCSIPersistentVolumeSource(csi *core.CSIPersistentVolumeSource, fldP
allErrs = append(allErrs, field.Required(fldPath.Child("volumeHandle"), ""))
}
if csi.ControllerPublishSecretRef != nil {

This comment has been minimized.

@liggitt

liggitt Feb 23, 2018

Member

for all of these, ensure name is a valid secret name and namespace is a valid namespace name

This comment has been minimized.

@sbezverk

sbezverk Feb 23, 2018

Contributor

done

nodePublishCredentials := map[string]string{}
if csiSource.NodePublishSecretRef != nil {
sn := csiSource.NodePublishSecretRef.Name
ns := c.pod.ObjectMeta.Namespace

This comment has been minimized.

@liggitt

liggitt Feb 23, 2018

Member

this should be csiSource.NodePublishSecretRef.Namespace

This comment has been minimized.

@liggitt

liggitt Feb 23, 2018

Member

to protect against this type of mistake, consider passing the secret ref to getCredentialsFromSecret instead of loose name/namespace fields

This comment has been minimized.

@sbezverk

sbezverk Feb 23, 2018

Contributor

done

nodeUnpublishCredentials := map[string]string{}
if csiSource.NodePublishSecretRef != nil {
sn := csiSource.NodePublishSecretRef.Name
ns := c.pod.ObjectMeta.Namespace

This comment has been minimized.

@liggitt

liggitt Feb 23, 2018

Member

NodePublishSecretRef.Namespace

This comment has been minimized.

@sbezverk

sbezverk Feb 23, 2018

Contributor

Will be passing SecreteReference to getCredentialsFromSecret.

@sbezverk

This comment has been minimized.

Contributor

sbezverk commented Feb 23, 2018

@saad-ali @liggitt addressed latest comments, PTAL

@saad-ali

LGTM mod nit

@liggitt for node authorizer review

@@ -40,11 +40,11 @@ func VisitPVSecretNames(pv *api.PersistentVolume, visitor Visitor) bool {
switch {
case source.AzureFile != nil:
if source.AzureFile.SecretNamespace != nil && len(*source.AzureFile.SecretNamespace) > 0 {
if len(source.AzureFile.SecretName) > 0 && !visitor(*source.AzureFile.SecretNamespace, source.AzureFile.SecretName) {
if len(source.AzureFile.SecretName) > 0 && !visitor(*source.AzureFile.SecretNamespace, source.AzureFile.SecretName, true) {

This comment has been minimized.

@saad-ali

saad-ali Feb 23, 2018

Member

nit: when it is unclear what the value means, it's nice to have a comment, for example:

!visitor(
    *source.AzureFile.SecretNamespace,
    source.AzureFile.SecretName,
    true /* kubeletVisible */) {

But it's a personal preference, up to you if you want to do that.

This comment has been minimized.

@sbezverk

sbezverk Feb 23, 2018

Contributor

agree, it is more next developer friendly :)

credentials := map[string]string{}
secret, err := k8s.CoreV1().Secrets(secretRef.Namespace).Get(secretRef.Name, meta.GetOptions{})
if err != nil {
glog.Warningf("failed to find the secret %s in the namespace %s with error: %v\n", secretRef.Name, secretRef.Namespace, err)

This comment has been minimized.

@liggitt

liggitt Feb 23, 2018

Member

I expected this to return the error and avoid making the CSI call

This comment has been minimized.

@sbezverk

sbezverk Feb 23, 2018

Contributor

I am not sure, credentials are not mandatory, so if not specified I do not think we should fail CSI call. @saad-ali what do you think?

This comment has been minimized.

@liggitt

liggitt Feb 23, 2018

Member

if they were specified by the PV, then it seems like an error resolving them should be considered an error in input gathering and short-circuit

nodePublishCredentials := map[string]string{}
if csiSource.NodePublishSecretRef != nil {
nodePublishCredentials = getCredentialsFromSecret(c.k8s, csiSource.NodePublishSecretRef)

This comment has been minimized.

@liggitt

liggitt Feb 23, 2018

Member

here and below, I expected to handle an error return from getCredentialsFromSecret by short-circuiting and returning the error, rather than making the csi call with an empty credentials map

@liggitt

This comment has been minimized.

Member

liggitt commented Feb 23, 2018

API and node authorizer bits LGTM
one comment on error handling on secret lookup

@liggitt

This comment has been minimized.

Member

liggitt commented Feb 23, 2018

/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 23, 2018

@saad-ali

/lgtm
/approve

@saad-ali

This comment has been minimized.

Member

saad-ali commented Feb 23, 2018

For vendored dep update:
/assign @thockin

@brendandburns

This comment has been minimized.

Contributor

brendandburns commented Feb 23, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 23, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, liggitt, saad-ali, sbezverk

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 24, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@sbezverk

This comment has been minimized.

Contributor

sbezverk commented Feb 24, 2018

/test pull-kubernetes-unit

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 24, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 8e8601a into kubernetes:master Feb 24, 2018

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-unit
Details
cla/linuxfoundation sbezverk authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@sbezverk sbezverk deleted the sbezverk:csi_core_credentials branch Mar 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment