Skip to content

Commit 4fba6f7

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #2918 from midavadim/427056894-reserve-static-ip
Avoid odd reservation calls for user specified IP addresses in "networking.gke.io/load-balancer-ip-addresses" annotation
2 parents 6557960 + 3fce564 commit 4fba6f7

File tree

13 files changed

+211
-81
lines changed

13 files changed

+211
-81
lines changed

‎pkg/address/hold.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,10 +45,11 @@ func HoldExternalIPv4(cfg HoldConfig) (HoldResult, error) {
4545
subnet := ""
4646
name := utils.LegacyForwardingRuleName(cfg.Service)
4747

48+
var ipv4AddressName string
4849
// Determine IP which will be used for this LB. If no forwarding rule has been established
4950
// or specified in the Service spec, then requestedIP = "".
5051
rule := pickForwardingRuleToInferIP(cfg.ExistingRules)
51-
res.IP, err = IPv4ToUse(cfg.Cloud, cfg.Recorder, cfg.Service, rule, subnet)
52+
res.IP, ipv4AddressName, err = IPv4ToUse(cfg.Cloud, cfg.Recorder, cfg.Service, rule, subnet)
5253
if err != nil {
5354
log.Error(err, "IPv4ToUse for service returned error")
5455
return res, err
@@ -67,7 +68,7 @@ func HoldExternalIPv4(cfg HoldConfig) (HoldResult, error) {
6768

6869
addrMgr := NewManager(
6970
cfg.Cloud, nm, cfg.Cloud.Region(),
70-
subnet, name, res.IP,
71+
subnet, name, ipv4AddressName, res.IP,
7172
cloud.SchemeExternal, netTier, IPv4Version, cfg.Logger,
7273
)
7374

‎pkg/address/hold_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ func TestHoldExternalIPv4(t *testing.T) {
6565
},
6666
want: address.HoldResult{
6767
IP: reservedIPv4,
68-
Managed: address.IPAddrManaged,
68+
Managed: address.IPAddrUnmanaged,
6969
},
7070
},
7171
{
@@ -266,6 +266,7 @@ func arrange(t *testing.T, existingRules []*composite.ForwardingRule, service *a
266266
Region: vals.Region,
267267
Address: reservedIPv4,
268268
AddressType: "EXTERNAL",
269+
NetworkTier: "PREMIUM",
269270
}
270271
if err := fakeGCE.ReserveRegionAddress(addr, vals.Region); err != nil {
271272
t.Fatal(err)

‎pkg/address/manager.go

Lines changed: 47 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ type Manager struct {
5858
svc gce.CloudAddressService
5959
name string
6060
serviceName string
61+
addressName string
6162
targetIP string
6263
addressType cloud.LbScheme
6364
region string
@@ -69,7 +70,7 @@ type Manager struct {
6970
frLogger klog.Logger
7071
}
7172

72-
func NewManager(svc gce.CloudAddressService, serviceName, region, subnetURL, name, targetIP string, addressType cloud.LbScheme, networkTier cloud.NetworkTier, ipVersion IPVersion, frLogger klog.Logger) *Manager {
73+
func NewManager(svc gce.CloudAddressService, serviceName, region, subnetURL, name, addressName, targetIP string, addressType cloud.LbScheme, networkTier cloud.NetworkTier, ipVersion IPVersion, frLogger klog.Logger) *Manager {
7374
if targetIP != "" {
7475
// Store address in normalized format.
7576
// This is required for IPv6 addresses, to be able to filter by exact address,
@@ -82,6 +83,7 @@ func NewManager(svc gce.CloudAddressService, serviceName, region, subnetURL, nam
8283
region: region,
8384
serviceName: serviceName,
8485
name: name,
86+
addressName: addressName,
8587
targetIP: targetIP,
8688
addressType: addressType,
8789
tryRelease: true,
@@ -103,8 +105,31 @@ func (m *Manager) HoldAddress() (string, IPAddressType, error) {
103105
// case of using a controller address, retrieving the address by name results in the fewest API
104106
// calls since it indicates whether a Delete is necessary before Reserve.
105107
m.frLogger.V(4).Info("Attempting hold of IP", "ip", m.targetIP, "addressType", m.addressType)
106-
// Get the address in case it was orphaned earlier
107-
addr, err := m.svc.GetRegionAddress(m.name, m.region)
108+
109+
// Name for default managed Address which is based on forwarding rule name
110+
defaultName := m.name
111+
addressName := defaultName
112+
addressManagementType := IPAddrManaged
113+
114+
if m.addressName != "" {
115+
// Name for custom unmanaged static Address which is reserved by user and specified in annotation
116+
addressName = m.addressName
117+
addressManagementType = IPAddrUnmanaged
118+
m.tryRelease = false
119+
120+
// Custom reserved Address is specified so no possible orphaned default address should exist
121+
if addressName != defaultName {
122+
if addrDefault, _ := m.svc.GetRegionAddress(defaultName, m.region); addrDefault != nil {
123+
releaseErr := m.removeAddress(defaultName, "address is orphaned and not needed anymore")
124+
if releaseErr != nil {
125+
m.frLogger.V(4).Info("Unable to release orphaned Address.", "addressName", defaultName, "err", releaseErr)
126+
}
127+
}
128+
}
129+
}
130+
131+
// Get and validate existing Address: user-specifed or default (orphaned)
132+
addr, err := m.svc.GetRegionAddress(addressName, m.region)
108133
if err != nil && !utils.IsNotFoundError(err) {
109134
return "", IPAddrUndefined, err
110135
}
@@ -114,19 +139,17 @@ func (m *Manager) HoldAddress() (string, IPAddressType, error) {
114139
validationError := m.validateAddress(addr)
115140
if validationError == nil {
116141
m.frLogger.V(4).Info("Address already reserves IP. No further action required.", "addressName", addr.Name, "ip", addr.Address, "type", addr.AddressType)
117-
return addr.Address, IPAddrManaged, nil
142+
return addr.Address, addressManagementType, nil
118143
}
119144

120-
m.frLogger.V(2).Info("Deleting existing address", "reason", validationError)
121-
err := m.svc.DeleteRegionAddress(addr.Name, m.region)
122-
if err != nil {
123-
if utils.IsNotFoundError(err) {
124-
m.frLogger.V(4).Info("Address was not found. Ignoring.", "addressName", addr.Name)
125-
} else {
126-
return "", IPAddrUndefined, err
127-
}
145+
if m.addressName != "" {
146+
return "", IPAddrUndefined, fmt.Errorf("reserved address (%q) is not valid, err: %w", addr.Name, validationError)
128147
} else {
129-
m.frLogger.V(4).Info("Successfully deleted previous address", "addressName", addr.Name)
148+
// Remove invalid orphaned default address. New Address will be reserved later in ensureAddressReservation()
149+
releaseErr := m.removeAddress(defaultName, fmt.Sprintf("address is orphaned and not valid, err: %v", validationError))
150+
if releaseErr != nil {
151+
m.frLogger.V(4).Info("Unable to release orphaned Address.", "addressName", defaultName, "err", releaseErr)
152+
}
130153
}
131154
}
132155

@@ -140,19 +163,21 @@ func (m *Manager) ReleaseAddress() error {
140163
return nil
141164
}
142165

143-
m.frLogger.V(4).Info("Releasing address", "ip", m.targetIP, "addressName", m.name)
144-
// Controller only ever tries to unreserve the address named with the load balancer's name.
145-
err := m.svc.DeleteRegionAddress(m.name, m.region)
166+
return m.removeAddress(m.name, "address was temporary reserved")
167+
}
168+
169+
func (m *Manager) removeAddress(name, reason string) error {
170+
171+
m.frLogger.V(2).Info("Releasing existing address", "name", name, "reason", reason)
172+
err := m.svc.DeleteRegionAddress(name, m.region)
146173
if err != nil {
147174
if utils.IsNotFoundError(err) {
148-
m.frLogger.Info("Address was not found. Ignoring.", "addressName", m.name)
149-
return nil
175+
m.frLogger.V(4).Info("Address was not found. Ignoring.", "addressName", name)
176+
} else {
177+
return err
150178
}
151-
152-
return err
153179
}
154-
155-
m.frLogger.V(4).Info("Successfully released IP named", "ip", m.targetIP, "addressName", m.name)
180+
m.frLogger.V(4).Info("Successfully released address", "addressName", name)
156181
return nil
157182
}
158183

‎pkg/address/manager_test.go

Lines changed: 79 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ func TestAddressManagerNoRequestedIP(t *testing.T) {
4848
require.NoError(t, err)
4949
targetIP := ""
5050

51-
mgr := address.NewManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierDefault, address.IPv4Version, klog.TODO())
51+
mgr := address.NewManager(svc, testSvcName, vals.Region, testSubnet, testLBName, "", targetIP, cloud.SchemeInternal, cloud.NetworkTierDefault, address.IPv4Version, klog.TODO())
5252
testHoldAddress(t, mgr, svc, testLBName, vals.Region, targetIP, string(cloud.SchemeInternal), cloud.NetworkTierDefault.ToGCEValue())
5353
testReleaseAddress(t, mgr, svc, testLBName, vals.Region)
5454
}
@@ -59,7 +59,7 @@ func TestAddressManagerBasic(t *testing.T) {
5959
require.NoError(t, err)
6060
targetIP := "1.1.1.1"
6161

62-
mgr := address.NewManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierDefault, address.IPv4Version, klog.TODO())
62+
mgr := address.NewManager(svc, testSvcName, vals.Region, testSubnet, testLBName, "", targetIP, cloud.SchemeInternal, cloud.NetworkTierDefault, address.IPv4Version, klog.TODO())
6363
testHoldAddress(t, mgr, svc, testLBName, vals.Region, targetIP, string(cloud.SchemeInternal), cloud.NetworkTierDefault.ToGCEValue())
6464
testReleaseAddress(t, mgr, svc, testLBName, vals.Region)
6565
}
@@ -75,7 +75,7 @@ func TestAddressManagerOrphaned(t *testing.T) {
7575
err = svc.ReserveRegionAddress(addr, vals.Region)
7676
require.NoError(t, err)
7777

78-
mgr := address.NewManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierDefault, address.IPv4Version, klog.TODO())
78+
mgr := address.NewManager(svc, testSvcName, vals.Region, testSubnet, testLBName, "", targetIP, cloud.SchemeInternal, cloud.NetworkTierDefault, address.IPv4Version, klog.TODO())
7979
testHoldAddress(t, mgr, svc, testLBName, vals.Region, targetIP, string(cloud.SchemeInternal), cloud.NetworkTierDefault.ToGCEValue())
8080
testReleaseAddress(t, mgr, svc, testLBName, vals.Region)
8181
}
@@ -87,7 +87,7 @@ func TestAddressManagerStandardNetworkTier(t *testing.T) {
8787
require.NoError(t, err)
8888
targetIP := "1.1.1.1"
8989

90-
mgr := address.NewManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeExternal, cloud.NetworkTierStandard, address.IPv4Version, klog.TODO())
90+
mgr := address.NewManager(svc, testSvcName, vals.Region, testSubnet, testLBName, "", targetIP, cloud.SchemeExternal, cloud.NetworkTierStandard, address.IPv4Version, klog.TODO())
9191
testHoldAddress(t, mgr, svc, testLBName, vals.Region, targetIP, string(cloud.SchemeExternal), cloud.NetworkTierStandard.ToGCEValue())
9292
testReleaseAddress(t, mgr, svc, testLBName, vals.Region)
9393
}
@@ -98,7 +98,7 @@ func TestAddressManagerStandardNetworkTierNotAvailableForInternalAddress(t *test
9898
require.NoError(t, err)
9999
targetIP := "1.1.1.1"
100100

101-
mgr := address.NewManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierStandard, address.IPv4Version, klog.TODO())
101+
mgr := address.NewManager(svc, testSvcName, vals.Region, testSubnet, testLBName, "", targetIP, cloud.SchemeInternal, cloud.NetworkTierStandard, address.IPv4Version, klog.TODO())
102102
testHoldAddress(t, mgr, svc, testLBName, vals.Region, targetIP, string(cloud.SchemeInternal), cloud.NetworkTierPremium.ToGCEValue())
103103
testReleaseAddress(t, mgr, svc, testLBName, vals.Region)
104104
}
@@ -115,7 +115,7 @@ func TestAddressManagerOutdatedOrphan(t *testing.T) {
115115
err = svc.ReserveRegionAddress(addr, vals.Region)
116116
require.NoError(t, err)
117117

118-
mgr := address.NewManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierDefault, address.IPv4Version, klog.TODO())
118+
mgr := address.NewManager(svc, testSvcName, vals.Region, testSubnet, testLBName, "", targetIP, cloud.SchemeInternal, cloud.NetworkTierDefault, address.IPv4Version, klog.TODO())
119119
testHoldAddress(t, mgr, svc, testLBName, vals.Region, targetIP, string(cloud.SchemeInternal), cloud.NetworkTierDefault.ToGCEValue())
120120
testReleaseAddress(t, mgr, svc, testLBName, vals.Region)
121121
}
@@ -131,7 +131,7 @@ func TestAddressManagerExternallyOwned(t *testing.T) {
131131
err = svc.ReserveRegionAddress(addr, vals.Region)
132132
require.NoError(t, err)
133133

134-
mgr := address.NewManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierPremium, address.IPv4Version, klog.TODO())
134+
mgr := address.NewManager(svc, testSvcName, vals.Region, testSubnet, testLBName, "", targetIP, cloud.SchemeInternal, cloud.NetworkTierPremium, address.IPv4Version, klog.TODO())
135135
ipToUse, ipType, err := mgr.HoldAddress()
136136
require.NoError(t, err)
137137
assert.NotEmpty(t, ipToUse)
@@ -144,14 +144,53 @@ func TestAddressManagerExternallyOwned(t *testing.T) {
144144
testReleaseAddress(t, mgr, svc, testLBName, vals.Region)
145145
}
146146

147+
// TestAddressManagerExternallyOwnedAndOrphaned tests the case where extrenal address is used
148+
// and obsolete orphaned address is removed
149+
func TestAddressManagerExternallyOwnedAndOrphaned(t *testing.T) {
150+
svc, err := fakeGCECloud(vals)
151+
require.NoError(t, err)
152+
153+
// ExternallyOwned IP
154+
externalName := "my-important-address"
155+
externalIP := "1.1.1.1"
156+
addr := &compute.Address{Name: externalName, Address: externalIP, AddressType: string(cloud.SchemeInternal)}
157+
err = svc.ReserveRegionAddress(addr, vals.Region)
158+
require.NoError(t, err)
159+
160+
// Orphaned IP with default LBName name
161+
orphanedName := testLBName
162+
orphanedIP := "1.1.1.100"
163+
orphaned_addr := &compute.Address{Name: orphanedName, Address: orphanedIP, AddressType: string(cloud.SchemeInternal)}
164+
err = svc.ReserveRegionAddress(orphaned_addr, vals.Region)
165+
require.NoError(t, err)
166+
167+
mgr := address.NewManager(svc, testSvcName, vals.Region, testSubnet, testLBName, externalName, externalIP, cloud.SchemeInternal, cloud.NetworkTierPremium, address.IPv4Version, klog.TODO())
168+
ipToUse, ipType, err := mgr.HoldAddress()
169+
require.NoError(t, err)
170+
assert.Equal(t, ipToUse, externalIP)
171+
assert.Equal(t, address.IPAddrUnmanaged, ipType, "IP Address should not be marked as controller's managed")
172+
173+
// Orphaned IP should be removed
174+
_, err = svc.GetRegionAddress(testLBName, vals.Region)
175+
assert.True(t, utils.IsNotFoundError(err), "Orphaned Address should be removed")
176+
177+
err = mgr.ReleaseAddress()
178+
require.NoError(t, err)
179+
180+
// ExternallyOwned IP should stay untouched
181+
addr, _ = svc.GetRegionAddress(externalName, vals.Region)
182+
assert.NotNil(t, addr)
183+
184+
}
185+
147186
// TestAddressManagerNonExisting tests the case where the address can't be reserved
148187
// automatically and was not reserved by the user (external address case).
149188
func TestAddressManagerNonExisting(t *testing.T) {
150189
svc, err := fakeGCECloud(vals)
151190
require.NoError(t, err)
152191
targetIP := "1.1.1.1"
153192

154-
mgr := address.NewManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeExternal, cloud.NetworkTierPremium, address.IPv4Version, klog.TODO())
193+
mgr := address.NewManager(svc, testSvcName, vals.Region, testSubnet, testLBName, "", targetIP, cloud.SchemeExternal, cloud.NetworkTierPremium, address.IPv4Version, klog.TODO())
155194

156195
svc.Compute().(*cloud.MockGCE).MockAddresses.InsertHook = test.InsertAddressNotAllocatedToProjectErrorHook
157196
_, _, err = mgr.HoldAddress()
@@ -171,7 +210,7 @@ func TestAddressManagerWrongTypeReserved(t *testing.T) {
171210
t.Errorf("svc.ReserveRegionAddress returned err: %v", err)
172211
}
173212

174-
mgr := address.NewManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeExternal, cloud.NetworkTierPremium, address.IPv4Version, klog.TODO())
213+
mgr := address.NewManager(svc, testSvcName, vals.Region, testSubnet, testLBName, "", targetIP, cloud.SchemeExternal, cloud.NetworkTierPremium, address.IPv4Version, klog.TODO())
175214

176215
_, _, err = mgr.HoldAddress()
177216
require.Error(t, err)
@@ -188,7 +227,7 @@ func TestAddressManagerExternallyOwnedWrongNetworkTier(t *testing.T) {
188227
addr := &compute.Address{Name: "my-important-address", Address: targetIP, AddressType: string(cloud.SchemeInternal), NetworkTier: string(cloud.NetworkTierStandard)}
189228
err = svc.ReserveRegionAddress(addr, vals.Region)
190229
require.NoError(t, err, "")
191-
mgr := address.NewManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierPremium, address.IPv4Version, klog.TODO())
230+
mgr := address.NewManager(svc, testSvcName, vals.Region, testSubnet, testLBName, "", targetIP, cloud.SchemeInternal, cloud.NetworkTierPremium, address.IPv4Version, klog.TODO())
192231
svc.Compute().(*cloud.MockGCE).MockAddresses.InsertHook = test.InsertAddressNetworkErrorHook
193232
_, _, err = mgr.HoldAddress()
194233
if err == nil || !utils.IsNetworkTierError(err) {
@@ -207,12 +246,40 @@ func TestAddressManagerBadExternallyOwned(t *testing.T) {
207246
err = svc.ReserveRegionAddress(addr, vals.Region)
208247
require.NoError(t, err)
209248

210-
mgr := address.NewManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierPremium, address.IPv4Version, klog.TODO())
249+
mgr := address.NewManager(svc, testSvcName, vals.Region, testSubnet, testLBName, "", targetIP, cloud.SchemeInternal, cloud.NetworkTierPremium, address.IPv4Version, klog.TODO())
211250
ad, _, err := mgr.HoldAddress()
212251
assert.NotNil(t, err) // FIXME
213252
require.Equal(t, ad, "")
214253
}
215254

255+
// TestAddressManagerBadExternallyOwnedFromAnnotation tests the case where the address exists but isn't
256+
// owned by the controller. However, this address has the wrong type.
257+
func TestAddressManagerBadExternallyOwnedFromAnnotation(t *testing.T) {
258+
svc, err := fakeGCECloud(vals)
259+
require.NoError(t, err)
260+
targetIP := "1.1.1.1"
261+
addrName := "my-important-address"
262+
263+
addrExternal := &compute.Address{Name: addrName, Address: targetIP, AddressType: string(cloud.SchemeExternal)}
264+
err = svc.ReserveRegionAddress(addrExternal, vals.Region)
265+
require.NoError(t, err)
266+
267+
addrDefault := &compute.Address{Name: testLBName, Address: "1.1.1.100", AddressType: string(cloud.SchemeInternal)}
268+
err = svc.ReserveRegionAddress(addrDefault, vals.Region)
269+
require.NoError(t, err)
270+
271+
mgr := address.NewManager(svc, testSvcName, vals.Region, testSubnet, testLBName, addrName, targetIP, cloud.SchemeInternal, cloud.NetworkTierPremium, address.IPv4Version, klog.TODO())
272+
ad, _, err := mgr.HoldAddress()
273+
assert.NotNil(t, err) // FIXME
274+
require.Equal(t, ad, "")
275+
276+
addrExternal, _ = svc.GetRegionAddress(addrName, vals.Region)
277+
assert.NotNil(t, addrExternal, "ExternallyOwned IP should stay untouched")
278+
279+
_, err = svc.GetRegionAddress(testLBName, vals.Region)
280+
assert.True(t, utils.IsNotFoundError(err), "Orphaned Address should be removed")
281+
}
282+
216283
// TestAddressManagerIPv6 tests the typical case of reserving and releasing an IPv6 address.
217284
func TestAddressManagerIPv6(t *testing.T) {
218285
testCases := []struct {
@@ -239,7 +306,7 @@ func TestAddressManagerIPv6(t *testing.T) {
239306
t.Fatalf("fakeGCECloud(%v) returned error %v", vals, err)
240307
}
241308

242-
mgr := address.NewManager(svc, testSvcName, vals.Region, testSubnet, testLBName, tc.targetIP, cloud.SchemeInternal, cloud.NetworkTierDefault, address.IPv6Version, klog.TODO())
309+
mgr := address.NewManager(svc, testSvcName, vals.Region, testSubnet, testLBName, "", tc.targetIP, cloud.SchemeInternal, cloud.NetworkTierDefault, address.IPv6Version, klog.TODO())
243310
testHoldAddress(t, mgr, svc, testLBName, vals.Region, tc.targetIP, string(cloud.SchemeInternal), cloud.NetworkTierDefault.ToGCEValue())
244311
testReleaseAddress(t, mgr, svc, testLBName, vals.Region)
245312
})

0 commit comments

Comments
 (0)