-
Notifications
You must be signed in to change notification settings - Fork 190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor reference resolution into Go code #401
Changes from all commits
60e6268
2a88d23
be6c652
798b8a5
52bdff9
2936bef
f9c485b
c314ce1
dde4df3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -98,7 +98,7 @@ func ReferenceFieldsValidation( | |
out += fmt.Sprintf("%sif %s.%s != nil"+ | ||
" && %s.%s != nil {\n", fIndent, pathVarPrefix, field.GetReferenceFieldName().Camel, pathVarPrefix, field.Names.Camel) | ||
out += fmt.Sprintf("%s\treturn "+ | ||
"ackerr.ResourceReferenceAndIDNotSupportedFor(\"%s\", \"%s\")\n", | ||
"ackerr.ResourceReferenceAndIDNotSupportedFor(%q, %q)\n", | ||
fIndent, field.Path, field.ReferenceFieldPath()) | ||
|
||
// Close out all the curly braces with proper indentation | ||
|
@@ -117,7 +117,7 @@ func ReferenceFieldsValidation( | |
" %s.%s == nil {\n", fIndent, pathVarPrefix, | ||
field.ReferenceFieldPath(), pathVarPrefix, field.Path) | ||
out += fmt.Sprintf("%s\treturn "+ | ||
"ackerr.ResourceReferenceOrIDRequiredFor(\"%s\", \"%s\")\n", | ||
"ackerr.ResourceReferenceOrIDRequiredFor(%q, %q)\n", | ||
fIndent, field.Path, field.ReferenceFieldPath()) | ||
out += fmt.Sprintf("%s}\n", fIndent) | ||
} | ||
|
@@ -185,6 +185,111 @@ func ReferenceFieldsPresent( | |
return iteratorsOut + returnOut | ||
} | ||
|
||
// ResolveReferencesForField produces Go code for accessing all references that | ||
// are related to the given concrete field, determining whether its in a valid | ||
// condition and updating the concrete field with the referenced value. | ||
// Sample code (resolving a nested singular reference): | ||
// | ||
// ``` | ||
// | ||
// if ko.Spec.JWTConfiguration != nil { | ||
// if ko.Spec.JWTConfiguration.IssuerRef != nil && ko.Spec.JWTConfiguration.IssuerRef.From != nil { | ||
// arr := ko.Spec.JWTConfiguration.IssuerRef.From | ||
// if arr == nil || arr.Name == nil || *arr.Name == "" { | ||
// return fmt.Errorf("provided resource reference is nil or empty: \"JWTConfiguration.IssuerRef"\") | ||
// } | ||
// obj := &svcapitypes.API{} | ||
// if err := getReferencedResourceState_API(ctx, apiReader, obj, *arr.Name, namespace); err != nil { | ||
// return err | ||
// } | ||
// ko.Spec.JWTConfiguration.Issuer = obj.Status.APIID | ||
// } | ||
// } | ||
// | ||
// ``` | ||
func ResolveReferencesForField(field *model.Field, sourceVarName string, indentLevel int) string { | ||
r := field.CRD | ||
fp := fieldpath.FromString(field.Path) | ||
|
||
outPrefix := "" | ||
outSuffix := "" | ||
|
||
fieldAccessPrefix := fmt.Sprintf("%s%s", sourceVarName, r.Config().PrefixConfig.SpecField) | ||
targetVarName := fmt.Sprintf("%s.%s", fieldAccessPrefix, field.Path) | ||
|
||
for idx := 0; idx < fp.Size(); idx++ { | ||
curFP := fp.CopyAt(idx).String() | ||
cur, ok := r.Fields[curFP] | ||
if !ok { | ||
panic(fmt.Sprintf("unable to find field with path %q. crd: %q", curFP, r.Kind)) | ||
} | ||
|
||
ref := cur.ShapeRef | ||
|
||
indent := strings.Repeat("\t", indentLevel+idx) | ||
|
||
switch ref.Shape.Type { | ||
case ("structure"): | ||
fieldAccessPrefix = fmt.Sprintf("%s.%s", fieldAccessPrefix, fp.At(idx)) | ||
|
||
outPrefix += fmt.Sprintf("%sif %s != nil {\n", indent, fieldAccessPrefix) | ||
outSuffix = fmt.Sprintf("%s}\n%s", indent, outSuffix) | ||
RedbackThomson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
case ("list"): | ||
if (fp.Size() - idx) > 1 { | ||
// TODO(nithomso): add support for structs nested within lists | ||
// The logic for structs nested within lists needs to not only | ||
// be added here, but also in a custom patching solution since | ||
// it isn't supported by `StrategicMergePatch` | ||
// see https://github.com/aws-controllers-k8s/community/issues/1291 | ||
panic(fmt.Errorf("references within lists inside lists aren't supported. crd: %q, path: %q", r.Kind, field.Path)) | ||
} | ||
fieldAccessPrefix = fmt.Sprintf("%s.%s", fieldAccessPrefix, fp.At(idx)) | ||
|
||
iterVarName := fmt.Sprintf("iter%d", idx) | ||
|
||
// base case for references in a list | ||
outPrefix += fmt.Sprintf("%s%s = %s{}\n", indent, targetVarName, field.GoType) | ||
outPrefix += fmt.Sprintf("%sfor _, %s := range %s {\n", indent, iterVarName, fieldAccessPrefix) | ||
|
||
fieldAccessPrefix = iterVarName | ||
outPrefix += fmt.Sprintf("%s\tarr := %s.From\n", indent, fieldAccessPrefix) | ||
outPrefix += fmt.Sprintf("%s\tif arr == nil || arr.Name == nil || *arr.Name == \"\" {\n", indent) | ||
outPrefix += fmt.Sprintf("%s\t\treturn fmt.Errorf(\"provided resource reference is nil or empty: \\%q\\\")\n", indent, field.ReferenceFieldPath()) | ||
outPrefix += fmt.Sprintf("%s\t}\n", indent) | ||
|
||
outPrefix += fmt.Sprintf("%s\tif err := getReferencedResourceState_%s(ctx, apiReader, obj, *arr.Name, namespace); err != nil {\n", indent, field.FieldConfig.References.Resource) | ||
outPrefix += fmt.Sprintf("%s\t\treturn err\n", indent) | ||
outPrefix += fmt.Sprintf("%s\t}\n", indent) | ||
outPrefix += fmt.Sprintf("%s\t%s = append(%s, obj.%s)\n", indent, targetVarName, targetVarName, field.FieldConfig.References.Path) | ||
outPrefix += fmt.Sprintf("%s}\n", indent) | ||
case ("map"): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't know parenthesis here are possible |
||
panic("references cannot be within a map") | ||
default: | ||
// base case for single references | ||
fieldAccessPrefix = fmt.Sprintf("%s.%s", fieldAccessPrefix, cur.GetReferenceFieldName().Camel) | ||
|
||
outPrefix += fmt.Sprintf("%sif %s != nil && %s.From != nil {\n", indent, fieldAccessPrefix, fieldAccessPrefix) | ||
outPrefix += fmt.Sprintf("%s\tarr := %s.From\n", indent, fieldAccessPrefix) | ||
outPrefix += fmt.Sprintf("%s\tif arr == nil || arr.Name == nil || *arr.Name == \"\" {\n", indent) | ||
outPrefix += fmt.Sprintf("%s\t\treturn fmt.Errorf(\"provided resource reference is nil or empty: \\%q\\\")\n", indent, field.ReferenceFieldPath()) | ||
outPrefix += fmt.Sprintf("%s\t}\n", indent) | ||
|
||
if field.FieldConfig.References.ServiceName == "" { | ||
outPrefix += fmt.Sprintf("%s\tobj := &svcapitypes.%s{}\n", indent, field.FieldConfig.References.Resource) | ||
} else { | ||
outPrefix += fmt.Sprintf("%s\tobj := &%sapitypes.%s{}\n", indent, field.ReferencedServiceName(), field.FieldConfig.References.Resource) | ||
} | ||
outPrefix += fmt.Sprintf("%s\tif err := getReferencedResourceState_%s(ctx, apiReader, obj, *arr.Name, namespace); err != nil {\n", indent, field.FieldConfig.References.Resource) | ||
outPrefix += fmt.Sprintf("%s\t\treturn err\n", indent) | ||
outPrefix += fmt.Sprintf("%s\t}\n", indent) | ||
outPrefix += fmt.Sprintf("%s\t%s = obj.%s\n", indent, targetVarName, field.FieldConfig.References.Path) | ||
outPrefix += fmt.Sprintf("%s}\n", indent) | ||
} | ||
} | ||
|
||
return outPrefix + outSuffix | ||
} | ||
|
||
func nestedStructNilCheck(path fieldpath.Path, fieldAccessPrefix string) string { | ||
out := "" | ||
fieldNamePrefix := "" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -198,3 +198,127 @@ if ko.Spec.Routes != nil { | |
return false || (ko.Spec.VPCRef != nil)` | ||
assert.Equal(expected, code.ReferenceFieldsPresent(crd, "ko")) | ||
} | ||
|
||
func Test_ResolveReferencesForField_SingleReference(t *testing.T) { | ||
assert := assert.New(t) | ||
require := require.New(t) | ||
|
||
g := testutil.NewModelForServiceWithOptions(t, "apigatewayv2", | ||
&testutil.TestingModelOptions{ | ||
GeneratorConfigFile: "generator-with-reference.yaml", | ||
}) | ||
|
||
crd := testutil.GetCRDByName(t, g, "Integration") | ||
require.NotNil(crd) | ||
expected := | ||
jljaco marked this conversation as resolved.
Show resolved
Hide resolved
|
||
` if ko.Spec.APIRef != nil && ko.Spec.APIRef.From != nil { | ||
arr := ko.Spec.APIRef.From | ||
if arr == nil || arr.Name == nil || *arr.Name == "" { | ||
return fmt.Errorf("provided resource reference is nil or empty: \"APIRef"\") | ||
} | ||
obj := &svcapitypes.API{} | ||
if err := getReferencedResourceState_API(ctx, apiReader, obj, *arr.Name, namespace); err != nil { | ||
return err | ||
} | ||
ko.Spec.APIID = obj.Status.APIID | ||
} | ||
` | ||
|
||
field := crd.Fields["APIID"] | ||
assert.Equal(expected, code.ResolveReferencesForField(field, "ko", 1)) | ||
} | ||
|
||
func Test_ResolveReferencesForField_SliceOfReferences(t *testing.T) { | ||
assert := assert.New(t) | ||
require := require.New(t) | ||
|
||
g := testutil.NewModelForServiceWithOptions(t, "apigatewayv2", | ||
&testutil.TestingModelOptions{ | ||
GeneratorConfigFile: "generator-with-reference.yaml", | ||
}) | ||
|
||
crd := testutil.GetCRDByName(t, g, "VpcLink") | ||
require.NotNil(crd) | ||
expected := | ||
jljaco marked this conversation as resolved.
Show resolved
Hide resolved
|
||
` ko.Spec.SecurityGroupIDs = []*string{} | ||
for _, iter0 := range ko.Spec.SecurityGroupIDs { | ||
arr := iter0.From | ||
if arr == nil || arr.Name == nil || *arr.Name == "" { | ||
return fmt.Errorf("provided resource reference is nil or empty: \"SecurityGroupRefs"\") | ||
} | ||
if err := getReferencedResourceState_SecurityGroup(ctx, apiReader, obj, *arr.Name, namespace); err != nil { | ||
return err | ||
} | ||
ko.Spec.SecurityGroupIDs = append(ko.Spec.SecurityGroupIDs, obj.Status.ID) | ||
} | ||
` | ||
|
||
field := crd.Fields["SecurityGroupIDs"] | ||
assert.Equal(expected, code.ResolveReferencesForField(field, "ko", 1)) | ||
} | ||
|
||
func Test_ResolveReferencesForField_NestedSingleReference(t *testing.T) { | ||
assert := assert.New(t) | ||
require := require.New(t) | ||
|
||
g := testutil.NewModelForServiceWithOptions(t, "apigatewayv2", | ||
&testutil.TestingModelOptions{ | ||
GeneratorConfigFile: "generator-with-nested-reference.yaml", | ||
}) | ||
|
||
crd := testutil.GetCRDByName(t, g, "Authorizer") | ||
require.NotNil(crd) | ||
expected := | ||
jljaco marked this conversation as resolved.
Show resolved
Hide resolved
|
||
` if ko.Spec.JWTConfiguration != nil { | ||
if ko.Spec.JWTConfiguration.IssuerRef != nil && ko.Spec.JWTConfiguration.IssuerRef.From != nil { | ||
arr := ko.Spec.JWTConfiguration.IssuerRef.From | ||
if arr == nil || arr.Name == nil || *arr.Name == "" { | ||
return fmt.Errorf("provided resource reference is nil or empty: \"JWTConfiguration.IssuerRef"\") | ||
} | ||
obj := &svcapitypes.API{} | ||
if err := getReferencedResourceState_API(ctx, apiReader, obj, *arr.Name, namespace); err != nil { | ||
return err | ||
} | ||
ko.Spec.JWTConfiguration.Issuer = obj.Status.APIID | ||
} | ||
} | ||
` | ||
|
||
field := crd.Fields["JWTConfiguration.Issuer"] | ||
assert.Equal(expected, code.ResolveReferencesForField(field, "ko", 1)) | ||
} | ||
|
||
func Test_ResolveReferencesForField_SingleReference_DeeplyNested(t *testing.T) { | ||
assert := assert.New(t) | ||
require := require.New(t) | ||
|
||
g := testutil.NewModelForServiceWithOptions(t, "s3", | ||
&testutil.TestingModelOptions{ | ||
GeneratorConfigFile: "generator-with-nested-references.yaml", | ||
}) | ||
|
||
crd := testutil.GetCRDByName(t, g, "Bucket") | ||
require.NotNil(crd) | ||
|
||
// the Go template has the appropriate nil checks to ensure the parent path exists | ||
expected := | ||
jljaco marked this conversation as resolved.
Show resolved
Hide resolved
|
||
` if ko.Spec.Logging != nil { | ||
if ko.Spec.Logging.LoggingEnabled != nil { | ||
if ko.Spec.Logging.LoggingEnabled.TargetBucketRef != nil && ko.Spec.Logging.LoggingEnabled.TargetBucketRef.From != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thoughts: The more i see those, the more i think that we can refactor a little bit our code generator to make nil checks generic. Thinking a recursive wrapper function that can be called everywhere
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would only really work for structs. As soon as we get into lists and maps then things get iffy. I initially started to do that but it didn't extend cleanly. |
||
arr := ko.Spec.Logging.LoggingEnabled.TargetBucketRef.From | ||
if arr == nil || arr.Name == nil || *arr.Name == "" { | ||
return fmt.Errorf("provided resource reference is nil or empty: \"Logging.LoggingEnabled.TargetBucketRef"\") | ||
} | ||
obj := &svcapitypes.Bucket{} | ||
if err := getReferencedResourceState_Bucket(ctx, apiReader, obj, *arr.Name, namespace); err != nil { | ||
return err | ||
} | ||
ko.Spec.Logging.LoggingEnabled.TargetBucket = obj.Spec.Name | ||
} | ||
} | ||
} | ||
` | ||
|
||
field := crd.Fields["Logging.LoggingEnabled.TargetBucket"] | ||
assert.Equal(expected, code.ResolveReferencesForField(field, "ko", 1)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this upgrade required for this patch? same for the upgrade below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly can't tell you where this came from. Go modules are a mystery to me