Skip to content

Extend resource configuration API to include resource metadata#19

Merged
ulucinar merged 5 commits into
crossplane:mainfrom
ulucinar:fix-6
Jun 30, 2022
Merged

Extend resource configuration API to include resource metadata#19
ulucinar merged 5 commits into
crossplane:mainfrom
ulucinar:fix-6

Conversation

@ulucinar

@ulucinar ulucinar commented Jun 20, 2022

Copy link
Copy Markdown
Collaborator

Description of your changes

Fixes #6

This PR extends the existing upjet resource configuration API to allow configuration & overrides related to the resource metadata (probably scraped from Terraform registry). Some possible configuration scenarios covered as of now:

  • Fixing syntax errors in the scraped examples from the Terraform registry, e.g., fix azurerm_key_vault_access_policy.example-user's key_permissions attribute.
  • Replacing/injecting attributes, if desired, in the extracted example manifest (resource specific or provider-wide), e.g., injecting a region attribute for all/select resources in provider-aws
  • Manipulating/replacing reference fields, e.g., replace location references in provider-azure
  • Marking resources that require globally unique names, such as the Azure Key Vaults or AWS S3 buckets.

I have:

  • Read and followed Crossplane'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

A resource specific metadata override example:

p.AddResourceConfigurator("azurerm_disk_encryption_set", func(r *config.Resource) {
                ...
		if err := r.MetaResource.Examples[0].SetPathValue("location", "the middle of nowhere"); err != nil {
			panic(err)
		}
	})

Or, as expected, a provider-wide metadata configuration can be performed as follows (in for example, config/provider.go):

        ...
	for _, r := range pc.Resources {
		if err := r.MetaResource.Examples[0].SetPathValue("region", "Test Region"); err != nil {
			panic(err)
		}
	}

An example manifest generated using these configuration overrides on top of scraped metadata:

apiVersion: compute.azure.upbound.io/v1beta1
kind: DiskEncryptionSet
metadata:
  name: example
spec:
  forProvider:
    identity:
    - type: SystemAssigned
    keyVaultKeyId: ${azurerm_key_vault_key.example.id}
    location: the middle of nowhere
    region: Test Region
    resourceGroupNameRef:
      name: example
  providerConfigRef:
    name: default

To configure a unique name to be generated by the test runner, one may use:

p.AddResourceConfigurator("azurerm_key_vault", func(r *config.Resource) {
    ...
    r.MetaResource.ExternalName = meta.RandRFC1123Subdomain

The generated example manifest now contains:

apiVersion: keyvault.azure.upbound.io/v1beta1
kind: Vault
metadata:
  annotations:
    crossplane.io/external-name: '{{ .Rand.RFC1123Subdomain }}'
...

which is to be substituted by the test runner at runtime with a random RFC1123 subdomain string.

@ulucinar ulucinar marked this pull request as draft June 21, 2022 07:03
@ulucinar ulucinar force-pushed the fix-6 branch 3 times, most recently from 594fdae to d5053ea Compare June 21, 2022 16:59
@ulucinar ulucinar marked this pull request as ready for review June 21, 2022 16:59
Comment thread pkg/meta/resource.go Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we're planning on keeping this as single file, would it make sense to use go:embed similar to schema so we don't need to worry about reading files in Upjet?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like that if we use go:embed, we don't need to add RootDir and ProviderMetadataPath fields to config.Provider, which is preferable.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think at some point it's better to split the monolithic provider-metadata.yaml file. My proposal would be to do the per-resource splitting in further rounds, but we may also do so as part of the ongoing https://github.com/upbound/official-providers/issues/19 issue.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Honestly, I can be convinced either way regarding single or multiple files. But if we do end up going with a single file, I'd prefer to go:embed it so that we do not deal with file reading at all in Upjet.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done. We now go:embed the monolithic provider metadata file. We may later split the provider metadata per resource in the future as discussed.

Comment thread pkg/meta/resource.go Outdated
Comment thread pkg/meta/resource.go Outdated
Comment thread pkg/meta/resource.go Outdated
Comment thread pkg/meta/resource.go Outdated
Comment thread pkg/meta/resource.go Outdated
@ulucinar

Copy link
Copy Markdown
Collaborator Author

Thanks @muvaf for the comments!

@ulucinar ulucinar requested a review from muvaf June 23, 2022 17:35
Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
- Rename Resource.TitleName as Title
- Add field documentation for Resource

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

@muvaf muvaf left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you @ulucinar ! It's all coming together!!

Comment thread pkg/config/provider.go Outdated
ProviderMetadataPath string
// ProviderMetadata is the scraped provider metadata
// from the corresponding Terraform registry.
ProviderMetadata string

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since we're using this only in NewProvider, would it make sense to not expose it in Provider struct?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Comment thread pkg/pipeline/example.go Outdated
"forProvider": exampleParams,
"providerConfigRef": map[string]interface{}{
"name": "example",
"name": "default",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: we can omit providerConfigRef altogether since it defaults to default.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Comment thread pkg/registry/resource.go Outdated
// NewProviderMetadataFromFile loads metadata from the specified YAML-formatted document
func NewProviderMetadataFromFile(providerMetadata string) (*ProviderMetadata, error) {
metadata := &ProviderMetadata{}
if err := yaml.Unmarshal([]byte(providerMetadata), metadata); err != nil {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since we're converting to []byte anyway, we might consider accepting providerMetadata as []byte in NewProvider. go:embed supports it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done.

ulucinar added 2 commits June 30, 2022 10:31
Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
@ulucinar ulucinar merged commit 17e0597 into crossplane:main Jun 30, 2022
@ulucinar ulucinar deleted the fix-6 branch June 30, 2022 08:41
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.

Configuration of the Example Generation Pipeline

2 participants