Fix the managed-by-label getting populated with invalid value. #1334

vamsikrishna-siddu · 2025-01-21T09:42:26Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change

/kind bug

/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
There will be a case when the length of the node name is equals to 53 and base length becomes zero. This will make the prefix value to be empty and the label will be generated starting with '-' which is invalid label.
Ex: nodename is
worker1.ocp-virt2-s390x.s390g.lab.eng.rdu2.redhat.com
generated label name:
-53f40b57-worker1.ocp-virt1-s390x.s390g.lab.eng.rdu2.redhat.com

This is causing the issue for the hostpath provisioner while generating csistoragecapacity objects.
W0116 09:21:22.515275 1 reflector.go:547] k8s.io/client-go/informers/factory.go:160: failed to list *v1.CSIStorageCapacity: unable to parse requirement: values[0][csi.storage.k8s.io/managed-by]: Invalid value: "-53f40b57-worker1.ocp-virt1-s390x.s390g.lab.eng.rdu2.redhat.com": a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyValue', or 'my_value', or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?')

Which issue(s) this PR fixes:

Fixes #1333

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

storage capacity: managed-by label getting invalid label  with a long node name and enableNodeDeployment set to true

linux-foundation-easycla · 2025-01-21T09:42:30Z

The committers listed above are authorized under a signed CLA.

✅ login: vamsikrishna-siddu (796fe98)

k8s-ci-robot · 2025-01-21T09:42:34Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vamsikrishna-siddu
Once this PR has been reviewed and has the lgtm label, please assign msau42 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

k8s-ci-robot · 2025-01-21T09:42:35Z

Welcome @vamsikrishna-siddu!

It looks like this is your first PR to kubernetes-csi/external-provisioner 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-csi/external-provisioner has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-01-21T09:42:36Z

Hi @vamsikrishna-siddu. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

vamsikrishna-siddu · 2025-01-21T09:53:45Z

/cc @awels

k8s-ci-robot · 2025-01-21T09:53:47Z

@vamsikrishna-siddu: GitHub didn't allow me to request PR reviews from the following users: awels.

Note that only kubernetes-csi members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @awels

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-sigs/prow repository.

mowangdk · 2025-01-22T05:49:11Z

Please sign the CLA Authorization first.

vamsikrishna-siddu · 2025-01-22T10:49:43Z

Please sign the CLA Authorization first.

sure

vamsikrishna-siddu · 2025-01-23T07:00:25Z

Hi @mowangdk I have got the CLA Authorization. Can you please review the pr?

mowangdk · 2025-01-23T15:03:57Z

/ok-to-test

awels · 2025-01-23T17:05:11Z

I will try to take a look at this soon, I have been out sick for a few days and I am finally getting to feeling better.

mowangdk · 2025-01-25T09:21:05Z

cmd/csi-provisioner/util.go

