-
Notifications
You must be signed in to change notification settings - Fork 113
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
Introduce 'vcd_external_network_v2' and dependencies with NSX-T support #560
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass.
Required: true, | ||
Description: "ID of NSX-T manager.", | ||
}, | ||
"is_assigned": &schema.Schema{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assigned
or used
in comments explaining the complexity you are using used
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but used
is somewhat "informal" for an attribute to me. We could probably discuss consumed
if that sounds better.
cc @dataclouder, @lvirbalas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dataclouder @lvirbalas your input still would be valuable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assigned
is good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_assigned
is fine for me in this context. BTW, it's TypeString
;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be TypeBool
|
||
ip_scope { | ||
gateway = "192.168.30.49" | ||
netmask = "24" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May the example be broken? I get these errors with it:
$ terraform apply
Error: Missing required argument
on nsx-t.tf line 90, in resource "vcd_external_network_v2" "ext-net-nsxv":
90: ip_scope {
The argument "prefix_length" is required, but no definition was found.
Error: Unsupported argument
on nsx-t.tf line 92, in resource "vcd_external_network_v2" "ext-net-nsxv":
92: netmask = "24"
An argument named "netmask" is not expected here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. Leftover from old example. Changed.
## NSX-T Network | ||
|
||
* `nsxt_manager_id` - (Required) NSX-T manager ID. Can be looked up using [`vcd_nsxt_manager`](/docs/providers/vcd/d/nsxt_manager.html) data source. | ||
* `nsxt_tier0_router_id` - (Required) NSX-T Tier-0 router ID. Can be looked up using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In contrast to this "normal" Tier-0 router, where/when Terraform user should use the VRF Tier-0 router?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact it can be supplied here as well and the datasource does look it up.
}, | ||
}, | ||
}, | ||
"nsxt_network": &schema.Schema{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason to group nsxt_manager_id
and nsxt_tier0_router_id
into a TypeSet
with MaxItems: 1
? Curious, is there a benefit compared to leaving these parameters flat. The reason I'm asking, because we had hit issues with the TypeSet
in the past, so just would like to get confirmation that this won't be the case here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason was that adjacent block vsphere_network can have more than one and is a TypeSet. I can change this one to TypeList. We can switch back if it allows to specify more than 1 in future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! I haven't realized it relates to NSX-V's vsphere_network
and that more than one can be defined.
It's OK, if you see no technical issues with TypeSet
, let's leave it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR introduces resource and data source
vcd_external_network_v2
which is using new NSX-T capable OpenAPI.Note. This resource requires API version of at least V33.0 (VCD 10.0+) to work.
The resource itself covers both NSX-V and NSX-T support as does the underlying API. Such improvements in comparison to
vcd_external_network
resource should be also there:TypeSet
everywhere so issues (resource/vcd_external_network
should use TypeSet instead of TypeList forip_scope
blocks #395, Issue while creating External Network with multiple IP Pools and Static IP Pools using dynamic block #503) currently present invcd_external_network
should not be here.Update
The resource itself is very much similar to original
vcd_external_network
but has a few notable changes:ip_scope
blocks useprefix_length
instead ofnetmask
(dictated by API itself)ip_scope
blocks have optionalenabled
flagretain_net_info_across_deployments
is not present in API therefore this resource does not expose itvsphere_network
andnsxt_network
blocks useID
references instead of names. For this purpose there are 4 new datasources:vcd_vcenter
vcd_portgroup
vcd_nsxt_manager
vcd_nsxt_tier0_router
Additional testing functions introduced:
testCheckMatchOutput
allows to checkoutput
value by regexptestCheckOutputNonEmpty
allows to check ifoutput
value is not emptyNotes:
dns1
,dns2
anddns_suffix
fields therefore it is not supported and will throw error.vcd/sample_vcd_test_config.json
has a few new fields required to cover NSX-T testing