rbd: migration secret support #2562
Conversation
humblec
force-pushed
the
mig-secret
branch
3 times, most recently
from
df32435
to
a1231c8
Compare
|
@humblec this was not part of the original plan as per #2509 (comment) why we are adding this one? Do we have a clear design doc/plan about the changes required in cephcsi? IMHO we are adding more complexity to cephcsi to support migration. |
@Madhu-1 I will add details about this change in the PR description and commit ..etc, thats why I kept this in draft atm . I have already added this change in the tracker and can add some more details there as well which explain how this helps. This looks to be pretty much the last change (unless some minor change comes up while doing complete e2e testing which cover kube.etc). To give some context the initial plan was to ask this change as part of the pre upgrade step, but found a way to help admin to stay away from this change mostly and we can take care of it from csi driver code itself without breaking any existing functionality. Reg# documentation, it is there in the todo as well. Thanks 👍 |
humblec
force-pushed
the
mig-secret
branch
7 times, most recently
from
ce589be
to
903bdf9
Compare
|
/retest ci/centos/mini-e2e-helm/k8s-1.20 |
|
/retest ci/centos/mini-e2e-helm/k8s-1.22 |
humblec
force-pushed
the
mig-secret
branch
2 times, most recently
from
50ce1af
to
903bdf9
Compare
@Madhu-1 as mentioned in the other PR comment #2562 (comment), this was the initial thought ie let admin patch the secret. Then after some discussions with community folks , it was recommended that, take care of it maximum from the CSI side . So this PR, the advantage with this PR is that, admin Dont want to patch the secret at all, if the admin user is not different from admin which is mostly the case. |
humblec
force-pushed
the
mig-secret
branch
2 times, most recently
from
539ecc3
to
6f723cf
Compare
|
@humblec please don't dismiss the requested review as am reviewing the PR. this can make the PR to be merged |
mergify
bot
dismissed stale reviews from nixpanic and yati1998
Pull request has been modified.
|
/retest ci/centos/mini-e2e/k8s-1.21 |
1 similar comment
|
/retest ci/centos/mini-e2e/k8s-1.21 |
|
/retest ci/centos/mini-e2e-helm/k8s-1.22 |
|
@Mergifyio rebase |
This commit adds a couple of helper functions to parse the migration request secret and set it for further csi driver operations. More details: The intree secret has a data field called "key" which is the base64 admin secret key. The ceph CSI driver currently expect the secret to contain data field "UserKey" for the equivalant. The CSI driver also expect the "UserID" field which is not available in the in-tree secret by deafult. This missing userID will be filled (if the username differ than 'admin') in the migration secret as 'adminId' field in the migration request, this commit adds the logic to parse this migration secret as below: "key" field value will be picked up from the migraion secret to "UserKey" field. "adminId" field value will be picked up from the migration secret to "UserID" field if `adminId` field is nil or not set, `UserID` field will be filled with default value ie `admin`.The above logic get activated only when the secret is a migration secret, otherwise skipped to the normal workflow as we have today. Signed-off-by: Humble Chirammal <[email protected]>
…MigSecret This commit adds unit tests for newly introduced migration specific functions. Signed-off-by: Humble Chirammal <[email protected]>
parseAndDeleteMigratedVolume() prviously clubbed the logic of parsing of migration volume handle and then continued with the deletion of the volume. however this commit split this logic into two, ie parsing has been done in parseMigrationVolID() and DeleteMigratedVolume() deletes the backend volume. Signed-off-by: Humble Chirammal <[email protected]>
this commit make use of the migration request secret parsing and set the required fields for further nodestage operations Signed-off-by: Humble Chirammal <[email protected]>
this commit create and make use of migration secret in the requests and validate various csi operations Signed-off-by: Humble Chirammal <[email protected]>
✅ Branch has been successfully rebased |
|
/retest all |
|
@Madhu-1 removed DNM on your behalf 👍 |
Note:
E2E tests are added to make sure migration kind of secret is passed in tests.
Additional note:
The initial plan was to ask the admin to adjust the secret as a pre-upgrade step
for migration which avoid this change in the CSI driver, however taking out admin burden
is the main ask from kubernetes community so this change is introduced. None of the
existing workflow is broken for CSI driver with this change.
Updates #2509
Signed-off-by: Humble Chirammal [email protected]