provider: avoid CSI secret name collisions #2537

jarrpa · 2024-03-28T23:37:37Z

Previously, when fetching a storage claim config, a remote consumer was given information for creating the Ceph CSI Secrets that was identical to the secret names on the provider side. Running a consumer in the same namespace as the provider introduced namespace collisions.

Hilariously, by virtue of being the same information, things just kinda worked out anyway. Still, this is likely going to produce massive problems later, so better to fix it now.

Signed-off-by: Jose A. Rivera [email protected]

Fixing a really small but obnoxious typo as well as a nil reference panic found during development on that same variable. Signed-off-by: Jose A. Rivera <[email protected]>

Signed-off-by: Jose A. Rivera <[email protected]>

openshift-ci · 2024-03-28T23:37:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jarrpa

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~~ [jarrpa]

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

services/provider/server/server.go

leelavg · 2024-03-29T14:48:47Z

LGTM.

services/provider/server/server.go

nb-ohad · 2024-04-01T17:41:01Z

services/provider/server/server.go

@@ -393,6 +393,10 @@ func (s *OCSProviderServer) getCephClientInformation(ctx context.Context, name s
 	return cephClient.Status.Info["secretName"], cephClient.Annotations[controllers.StorageCephUserTypeAnnotation], nil
 }

+func scrCephClientCsiSecretName(secretType, suffix string) string {


Shouldn't this be called storageClaimCepCSISecretName mainly because it produces a name for a secret reconciled by the storage claim, not the request?

Sure, done.

services/provider/server/server.go

nb-ohad · 2024-04-01T17:45:58Z

services/provider/server/storageclassrequest.go

+		// This field was named before a small refactor to clarify
+		// what it actually represents, but is being kept as-is so as
+		// to not change the generative logic.
+		StorageClassClaimName string `json:"storageClassRequestName"`


Shouldn't this be called StorageClaimName? we moved away from StorageClassClaim API and replaced it with StorageClaim API

I didn't think it was worth the effort to unify that everywhere in the codebase on the provider side. That's another follow-up PR, if anything.

As the final results are changes we don't need to keep storageClassRequestName in the json we can just change it to storageclaimName. WDYT?

@jarrpa I am not asking to change existing misleading names. Just to not add new fields/renames into misleading names

nb-ohad · 2024-04-01T17:48:48Z

services/provider/server/storageclassrequest.go

-func getStorageClassRequestName(consumerUUID, storageClassRequestName string) string {
+// getStorageClassRequestHash generates a hash for a StorageClassRequest based
+// on the MD5 hash of the StorageClassClaim name and storageConsumer UUID.
+func getStorageClassRequestHash(consumerUUID, storageClassClaimName string) string {


If we are doing this change then you need to change the call sites on storageClassRequestManager that refer to the same param as StorageClassRequestName which I agree is misleading

Fair, done.

change storageClassClaimName to storageClaimName ?

Madhu-1

LGTM, can we switch from storageclassclaim to storageclaim as we are changing things respect to Marhsal/Unmarshal to generate names as we also need to handle migration in this PR or are we going to handle that in followup PR?

Madhu-1 · 2024-04-02T05:46:20Z

services/provider/server/server.go

+	TicketAnnotation           = "ocs.openshift.io/provider-onboarding-ticket"
+	ProviderCertsMountPoint    = "/mnt/cert"
+	onboardingTicketKeySecret  = "onboarding-ticket-key"
+	storageClassClaimNameLabel = "ocs.openshift.io/storageclassclaim-name"


Suggested change

storageClassClaimNameLabel = "ocs.openshift.io/storageclassclaim-name"

storageClaimNameLabel = "ocs.openshift.io/storageclaim-name"

it should be storageClaimNameLabel?

I think this is more dangerous (and might have side effects) and regardless should not be part of this PR

Madhu-1 · 2024-04-02T05:48:21Z

services/provider/server/storageclassrequest.go

-func getStorageClassRequestName(consumerUUID, storageClassRequestName string) string {
+// getStorageClassRequestHash generates a hash for a StorageClassRequest based
+// on the MD5 hash of the StorageClassClaim name and storageConsumer UUID.
+func getStorageClassRequestHash(consumerUUID, storageClassClaimName string) string {


change storageClassClaimName to storageClaimName ?

Madhu-1 · 2024-04-02T05:49:48Z

services/provider/server/storageclassrequest.go

+		// This field was named before a small refactor to clarify
+		// what it actually represents, but is being kept as-is so as
+		// to not change the generative logic.
+		StorageClassClaimName string `json:"storageClassRequestName"`


As the final results are changes we don't need to keep storageClassRequestName in the json we can just change it to storageclaimName. WDYT?

leelavg · 2024-04-02T06:30:21Z

LGTM, can we switch from storageclassclaim to storageclaim as we are changing things respect to Marhsal/Unmarshal to generate names as we also need to handle migration in this PR or are we going to handle that in followup PR?

I believe there'll be two followup PRs (mostly I'll be the one working on it), one that changes gRPC which renames StorageClassClaims to StorageClaims and another that changes StorageClassRequests to StorageRequests.

Edit: I see you mentioned a follow-up in one of the comments, from above what will you be working on @jarrpa?

jarrpa · 2024-04-02T13:38:55Z

@Madhu-1: To all your StrorageClassClaim to StorageClaim comments: no. That should be handled by a larger PR that comes and does that across the entire code base.

@leelavg Neither! The follow-up PR will be to refactor the logic in the FulfillStorageClassClaim function and has nothing to do with the renaming.

Probably as a holdover from a previous design, the provider server code uses "storageClassRequestName" in multiple places where it is not appropriate. These often hold the name for the *StorageClassClaim* that came in as part of the FulfillStorageClassClaim() call. I seriously lost hours of my time before I figured out this mistake, so I'm saving everyone else the headache in the future. Signed-off-by: Jose A. Rivera <[email protected]>

Signed-off-by: Jose A. Rivera <[email protected]>

Previously, when fetching a storage claim config, a remote consumer was given information for creating the Ceph CSI Secrets that was identical to the secret names on the provider side. Running a consumer in the same namespace as the provider introduced namespace collisions. Hilariously, by virtue of being the same information, things just kinda worked out anyway. Still, this is likely going to produce massive problems later, so better to fix it now. Signed-off-by: Jose A. Rivera <[email protected]>

leelavg · 2024-04-02T14:17:26Z

@nb-ohad @Madhu-1 wrt reviews could we pls consider this PR as bug fix only, unless I'm missing something which needs more changes ie, update secret names correctly and ntg more?

nb-ohad · 2024-04-02T14:34:38Z

@nb-ohad @Madhu-1 wrt reviews could we pls consider this PR as bug fix only, unless I'm missing something which needs more changes ie, update secret names correctly and ntg more?

I agree with a small caveat. If you are renaming existing names or adding new vars/fields with new names you should use the correct terminology with this new stuff. This does not mean you need to go and rename the entirety of the existing codebase

leelavg · 2024-04-08T03:28:02Z

to author & reviewers, may I know the status quo of this PR? if it makes sense, can we resist the renames not to ride on a bug fix?

yes, we renamed storageclassclaim api to storageclaim api in client side and storageclassrequest gets the fields indirectly from client via gRPC and gRPC methods aren't updated yet, can we not do some part here and some part in another PR?

I understand the refactor of func to get hash, however at the end it'll neither be storageclassrequest nor storageclassclaim and can we not make diff there?

leelavg · 2024-04-19T12:10:27Z

Superseded by #2571, thanks for working on this!

openshift-merge-robot · 2024-04-19T12:11:37Z

PR needs rebase.

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.

jarrpa added 2 commits March 28, 2024 18:36

provider: fix typo and panic in server_test.go

d00012b

Fixing a really small but obnoxious typo as well as a nil reference panic found during development on that same variable. Signed-off-by: Jose A. Rivera <[email protected]>

provider: clarify healthchecker secret name in server_test.go

89a9b71

Signed-off-by: Jose A. Rivera <[email protected]>

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 28, 2024

leelavg reviewed Mar 29, 2024

View reviewed changes

services/provider/server/server.go Show resolved Hide resolved

jarrpa force-pushed the jarrpa/csi-secret-collision branch from c9bb002 to 77ce704 Compare March 29, 2024 14:44

nb-ohad requested changes Apr 1, 2024

View reviewed changes

services/provider/server/server.go Outdated Show resolved Hide resolved

services/provider/server/server.go Outdated Show resolved Hide resolved

services/provider/server/server.go Show resolved Hide resolved

openshift-ci bot assigned nb-ohad Apr 1, 2024

jarrpa force-pushed the jarrpa/csi-secret-collision branch from 77ce704 to 44dbbb8 Compare April 1, 2024 17:30

nb-ohad requested changes Apr 1, 2024

View reviewed changes

jarrpa force-pushed the jarrpa/csi-secret-collision branch from 44dbbb8 to d7b072f Compare April 1, 2024 20:59

Madhu-1 reviewed Apr 2, 2024

View reviewed changes

jarrpa added 3 commits April 2, 2024 08:40

provider: move storageclassrequest hashing into subfunction

f78e25f

Signed-off-by: Jose A. Rivera <[email protected]>

jarrpa force-pushed the jarrpa/csi-secret-collision branch from d7b072f to b1da566 Compare April 2, 2024 13:43

leelavg mentioned this pull request Apr 19, 2024

provider: avoid CSI secret name collisions #2571

Merged

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider: avoid CSI secret name collisions #2537

provider: avoid CSI secret name collisions #2537

jarrpa commented Mar 28, 2024 •

edited

Loading

openshift-ci bot commented Mar 28, 2024

leelavg commented Mar 29, 2024

nb-ohad Apr 1, 2024

jarrpa Apr 1, 2024

nb-ohad Apr 1, 2024

jarrpa Apr 1, 2024

Madhu-1 Apr 2, 2024

nb-ohad Apr 2, 2024

nb-ohad Apr 1, 2024

jarrpa Apr 1, 2024

Madhu-1 Apr 2, 2024

Madhu-1 left a comment

Madhu-1 Apr 2, 2024

nb-ohad Apr 2, 2024

Madhu-1 Apr 2, 2024

Madhu-1 Apr 2, 2024

leelavg commented Apr 2, 2024 •

edited

Loading

jarrpa commented Apr 2, 2024

leelavg commented Apr 2, 2024

nb-ohad commented Apr 2, 2024

leelavg commented Apr 8, 2024 •

edited

Loading

leelavg commented Apr 19, 2024

openshift-merge-robot commented Apr 19, 2024

	storageClassClaimNameLabel = "ocs.openshift.io/storageclassclaim-name"
	storageClaimNameLabel = "ocs.openshift.io/storageclaim-name"

provider: avoid CSI secret name collisions #2537

Are you sure you want to change the base?

provider: avoid CSI secret name collisions #2537

Conversation

jarrpa commented Mar 28, 2024 • edited Loading

openshift-ci bot commented Mar 28, 2024

leelavg commented Mar 29, 2024

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

Madhu-1 left a 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

leelavg commented Apr 2, 2024 • edited Loading

jarrpa commented Apr 2, 2024

leelavg commented Apr 2, 2024

nb-ohad commented Apr 2, 2024

leelavg commented Apr 8, 2024 • edited Loading

leelavg commented Apr 19, 2024

openshift-merge-robot commented Apr 19, 2024

jarrpa commented Mar 28, 2024 •

edited

Loading

leelavg commented Apr 2, 2024 •

edited

Loading

leelavg commented Apr 8, 2024 •

edited

Loading