fix: fix slice init length #127785

huochexizhan · 2024-10-01T14:58:00Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

The intention here should be to initialize a slice with a capacity of len(topologies) rather than initializing the length of this slice.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

fix slice init length

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

linux-foundation-easycla · 2024-10-01T14:58:05Z

The committers listed above are authorized under a signed CLA.

✅ login: huochexizhan (6d853da)

k8s-ci-robot · 2024-10-01T14:58:10Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

k8s-ci-robot · 2024-10-01T14:58:10Z

Welcome @huochexizhan!

It looks like this is your first PR to kubernetes/kubernetes 🎉. 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/kubernetes 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 · 2024-10-01T14:58:11Z

Hi @huochexizhan. Thanks for your PR.

I'm waiting for a kubernetes 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.

mmorel-35 · 2024-10-02T04:08:11Z

Hi @huochexizhan ,
It looks like something that could have been identified with the use of makezero, do you agree?
And if you do, maybe it worth adding the linter so this best practice stay always applied ?

googs1025 · 2024-10-02T15:25:47Z

pkg/apis/storage/validation/validation.go

@@ -274,7 +274,7 @@ func validateAllowedTopologies(topologies []api.TopologySelectorTerm, fldPath *f
 		return allErrs
 	}

-	rawTopologies := make([]map[string]sets.Set[string], len(topologies))
+	rawTopologies := make([]map[string]sets.Set[string], 0, len(topologies))


If we use the modified method, the list will be considered empty but the capacity will be len(topologies). Is this what we expect? The original initialization method should create len(topologies) objects, right?

rawTopologies := make([]map[string]sets.Set[string], 0, len(topologies))

yeah, see below it is appending values to the list, that it seems are the supposed to be compared against, right now the len(topologies) entries are empty https://go.dev/play/p/eXaQdxsH5GP

thanks for answering this! :)

upps #127785 (comment) , it seems the logic depended on the empty values

aojea · 2024-10-03T20:44:01Z

/ok-to-test
/lgmt
/assign @thockin

thockin

Thanks!

/lgtm
/approve

k8s-ci-robot · 2024-10-03T21:06:51Z

LGTM label has been added.

Git tree hash: f28eec9155b7faebb954e2c9aea94a4ad271d129

k8s-ci-robot · 2024-10-03T21:07:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: huochexizhan, thockin

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:

~~pkg/apis/storage/OWNERS~~ [thockin]

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

k8s-triage-robot · 2024-10-04T00:03:53Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-triage-robot · 2024-10-04T03:23:54Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-triage-robot · 2024-10-04T06:04:55Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

aojea · 2024-10-04T10:51:34Z

interesting side effect @liggitt @thockin , since the slice was created with empty values, and the code considered invaliud duplicates, the empty value was considered invalid ... and now we need to keep this behavior, so we should keep injecting one empty case 🤕

