Support for configuring blocks with optional parameters in attributes-as-blocks mode

[sergenyalcin]

Description of your changes

Fixes #232

With this PR, configuring blocks with optional parameters will be possible. In terraform schemas, this logic works with a field: ConfigMode. If the value of the ConfigMode field is SchemaConfigModeAttr, we have to provide values for all options. Please see the details: https://stackoverflow.com/questions/69079945/terraform-inappropriate-value-for-attribute-ingress-while-creating-sg/69080432#69080432

Reference document: https://www.terraform.io/language/attr-as-blocks

In this PR, a new check was added for tfTag. If the parent field is in the SchemaConfigModeAttr, the omitempty tag is deleted from tftags of subfields.

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

This PR was tested in provider-jet-aws side by provisioning, updating and deleting SecurityGroup resource. In tests the manifest was used:

apiVersion: ec2.aws.jet.crossplane.io/v1alpha2
kind: SecurityGroup
metadata:
    name: example-securitygroup
spec:
  forProvider:
    name: example-securitygroup-sg
    description: "i am a description"
    region: us-west-2
    vpcId: vpc-somevpc
    ingress:
    - fromPort: 6379
      toPort: 6379
      protocol: tcp
      description: "i am also a description"
      cidrBlocks:
      - 10.0.0.0/16
      ipv6CidrBlocks: []
      self: false
      prefixListIds: []
      securityGroups: []
    tags:
      managed-by: crossplane
  providerConfigRef:
    name: jet-aws-provider

Please see the original issue in jet-aws side: crossplane-contrib/provider-jet-aws#157

[sergenyalcin]

I checked if the ConfigMode value is passed from json output to schemas. In json output, unfortunately the there is no ConfigMode. Therefore, it seems that we need to handle this situation as a per resource configuration.

I have gathered statistics showing which fields are working in this mode:

AWS:

• 10 Fields
• 6 Unique Resources:
  -- aws_default_route_table
  -- aws_default_security_group
  -- aws_network_acl
  -- aws_route_table
  -- aws_security_group
  -- aws_emr_cluster

Azure:

• 54 Fields
• 26 Unique Resources:
  -- azurerm_mssql_server
  -- azurerm_sql_server
  -- azurerm_mssql_database
  -- azurerm_sql_database
  -- azurerm_cognitive_account
  -- azurerm_kubernetes_cluster
  -- azurerm_automation_runbook
  -- azurerm_storage_account_network_rules
  -- azurerm_app_service
  -- azurerm_app_service_slot
  -- azurerm_function_app
  -- azurerm_function_app_slot
  -- azurerm_eventhub_namespace
  -- azurerm_network_connection_monitor
  -- azurerm_network_security_group
  -- azurerm_subnet
  -- azurerm_virtual_network
  -- azurerm_iothub
  -- azurerm_eventgrid_domain
  -- azurerm_eventgrid_topic
  -- azurerm_container_registry
  -- azurerm_site_recovery_replicated_vm
  -- azurerm_api_management
  -- azurerm_batch_pool
  -- azurerm_synapse_workspace
  -- azurerm_kusto_cluster

So, the usage of this mode does not seem very common. However, we also cannot say the occurrence rate is very low for example, Azure provider. Since there is no automatic solution to this problem, the resources will need to be configured for the solution. At this point, it can be discussed how to handle the configuration.

My proposal is to manipulate the schema during resource configurations and mark/set the fields that work in this mode with their correct values. Thus, the change on the terrajet side can be used in its current form. At the same time, the size and complexity of the change will be very minimal.

Example usage of my proposal while resource configuration:

r.TerraformResource.Schema["ingress"].ConfigMode = schema.SchemaConfigModeAttr
r.TerraformResource.Schema["egress"].ConfigMode = schema.SchemaConfigModeAttr

What are your thoughts @turkenh @muvaf @ulucinar ?

[muvaf]

@sergenyalcin Thank you for that impact analysis! I think your proposal looks promising but it'd also mean that we can't move to CLI schema representation in the builder.

I looked at the reverse conversion function here and it seems like TF CLI does some things depending on the config mode. Have you had the chance to look at that to see if there is anything we can derive the config mode?

[sergenyalcin]

I looked at the reverse conversion function here and it seems like TF CLI does some things depending on the config mode. Have you had the chance to look at that to see if there is anything we can derive the config mode?

I checked this code block. As you said that in this function, there are some things depending on the ConfigMode values of fields. So, at first glance, it seems that doing the work done here in reverse may enable us to reach the relevant ConfigMode value.

When we check this code block in details, when the value of ConfigMode value is SchemaConfigModeAttr (this is the case we are trying to resolve), then the coreConfigSchemaAttribute() is calling. However, this function is calling for another cases:

• https://github.com/hashicorp/terraform-plugin-sdk/blob/7e0a333644f1971a936995677b7a106140a0659f/helper/schema/core_schema.go#L86
• https://github.com/hashicorp/terraform-plugin-sdk/blob/7e0a333644f1971a936995677b7a106140a0659f/helper/schema/core_schema.go#L99

In other words, we see that the work done according to the ConfigMode value is not only done specifically for this value, but also in certain other cases. Therefore, trying to reach the ConfigMode value by running the work done here backwards may lead us to wrong results in some cases.

For example, if the schema element is a map and the schema's Elem is a Resource object, the same function is called. So I'm not sure it's very possible to derive ConfigMode from the results in the generated schema. Please write if I'm missing a point.

@sergenyalcin Thank you for that impact analysis! I think your proposal looks promising but it'd also mean that we can't move to CLI schema representation in the builder.

I got your point. So we may need to consider another configuration option to handle this problem. For example, while doing manuel configuration, the relevant fieldPath value can be passed for a configuration option, and we can check this fieldPath in builder.go instead of checking the ConfigMode value.

[turkenh]

@sergenyalcin @muvaf just opened a fix/change setting ConfigMode during cli schema to sdk schema conversion. I've tested using existing provider-aws resources on main and looking good.

[sergenyalcin]

@turkenh Since this PR #250 was merged, I think we may consider to review and also merge this.

[turkenh]

The number of parameters that this method takes is annoying but given that we would refactor this part soon that should be fine for now.

[github-actions]

Successfully created backport PR #254 for release-0.4.