diff --git a/pkg/registry/core/service/storage/alloc.go b/pkg/registry/core/service/storage/alloc.go index c2144f72023..e0c7bc81ed0 100644 --- a/pkg/registry/core/service/storage/alloc.go +++ b/pkg/registry/core/service/storage/alloc.go @@ -65,7 +65,7 @@ func (al *RESTAllocStuff) allocateCreate(service *api.Service, dryRun bool) (tra // Ensure IP family fields are correctly initialized. We do it here, since // we want this to be visible even when dryRun == true. - if err := al.initIPFamilyFields(nil, service); err != nil { + if err := al.initIPFamilyFields(After{service}, Before{nil}); err != nil { return nil, err } @@ -93,7 +93,9 @@ func (al *RESTAllocStuff) allocateCreate(service *api.Service, dryRun bool) (tra // attempts to default service ip families according to cluster configuration // while ensuring that provided families are configured on cluster. -func (al *RESTAllocStuff) initIPFamilyFields(oldService, service *api.Service) error { +func (al *RESTAllocStuff) initIPFamilyFields(after After, before Before) error { + oldService, service := before.Service, after.Service + // can not do anything here if service.Spec.Type == api.ServiceTypeExternalName { return nil @@ -113,7 +115,7 @@ func (al *RESTAllocStuff) initIPFamilyFields(oldService, service *api.Service) e // when: // - changing ipFamilyPolicy to "RequireDualStack" or "SingleStack" AND // - adding or removing a secondary clusterIP or ipFamily - if isMatchingPreferDualStackClusterIPFields(oldService, service) { + if isMatchingPreferDualStackClusterIPFields(after, before) { return nil // nothing more to do. } @@ -172,11 +174,11 @@ func (al *RESTAllocStuff) initIPFamilyFields(oldService, service *api.Service) e } else { // If the policy is anything but single-stack AND they reduced these // fields, it's an error. They need to specify policy. - if reducedClusterIPs(oldService, service) { + if reducedClusterIPs(After{service}, Before{oldService}) { el = append(el, field.Invalid(field.NewPath("spec", "ipFamilyPolicy"), service.Spec.IPFamilyPolicy, "must be 'SingleStack' to release the secondary cluster IP")) } - if reducedIPFamilies(oldService, service) { + if reducedIPFamilies(After{service}, Before{oldService}) { el = append(el, field.Invalid(field.NewPath("spec", "ipFamilyPolicy"), service.Spec.IPFamilyPolicy, "must be 'SingleStack' to release the secondary IP family")) } @@ -565,12 +567,12 @@ func (al *RESTAllocStuff) allocHealthCheckNodePort(service *api.Service, nodePor return nil } -func (al *RESTAllocStuff) allocateUpdate(service, oldService *api.Service, dryRun bool) (transaction, error) { +func (al *RESTAllocStuff) allocateUpdate(after After, before Before, dryRun bool) (transaction, error) { result := metaTransaction{} // Ensure IP family fields are correctly initialized. We do it here, since // we want this to be visible even when dryRun == true. - if err := al.initIPFamilyFields(oldService, service); err != nil { + if err := al.initIPFamilyFields(after, before); err != nil { return nil, err } @@ -578,7 +580,7 @@ func (al *RESTAllocStuff) allocateUpdate(service, oldService *api.Service, dryRu //TODO(thockin): validation should not pass with empty clusterIP, but it //does (and is tested!). Fixing that all is a big PR and will have to //happen later. - if txn, err := al.txnUpdateClusterIPs(service, oldService, dryRun); err != nil { + if txn, err := al.txnUpdateClusterIPs(after, before, dryRun); err != nil { result.Revert() return nil, err } else { @@ -586,7 +588,7 @@ func (al *RESTAllocStuff) allocateUpdate(service, oldService *api.Service, dryRu } // Allocate ports - if txn, err := al.txnUpdateNodePorts(service, oldService, dryRun); err != nil { + if txn, err := al.txnUpdateNodePorts(after, before, dryRun); err != nil { result.Revert() return nil, err } else { @@ -596,8 +598,10 @@ func (al *RESTAllocStuff) allocateUpdate(service, oldService *api.Service, dryRu return result, nil } -func (al *RESTAllocStuff) txnUpdateClusterIPs(service *api.Service, oldService *api.Service, dryRun bool) (transaction, error) { - allocated, released, err := al.updateClusterIPs(oldService, service, dryRun) +func (al *RESTAllocStuff) txnUpdateClusterIPs(after After, before Before, dryRun bool) (transaction, error) { + service := after.Service + + allocated, released, err := al.updateClusterIPs(after, before, dryRun) if err != nil { return nil, err } @@ -634,7 +638,8 @@ func (al *RESTAllocStuff) txnUpdateClusterIPs(service *api.Service, oldService * // this func does not perform actual release of clusterIPs. it returns // a map[family]ip for the caller to release when everything else has // executed successfully -func (al *RESTAllocStuff) updateClusterIPs(oldService *api.Service, service *api.Service, dryRun bool) (allocated map[api.IPFamily]string, toRelease map[api.IPFamily]string, err error) { +func (al *RESTAllocStuff) updateClusterIPs(after After, before Before, dryRun bool) (allocated map[api.IPFamily]string, toRelease map[api.IPFamily]string, err error) { + oldService, service := before.Service, after.Service // We don't want to auto-upgrade (add an IP) or downgrade (remove an IP) // PreferDualStack services following a cluster change to/from @@ -644,7 +649,7 @@ func (al *RESTAllocStuff) updateClusterIPs(oldService *api.Service, service *api // when: // - changing ipFamilyPolicy to "RequireDualStack" or "SingleStack" AND // - adding or removing a secondary clusterIP or ipFamily - if isMatchingPreferDualStackClusterIPFields(oldService, service) { + if isMatchingPreferDualStackClusterIPFields(after, before) { return allocated, toRelease, nil // nothing more to do. } @@ -722,7 +727,9 @@ func (al *RESTAllocStuff) updateClusterIPs(oldService *api.Service, service *api return nil, nil, nil } -func (al *RESTAllocStuff) txnUpdateNodePorts(service, oldService *api.Service, dryRun bool) (transaction, error) { +func (al *RESTAllocStuff) txnUpdateNodePorts(after After, before Before, dryRun bool) (transaction, error) { + oldService, service := before.Service, after.Service + // The allocator tracks dry-run-ness internally. nodePortOp := portallocator.StartOperation(al.serviceNodePorts, dryRun) @@ -747,14 +754,14 @@ func (al *RESTAllocStuff) txnUpdateNodePorts(service, oldService *api.Service, d // Update service from any type to NodePort or LoadBalancer, should update NodePort. if service.Spec.Type == api.ServiceTypeNodePort || service.Spec.Type == api.ServiceTypeLoadBalancer { - if err := al.updateNodePorts(oldService, service, nodePortOp); err != nil { + if err := al.updateNodePorts(After{service}, Before{oldService}, nodePortOp); err != nil { txn.Revert() return nil, err } } // Handle ExternalTraffic related updates. - success, err := al.updateHealthCheckNodePort(oldService, service, nodePortOp) + success, err := al.updateHealthCheckNodePort(After{service}, Before{oldService}, nodePortOp) if !success || err != nil { txn.Revert() return nil, err @@ -771,7 +778,9 @@ func (al *RESTAllocStuff) releaseNodePorts(service *api.Service, nodePortOp *por } } -func (al *RESTAllocStuff) updateNodePorts(oldService, newService *api.Service, nodePortOp *portallocator.PortAllocationOperation) error { +func (al *RESTAllocStuff) updateNodePorts(after After, before Before, nodePortOp *portallocator.PortAllocationOperation) error { + oldService, newService := before.Service, after.Service + oldNodePortsNumbers := collectServiceNodePorts(oldService) newNodePorts := []ServiceNodePort{} portAllocated := map[int]bool{} @@ -825,7 +834,9 @@ func (al *RESTAllocStuff) updateNodePorts(oldService, newService *api.Service, n // updateHealthCheckNodePort handles HealthCheckNodePort allocation/release // and adjusts HealthCheckNodePort during service update if needed. -func (al *RESTAllocStuff) updateHealthCheckNodePort(oldService, service *api.Service, nodePortOp *portallocator.PortAllocationOperation) (bool, error) { +func (al *RESTAllocStuff) updateHealthCheckNodePort(after After, before Before, nodePortOp *portallocator.PortAllocationOperation) (bool, error) { + oldService, service := before.Service, after.Service + neededHealthCheckNodePort := apiservice.NeedsHealthCheck(oldService) oldHealthCheckNodePort := oldService.Spec.HealthCheckNodePort @@ -976,7 +987,9 @@ func collectServiceNodePorts(service *api.Service) []int { // tests if two preferred dual-stack service have matching ClusterIPFields // assumption: old service is a valid, default service (e.g., loaded from store) -func isMatchingPreferDualStackClusterIPFields(oldService, service *api.Service) bool { +func isMatchingPreferDualStackClusterIPFields(after After, before Before) bool { + oldService, service := before.Service, after.Service + if oldService == nil { return false } @@ -1044,11 +1057,13 @@ func sameClusterIPs(lhs, rhs *api.Service) bool { return true } -func reducedClusterIPs(before, after *api.Service) bool { - if len(after.Spec.ClusterIPs) == 0 { // Not specified +func reducedClusterIPs(after After, before Before) bool { + oldSvc, newSvc := before.Service, after.Service + + if len(newSvc.Spec.ClusterIPs) == 0 { // Not specified return false } - return len(after.Spec.ClusterIPs) < len(before.Spec.ClusterIPs) + return len(newSvc.Spec.ClusterIPs) < len(oldSvc.Spec.ClusterIPs) } func sameIPFamilies(lhs, rhs *api.Service) bool { @@ -1065,11 +1080,13 @@ func sameIPFamilies(lhs, rhs *api.Service) bool { return true } -func reducedIPFamilies(before, after *api.Service) bool { - if len(after.Spec.IPFamilies) == 0 { // Not specified +func reducedIPFamilies(after After, before Before) bool { + oldSvc, newSvc := before.Service, after.Service + + if len(newSvc.Spec.IPFamilies) == 0 { // Not specified return false } - return len(after.Spec.IPFamilies) < len(before.Spec.IPFamilies) + return len(newSvc.Spec.IPFamilies) < len(oldSvc.Spec.IPFamilies) } // Helper to get the IP family of a given IP. diff --git a/pkg/registry/core/service/storage/storage.go b/pkg/registry/core/service/storage/storage.go index 3ba0bc6d574..6fdc002ed91 100644 --- a/pkg/registry/core/service/storage/storage.go +++ b/pkg/registry/core/service/storage/storage.go @@ -186,6 +186,25 @@ func (r *StatusREST) GetResetFields() map[fieldpath.APIVersion]*fieldpath.Set { return r.store.GetResetFields() } +// We have a lot of functions that take a pair of "before" and "after" or +// "oldSvc" and "newSvc" args. Convention across the codebase is to pass them +// as (new, old), but it's easy to screw up when they are the same type. +// +// These types force us to pay attention. If the order of the arguments +// matters, please receive them as: +// func something(after After, before Before) { +// oldSvc, newSvc := before.Service, after.Service +// +// If the order of arguments DOES NOT matter, please receive them as: +// func something(lhs, rhs *api.Service) { + +type Before struct { + *api.Service +} +type After struct { + *api.Service +} + // defaultOnRead sets interlinked fields that were not previously set on read. // We can't do this in the normal defaulting path because that same logic // applies on Get, Create, and Update, but we need to distinguish between them. @@ -222,8 +241,7 @@ func (r *REST) defaultOnReadService(service *api.Service) { // We might find Services that were written before ClusterIP became plural. // We still want to present a consistent view of them. - // NOTE: the args are (old, new) - normalizeClusterIPs(nil, service) + normalizeClusterIPs(After{service}, Before{nil}) // The rest of this does not apply unless dual-stack is enabled. if !utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) { @@ -324,8 +342,7 @@ func (r *REST) beginCreate(ctx context.Context, obj runtime.Object, options *met // Make sure ClusterIP and ClusterIPs are in sync. This has to happen // early, before anyone looks at them. - // NOTE: the args are (old, new) - normalizeClusterIPs(nil, svc) + normalizeClusterIPs(After{svc}, Before{nil}) // Allocate IPs and ports. If we had a transactional store, this would just // be part of the larger transaction. We don't have that, so we have to do @@ -355,15 +372,14 @@ func (r *REST) beginUpdate(ctx context.Context, obj, oldObj runtime.Object, opti // Fix up allocated values that the client may have not specified (for // idempotence). - patchAllocatedValues(newSvc, oldSvc) + patchAllocatedValues(After{newSvc}, Before{oldSvc}) // Make sure ClusterIP and ClusterIPs are in sync. This has to happen // early, before anyone looks at them. - // NOTE: the args are (old, new) - normalizeClusterIPs(oldSvc, newSvc) + normalizeClusterIPs(After{newSvc}, Before{oldSvc}) // Allocate and initialize fields. - txn, err := r.alloc.allocateUpdate(newSvc, oldSvc, dryrun.IsDryRun(options.DryRun)) + txn, err := r.alloc.allocateUpdate(After{newSvc}, Before{oldSvc}, dryrun.IsDryRun(options.DryRun)) if err != nil { return nil, err } @@ -481,7 +497,9 @@ func isValidAddress(ctx context.Context, addr *api.EndpointAddress, pods rest.Ge // normalizeClusterIPs adjust clusterIPs based on ClusterIP. This must not // consider any other fields. -func normalizeClusterIPs(oldSvc, newSvc *api.Service) { +func normalizeClusterIPs(after After, before Before) { + oldSvc, newSvc := before.Service, after.Service + // In all cases here, we don't need to over-think the inputs. Validation // will be called on the new object soon enough. All this needs to do is // try to divine what user meant with these linked fields. The below @@ -546,7 +564,9 @@ func normalizeClusterIPs(oldSvc, newSvc *api.Service) { // preserving values that we allocated on their behalf. For example, they // might create a Service without specifying the ClusterIP, in which case we // allocate one. If they resubmit that same YAML, we want it to succeed. -func patchAllocatedValues(newSvc, oldSvc *api.Service) { +func patchAllocatedValues(after After, before Before) { + oldSvc, newSvc := before.Service, after.Service + if needsClusterIP(oldSvc) && needsClusterIP(newSvc) { if newSvc.Spec.ClusterIP == "" { newSvc.Spec.ClusterIP = oldSvc.Spec.ClusterIP diff --git a/pkg/registry/core/service/storage/storage_test.go b/pkg/registry/core/service/storage/storage_test.go index 716deac191d..59bc4c03f15 100644 --- a/pkg/registry/core/service/storage/storage_test.go +++ b/pkg/registry/core/service/storage/storage_test.go @@ -402,7 +402,7 @@ func TestNormalizeClusterIPs(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - normalizeClusterIPs(tc.oldService, tc.newService) + normalizeClusterIPs(After{tc.newService}, Before{tc.oldService}) if tc.newService == nil { t.Fatalf("unexpected new service to be nil") @@ -506,7 +506,7 @@ func TestPatchAllocatedValues(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { update := tc.update.DeepCopy() - patchAllocatedValues(update, tc.before) + patchAllocatedValues(After{update}, Before{tc.before}) beforeIP := tc.before.Spec.ClusterIP updateIP := update.Spec.ClusterIP