Skip to content

Commit 77c3c71

Browse files
sapphirewsapphirew
committed
fix: Use combined ResolveCrossNamespaceReference helper
Address review feedback from michaelhtm (aws-controllers-k8s#699) to fold the entire validate + handle flow into a single helper call. The generated code no longer needs to inspect or branch on isCrossNs. - resource_reference.go: replace ValidateCrossNamespaceReference + Handle block with single ResolveCrossNamespaceReference call (3 returns -> 2) - set_sdk.go: same simplification for secret references using ResolveCrossNamespaceReferenceString - Update doc comments in both generators to reflect the new output - Update 7 test expectations in resource_reference_test.go and 7 in set_sdk_test.go Requires runtime helpers added in aws-controllers-k8s/runtime#249 (ResolveCrossNamespaceReference, ResolveCrossNamespaceReferenceString).
1 parent e944078 commit 77c3c71

4 files changed

Lines changed: 91 additions & 166 deletions

File tree

‎pkg/generate/code/resource_reference.go‎

Lines changed: 18 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -122,11 +122,11 @@ func ReferenceFieldsValidation(
122122
// are related to the given concrete field, determining whether its in a valid
123123
// condition and updating the concrete field with the referenced value.
124124
//
125-
// The generated code calls ackrt.ValidateCrossNamespaceReference to check
126-
// whether the reference targets a different namespace. If the cross-namespace
127-
// flag is disabled, the helper returns an error. If enabled and cross-namespace,
128-
// ackrt.HandleCrossNamespaceReference logs a deprecation warning and sets the
129-
// ACK.CrossNamespaceOptInRequired condition on the resource.
125+
// The generated code calls ackrt.ResolveCrossNamespaceReference to validate
126+
// the reference and, when the reference targets a different namespace and
127+
// the cross-namespace flag is enabled, emit a deprecation warning and set
128+
// the ACK.CrossNamespaceOptInRequired condition on the resource. When the
129+
// flag is disabled, the helper returns a terminal error.
130130
//
131131
// Sample output (resolving a singular reference):
132132
//
@@ -136,22 +136,18 @@ func ReferenceFieldsValidation(
136136
// if arr.Name == nil || *arr.Name == "" {
137137
// return hasReferences, fmt.Errorf("provided resource reference is nil or empty: APIRef")
138138
// }
139-
// namespace, isCrossNs, err := ackrt.ValidateCrossNamespaceReference(
139+
// namespace, err := ackrt.ResolveCrossNamespaceReference(
140+
// ctx,
140141
// rm.cfg.EnableCrossNamespace,
142+
// &ko.Status.Conditions,
143+
// ackrt.CrossNamespaceRefKindResource,
141144
// ko.ObjectMeta.GetNamespace(),
142145
// arr.Namespace,
143146
// *arr.Name,
144147
// )
145148
// if err != nil {
146149
// return hasReferences, err
147150
// }
148-
// if isCrossNs {
149-
// ko.Status.Conditions = ackrt.HandleCrossNamespaceReference(
150-
// ctx, ko.Status.Conditions,
151-
// ackrt.CrossNamespaceRefKindResource,
152-
// ko.ObjectMeta.GetNamespace(), *arr.Namespace, *arr.Name,
153-
// )
154-
// }
155151
// obj := &svcapitypes.API{}
156152
// if err := getReferencedResourceState_API(ctx, apiReader, obj, *arr.Name, namespace); err != nil {
157153
// return hasReferences, err
@@ -168,16 +164,10 @@ func ReferenceFieldsValidation(
168164
// if arr.Name == nil || *arr.Name == "" {
169165
// return hasReferences, fmt.Errorf("provided resource reference is nil or empty: SecurityGroupRefs")
170166
// }
171-
// namespace, isCrossNs, err := ackrt.ValidateCrossNamespaceReference(
172-
// rm.cfg.EnableCrossNamespace,
173-
// ko.ObjectMeta.GetNamespace(),
174-
// arr.Namespace,
175-
// *arr.Name,
176-
// )
167+
// namespace, err := ackrt.ResolveCrossNamespaceReference( ... )
177168
// if err != nil {
178169
// return hasReferences, err
179170
// }
180-
// if isCrossNs { ... }
181171
// obj := &ec2apitypes.SecurityGroup{}
182172
// if err := getReferencedResourceState_SecurityGroup(ctx, apiReader, obj, *arr.Name, namespace); err != nil {
183173
// return hasReferences, err
@@ -202,9 +192,10 @@ func ReferenceFieldsValidation(
202192
// if arr.Name == nil || *arr.Name == "" {
203193
// return hasReferences, fmt.Errorf("provided resource reference is nil or empty: Notification.LambdaFunctionConfigurations.Filter.Key.FilterRules.ValueRef")
204194
// }
205-
// namespace, isCrossNs, err := ackrt.ValidateCrossNamespaceReference( ... )
206-
// if err != nil { return hasReferences, err }
207-
// if isCrossNs { ... }
195+
// namespace, err := ackrt.ResolveCrossNamespaceReference( ... )
196+
// if err != nil {
197+
// return hasReferences, err
198+
// }
208199
// obj := &svcapitypes.Bucket{}
209200
// if err := getReferencedResourceState_Bucket(ctx, apiReader, obj, *arr.Name, namespace); err != nil {
210201
// return hasReferences, err
@@ -263,22 +254,18 @@ func ResolveReferencesForField(field *model.Field, sourceVarName string, indentL
263254
outPrefix += fmt.Sprintf("%s\treturn hasReferences, fmt.Errorf(\"provided resource reference is nil or empty: %s\")\n", innerIndent, refFieldPath)
264255
outPrefix += fmt.Sprintf("%s}\n", innerIndent)
265256

266-
outPrefix += fmt.Sprintf("%snamespace, isCrossNs, err := ackrt.ValidateCrossNamespaceReference(\n", innerIndent)
257+
outPrefix += fmt.Sprintf("%snamespace, err := ackrt.ResolveCrossNamespaceReference(\n", innerIndent)
258+
outPrefix += fmt.Sprintf("%s\tctx,\n", innerIndent)
267259
outPrefix += fmt.Sprintf("%s\trm.cfg.EnableCrossNamespace,\n", innerIndent)
260+
outPrefix += fmt.Sprintf("%s\t&ko.Status.Conditions,\n", innerIndent)
261+
outPrefix += fmt.Sprintf("%s\tackrt.CrossNamespaceRefKindResource,\n", innerIndent)
268262
outPrefix += fmt.Sprintf("%s\tko.ObjectMeta.GetNamespace(),\n", innerIndent)
269263
outPrefix += fmt.Sprintf("%s\tarr.Namespace,\n", innerIndent)
270264
outPrefix += fmt.Sprintf("%s\t*arr.Name,\n", innerIndent)
271265
outPrefix += fmt.Sprintf("%s)\n", innerIndent)
272266
outPrefix += fmt.Sprintf("%sif err != nil {\n", innerIndent)
273267
outPrefix += fmt.Sprintf("%s\treturn hasReferences, err\n", innerIndent)
274268
outPrefix += fmt.Sprintf("%s}\n", innerIndent)
275-
outPrefix += fmt.Sprintf("%sif isCrossNs {\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)
280-
outPrefix += fmt.Sprintf("%s\t)\n", innerIndent)
281-
outPrefix += fmt.Sprintf("%s}\n", innerIndent)
282269

283270
outPrefix += getReferencedStateForField(field, innerIndentLevel)
284271

‎pkg/generate/code/resource_reference_test.go‎

Lines changed: 28 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -144,22 +144,18 @@ func Test_ResolveReferencesForField_SingleReference(t *testing.T) {
144144
if arr.Name == nil || *arr.Name == "" {
145145
return hasReferences, fmt.Errorf("provided resource reference is nil or empty: APIRef")
146146
}
147-
namespace, isCrossNs, err := ackrt.ValidateCrossNamespaceReference(
147+
namespace, err := ackrt.ResolveCrossNamespaceReference(
148+
ctx,
148149
rm.cfg.EnableCrossNamespace,
150+
&ko.Status.Conditions,
151+
ackrt.CrossNamespaceRefKindResource,
149152
ko.ObjectMeta.GetNamespace(),
150153
arr.Namespace,
151154
*arr.Name,
152155
)
153156
if err != nil {
154157
return hasReferences, err
155158
}
156-
if isCrossNs {
157-
ko.Status.Conditions = ackrt.HandleCrossNamespaceReference(
158-
ctx, ko.Status.Conditions,
159-
ackrt.CrossNamespaceRefKindResource,
160-
ko.ObjectMeta.GetNamespace(), *arr.Namespace, *arr.Name,
161-
)
162-
}
163159
obj := &svcapitypes.API{}
164160
if err := getReferencedResourceState_API(ctx, apiReader, obj, *arr.Name, namespace); err != nil {
165161
return hasReferences, err
@@ -192,22 +188,18 @@ func Test_ResolveReferencesForField_ReferencingARN(t *testing.T) {
192188
if arr.Name == nil || *arr.Name == "" {
193189
return hasReferences, fmt.Errorf("provided resource reference is nil or empty: PermissionsBoundaryRef")
194190
}
195-
namespace, isCrossNs, err := ackrt.ValidateCrossNamespaceReference(
191+
namespace, err := ackrt.ResolveCrossNamespaceReference(
192+
ctx,
196193
rm.cfg.EnableCrossNamespace,
194+
&ko.Status.Conditions,
195+
ackrt.CrossNamespaceRefKindResource,
197196
ko.ObjectMeta.GetNamespace(),
198197
arr.Namespace,
199198
*arr.Name,
200199
)
201200
if err != nil {
202201
return hasReferences, err
203202
}
204-
if isCrossNs {
205-
ko.Status.Conditions = ackrt.HandleCrossNamespaceReference(
206-
ctx, ko.Status.Conditions,
207-
ackrt.CrossNamespaceRefKindResource,
208-
ko.ObjectMeta.GetNamespace(), *arr.Namespace, *arr.Name,
209-
)
210-
}
211203
obj := &svcapitypes.Policy{}
212204
if err := getReferencedResourceState_Policy(ctx, apiReader, obj, *arr.Name, namespace); err != nil {
213205
return hasReferences, err
@@ -241,22 +233,18 @@ func Test_ResolveReferencesForField_SliceOfReferences(t *testing.T) {
241233
if arr.Name == nil || *arr.Name == "" {
242234
return hasReferences, fmt.Errorf("provided resource reference is nil or empty: SecurityGroupRefs")
243235
}
244-
namespace, isCrossNs, err := ackrt.ValidateCrossNamespaceReference(
236+
namespace, err := ackrt.ResolveCrossNamespaceReference(
237+
ctx,
245238
rm.cfg.EnableCrossNamespace,
239+
&ko.Status.Conditions,
240+
ackrt.CrossNamespaceRefKindResource,
246241
ko.ObjectMeta.GetNamespace(),
247242
arr.Namespace,
248243
*arr.Name,
249244
)
250245
if err != nil {
251246
return hasReferences, err
252247
}
253-
if isCrossNs {
254-
ko.Status.Conditions = ackrt.HandleCrossNamespaceReference(
255-
ctx, ko.Status.Conditions,
256-
ackrt.CrossNamespaceRefKindResource,
257-
ko.ObjectMeta.GetNamespace(), *arr.Namespace, *arr.Name,
258-
)
259-
}
260248
obj := &ec2apitypes.SecurityGroup{}
261249
if err := getReferencedResourceState_SecurityGroup(ctx, apiReader, obj, *arr.Name, namespace); err != nil {
262250
return hasReferences, err
@@ -294,22 +282,18 @@ func Test_ResolveReferencesForField_NestedSingleReference(t *testing.T) {
294282
if arr.Name == nil || *arr.Name == "" {
295283
return hasReferences, fmt.Errorf("provided resource reference is nil or empty: JWTConfiguration.IssuerRef")
296284
}
297-
namespace, isCrossNs, err := ackrt.ValidateCrossNamespaceReference(
285+
namespace, err := ackrt.ResolveCrossNamespaceReference(
286+
ctx,
298287
rm.cfg.EnableCrossNamespace,
288+
&ko.Status.Conditions,
289+
ackrt.CrossNamespaceRefKindResource,
299290
ko.ObjectMeta.GetNamespace(),
300291
arr.Namespace,
301292
*arr.Name,
302293
)
303294
if err != nil {
304295
return hasReferences, err
305296
}
306-
if isCrossNs {
307-
ko.Status.Conditions = ackrt.HandleCrossNamespaceReference(
308-
ctx, ko.Status.Conditions,
309-
ackrt.CrossNamespaceRefKindResource,
310-
ko.ObjectMeta.GetNamespace(), *arr.Namespace, *arr.Name,
311-
)
312-
}
313297
obj := &svcapitypes.API{}
314298
if err := getReferencedResourceState_API(ctx, apiReader, obj, *arr.Name, namespace); err != nil {
315299
return hasReferences, err
@@ -347,22 +331,18 @@ func Test_ResolveReferencesForField_SingleReference_DeeplyNested(t *testing.T) {
347331
if arr.Name == nil || *arr.Name == "" {
348332
return hasReferences, fmt.Errorf("provided resource reference is nil or empty: Logging.LoggingEnabled.TargetBucketRef")
349333
}
350-
namespace, isCrossNs, err := ackrt.ValidateCrossNamespaceReference(
334+
namespace, err := ackrt.ResolveCrossNamespaceReference(
335+
ctx,
351336
rm.cfg.EnableCrossNamespace,
337+
&ko.Status.Conditions,
338+
ackrt.CrossNamespaceRefKindResource,
352339
ko.ObjectMeta.GetNamespace(),
353340
arr.Namespace,
354341
*arr.Name,
355342
)
356343
if err != nil {
357344
return hasReferences, err
358345
}
359-
if isCrossNs {
360-
ko.Status.Conditions = ackrt.HandleCrossNamespaceReference(
361-
ctx, ko.Status.Conditions,
362-
ackrt.CrossNamespaceRefKindResource,
363-
ko.ObjectMeta.GetNamespace(), *arr.Namespace, *arr.Name,
364-
)
365-
}
366346
obj := &svcapitypes.Bucket{}
367347
if err := getReferencedResourceState_Bucket(ctx, apiReader, obj, *arr.Name, namespace); err != nil {
368348
return hasReferences, err
@@ -400,22 +380,18 @@ func Test_ResolveReferencesForField_SingleReference_WithinSlice(t *testing.T) {
400380
if arr.Name == nil || *arr.Name == "" {
401381
return hasReferences, fmt.Errorf("provided resource reference is nil or empty: Routes.GatewayRef")
402382
}
403-
namespace, isCrossNs, err := ackrt.ValidateCrossNamespaceReference(
383+
namespace, err := ackrt.ResolveCrossNamespaceReference(
384+
ctx,
404385
rm.cfg.EnableCrossNamespace,
386+
&ko.Status.Conditions,
387+
ackrt.CrossNamespaceRefKindResource,
405388
ko.ObjectMeta.GetNamespace(),
406389
arr.Namespace,
407390
*arr.Name,
408391
)
409392
if err != nil {
410393
return hasReferences, err
411394
}
412-
if isCrossNs {
413-
ko.Status.Conditions = ackrt.HandleCrossNamespaceReference(
414-
ctx, ko.Status.Conditions,
415-
ackrt.CrossNamespaceRefKindResource,
416-
ko.ObjectMeta.GetNamespace(), *arr.Namespace, *arr.Name,
417-
)
418-
}
419395
obj := &svcapitypes.InternetGateway{}
420396
if err := getReferencedResourceState_InternetGateway(ctx, apiReader, obj, *arr.Name, namespace); err != nil {
421397
return hasReferences, err
@@ -456,22 +432,18 @@ func Test_ResolveReferencesForField_SingleReference_WithinMultipleSlices(t *test
456432
if arr.Name == nil || *arr.Name == "" {
457433
return hasReferences, fmt.Errorf("provided resource reference is nil or empty: Notification.LambdaFunctionConfigurations.Filter.Key.FilterRules.ValueRef")
458434
}
459-
namespace, isCrossNs, err := ackrt.ValidateCrossNamespaceReference(
435+
namespace, err := ackrt.ResolveCrossNamespaceReference(
436+
ctx,
460437
rm.cfg.EnableCrossNamespace,
438+
&ko.Status.Conditions,
439+
ackrt.CrossNamespaceRefKindResource,
461440
ko.ObjectMeta.GetNamespace(),
462441
arr.Namespace,
463442
*arr.Name,
464443
)
465444
if err != nil {
466445
return hasReferences, err
467446
}
468-
if isCrossNs {
469-
ko.Status.Conditions = ackrt.HandleCrossNamespaceReference(
470-
ctx, ko.Status.Conditions,
471-
ackrt.CrossNamespaceRefKindResource,
472-
ko.ObjectMeta.GetNamespace(), *arr.Namespace, *arr.Name,
473-
)
474-
}
475447
obj := &svcapitypes.Bucket{}
476448
if err := getReferencedResourceState_Bucket(ctx, apiReader, obj, *arr.Name, namespace); err != nil {
477449
return hasReferences, err

‎pkg/generate/code/set_sdk.go‎

Lines changed: 17 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1099,29 +1099,24 @@ func setSDKForContainer(
10991099

11001100
// setSDKForSecret returns a string of Go code that sets a target variable to
11011101
// the value of a Secret when the type of the source variable is a
1102-
// SecretKeyReference. It first validates cross-namespace access using
1103-
// ValidateCrossNamespaceReferenceString, then fetches the secret value.
1102+
// SecretKeyReference. It first calls ResolveCrossNamespaceReferenceString to
1103+
// validate cross-namespace access (and emit a deprecation warning + condition
1104+
// when the reference is cross-namespace), then fetches the secret value.
11041105
//
11051106
// The Go code output from this function looks like this:
11061107
//
1107-
// secretNamespace, isCrossNs, err := ackrt.ValidateCrossNamespaceReferenceString(
1108+
// secretNamespace, err := ackrt.ResolveCrossNamespaceReferenceString(
1109+
// ctx,
11081110
// rm.cfg.EnableCrossNamespace,
1111+
// &r.ko.Status.Conditions,
1112+
// ackrt.CrossNamespaceRefKindSecret,
11091113
// r.ko.ObjectMeta.GetNamespace(),
11101114
// ko.Spec.MasterUserPassword.Namespace,
11111115
// ko.Spec.MasterUserPassword.Name,
11121116
// )
11131117
// if err != nil {
11141118
// return nil, err
11151119
// }
1116-
// if isCrossNs {
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-
// )
1124-
// }
11251120
// ko.Spec.MasterUserPassword.Namespace = secretNamespace
11261121
// tmpSecret, err := rm.rr.SecretValueFromReference(ctx, ko.Spec.MasterUserPassword)
11271122
// if err != nil {
@@ -1150,33 +1145,32 @@ func setSDKForSecret(
11501145
indent := strings.Repeat("\t", indentLevel)
11511146
secVar := "tmpSecret"
11521147

1153-
// Validate cross-namespace access before fetching the secret
1154-
// secretNamespace, isCrossNs, err := ackrt.ValidateCrossNamespaceReferenceString(
1148+
// Resolve cross-namespace access before fetching the secret
1149+
// secretNamespace, err := ackrt.ResolveCrossNamespaceReferenceString(
1150+
// ctx,
11551151
// rm.cfg.EnableCrossNamespace,
1152+
// &r.ko.Status.Conditions,
1153+
// ackrt.CrossNamespaceRefKindSecret,
11561154
// r.ko.ObjectMeta.GetNamespace(),
11571155
// sourceVarName.Namespace,
11581156
// sourceVarName.Name,
11591157
// )
11601158
out += fmt.Sprintf(
1161-
"%s\tsecretNamespace, isCrossNs, err := ackrt.ValidateCrossNamespaceReferenceString(\n",
1159+
"%s\tsecretNamespace, err := ackrt.ResolveCrossNamespaceReferenceString(\n",
11621160
indent,
11631161
)
1162+
out += fmt.Sprintf("%s\t\tctx,\n", indent)
11641163
out += fmt.Sprintf("%s\t\trm.cfg.EnableCrossNamespace,\n", indent)
1164+
out += fmt.Sprintf("%s\t\t&r.ko.Status.Conditions,\n", indent)
1165+
out += fmt.Sprintf("%s\t\tackrt.CrossNamespaceRefKindSecret,\n", indent)
11651166
out += fmt.Sprintf("%s\t\tr.ko.ObjectMeta.GetNamespace(),\n", indent)
11661167
out += fmt.Sprintf("%s\t\t%s.Namespace,\n", indent, sourceVarName)
11671168
out += fmt.Sprintf("%s\t\t%s.Name,\n", indent, sourceVarName)
11681169
out += fmt.Sprintf("%s\t)\n", indent)
11691170
out += fmt.Sprintf("%s\tif err != nil {\n", indent)
11701171
out += fmt.Sprintf("%s\t\treturn nil, err\n", indent)
11711172
out += fmt.Sprintf("%s\t}\n", indent)
1172-
out += fmt.Sprintf("%s\tif isCrossNs {\n", indent)
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)
1177-
out += fmt.Sprintf("%s\t\t)\n", indent)
1178-
out += fmt.Sprintf("%s\t}\n", indent)
1179-
// Override the secret reference namespace with the validated namespace
1173+
// Override the secret reference namespace with the resolved namespace
11801174
out += fmt.Sprintf("%s\t%s.Namespace = secretNamespace\n", indent, sourceVarName)
11811175

11821176
// tmpSecret, err := rm.rr.SecretValueFromReference(ctx, ko.Spec.MasterUserPassword)

0 commit comments

Comments
 (0)