vmware / terraform-provider-nsxt

Terraform VMware NSX-T provider
https://www.terraform.io/docs/providers/nsxt/
Other
123 stars 80 forks source link

Add support of polymorphic struct in reflect lib #1222

Closed wsquan171 closed 1 month ago

wsquan171 commented 1 month ago

This PR adds support of polymorphic struct for the reflect lib. Two approaches used in existing TF resource schema are supported in this PR.

Dedicated attribute wrapping polymorphic classes

Suppose an attr poly_struct can accept two types: cat and coffee, the "wrapped" format expects the followings:

Suppose poly_struct is a list, and resource is defined as below:

resource "fake_poly_res" "test" {
  poly_struct {
    cat {
      name = "matcha"
      age  = 1
    }
  }

  poly_struct {
    coffee {
      name      = "latte"
      is_decaf  = false
    }
  }
}

metadata definition expects the followings:

At poly_struct level

At sub-object level

No wrapping level ("flat")

Similiarly, if the SDK field is a polymorphic list, it's also possible to express elements of the same child class as a separate terraform schema attr. In this case, the resource is defined as below:

resource "fake_poly_res" "test" {
  cat {
    name = "oolong"
    age  = 2
  }

  coffee {
    name      = "mocha"
    is_decaf  = false
  }
}

In this case, multiple attrs will be combined into one SDK list. For each object, the followings are expected:

Refer to nsxt/metadata/metadata_poly_test.go for the full example for both use cases.

annakhm commented 1 month ago

Is poly_struct a necessary building block in polymorphic support? Unfortunately resources we have today do not have this wrapper. For example, service resource with polymorphic entries may look something like:

resource "nsxt_policy_service" "x" {
  display_name = "S1"

  l4_port_set_entry {
    display_name      = "TCP80"
    protocol          = "TCP"
    destination_ports = ["80"]
  }
}

I understand this would mean that we will not be able to refactor most existing resources with the new library.

wsquan171 commented 1 month ago

Is poly_struct a necessary building block in polymorphic support? Unfortunately resources we have today do not have this wrapper. For example, service resource with polymorphic entries may look something like:

For the current approach this is true. For some resources like you mentioned, poly_struct is the additional layer, however, for some others the inner layer is the extra one (especially when most attrs are shared across the child classes on NSX)

The problem here to address is that we want an explicit (and standard) way to express the two following items:

  1. flag that an attr should be converted to some fields inside a polymorphic struct / list in the sdk
  2. which type (resource type / golang type) is specific to which tf schema.

If we remove the poly_struct layer, the sdk name need to be repeated for all child attrs in tf schema (i.e. in the test example cat and coffee), along with the mapping of tf attr name to nsx resource type. I don't think this will fit all current resources, as we don't currently have a standard approach to polymorphic fields. However, if this is a fit for majority of cases, I think it would worth it to make the change. Do you think the new format looks better?

wsquan171 commented 1 month ago

@annakhm I've added support for both formats: with or without an wrapping layer. Also cleaned up some exisiting code.

As summerized in the internal doc, this should be able to cover >50% of exisiting resource that involves polymorphism. The ones needs further work (probably a future PR when needed) are:

  1. Flat with explicit type: attrs from different class are mixed at the same level, and an explict type in schema tells which type to convert this to. In most cases, this approach is only used either when most attrs are shared across all child classes, or all the child classes are really simple
  2. Static conversion. This just means an extra step to convert some field into structValue before setting. However, I'm thinking if we should introduce some custom transformer funcs to make this more generic, so we can covers those cases with "degenerate" child class.
annakhm commented 1 month ago

Looks great and also thanks for the thorough test coverage! The only thing I think can be improved it tests is nested polymorphic structure (unless i missed it). Is is possible to cover this?

wsquan171 commented 1 month ago

@annakhm Thanks. Let's track the test imporvements in a follow-up PR