allow different Go types for input/output shapes #232

jaypipes · 2021-10-26T17:30:13Z

Adds a new SetFieldConfig struct to the code generator config that
allows teams to instruct the code generator to handle the SetResource
generator specially for a particular pkg/model.OpType.

This is useful for at least a couple use cases:

Different Go types in input and output shapes

When the Go type of a field in the Create Input shape differs from the
Go type of the same-named field in another operation's Output shape, the
code generator doesn't know how to handle this situation and ends up
outputting invalid Go code.

An example of this behaviour is in the RDS controller's DBInstance
resource. The DBSecurityGroups field in the Create Input shape is of
type []*string, but the same field in the Create, ReadMany and Update
output shapes is of type []*DBSecurityGroupMembership. The
DBSecurityGroupMembership struct has two fields, DBSecurityGroupName
and Status. The only part of that struct that is relevant to the
Spec.DBSecurityGroups field is the DBSecurityGroupName field, which
corresponds to the string identifier of the DB security group provided
by the user in the CreateDBInstanceRequest.DBSecurityGroups field.

The code generator ends up calling pkg/generate/code.SetResource for
the DBInstance resource, gets to the Spec.DBSecurityGroups field and
just doesn't know how to handle the different Go types and so ends up
outputting nothing, which breaks compilation and has caused us to
manually comment out or fix the generated code. operation.

See: https://github.com/aws-controllers-k8s/rds-controller/blob/dc3271fa7b455fc99c3531ce372ea221c9f5b8e7/pkg/resource/db_instance/sdk.go#L853-L864

In the RDS DBInstance DBSecurityGroups case, we'd add the following to
the RDS generator.yaml file to instruct the code generator how to
transform the response from the DescribeDBInstances Output shape for
setting the value of Spec.DBSecurityGroups from the set of
DBSecurityGroupMembership.DBSecurityGroupName values:

resources:
  DBInstance:
    fields:
      DBSecurityGroups:
        set:
         - on: Create
           from: DBSecurityGroupName
         - on: ReadOne
           from: DBSecurityGroupName

Ignoring fields in output shapes containing stale data

For some service APIs, the returned value for a field in the Output
shape of an Update operation contains the originally-set value instead
of the value of the field that was set in the Update operation itself.

An example of this situation is the ElastiCache ModifyReplicationGroup
API call. If you pass some value for the LogDeliveryConfiguration field
in the Input shape (which comes from the Spec.LogDeliveryConfiguration
field of the ReplicationGroup resource), the value that is returned in
the Output shape's LogDeliveryConfiguration is actually the old
(stale) value for the field. The reason for this return of stale data is
because the log delivery configuration is asynchronously mutated.

For these cases, we want to be able to tell the code generator to ignore
these fields when outputting code for the
pkg/generate/code.SetResource function, but only for the Update
operation. In this case, we'd add the following to the ElastiCache
generator.yaml file:

resources:
  ReplicationGroup:
    fields:
      LogDeliveryConfiguration:
        set:
         - on: Update
           ignore: true

Signed-off-by: Jay Pipes jaypipes@gmail.com

Issue: aws-controllers-k8s/community#178

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

brycahta · 2021-10-26T18:12:39Z

pkg/generate/config/field.go

+	//           - on: UPDATE
+	//			   ignore: true
+	// ```
+	Ignore bool `json:"ignore,omitempty"`


Could you help me understand the difference between this and leveraging FieldPaths in IgnoreSpec ? Ex:

ignore: field_paths: - ModifyReplicationGroupOutput.LogDeliveryConfiguration

The big difference is making the configuration more field-centric as opposed to aws-sdk-go Operation-centric. In this new SetFieldConfig.Ignore functionality, the ACK contributor doesn't need to know or specify the exact name of the aws-sdk-go Shape or Operation at all. They only need to say that the code generator should skip outputting "setter" Go code for the ReplicationGroup resource's Spec.LogDeliveryConfiguration field for the UPDATE code paths.

