Skip to content

Commit

Permalink
Updates related to PR feedback
Browse files Browse the repository at this point in the history 
- Remove feature gate consideration from EndpointSlice validation
- Deprecate topology field, note that it will be removed in future
release
- Update kube-proxy to check for NodeName if feature gate is enabled
- Add comments indicating the feature gates that can be used to enable
alpha API fields
- Add comments explaining use of deprecated address type in tests
  • Loading branch information
@robscott
robscott committed Nov 12, 2020
1 parent b98cab7 commit 84e4b30
Show file tree
Hide file tree
Showing 15 changed files with 104 additions and 176 deletions.
8 changes: 4 additions & 4 deletions api/openapi-spec/swagger.json
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 12 additions & 8 deletions pkg/apis/discovery/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,11 +99,12 @@ type Endpoint struct {
// endpoint is located. This should match the corresponding node label.
// * topology.kubernetes.io/region: the value indicates the region where the
// endpoint is located. This should match the corresponding node label.
// this field will be deprecated in an upcoming release.
// This field is deprecated and will be removed in future api versions.
// +optional
Topology map[string]string
// nodeName represents the name of the Node hosting this endpoint. This can
// be used to determine endpoints local to a Node.
// be used to determine endpoints local to a Node. This field can be enabled
// with the EndpointSliceNodeName feature gate.
// +optional
NodeName *string
}
Expand All @@ -117,15 +118,18 @@ type EndpointConditions struct {
// "true" for terminating endpoints.
Ready *bool

// serving is identical to ready except that it is set regardless of the terminating
// state of endpoints. This condition should be set to true for a ready endpoint that
// is terminating. If nil, consumers should defer to the ready condition.
// serving is identical to ready except that it is set regardless of the
// terminating state of endpoints. This condition should be set to true for
// a ready endpoint that is terminating. If nil, consumers should defer to
// the ready condition. This field can be enabled with the
// EndpointSliceTerminatingCondition feature gate.
// +optional
Serving *bool

// terminating indicates that this endpoint is terminating. A nil value indicates an
// unknown state. Consumers should interpret this unknown state to mean that the
// endpoint is not terminating.
// terminating indicates that this endpoint is terminating. A nil value
// indicates an unknown state. Consumers should interpret this unknown state
// to mean that the endpoint is not terminating. This field can be enabled
// with the EndpointSliceTerminatingCondition feature gate.
// +optional
Terminating *bool
}
Expand Down
5 changes: 0 additions & 5 deletions pkg/apis/discovery/validation/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,11 @@ go_library(
"//pkg/apis/core:go_default_library",
"//pkg/apis/core/validation:go_default_library",
"//pkg/apis/discovery:go_default_library",
"//pkg/features:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/api/validation:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/validation:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
],
)

Expand All @@ -26,10 +24,7 @@ go_test(
deps = [
"//pkg/apis/core:go_default_library",
"//pkg/apis/discovery:go_default_library",
"//pkg/features:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
"//staging/src/k8s.io/component-base/featuregate/testing:go_default_library",
"//vendor/k8s.io/utils/pointer:go_default_library",
],
)
Expand Down
35 changes: 7 additions & 28 deletions pkg/apis/discovery/validation/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,9 @@ import (
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/util/validation"
"k8s.io/apimachinery/pkg/util/validation/field"
utilfeature "k8s.io/apiserver/pkg/util/feature"
api "k8s.io/kubernetes/pkg/apis/core"
apivalidation "k8s.io/kubernetes/pkg/apis/core/validation"
"k8s.io/kubernetes/pkg/apis/discovery"
"k8s.io/kubernetes/pkg/features"
)

