Skip to content

Commit 604759f

Browse files
yfodilyfodil
authored
fix(lb): unnecessary reattach of private networks (scaleway#2531)
* fix(lb): unnecessary reattach of private networks * add unit tests
1 parent 7a2868f commit 604759f

File tree

4 files changed

+6695
-4737
lines changed

4 files changed

+6695
-4737
lines changed

‎internal/services/lb/helpers_lb.go‎

Lines changed: 40 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -49,42 +49,53 @@ func NewAPIWithZoneAndID(m interface{}, id string) (*lbSDK.ZonedAPI, scw.Zone, s
4949
return lbAPI, zone, ID, nil
5050
}
5151

52-
func IsPrivateNetworkEqual(a, b interface{}) bool {
53-
// Find out the diff Private Network or not
54-
if _, ok := a.(*lbSDK.PrivateNetwork); ok {
55-
if _, ok := b.(*lbSDK.PrivateNetwork); ok {
56-
if a.(*lbSDK.PrivateNetwork).PrivateNetworkID == b.(*lbSDK.PrivateNetwork).PrivateNetworkID {
57-
// if both has dhcp config should not update
58-
if a.(*lbSDK.PrivateNetwork).DHCPConfig != nil && b.(*lbSDK.PrivateNetwork).DHCPConfig != nil {
59-
return true
60-
}
61-
// check static config
62-
aConfig := a.(*lbSDK.PrivateNetwork).StaticConfig
63-
bConfig := b.(*lbSDK.PrivateNetwork).StaticConfig
64-
if aConfig != nil && bConfig != nil {
65-
// check if static config is different
66-
return reflect.DeepEqual(aConfig.IPAddress, bConfig.IPAddress)
67-
}
68-
}
69-
}
52+
func IsPrivateNetworkEqual(a, b *lbSDK.PrivateNetwork) bool {
53+
if a == nil || b == nil {
54+
return a == b
55+
}
56+
if a.PrivateNetworkID != b.PrivateNetworkID {
57+
return false
58+
}
59+
if !reflect.DeepEqual(a.DHCPConfig, b.DHCPConfig) {
60+
return false
61+
}
62+
if !reflect.DeepEqual(a.StaticConfig, b.StaticConfig) {
63+
return false
7064
}
71-
return false
65+
66+
return true
7267
}
7368

74-
func privateNetworksCompare(slice1, slice2 []*lbSDK.PrivateNetwork) []*lbSDK.PrivateNetwork {
75-
var diff []*lbSDK.PrivateNetwork
69+
func PrivateNetworksCompare(oldPNs, newPNs []*lbSDK.PrivateNetwork) ([]*lbSDK.PrivateNetwork, []*lbSDK.PrivateNetwork) {
70+
var toDetach, toAttach []*lbSDK.PrivateNetwork
7671

77-
m := make(map[string]struct{}, len(slice1))
78-
for _, pn := range slice1 {
79-
m[pn.PrivateNetworkID] = struct{}{}
72+
oldPNMap := make(map[string]*lbSDK.PrivateNetwork, len(oldPNs))
73+
for _, pn := range oldPNs {
74+
oldPNMap[pn.PrivateNetworkID] = pn
8075
}
81-
// find the differences
82-
for _, pn := range slice2 {
83-
if _, foundID := m[pn.PrivateNetworkID]; !foundID || (foundID && !IsPrivateNetworkEqual(slice1, slice2)) {
84-
diff = append(diff, pn)
76+
77+
newPNMap := make(map[string]*lbSDK.PrivateNetwork, len(newPNs))
78+
for _, pn := range newPNs {
79+
newPNMap[pn.PrivateNetworkID] = pn
80+
}
81+
82+
for id, oldPN := range oldPNMap {
83+
newPN, found := newPNMap[id]
84+
if !found {
85+
toDetach = append(toDetach, oldPN)
86+
} else if !IsPrivateNetworkEqual(oldPN, newPN) {
87+
toDetach = append(toDetach, oldPN)
88+
toAttach = append(toAttach, newPN)
8589
}
8690
}
87-
return diff
91+
92+
for id, newPN := range newPNMap {
93+
if _, found := oldPNMap[id]; !found {
94+
toAttach = append(toAttach, newPN)
95+
}
96+
}
97+
98+
return toDetach, toAttach
8899
}
89100

90101
func lbUpgradeV1SchemaType() cty.Type {

‎internal/services/lb/helpers_lb_test.go‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,3 +53,109 @@ func TestIsEqualPrivateNetwork(t *testing.T) {
5353
})
5454
}
5555
}
56+
57+
func TestPrivateNetworksCompare(t *testing.T) {
58+
tests := []struct {
59+
name string
60+
oldPNs []*lbSDK.PrivateNetwork
61+
newPNs []*lbSDK.PrivateNetwork
62+
expectedToDetach []*lbSDK.PrivateNetwork
63+
expectedToAttach []*lbSDK.PrivateNetwork
64+
}{
65+
{
66+
name: "no changes",
67+
oldPNs: []*lbSDK.PrivateNetwork{
68+
{PrivateNetworkID: "pn1", StaticConfig: &lbSDK.PrivateNetworkStaticConfig{IPAddress: scw.StringsPtr([]string{"192.168.1.1"})}},
69+
{PrivateNetworkID: "pn2", DHCPConfig: &lbSDK.PrivateNetworkDHCPConfig{}},
70+
},
71+
newPNs: []*lbSDK.PrivateNetwork{
72+
{PrivateNetworkID: "pn1", StaticConfig: &lbSDK.PrivateNetworkStaticConfig{IPAddress: scw.StringsPtr([]string{"192.168.1.1"})}},
73+
{PrivateNetworkID: "pn2", DHCPConfig: &lbSDK.PrivateNetworkDHCPConfig{}},
74+
},
75+
expectedToDetach: []*lbSDK.PrivateNetwork(nil),
76+
expectedToAttach: []*lbSDK.PrivateNetwork(nil),
77+
},
78+
{
79+
name: "private network removed",
80+
oldPNs: []*lbSDK.PrivateNetwork{
81+
{PrivateNetworkID: "pn1", StaticConfig: &lbSDK.PrivateNetworkStaticConfig{IPAddress: scw.StringsPtr([]string{"192.168.1.1"})}},
82+
{PrivateNetworkID: "pn2", DHCPConfig: &lbSDK.PrivateNetworkDHCPConfig{}},
83+
},
84+
newPNs: []*lbSDK.PrivateNetwork{
85+
{PrivateNetworkID: "pn1", StaticConfig: &lbSDK.PrivateNetworkStaticConfig{IPAddress: scw.StringsPtr([]string{"192.168.1.1"})}},
86+
},
87+
expectedToDetach: []*lbSDK.PrivateNetwork{
88+
{PrivateNetworkID: "pn2", DHCPConfig: &lbSDK.PrivateNetworkDHCPConfig{}},
89+
},
90+
expectedToAttach: []*lbSDK.PrivateNetwork(nil),
91+
},
92+
{
93+
name: "private network added",
94+
oldPNs: []*lbSDK.PrivateNetwork{
95+
{PrivateNetworkID: "pn1", StaticConfig: &lbSDK.PrivateNetworkStaticConfig{IPAddress: scw.StringsPtr([]string{"192.168.1.1"})}},
96+
},
97+
newPNs: []*lbSDK.PrivateNetwork{
98+
{PrivateNetworkID: "pn1", StaticConfig: &lbSDK.PrivateNetworkStaticConfig{IPAddress: scw.StringsPtr([]string{"192.168.1.1"})}},
99+
{PrivateNetworkID: "pn2", DHCPConfig: &lbSDK.PrivateNetworkDHCPConfig{}},
100+
},
101+
expectedToDetach: []*lbSDK.PrivateNetwork(nil),
102+
expectedToAttach: []*lbSDK.PrivateNetwork{
103+
{PrivateNetworkID: "pn2", DHCPConfig: &lbSDK.PrivateNetworkDHCPConfig{}},
104+
},
105+
},
106+
{
107+
name: "private network static configuration changed",
108+
oldPNs: []*lbSDK.PrivateNetwork{
109+
{PrivateNetworkID: "pn1", StaticConfig: &lbSDK.PrivateNetworkStaticConfig{IPAddress: scw.StringsPtr([]string{"192.168.1.1"})}},
110+
},
111+
newPNs: []*lbSDK.PrivateNetwork{
112+
{PrivateNetworkID: "pn1", StaticConfig: &lbSDK.PrivateNetworkStaticConfig{IPAddress: scw.StringsPtr([]string{"192.168.1.2"})}},
113+
},
114+
expectedToDetach: []*lbSDK.PrivateNetwork{
115+
{PrivateNetworkID: "pn1", StaticConfig: &lbSDK.PrivateNetworkStaticConfig{IPAddress: scw.StringsPtr([]string{"192.168.1.1"})}},
116+
},
117+
expectedToAttach: []*lbSDK.PrivateNetwork{
118+
{PrivateNetworkID: "pn1", StaticConfig: &lbSDK.PrivateNetworkStaticConfig{IPAddress: scw.StringsPtr([]string{"192.168.1.2"})}},
119+
},
120+
},
121+
{
122+
name: "private network configuration changed from static to DHCP",
123+
oldPNs: []*lbSDK.PrivateNetwork{
124+
{PrivateNetworkID: "pn1", StaticConfig: &lbSDK.PrivateNetworkStaticConfig{IPAddress: scw.StringsPtr([]string{"192.168.1.1"})}},
125+
},
126+
newPNs: []*lbSDK.PrivateNetwork{
127+
{PrivateNetworkID: "pn1", DHCPConfig: &lbSDK.PrivateNetworkDHCPConfig{}},
128+
},
129+
expectedToDetach: []*lbSDK.PrivateNetwork{
130+
{PrivateNetworkID: "pn1", StaticConfig: &lbSDK.PrivateNetworkStaticConfig{IPAddress: scw.StringsPtr([]string{"192.168.1.1"})}},
131+
},
132+
expectedToAttach: []*lbSDK.PrivateNetwork{
133+
{PrivateNetworkID: "pn1", DHCPConfig: &lbSDK.PrivateNetworkDHCPConfig{}},
134+
},
135+
},
136+
{
137+
name: "multiple private networks removed",
138+
oldPNs: []*lbSDK.PrivateNetwork{
139+
{PrivateNetworkID: "pn1", StaticConfig: &lbSDK.PrivateNetworkStaticConfig{IPAddress: scw.StringsPtr([]string{"192.168.1.1"})}},
140+
{PrivateNetworkID: "pn2", DHCPConfig: &lbSDK.PrivateNetworkDHCPConfig{}},
141+
{PrivateNetworkID: "pn3", StaticConfig: &lbSDK.PrivateNetworkStaticConfig{IPAddress: scw.StringsPtr([]string{"192.168.1.3"})}},
142+
},
143+
newPNs: []*lbSDK.PrivateNetwork{
144+
{PrivateNetworkID: "pn1", StaticConfig: &lbSDK.PrivateNetworkStaticConfig{IPAddress: scw.StringsPtr([]string{"192.168.1.1"})}},
145+
},
146+
expectedToDetach: []*lbSDK.PrivateNetwork{
147+
{PrivateNetworkID: "pn2", DHCPConfig: &lbSDK.PrivateNetworkDHCPConfig{}},
148+
{PrivateNetworkID: "pn3", StaticConfig: &lbSDK.PrivateNetworkStaticConfig{IPAddress: scw.StringsPtr([]string{"192.168.1.3"})}},
149+
},
150+
expectedToAttach: []*lbSDK.PrivateNetwork(nil),
151+
},
152+
}
153+
154+
for _, tt := range tests {
155+
t.Run(tt.name, func(t *testing.T) {
156+
toDetach, toAttach := lb.PrivateNetworksCompare(tt.oldPNs, tt.newPNs)
157+
assert.ElementsMatch(t, tt.expectedToDetach, toDetach)
158+
assert.ElementsMatch(t, tt.expectedToAttach, toAttach)
159+
})
160+
}
161+
}

‎internal/services/lb/lb.go‎

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -465,45 +465,47 @@ func resourceLbUpdate(ctx context.Context, d *schema.ResourceData, m interface{}
465465
}
466466

467467
// check that pns are in a stable state
468-
pns, err := waitForPrivateNetworks(ctx, lbAPI, zone, ID, d.Timeout(schema.TimeoutUpdate))
468+
_, err := waitForPrivateNetworks(ctx, lbAPI, zone, ID, d.Timeout(schema.TimeoutUpdate))
469469
if err != nil && !httperrors.Is404(err) {
470470
return diag.FromErr(err)
471471
}
472472

473-
pnConfigs, err := expandPrivateNetworks(d.Get("private_network"))
473+
oldPNs, newPNs := d.GetChange("private_network")
474+
oldPNConfigs, err := expandPrivateNetworks(oldPNs)
475+
if err != nil {
476+
return diag.FromErr(err)
477+
}
478+
newPNConfigs, err := expandPrivateNetworks(newPNs)
474479
if err != nil {
475480
return diag.FromErr(err)
476481
}
477-
// select only private networks that have changed
478-
pnToDetach := privateNetworksCompare(pnConfigs, pns)
482+
483+
toDetach, toAttach := PrivateNetworksCompare(oldPNConfigs, newPNConfigs)
479484

480485
// detach private networks
481-
for i := range pnToDetach {
486+
for _, pn := range toDetach {
482487
err = lbAPI.DetachPrivateNetwork(&lbSDK.ZonedAPIDetachPrivateNetworkRequest{
483488
Zone: zone,
484489
LBID: ID,
485-
PrivateNetworkID: pnToDetach[i].PrivateNetworkID,
490+
PrivateNetworkID: pn.PrivateNetworkID,
486491
}, scw.WithContext(ctx))
487492
if err != nil && !httperrors.Is404(err) {
488493
return diag.FromErr(err)
489494
}
490495
}
491496

492-
// check load balancer state
493497
_, err = waitForLB(ctx, lbAPI, zone, ID, d.Timeout(schema.TimeoutUpdate))
494498
if err != nil && !httperrors.Is404(err) {
495499
return diag.FromErr(err)
496500
}
497501

498-
// check that pns are in a stable state
499-
pns, err = waitForPrivateNetworks(ctx, lbAPI, zone, ID, d.Timeout(schema.TimeoutUpdate))
502+
_, err = waitForPrivateNetworks(ctx, lbAPI, zone, ID, d.Timeout(schema.TimeoutUpdate))
500503
if err != nil && !httperrors.Is404(err) {
501504
return diag.FromErr(err)
502505
}
503506

504-
pnToAttach := privateNetworksCompare(pns, pnConfigs)
505-
// attach new/updated private networks
506-
_, err = attachLBPrivateNetworks(ctx, lbAPI, zone, pnToAttach, ID, d.Timeout(schema.TimeoutUpdate))
507+
// attach private networks
508+
_, err = attachLBPrivateNetworks(ctx, lbAPI, zone, toAttach, ID, d.Timeout(schema.TimeoutUpdate))
507509
if err != nil {
508510
return diag.FromErr(err)
509511
}

0 commit comments

Comments
 (0)