github/kubernetes
1
0
Fork 0
mirror of https://github.com/k3s-io/kubernetes.git synced 2025-08-05 02:09:56 +00:00
Code Issues Projects Releases Wiki Activity

Svc REST: Use types for safer arg ordering

Browse Source 
In all the places we pass (old, new) or (new, old), use wrapper-types to
make sure that we don't flip the order by accident.
This commit is contained in:
Tim Hockin 2021-08-24 23:47:59 -07:00
parent d5143bca84
commit 5847426e5e
3 changed files with 74 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

67
pkg/registry/core/service/storage/alloc.go
View File

@ -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 // Ensure IP family fields are correctly initialized. We do it here, since
// we want this to be visible even when dryRun == true. // 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 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 // attempts to default service ip families according to cluster configuration
// while ensuring that provided families are configured on cluster. // 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 // can not do anything here
if service.Spec.Type == api.ServiceTypeExternalName { if service.Spec.Type == api.ServiceTypeExternalName {
return nil return nil
@ -113,7 +115,7 @@ func (al *RESTAllocStuff) initIPFamilyFields(oldService, service *api.Service) e
// when: // when:
// - changing ipFamilyPolicy to "RequireDualStack" or "SingleStack" AND // - changing ipFamilyPolicy to "RequireDualStack" or "SingleStack" AND
// - adding or removing a secondary clusterIP or ipFamily // - adding or removing a secondary clusterIP or ipFamily
if isMatchingPreferDualStackClusterIPFields(oldService, service) { if isMatchingPreferDualStackClusterIPFields(after, before) {
return nil // nothing more to do. return nil // nothing more to do.
} }
@ -172,11 +174,11 @@ func (al *RESTAllocStuff) initIPFamilyFields(oldService, service *api.Service) e
} else { } else {
// If the policy is anything but single-stack AND they reduced these // If the policy is anything but single-stack AND they reduced these
// fields, it's an error. They need to specify policy. // 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, el = append(el, field.Invalid(field.NewPath("spec", "ipFamilyPolicy"), service.Spec.IPFamilyPolicy,
"must be 'SingleStack' to release the secondary cluster IP")) "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, el = append(el, field.Invalid(field.NewPath("spec", "ipFamilyPolicy"), service.Spec.IPFamilyPolicy,
"must be 'SingleStack' to release the secondary IP family")) "must be 'SingleStack' to release the secondary IP family"))
} }
@ -565,12 +567,12 @@ func (al *RESTAllocStuff) allocHealthCheckNodePort(service *api.Service, nodePor
return nil 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{} result := metaTransaction{}
// Ensure IP family fields are correctly initialized. We do it here, since // Ensure IP family fields are correctly initialized. We do it here, since
// we want this to be visible even when dryRun == true. // 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 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 //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 //does (and is tested!). Fixing that all is a big PR and will have to
//happen later. //happen later.
if txn, err := al.txnUpdateClusterIPs(service, oldService, dryRun); err != nil { if txn, err := al.txnUpdateClusterIPs(after, before, dryRun); err != nil {
result.Revert() result.Revert()
return nil, err return nil, err
} else { } else {
@ -586,7 +588,7 @@ func (al *RESTAllocStuff) allocateUpdate(service, oldService *api.Service, dryRu
} }
// Allocate ports // Allocate ports
if txn, err := al.txnUpdateNodePorts(service, oldService, dryRun); err != nil { if txn, err := al.txnUpdateNodePorts(after, before, dryRun); err != nil {
result.Revert() result.Revert()
return nil, err return nil, err
} else { } else {
@ -596,8 +598,10 @@ func (al *RESTAllocStuff) allocateUpdate(service, oldService *api.Service, dryRu
return result, nil return result, nil
} }
func (al *RESTAllocStuff) txnUpdateClusterIPs(service *api.Service, oldService *api.Service, dryRun bool) (transaction, error) { func (al *RESTAllocStuff) txnUpdateClusterIPs(after After, before Before, dryRun bool) (transaction, error) {
allocated, released, err := al.updateClusterIPs(oldService, service, dryRun) service := after.Service
allocated, released, err := al.updateClusterIPs(after, before, dryRun)
if err != nil { if err != nil {
return nil, err 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 // this func does not perform actual release of clusterIPs. it returns
// a map[family]ip for the caller to release when everything else has // a map[family]ip for the caller to release when everything else has
// executed successfully // 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) // We don't want to auto-upgrade (add an IP) or downgrade (remove an IP)
// PreferDualStack services following a cluster change to/from // PreferDualStack services following a cluster change to/from
@ -644,7 +649,7 @@ func (al *RESTAllocStuff) updateClusterIPs(oldService *api.Service, service *api
// when: // when:
// - changing ipFamilyPolicy to "RequireDualStack" or "SingleStack" AND // - changing ipFamilyPolicy to "RequireDualStack" or "SingleStack" AND
// - adding or removing a secondary clusterIP or ipFamily // - adding or removing a secondary clusterIP or ipFamily
if isMatchingPreferDualStackClusterIPFields(oldService, service) { if isMatchingPreferDualStackClusterIPFields(after, before) {
return allocated, toRelease, nil // nothing more to do. 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 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. // The allocator tracks dry-run-ness internally.
nodePortOp := portallocator.StartOperation(al.serviceNodePorts, dryRun) 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. // Update service from any type to NodePort or LoadBalancer, should update NodePort.
if service.Spec.Type == api.ServiceTypeNodePort || service.Spec.Type == api.ServiceTypeLoadBalancer { 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() txn.Revert()
return nil, err return nil, err
} }
} }
// Handle ExternalTraffic related updates. // Handle ExternalTraffic related updates.
success, err := al.updateHealthCheckNodePort(oldService, service, nodePortOp) success, err := al.updateHealthCheckNodePort(After{service}, Before{oldService}, nodePortOp)
if !success || err != nil { if !success || err != nil {
txn.Revert() txn.Revert()
return nil, err 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) oldNodePortsNumbers := collectServiceNodePorts(oldService)
newNodePorts := []ServiceNodePort{} newNodePorts := []ServiceNodePort{}
portAllocated := map[int]bool{} portAllocated := map[int]bool{}
@ -825,7 +834,9 @@ func (al *RESTAllocStuff) updateNodePorts(oldService, newService *api.Service, n
// updateHealthCheckNodePort handles HealthCheckNodePort allocation/release // updateHealthCheckNodePort handles HealthCheckNodePort allocation/release
// and adjusts HealthCheckNodePort during service update if needed. // 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) neededHealthCheckNodePort := apiservice.NeedsHealthCheck(oldService)
oldHealthCheckNodePort := oldService.Spec.HealthCheckNodePort oldHealthCheckNodePort := oldService.Spec.HealthCheckNodePort
@ -976,7 +987,9 @@ func collectServiceNodePorts(service *api.Service) []int {
// tests if two preferred dual-stack service have matching ClusterIPFields // tests if two preferred dual-stack service have matching ClusterIPFields
// assumption: old service is a valid, default service (e.g., loaded from store) // 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 { if oldService == nil {
return false return false
} }
@ -1044,11 +1057,13 @@ func sameClusterIPs(lhs, rhs *api.Service) bool {
return true return true
} }
func reducedClusterIPs(before, after *api.Service) bool { func reducedClusterIPs(after After, before Before) bool {
if len(after.Spec.ClusterIPs) == 0 { // Not specified oldSvc, newSvc := before.Service, after.Service
if len(newSvc.Spec.ClusterIPs) == 0 { // Not specified
return false 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 { func sameIPFamilies(lhs, rhs *api.Service) bool {
@ -1065,11 +1080,13 @@ func sameIPFamilies(lhs, rhs *api.Service) bool {
return true return true
} }
func reducedIPFamilies(before, after *api.Service) bool { func reducedIPFamilies(after After, before Before) bool {
if len(after.Spec.IPFamilies) == 0 { // Not specified oldSvc, newSvc := before.Service, after.Service
if len(newSvc.Spec.IPFamilies) == 0 { // Not specified
return false 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. // Helper to get the IP family of a given IP.

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

@ -186,6 +186,25 @@ func (r *StatusREST) GetResetFields() map[fieldpath.APIVersion]*fieldpath.Set {
return r.store.GetResetFields() 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. // 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 // 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. // 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 might find Services that were written before ClusterIP became plural.
// We still want to present a consistent view of them. // We still want to present a consistent view of them.
// NOTE: the args are (old, new) normalizeClusterIPs(After{service}, Before{nil})
normalizeClusterIPs(nil, service)
// The rest of this does not apply unless dual-stack is enabled. // The rest of this does not apply unless dual-stack is enabled.
if !utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) { 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 // Make sure ClusterIP and ClusterIPs are in sync. This has to happen
// early, before anyone looks at them. // early, before anyone looks at them.
// NOTE: the args are (old, new) normalizeClusterIPs(After{svc}, Before{nil})
normalizeClusterIPs(nil, svc)
// Allocate IPs and ports. If we had a transactional store, this would just // 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 // 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 // Fix up allocated values that the client may have not specified (for
// idempotence). // idempotence).
patchAllocatedValues(newSvc, oldSvc) patchAllocatedValues(After{newSvc}, Before{oldSvc})
// Make sure ClusterIP and ClusterIPs are in sync. This has to happen // Make sure ClusterIP and ClusterIPs are in sync. This has to happen
// early, before anyone looks at them. // early, before anyone looks at them.
// NOTE: the args are (old, new) normalizeClusterIPs(After{newSvc}, Before{oldSvc})
normalizeClusterIPs(oldSvc, newSvc)
// Allocate and initialize fields. // 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 { if err != nil {
return nil, err 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 // normalizeClusterIPs adjust clusterIPs based on ClusterIP. This must not
// consider any other fields. // 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 // 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 // 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 // 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 // preserving values that we allocated on their behalf. For example, they
// might create a Service without specifying the ClusterIP, in which case we // 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. // 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 needsClusterIP(oldSvc) && needsClusterIP(newSvc) {
if newSvc.Spec.ClusterIP == "" { if newSvc.Spec.ClusterIP == "" {
newSvc.Spec.ClusterIP = oldSvc.Spec.ClusterIP newSvc.Spec.ClusterIP = oldSvc.Spec.ClusterIP

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

@ -402,7 +402,7 @@ func TestNormalizeClusterIPs(t *testing.T) {
for _, tc := range testCases { for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) { t.Run(tc.name, func(t *testing.T) {
normalizeClusterIPs(tc.oldService, tc.newService) normalizeClusterIPs(After{tc.newService}, Before{tc.oldService})
if tc.newService == nil { if tc.newService == nil {
t.Fatalf("unexpected new service to be nil") t.Fatalf("unexpected new service to be nil")
@ -506,7 +506,7 @@ func TestPatchAllocatedValues(t *testing.T) {
for _, tc := range testCases { for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) { t.Run(tc.name, func(t *testing.T) {
update := tc.update.DeepCopy() update := tc.update.DeepCopy()
patchAllocatedValues(update, tc.before) patchAllocatedValues(After{update}, Before{tc.before})
beforeIP := tc.before.Spec.ClusterIP beforeIP := tc.before.Spec.ClusterIP
updateIP := update.Spec.ClusterIP updateIP := update.Spec.ClusterIP