Skip to content
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

replace nested field SecretKeyReference GoType #49

Merged
merged 4 commits into from
May 3, 2021

Conversation

jaypipes
Copy link
Collaborator

In this patch, we post-process the pkg/model.TypeDef objects that are
constructed by the code generator for nested fields of type "structure".
This post-processing is used to replace the GoType of an Attr
representing a Secret field in a nested parent field.

For example, the AmazonMQ Broker resource has a Spec.Users top-level
field. This top-level field is of type []*User. The User Go struct
type is constructed from a pkg/model.TypeDef object that describes the
struct's attributes. One of those struct attributes is called
Password, and we want to replace the Go type for this Attr from
string to *ackv1alpha1.SecretKeyReference. In order to do this Go
type replacement, we need to search through the TypeDef objects, looking
for the parent field TypeDef, then look through that TypeDef's Attr
collection for the Attr with the same name as our Field that has a
FieldConfig.IsSecret setting of true.

Issue aws-controllers-k8s/community#743

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@RedbackThomson
Copy link
Contributor

/approve

@aws-controllers-k8s aws-controllers-k8s deleted a comment from ack-bot Apr 29, 2021
@a-hilaly
Copy link
Member

/lgtm

@ack-bot ack-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 29, 2021
@aws-controllers-k8s aws-controllers-k8s deleted a comment from ack-bot Apr 29, 2021
Copy link
Contributor

@vijtrip2 vijtrip2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

In this patch, we post-process the `pkg/model.TypeDef` objects that are
constructed by the code generator for nested fields of type "structure".
This post-processing is used to replace the GoType of an Attr
representing a Secret field in a nested parent field.

For example, the AmazonMQ `Broker` resource has a `Spec.Users` top-level
field. This top-level field is of type `[]*User`. The `User` Go struct
type is constructed from a `pkg/model.TypeDef` object that describes the
struct's attributes. One of those struct attributes is called
`Password`, and we want to replace the Go type for this `Attr` from
`string` to `*ackv1alpha1.SecretKeyReference`. In order to do this Go
type replacement, we need to search through the TypeDef objects, looking
for the parent field TypeDef, then look through that TypeDef's Attr
collection for the Attr with the same name as our Field that has a
FieldConfig.IsSecret setting of `true`.

Issue aws-controllers-k8s/community#743
@jaypipes jaypipes force-pushed the field-config-nested branch from f2591ca to 6b6f1c2 Compare April 29, 2021 17:00
@ack-bot
Copy link
Collaborator

ack-bot commented Apr 29, 2021

New changes are detected. LGTM label has been removed.

@ack-bot ack-bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 29, 2021
@a-hilaly
Copy link
Member

I was trying to regenerate ecr with a fake secret field - I have two comments regarding the sdk and crd templates:

  1. Since the nested types used in apis/{{CRD.Name}}.go are defined in apis/types.go (for example User type). I think that we also need a hack to avoid import errors similar the one we have in sdk.go

  2. Currently SetSDK only sets secret values for top level fields. I guess we should add the same for CRD nested types ?

@jaypipes
Copy link
Collaborator Author

I was trying to regenerate ecr with a fake secret field - I have two comments regarding the sdk and crd templates:

1. Since the nested types used in `apis/{{CRD.Name}}.go` are defined in `apis/types.go` (for example `User` type). I think that we also need a hack to avoid import errors [similar the one we have in `sdk.go`](https://github.com/aws-controllers-k8s/code-generator/blob/main/templates/pkg/resource/sdk.go.tpl#L20-L29)

Yes, good callout. I will update in the morning.

2. Currently `SetSDK` only sets secret values for [top level fields](https://github.com/aws-controllers-k8s/code-generator/blob/main/pkg/generate/code/set_sdk.go#L194-L207). I guess we should add the same for CRD nested types ?

Yep. ++

@jaypipes jaypipes changed the title replace nested field SecretKeyReference GoType DO NOT MERGE replace nested field SecretKeyReference GoType Apr 29, 2021
@jaypipes jaypipes changed the title DO NOT MERGE replace nested field SecretKeyReference GoType replace nested field SecretKeyReference GoType May 1, 2021
@jaypipes
Copy link
Collaborator Author

jaypipes commented May 1, 2021

@a-hilaly great callouts on your review. I've addressed both of them in the latest commits on this PR. Thanks for your review again!

Copy link
Member

@a-hilaly a-hilaly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat! I like the 5a5300d commit 👍
I left two non blocking comments

@@ -330,22 +326,6 @@ func (g *Generator) GetTypeDefs() ([]*ackmodel.TypeDef, map[string]string, error
// pointer to a struct...
goPkgType = goPkgType[1:]
}
pkg := strings.Split(goPkgType, ".")[0]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

goPkgType is not used anymore - we can remove the block above

Comment on lines 239 to 259
if r.IsSecretField(memberName) {
out += setSDKForSecret(
cfg, r,
memberName,
targetVarName,
sourceAdaptedVarName,
indentLevel,
)
continue
}

if r.IsSecretField(memberName) {
out += setSDKForSecret(
cfg, r,
memberName,
targetVarName,
sourceAdaptedVarName,
indentLevel,
)
continue
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate code :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, wow... copy pasta. Will fix ASAP. thank you!

jaypipes added 3 commits May 3, 2021 11:08
This commit replaces the tracking of unique imported packages from the
processing of TypeDef structs by the code generator. Instead, we now
use the same technique to avoid import errors that we do in the `sdk.go`
code:

```go
import (
	ackv1alpha1 "github.com/aws-controllers-k8s/runtime/apis/core/v1alpha1"
	"github.com/aws/aws-sdk-go/aws"
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

// Hack to avoid import errors during build...
var (
	_ = &metav1.Time{}
	_ = &aws.JSONValue{}
	_ = ackv1alpha1.AWSAccountID("")
)
```

The above is all the packages that are referenced by any type
definition's attribute fields that is collected by the code generator
during API inference.
Encapsulates the code that generates Go code for setting an SDK Input shape
from the value of a SecretKeyReference in a separate `setSDKForSecret`
function in `pkg/generate/code` and creates a `CRD.IsSecretField` that
accepts a field path and returns whether the field at that field path is
a SecretKeyReference.
Implements the `pkg/generate/code.SetSDK()` function for all nested
fields that are SecretKeyReference types. This involved passing a field
path down into the recursive `setSDKForXXX()` functions to keep track of
where we were in the processing of nested fields. We look up whether a
field is a SecretKeyReference by calling the
`pkg/model.CRD:IsSecretField()` method, which accepts a field path,
which is why we needed to pass this field path down into the recursive
code generators.

Issue aws-controllers-k8s/community#743
@jaypipes jaypipes force-pushed the field-config-nested branch from 7fa1166 to eef7ca5 Compare May 3, 2021 16:04
Copy link
Member

@a-hilaly a-hilaly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - let's 🚢 it!

@ack-bot
Copy link
Collaborator

ack-bot commented May 3, 2021

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: A-Hilaly, jaypipes, RedbackThomson, vijtrip2

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jaypipes jaypipes merged commit b91b18d into aws-controllers-k8s:main May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants