Skip to content

Commit e944078

Browse files
sapphirewsapphirew
committed
fix: Address review comments from michaelhtm
- Replace inlined cross-namespace warning + condition logic with calls to the new ackrt.HandleCrossNamespaceReference runtime helper. This collapses ~14 lines of generated code per cross-namespace check into a single helper call. Applies to both resource references (resource_reference.go) and secret references (set_sdk.go). - Remove the per-package setCrossNamespaceCondition function from the sdk.go.tpl template; the runtime helper now owns the lookup-or-create pattern, avoiding 26 lines of generated code per resource. - Drop the now-unused ackrtlog import and var-block from the references.go.tpl template since the runtime helper handles logging. - Remove the empty {{ if .CRD.HasReferenceFields -}}{{ end -}} block at the bottom of references.go.tpl that was left over from the previous refactor. - Restore the third sample output (resolving nested lists of structs containing references) in the ResolveReferencesForField doc comment, updated to reflect the new generated output. - Update set_sdk_test.go and resource_reference_test.go expected outputs to match the simplified generated code. Requires runtime helpers added in aws-controllers-k8s/runtime PR ( HandleCrossNamespaceReference, SetCrossNamespaceOptInRequired, CrossNamespaceRefKindResource, CrossNamespaceRefKindSecret).
1 parent 30bdd70 commit e944078

6 files changed

Lines changed: 106 additions & 232 deletions

File tree

‎pkg/generate/code/resource_reference.go‎

Lines changed: 39 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,8 @@ func ReferenceFieldsValidation(
125125
// The generated code calls ackrt.ValidateCrossNamespaceReference to check
126126
// whether the reference targets a different namespace. If the cross-namespace
127127
// flag is disabled, the helper returns an error. If enabled and cross-namespace,
128-
// a warning is logged and a condition is set on the resource.
128+
// ackrt.HandleCrossNamespaceReference logs a deprecation warning and sets the
129+
// ACK.CrossNamespaceOptInRequired condition on the resource.
129130
//
130131
// Sample output (resolving a singular reference):
131132
//
@@ -145,8 +146,11 @@ func ReferenceFieldsValidation(
145146
// return hasReferences, err
146147
// }
147148
// if isCrossNs {
148-
// ackrtlog.FromContext(ctx).Info("cross-namespace resource reference detected; ...")
149-
// setCrossNamespaceCondition(ko, crossNsMsg)
149+
// ko.Status.Conditions = ackrt.HandleCrossNamespaceReference(
150+
// ctx, ko.Status.Conditions,
151+
// ackrt.CrossNamespaceRefKindResource,
152+
// ko.ObjectMeta.GetNamespace(), *arr.Namespace, *arr.Name,
153+
// )
150154
// }
151155
// obj := &svcapitypes.API{}
152156
// if err := getReferencedResourceState_API(ctx, apiReader, obj, *arr.Name, namespace); err != nil {
@@ -184,6 +188,34 @@ func ReferenceFieldsValidation(
184188
// ko.Spec.SecurityGroupIDs = append(ko.Spec.SecurityGroupIDs, (*string)(obj.Status.ID))
185189
// }
186190
// }
191+
//
192+
// Sample output (resolving nested lists of structs containing references):
193+
//
194+
// if ko.Spec.Notification != nil {
195+
// for f0idx, f0iter := range ko.Spec.Notification.LambdaFunctionConfigurations {
196+
// if f0iter.Filter != nil {
197+
// if f0iter.Filter.Key != nil {
198+
// for f1idx, f1iter := range f0iter.Filter.Key.FilterRules {
199+
// if f1iter.ValueRef != nil && f1iter.ValueRef.From != nil {
200+
// hasReferences = true
201+
// arr := f1iter.ValueRef.From
202+
// if arr.Name == nil || *arr.Name == "" {
203+
// return hasReferences, fmt.Errorf("provided resource reference is nil or empty: Notification.LambdaFunctionConfigurations.Filter.Key.FilterRules.ValueRef")
204+
// }
205+
// namespace, isCrossNs, err := ackrt.ValidateCrossNamespaceReference( ... )
206+
// if err != nil { return hasReferences, err }
207+
// if isCrossNs { ... }
208+
// obj := &svcapitypes.Bucket{}
209+
// if err := getReferencedResourceState_Bucket(ctx, apiReader, obj, *arr.Name, namespace); err != nil {
210+
// return hasReferences, err
211+
// }
212+
// ko.Spec.Notification.LambdaFunctionConfigurations[f0idx].Filter.Key.FilterRules[f1idx].Value = (*string)(obj.Spec.Name)
213+
// }
214+
// }
215+
// }
216+
// }
217+
// }
218+
// }
187219
func ResolveReferencesForField(field *model.Field, sourceVarName string, indentLevel int) (string, error) {
188220
isListOfRefs := field.ShapeRef.Shape.Type == "list"
189221

@@ -241,19 +273,11 @@ func ResolveReferencesForField(field *model.Field, sourceVarName string, indentL
241273
outPrefix += fmt.Sprintf("%s\treturn hasReferences, err\n", innerIndent)
242274
outPrefix += fmt.Sprintf("%s}\n", innerIndent)
243275
outPrefix += fmt.Sprintf("%sif isCrossNs {\n", innerIndent)
244-
outPrefix += fmt.Sprintf("%s\tackrtlog.FromContext(ctx).Info(\"cross-namespace resource reference detected; \"+\n", innerIndent)
245-
outPrefix += fmt.Sprintf("%s\t\t\"this behavior will be disabled by default in a future release. \"+\n", innerIndent)
246-
outPrefix += fmt.Sprintf("%s\t\t\"Set --enable-cross-namespace to preserve this behavior.\",\n", innerIndent)
247-
outPrefix += fmt.Sprintf("%s\t\t\"ownerNamespace\", ko.ObjectMeta.GetNamespace(),\n", innerIndent)
248-
outPrefix += fmt.Sprintf("%s\t\t\"targetNamespace\", *arr.Namespace,\n", innerIndent)
249-
outPrefix += fmt.Sprintf("%s\t\t\"referenceName\", *arr.Name,\n", innerIndent)
276+
outPrefix += fmt.Sprintf("%s\tko.Status.Conditions = ackrt.HandleCrossNamespaceReference(\n", innerIndent)
277+
outPrefix += fmt.Sprintf("%s\t\tctx, ko.Status.Conditions,\n", innerIndent)
278+
outPrefix += fmt.Sprintf("%s\t\tackrt.CrossNamespaceRefKindResource,\n", innerIndent)
279+
outPrefix += fmt.Sprintf("%s\t\tko.ObjectMeta.GetNamespace(), *arr.Namespace, *arr.Name,\n", innerIndent)
250280
outPrefix += fmt.Sprintf("%s\t)\n", innerIndent)
251-
outPrefix += fmt.Sprintf("%s\tcrossNsMsg := fmt.Sprintf(\"Cross-namespace resource reference detected: \"+\n", innerIndent)
252-
outPrefix += fmt.Sprintf("%s\t\t\"resource in namespace %%q references %%q in namespace %%q. \"+\n", innerIndent)
253-
outPrefix += fmt.Sprintf("%s\t\t\"Cross-namespace behavior will be disabled by default in a future release. \"+\n", innerIndent)
254-
outPrefix += fmt.Sprintf("%s\t\t\"Set --enable-cross-namespace=true to preserve this behavior.\",\n", innerIndent)
255-
outPrefix += fmt.Sprintf("%s\t\tko.ObjectMeta.GetNamespace(), *arr.Name, *arr.Namespace)\n", innerIndent)
256-
outPrefix += fmt.Sprintf("%s\tsetCrossNamespaceCondition(ko, crossNsMsg)\n", innerIndent)
257281
outPrefix += fmt.Sprintf("%s}\n", innerIndent)
258282

259283
outPrefix += getReferencedStateForField(field, innerIndentLevel)

‎pkg/generate/code/resource_reference_test.go‎

Lines changed: 28 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -154,19 +154,11 @@ func Test_ResolveReferencesForField_SingleReference(t *testing.T) {
154154
return hasReferences, err
155155
}
156156
if isCrossNs {
157-
ackrtlog.FromContext(ctx).Info("cross-namespace resource reference detected; "+
158-
"this behavior will be disabled by default in a future release. "+
159-
"Set --enable-cross-namespace to preserve this behavior.",
160-
"ownerNamespace", ko.ObjectMeta.GetNamespace(),
161-
"targetNamespace", *arr.Namespace,
162-
"referenceName", *arr.Name,
157+
ko.Status.Conditions = ackrt.HandleCrossNamespaceReference(
158+
ctx, ko.Status.Conditions,
159+
ackrt.CrossNamespaceRefKindResource,
160+
ko.ObjectMeta.GetNamespace(), *arr.Namespace, *arr.Name,
163161
)
164-
crossNsMsg := fmt.Sprintf("Cross-namespace resource reference detected: "+
165-
"resource in namespace %q references %q in namespace %q. "+
166-
"Cross-namespace behavior will be disabled by default in a future release. "+
167-
"Set --enable-cross-namespace=true to preserve this behavior.",
168-
ko.ObjectMeta.GetNamespace(), *arr.Name, *arr.Namespace)
169-
setCrossNamespaceCondition(ko, crossNsMsg)
170162
}
171163
obj := &svcapitypes.API{}
172164
if err := getReferencedResourceState_API(ctx, apiReader, obj, *arr.Name, namespace); err != nil {
@@ -210,19 +202,11 @@ func Test_ResolveReferencesForField_ReferencingARN(t *testing.T) {
210202
return hasReferences, err
211203
}
212204
if isCrossNs {
213-
ackrtlog.FromContext(ctx).Info("cross-namespace resource reference detected; "+
214-
"this behavior will be disabled by default in a future release. "+
215-
"Set --enable-cross-namespace to preserve this behavior.",
216-
"ownerNamespace", ko.ObjectMeta.GetNamespace(),
217-
"targetNamespace", *arr.Namespace,
218-
"referenceName", *arr.Name,
205+
ko.Status.Conditions = ackrt.HandleCrossNamespaceReference(
206+
ctx, ko.Status.Conditions,
207+
ackrt.CrossNamespaceRefKindResource,
208+
ko.ObjectMeta.GetNamespace(), *arr.Namespace, *arr.Name,
219209
)
220-
crossNsMsg := fmt.Sprintf("Cross-namespace resource reference detected: "+
221-
"resource in namespace %q references %q in namespace %q. "+
222-
"Cross-namespace behavior will be disabled by default in a future release. "+
223-
"Set --enable-cross-namespace=true to preserve this behavior.",
224-
ko.ObjectMeta.GetNamespace(), *arr.Name, *arr.Namespace)
225-
setCrossNamespaceCondition(ko, crossNsMsg)
226210
}
227211
obj := &svcapitypes.Policy{}
228212
if err := getReferencedResourceState_Policy(ctx, apiReader, obj, *arr.Name, namespace); err != nil {
@@ -267,19 +251,11 @@ func Test_ResolveReferencesForField_SliceOfReferences(t *testing.T) {
267251
return hasReferences, err
268252
}
269253
if isCrossNs {
270-
ackrtlog.FromContext(ctx).Info("cross-namespace resource reference detected; "+
271-
"this behavior will be disabled by default in a future release. "+
272-
"Set --enable-cross-namespace to preserve this behavior.",
273-
"ownerNamespace", ko.ObjectMeta.GetNamespace(),
274-
"targetNamespace", *arr.Namespace,
275-
"referenceName", *arr.Name,
254+
ko.Status.Conditions = ackrt.HandleCrossNamespaceReference(
255+
ctx, ko.Status.Conditions,
256+
ackrt.CrossNamespaceRefKindResource,
257+
ko.ObjectMeta.GetNamespace(), *arr.Namespace, *arr.Name,
276258
)
277-
crossNsMsg := fmt.Sprintf("Cross-namespace resource reference detected: "+
278-
"resource in namespace %q references %q in namespace %q. "+
279-
"Cross-namespace behavior will be disabled by default in a future release. "+
280-
"Set --enable-cross-namespace=true to preserve this behavior.",
281-
ko.ObjectMeta.GetNamespace(), *arr.Name, *arr.Namespace)
282-
setCrossNamespaceCondition(ko, crossNsMsg)
283259
}
284260
obj := &ec2apitypes.SecurityGroup{}
285261
if err := getReferencedResourceState_SecurityGroup(ctx, apiReader, obj, *arr.Name, namespace); err != nil {
@@ -328,19 +304,11 @@ func Test_ResolveReferencesForField_NestedSingleReference(t *testing.T) {
328304
return hasReferences, err
329305
}
330306
if isCrossNs {
331-
ackrtlog.FromContext(ctx).Info("cross-namespace resource reference detected; "+
332-
"this behavior will be disabled by default in a future release. "+
333-
"Set --enable-cross-namespace to preserve this behavior.",
334-
"ownerNamespace", ko.ObjectMeta.GetNamespace(),
335-
"targetNamespace", *arr.Namespace,
336-
"referenceName", *arr.Name,
307+
ko.Status.Conditions = ackrt.HandleCrossNamespaceReference(
308+
ctx, ko.Status.Conditions,
309+
ackrt.CrossNamespaceRefKindResource,
310+
ko.ObjectMeta.GetNamespace(), *arr.Namespace, *arr.Name,
337311
)
338-
crossNsMsg := fmt.Sprintf("Cross-namespace resource reference detected: "+
339-
"resource in namespace %q references %q in namespace %q. "+
340-
"Cross-namespace behavior will be disabled by default in a future release. "+
341-
"Set --enable-cross-namespace=true to preserve this behavior.",
342-
ko.ObjectMeta.GetNamespace(), *arr.Name, *arr.Namespace)
343-
setCrossNamespaceCondition(ko, crossNsMsg)
344312
}
345313
obj := &svcapitypes.API{}
346314
if err := getReferencedResourceState_API(ctx, apiReader, obj, *arr.Name, namespace); err != nil {
@@ -389,19 +357,11 @@ func Test_ResolveReferencesForField_SingleReference_DeeplyNested(t *testing.T) {
389357
return hasReferences, err
390358
}
391359
if isCrossNs {
392-
ackrtlog.FromContext(ctx).Info("cross-namespace resource reference detected; "+
393-
"this behavior will be disabled by default in a future release. "+
394-
"Set --enable-cross-namespace to preserve this behavior.",
395-
"ownerNamespace", ko.ObjectMeta.GetNamespace(),
396-
"targetNamespace", *arr.Namespace,
397-
"referenceName", *arr.Name,
360+
ko.Status.Conditions = ackrt.HandleCrossNamespaceReference(
361+
ctx, ko.Status.Conditions,
362+
ackrt.CrossNamespaceRefKindResource,
363+
ko.ObjectMeta.GetNamespace(), *arr.Namespace, *arr.Name,
398364
)
399-
crossNsMsg := fmt.Sprintf("Cross-namespace resource reference detected: "+
400-
"resource in namespace %q references %q in namespace %q. "+
401-
"Cross-namespace behavior will be disabled by default in a future release. "+
402-
"Set --enable-cross-namespace=true to preserve this behavior.",
403-
ko.ObjectMeta.GetNamespace(), *arr.Name, *arr.Namespace)
404-
setCrossNamespaceCondition(ko, crossNsMsg)
405365
}
406366
obj := &svcapitypes.Bucket{}
407367
if err := getReferencedResourceState_Bucket(ctx, apiReader, obj, *arr.Name, namespace); err != nil {
@@ -450,19 +410,11 @@ func Test_ResolveReferencesForField_SingleReference_WithinSlice(t *testing.T) {
450410
return hasReferences, err
451411
}
452412
if isCrossNs {
453-
ackrtlog.FromContext(ctx).Info("cross-namespace resource reference detected; "+
454-
"this behavior will be disabled by default in a future release. "+
455-
"Set --enable-cross-namespace to preserve this behavior.",
456-
"ownerNamespace", ko.ObjectMeta.GetNamespace(),
457-
"targetNamespace", *arr.Namespace,
458-
"referenceName", *arr.Name,
413+
ko.Status.Conditions = ackrt.HandleCrossNamespaceReference(
414+
ctx, ko.Status.Conditions,
415+
ackrt.CrossNamespaceRefKindResource,
416+
ko.ObjectMeta.GetNamespace(), *arr.Namespace, *arr.Name,
459417
)
460-
crossNsMsg := fmt.Sprintf("Cross-namespace resource reference detected: "+
461-
"resource in namespace %q references %q in namespace %q. "+
462-
"Cross-namespace behavior will be disabled by default in a future release. "+
463-
"Set --enable-cross-namespace=true to preserve this behavior.",
464-
ko.ObjectMeta.GetNamespace(), *arr.Name, *arr.Namespace)
465-
setCrossNamespaceCondition(ko, crossNsMsg)
466418
}
467419
obj := &svcapitypes.InternetGateway{}
468420
if err := getReferencedResourceState_InternetGateway(ctx, apiReader, obj, *arr.Name, namespace); err != nil {
@@ -514,19 +466,11 @@ func Test_ResolveReferencesForField_SingleReference_WithinMultipleSlices(t *test
514466
return hasReferences, err
515467
}
516468
if isCrossNs {
517-
ackrtlog.FromContext(ctx).Info("cross-namespace resource reference detected; "+
518-
"this behavior will be disabled by default in a future release. "+
519-
"Set --enable-cross-namespace to preserve this behavior.",
520-
"ownerNamespace", ko.ObjectMeta.GetNamespace(),
521-
"targetNamespace", *arr.Namespace,
522-
"referenceName", *arr.Name,
469+
ko.Status.Conditions = ackrt.HandleCrossNamespaceReference(
470+
ctx, ko.Status.Conditions,
471+
ackrt.CrossNamespaceRefKindResource,
472+
ko.ObjectMeta.GetNamespace(), *arr.Namespace, *arr.Name,
523473
)
524-
crossNsMsg := fmt.Sprintf("Cross-namespace resource reference detected: "+
525-
"resource in namespace %q references %q in namespace %q. "+
526-
"Cross-namespace behavior will be disabled by default in a future release. "+
527-
"Set --enable-cross-namespace=true to preserve this behavior.",
528-
ko.ObjectMeta.GetNamespace(), *arr.Name, *arr.Namespace)
529-
setCrossNamespaceCondition(ko, crossNsMsg)
530474
}
531475
obj := &svcapitypes.Bucket{}
532476
if err := getReferencedResourceState_Bucket(ctx, apiReader, obj, *arr.Name, namespace); err != nil {

‎pkg/generate/code/set_sdk.go‎

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1114,7 +1114,13 @@ func setSDKForContainer(
11141114
// return nil, err
11151115
// }
11161116
// if isCrossNs {
1117-
// // log warning and set condition
1117+
// r.ko.Status.Conditions = ackrt.HandleCrossNamespaceReference(
1118+
// ctx, r.ko.Status.Conditions,
1119+
// ackrt.CrossNamespaceRefKindSecret,
1120+
// r.ko.ObjectMeta.GetNamespace(),
1121+
// ko.Spec.MasterUserPassword.Namespace,
1122+
// ko.Spec.MasterUserPassword.Name,
1123+
// )
11181124
// }
11191125
// ko.Spec.MasterUserPassword.Namespace = secretNamespace
11201126
// tmpSecret, err := rm.rr.SecretValueFromReference(ctx, ko.Spec.MasterUserPassword)
@@ -1164,19 +1170,11 @@ func setSDKForSecret(
11641170
out += fmt.Sprintf("%s\t\treturn nil, err\n", indent)
11651171
out += fmt.Sprintf("%s\t}\n", indent)
11661172
out += fmt.Sprintf("%s\tif isCrossNs {\n", indent)
1167-
out += fmt.Sprintf("%s\t\tackrtlog.FromContext(ctx).Info(\"cross-namespace secret reference detected; \"+\n", indent)
1168-
out += fmt.Sprintf("%s\t\t\t\"this behavior will be disabled by default in a future release. \"+\n", indent)
1169-
out += fmt.Sprintf("%s\t\t\t\"Set --enable-cross-namespace to preserve this behavior.\",\n", indent)
1170-
out += fmt.Sprintf("%s\t\t\t\"ownerNamespace\", r.ko.ObjectMeta.GetNamespace(),\n", indent)
1171-
out += fmt.Sprintf("%s\t\t\t\"secretNamespace\", %s.Namespace,\n", indent, sourceVarName)
1172-
out += fmt.Sprintf("%s\t\t\t\"secretName\", %s.Name,\n", indent, sourceVarName)
1173+
out += fmt.Sprintf("%s\t\tr.ko.Status.Conditions = ackrt.HandleCrossNamespaceReference(\n", indent)
1174+
out += fmt.Sprintf("%s\t\t\tctx, r.ko.Status.Conditions,\n", indent)
1175+
out += fmt.Sprintf("%s\t\t\tackrt.CrossNamespaceRefKindSecret,\n", indent)
1176+
out += fmt.Sprintf("%s\t\t\tr.ko.ObjectMeta.GetNamespace(), %s.Namespace, %s.Name,\n", indent, sourceVarName, sourceVarName)
11731177
out += fmt.Sprintf("%s\t\t)\n", indent)
1174-
out += fmt.Sprintf("%s\t\tcrossNsMsg := fmt.Sprintf(\"Cross-namespace secret reference detected: \"+\n", indent)
1175-
out += fmt.Sprintf("%s\t\t\t\"resource in namespace %%q references secret %%q in namespace %%q. \"+\n", indent)
1176-
out += fmt.Sprintf("%s\t\t\t\"Cross-namespace behavior will be disabled by default in a future release. \"+\n", indent)
1177-
out += fmt.Sprintf("%s\t\t\t\"Set --enable-cross-namespace=true to preserve this behavior.\",\n", indent)
1178-
out += fmt.Sprintf("%s\t\t\tr.ko.ObjectMeta.GetNamespace(), %s.Name, %s.Namespace)\n", indent, sourceVarName, sourceVarName)
1179-
out += fmt.Sprintf("%s\t\tsetCrossNamespaceCondition(r.ko, crossNsMsg)\n", indent)
11801178
out += fmt.Sprintf("%s\t}\n", indent)
11811179
// Override the secret reference namespace with the validated namespace
11821180
out += fmt.Sprintf("%s\t%s.Namespace = secretNamespace\n", indent, sourceVarName)

0 commit comments

Comments
 (0)