From 009aa36c89e1aca05cd09ad228f801aacae607fc Mon Sep 17 00:00:00 2001
From: Tim Hockin <thockin@google.com>
Date: Fri, 10 Sep 2021 21:40:22 -0700
Subject: [PATCH] Svc REST: Make transaction-accumulating funcs safe

Identified in review, these funcs are now more reslient to future
changes.
---
 pkg/registry/core/service/storage/alloc.go | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/pkg/registry/core/service/storage/alloc.go b/pkg/registry/core/service/storage/alloc.go
index b80dbe25bfb..45cc8154410 100644
--- a/pkg/registry/core/service/storage/alloc.go
+++ b/pkg/registry/core/service/storage/alloc.go
@@ -62,6 +62,13 @@ func makeAlloc(defaultFamily api.IPFamily, ipAllocs map[api.IPFamily]ipallocator
 
 func (al *Allocators) allocateCreate(service *api.Service, dryRun bool) (transaction, error) {
 	result := metaTransaction{}
+	success := false
+
+	defer func() {
+		if !success {
+			result.Revert()
+		}
+	}()
 
 	// Ensure IP family fields are correctly initialized.  We do it here, since
 	// we want this to be visible even when dryRun == true.
@@ -74,7 +81,6 @@ func (al *Allocators) allocateCreate(service *api.Service, dryRun bool) (transac
 	//does (and is tested!).  Fixing that all is a big PR and will have to
 	//happen later.
 	if txn, err := al.txnAllocClusterIPs(service, dryRun); err != nil {
-		result.Revert()
 		return nil, err
 	} else {
 		result = append(result, txn)
@@ -82,12 +88,12 @@ func (al *Allocators) allocateCreate(service *api.Service, dryRun bool) (transac
 
 	// Allocate ports
 	if txn, err := al.txnAllocNodePorts(service, dryRun); err != nil {
-		result.Revert()
 		return nil, err
 	} else {
 		result = append(result, txn)
 	}
 
+	success = true
 	return result, nil
 }
 
@@ -569,6 +575,13 @@ func (al *Allocators) allocHealthCheckNodePort(service *api.Service, nodePortOp
 
 func (al *Allocators) allocateUpdate(after After, before Before, dryRun bool) (transaction, error) {
 	result := metaTransaction{}
+	success := false
+
+	defer func() {
+		if !success {
+			result.Revert()
+		}
+	}()
 
 	// Ensure IP family fields are correctly initialized.  We do it here, since
 	// we want this to be visible even when dryRun == true.
@@ -581,7 +594,6 @@ func (al *Allocators) allocateUpdate(after After, before Before, dryRun bool) (t
 	//does (and is tested!).  Fixing that all is a big PR and will have to
 	//happen later.
 	if txn, err := al.txnUpdateClusterIPs(after, before, dryRun); err != nil {
-		result.Revert()
 		return nil, err
 	} else {
 		result = append(result, txn)
@@ -589,12 +601,12 @@ func (al *Allocators) allocateUpdate(after After, before Before, dryRun bool) (t
 
 	// Allocate ports
 	if txn, err := al.txnUpdateNodePorts(after, before, dryRun); err != nil {
-		result.Revert()
 		return nil, err
 	} else {
 		result = append(result, txn)
 	}
 
+	success = true
 	return result, nil
 }