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

allow "nested fields" to be configured #47

Merged
merged 1 commit into from
Apr 28, 2021

Conversation

jaypipes
Copy link
Collaborator

When designing the pkg/generate/config.FieldConfig object, we really
only thought about (and tested for) "top-level fields". In other words,
fields that were on the CR's Spec or Status objects.

However, as we've added more configurability to the code-generator
regarding fields -- including marking fields as printable or secrets --
we're now limited substantially by our initial implementation that only
allows referring to top-level fields.

For example, we cannot mark a field as being a secret field if that
field is a subfield of one of these top-level fields. Concretely, this
means that with the existing generator configuration, we cannot support
configuring AmazonMQ's Broker CRD's Spec.Users.Password field as a
SecretKeyReference because Password is not a top-level field. It is
instead a field of a top-level field -- a "nested field" -- and
therefore our simplistic configuration system isn't able to tell the
code generator to replace the string Go type with
*ackv1alpha1.SecretKeyReference like it does for
top-level fields.

The solution to this dilemma is contained in this PR and involves
reworking our generator model in a couple ways:

  1. The pkg/model.CRD struct now has a Fields attribute to go along
    with the existing SpecFields and StatusFields attributes.
    CRD.Fields is a map[string]*Field like SpecFields and
    StatusFields, but unlike those attributes, the Fields map keys
    are field paths and not the original aws-sdk-go Shape member names.
    When inspecting the AWS service API model as we do in
    pkg/generate.Generator:GetCRDs(), we now construct a field path for
    each pkg/model.Field object that corresponds to "where" in the CR the
    field is. For example, Spec.Name or Status.BrokerInstances

  2. The pkg/generate.Generator has been updated with a
    processNestedFields() method to populate the new field-path-keyed
    CRD.Fields map with nested fields, making it possible to refer in our
    generator config, by field path, to fields that are not directly in the
    Status or Spec object.

A following patch will update the Generator.GetTypeDefs() method to
look up field configuration by this new field-path-based map and replace
string with *ackv1alpha1.SecretKeyReference when the field is a
secret field (Issue aws-controllers-k8s/community#743).

Issue aws-controllers-k8s/community#748

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

When designing the `pkg/generate/config.FieldConfig` object, we really
only thought about (and tested for) "top-level fields". In other words,
fields that were on the CR's `Spec` or `Status` objects.

However, as we've added more configurability to the code-generator
regarding fields -- including marking fields as printable or secrets --
we're now limited substantially by our initial implementation that only
allows referring to top-level fields.

For example, we cannot mark a field as being a secret field if that
field is a subfield of one of these top-level fields. Concretely, this
means that with the existing generator configuration, we cannot support
configuring AmazonMQ's `Broker` CRD's `Spec.Users.Password` field as a
SecretKeyReference because `Password` is not a top-level field. It is
instead a field *of* a top-level field -- a "nested field" -- and
therefore our simplistic configuration system isn't able to tell the
code generator to replace the `string` Go type with
`*ackv1alpha1.SecretKeyReference` like it [does][secref-inject] for
top-level fields.

[secref-inject]: https://github.com/aws-controllers-k8s/code-generator/blob/94186d92e778792ccba11b5db3478e037256b36b/pkg/model/field.go#L61-L64

The solution to this dilemma is contained in this PR and involves
reworking our generator model in a couple ways:

1) The `pkg/model.CRD` struct now has a `Fields` attribute to go along
   with the existing `SpecFields` and `StatusFields` attributes.
   `CRD.Fields` is a `map[string]*Field` like `SpecFields` and
   `StatusFields`, but unlike those attributes, the `Fields` map keys
   are field *paths* and not the original aws-sdk-go Shape member names.
   When inspecting the AWS service API model as we do in
   `pkg/generate.Generator:GetCRDs()`, we now construct a field path for
   each `pkg/model.Field` object that corresponds to "where" in the CR the
   field is. For example, `Spec.Name` or `Status.BrokerInstances`

2) The `pkg/generate.Generator` has been updated with a
   `processNestedFields()` method to populate the new field-path-keyed
   `CRD.Fields` map with nested fields, making it possible to refer in our
   generator config, by field path, to fields that are not directly in the
   Status or Spec object.

A following patch will update the `Generator.GetTypeDefs()` method to
look up field configuration by this new field-path-based map and replace
`string` with `*ackv1alpha1.SecretKeyReference` when the field is a
secret field (Issue aws-controllers-k8s/community#743).

Issue aws-controllers-k8s/community#748
@jaypipes jaypipes force-pushed the field-config-nested branch from 3f47e88 to 9e2458d Compare April 28, 2021 15:45
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

I like the distinction you made using '..' for list and Map members. Haven't seen that before.

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

Comment on lines +491 to +493
if elementFieldShape.Type != "structure" {
return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: What does this conditional shield against?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's no need to recurse into list fields when it's a list of strings, for example. Only when it's a list of structs is it necessary to dig deeper into the nesting.

@jaypipes jaypipes merged commit 943efb0 into aws-controllers-k8s:main Apr 28, 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.

4 participants