-
Notifications
You must be signed in to change notification settings - Fork 39.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce APIs to support multiple ClusterCIDRs #111123
Conversation
Hi @sarveshr7. 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 Once the patch is verified, the new status will be reflected by the 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/test-infra repository. |
@sarveshr7: GitHub didn't allow me to request PR reviews from the following users: rahulkjoshi. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this: 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. |
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
pkg/apis/core/v1/helper/helpers.go
Outdated
func NodeSelectorRequirementsAsLabelRequirements(nsr v1.NodeSelectorRequirement) (*labels.Requirement, error) { | ||
var op selection.Operator | ||
switch nsr.Operator { | ||
case v1.NodeSelectorOpIn: | ||
op = selection.In | ||
case v1.NodeSelectorOpNotIn: | ||
op = selection.NotIn | ||
case v1.NodeSelectorOpExists: | ||
op = selection.Exists | ||
case v1.NodeSelectorOpDoesNotExist: | ||
op = selection.DoesNotExist | ||
case v1.NodeSelectorOpGt: | ||
op = selection.GreaterThan | ||
case v1.NodeSelectorOpLt: | ||
op = selection.LessThan | ||
default: | ||
return nil, fmt.Errorf("%q is not a valid node selector operator", nsr.Operator) | ||
} | ||
return labels.NewRequirement(nsr.Key, op, nsr.Values) | ||
} | ||
|
||
// 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 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is not equivalent to ?
kubernetes/pkg/apis/core/helper/helpers.go
Lines 421 to 453 in 33b20f2
// NodeSelectorRequirementsAsSelector converts the []NodeSelectorRequirement core type into a struct that implements | |
// labels.Selector. | |
func NodeSelectorRequirementsAsSelector(nsm []core.NodeSelectorRequirement) (labels.Selector, error) { | |
if len(nsm) == 0 { | |
return labels.Nothing(), nil | |
} | |
selector := labels.NewSelector() | |
for _, expr := range nsm { | |
var op selection.Operator | |
switch expr.Operator { | |
case core.NodeSelectorOpIn: | |
op = selection.In | |
case core.NodeSelectorOpNotIn: | |
op = selection.NotIn | |
case core.NodeSelectorOpExists: | |
op = selection.Exists | |
case core.NodeSelectorOpDoesNotExist: | |
op = selection.DoesNotExist | |
case core.NodeSelectorOpGt: | |
op = selection.GreaterThan | |
case core.NodeSelectorOpLt: | |
op = selection.LessThan | |
default: | |
return nil, fmt.Errorf("%q is not a valid node selector operator", expr.Operator) | |
} | |
r, err := labels.NewRequirement(expr.Key, op, expr.Values) | |
if err != nil { | |
return nil, err | |
} | |
selector = selector.Add(*r) | |
} | |
return selector, nil | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NodeSelectorRequirementsAsSelector method is defined for core.NodeSelectorRequirement
object however we need to use core.v1.NodeSelectorRequirement
, also I wanted a method similar to LabelSelectorAsSelector which would take in the NodeSelector as input and return the selector as output, without iterating through the requirements everytime
Searched for the usage of NodeSelectorRequirementsAsSelector
throughout the code, however it doesn't seem to be used anywhere
// ValidateClusterCIDRConfigUpdate tests if an update to a ClusterCIDRConfig is valid. | ||
func ValidateClusterCIDRConfigUpdate(update, old *networking.ClusterCIDRConfig) field.ErrorList { | ||
var allErrs field.ErrorList | ||
allErrs = append(allErrs, apivalidation.ValidateObjectMetaUpdate(&update.ObjectMeta, &old.ObjectMeta, field.NewPath("metadata"))...) | ||
allErrs = append(allErrs, validateClusterCIDRConfigUpdateSpec(&update.Spec, &old.Spec, field.NewPath("spec"))...) | ||
return allErrs | ||
} | ||
|
||
func validateClusterCIDRConfigUpdateSpec(update, old *networking.ClusterCIDRConfigSpec, fldPath *field.Path) field.ErrorList { | ||
var allErrs field.ErrorList | ||
|
||
allErrs = append(allErrs, apivalidation.ValidateImmutableField(update.NodeSelector, old.NodeSelector, fldPath.Child("nodeSelector"))...) | ||
allErrs = append(allErrs, apivalidation.ValidateImmutableField(update.PerNodeHostBits, old.PerNodeHostBits, fldPath.Child("perNodeHostBits"))...) | ||
allErrs = append(allErrs, apivalidation.ValidateImmutableField(update.IPv4CIDR, old.IPv4CIDR, fldPath.Child("ipv4CIDR"))...) | ||
allErrs = append(allErrs, apivalidation.ValidateImmutableField(update.IPv6CIDR, old.IPv6CIDR, fldPath.Child("ipv6CIDR"))...) | ||
|
||
return allErrs | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thockin if we don't allow Updates, do we need to add it to validation or is enough with denying it in the registry?
plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go
Outdated
Show resolved
Hide resolved
// Status is the current state of the ClusterCIDRConfig. | ||
// More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#spec-and-status | ||
// +optional | ||
Status ClusterCIDRConfigStatus `json:"status,omitempty" protobuf:"bytes,3,opt,name=status"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thockin do we need status if we don't use it?
(I have the same question in the internal types)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
We've talked offline, some of the outcome of the conversation between Tim, Sarvesh and I are
|
d00df7d
to
3ec4f72
Compare
/retest |
3ec4f72
to
f2fd607
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sarveshr7 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
f2fd607
to
76041b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly minor things in the API PR. Will try to look at IMPL in the AM
expectErr: false, | ||
}, | ||
{ | ||
name: "valid SingleStack IPv6 ClusterCIDR perNodeHostBits=100", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this testing something the above doesn't?
PerNodeHostBits: int32(8), | ||
IPv4: "10.1.0.0/16", | ||
IPv6: "fd00:1:1::/64", | ||
NodeSelector: &api.NodeSelector{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above, you can make this denser
|
||
test := genericregistrytest.New(t, storage.Store) | ||
test = test.ClusterScope() | ||
validCCC := validClusterCIDR() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about taking a variadic list of funcs, like:
func validClusterCIDR(tweaks ...func(*ClusterCIDR)) *networking.ClusterCIDR {
cc := newClusterCIDR()
for t := range tweaks {
t(cc)
}
return cc
}
then:
validCCC := validClusterCIDR()
noCIDRCCC := validClusterCIDR(func(cc *ClusterCIDR) {
cc.Spec.IPv4 = ""
cc.Spec.IPv6 = ""
})
invalidCCCPerNodeHostBits := validClusterCIDR(func(cc *ClusterCIDR) {
cc.Spec.PerNodeHostBits = 100
})
invalidCCCCIDR := validClusterCIDR(func(cc *ClusterCIDR) {
cc.Spec.IPv6 = "10.1.0.0/16"
})
Easier to read, IMO.
Also s/CCC/CC/g
plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go
Outdated
Show resolved
Hide resolved
Introduce networking/v1alpha1 api group. Add `ClusterCIDR` type to networking/v1alpha1 api group, this type will enable the NodeIPAM controller to support multiple ClusterCIDRs.
76041b7
to
89ec54c
Compare
/test pull-kubernetes-e2e-ubuntu-gce-network-policies |
Hello 👋, 1.25 Release Lead here. The exception request is approved and your updated deadline to make any changes to your PR is 3:30 AM PST Monday 8th August 2022. Thank you! /milestone v1.25 |
Let's focus the review on #109090 which includes these commits, so we can merge atomically or not at all. Fair? |
Sounds good! Had separated the PRs to make the review easier, as the API review is done and the commits are present in #109090, we can merge atomically. |
What type of PR is this?
/kind feature
/kind api-change
What this PR does / why we need it:
This PR implements APIs from kubernetes/enhancements#2593
API Changes:
Which issue(s) this PR fixes:
NONE
Special notes for your reviewer:
#108290 was already approved and merged in 1.24 , however was reverted by #109436 as it was merged post the 1.24 release cut deadline. Rebasing the old PR and planning to get it in for 1.25
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: