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 of polymorphic struct in reflect lib #1222

Merged
merged 7 commits into from
Jun 12, 2024

Conversation

wsquan171
Copy link
Contributor

@wsquan171 wsquan171 commented May 31, 2024

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

  • Declare PolymorphicType as wrapped
  • Provide ResourceTypeMap, where the key is all potential tf sub-object (in this case, cat and coffee), and value is the ResourceType value reported from NSX
  • Optional TypeIdentifier providing the SDK field name and API JSON name for the attr to identify the resource type. Defaults to use ResourceType and resource_type respectively
  • The schema of poly_struct should include all sub-objects, while indicating them as mutal exclusive using ConflictsWith

At sub-object level

  • ReflectType should be the golang type directly matching with this sub class
  • BindingType is required from SDK so it can be statically converted as *data.StructValue(or slice equivalant)
  • TypeIdentifier to match with the wrapping level
  • SdkFieldName will be ignored

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:

  • Declare PolymorphicType as flat
  • ReflectType matching with the corresponding sub class
  • BindingType matching with ReflectType
  • Optional TypeIdentifier
  • ResourceType

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

@annakhm
Copy link
Collaborator

annakhm commented May 31, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

wsquan171 commented Jun 7, 2024

@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
Copy link
Collaborator

annakhm commented Jun 8, 2024

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?

For certain polymorphic resources, typically minor child classes that
are not a standalone resource in NSX, the type identifier could be value
other than "ResourceType" / "resource_type".

This change adds support of declaring the type identifier in pair.

Signed-off-by: Shawn Wang <[email protected]>
This change adds support of the "flat" polymorphic list conversion,
where different type in a polymorphic SDK list is combined and
mapped to a separate tf schema attr.

Signed-off-by: Shawn Wang <[email protected]>
Signed-off-by: Shawn Wang <[email protected]>
Signed-off-by: Shawn Wang <[email protected]>
@wsquan171
Copy link
Contributor Author

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

@wsquan171 wsquan171 merged commit e43e85b into vmware:master Jun 12, 2024
3 checks passed
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