I am onboard with the more field-centric approach 👍 However, would you be opposed to keeping operation name as it is in the sdk instead of introducing ACK naming (ex. UPDATE)?

ACK contributor doesn't need to know or specify the exact name of the aws-sdk-go Shape or Operation at all

I'm of the mind that generator.yaml should align with aws-sdk-go when it comes to resources, operations, naming, capitalization, etc. My thinking is we will have a decent chunk of contributors who only edit/maintain their service's generator.yaml (i.e. never work on code-generator), and a maintainer like a service owner from AWS would be more familiar with aws-sdk concept than they would any ACK ones. Therefore, try not to introduce too many ACK concepts in generator.yaml to make it easier for people to onboard/maintain without ACK platform-specific knowledge.

If you don't think this scenario is likely (i.e. all contributors will have ACK platform knowledge), then I have no qualms mixing ACK + aws-sdk-go in generator.yaml

a maintainer like a service owner from AWS would be more familiar with aws-sdk concept than they would any ACK ones

I agree that this will be the case. However, structuring the generator with a CRD-/reconciler-centric focus will more easily lead to a better K8s-feeling experience for the ACK users. By that I mean understanding how the generator config impacts the end result (the CRD and controller) in a more concrete way will hopefully lead to service teams building more logical K8s abstractions on top of their services.

Also by using ACK terminology, we aren't locking ourselves into such a deep cohesion with the AWS SDK for Go in the long-run.

brycahta · 2021-10-26T18:43:49Z

pkg/generate/config/field.go

+	// ```yaml
+	// resources:
+	//   DBInstance:
+	//     fields:


Are we opposed to making this new config part of an operation config (which one TBD)? If the opType will influence the override, then I think it makes sense to make the override an attribute of operation config and it will read a little clearer in the generator.yaml.

Options for Op Config

Extend ListOperationConfig, UpdateOperationConfig, and add support for others

resources: DBInstance: list_operation: set: DBSecurityGroups from: DBSecurityGroupName

Add a new "Resource Operation Config" containing a remaps field and that we could extend for renames later:

resources: DBInstance: operations: DescribeDBSecurityGroups: remaps: set: DBSecurityGroups from: DBSecurityGroupName

Add RemapsConfig.OperationRemapsConfig aligning with existing renames

resources: DBInstance: remaps: operations: DescribeDBSecurityGroups: set: DBSecurityGroups from: DBSecurityGroupName

I'd like to move away from the OperationConfig as much as possible, especially for renaming things. I feel a Field-centric configuration model is more natural for ACK contributors: they just focus on the fields of their resources and instructing the code generator how to handle specially certain fields.

RedbackThomson · 2021-10-27T19:30:06Z

pkg/generate/config/field.go

+	// On is the `pkg/model.OpType` whose Output shape will be transformed by
+	// this config. If empty, this set field config applies to all operations
+	On *string `json:on,omitempty`


Is it necessary to discriminate between READ_ONE and READ_MANY? Could we stick to create, find, update, delete?

Also note that this field should not omitempty as it is required?

It's not required (see code comment above). If empty, this config applies to all operation types.

Is it necessary to discriminate between READ_ONE and READ_MANY? Could we stick to create, find, update, delete?

Great point. I will go with the top-level resource manager method names: ReadOne, Create, Update, and Delete and make a note in the code comment that it's not the model OpType but rather just the overall resource manager method that is referred to here. I'll also make it case-insensitive matching.

RedbackThomson · 2021-10-27T19:32:00Z

pkg/generate/config/field.go

+	// MemberAccessPath tells the code generator to output Go code that sets
+	// the value of a variable containing the target resource field with the
+	// contents of a member field in the source struct.


Is this expected to handle struct-struct mapping ie. adapting between struct shapes? Or is it just for primitives?

Great question. For right now, I was thinking just struct -> primitive, including []struct -> []primitive.