@@ -36,7 +36,7 @@ func getNameWithMaxLength(base, suffix string, maxLength int) string {
 	baseLength := maxLength - 10 /*length of -hash-*/ - len(suffix)

 	// if the suffix is too long, ignore it
-	if baseLength < 0 {
+	if baseLength <= 0 {
 		prefix := base[0:min(len(base), max(0, maxLength-9))]
 		// Calculate hash on initial base-suffix string


I think we need to ensure prefix must not equals to ""

And maybe we also need add more ut here to avoid corner cases

Hi @mowangdk ,
with the condition
if baseLength <= 0
the prefix will never by empty.
and I have added a unit test for the above issue. With this it will cover all the corner cases.

@vamsikrishna-siddu Did you mean the prefix outside the if condition? The prefix inside the if condition in getNameWithMaxLength, is not guaranteed i think?

@mowangdk yeah i meant the prefix outside the if condition. But the prefix inside the if condition is also never become empty. The reason is the base is always external-provisioner and the maxLength is always 63. so the prefix will never be empty. The only case where the prefix will be empty is when we pass the base as empty string. But in code, I dont see any place where we are using this function for any other use case which provides the base as empty string.

yeah，I agreed. But IMHO, it's better not to rely on function parameters because they can change anytime. Instead, parameter validation should be handled within the function itself.

@mowangdk okay got it. I have done the required changes. Can you please review it once again.

awels · 2025-01-28T12:54:17Z

looks good to me, I don't think I can /lgtm though

jsafrane · 2025-02-05T12:41:07Z

cmd/csi-provisioner/util_test.go

@@ -45,6 +45,10 @@ func TestGetNameWithMaxLength(t *testing.T) {
 			nodeName: fmt.Sprintf("node%s", strings.Repeat("a", 39)),
 		},
 		"very long, ignore suffix": {
+			expected: fmt.Sprintf("%s-%s", externalProvisioner, "a3607ff1"),
+			nodeName: fmt.Sprintf("node%s", strings.Repeat("a", 49)),


Please add an unit test also with strings.Repeat("a", 48) - that's the last that produces e-<hash>-nodeaaaaa....
49 here is then the first that skips the node name entirely.

vamsikrishna-siddu · 2025-02-17T06:31:54Z

Hi @mowangdk @jsafrane , I have addressed your review comments. Can you please review it once.

Signed-off-by: Vamsi Krishna Siddu <[email protected]>

mowangdk · 2025-02-18T07:39:31Z

/lgtm

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jan 21, 2025

k8s-ci-robot requested review from andyzhangx and humblec January 21, 2025 09:42

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jan 21, 2025

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 21, 2025

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 21, 2025

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jan 23, 2025

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 23, 2025

mowangdk reviewed Jan 25, 2025

View reviewed changes

jsafrane reviewed Feb 5, 2025

View reviewed changes

vamsikrishna-siddu force-pushed the fix-max-length-managed-label branch from 5d30e41 to 86f7109 Compare February 17, 2025 06:30

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 17, 2025

Fix the managed-by-label getting populated with invalid value.

796fe98

Signed-off-by: Vamsi Krishna Siddu <[email protected]>

vamsikrishna-siddu force-pushed the fix-max-length-managed-label branch from 86f7109 to 796fe98 Compare February 18, 2025 04:30

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 Feb 18, 2025

k8s-ci-robot assigned mowangdk Feb 18, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 18, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the managed-by-label getting populated with invalid value. #1334

Fix the managed-by-label getting populated with invalid value. #1334

vamsikrishna-siddu commented Jan 21, 2025

linux-foundation-easycla bot commented Jan 21, 2025 •

edited

Loading

k8s-ci-robot commented Jan 21, 2025

k8s-ci-robot commented Jan 21, 2025

k8s-ci-robot commented Jan 21, 2025

vamsikrishna-siddu commented Jan 21, 2025

k8s-ci-robot commented Jan 21, 2025

mowangdk commented Jan 22, 2025

vamsikrishna-siddu commented Jan 22, 2025

vamsikrishna-siddu commented Jan 23, 2025

mowangdk commented Jan 23, 2025

awels commented Jan 23, 2025

mowangdk Jan 25, 2025

mowangdk Jan 25, 2025

vamsikrishna-siddu Jan 28, 2025

mowangdk Jan 29, 2025

vamsikrishna-siddu Feb 17, 2025

mowangdk Feb 17, 2025

vamsikrishna-siddu Feb 18, 2025

awels commented Jan 28, 2025

jsafrane Feb 5, 2025 •

edited

Loading

vamsikrishna-siddu Feb 17, 2025

vamsikrishna-siddu Feb 17, 2025

vamsikrishna-siddu commented Feb 17, 2025

mowangdk commented Feb 18, 2025

Fix the managed-by-label getting populated with invalid value. #1334

Are you sure you want to change the base?

Fix the managed-by-label getting populated with invalid value. #1334

Conversation

vamsikrishna-siddu commented Jan 21, 2025

linux-foundation-easycla bot commented Jan 21, 2025 • edited Loading

k8s-ci-robot commented Jan 21, 2025

k8s-ci-robot commented Jan 21, 2025

k8s-ci-robot commented Jan 21, 2025

vamsikrishna-siddu commented Jan 21, 2025

k8s-ci-robot commented Jan 21, 2025

mowangdk commented Jan 22, 2025

vamsikrishna-siddu commented Jan 22, 2025

vamsikrishna-siddu commented Jan 23, 2025

mowangdk commented Jan 23, 2025

awels commented Jan 23, 2025

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

awels commented Jan 28, 2025

jsafrane Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vamsikrishna-siddu commented Feb 17, 2025

mowangdk commented Feb 18, 2025

linux-foundation-easycla bot commented Jan 21, 2025 •

edited

Loading

jsafrane Feb 5, 2025 •

edited

Loading