var (
Expand All @@ -52,44 +50,29 @@ var (
var ValidateEndpointSliceName = apimachineryvalidation.NameIsDNSSubdomain

// ValidateEndpointSlice validates an EndpointSlice.
func ValidateEndpointSlice(endpointSlice *discovery.EndpointSlice, allowNodeName bool) field.ErrorList {
func ValidateEndpointSlice(endpointSlice *discovery.EndpointSlice) field.ErrorList {
allErrs := apivalidation.ValidateObjectMeta(&endpointSlice.ObjectMeta, true, ValidateEndpointSliceName, field.NewPath("metadata"))
allErrs = append(allErrs, validateAddressType(endpointSlice.AddressType)...)
allErrs = append(allErrs, validateEndpoints(endpointSlice.Endpoints, endpointSlice.AddressType, allowNodeName, field.NewPath("endpoints"))...)
allErrs = append(allErrs, validateEndpoints(endpointSlice.Endpoints, endpointSlice.AddressType, field.NewPath("endpoints"))...)
allErrs = append(allErrs, validatePorts(endpointSlice.Ports, field.NewPath("ports"))...)

return allErrs
}

// ValidateEndpointSliceCreate validates an EndpointSlice when it is created.
func ValidateEndpointSliceCreate(endpointSlice *discovery.EndpointSlice) field.ErrorList {
// allow NodeName value if the feature gate is set.
allowNodeName := utilfeature.DefaultFeatureGate.Enabled(features.EndpointSliceNodeName)

return ValidateEndpointSlice(endpointSlice, allowNodeName)
return ValidateEndpointSlice(endpointSlice)
}

// ValidateEndpointSliceUpdate validates an EndpointSlice when it is updated.
func ValidateEndpointSliceUpdate(newEndpointSlice, oldEndpointSlice *discovery.EndpointSlice) field.ErrorList {
// allow NodeName value if the feature gate is set.
allowNodeName := utilfeature.DefaultFeatureGate.Enabled(features.EndpointSliceNodeName)

if !allowNodeName {
for _, ep := range oldEndpointSlice.Endpoints {
if ep.NodeName != nil {
allowNodeName = true
break
}
}
}

allErrs := ValidateEndpointSlice(newEndpointSlice, allowNodeName)
allErrs := ValidateEndpointSlice(newEndpointSlice)
allErrs = append(allErrs, apivalidation.ValidateImmutableField(newEndpointSlice.AddressType, oldEndpointSlice.AddressType, field.NewPath("addressType"))...)

return allErrs
}

func validateEndpoints(endpoints []discovery.Endpoint, addrType discovery.AddressType, allowNodeName bool, fldPath *field.Path) field.ErrorList {
func validateEndpoints(endpoints []discovery.Endpoint, addrType discovery.AddressType, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}

if len(endpoints) > maxEndpoints {
Expand Down Expand Up @@ -122,12 +105,8 @@ func validateEndpoints(endpoints []discovery.Endpoint, addrType discovery.Addres

if endpoint.NodeName != nil {
nnPath := idxPath.Child("nodeName")
if allowNodeName {
for _, msg := range apivalidation.ValidateNodeName(*endpoint.NodeName, false) {
allErrs = append(allErrs, field.Invalid(nnPath, *endpoint.NodeName, msg))
}
} else {
allErrs = append(allErrs, field.Forbidden(nnPath, "may not be set unless EndpointSliceNodeName feature gate is enabled"))
for _, msg := range apivalidation.ValidateNodeName(*endpoint.NodeName, false) {
allErrs = append(allErrs, field.Invalid(nnPath, *endpoint.NodeName, msg))
}
}

Expand Down
93 changes: 5 additions & 88 deletions pkg/apis/discovery/validation/validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,8 @@ import (
"testing"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
utilfeature "k8s.io/apiserver/pkg/util/feature"
featuregatetesting "k8s.io/component-base/featuregate/testing"
api "k8s.io/kubernetes/pkg/apis/core"
"k8s.io/kubernetes/pkg/apis/discovery"
"k8s.io/kubernetes/pkg/features"
utilpointer "k8s.io/utils/pointer"
)

Expand Down Expand Up @@ -461,7 +458,7 @@ func TestValidateEndpointSlice(t *testing.T) {

for name, testCase := range testCases {
t.Run(name, func(t *testing.T) {
errs := ValidateEndpointSlice(testCase.endpointSlice, true)
errs := ValidateEndpointSlice(testCase.endpointSlice)
if len(errs) != testCase.expectedErrors {
t.Errorf("Expected %d errors, got %d errors: %v", testCase.expectedErrors, len(errs), errs)
}
Expand Down Expand Up @@ -496,8 +493,7 @@ func TestValidateEndpointSliceCreate(t *testing.T) {
},
},
"good-slice-node-name": {
expectedErrors: 0,
nodeNameGateEnabled: true,
expectedErrors: 0,
endpointSlice: &discovery.EndpointSlice{
ObjectMeta: standardMeta,
AddressType: discovery.AddressTypeIPv4,
Expand All @@ -515,8 +511,7 @@ func TestValidateEndpointSliceCreate(t *testing.T) {

// expected failures
"bad-node-name": {
expectedErrors: 1,
nodeNameGateEnabled: true,
expectedErrors: 1,
endpointSlice: &discovery.EndpointSlice{
ObjectMeta: standardMeta,
AddressType: discovery.AddressTypeIPv4,
Expand All @@ -531,23 +526,6 @@ func TestValidateEndpointSliceCreate(t *testing.T) {
}},
},
},
"node-name-disabled": {
expectedErrors: 1,
nodeNameGateEnabled: false,
endpointSlice: &discovery.EndpointSlice{
ObjectMeta: standardMeta,
AddressType: discovery.AddressTypeIPv4,
Ports: []discovery.EndpointPort{{
Name: utilpointer.StringPtr("http"),
Protocol: protocolPtr(api.ProtocolTCP),
}},
Endpoints: []discovery.Endpoint{{
Addresses: generateIPAddresses(1),
Hostname: utilpointer.StringPtr("valid-123"),
NodeName: utilpointer.StringPtr("valid-node-name"),
}},
},
},
"deprecated-address-type": {
expectedErrors: 1,
endpointSlice: &discovery.EndpointSlice{
Expand Down Expand Up @@ -579,8 +557,6 @@ func TestValidateEndpointSliceCreate(t *testing.T) {
}

for name, testCase := range testCases {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.EndpointSliceNodeName, testCase.nodeNameGateEnabled)()

t.Run(name, func(t *testing.T) {
errs := ValidateEndpointSliceCreate(testCase.endpointSlice)
if len(errs) != testCase.expectedErrors {
Expand Down Expand Up @@ -610,48 +586,9 @@ func TestValidateEndpointSliceUpdate(t *testing.T) {
},
expectedErrors: 0,
},
"node name set before + after, gate disabled": {
oldEndpointSlice: &discovery.EndpointSlice{
ObjectMeta: standardMeta,
AddressType: discovery.AddressTypeIPv4,
Endpoints: []discovery.Endpoint{{
Addresses: []string{"10.1.2.3"},
NodeName: utilpointer.StringPtr("foo"),
}},
},
newEndpointSlice: &discovery.EndpointSlice{
ObjectMeta: standardMeta,
AddressType: discovery.AddressTypeIPv4,
Endpoints: []discovery.Endpoint{{
Addresses: []string{"10.1.2.3"},
NodeName: utilpointer.StringPtr("foo"),
}},
},
expectedErrors: 0,
},
"node name set after, gate enabled": {
nodeNameGateEnabled: true,
oldEndpointSlice: &discovery.EndpointSlice{
ObjectMeta: standardMeta,
AddressType: discovery.AddressTypeIPv4,
Endpoints: []discovery.Endpoint{{
Addresses: []string{"10.1.2.3"},
}},
},
newEndpointSlice: &discovery.EndpointSlice{
ObjectMeta: standardMeta,
AddressType: discovery.AddressTypeIPv4,
Endpoints: []discovery.Endpoint{{
Addresses: []string{"10.1.2.3"},
NodeName: utilpointer.StringPtr("foo"),
}},
},
expectedErrors: 0,
},

// expected errors
"invalide node name set after, gate enabled": {
nodeNameGateEnabled: true,
"invalide node name set": {
oldEndpointSlice: &discovery.EndpointSlice{
ObjectMeta: standardMeta,
AddressType: discovery.AddressTypeIPv4,
Expand All @@ -669,25 +606,7 @@ func TestValidateEndpointSliceUpdate(t *testing.T) {
},
expectedErrors: 1,
},
"node name set after, gate disabled": {
nodeNameGateEnabled: false,
oldEndpointSlice: &discovery.EndpointSlice{
ObjectMeta: standardMeta,
AddressType: discovery.AddressTypeIPv4,
Endpoints: []discovery.Endpoint{{
Addresses: []string{"10.1.2.3"},
}},
},
newEndpointSlice: &discovery.EndpointSlice{
ObjectMeta: standardMeta,
AddressType: discovery.AddressTypeIPv4,
Endpoints: []discovery.Endpoint{{
Addresses: []string{"10.1.2.3"},
NodeName: utilpointer.StringPtr("foo"),
}},
},
expectedErrors: 1,
},

"deprecated address type": {
expectedErrors: 1,
oldEndpointSlice: &discovery.EndpointSlice{
Expand Down Expand Up @@ -729,8 +648,6 @@ func TestValidateEndpointSliceUpdate(t *testing.T) {

for name, testCase := range testCases {
t.Run(name, func(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.EndpointSliceNodeName, testCase.nodeNameGateEnabled)()

errs := ValidateEndpointSliceUpdate(testCase.newEndpointSlice, testCase.oldEndpointSlice)
if len(errs) != testCase.expectedErrors {
t.Errorf("Expected %d errors, got %d errors: %v", testCase.expectedErrors, len(errs), errs)
Expand Down
1 change: 1 addition & 0 deletions pkg/controller/endpointslice/reconciler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -917,6 +917,7 @@ func TestReconcileEndpointSlicesReplaceDeprecated(t *testing.T) {
namespace := "test"

svc, endpointMeta := newServiceAndEndpointMeta("foo", namespace)
// "IP" is a deprecated address type, ensuring that it is handled properly.
endpointMeta.AddressType = discovery.AddressType("IP")

existingSlices := []*discovery.EndpointSlice{}
Expand Down
1 change: 1 addition & 0 deletions pkg/controlplane/reconcilers/endpointsadapter_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,7 @@ func TestEndpointsAdapterUpdate(t *testing.T) {
// with one that has an IPv4 address type.
endpoints4, _ := generateEndpointsAndSlice("foo", "testing", []int{80}, []string{"10.1.2.7", "10.1.2.8"})
_, epSlice4IP := generateEndpointsAndSlice("foo", "testing", []int{80}, []string{"10.1.2.7", "10.1.2.8"})
// "IP" is a deprecated address type, ensuring that it is handled properly.
epSlice4IP.AddressType = discovery.AddressType("IP")
_, epSlice4IPv4 := generateEndpointsAndSlice("foo", "testing", []int{80}, []string{"10.1.2.7", "10.1.2.8"})

Expand Down
2 changes: 2 additions & 0 deletions pkg/proxy/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,15 @@ go_library(
importpath = "k8s.io/kubernetes/pkg/proxy",
deps = [
"//pkg/api/v1/service:go_default_library",
"//pkg/features:go_default_library",
"//pkg/proxy/config:go_default_library",
"//pkg/proxy/metrics:go_default_library",
"//pkg/proxy/util:go_default_library",
"//staging/src/k8s.io/api/core/v1:go_default_library",
"//staging/src/k8s.io/api/discovery/v1beta1:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/types:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
"//staging/src/k8s.io/client-go/tools/record:go_default_library",
"//vendor/k8s.io/klog/v2:go_default_library",
"//vendor/k8s.io/utils/net:go_default_library",
Expand Down
Loading

0 comments on commit 84e4b30

Please sign in to comment.