Skip to content

Commit 54e2d36

Browse files
piyush-tiwaripiyush-tiwari
committed
preserve defined tags on LB when defined-tag related annotations are absent
1 parent 82a3aeb commit 54e2d36

File tree

5 files changed

+55
-28
lines changed

5 files changed

+55
-28
lines changed

‎pkg/controllers/ingressclass/ingressclass.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -290,7 +290,7 @@ func (c *Controller) createLoadBalancer(ctx context.Context, ic *networkingv1.In
290290
icp *v1beta1.IngressClassParameters) (*ociloadbalancer.LoadBalancer, error) {
291291
klog.V(2).InfoS("Creating load balancer for ingress class", "ingressClass", ic.Name)
292292
compartmentId := common.String(util.GetIngressClassCompartmentId(icp, c.defaultCompartmentId))
293-
definedTags, err := util.GetIngressClassDefinedTags(ic)
293+
_, definedTags, err := util.GetIngressClassDefinedTags(ic)
294294
if err != nil {
295295
return nil, err
296296
}
@@ -387,7 +387,7 @@ func (c *Controller) checkForIngressClassParameterUpdates(ctx context.Context, i
387387

388388
// check LoadBalancerName, Defined and Freeform tags
389389
displayName := util.GetIngressClassLoadBalancerName(ic, icp)
390-
implicitDefaultTags, err := util.GetIngressClassImplicitDefaultTags(ic)
390+
defaultTagsAnnotationPresent, implicitDefaultTags, err := util.GetIngressClassImplicitDefaultTags(ic)
391391
if err != nil {
392392
return err
393393
}
@@ -409,7 +409,7 @@ func (c *Controller) checkForIngressClassParameterUpdates(ctx context.Context, i
409409
}
410410
}
411411

412-
if !isDefinedTagsEqual(implicitDefaultTags, updatedImplicitDefaultTags) {
412+
if !defaultTagsAnnotationPresent || !isDefinedTagsEqual(implicitDefaultTags, updatedImplicitDefaultTags) {
413413
klog.Infof("Updating implicit default tags %+v in LB %s for IC %s", updatedImplicitDefaultTags, *lb.Id, ic.Name)
414414
err = updateImplicitDefaultTagsAnnotation(wrapperClient.GetK8Client(), ic, updatedImplicitDefaultTags)
415415
if err != nil {

‎pkg/controllers/ingressclass/util.go‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,21 +49,36 @@ func getImplicitDefaultTagsForNewLoadBalancer(actualDefinedTags, suppliedDefined
4949
return defaultTags
5050
}
5151

52+
// Return values - (new tags content for LB, new default tags annotation content, error)
5253
func getUpdatedDefinedAndImplicitDefaultTags(actualTags util.DefinedTagsType,
5354
ic *networkingv1.IngressClass) (util.DefinedTagsType, util.DefinedTagsType, error) {
55+
// Updated value for LB's actual Defined Tags
5456
updatedDefinedTags := util.DefinedTagsType{}
57+
// Updated value to be placed into the util.IngressClassImplicitDefaultTagsAnnotation annotation
5558
updatedDefaultTags := util.DefinedTagsType{}
5659

57-
definedTags, err := util.GetIngressClassDefinedTags(ic)
60+
// Defined Tags supplied by customer
61+
definedTagsAnnotationPresent, definedTags, err := util.GetIngressClassDefinedTags(ic)
5862
if err != nil {
5963
return nil, nil, err
6064
}
6165

62-
defaultTags, err := util.GetIngressClassImplicitDefaultTags(ic)
66+
// Currently stored default tags on the IngressClass
67+
defaultTagsAnnotationPresent, defaultTags, err := util.GetIngressClassImplicitDefaultTags(ic)
6368
if err != nil {
6469
return nil, nil, err
6570
}
6671

72+
// If no defined tag annotations are present, we process this as if the LoadBalancer just got created
73+
// This can happen in two cases usually
74+
// (a) migrating from an NIC version that did not have tagging support
75+
// (b) filling id annotation on the IngressClass on creation to import a pre-existing load balancer, but not filling the defined-tags annotation
76+
// We fill the implicit-default-tag annotation with the current tags on LB and preserve them
77+
// Here, definedTags and defaultTags both will be empty
78+
if !definedTagsAnnotationPresent && !defaultTagsAnnotationPresent {
79+
return actualTags, actualTags, nil
80+
}
81+
6782
klog.Infof("Calculating defined/default tags where actualTags: %+v, suppliedDefinedTags: %+v, implicitDefaultTags: %+v",
6883
actualTags, definedTags, defaultTags)
6984

‎pkg/controllers/ingressclass/util_test.go‎

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ func TestIsDefinedTagsEqual(t *testing.T) {
3434
func TestGetImplicitDefaultTagsForNewLoadBalancer(t *testing.T) {
3535
RegisterTestingT(t)
3636

37-
actualDefinedTags := util.DefinedTagsType{"n1": {"k1": "v1", "KI1": "vi1"}, "n2": {"k2": "V2", "K3": "v3"}, "n3": {"k4": "v4"}}
37+
actualDefinedTags := util.DefinedTagsType{"n1": {"k1": "newValue", "KI1": "vi1"}, "n2": {"k2": "V2", "K3": "v3"}, "n3": {"k4": "v4"}}
3838
suppliedDefinedTags := util.DefinedTagsType{"N1": {"k1": "v1"}, "n2": {"K2": "V2", "k3": "v3"}}
3939
expectedImplicitDefaultTags := util.DefinedTagsType{"n1": {"KI1": "vi1"}, "n3": {"k4": "v4"}}
4040

@@ -44,22 +44,29 @@ func TestGetImplicitDefaultTagsForNewLoadBalancer(t *testing.T) {
4444
func TestGetUpdatedDefinedAndImplicitDefaultTags(t *testing.T) {
4545
RegisterTestingT(t)
4646

47-
actualTags := util.DefinedTagsType{"n1": {"k1": "v1", "KI1": "vi1"}, "n2": {"K2": "V2", "k3": "v3"}, "n3": {"K4": "v5"}}
48-
4947
ingressClass := &networkingv1.IngressClass{
5048
ObjectMeta: v1.ObjectMeta{
51-
Annotations: map[string]string{
52-
util.IngressClassDefinedTagsAnnotation: `{"n1": {"k1": "v1"}, "N2": {"k2": "v3", "K3": "V3"}}`,
53-
util.IngressClassImplicitDefaultTagsAnnotation: `{"n1": {"KI1": "vi1"}, "n2": {"K2": "V2"}, "n3": {"k4": "V4"}}`,
54-
},
49+
Annotations: map[string]string{},
5550
},
5651
}
5752

58-
expectedDefinedTags := util.DefinedTagsType{"n1": {"k1": "v1", "ki1": "vi1"}, "n2": {"k2": "v3", "k3": "V3"}, "n3": {"k4": "v5"}}
59-
expectedDefaultTags := util.DefinedTagsType{"n1": {"KI1": "vi1"}, "n3": {"K4": "v5"}}
53+
actualTags := util.DefinedTagsType{"n1": {"k1": "newValue", "KI1": "vi1", "deletedKey": "val"},
54+
"n2": {"K2": "V2", "k3": "v3"}, "n3": {"K4": "v5"}}
6055

56+
// If both DefinedTags and DefaultTags annotations are missing, should behave like a new LoadBalancer
6157
definedTags, defaultTags, err := getUpdatedDefinedAndImplicitDefaultTags(actualTags, ingressClass)
6258
Expect(err).To(BeNil())
59+
Expect(defaultTags).Should(Equal(actualTags))
60+
Expect(definedTags).Should(Equal(actualTags))
61+
62+
ingressClass.Annotations[util.IngressClassDefinedTagsAnnotation] = `{"n1": {"k1": "has ${oci.datetime}"}, "N2": {"k2": "v3", "K3": "V3", "K5": "name-${iam.principal.name}"}}`
63+
ingressClass.Annotations[util.IngressClassImplicitDefaultTagsAnnotation] = `{"n1": {"KI1": "vi1"}, "n2": {"K2": "V2"}, "n3": {"k4": "V4"}}`
64+
65+
expectedDefinedTags := util.DefinedTagsType{"n1": {"k1": "newValue", "ki1": "vi1"}, "n2": {"k2": "v3", "k3": "V3", "k5": "name-${iam.principal.name}"}, "n3": {"k4": "v5"}}
66+
expectedDefaultTags := util.DefinedTagsType{"n1": {"KI1": "vi1"}, "n3": {"K4": "v5"}}
67+
68+
definedTags, defaultTags, err = getUpdatedDefinedAndImplicitDefaultTags(actualTags, ingressClass)
69+
Expect(err).To(BeNil())
6370
Expect(expectedDefinedTags).Should(Equal(definedTags))
6471
Expect(expectedDefaultTags).Should(Equal(defaultTags))
6572
}

‎pkg/util/util.go‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -215,7 +215,9 @@ func GetIngressClassDeleteProtectionEnabled(ic *networkingv1.IngressClass) bool
215215
return result
216216
}
217217

218-
func GetIngressClassDefinedTags(ic *networkingv1.IngressClass) (DefinedTagsType, error) {
218+
// GetIngressClassDefinedTags gets the content of IngressClassDefinedTagsAnnotation in DefinedTagsType type.
219+
// First return value is true if the annotation is present on the IngressClass.
220+
func GetIngressClassDefinedTags(ic *networkingv1.IngressClass) (bool, DefinedTagsType, error) {
219221
annotation := IngressClassDefinedTagsAnnotation
220222
value, ok := ic.Annotations[annotation]
221223

@@ -224,33 +226,35 @@ func GetIngressClassDefinedTags(ic *networkingv1.IngressClass) (DefinedTagsType,
224226
definedTags := DefinedTagsType{}
225227

226228
if !ok || strings.TrimSpace(value) == "" {
227-
return definedTags, nil
229+
return ok, definedTags, nil
228230
}
229231

230232
err := json.Unmarshal([]byte(value), &definedTags)
231233
if err != nil {
232-
return nil, fmt.Errorf("error parsing value %s for annotation %s: %w", value, annotation, err)
234+
return ok, nil, fmt.Errorf("error parsing value %s for annotation %s: %w", value, annotation, err)
233235
}
234236

235-
return definedTags, nil
237+
return ok, definedTags, nil
236238
}
237239

238-
func GetIngressClassImplicitDefaultTags(ic *networkingv1.IngressClass) (DefinedTagsType, error) {
240+
// GetIngressClassImplicitDefaultTags gets the content of IngressClassImplicitDefaultTagsAnnotation in DefinedTagsType type.
241+
// First return value is true if the annotation is present on the IngressClass.
242+
func GetIngressClassImplicitDefaultTags(ic *networkingv1.IngressClass) (bool, DefinedTagsType, error) {
239243
annotation := IngressClassImplicitDefaultTagsAnnotation
240244
value, ok := ic.Annotations[annotation]
241245

242246
defaultTags := DefinedTagsType{}
243247

244248
if !ok || strings.TrimSpace(value) == "" {
245-
return defaultTags, nil
249+
return ok, defaultTags, nil
246250
}
247251

248252
err := json.Unmarshal([]byte(value), &defaultTags)
249253
if err != nil {
250-
return nil, fmt.Errorf("error parsing value %s for annotation %s: %w", value, annotation, err)
254+
return ok, nil, fmt.Errorf("error parsing value %s for annotation %s: %w", value, annotation, err)
251255
}
252256

253-
return defaultTags, nil
257+
return ok, defaultTags, nil
254258
}
255259

256260
func GetIngressClassFreeformTags(ic *networkingv1.IngressClass) (map[string]string, error) {

‎pkg/util/util_test.go‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -232,17 +232,18 @@ func TestGetIngressClassDefinedTags(t *testing.T) {
232232
ingressClassWithSampleTags := getIngressClassWithDefinedTagsAnnotation(sampleTags)
233233
ingressClassWithFaultyTags := getIngressClassWithDefinedTagsAnnotation(faultyTags)
234234

235-
verifyGetIngressClassDefinedTags := func(ic *networkingv1.IngressClass, shouldError bool,
235+
verifyGetIngressClassDefinedTags := func(ic *networkingv1.IngressClass, shouldBePresent bool, shouldError bool,
236236
expectedResult map[string]map[string]interface{}) {
237-
res, err := GetIngressClassDefinedTags(ic)
237+
present, res, err := GetIngressClassDefinedTags(ic)
238+
Expect(present).To(Equal(shouldBePresent))
238239
Expect(err != nil).To(Equal(shouldError))
239240
Expect(res).To(Equal(expectedResult))
240241
}
241242

242-
verifyGetIngressClassDefinedTags(ingressClassWithNoAnnotation, false, emptyTagsResult)
243-
verifyGetIngressClassDefinedTags(ingressClassWithEmptyTags, false, emptyTagsResult)
244-
verifyGetIngressClassDefinedTags(ingressClassWithSampleTags, false, sampleTagsResult)
245-
verifyGetIngressClassDefinedTags(ingressClassWithFaultyTags, true, nil)
243+
verifyGetIngressClassDefinedTags(ingressClassWithNoAnnotation, false, false, emptyTagsResult)
244+
verifyGetIngressClassDefinedTags(ingressClassWithEmptyTags, true, false, emptyTagsResult)
245+
verifyGetIngressClassDefinedTags(ingressClassWithSampleTags, true, false, sampleTagsResult)
246+
verifyGetIngressClassDefinedTags(ingressClassWithFaultyTags, true, true, nil)
246247
}
247248

248249
func TestGetIngressClassFreeformTags(t *testing.T) {

0 commit comments

Comments
 (0)