Skip to content

Commit 38e877f

Browse files
pbenasGouthamML
pbenas
authored and
GouthamML
committed
Load balancer OCID cache comparment check
Check if compartment OCID matches when returning a cached (network) load balancer name. If the user requests the LB to be moved to a different compartment, a cache hit would prevent the reprovisioning from happening. This restores the original behavior.
1 parent b052a8e commit 38e877f

File tree

4 files changed

+176
-30
lines changed

4 files changed

+176
-30
lines changed

‎pkg/oci/client/load_balancer.go‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -106,16 +106,24 @@ func (c *loadbalancerClientStruct) GetLoadBalancerByName(ctx context.Context, co
106106

107107
if ocid, ok := c.nameToOcid.Load(name); ok {
108108
var err error
109+
var lb *GenericLoadBalancer
110+
109111
ocidStr, ok := ocid.(string)
110112
if ok {
111-
lb, err := c.GetLoadBalancer(ctx, ocidStr)
112-
if err == nil && *lb.DisplayName == name {
113-
return lb, err
113+
lb, err = c.GetLoadBalancer(ctx, ocidStr)
114+
if err == nil && lb.DisplayName != nil && lb.CompartmentId != nil {
115+
if *lb.DisplayName == name && *lb.CompartmentId == compartmentID {
116+
return lb, err
117+
}
118+
logger.Info("LB name to OCID cache stale record. Actual display name: %s, compartment %s", *lb.DisplayName, *lb.CompartmentId)
119+
} else {
120+
logger.Info("LB name to OCID cache failed to get LB or the response contained unexpected nil value")
114121
}
115122
}
116123

117-
if !ok || IsNotFound(err) { // Only remove the cached value on 404, not on a 5XX
124+
if IsNotFound(err) { // Only remove the cached value on 404, not on a 5XX
118125
c.nameToOcid.Delete(name)
126+
logger.Info("LB name to OCID cache deleted record")
119127
}
120128
} else {
121129
logger.Info("LB name to OCID cache miss")

‎pkg/oci/client/load_balancer_test.go‎

Lines changed: 130 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -100,10 +100,123 @@ func TestLB_AwaitWorkRequest(t *testing.T) {
100100
}
101101
}
102102

103+
var (
104+
fakeLbCompartment = "ocid1.compartment.totally.fake"
105+
fakeLbOcid1 = "ocid.lb.fake1"
106+
fakeLbName1 = "fake display name 1"
107+
fakeLbOcid2 = "ocid.lb.fake2"
108+
fakeLbName2 = "fake display name 2"
109+
fakeLbSubnetOcid = "ocid.subnet.fake"
110+
111+
LBMap = map[string]loadbalancer.LoadBalancer{
112+
fakeLbOcid1: loadbalancer.LoadBalancer{
113+
Id: &fakeLbOcid1,
114+
CompartmentId: &fakeLbCompartment,
115+
DisplayName: &fakeLbName1,
116+
SubnetIds: []string{fakeLbSubnetOcid},
117+
},
118+
fakeLbOcid2: loadbalancer.LoadBalancer{
119+
Id: &fakeLbOcid2,
120+
CompartmentId: &fakeLbCompartment,
121+
DisplayName: &fakeLbName2,
122+
SubnetIds: []string{fakeLbSubnetOcid},
123+
},
124+
}
125+
)
126+
127+
func TestGetLoadBalancerByName(t *testing.T) {
128+
var totalListCalls int
129+
var lbClient = NewLBClient(
130+
&MockLoadBalancerClient{debug: true, listCalls: &totalListCalls},
131+
common.RequestMetadata{},
132+
&RateLimiter{
133+
Reader: flowcontrol.NewFakeAlwaysRateLimiter(),
134+
Writer: flowcontrol.NewFakeAlwaysRateLimiter(),
135+
})
136+
137+
var tests = []struct {
138+
skip bool // set true to skip a test-case
139+
compartment, name, testname string
140+
want string
141+
wantErr error
142+
wantListCalls int
143+
}{
144+
{
145+
testname: "getLBFirstTime",
146+
compartment: fakeLbCompartment,
147+
name: fakeLbName1,
148+
want: fakeLbOcid1,
149+
wantErr: nil,
150+
wantListCalls: 1,
151+
},
152+
{
153+
testname: "getLBSecondTime",
154+
compartment: fakeLbCompartment,
155+
name: fakeLbName1,
156+
want: fakeLbOcid1,
157+
wantErr: nil,
158+
wantListCalls: 1, // totals, no new list should be performed
159+
},
160+
{
161+
testname: "getLBDifferentCompartment",
162+
compartment: "differentCompartment",
163+
name: fakeLbName1,
164+
want: fakeLbOcid1,
165+
wantErr: nil,
166+
wantListCalls: 2,
167+
},
168+
{
169+
testname: "getLBCompartmentUpdated",
170+
compartment: "differentCompartment",
171+
name: fakeLbName1,
172+
want: fakeLbOcid1,
173+
wantErr: nil,
174+
wantListCalls: 2,
175+
},
176+
{
177+
testname: "getSecondLBFirstTime",
178+
compartment: fakeLbCompartment,
179+
name: fakeLbName2,
180+
want: fakeLbOcid2,
181+
wantErr: nil,
182+
wantListCalls: 3,
183+
},
184+
{
185+
testname: "getSecondLBSecondTime",
186+
compartment: fakeLbCompartment,
187+
name: fakeLbName2,
188+
want: fakeLbOcid2,
189+
wantErr: nil,
190+
wantListCalls: 4,
191+
},
192+
}
193+
194+
for _, tt := range tests {
195+
if tt.skip {
196+
continue
197+
}
198+
199+
t.Run(tt.testname, func(t *testing.T) {
200+
log.Println("running test ", tt.testname)
201+
got, err := lbClient.GetLoadBalancerByName(context.Background(), tt.compartment, tt.name)
202+
if got == nil || *got.Id != tt.want {
203+
t.Errorf("Expected %v, but got %v", tt.want, got)
204+
}
205+
if !errors.Is(err, tt.wantErr) {
206+
t.Errorf("Expected error = %v, but got %v", tt.wantErr, err)
207+
}
208+
if totalListCalls != tt.wantListCalls {
209+
t.Errorf("Expected the total number of LB list calls %d, but got %d", tt.wantListCalls, totalListCalls)
210+
}
211+
})
212+
}
213+
}
214+
103215
// MockLoadBalancerClient mocks LoadBalancer client implementation.
104216
type MockLoadBalancerClient struct {
105-
counter int
106-
debug bool // set true to run tests with debug logs
217+
counter int
218+
debug bool // set true to run tests with debug logs
219+
listCalls *int // number of list operations performed
107220
}
108221

109222
type getLoadBalancerWorkRequestResponse struct {
@@ -176,12 +289,26 @@ func (c *MockLoadBalancerClient) GetWorkRequest(ctx context.Context, request loa
176289
}
177290

178291
func (c *MockLoadBalancerClient) GetLoadBalancer(ctx context.Context, request loadbalancer.GetLoadBalancerRequest) (response loadbalancer.GetLoadBalancerResponse, err error) {
179-
return
292+
if *request.LoadBalancerId == fakeLbOcid2 {
293+
return loadbalancer.GetLoadBalancerResponse{}, errors.New("not found")
294+
}
295+
296+
return loadbalancer.GetLoadBalancerResponse{LoadBalancer: LBMap[*request.LoadBalancerId]}, nil
180297
}
181298
func (c *MockLoadBalancerClient) ListWorkRequests(ctx context.Context, request loadbalancer.ListWorkRequestsRequest) (response loadbalancer.ListWorkRequestsResponse, err error) {
182299
return
183300
}
184301
func (c *MockLoadBalancerClient) ListLoadBalancers(ctx context.Context, request loadbalancer.ListLoadBalancersRequest) (response loadbalancer.ListLoadBalancersResponse, err error) {
302+
for _, lb := range LBMap {
303+
response.Items = append(response.Items, lb)
304+
}
305+
*c.listCalls += 1
306+
307+
if *c.listCalls == 2 {
308+
lb := LBMap[fakeLbOcid1]
309+
lb.CompartmentId = request.CompartmentId
310+
LBMap[fakeLbOcid1] = lb
311+
}
185312
return
186313
}
187314
func (c *MockLoadBalancerClient) CreateLoadBalancer(ctx context.Context, request loadbalancer.CreateLoadBalancerRequest) (response loadbalancer.CreateLoadBalancerResponse, err error) {

‎pkg/oci/client/network_load_balancer.go‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -79,16 +79,24 @@ func (c *networkLoadbalancer) GetLoadBalancerByName(ctx context.Context, compart
7979

8080
if ocid, ok := c.nameToOcid.Load(name); ok {
8181
var err error
82+
var lb *GenericLoadBalancer
83+
8284
ocidStr, ok := ocid.(string)
8385
if ok {
84-
lb, err := c.GetLoadBalancer(ctx, ocidStr)
85-
if err == nil && *lb.DisplayName == name {
86-
return lb, err
86+
lb, err = c.GetLoadBalancer(ctx, ocidStr)
87+
if err == nil && lb.DisplayName != nil && lb.CompartmentId != nil {
88+
if *lb.DisplayName == name && *lb.CompartmentId == compartmentID {
89+
return lb, err
90+
}
91+
logger.Info("NLB name to OCID cache stale record. Actual display name: %s, compartment %s", *lb.DisplayName, *lb.CompartmentId)
92+
} else {
93+
logger.Info("NLB name to OCID cache failed to get LB or the response contained unexpected nil value")
8794
}
8895
}
8996

90-
if !ok || IsNotFound(err) { // Only remove the cached value on 404, not on a 5XX
97+
if IsNotFound(err) { // Only remove the cached value on 404, not on a 5XX
9198
c.nameToOcid.Delete(name)
99+
logger.Info("LB name to OCID cache deleted record")
92100
}
93101
} else {
94102
logger.Info("NLB name to OCID cache miss")

‎pkg/oci/client/network_load_balancer_test.go‎

Lines changed: 22 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -100,27 +100,30 @@ func TestNLB_AwaitWorkRequest(t *testing.T) {
100100
}
101101

102102
var (
103-
fakeNlbOcid1 = "ocid.nlb.fake1"
104-
fakeNlbName1 = "fake display name 1"
105-
fakeNlbOcid2 = "ocid.nlb.fake2"
106-
fakeNlbName2 = "fake display name 2"
107-
fakeSubnetOcid = "ocid.subnet.fake"
103+
fakeNlbCompartment = "ocid1.compartment.totally.fake"
104+
fakeNlbOcid1 = "ocid.nlb.fake1"
105+
fakeNlbName1 = "fake display name 1"
106+
fakeNlbOcid2 = "ocid.nlb.fake2"
107+
fakeNlbName2 = "fake display name 2"
108+
fakeSubnetOcid = "ocid.subnet.fake"
108109

109110
NLBMap = map[string]networkloadbalancer.NetworkLoadBalancer{
110-
"ocid.nlb.fake1": networkloadbalancer.NetworkLoadBalancer{
111-
Id: &fakeNlbOcid1,
112-
DisplayName: &fakeNlbName1,
113-
SubnetId: &fakeSubnetOcid,
111+
fakeNlbOcid1: networkloadbalancer.NetworkLoadBalancer{
112+
Id: &fakeNlbOcid1,
113+
CompartmentId: &fakeNlbCompartment,
114+
DisplayName: &fakeNlbName1,
115+
SubnetId: &fakeSubnetOcid,
114116
},
115-
"ocid.nlb.fake2": networkloadbalancer.NetworkLoadBalancer{
116-
Id: &fakeNlbOcid2,
117-
DisplayName: &fakeNlbName2,
118-
SubnetId: &fakeSubnetOcid,
117+
fakeNlbOcid2: networkloadbalancer.NetworkLoadBalancer{
118+
Id: &fakeNlbOcid2,
119+
CompartmentId: &fakeNlbCompartment,
120+
DisplayName: &fakeNlbName2,
121+
SubnetId: &fakeSubnetOcid,
119122
},
120123
}
121124
)
122125

123-
func TestGetLoadBalancerByName(t *testing.T) {
126+
func TestGetNetworkLoadBalancerByName(t *testing.T) {
124127
var totalListCalls int
125128
var loadbalancer = NewNLBClient(
126129
&MockNetworkLoadBalancerClient{debug: true, listCalls: &totalListCalls},
@@ -139,39 +142,39 @@ func TestGetLoadBalancerByName(t *testing.T) {
139142
}{
140143
{
141144
testname: "getFirstNLBFirstTime",
142-
compartment: "ocid.compartment.fake",
145+
compartment: fakeNlbCompartment,
143146
name: fakeNlbName1,
144147
want: fakeNlbOcid1,
145148
wantErr: nil,
146149
wantListCalls: 1,
147150
},
148151
{
149152
testname: "getFirstNLBSecondTime",
150-
compartment: "ocid.compartment.fake",
153+
compartment: fakeNlbCompartment,
151154
name: fakeNlbName1,
152155
want: fakeNlbOcid1,
153156
wantErr: nil,
154157
wantListCalls: 1, // totals, no new list should be performed
155158
},
156159
{
157160
testname: "getSecondNLBTime",
158-
compartment: "ocid.compartment.fake",
161+
compartment: fakeNlbCompartment,
159162
name: fakeNlbName2,
160163
want: fakeNlbOcid2,
161164
wantErr: nil,
162165
wantListCalls: 2,
163166
},
164167
{
165168
testname: "getFirstNLBThirdTime",
166-
compartment: "ocid.compartment.fake",
169+
compartment: fakeNlbCompartment,
167170
name: fakeNlbName1,
168171
want: fakeNlbOcid1,
169172
wantErr: nil,
170173
wantListCalls: 2,
171174
},
172175
{
173176
testname: "getSecondNLBSecondTime",
174-
compartment: "ocid.compartment.fake",
177+
compartment: fakeNlbCompartment,
175178
name: fakeNlbName2,
176179
want: fakeNlbOcid2,
177180
wantErr: nil,

0 commit comments

Comments
 (0)