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 Edge Gateway DHCP relay configuration support #416

Merged
merged 20 commits into from
Dec 10, 2019

Conversation

Didainius
Copy link
Collaborator

This PR adds support for NSX-V Edge Gateway DHCP relay configuration which represents relay DHCP relay configuration tab in edge gateway.

Note. This resource is a bit "special" because it actually does not create any objects and there is no real ID for it therefore there is a fake ID created which includes Edge Gateway ID and additional hardcoded string part.

@ghost ghost added size/XL and removed size/XXL labels Dec 6, 2019
website/docs/r/nsxv_dhcp_relay.markdown Outdated Show resolved Hide resolved

ip_addresses = ["1.1.1.1", "2.2.2.2"]
domain_names = ["servergroups.domainname.com", "other.domain.com"]
ip_sets = [vcd_ipset.myset1.name, vcd_ipset.myset2.name]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking that it may be clearer for new users if we defined these IP set resources in this example as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added

website/docs/r/nsxv_dhcp_relay.markdown Show resolved Hide resolved
website/docs/d/nsxv_dhcp_relay.markdown Outdated Show resolved Hide resolved
vcd/resource_vcd_nsxv_dhcp_relay_test.go Outdated Show resolved Hide resolved
Type: schema.TypeSet,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"org_network": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Two comments:

1.) It seems, in H5 UI in addition to Org networks you can select external network:

Screen Shot 2019-12-06 at 16 30 20

Are you sure it's only org networks?

2.) I'm also thinking whether we shouldn't rename org_network -> org_network_name for clarity. It would be similar to network_name in vcd_nsxv_snat resource.

3.) And continuing on the train of thought of (1) and (2), if it turns out that we can select external network, then we could just name the field network_name as in NAT rules.

Copy link
Collaborator Author

@Didainius Didainius Dec 6, 2019

Choose a reason for hiding this comment

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

Two comments:

1.) It seems, in H5 UI in addition to Org networks you can select external network:

Screen Shot 2019-12-06 at 16 30 20

Are you sure it's only org networks?

Yes. It does allow to choose, but here is what happens:
image

2.) I'm also thinking whether we shouldn't rename org_network -> org_network_name for clarity. It would be similar to network_name in vcd_nsxv_snat resource.

Can do. I kind of thought between this and that. In some places we don't ask for "name", in some we do. (like edge_gateway also asks for name, but we don't mention it).

3.) And continuing on the train of thought of (1) and (2), if it turns out that we can select external network, then we could just name the field network_name as in NAT rules.

I can make it broader - maybe in future it works although for my limited understanding this wouldn't make sense. The general point of relay is to "forward" DHCP request messages to defined servers outside of NSX. One shouldn't be able to forward DHCP request messages from external network. I might be wrong though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, so we have three choices:

  • org_network
  • org_network_name
  • network_name

@Didainius @dataclouder @vbauzysvmware your votes?

To me network_name looks most consistent with the same field from NSX-V NAT rules. However, it doesn't reflect the point that these are only Org networks.

Copy link
Contributor

Choose a reason for hiding this comment

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

network_name is more generic and will work even if we end up supporting different kinds of network

Copy link
Contributor

Choose a reason for hiding this comment

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

I am fine with network_name

vcd/resource_vcd_nsxv_dhcp_relay.go Outdated Show resolved Hide resolved
@ghost ghost added size/XXL and removed size/XL labels Dec 9, 2019
Copy link
Contributor

@vbauzys vbauzys left a comment

Choose a reason for hiding this comment

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

Overall looks good

vcd/resource_vcd_nsxv_dhcp_relay.go Outdated Show resolved Hide resolved
vcd/resource_vcd_nsxv_dhcp_relay.go Outdated Show resolved Hide resolved
vcd/resource_vcd_nsxv_dhcp_relay.go Outdated Show resolved Hide resolved
vcd/resource_vcd_nsxv_dhcp_relay.go Outdated Show resolved Hide resolved
vcd/resource_vcd_nsxv_dhcp_relay.go Outdated Show resolved Hide resolved
vcd/resource_vcd_nsxv_dhcp_relay.go Outdated Show resolved Hide resolved
@ghost ghost added size/XL and removed size/XXL labels Dec 10, 2019
Copy link
Contributor

@vbauzys vbauzys left a comment

Choose a reason for hiding this comment

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

LGTM now

@ghost ghost added size/XXL and removed size/XL labels Dec 10, 2019
Copy link
Contributor

@dataclouder dataclouder left a comment

Choose a reason for hiding this comment

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

LGTM

@Didainius Didainius merged commit 8e338d3 into vmware:master Dec 10, 2019
@Didainius Didainius deleted the vcd_dhcp_relay branch December 10, 2019 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants