github/kubernetes
1
0
Fork 0
mirror of https://github.com/k3s-io/kubernetes.git synced 2025-07-23 19:56:01 +00:00
Code Issues Projects Releases Wiki Activity

service: fix IPFamily validation and defaulting problems

Browse Source 
If the dual-stack flag is enabled and the cluster is single stack IPv6,
the allocator logic for service clusterIP does not properly handle rejecting
a request for an IPv4 family. Return a 422 Invalid on the ipFamily field
when the dual stack flag is on (as it would when it hits beta) and the
cluster is configured for single-stack IPv6.

The family is now defaulted or cleared in BeforeCreate/BeforeUpdate,
and is either inherited from the previous object (if nil or unchanged),
or set to the default strategy's family as necessary. The existing
family defaulting when cluster ip is provided remains in the api
section. We add additonal family defaulting at the time we allocate
the IP to ensure that IPFamily is a consequence of the ClusterIP
and prevent accidental reversion. This defaulting also ensures that
old clients that submit a nil IPFamily for non ClusterIP services
receive a default.

To properly handle validation, make the strategy and the validation code
path condition on which configuration options are passed to service
storage. Move validation and preparation logic inside the strategy where
it belongs. Service validation is now dependent on the configuration of
the server, and as such ValidateConditionService needs to know what the
allowed families are.
This commit is contained in:
Clayton Coleman 2020-01-06 19:50:04 -05:00 committed by Dan Winship
parent f01d848c48
commit c6b833ac3c
13 changed files with 1263 additions and 363 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

84
pkg/apis/core/validation/conditional_validation.go
View File

@ -17,14 +17,21 @@ limitations under the License.
package validation
import (
"fmt"
"net"
"strings"
"k8s.io/apimachinery/pkg/util/validation/field"
utilfeature "k8s.io/apiserver/pkg/util/feature"
api "k8s.io/kubernetes/pkg/apis/core"
"k8s.io/kubernetes/pkg/features"
netutils "k8s.io/utils/net"
)
// ValidateConditionalService validates conditionally valid fields.
func ValidateConditionalService(service, oldService *api.Service) field.ErrorList {
// ValidateConditionalService validates conditionally valid fields. allowedIPFamilies is an ordered
// list of the valid IP families (IPv4 or IPv6) that are supported. The first family in the slice
// is the cluster default, although the clusterIP here dictates the family defaulting.
func ValidateConditionalService(service, oldService *api.Service, allowedIPFamilies []api.IPFamily) field.ErrorList {
var errs field.ErrorList
// If the SCTPSupport feature is disabled, and the old object isn't using the SCTP feature, prevent the new object from using it
if !utilfeature.DefaultFeatureGate.Enabled(features.SCTPSupport) && len(serviceSCTPFields(oldService)) == 0 {
@ -32,9 +39,82 @@ func ValidateConditionalService(service, oldService *api.Service) field.ErrorLis
errs = append(errs, field.NotSupported(f, api.ProtocolSCTP, []string{string(api.ProtocolTCP), string(api.ProtocolUDP)}))
}
}
errs = append(errs, validateIPFamily(service, oldService, allowedIPFamilies)...)
return errs
}
// validateIPFamily checks the IPFamily field.
func validateIPFamily(service, oldService *api.Service, allowedIPFamilies []api.IPFamily) field.ErrorList {
var errs field.ErrorList
// specifically allow an invalid value to remain in storage as long as the user isn't changing it, regardless of gate
if oldService != nil && oldService.Spec.IPFamily != nil && service.Spec.IPFamily != nil && *oldService.Spec.IPFamily == *service.Spec.IPFamily {
return errs
}
// If the gate is off, setting or changing IPFamily is not allowed, but clearing it is
if !utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) {
if service.Spec.IPFamily != nil {
if oldService != nil {
errs = append(errs, ValidateImmutableField(service.Spec.IPFamily, oldService.Spec.IPFamily, field.NewPath("spec", "ipFamily"))...)
} else {
errs = append(errs, field.Forbidden(field.NewPath("spec", "ipFamily"), "programmer error, must be cleared when the dual-stack feature gate is off"))
}
}
return errs
}
// PrepareCreate, PrepareUpdate, and test cases must all set IPFamily when the gate is on
if service.Spec.IPFamily == nil {
errs = append(errs, field.Required(field.NewPath("spec", "ipFamily"), "programmer error, must be set or defaulted by other fields"))
return errs
}
// A user is not allowed to change the IPFamily field, except for ExternalName services
if oldService != nil && oldService.Spec.IPFamily != nil && service.Spec.Type != api.ServiceTypeExternalName {
errs = append(errs, ValidateImmutableField(service.Spec.IPFamily, oldService.Spec.IPFamily, field.NewPath("spec", "ipFamily"))...)
}
// Verify the IPFamily is one of the allowed families
desiredFamily := *service.Spec.IPFamily
if hasIPFamily(allowedIPFamilies, desiredFamily) {
// the IP family is one of the allowed families, verify that it matches cluster IP
switch ip := net.ParseIP(service.Spec.ClusterIP); {
case ip == nil:
// do not need to check anything
case netutils.IsIPv6(ip) && desiredFamily != api.IPv6Protocol:
errs = append(errs, field.Invalid(field.NewPath("spec", "ipFamily"), *service.Spec.IPFamily, "does not match IPv6 cluster IP"))
case !netutils.IsIPv6(ip) && desiredFamily != api.IPv4Protocol:
errs = append(errs, field.Invalid(field.NewPath("spec", "ipFamily"), *service.Spec.IPFamily, "does not match IPv4 cluster IP"))
}
} else {
errs = append(errs, field.Invalid(field.NewPath("spec", "ipFamily"), desiredFamily, fmt.Sprintf("only the following families are allowed: %s", joinIPFamilies(allowedIPFamilies, ", "))))
}
return errs
}
func hasIPFamily(families []api.IPFamily, family api.IPFamily) bool {
for _, allow := range families {
if allow == family {
return true
}
}
return false
}
func joinIPFamilies(families []api.IPFamily, separator string) string {
var b strings.Builder
for i, family := range families {
if i != 0 {
b.WriteString(separator)
}
b.WriteString(string(family))
}
return b.String()
}
func serviceSCTPFields(service *api.Service) []*field.Path {
if service == nil {
return nil

240
pkg/apis/core/validation/conditional_validation_test.go
View File

@ -19,6 +19,7 @@ package validation
import (
"fmt"
"reflect"
"strings"
"testing"
"k8s.io/apimachinery/pkg/util/diff"
@ -153,7 +154,7 @@ func TestValidateServiceSCTP(t *testing.T) {
t.Run(fmt.Sprintf("feature enabled=%v, old object %v, new object %v", enabled, oldServiceInfo.description, newServiceInfo.description), func(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SCTPSupport, enabled)()
errs := ValidateConditionalService(newService, oldService)
errs := ValidateConditionalService(newService, oldService, []api.IPFamily{api.IPv4Protocol})
// objects should never be changed
if !reflect.DeepEqual(oldService, oldServiceInfo.object()) {
t.Errorf("old object changed: %v", diff.ObjectReflectDiff(oldService, oldServiceInfo.object()))
@ -251,3 +252,240 @@ func TestValidateEndpointsSCTP(t *testing.T) {
}
}
}
func TestValidateServiceIPFamily(t *testing.T) {
ipv4 := api.IPv4Protocol
ipv6 := api.IPv6Protocol
var unknown api.IPFamily = "Unknown"
testCases := []struct {
name string
dualStackEnabled bool
ipFamilies []api.IPFamily
svc *api.Service
oldSvc *api.Service
expectErr []string
}{
{
name: "allowed ipv4",
dualStackEnabled: true,
ipFamilies: []api.IPFamily{api.IPv4Protocol},
svc: &api.Service{
Spec: api.ServiceSpec{
IPFamily: &ipv4,
},
},
},
{
name: "allowed ipv6",
dualStackEnabled: true,
ipFamilies: []api.IPFamily{api.IPv6Protocol},
svc: &api.Service{
Spec: api.ServiceSpec{
IPFamily: &ipv6,
},
},
},
{
name: "allowed ipv4 dual stack default IPv6",
dualStackEnabled: true,
ipFamilies: []api.IPFamily{api.IPv6Protocol, api.IPv4Protocol},
svc: &api.Service{
Spec: api.ServiceSpec{
IPFamily: &ipv4,
},
},
},
{
name: "allowed ipv4 dual stack default IPv4",
dualStackEnabled: true,
ipFamilies: []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},
svc: &api.Service{
Spec: api.ServiceSpec{
IPFamily: &ipv4,
},
},
},
{
name: "allowed ipv6 dual stack default IPv6",
dualStackEnabled: true,
ipFamilies: []api.IPFamily{api.IPv6Protocol, api.IPv4Protocol},
svc: &api.Service{
Spec: api.ServiceSpec{
IPFamily: &ipv6,
},
},
},
{
name: "allowed ipv6 dual stack default IPv4",
dualStackEnabled: true,
ipFamilies: []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},
svc: &api.Service{
Spec: api.ServiceSpec{
IPFamily: &ipv6,
},
},
},
{
name: "allow ipfamily to remain invalid if update doesn't change it",
dualStackEnabled: true,
ipFamilies: []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},
svc: &api.Service{
Spec: api.ServiceSpec{
IPFamily: &unknown,
},
},
oldSvc: &api.Service{
Spec: api.ServiceSpec{
IPFamily: &unknown,
},
},
},
{
name: "not allowed ipfamily/clusterip mismatch",
dualStackEnabled: true,
ipFamilies: []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},
svc: &api.Service{
Spec: api.ServiceSpec{
IPFamily: &ipv4,
ClusterIP: "ffd0::1",
},
},
expectErr: []string{"spec.ipFamily: Invalid value: \"IPv4\": does not match IPv6 cluster IP"},
},
{
name: "not allowed unknown family",
dualStackEnabled: true,
ipFamilies: []api.IPFamily{api.IPv4Protocol},
svc: &api.Service{
Spec: api.ServiceSpec{
IPFamily: &unknown,
},
},
expectErr: []string{"spec.ipFamily: Invalid value: \"Unknown\": only the following families are allowed: IPv4"},
},
{
name: "not allowed ipv4 cluster ip without family",
dualStackEnabled: true,
ipFamilies: []api.IPFamily{api.IPv6Protocol},
svc: &api.Service{
Spec: api.ServiceSpec{
ClusterIP: "127.0.0.1",
},
},
expectErr: []string{"spec.ipFamily: Required value: programmer error, must be set or defaulted by other fields"},
},
{
name: "not allowed ipv6 cluster ip without family",
dualStackEnabled: true,
ipFamilies: []api.IPFamily{api.IPv4Protocol},
svc: &api.Service{
Spec: api.ServiceSpec{
ClusterIP: "ffd0::1",
},
},
expectErr: []string{"spec.ipFamily: Required value: programmer error, must be set or defaulted by other fields"},
},
{
name: "not allowed to change ipfamily for default type",
dualStackEnabled: true,
ipFamilies: []api.IPFamily{api.IPv4Protocol},
svc: &api.Service{
Spec: api.ServiceSpec{
IPFamily: &ipv4,
},
},
oldSvc: &api.Service{
Spec: api.ServiceSpec{
IPFamily: &ipv6,
},
},
expectErr: []string{"spec.ipFamily: Invalid value: \"IPv4\": field is immutable"},
},
{
name: "allowed to change ipfamily for external name",
dualStackEnabled: true,
ipFamilies: []api.IPFamily{api.IPv4Protocol},
svc: &api.Service{
Spec: api.ServiceSpec{
Type: api.ServiceTypeExternalName,
IPFamily: &ipv4,
},
},
oldSvc: &api.Service{
Spec: api.ServiceSpec{
Type: api.ServiceTypeExternalName,
IPFamily: &ipv6,
},
},
},
{
name: "ipfamily allowed to be empty when dual stack is off",
dualStackEnabled: false,
ipFamilies: []api.IPFamily{api.IPv4Protocol},
svc: &api.Service{
Spec: api.ServiceSpec{
ClusterIP: "127.0.0.1",
},
},
},
{
name: "ipfamily must be empty when dual stack is off",
dualStackEnabled: false,
ipFamilies: []api.IPFamily{api.IPv4Protocol},
svc: &api.Service{
Spec: api.ServiceSpec{
IPFamily: &ipv4,
ClusterIP: "127.0.0.1",
},
},
expectErr: []string{"spec.ipFamily: Forbidden: programmer error, must be cleared when the dual-stack feature gate is off"},
},
{
name: "ipfamily allowed to be cleared when dual stack is off",
dualStackEnabled: false,
ipFamilies: []api.IPFamily{api.IPv4Protocol},
svc: &api.Service{
Spec: api.ServiceSpec{
Type: api.ServiceTypeClusterIP,
ClusterIP: "127.0.0.1",
},
},
oldSvc: &api.Service{
Spec: api.ServiceSpec{
Type: api.ServiceTypeClusterIP,
ClusterIP: "127.0.0.1",
IPFamily: &ipv4,
},
},
expectErr: nil,
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, tc.dualStackEnabled)()
oldSvc := tc.oldSvc.DeepCopy()
newSvc := tc.svc.DeepCopy()
originalNewSvc := newSvc.DeepCopy()
errs := ValidateConditionalService(newSvc, oldSvc, tc.ipFamilies)
// objects should never be changed
if !reflect.DeepEqual(oldSvc, tc.oldSvc) {
t.Errorf("old object changed: %v", diff.ObjectReflectDiff(oldSvc, tc.svc))
}
if !reflect.DeepEqual(newSvc, originalNewSvc) {
t.Errorf("new object changed: %v", diff.ObjectReflectDiff(newSvc, originalNewSvc))
}
if len(errs) != len(tc.expectErr) {
t.Fatalf("unexpected number of errors: %v", errs)
}
for i := range errs {
if !strings.Contains(errs[i].Error(), tc.expectErr[i]) {
t.Errorf("unexpected error %d: %v", i, errs[i])
}
}
})
}
}

