NodeSelectorAsSelector needs to be fixed

[aojea]

Spotted by @liggitt

7093b10#r103711882

7093b10#r103711822

The function needs to be fixed based on Jordan comments before moving the feature to beta.

This issue is a blocker for graduating the feature.

kubernetes/pkg/apis/core/v1/helper/helpers.go

Lines 397 to 431 in 20e4491

	// NodeSelectorAsSelector converts the NodeSelector api type into a struct that
	// implements labels.Selector
	// Note: This function should be kept in sync with the selector methods in
	// pkg/labels/selector.go
	func NodeSelectorAsSelector(ns *v1.NodeSelector) (labels.Selector, error) {
	if ns == nil {
	return labels.Nothing(), nil
	}
	if len(ns.NodeSelectorTerms) == 0 {
	return labels.Everything(), nil
	}
	var requirements []labels.Requirement
	for _, nsTerm := range ns.NodeSelectorTerms {
	for _, expr := range nsTerm.MatchExpressions {
	req, err := nodeSelectorRequirementsAsLabelRequirements(expr)
	if err != nil {
	return nil, err
	}
	requirements = append(requirements, *req)
	}
	for _, field := range nsTerm.MatchFields {
	req, err := nodeSelectorRequirementsAsLabelRequirements(field)
	if err != nil {
	return nil, err
	}
	requirements = append(requirements, *req)
	}
	}
	selector := labels.NewSelector()
	selector = selector.Add(requirements...)
	return selector, nil
	}

/sig network
/kind bug
/assign @sarveshr7

[k8s-ci-robot]

@aojea: GitHub didn't allow me to assign the following users: sarveshr7.

Note that only kubernetes members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

Spotted by @liggitt

7093b10#r103711882

7093b10#r103711822

The function needs to be fixed based on Jordan comments before moving the feature to beta.

This issue is a blocker for graduating the feature.

kubernetes/pkg/apis/core/v1/helper/helpers.go

Lines 397 to 431 in 20e4491

	// NodeSelectorAsSelector converts the NodeSelector api type into a struct that
	// implements labels.Selector
	// Note: This function should be kept in sync with the selector methods in
	// pkg/labels/selector.go
	func NodeSelectorAsSelector(ns *v1.NodeSelector) (labels.Selector, error) {
	if ns == nil {
	return labels.Nothing(), nil
	}
	if len(ns.NodeSelectorTerms) == 0 {
	return labels.Everything(), nil
	}
	var requirements []labels.Requirement
	for _, nsTerm := range ns.NodeSelectorTerms {
	for _, expr := range nsTerm.MatchExpressions {
	req, err := nodeSelectorRequirementsAsLabelRequirements(expr)
	if err != nil {
	return nil, err
	}
	requirements = append(requirements, *req)
	}
	for _, field := range nsTerm.MatchFields {
	req, err := nodeSelectorRequirementsAsLabelRequirements(field)
	if err != nil {
	return nil, err
	}
	requirements = append(requirements, *req)
	}
	}
	selector := labels.NewSelector()
	selector = selector.Add(requirements...)
	return selector, nil
	}

/sig network
/kind bug
/assign @sarveshr7

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.

[aojea]

/priority important-soon

[liggitt]

if this can't be fixed quickly (e.g. in time for 1.27), please move the function to the multi-cidr package and unexport it so no one else starts using the buggy helper method in the meantime

[aojea]

we should remove it from helper and make it local, it doesn't make sense to export it if is only used there
https://grep.app/search?q=NodeSelectorAsSelector

[aojea]

/assign

let me see if is not as complicated as I thought

[MikeZappa87]

/triage accepted

[k8s-triage-robot]

This issue is labeled with priority/important-soon but has not been updated in over 90 days, and should be re-triaged.
Important-soon issues must be staffed and worked on either currently, or very soon, ideally in time for the next release.

You can:

• Confirm that this issue is still relevant with /triage accepted (org members only)
• Deprioritize it with /priority important-longterm or /priority backlog
• Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

[thockin]

Should we triage-accept this?

[aojea]

Should we triage-accept this?

this is blocked on the go/no-go decision of the previously called ClusterCIDR KEP and now NodeIPAMConfig

kubernetes/enhancements#4174

please @danwinship @thockin we need to decide if we want to make it or remove it

[k8s-triage-robot]

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

This bot triages un-triaged issues 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 issue is closed

You can:

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

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

/lifecycle stale

[danwinship]

this is blocked on the go/no-go decision of the previously called ClusterCIDR KEP and now NodeIPAMConfig

that's no longer in-tree... I'm not sure what happened with the code mentioned here?

[k8s-triage-robot]

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

This bot triages un-triaged issues 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 issue is closed

You can:

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

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

/lifecycle rotten

[uablrek]

that's no longer in-tree... I'm not sure what happened with the code mentioned here?

Moved to https://github.com/kubernetes-sigs/node-ipam-controller I suppose. Anyway, this is no longer a K8s issue, closing ...

/close

[k8s-ci-robot]

@uablrek: Closing this issue.

In response to this:

that's no longer in-tree... I'm not sure what happened with the code mentioned here?

Moved to https://github.com/kubernetes-sigs/node-ipam-controller I suppose. Anyway, this is no longer a K8s issue, closing ...

/close

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.