From 7df1afe71f26b4ffee645e0c82a548f866bd554b Mon Sep 17 00:00:00 2001 From: Maciej Kwiek Date: Thu, 13 Oct 2016 10:11:26 +0200 Subject: [PATCH 1/2] Deny ClusterIP update for services using it If the service is of the type using the ClusterIP (ClusterIP, NodePort, LoadBalancer), the update of ClusterIP is prohibited. --- pkg/api/validation/validation.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index aa9ef469ae7..cc9689eee8f 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -2635,8 +2635,12 @@ func validateServiceAnnotations(service *api.Service, oldService *api.Service) ( func ValidateServiceUpdate(service, oldService *api.Service) field.ErrorList { allErrs := ValidateObjectMetaUpdate(&service.ObjectMeta, &oldService.ObjectMeta, field.NewPath("metadata")) - if api.IsServiceIPSet(oldService) { - allErrs = append(allErrs, ValidateImmutableField(service.Spec.ClusterIP, oldService.Spec.ClusterIP, field.NewPath("spec", "clusterIP"))...) + // 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 != api.ServiceTypeExternalName { + if oldService.Spec.Type != api.ServiceTypeExternalName && oldService.Spec.ClusterIP != "" { + allErrs = append(allErrs, ValidateImmutableField(service.Spec.ClusterIP, oldService.Spec.ClusterIP, field.NewPath("spec", "clusterIP"))...) + } } // TODO(freehan): allow user to update loadbalancerSourceRanges From d1c32b81948ba6224fc6d5726e5287631e644d95 Mon Sep 17 00:00:00 2001 From: Maciej Kwiek Date: Thu, 13 Oct 2016 10:17:44 +0200 Subject: [PATCH 2/2] Test cases for service ClusterIP updates Test cases from ClusterIP using types to other ClusterIP using types (ClusterIP, NodePort, LoadBalancer) added. --- pkg/api/validation/validation_test.go | 236 ++++++++++++++++++++ pkg/registry/core/service/etcd/etcd_test.go | 1 + 2 files changed, 237 insertions(+) diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index 664290953c3..425ee031780 100644 --- a/pkg/api/validation/validation_test.go +++ b/pkg/api/validation/validation_test.go @@ -6519,6 +6519,242 @@ func TestValidateServiceUpdate(t *testing.T) { }, numErrs: 0, }, + { + name: "`None` ClusterIP cannot be changed", + tweakSvc: func(oldSvc, newSvc *api.Service) { + oldSvc.Spec.ClusterIP = "None" + newSvc.Spec.ClusterIP = "1.2.3.4" + }, + numErrs: 1, + }, + { + name: "`None` ClusterIP cannot be removed", + tweakSvc: func(oldSvc, newSvc *api.Service) { + oldSvc.Spec.ClusterIP = "None" + newSvc.Spec.ClusterIP = "" + }, + numErrs: 1, + }, + { + name: "Service with ClusterIP type cannot change its set ClusterIP", + tweakSvc: func(oldSvc, newSvc *api.Service) { + oldSvc.Spec.Type = api.ServiceTypeClusterIP + newSvc.Spec.Type = api.ServiceTypeClusterIP + + oldSvc.Spec.ClusterIP = "1.2.3.4" + newSvc.Spec.ClusterIP = "1.2.3.5" + }, + numErrs: 1, + }, + { + name: "Service with ClusterIP type can change its empty ClusterIP", + tweakSvc: func(oldSvc, newSvc *api.Service) { + oldSvc.Spec.Type = api.ServiceTypeClusterIP + newSvc.Spec.Type = api.ServiceTypeClusterIP + + oldSvc.Spec.ClusterIP = "" + newSvc.Spec.ClusterIP = "1.2.3.5" + }, + numErrs: 0, + }, + { + name: "Service with ClusterIP type cannot change its set ClusterIP when changing type to NodePort", + tweakSvc: func(oldSvc, newSvc *api.Service) { + oldSvc.Spec.Type = api.ServiceTypeClusterIP + newSvc.Spec.Type = api.ServiceTypeNodePort + + oldSvc.Spec.ClusterIP = "1.2.3.4" + newSvc.Spec.ClusterIP = "1.2.3.5" + }, + numErrs: 1, + }, + { + name: "Service with ClusterIP type can change its empty ClusterIP when changing type to NodePort", + tweakSvc: func(oldSvc, newSvc *api.Service) { + oldSvc.Spec.Type = api.ServiceTypeClusterIP + newSvc.Spec.Type = api.ServiceTypeNodePort + + oldSvc.Spec.ClusterIP = "" + newSvc.Spec.ClusterIP = "1.2.3.5" + }, + numErrs: 0, + }, + { + name: "Service with ClusterIP type cannot change its ClusterIP when changing type to LoadBalancer", + tweakSvc: func(oldSvc, newSvc *api.Service) { + oldSvc.Spec.Type = api.ServiceTypeClusterIP + newSvc.Spec.Type = api.ServiceTypeLoadBalancer + + oldSvc.Spec.ClusterIP = "1.2.3.4" + newSvc.Spec.ClusterIP = "1.2.3.5" + }, + numErrs: 1, + }, + { + name: "Service with ClusterIP type can change its empty ClusterIP when changing type to LoadBalancer", + tweakSvc: func(oldSvc, newSvc *api.Service) { + oldSvc.Spec.Type = api.ServiceTypeClusterIP + newSvc.Spec.Type = api.ServiceTypeLoadBalancer + + oldSvc.Spec.ClusterIP = "" + newSvc.Spec.ClusterIP = "1.2.3.5" + }, + numErrs: 0, + }, + { + name: "Service with NodePort type cannot change its set ClusterIP", + tweakSvc: func(oldSvc, newSvc *api.Service) { + oldSvc.Spec.Type = api.ServiceTypeNodePort + newSvc.Spec.Type = api.ServiceTypeNodePort + + oldSvc.Spec.ClusterIP = "1.2.3.4" + newSvc.Spec.ClusterIP = "1.2.3.5" + }, + numErrs: 1, + }, + { + name: "Service with NodePort type can change its empty ClusterIP", + tweakSvc: func(oldSvc, newSvc *api.Service) { + oldSvc.Spec.Type = api.ServiceTypeNodePort + newSvc.Spec.Type = api.ServiceTypeNodePort + + oldSvc.Spec.ClusterIP = "" + newSvc.Spec.ClusterIP = "1.2.3.5" + }, + numErrs: 0, + }, + { + name: "Service with NodePort type cannot change its set ClusterIP when changing type to ClusterIP", + tweakSvc: func(oldSvc, newSvc *api.Service) { + oldSvc.Spec.Type = api.ServiceTypeNodePort + newSvc.Spec.Type = api.ServiceTypeClusterIP + + oldSvc.Spec.ClusterIP = "1.2.3.4" + newSvc.Spec.ClusterIP = "1.2.3.5" + }, + numErrs: 1, + }, + { + name: "Service with NodePort type can change its empty ClusterIP when changing type to ClusterIP", + tweakSvc: func(oldSvc, newSvc *api.Service) { + oldSvc.Spec.Type = api.ServiceTypeNodePort + newSvc.Spec.Type = api.ServiceTypeClusterIP + + oldSvc.Spec.ClusterIP = "" + newSvc.Spec.ClusterIP = "1.2.3.5" + }, + numErrs: 0, + }, + { + name: "Service with NodePort type cannot change its set ClusterIP when changing type to LoadBalancer", + tweakSvc: func(oldSvc, newSvc *api.Service) { + oldSvc.Spec.Type = api.ServiceTypeNodePort + newSvc.Spec.Type = api.ServiceTypeLoadBalancer + + oldSvc.Spec.ClusterIP = "1.2.3.4" + newSvc.Spec.ClusterIP = "1.2.3.5" + }, + numErrs: 1, + }, + { + name: "Service with NodePort type can change its empty ClusterIP when changing type to LoadBalancer", + tweakSvc: func(oldSvc, newSvc *api.Service) { + oldSvc.Spec.Type = api.ServiceTypeNodePort + newSvc.Spec.Type = api.ServiceTypeLoadBalancer + + oldSvc.Spec.ClusterIP = "" + newSvc.Spec.ClusterIP = "1.2.3.5" + }, + numErrs: 0, + }, + { + name: "Service with LoadBalancer type cannot change its set ClusterIP", + tweakSvc: func(oldSvc, newSvc *api.Service) { + oldSvc.Spec.Type = api.ServiceTypeLoadBalancer + newSvc.Spec.Type = api.ServiceTypeLoadBalancer + + oldSvc.Spec.ClusterIP = "1.2.3.4" + newSvc.Spec.ClusterIP = "1.2.3.5" + }, + numErrs: 1, + }, + { + name: "Service with LoadBalancer type can change its empty ClusterIP", + tweakSvc: func(oldSvc, newSvc *api.Service) { + oldSvc.Spec.Type = api.ServiceTypeLoadBalancer + newSvc.Spec.Type = api.ServiceTypeLoadBalancer + + oldSvc.Spec.ClusterIP = "" + newSvc.Spec.ClusterIP = "1.2.3.5" + }, + numErrs: 0, + }, + { + name: "Service with LoadBalancer type cannot change its set ClusterIP when changing type to ClusterIP", + tweakSvc: func(oldSvc, newSvc *api.Service) { + oldSvc.Spec.Type = api.ServiceTypeLoadBalancer + newSvc.Spec.Type = api.ServiceTypeClusterIP + + oldSvc.Spec.ClusterIP = "1.2.3.4" + newSvc.Spec.ClusterIP = "1.2.3.5" + }, + numErrs: 1, + }, + { + name: "Service with LoadBalancer type can change its empty ClusterIP when changing type to ClusterIP", + tweakSvc: func(oldSvc, newSvc *api.Service) { + oldSvc.Spec.Type = api.ServiceTypeLoadBalancer + newSvc.Spec.Type = api.ServiceTypeClusterIP + + oldSvc.Spec.ClusterIP = "" + newSvc.Spec.ClusterIP = "1.2.3.5" + }, + numErrs: 0, + }, + { + name: "Service with LoadBalancer type cannot change its set ClusterIP when changing type to NodePort", + tweakSvc: func(oldSvc, newSvc *api.Service) { + oldSvc.Spec.Type = api.ServiceTypeLoadBalancer + newSvc.Spec.Type = api.ServiceTypeNodePort + + oldSvc.Spec.ClusterIP = "1.2.3.4" + newSvc.Spec.ClusterIP = "1.2.3.5" + }, + numErrs: 1, + }, + { + name: "Service with LoadBalancer type can change its empty ClusterIP when changing type to NodePort", + tweakSvc: func(oldSvc, newSvc *api.Service) { + oldSvc.Spec.Type = api.ServiceTypeLoadBalancer + newSvc.Spec.Type = api.ServiceTypeNodePort + + oldSvc.Spec.ClusterIP = "" + newSvc.Spec.ClusterIP = "1.2.3.5" + }, + numErrs: 0, + }, + { + name: "Service with ExternalName type can change its empty ClusterIP when changing type to ClusterIP", + tweakSvc: func(oldSvc, newSvc *api.Service) { + oldSvc.Spec.Type = api.ServiceTypeExternalName + newSvc.Spec.Type = api.ServiceTypeClusterIP + + oldSvc.Spec.ClusterIP = "" + newSvc.Spec.ClusterIP = "1.2.3.5" + }, + numErrs: 0, + }, + { + name: "Service with ExternalName type can change its set ClusterIP when changing type to ClusterIP", + tweakSvc: func(oldSvc, newSvc *api.Service) { + oldSvc.Spec.Type = api.ServiceTypeExternalName + newSvc.Spec.Type = api.ServiceTypeClusterIP + + oldSvc.Spec.ClusterIP = "1.2.3.4" + newSvc.Spec.ClusterIP = "1.2.3.5" + }, + numErrs: 0, + }, } for _, tc := range testCases { diff --git a/pkg/registry/core/service/etcd/etcd_test.go b/pkg/registry/core/service/etcd/etcd_test.go index f9702dd3efb..003a34448d6 100644 --- a/pkg/registry/core/service/etcd/etcd_test.go +++ b/pkg/registry/core/service/etcd/etcd_test.go @@ -100,6 +100,7 @@ func TestUpdate(t *testing.T) { object := obj.(*api.Service) object.Spec = api.ServiceSpec{ Selector: map[string]string{"bar": "baz2"}, + ClusterIP: "None", SessionAffinity: api.ServiceAffinityNone, Type: api.ServiceTypeClusterIP, Ports: []api.ServicePort{{