25
pkg/apis/core/validation/validation.go
View File

@ -3951,7 +3951,6 @@ func ValidatePodTemplateUpdate(newPod, oldPod *core.PodTemplate) field.ErrorList
var supportedSessionAffinityType = sets.NewString(string(core.ServiceAffinityClientIP), string(core.ServiceAffinityNone))
var supportedServiceType = sets.NewString(string(core.ServiceTypeClusterIP), string(core.ServiceTypeNodePort),
string(core.ServiceTypeLoadBalancer), string(core.ServiceTypeExternalName))
var supportedServiceIPFamily = sets.NewString(string(core.IPv4Protocol), string(core.IPv6Protocol))
// ValidateService tests if required fields/annotations of a Service are valid.
func ValidateService(service *core.Service, allowAppProtocol bool) field.ErrorList {
@ -4152,21 +4151,6 @@ func ValidateService(service *core.Service, allowAppProtocol bool) field.ErrorLi
}
}
//if an ipfamily provided then it has to be one of the supported values
// note:
// - we don't validate service.Spec.IPFamily is supported by the cluster
// - we don't validate service.Spec.ClusterIP is within a range supported by the cluster
// both of these validations are done by the ipallocator
// if the gate is on this field is required (and defaulted by REST if not provided by user)
if utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) && service.Spec.IPFamily == nil {
allErrs = append(allErrs, field.Required(specPath.Child("ipFamily"), ""))
}
if service.Spec.IPFamily != nil && !supportedServiceIPFamily.Has(string(*service.Spec.IPFamily)) {
allErrs = append(allErrs, field.NotSupported(specPath.Child("ipFamily"), service.Spec.IPFamily, supportedServiceIPFamily.List()))
}
allErrs = append(allErrs, validateServiceExternalTrafficFieldsValue(service)...)
return allErrs
}
@ -4275,19 +4259,12 @@ func ValidateServiceCreate(service *core.Service) field.ErrorList {
func ValidateServiceUpdate(service, oldService *core.Service) field.ErrorList {
allErrs := ValidateObjectMetaUpdate(&service.ObjectMeta, &oldService.ObjectMeta, field.NewPath("metadata"))
// ClusterIP and IPFamily should be immutable for services using it (every type other than ExternalName)
// ClusterIP should be immutable for services using it (every type other than ExternalName)
// which do not have ClusterIP assigned yet (empty string value)
if service.Spec.Type != core.ServiceTypeExternalName {
if oldService.Spec.Type != core.ServiceTypeExternalName && oldService.Spec.ClusterIP != "" {
allErrs = append(allErrs, ValidateImmutableField(service.Spec.ClusterIP, oldService.Spec.ClusterIP, field.NewPath("spec", "clusterIP"))...)
}
// notes:
// we drop the IPFamily field when the Dualstack gate is off.
// once the gate is on, we start assigning default ipfamily according to cluster settings. in other words
// though the field is immutable, we allow (onetime) change from nil==> to value
if oldService.Spec.IPFamily != nil {
allErrs = append(allErrs, ValidateImmutableField(service.Spec.IPFamily, oldService.Spec.IPFamily, field.NewPath("spec", "ipFamily"))...)
}
}
// allow AppProtocol value if the feature gate is set or the field is

12
pkg/apis/core/validation/validation_test.go
View File

@ -10193,12 +10193,12 @@ func TestValidateServiceCreate(t *testing.T) {
numErrs: 0,
},
{
name: "invalid, service with invalid IPFamily",
name: "allowed valid, service with invalid IPFamily is ignored (tested in conditional validation)",
tweakSvc: func(s *core.Service) {
invalidServiceIPFamily := core.IPFamily("not-a-valid-ip-family")
s.Spec.IPFamily = &invalidServiceIPFamily
},
numErrs: 1,
numErrs: 0,
},
{
name: "valid topology keys",
@ -12204,18 +12204,18 @@ func TestValidateServiceUpdate(t *testing.T) {
numErrs: 0,
},
{
name: "remove ipfamily",
name: "remove ipfamily (covered by conditional validation)",
tweakSvc: func(oldSvc, newSvc *core.Service) {
ipv6Service := core.IPv6Protocol
oldSvc.Spec.IPFamily = &ipv6Service
newSvc.Spec.IPFamily = nil
},
numErrs: 1,
numErrs: 0,
},
{
name: "change ServiceIPFamily",
name: "change ServiceIPFamily (covered by conditional validation)",
tweakSvc: func(oldSvc, newSvc *core.Service) {
ipv4Service := core.IPv4Protocol
oldSvc.Spec.Type = core.ServiceTypeClusterIP
@ -12225,7 +12225,7 @@ func TestValidateServiceUpdate(t *testing.T) {
newSvc.Spec.Type = core.ServiceTypeClusterIP
newSvc.Spec.IPFamily = &ipv6Service
},
numErrs: 1,
numErrs: 0,
},
{
name: "update with valid app protocol, field unset, gate disabled",

10
pkg/registry/core/rest/storage_core.go
View File

@ -190,11 +190,6 @@ func (c LegacyRESTStorageProvider) NewLegacyRESTStorage(restOptionsGetter generi
return LegacyRESTStorage{}, genericapiserver.APIGroupInfo{}, err
}
serviceRESTStorage, serviceStatusStorage, err := servicestore.NewGenericREST(restOptionsGetter)
if err != nil {
return LegacyRESTStorage{}, genericapiserver.APIGroupInfo{}, err
}
var serviceClusterIPRegistry rangeallocation.RangeRegistry
serviceClusterIPRange := c.ServiceIPRange
if serviceClusterIPRange.IP == nil {
@ -262,6 +257,11 @@ func (c LegacyRESTStorageProvider) NewLegacyRESTStorage(restOptionsGetter generi
return LegacyRESTStorage{}, genericapiserver.APIGroupInfo{}, err
}
serviceRESTStorage, serviceStatusStorage, err := servicestore.NewGenericREST(restOptionsGetter, serviceClusterIPRange, secondaryServiceClusterIPAllocator != nil)
if err != nil {
return LegacyRESTStorage{}, genericapiserver.APIGroupInfo{}, err
}
serviceRest, serviceRestProxy := servicestore.NewREST(serviceRESTStorage,
endpointsStorage,
podStorage.Pod,

1
pkg/registry/core/service/BUILD
View File

@ -27,6 +27,7 @@ go_library(
"//staging/src/k8s.io/apiserver/pkg/registry/rest:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/storage/names:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
"//vendor/k8s.io/utils/net:go_default_library",
],
)

1
pkg/registry/core/service/storage/BUILD
View File

@ -29,6 +29,7 @@ go_test(
"//staging/src/k8s.io/apimachinery/pkg/fields:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/labels:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/intstr:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/net:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/rand:go_default_library",

76
pkg/registry/core/service/storage/rest.go
View File

@ -53,14 +53,14 @@ import (
// REST adapts a service registry into apiserver's RESTStorage model.
type REST struct {
services ServiceStorage
endpoints EndpointsStorage
serviceIPs ipallocator.Interface
secondaryServiceIPs ipallocator.Interface
defaultServiceIPFamily api.IPFamily
serviceNodePorts portallocator.Interface
proxyTransport http.RoundTripper
pods rest.Getter
strategy rest.RESTCreateUpdateStrategy
services ServiceStorage
endpoints EndpointsStorage
serviceIPs ipallocator.Interface
secondaryServiceIPs ipallocator.Interface
serviceNodePorts portallocator.Interface
proxyTransport http.RoundTripper
pods rest.Getter
}
// ServiceNodePort includes protocol and port number of a service NodePort.
@ -102,26 +102,20 @@ func NewREST(
serviceNodePorts portallocator.Interface,
proxyTransport http.RoundTripper,
) (*REST, *registry.ProxyREST) {
// detect this cluster default Service IPFamily (ipfamily of --service-cluster-ip-range)
// we do it once here, to avoid having to do it over and over during ipfamily assignment
serviceIPFamily := api.IPv4Protocol
cidr := serviceIPs.CIDR()
if netutil.IsIPv6CIDR(&cidr) {
serviceIPFamily = api.IPv6Protocol
}
klog.V(0).Infof("the default service ipfamily for this cluster is: %s", string(serviceIPFamily))
strategy, _ := registry.StrategyForServiceCIDRs(serviceIPs.CIDR(), secondaryServiceIPs != nil)
rest := &REST{
services: services,
endpoints: endpoints,
serviceIPs: serviceIPs,
secondaryServiceIPs: secondaryServiceIPs,
serviceNodePorts: serviceNodePorts,
defaultServiceIPFamily: serviceIPFamily,
proxyTransport: proxyTransport,
pods: pods,
strategy: strategy,
services: services,
endpoints: endpoints,
serviceIPs: serviceIPs,
secondaryServiceIPs: secondaryServiceIPs,
serviceNodePorts: serviceNodePorts,
proxyTransport: proxyTransport,
pods: pods,
}
return rest, &registry.ProxyREST{Redirector: rest, ProxyTransport: proxyTransport}
}
@ -177,12 +171,7 @@ func (rs *REST) Export(ctx context.Context, name string, opts metav1.ExportOptio
func (rs *REST) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) {
service := obj.(*api.Service)
// set the service ip family, if it was not already set
if utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) && service.Spec.IPFamily == nil {
service.Spec.IPFamily = &rs.defaultServiceIPFamily
}
if err := rest.BeforeCreate(registry.Strategy, ctx, obj); err != nil {
if err := rest.BeforeCreate(rs.strategy, ctx, obj); err != nil {
return nil, err
}
@ -228,7 +217,7 @@ func (rs *REST) Create(ctx context.Context, obj runtime.Object, createValidation
out, err := rs.services.Create(ctx, service, createValidation, options)
if err != nil {
err = rest.CheckGeneratedNameError(registry.Strategy, err, service)
err = rest.CheckGeneratedNameError(rs.strategy, err, service)
}
if err == nil {
@ -396,7 +385,7 @@ func (rs *REST) Update(ctx context.Context, name string, objInfo rest.UpdatedObj
}
// Copy over non-user fields
if err := rest.BeforeUpdate(registry.Strategy, ctx, service, oldService); err != nil {
if err := rest.BeforeUpdate(rs.strategy, ctx, service, oldService); err != nil {
return nil, false, err
}
@ -666,6 +655,8 @@ func allocateHealthCheckNodePort(service *api.Service, nodePortOp *portallocator
// The return bool value indicates if a cluster IP is allocated successfully.
func initClusterIP(service *api.Service, allocator ipallocator.Interface) (bool, error) {
var allocatedIP net.IP
switch {
case service.Spec.ClusterIP == "":
// Allocate next available.
@ -676,19 +667,32 @@ func initClusterIP(service *api.Service, allocator ipallocator.Interface) (bool,
// not really an internal error.
return false, errors.NewInternalError(fmt.Errorf("failed to allocate a serviceIP: %v", err))
}
allocatedIP = ip
service.Spec.ClusterIP = ip.String()
return true, nil
case service.Spec.ClusterIP != api.ClusterIPNone && service.Spec.ClusterIP != "":
// Try to respect the requested IP.
if err := allocator.Allocate(net.ParseIP(service.Spec.ClusterIP)); err != nil {
ip := net.ParseIP(service.Spec.ClusterIP)
if err := allocator.Allocate(ip); err != nil {
// TODO: when validation becomes versioned, this gets more complicated.
el := field.ErrorList{field.Invalid(field.NewPath("spec", "clusterIP"), service.Spec.ClusterIP, err.Error())}
return false, errors.NewInvalid(api.Kind("Service"), service.Name, el)
}
return true, nil
allocatedIP = ip
}
return false, nil
// assuming the object was valid prior to setting, always force the IPFamily
// to match the allocated IP at this point
if allocatedIP != nil {
if utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) {
ipFamily := api.IPv4Protocol
if netutil.IsIPv6(allocatedIP) {
ipFamily = api.IPv6Protocol
}
service.Spec.IPFamily = &ipFamily
}
}
return allocatedIP != nil, nil
}
func initNodePorts(service *api.Service, nodePortOp *portallocator.PortAllocationOperation) error {

848
pkg/registry/core/service/storage/rest_test.go
View File

File diff suppressed because it is too large Load Diff

16
pkg/registry/core/service/storage/storage.go
View File

@ -18,6 +18,7 @@ package storage
import (
"context"
"net"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
@ -29,6 +30,7 @@ import (
printersinternal "k8s.io/kubernetes/pkg/printers/internalversion"
printerstorage "k8s.io/kubernetes/pkg/printers/storage"
"k8s.io/kubernetes/pkg/registry/core/service"
registry "k8s.io/kubernetes/pkg/registry/core/service"
)
type GenericREST struct {
@ -36,17 +38,19 @@ type GenericREST struct {
}
// NewREST returns a RESTStorage object that will work against services.
func NewGenericREST(optsGetter generic.RESTOptionsGetter) (*GenericREST, *StatusREST, error) {
func NewGenericREST(optsGetter generic.RESTOptionsGetter, serviceCIDR net.IPNet, hasSecondary bool) (*GenericREST, *StatusREST, error) {
strategy, _ := registry.StrategyForServiceCIDRs(serviceCIDR, hasSecondary)
store := &genericregistry.Store{
NewFunc: func() runtime.Object { return &api.Service{} },
NewListFunc: func() runtime.Object { return &api.ServiceList{} },
DefaultQualifiedResource: api.Resource("services"),
ReturnDeletedObject: true,
CreateStrategy: service.Strategy,
UpdateStrategy: service.Strategy,
DeleteStrategy: service.Strategy,
ExportStrategy: service.Strategy,
CreateStrategy: strategy,
UpdateStrategy: strategy,
DeleteStrategy: strategy,
ExportStrategy: strategy,
TableConvertor: printerstorage.TableConvertor{TableGenerator: printers.NewTableGenerator().With(printersinternal.AddHandlers)},
}
@ -56,7 +60,7 @@ func NewGenericREST(optsGetter generic.RESTOptionsGetter) (*GenericREST, *Status
}
statusStore := *store
statusStore.UpdateStrategy = service.StatusStrategy
statusStore.UpdateStrategy = service.NewServiceStatusStrategy(strategy)
return &GenericREST{store}, &StatusREST{store: &statusStore}, nil
}

2
pkg/registry/core/service/storage/storage_test.go
View File

@ -39,7 +39,7 @@ func newStorage(t *testing.T) (*GenericREST, *StatusREST, *etcd3testing.EtcdTest
DeleteCollectionWorkers: 1,
ResourcePrefix: "services",
}
serviceStorage, statusStorage, err := NewGenericREST(restOptions)
serviceStorage, statusStorage, err := NewGenericREST(restOptions, *makeIPNet(t), false)
if err != nil {
t.Fatalf("unexpected error from REST storage: %v", err)
}

90
pkg/registry/core/service/strategy.go
View File

@ -19,54 +19,113 @@ package service
import (
"context"
"fmt"
"net"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/validation/field"
"k8s.io/apiserver/pkg/registry/rest"
"k8s.io/apiserver/pkg/storage/names"
utilfeature "k8s.io/apiserver/pkg/util/feature"
"k8s.io/kubernetes/pkg/api/legacyscheme"
api "k8s.io/kubernetes/pkg/apis/core"
"k8s.io/kubernetes/pkg/apis/core/validation"
"k8s.io/kubernetes/pkg/features"
netutil "k8s.io/utils/net"
)
type Strategy interface {
rest.RESTCreateUpdateStrategy
rest.RESTExportStrategy
}
// svcStrategy implements behavior for Services
type svcStrategy struct {
runtime.ObjectTyper
names.NameGenerator
ipFamilies []api.IPFamily
}
// Services is the default logic that applies when creating and updating Service
// objects.
var Strategy = svcStrategy{legacyscheme.Scheme, names.SimpleNameGenerator}
// StrategyForServiceCIDRs returns the appropriate service strategy for the given configuration.
func StrategyForServiceCIDRs(primaryCIDR net.IPNet, hasSecondary bool) (Strategy, api.IPFamily) {
// detect this cluster default Service IPFamily (ipfamily of --service-cluster-ip-range)
// we do it once here, to avoid having to do it over and over during ipfamily assignment
serviceIPFamily := api.IPv4Protocol
if netutil.IsIPv6CIDR(&primaryCIDR) {
serviceIPFamily = api.IPv6Protocol
}
var strategy Strategy
switch {
case hasSecondary && serviceIPFamily == api.IPv4Protocol:
strategy = svcStrategy{
ObjectTyper: legacyscheme.Scheme,
NameGenerator: names.SimpleNameGenerator,
ipFamilies: []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},
}
case hasSecondary && serviceIPFamily == api.IPv6Protocol:
strategy = svcStrategy{
ObjectTyper: legacyscheme.Scheme,
NameGenerator: names.SimpleNameGenerator,
ipFamilies: []api.IPFamily{api.IPv6Protocol, api.IPv4Protocol},
}
case serviceIPFamily == api.IPv6Protocol:
strategy = svcStrategy{
ObjectTyper: legacyscheme.Scheme,
NameGenerator: names.SimpleNameGenerator,
ipFamilies: []api.IPFamily{api.IPv6Protocol},
}
default:
strategy = svcStrategy{
ObjectTyper: legacyscheme.Scheme,
NameGenerator: names.SimpleNameGenerator,
ipFamilies: []api.IPFamily{api.IPv4Protocol},
}
}
return strategy, serviceIPFamily
}
// NamespaceScoped is true for services.
func (svcStrategy) NamespaceScoped() bool {
return true
}
// PrepareForCreate clears fields that are not allowed to be set by end users on creation.
func (svcStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) {
// PrepareForCreate sets contextual defaults and clears fields that are not allowed to be set by end users on creation.
func (strategy svcStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) {
service := obj.(*api.Service)
service.Status = api.ServiceStatus{}
if utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) && service.Spec.IPFamily == nil {
family := strategy.ipFamilies[0]
service.Spec.IPFamily = &family
}
dropServiceDisabledFields(service, nil)
}
// PrepareForUpdate clears fields that are not allowed to be set by end users on update.
func (svcStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) {
// PrepareForUpdate sets contextual defaults and clears fields that are not allowed to be set by end users on update.
func (strategy svcStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) {
newService := obj.(*api.Service)
oldService := old.(*api.Service)
newService.Status = oldService.Status
if utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) && newService.Spec.IPFamily == nil {
if oldService.Spec.IPFamily != nil {
newService.Spec.IPFamily = oldService.Spec.IPFamily
} else {
family := strategy.ipFamilies[0]
newService.Spec.IPFamily = &family
}
}
dropServiceDisabledFields(newService, oldService)
}
// Validate validates a new service.
func (svcStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList {
func (strategy svcStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList {
service := obj.(*api.Service)
allErrs := validation.ValidateServiceCreate(service)
allErrs = append(allErrs, validation.ValidateConditionalService(service, nil)...)
allErrs = append(allErrs, validation.ValidateConditionalService(service, nil, strategy.ipFamilies)...)
return allErrs
}
@ -78,9 +137,9 @@ func (svcStrategy) AllowCreateOnUpdate() bool {
return true
}
func (svcStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList {
func (strategy svcStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList {
allErrs := validation.ValidateServiceUpdate(obj.(*api.Service), old.(*api.Service))
allErrs = append(allErrs, validation.ValidateConditionalService(obj.(*api.Service), old.(*api.Service))...)
allErrs = append(allErrs, validation.ValidateConditionalService(obj.(*api.Service), old.(*api.Service), strategy.ipFamilies)...)
return allErrs
}
@ -120,6 +179,7 @@ func dropServiceDisabledFields(newSvc *api.Service, oldSvc *api.Service) {
if !utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) && !serviceIPFamilyInUse(oldSvc) {
newSvc.Spec.IPFamily = nil
}
// Drop TopologyKeys if ServiceTopology is not enabled
if !utilfeature.DefaultFeatureGate.Enabled(features.ServiceTopology) && !topologyKeysInUse(oldSvc) {
newSvc.Spec.TopologyKeys = nil
@ -146,11 +206,13 @@ func topologyKeysInUse(svc *api.Service) bool {
}
type serviceStatusStrategy struct {
svcStrategy
Strategy
}
// StatusStrategy is the default logic invoked when updating service status.
var StatusStrategy = serviceStatusStrategy{Strategy}
// NewServiceStatusStrategy creates a status strategy for the provided base strategy.
func NewServiceStatusStrategy(strategy Strategy) Strategy {
return serviceStatusStrategy{strategy}
}
// PrepareForUpdate clears fields that are not allowed to be set by end users on update of status
func (serviceStatusStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) {

221
pkg/registry/core/service/strategy_test.go
View File

@ -17,6 +17,7 @@ limitations under the License.
package service
import (
"net"
"reflect"
"testing"
@ -35,7 +36,18 @@ import (
"k8s.io/kubernetes/pkg/features"
)
func newStrategy(cidr string, hasSecondary bool) (testStrategy Strategy, testStatusStrategy Strategy) {
_, testCIDR, err := net.ParseCIDR(cidr)
if err != nil {
panic("invalid CIDR")
}
testStrategy, _ = StrategyForServiceCIDRs(*testCIDR, hasSecondary)
testStatusStrategy = NewServiceStatusStrategy(testStrategy)
return
}
func TestExportService(t *testing.T) {
testStrategy, _ := newStrategy("10.0.0.0/16", false)
tests := []struct {
objIn runtime.Object
objOut runtime.Object
@ -98,7 +110,7 @@ func TestExportService(t *testing.T) {
}
for _, test := range tests {
err := Strategy.Export(genericapirequest.NewContext(), test.objIn, test.exact)
err := testStrategy.Export(genericapirequest.NewContext(), test.objIn, test.exact)
if err != nil {
if !test.expectErr {
t.Errorf("unexpected error: %v", err)
@ -116,18 +128,19 @@ func TestExportService(t *testing.T) {
}
func TestCheckGeneratedNameError(t *testing.T) {
testStrategy, _ := newStrategy("10.0.0.0/16", false)
expect := errors.NewNotFound(api.Resource("foos"), "bar")
if err := rest.CheckGeneratedNameError(Strategy, expect, &api.Service{}); err != expect {
if err := rest.CheckGeneratedNameError(testStrategy, expect, &api.Service{}); err != expect {
t.Errorf("NotFoundError should be ignored: %v", err)
}
expect = errors.NewAlreadyExists(api.Resource("foos"), "bar")
if err := rest.CheckGeneratedNameError(Strategy, expect, &api.Service{}); err != expect {
if err := rest.CheckGeneratedNameError(testStrategy, expect, &api.Service{}); err != expect {
t.Errorf("AlreadyExists should be returned when no GenerateName field: %v", err)
}
expect = errors.NewAlreadyExists(api.Resource("foos"), "bar")
if err := rest.CheckGeneratedNameError(Strategy, expect, &api.Service{ObjectMeta: metav1.ObjectMeta{GenerateName: "foo"}}); err == nil || !errors.IsServerTimeout(err) {
if err := rest.CheckGeneratedNameError(testStrategy, expect, &api.Service{ObjectMeta: metav1.ObjectMeta{GenerateName: "foo"}}); err == nil || !errors.IsServerTimeout(err) {
t.Errorf("expected try again later error: %v", err)
}
}
@ -153,11 +166,131 @@ func makeValidService() api.Service {
}
// TODO: This should be done on types that are not part of our API
func TestBeforeCreate(t *testing.T) {
withIP := func(family *api.IPFamily, ip string) *api.Service {
svc := makeValidService()
svc.Spec.IPFamily = family
svc.Spec.ClusterIP = ip
return &svc
}
ipv4 := api.IPv4Protocol
ipv6 := api.IPv6Protocol
testCases := []struct {
name string
cidr string
configureDualStack bool
enableDualStack bool
in *api.Service
expect *api.Service
expectErr bool
}{
{
name: "does not set ipfamily when dual stack gate is disabled",
cidr: "10.0.0.0/16",
in: withIP(nil, ""),
expect: withIP(nil, ""),
},
{
name: "clears ipfamily when dual stack gate is disabled",
cidr: "10.0.0.0/16",
in: withIP(&ipv4, ""),
expect: withIP(nil, ""),
},
{
name: "allows ipfamily to configured ipv4 value",
cidr: "10.0.0.0/16",
enableDualStack: true,
in: withIP(nil, ""),
expect: withIP(&ipv4, ""),
},
{
name: "allows ipfamily to configured ipv4 value when dual stack is in use",
cidr: "10.0.0.0/16",
enableDualStack: true,
configureDualStack: true,
in: withIP(nil, ""),
expect: withIP(&ipv4, ""),
},
{
name: "allows ipfamily to configured ipv6 value",
cidr: "fd00::/64",
enableDualStack: true,
in: withIP(nil, ""),
expect: withIP(&ipv6, ""),
},
{
name: "allows ipfamily to configured ipv6 value when dual stack is in use",
cidr: "fd00::/64",
enableDualStack: true,
configureDualStack: true,
in: withIP(nil, ""),
expect: withIP(&ipv6, ""),
},
{
name: "rejects ipv6 ipfamily when single-stack ipv4",
enableDualStack: true,
cidr: "10.0.0.0/16",
in: withIP(&ipv6, ""),
expectErr: true,
},
{
name: "rejects ipv4 ipfamily when single-stack ipv6",
enableDualStack: true,
cidr: "fd00::/64",
in: withIP(&ipv4, ""),
expectErr: true,
},
{
name: "rejects implicit ipv4 ipfamily when single-stack ipv6",
enableDualStack: true,
cidr: "fd00::/64",
in: withIP(nil, "10.0.1.0"),
expectErr: true,
},
{
name: "rejects implicit ipv6 ipfamily when single-stack ipv4",
enableDualStack: true,
cidr: "10.0.0.0/16",
in: withIP(nil, "fd00::1"),
expectErr: true,
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, tc.enableDualStack)()
testStrategy, _ := newStrategy(tc.cidr, tc.configureDualStack)
ctx := genericapirequest.NewDefaultContext()
err := rest.BeforeCreate(testStrategy, ctx, runtime.Object(tc.in))
if tc.expectErr != (err != nil) {
t.Fatalf("unexpected error: %v", err)
}
if err != nil {
return
}
if tc.expect != nil && tc.in != nil {
tc.expect.ObjectMeta = tc.in.ObjectMeta
}
if !reflect.DeepEqual(tc.expect, tc.in) {
t.Fatalf("unexpected change: %s", diff.ObjectReflectDiff(tc.expect, tc.in))
}
})
}
}
func TestBeforeUpdate(t *testing.T) {
testCases := []struct {
name string
tweakSvc func(oldSvc, newSvc *api.Service) // given basic valid services, each test case can customize them
expectErr bool
name string
enableDualStack bool
defaultIPv6 bool
allowSecondary bool
tweakSvc func(oldSvc, newSvc *api.Service) // given basic valid services, each test case can customize them
expectErr bool
expectObj func(t *testing.T, svc *api.Service)
}{
{
name: "no change",
@ -195,6 +328,19 @@ func TestBeforeUpdate(t *testing.T) {
},
expectErr: true,
},
{
name: "clear IP family is allowed (defaulted back by before update)",
enableDualStack: true,
tweakSvc: func(oldSvc, newSvc *api.Service) {
oldSvc.Spec.IPFamily = nil
},
expectErr: false,
expectObj: func(t *testing.T, svc *api.Service) {
if svc.Spec.IPFamily == nil {
t.Errorf("ipfamily was not defaulted")
}
},
},
{
name: "change selector",
tweakSvc: func(oldSvc, newSvc *api.Service) {
@ -205,23 +351,36 @@ func TestBeforeUpdate(t *testing.T) {
}
for _, tc := range testCases {
oldSvc := makeValidService()
newSvc := makeValidService()
tc.tweakSvc(&oldSvc, &newSvc)
ctx := genericapirequest.NewDefaultContext()
err := rest.BeforeUpdate(Strategy, ctx, runtime.Object(&oldSvc), runtime.Object(&newSvc))
if tc.expectErr && err == nil {
t.Errorf("unexpected non-error for %q", tc.name)
}
if !tc.expectErr && err != nil {
t.Errorf("unexpected error for %q: %v", tc.name, err)
}
t.Run(tc.name, func(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, tc.enableDualStack)()
var cidr string
if tc.defaultIPv6 {
cidr = "ffd0::/64"
} else {
cidr = "172.30.0.0/16"
}
strategy, _ := newStrategy(cidr, tc.allowSecondary)
oldSvc := makeValidService()
newSvc := makeValidService()
tc.tweakSvc(&oldSvc, &newSvc)
ctx := genericapirequest.NewDefaultContext()
err := rest.BeforeUpdate(strategy, ctx, runtime.Object(&newSvc), runtime.Object(&oldSvc))
if tc.expectObj != nil {
tc.expectObj(t, &newSvc)
}
if tc.expectErr && err == nil {
t.Fatalf("unexpected non-error: %v", err)
}
if !tc.expectErr && err != nil {
t.Fatalf("unexpected error: %v", err)
}
})
}
}
func TestServiceStatusStrategy(t *testing.T) {
_, testStatusStrategy := newStrategy("10.0.0.0/16", false)
ctx := genericapirequest.NewDefaultContext()
if !StatusStrategy.NamespaceScoped() {
if !testStatusStrategy.NamespaceScoped() {
t.Errorf("Service must be namespace scoped")
}
oldService := makeValidService()
@ -236,23 +395,23 @@ func TestServiceStatusStrategy(t *testing.T) {
},
},
}
StatusStrategy.PrepareForUpdate(ctx, &newService, &oldService)
testStatusStrategy.PrepareForUpdate(ctx, &newService, &oldService)
if newService.Status.LoadBalancer.Ingress[0].IP != "127.0.0.2" {
t.Errorf("Service status updates should allow change of status fields")
}
if newService.Spec.SessionAffinity != "None" {
t.Errorf("PrepareForUpdate should have preserved old spec")
}
errs := StatusStrategy.ValidateUpdate(ctx, &newService, &oldService)
errs := testStatusStrategy.ValidateUpdate(ctx, &newService, &oldService)
if len(errs) != 0 {
t.Errorf("Unexpected error %v", errs)
}
}
func makeServiceWithIPFamily(IPFamily *api.IPFamily) *api.Service {
func makeServiceWithIPFamily(ipFamily *api.IPFamily) *api.Service {
return &api.Service{
Spec: api.ServiceSpec{
IPFamily: IPFamily,
IPFamily: ipFamily,
},
}
}
@ -294,6 +453,20 @@ func TestDropDisabledField(t *testing.T) {
oldSvc: nil,
compareSvc: makeServiceWithIPFamily(&ipv6Service),
},
{
name: "dualstack, field used, changed",
enableDualStack: true,
svc: makeServiceWithIPFamily(&ipv6Service),
oldSvc: makeServiceWithIPFamily(&ipv4Service),
compareSvc: makeServiceWithIPFamily(&ipv6Service),
},
{
name: "dualstack, field used, not changed",
enableDualStack: true,
svc: makeServiceWithIPFamily(&ipv6Service),
oldSvc: makeServiceWithIPFamily(&ipv6Service),
compareSvc: makeServiceWithIPFamily(&ipv6Service),
},
/* add more tests for other dropped fields as needed */
}