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

Add support for generating MR API for Terraform resource's nested single configuration blocks #342

Merged
merged 3 commits into from
Feb 14, 2024

Conversation

ulucinar
Copy link
Collaborator

@ulucinar ulucinar commented Feb 9, 2024

Description of your changes

Related with: crossplane-contrib/provider-upjet-aws#889, crossplane-contrib/provider-upjet-aws#1130

While generating the managed resource (MR) API for Terraform's aws_opensearchserverless_security_config resource, we've observed that the required parameter saml_options is not generated and thus, the resulting API is not usable. @erhancagirici did an initial analysis and the root cause of the issue turns out to be upjet skipping code generation for nested configuration blocks with the SchemaNestingModeSingle nesting mode. Looks like we have initially chosen to do so to prevent the code generation for the resource CRUD timeout parameter generation for each MR API and opted for a static CRUD timeout configuration for the generated MRs (it's currently not possible to configure such timeouts dynamically at runtime).

However, this is a blocker for the generation of resources with a required nested configuration block in single mode.

This PR still prevents the generation of top-level CRUD timeouts for the resource.Terraformed resources (so we still don't allow dynamic configuration of such timeouts) but allows the code generation for other nested configuration blocks in single mode. We now filter the top-level CRUD timeout configuration blocks by name (instead of the previous nesting mode based approach).

For the converted configuration blocks, Schema.MaxItems is always set to 1, and if there are any required mappings for the configuration block, Schema.MinItems is also set to 1, together with the Optional and Required attributes. The Schema.Type is always schema.TypeList for such nested configuration blocks. Thus they are modeled as lists with length constraints.

This PR also introduces the config.SchemaElementOption.EmbeddedObject API which can be used to configure upjet's code generation pipeline to generate an embedded object instead of a singleton list. We've chosen this explicit approach instead of making the schema block information available to the code generation pipeline and automatically deducing the schema elements with the single nesting mode with the following motivations:

  1. Currently, we don't want to generate embedded objects for all singleton lists
  2. We need to be careful converting singleton lists (without the single nesting mode) into embedded objects because the MaxLength = 1 constraint on them could change in the future depending on their semantics. So we need to be careful with any sort of automatic conversions and plan ahead for API changes (possibly via conversion functions but converting between a new API supporting multiple elements in a list and an older API with an embedded object is cumbersome to implement in our generated APIs with the proper backwards compatibility constraints).
  3. There are currently only two resources in the Terraform AWS provider that use single nesting mode, one of them being the aws_opensearchserverless_security_config resource's saml_options field. So, a manual configuration approach is okay until we would like to tackle with One-element arrays could be considered as objects #136 (which is subject to [2] above).
    An example configuration for the aws_opensearchserverless_security_config resource's saml_options configuration is as follows:
	p.AddResourceConfigurator("aws_opensearchserverless_security_config", func(r *config.Resource) {
		r.SchemaElementOptions.SetEmbeddedObject("saml_options")
	})

The specified path is the Terraform path of the schema element which is to be generated as an embedded object without any indices or wildcards (the same syntax for the config.SchemaElementOptions.SetAddToObservation). Please note that the code generation pipeline currently makes no checks on the size constraints of the list, i.e., it does not assert that the specified path is a singleton list (due to [3] above, this is a very special case we need to handle for just two resources).

We also add the config.ExternalNameFrom external-name configuration with this PR. ExternalNameFrom is an ExternalName configuration which uses a parent configuration as its base and modifies any of the GetIDFn,
GetExternalNameFn or SetIdentifierArgumentsFn functions of the configuration. This enables us to reuse the existing ExternalName configurations with modifications in their behaviors via compositions. As an example in https://github.com/upbound/provider-aws, there are resources that lend themselves to config.TemplatedStringAsIdentifier but fail or misbehave when the Terraform ID string is initially invalid due to a missing external-name annotation. A common workaround is to use a default string for the external-name if it's missing (that should never resolve to an existing external resource). This can now be achieved with a configuration like the following:

// templatedStringWithDefaultedName is a config.TemplatedStringAsIdentifier
// that sets the external-name to the given default value if it's empty.
// This is required for certain Terraform resources which reject or misbehave
// when their IDs are initially constructed with an empty external-name
// annotation value.
func templatedStringWithDefaultedName(nameFieldPath, tmpl, defaultName string) config.ExternalName {
	return config.NewExternalNameFrom(config.TemplatedStringAsIdentifier(nameFieldPath, tmpl),
		config.WithGetIDFn(func(fn config.GetIDFn, ctx context.Context, externalName string, parameters map[string]any, terraformProviderConfig map[string]any) (string, error) {
			if externalName == "" {
				externalName = defaultName
			}
			return fn(ctx, externalName, parameters, terraformProviderConfig)
		}))
}

This ExternalName configuration will use the supplied defaultName when the external-name annotation is empty to prevent the underlying resource from misbehaving.

I have:

  • Read and followed Upjet's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Tested via crossplane-contrib/provider-upjet-aws#1130.

…gle configuration blocks

Signed-off-by: Alper Rifat Ulucinar <[email protected]>
which will be generated as embedded objects instead of singleton lists in CRDs.

Signed-off-by: Alper Rifat Ulucinar <[email protected]>
…onfigurations

reusable via compositions.

- Format the doc comments for the config.TemplatedStringAsIdentifier function.

Signed-off-by: Alper Rifat Ulucinar <[email protected]>
Copy link
Member

@sergenyalcin sergenyalcin left a comment

Choose a reason for hiding this comment

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

Thanks @ulucinar LGTM!

pkg/config/externalname.go Show resolved Hide resolved
pkg/types/conversion/tfjson/tfjson.go Show resolved Hide resolved
@ulucinar ulucinar merged commit 9cb2f6b into crossplane:main Feb 14, 2024
7 checks passed
@ulucinar ulucinar deleted the single-nested branch February 14, 2024 19:35
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.

2 participants