Skip to content
This repository has been archived by the owner on Oct 21, 2020. It is now read-only.

Remove dynamic users when pv is remove #466

Merged
merged 1 commit into from
Nov 20, 2017

Conversation

JrCs
Copy link
Contributor

@JrCs JrCs commented Nov 16, 2017

This PR will remove dynamic users create by the provisionner (k8s secret and ceph client auth).

Fixes: #455

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 16, 2017
def _remove_ceph_user(self, user_id):
client_entity = "client.{0}".format(user_id)
ret = self._volume_client._rados_command(
'auth rm',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting, auth rm works but according to doc, should be auth del

@@ -196,6 +196,15 @@ def cap_update(orig, want, unwanted):
assert caps[0]['entity'] == client_entity
return caps[0]

def _remove_ceph_user(self, user_id):
client_entity = "client.{0}".format(user_id)
ret = self._volume_client._rados_command(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add try/catch in case user already removed.

@@ -196,6 +200,14 @@ func (p *cephFSProvisioner) Delete(volume *v1.PersistentVolume) error {
return cmdErr
}

// Remove dynamic user secret
secretName := generateSecretName(user)
err = p.client.Core().Secrets(volume.Spec.ClaimRef.Namespace).Delete(secretName, &metav1.DeleteOptions{})
Copy link
Contributor

@rootfs rootfs Nov 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rootfs
Copy link
Contributor

rootfs commented Nov 16, 2017

thanks, can you look into #309 as well?

@cofyc
Copy link
Contributor

cofyc commented Nov 17, 2017

hi,

cephfs_provisioner.py implements its customized _authorize_ceph method instead of using ceph_volume_client.py's _authorize_ceph method because we need extra caps.

How about following the same logic to implement a customized _deauthorize method instead of using ceph_volume_client.py's _deauthorize ?

In this customized _deauthorize we can remove caps first, and if all caps are removed then remove the given user like ceph_volume_client.py's _deauthorize do.

Doing it in this way, we can avoid hard-coding in cephfs_provisioner.py script. And it's possible to share a ceph user for many pvs in future (if we need).

@JrCs JrCs force-pushed the remove-dynamic-users branch from 75052d6 to 02d388e Compare November 17, 2017 09:42
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 17, 2017
@JrCs
Copy link
Contributor Author

JrCs commented Nov 17, 2017

Modification done from @rootfs reviews.
I'm agree with @cofyc: it will be better to have a custom _deauthorize method. My python skill is limited so i prefer to let you code this method.

@rootfs
Copy link
Contributor

rootfs commented Nov 17, 2017

@JrCs how about just merge your golang part (i.e. secret) and wait for python fix from @cofyc

@JrCs
Copy link
Contributor Author

JrCs commented Nov 17, 2017

@rootfs we can merge this PR completly (it fixes #455) and create another PR to have an enhance method to deauthorize the user ?

@rootfs
Copy link
Contributor

rootfs commented Nov 17, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2017
cofyc added a commit to cofyc/external-storage that referenced this pull request Nov 20, 2017
cofyc added a commit to cofyc/external-storage that referenced this pull request Nov 20, 2017
@rootfs
Copy link
Contributor

rootfs commented Nov 20, 2017

ping @wongma7, can you merge? thanks

@wongma7 wongma7 merged commit e46c661 into kubernetes-retired:master Nov 20, 2017
cofyc added a commit to cofyc/external-storage that referenced this pull request Nov 21, 2017
cofyc added a commit to cofyc/external-storage that referenced this pull request Nov 21, 2017
cofyc added a commit to cofyc/external-storage that referenced this pull request Nov 21, 2017
cofyc added a commit to cofyc/external-storage that referenced this pull request Nov 21, 2017
@JrCs JrCs deleted the remove-dynamic-users branch November 21, 2017 11:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/ceph/cephfs cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants