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

feat: vmware_distributed_virtual_switch_pvlan_mapping resource #2291

Merged

Conversation

GCHQDeveloper609
Copy link
Contributor

@GCHQDeveloper609 GCHQDeveloper609 commented Oct 22, 2024

Description

Implements #2262 to add a standalone resource for a single PVLAN mapping on a distributed virtual switch, similar in concept to the 'aws_security_group_rule' resource in the AWS provider

Acceptance tests

  • Have you added an acceptance test for the functionality being added?
  • Have you run the acceptance tests on this branch?

Output from acceptance testing:

$ make testacc TESTARGS="-run .*VirtualSwitch.*"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run .*VirtualSwitch.* -test.v -timeout 360m
?       github.com/hashicorp/terraform-provider-vsphere [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/administrationroles    [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/clustercomputeresource  [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/computeresource [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/contentlibrary  [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/customattribute [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/datacenter      [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/datastore       [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/dvportgroup     [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/envbrowse       [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/folder  [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/guestoscustomizations   [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/hostsystem      [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/nsx     [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/ovfdeploy       [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/provider        [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/network [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/resourcepool    [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/spbm    [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/storagepod      [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/structure       [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/utils   [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/testhelper      [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/vappcontainer   [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/virtualmachine  [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/vsanclient      [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/vsansystem      [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/vmworkflow     [no test files]
=== RUN   TestAccDataSourceVSphereDistributedVirtualSwitch_basic
--- PASS: TestAccDataSourceVSphereDistributedVirtualSwitch_basic (2.39s)
=== RUN   TestAccDataSourceVSphereDistributedVirtualSwitch_absolutePathNoDatacenterSpecified
--- PASS: TestAccDataSourceVSphereDistributedVirtualSwitch_absolutePathNoDatacenterSpecified (3.26s)
=== RUN   TestAccDataSourceVSphereDistributedVirtualSwitch_CreatePortgroup
--- PASS: TestAccDataSourceVSphereDistributedVirtualSwitch_CreatePortgroup (2.61s)
=== RUN   TestAccResourceVSphereDistributedVirtualSwitchPvlanMapping_basic
--- PASS: TestAccResourceVSphereDistributedVirtualSwitchPvlanMapping_basic (3.15s)
=== RUN   TestAccResourceVSphereDistributedVirtualSwitch_basic
--- PASS: TestAccResourceVSphereDistributedVirtualSwitch_basic (4.04s)
=== RUN   TestAccResourceVSphereDistributedVirtualSwitch_noHosts
--- PASS: TestAccResourceVSphereDistributedVirtualSwitch_noHosts (2.05s)
=== RUN   TestAccResourceVSphereDistributedVirtualSwitch_removeNIC
--- PASS: TestAccResourceVSphereDistributedVirtualSwitch_removeNIC (3.55s)
=== RUN   TestAccResourceVSphereDistributedVirtualSwitch_standbyWithExplicitFailoverOrder
--- PASS: TestAccResourceVSphereDistributedVirtualSwitch_standbyWithExplicitFailoverOrder (7.31s)
=== RUN   TestAccResourceVSphereDistributedVirtualSwitch_basicToStandbyWithFailover
--- PASS: TestAccResourceVSphereDistributedVirtualSwitch_basicToStandbyWithFailover (6.85s)
=== RUN   TestAccResourceVSphereDistributedVirtualSwitch_upgradeVersion
--- PASS: TestAccResourceVSphereDistributedVirtualSwitch_upgradeVersion (7.72s)
=== RUN   TestAccResourceVSphereDistributedVirtualSwitch_networkResourceControl
--- PASS: TestAccResourceVSphereDistributedVirtualSwitch_networkResourceControl (3.80s)
=== RUN   TestAccResourceVSphereDistributedVirtualSwitch_explicitUplinks
--- PASS: TestAccResourceVSphereDistributedVirtualSwitch_explicitUplinks (2.60s)
=== RUN   TestAccResourceVSphereDistributedVirtualSwitch_modifyUplinks
--- PASS: TestAccResourceVSphereDistributedVirtualSwitch_modifyUplinks (4.54s)
=== RUN   TestAccResourceVSphereDistributedVirtualSwitch_inFolder
--- PASS: TestAccResourceVSphereDistributedVirtualSwitch_inFolder (3.99s)
=== RUN   TestAccResourceVSphereDistributedVirtualSwitch_singleTag
categoryID: urn:vmomi:InventoryServiceCategory:c0e6f110-9a13-4e66-9402-2c8833b14eb6:GLOBAL
--- PASS: TestAccResourceVSphereDistributedVirtualSwitch_singleTag (3.74s)
=== RUN   TestAccResourceVSphereDistributedVirtualSwitch_modifyTags
categoryID: urn:vmomi:InventoryServiceCategory:e0fc535c-85c3-4f91-8144-1acd9c178902:GLOBAL
categoryID: urn:vmomi:InventoryServiceCategory:e0fc535c-85c3-4f91-8144-1acd9c178902:GLOBAL
categoryID: urn:vmomi:InventoryServiceCategory:e0fc535c-85c3-4f91-8144-1acd9c178902:GLOBAL
--- PASS: TestAccResourceVSphereDistributedVirtualSwitch_modifyTags (5.79s)
=== RUN   TestAccResourceVSphereDistributedVirtualSwitch_netflow
--- PASS: TestAccResourceVSphereDistributedVirtualSwitch_netflow (3.11s)
=== RUN   TestAccResourceVSphereDistributedVirtualSwitch_vlanRanges
--- PASS: TestAccResourceVSphereDistributedVirtualSwitch_vlanRanges (3.66s)
=== RUN   TestAccResourceVSphereDistributedVirtualSwitch_singleCustomAttribute
--- PASS: TestAccResourceVSphereDistributedVirtualSwitch_singleCustomAttribute (3.64s)
=== RUN   TestAccResourceVSphereDistributedVirtualSwitch_multiCustomAttribute
--- PASS: TestAccResourceVSphereDistributedVirtualSwitch_multiCustomAttribute (5.15s)
=== RUN   TestAccResourceVSphereHostVirtualSwitch_basic
--- PASS: TestAccResourceVSphereHostVirtualSwitch_basic (3.83s)
=== RUN   TestAccResourceVSphereHostVirtualSwitch_removeNIC
--- PASS: TestAccResourceVSphereHostVirtualSwitch_removeNIC (4.10s)
=== RUN   TestAccResourceVSphereHostVirtualSwitch_noNICs
--- PASS: TestAccResourceVSphereHostVirtualSwitch_noNICs (3.39s)
=== RUN   TestAccResourceVSphereHostVirtualSwitch_badActiveNICList
--- PASS: TestAccResourceVSphereHostVirtualSwitch_badActiveNICList (1.93s)
=== RUN   TestAccResourceVSphereHostVirtualSwitch_badStandbyNICList
--- PASS: TestAccResourceVSphereHostVirtualSwitch_badStandbyNICList (1.28s)
=== RUN   TestAccResourceVSphereHostVirtualSwitch_removeAllNICs
--- PASS: TestAccResourceVSphereHostVirtualSwitch_removeAllNICs (5.07s)
PASS
ok      github.com/hashicorp/terraform-provider-vsphere/vsphere 103.042s
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/viapi   (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/virtualdisk     (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/virtualdevice  (cached) [no tests to run]

Release Note

Release note for CHANGELOG:

FEATURES:
- `resource/vsphere_distributed_virtual_switch_pvlan_mapping`: New resource added to support management of individual PVLAN mapping records on a distributed virtual switch

References

Closes #2262

@GCHQDeveloper609 GCHQDeveloper609 requested a review from a team as a code owner October 22, 2024 14:14
Copy link

hashicorp-cla-app bot commented Oct 22, 2024

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added provider Type: Provider needs-review Status: Pull Request Needs Review labels Oct 22, 2024
@tenthirtyam tenthirtyam self-assigned this Oct 25, 2024
@tenthirtyam tenthirtyam added enhancement Type: Enhancement area/networking Area: Networking labels Oct 25, 2024
@tenthirtyam tenthirtyam added this to the Backlog milestone Oct 25, 2024
@tenthirtyam tenthirtyam force-pushed the feature/pvlan-mapping-resource branch from d36096e to 98b5324 Compare November 5, 2024 12:09
@tenthirtyam tenthirtyam modified the milestones: Backlog, On Deck Nov 5, 2024
@tenthirtyam tenthirtyam self-requested a review November 5, 2024 12:11
Copy link
Collaborator

@spacegospod spacegospod left a comment

Choose a reason for hiding this comment

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

r/distributed_virtual_switch already contains an optional property pvlan_mapping.

Can you explain why you need a separate resource that achieves the same result?
And more importantly - how does the new resource interact and/or conflict with the existing property?

vsphere/resource_vsphere_distributed_virtual_switch.go Outdated Show resolved Hide resolved
@GCHQDeveloper609
Copy link
Contributor Author

Hi @spacegospod, thank you for the review. The reasoning and logic behind the utility of the extra resource is as follows.

In some (most?) vSphere environments, there will be a limited number of Distributed Virtual Switch objects, usually just one. Regardless the number of DVSwitches is limited by the number of physical uplink ports on each host. Therefore DVSwitches are often shared between projects.

However, when each project is its own Terraform root, this poses a challenge of how to define and create the PVLAN mappings each project requires. Take, for example, a Terraform root that deploys a number of web servers all on a community, and a load balancer on promiscuous.

With the current state of the module, that code would look something like this (pseudocode of course):

  networking/
    main.tf/
      resource vsphere_distributed_virtual_switch {
        pvlan_mapping { // for webserver A
            primary = 20
            secondary = 21
            type = promiscuous
        }
        pvlan_mapping {
            primary = 20
            secondary = 21
            type = community
        }
        pvlan_mapping { // for webserver B
            primary = 22
            secondary = 22
            type = promiscuous
        }
        pvlan_mapping {
            primary = 22
            secondary = 23
            type = community
        }
      }
  webserver-a/
    main.tf/
      resource vsphere_distributed_port_group p {
        pvlan_id = 20
      }
      resource vsphere_distributed_port_group c {
        pvlan_id = 21
      }
  webserver-b/
    main.tf/
      resource vsphere_distributed_port_group p {
        pvlan_id = 22
      }
      resource vsphere_distributed_port_group c {
        pvlan_id = 23
      }

Notice how the 'networking' project which 'owns' the DVSwitch has to know about all of the mappings, if I were to add another webserver I would have to go back to the networking project and add the mappings there.

With this new resource the code would look more like:

  networking/
    main.tf/
      resource vsphere_distributed_virtual_switch {
      }
  webserver-a/
    main.tf/
      resource vsphere_distributed_virtual_switch_pvlan_mapping p {
          primary = 20
          secondary = 20
          type = promiscuous
      }
      resource vsphere_distributed_port_group p {
        pvlan_id = 20
      }
      resource vsphere_distributed_virtual_switch_pvlan_mapping c {
          primary = 20
          secondary = 21
          type = community
      }
      resource vsphere_distributed_port_group c {
        pvlan_id = 21
      }
  webserver-b/
    main.tf/
      resource vsphere_distributed_virtual_switch_pvlan_mapping p {
          primary = 22
          secondary = 22
          type = promiscuous
      }
      resource vsphere_distributed_port_group p {
        pvlan_id = 22
      }
      resource vsphere_distributed_virtual_switch_pvlan_mapping c {
          primary = 22
          secondary = 23
          type = community
      }
      resource vsphere_distributed_port_group c {
        pvlan_id = 23
      }

With this design, when an additional webserver project is created, it's not necessary to make any updates to the 'networking' project.

Regarding interoperability, a flag already exists on the DVSwitch object called ignore_other_pvlan_mappings. If this is in use then it will interoperate as expected with this new resource, and the terraform states will not conflict. This is similar to how the aws_security_group_rule (and other) resources behave.

I hope that makes sense but let me know if I can clarify any further

@spacegospod
Copy link
Collaborator

Hi @spacegospod, thank you for the review. The reasoning and logic behind the utility of the extra resource is as follows.

In some (most?) vSphere environments, there will be a limited number of Distributed Virtual Switch objects, usually just one. Regardless the number of DVSwitches is limited by the number of physical uplink ports on each host. Therefore DVSwitches are often shared between projects.

However, when each project is its own Terraform root, this poses a challenge of how to define and create the PVLAN mappings each project requires. Take, for example, a Terraform root that deploys a number of web servers all on a community, and a load balancer on promiscuous.

With the current state of the module, that code would look something like this (pseudocode of course):

  networking/
    main.tf/
      resource vsphere_distributed_virtual_switch {
        pvlan_mapping { // for webserver A
            primary = 20
            secondary = 21
            type = promiscuous
        }
        pvlan_mapping {
            primary = 20
            secondary = 21
            type = community
        }
        pvlan_mapping { // for webserver B
            primary = 22
            secondary = 22
            type = promiscuous
        }
        pvlan_mapping {
            primary = 22
            secondary = 23
            type = community
        }
      }
  webserver-a/
    main.tf/
      resource vsphere_distributed_port_group p {
        pvlan_id = 20
      }
      resource vsphere_distributed_port_group c {
        pvlan_id = 21
      }
  webserver-b/
    main.tf/
      resource vsphere_distributed_port_group p {
        pvlan_id = 22
      }
      resource vsphere_distributed_port_group c {
        pvlan_id = 23
      }

Notice how the 'networking' project which 'owns' the DVSwitch has to know about all of the mappings, if I were to add another webserver I would have to go back to the networking project and add the mappings there.

With this new resource the code would look more like:

  networking/
    main.tf/
      resource vsphere_distributed_virtual_switch {
      }
  webserver-a/
    main.tf/
      resource vsphere_distributed_virtual_switch_pvlan_mapping p {
          primary = 20
          secondary = 20
          type = promiscuous
      }
      resource vsphere_distributed_port_group p {
        pvlan_id = 20
      }
      resource vsphere_distributed_virtual_switch_pvlan_mapping c {
          primary = 20
          secondary = 21
          type = community
      }
      resource vsphere_distributed_port_group c {
        pvlan_id = 21
      }
  webserver-b/
    main.tf/
      resource vsphere_distributed_virtual_switch_pvlan_mapping p {
          primary = 22
          secondary = 22
          type = promiscuous
      }
      resource vsphere_distributed_port_group p {
        pvlan_id = 22
      }
      resource vsphere_distributed_virtual_switch_pvlan_mapping c {
          primary = 22
          secondary = 23
          type = community
      }
      resource vsphere_distributed_port_group c {
        pvlan_id = 23
      }

With this design, when an additional webserver project is created, it's not necessary to make any updates to the 'networking' project.

Regarding interoperability, a flag already exists on the DVSwitch object called ignore_other_pvlan_mappings. If this is in use then it will interoperate as expected with this new resource, and the terraform states will not conflict. This is similar to how the aws_security_group_rule (and other) resources behave.

I hope that makes sense but let me know if I can clarify any further

This actually makes sense, thank you

@tenthirtyam tenthirtyam force-pushed the feature/pvlan-mapping-resource branch from 06e2261 to 332d8be Compare November 6, 2024 15:04
@tenthirtyam tenthirtyam modified the milestones: On Deck, v.2.11.0 Nov 6, 2024
@tenthirtyam tenthirtyam force-pushed the feature/pvlan-mapping-resource branch from 332d8be to 555d09b Compare November 7, 2024 21:41
@tenthirtyam tenthirtyam merged commit da5dd22 into hashicorp:main Nov 7, 2024
2 checks passed
@GCHQDeveloper609
Copy link
Contributor Author

Thanks all! My first ever proper open source contribution is complete 😎

@tenthirtyam tenthirtyam removed the needs-review Status: Pull Request Needs Review label Dec 4, 2024
Copy link

github-actions bot commented Jan 4, 2025

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/networking Area: Networking enhancement Type: Enhancement provider Type: Provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for defining DVSwitch PVLAN mappings as an independent resource
4 participants