{Failed;  === RUN   TestValidateAllowedTopologies
    validation_test.go:878: Expected failure for test "empty MatchLabelExpressions", got success
--- FAIL: TestValidateAllowedTopologies (0.00s)

huochexizhan · 2024-10-08T10:35:01Z

Thank! If there's anything I need to do, please feel free to let me know

thockin · 2024-11-07T23:40:37Z

I think the empty value SHOULD be allowed - it has a well defined meaning:

// TopologySelectorTerm represents the result of label queries.
// A null or empty topology selector term matches no objects.
// The requirements of them are ANDed.
// It provides a subset of functionality as NodeSelectorTerm.
// This is an alpha feature and may change in the future.
type TopologySelectorTerm struct {                                                                                                   
    // A list of topology selector requirements by labels.
    // +optional
    MatchLabelExpressions []TopologySelectorLabelRequirement
}

thockin · 2024-11-07T23:41:35Z

I think the correct thing to do here is fix the code and also:

diff --git a/pkg/apis/storage/validation/validation_test.go b/pkg/apis/storage/validation/validation_test.go
index 9f8c5bd0f72..16bd94ff364 100644
--- a/pkg/apis/storage/validation/validation_test.go
+++ b/pkg/apis/storage/validation/validation_test.go
@@ -837,7 +837,7 @@ func TestValidateAllowedTopologies(t *testing.T) {
                },
                "empty MatchLabelExpressions": {
                        class:         makeClass(&waitingMode, topologyEmptyMatchLabelExpressions),
-                       shouldSucceed: false,
+                       shouldSucceed: true,
                },
                "duplicate MatchLabelExpression keys": {
                        class:         makeClass(&waitingMode, topologyDupKeys),

Signed-off-by: huochexizhan <[email protected]>

huochexizhan · 2024-11-19T04:40:14Z

I think the correct thing to do here is fix the code and also:

diff --git a/pkg/apis/storage/validation/validation_test.go b/pkg/apis/storage/validation/validation_test.go
index 9f8c5bd0f72..16bd94ff364 100644
--- a/pkg/apis/storage/validation/validation_test.go
+++ b/pkg/apis/storage/validation/validation_test.go
@@ -837,7 +837,7 @@ func TestValidateAllowedTopologies(t *testing.T) {
                },
                "empty MatchLabelExpressions": {
                        class:         makeClass(&waitingMode, topologyEmptyMatchLabelExpressions),
-                       shouldSucceed: false,
+                       shouldSucceed: true,
                },
                "duplicate MatchLabelExpression keys": {
                        class:         makeClass(&waitingMode, topologyDupKeys),

Sorry to deal with it so late.

Has been modified, please review again.

thockin · 2024-11-19T17:30:39Z

Looking at this again, the effect of this is that we are loosening validation. It used to behave like:

$ cat /tmp/test-sc.yaml 
allowVolumeExpansion: true
apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
  name: test-empty-topology
provisioner: test.example.com
reclaimPolicy: Delete
volumeBindingMode: WaitForFirstConsumer
allowedTopologies:
  - {}

$ k --context=diy apply -f /tmp/test-sc.yaml 
The StorageClass "test-empty-topology" is invalid: allowedTopologies[0].matchLabelExpressions: Duplicate value: ""

Note the "duplicate value" error.

If we change this, then my YAML above will be accepted and stored. If a user subsequently rolls back to the previosu k8s release, this will fail to validate any update operations.

Worse, docs are somewhat conflicting:

staging/src/k8s.io/api/storage/v1/types.go:

 73     // allowedTopologies restrict the node topologies where volumes can be dynamically provisioned.
 74     // Each volume plugin defines its own supported topology specifications.
 75     // An empty TopologySelectorTerm list means there is no topology restriction.
 76     // This field is only honored by servers that enable the VolumeScheduling feature.
 77     // +optional
 78     // +listType=atomic
 79     AllowedTopologies []v1.TopologySelectorTerm `json:"allowedTopologies,omitempty" protobuf:"bytes,8,rep,name=allowedTopologies"    `

vs:

staging/src/k8s.io/api/core/v1/types.go:

3469 // A topology selector term represents the result of label queries.
3470 // A null or empty topology selector term matches no objects.
3471 // The requirements of them are ANDed.
3472 // It provides a subset of functionality as NodeSelectorTerm.
3473 // This is an alpha feature and may change in the future.
3474 // +structType=atomic
3475 type TopologySelectorTerm struct {

@msau42 @jsafrane - can you spend a moment thinking about this?

I see two paths:

Loosen validation in the "usual" way: add an alpha gate, tolerate the value in 33, move the gate to beta or even GA in 34.
Fix validation to fail on empty terms with a proper error, document it.

k8s-triage-robot · 2025-02-17T17:42:02Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

huochexizhan · 2025-02-28T11:18:01Z

Thanks. Anything need I to do?

thockin · 2025-02-28T23:57:04Z

#127785 (comment)

huochexizhan · 2025-03-01T00:58:58Z

#127785 (comment)

Thanks!

huochexizhan · 2025-03-01T00:59:12Z

Looking at this again, the effect of this is that we are loosening validation. It used to behave like:
$ cat /tmp/test-sc.yaml 
allowVolumeExpansion: true
apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
  name: test-empty-topology
provisioner: test.example.com
reclaimPolicy: Delete
volumeBindingMode: WaitForFirstConsumer
allowedTopologies:
  - {}

$ k --context=diy apply -f /tmp/test-sc.yaml 
The StorageClass "test-empty-topology" is invalid: allowedTopologies[0].matchLabelExpressions: Duplicate value: ""
Note the "duplicate value" error.

If we change this, then my YAML above will be accepted and stored. If a user subsequently rolls back to the previosu k8s release, this will fail to validate any update operations.

Worse, docs are somewhat conflicting:

staging/src/k8s.io/api/storage/v1/types.go:
 73     // allowedTopologies restrict the node topologies where volumes can be dynamically provisioned.
 74     // Each volume plugin defines its own supported topology specifications.
 75     // An empty TopologySelectorTerm list means there is no topology restriction.
 76     // This field is only honored by servers that enable the VolumeScheduling feature.
 77     // +optional
 78     // +listType=atomic
 79     AllowedTopologies []v1.TopologySelectorTerm `json:"allowedTopologies,omitempty" protobuf:"bytes,8,rep,name=allowedTopologies"    `
vs:

staging/src/k8s.io/api/core/v1/types.go:
3469 // A topology selector term represents the result of label queries.
3470 // A null or empty topology selector term matches no objects.
3471 // The requirements of them are ANDed.
3472 // It provides a subset of functionality as NodeSelectorTerm.
3473 // This is an alpha feature and may change in the future.
3474 // +structType=atomic
3475 type TopologySelectorTerm struct {
@msau42 @jsafrane - can you spend a moment thinking about this?

I see two paths:

Loosen validation in the "usual" way: add an alpha gate, tolerate the value in 33, move the gate to beta or even GA in 34.

Fix validation to fail on empty terms with a proper error, document it.

@msau42 @jsafrane - can you spend a moment thinking about this? Thanks.

k8s-triage-robot · 2025-05-30T01:38:59Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2025-06-29T01:50:51Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. labels Oct 1, 2024

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 1, 2024

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 Oct 1, 2024

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Oct 1, 2024

k8s-ci-robot requested review from deads2k and xing-yang October 1, 2024 14:58

googs1025 reviewed Oct 2, 2024

View reviewed changes

mmorel-35 mentioned this pull request Oct 3, 2024

fix: apply makezero linter with golangci strict and hints #127839

Closed

k8s-ci-robot assigned thockin Oct 3, 2024

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 Oct 3, 2024

thockin reviewed Oct 3, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 3, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 3, 2024

thockin removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2024

cuishuang mentioned this pull request Oct 14, 2024

proposal: x/tools/go/analysis/passes/appends: check for incorrect slice length initialization golang/go#69872

Open

fix: fix slice init length

6d853da

Signed-off-by: huochexizhan <[email protected]>

huochexizhan force-pushed the master branch from 225fbcf to 6d853da Compare November 19, 2024 04:39

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 17, 2025

thockin removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 17, 2025

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 30, 2025

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jun 29, 2025

fix: fix slice init length #127785

Are you sure you want to change the base?

fix: fix slice init length #127785

Uh oh!

Conversation

huochexizhan commented Oct 1, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

linux-foundation-easycla bot commented Oct 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Oct 1, 2024

Uh oh!

k8s-ci-robot commented Oct 1, 2024

Uh oh!

k8s-ci-robot commented Oct 1, 2024

Uh oh!

mmorel-35 commented Oct 2, 2024

Uh oh!

googs1025 Oct 2, 2024

Choose a reason for hiding this comment

Uh oh!

aojea Oct 3, 2024

Choose a reason for hiding this comment

Uh oh!

googs1025 Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

aojea Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

huochexizhan Oct 8, 2024

Choose a reason for hiding this comment

Uh oh!

aojea commented Oct 3, 2024

Uh oh!

thockin left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Oct 3, 2024

Uh oh!

k8s-ci-robot commented Oct 3, 2024

Uh oh!

k8s-triage-robot commented Oct 4, 2024

Uh oh!

k8s-triage-robot commented Oct 4, 2024

Uh oh!

k8s-triage-robot commented Oct 4, 2024

Uh oh!

aojea commented Oct 4, 2024

Uh oh!

huochexizhan commented Oct 8, 2024

Uh oh!

thockin commented Nov 7, 2024

Uh oh!

thockin commented Nov 7, 2024

Uh oh!

huochexizhan commented Nov 19, 2024

Uh oh!

thockin commented Nov 19, 2024

Uh oh!

k8s-triage-robot commented Feb 17, 2025

Uh oh!

huochexizhan commented Feb 28, 2025

Uh oh!

thockin commented Feb 28, 2025

Uh oh!

huochexizhan commented Mar 1, 2025

Uh oh!

huochexizhan commented Mar 1, 2025

Uh oh!

k8s-triage-robot commented May 30, 2025

Uh oh!

k8s-triage-robot commented Jun 29, 2025

Uh oh!

linux-foundation-easycla bot commented Oct 1, 2024 •

edited

Loading