Adds a new SetFieldConfig struct to the code generator config that allows teams to instruct the code generator to handle the SetResource generator specially for a particular `pkg/model.OpType`. This is useful for at least a couple use cases: 1. Different Go types in input and output shapes When the Go type of a field in the Create Input shape differs from the Go type of the same-named field in another operation's Output shape, the code generator doesn't know how to handle this situation and ends up outputting invalid Go code. An example of this behaviour is in the RDS controller's DBInstance resource. The DBSecurityGroups field in the Create Input shape is of type `[]*string`, but the same field in the Create, ReadMany and Update output shapes is of type `[]*DBSecurityGroupMembership`. The `DBSecurityGroupMembership` struct has two fields, `DBSecurityGroupName` and `Status`. The only part of that struct that is relevant to the `Spec.DBSecurityGroups` field is the `DBSecurityGroupName` field, which corresponds to the string identifier of the DB security group provided by the user in the `CreateDBInstanceRequest.DBSecurityGroups` field. The code generator ends up calling `pkg/generate/code.SetResource` for the DBInstance resource, gets to the `Spec.DBSecurityGroups` field and just doesn't know how to handle the different Go types and so ends up outputting nothing, which breaks compilation and has caused us to manually comment out or fix the generated code. operation. See: https://github.com/aws-controllers-k8s/rds-controller/blob/dc3271fa7b455fc99c3531ce372ea221c9f5b8e7/pkg/resource/db_instance/sdk.go#L853-L864 In the RDS DBInstance DBSecurityGroups case, we'd add the following to the RDS generator.yaml file to instruct the code generator how to transform the response from the DescribeDBInstances Output shape for setting the value of `Spec.DBSecurityGroups` from the set of `DBSecurityGroupMembership.DBSecurityGroupName` values: ```yaml resources: DBInstance: fields: DBSecurityGroups: set: - on: Create from: DBSecurityGroupName - on: ReadOne from: DBSecurityGroupName ``` 2. Ignoring fields in output shapes containing stale data For some service APIs, the returned value for a field in the Output shape of an Update operation contains the *originally-set* value instead of the value of the field that was set in the Update operation itself. An example of this situation is the ElastiCache ModifyReplicationGroup API call. If you pass some value for the LogDeliveryConfiguration field in the Input shape (which comes from the Spec.LogDeliveryConfiguration field of the ReplicationGroup resource), the value that is returned in the Output shape's LogDeliveryConfiguration is actually the *old* (stale) value for the field. The reason for this return of stale data is because the log delivery configuration is asynchronously mutated. For these cases, we want to be able to tell the code generator to ignore these fields when outputting code for the `pkg/generate/code.SetResource` function, but only for the Update operation. In this case, we'd add the following to the ElastiCache generator.yaml file: ```yaml resources: ReplicationGroup: fields: LogDeliveryConfiguration: set: - on: Update ignore: true ``` Signed-off-by: Jay Pipes <jaypipes@gmail.com>

vijtrip2 · 2021-11-09T21:30:06Z

pkg/generate/ack/controller.go

+			}
+			// We may have some special instructions for how to handle setting the
+			// field value...
+			setCfg := f.GetSetterConfig(model.OpTypeList)


Why model.OpTypeList as operation?
I think adding a comment for the same will be useful as well.

vijtrip2 · 2021-11-09T21:48:11Z

pkg/model/field.go

+	for _, setCfg := range f.FieldConfig.Set {
+		// If the Method attribute is nil, that means the setter config applies to
+		// all resource manager methods for this field.
+		if setCfg.Method == nil || strings.EqualFold(rmMethod, *setCfg.Method) {


What should be the strategy if there is a setConfig with empty method name and one setConfig with OpType from parameter?

I think currently a single one will be returned depending on which is defined earlier. Should a priority be added for method match first and then fallback to empty method?

RedbackThomson · 2021-11-09T22:04:12Z

pkg/generate/code/set_resource.go

+			// field.
+			var found bool
+			if setCfg != nil && setCfg.From != nil {
+				sourceMemberShapeRef, found = outputShape.MemberRefs[*setCfg.From]


Do we want to supported nested field paths here, as well?

+1, I'm fairly certain I will need nested field paths support for DhcpOptions.

RedbackThomson · 2021-11-09T22:06:32Z

pkg/generate/code/set_resource.go

 			targetFieldName,
 			targetVarName,
 			targetShapeRef,
+			targetSetCfg,
 			sourceVarName,
 			sourceShapeRef,
 			indentLevel,


Oh wow these are getting out of hand :/

Yeah at first glance looks like we might be able to refactor by passing Field type in directly instead of each of its fields or maybe extend Field type with these methods

brycahta

Thanks for this Jay; left some comments and will test this locally with dhcpOptions.

update: after pulling this down, it won't work for DhcpOptions. To summarize, DhcpOptions Spec has a slice of *NewDHCPConfiguration:

type NewDHCPConfiguration struct {
	Key *string `json:"key,omitempty"`
	Values []*string `json:"values,omitempty"`
}

and the CreateDhcpOptions Result output shape returns a slice of *DHCPConfiguration:

type AttributeValue struct {
	Value *string `json:"value,omitempty"`
}

type DHCPConfiguration struct {
	Key *string `json:"key,omitempty"`
	Values []*AttributeValue `json:"values,omitempty"`
}

I need entries in NewDHCPConfiguration.Values to be populated with data from DHCPConfiguration.Values.Value

If supporting this is outside the scope of the PR let me know and I can open an issue to track

brycahta · 2021-11-09T21:43:05Z

pkg/generate/ack/controller.go

+			}
+			// We may have some special instructions for how to handle setting the
+			// field value...
+			setCfg := f.GetSetterConfig(model.OpTypeList)


why hard-code model.OpTypeList? From the PR description it looks like both Create and Update support setter configs as well.

brycahta · 2021-11-09T21:58:08Z

pkg/generate/code/set_resource.go

+		setCfg := f.GetSetterConfig(opType)
+
+		if setCfg != nil && setCfg.Ignore {
+			continue
+		}
+
+		sourceMemberShapeRef := outputShape.MemberRefs[memberName]
+		if sourceMemberShapeRef.Shape == nil {
+			// We may have some instructions to specially handle this field by
+			// accessing a different Output shape member.
+			//
+			// As an example, let's say we're dealing with a resource called
+			// Bucket that has a "Name" field. In the Output shape of the
+			// ReadOne resource manager method, the field name is "Bucket",
+			// though. There may be a generator.yaml file that looks like this:
+			//
+			// resources:
+			//   Bucket:
+			//     fields:
+			//       Name:
+			//         set:
+			//          - method: ReadOne
+			//            from: Bucket
+			//
+			// And this would indicate to the code generator that the Spec.Name
+			// field should be set from the value of the Output shape's Bucket
+			// field.
+			var found bool


Nit cleanup suggestions:

setCfg := f.GetSetterConfig(opType) sourceMemberShapeRef := outputShape.MemberRefs[memberName] if sourceMemberShapeRef.Shape == nil && setCfg != nil { // We may have some instructions... if setCfg.Ignore != nil { continue } if setCfg.From != nil ... }

brycahta · 2021-11-09T22:26:54Z

pkg/generate/code/set_resource.go

+	//       DBSecurityGroups:
+	//         set:
+	//          - method: ReadOne
+	//            from: DBSecurityGroupName


nit: Don't want to hold up review with this, but think interface can be improved.

Not a huge fan of DBSecurityGroups being used to reference both CRD field and Output Shape field and/or that the user needs background knowledge on this override (fields must be same name) in order to use it. This lack of clarity becomes more apparent when combining this config with output_wrapper_field_path for the same operation. Example is DhcpOptions:

operations: CreateDhcpOptions: output_wrapper_field_path: DhcpOptions ... resources: DhcpOptions: fields: DhcpConfigurations: set: - on: Create from: Values

Since you are resolving a type mismatch issue here, what if the setconfig from reflected the path to the value of a given type -- not the exact field name in Output Shape? Something like:

// resources: // DBInstance: // fields: // DBSecurityGroups: // set: // - method: ReadOne // from_type: DBSecurityGroupMembership.DBSecurityGroupName

brycahta

Discussed offline; nested fields will be in a follow-up patch

brycahta · 2021-11-11T19:33:35Z

/lgtm

ack-bot · 2021-11-11T19:33:37Z

@brycahta: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

brycahta · 2021-11-11T19:41:26Z

/lgtm

ack-bot · 2021-11-11T19:41:31Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [RedbackThomson,jaypipes,vijtrip2]

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

ack-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 26, 2021

ack-bot requested review from a-hilaly and RedbackThomson October 26, 2021 17:30

ack-bot added the approved label Oct 26, 2021

brycahta reviewed Oct 26, 2021

View reviewed changes

jaypipes force-pushed the diff-go-types branch from 1ab1db4 to 7e7730b Compare October 26, 2021 18:56

RedbackThomson reviewed Oct 27, 2021

View reviewed changes

jaypipes force-pushed the diff-go-types branch from 7e7730b to a08380a Compare November 9, 2021 19:53

jaypipes changed the title ~~WIP - deal with diff Go types~~ allow different Go types for input/output shapes Nov 9, 2021

ack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 9, 2021

jaypipes force-pushed the diff-go-types branch from a08380a to 1e1697f Compare November 9, 2021 20:01

vijtrip2 approved these changes Nov 9, 2021

View reviewed changes

RedbackThomson approved these changes Nov 9, 2021

View reviewed changes

brycahta reviewed Nov 9, 2021

View reviewed changes

brycahta approved these changes Nov 11, 2021

View reviewed changes

ack-bot assigned brycahta Nov 11, 2021

ack-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 11, 2021

ack-bot merged commit 30510fc into aws-controllers-k8s:main Nov 11, 2021

RedbackThomson mentioned this pull request Nov 12, 2021

same-named fields in Input and Output shapes with different Go types aws-controllers-k8s/community#178

Closed

jaypipes mentioned this pull request Nov 12, 2021

Handle nested fields in SetFieldConfig aws-controllers-k8s/community#1065

Closed

AaronME mentioned this pull request Nov 24, 2021

SetResource does not handle String Pointers properly in GenerateCrawler aws-controllers-k8s/community#1078

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow different Go types for input/output shapes #232

allow different Go types for input/output shapes #232

jaypipes commented Oct 26, 2021 •

edited

Loading

brycahta Oct 26, 2021

jaypipes Oct 26, 2021

brycahta Oct 26, 2021

RedbackThomson Oct 27, 2021

brycahta Oct 26, 2021

jaypipes Oct 26, 2021

RedbackThomson Oct 27, 2021

RedbackThomson Oct 27, 2021

jaypipes Oct 28, 2021

jaypipes Oct 28, 2021

RedbackThomson Oct 27, 2021

jaypipes Oct 28, 2021

vijtrip2 Nov 9, 2021

vijtrip2 Nov 9, 2021

RedbackThomson Nov 9, 2021

brycahta Nov 10, 2021 •

edited

Loading

RedbackThomson Nov 9, 2021

brycahta Nov 9, 2021

brycahta left a comment •

edited

Loading

brycahta Nov 9, 2021

brycahta Nov 9, 2021

brycahta Nov 9, 2021 •

edited

Loading

brycahta left a comment

brycahta commented Nov 11, 2021

ack-bot commented Nov 11, 2021

brycahta commented Nov 11, 2021

ack-bot commented Nov 11, 2021

allow different Go types for input/output shapes #232

allow different Go types for input/output shapes #232

Conversation

jaypipes commented Oct 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brycahta Nov 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brycahta left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brycahta Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

brycahta left a comment

Choose a reason for hiding this comment

brycahta commented Nov 11, 2021

ack-bot commented Nov 11, 2021

brycahta commented Nov 11, 2021

ack-bot commented Nov 11, 2021

jaypipes commented Oct 26, 2021 •

edited

Loading

brycahta Nov 10, 2021 •

edited

Loading

brycahta left a comment •

edited

Loading

brycahta Nov 9, 2021 •

edited

Loading