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

NAT rule support IDs and external or Org networks #282

Merged
merged 29 commits into from
Jul 22, 2019
Merged

NAT rule support IDs and external or Org networks #282

merged 29 commits into from
Jul 22, 2019

Conversation

vbauzys
Copy link
Contributor

@vbauzys vbauzys commented Jul 15, 2019

Ref: terraform-providers/terraform-provider-vcd#244
Ref: https://github.com/terraform-providers/terraform-provider-vcd/pull/224
Depends on: vmware/go-vcloud-director#216

Uses nat rule Id's for handling rules. Also network_name and network_type used to handle which network to use. Old way left to be intact (allows only creating rule for first external network)

vbauzys added 13 commits June 20, 2019 12:37
Signed-off-by: Vaidotas Bauzys <[email protected]>

# Conflicts:
#	vcd/resource_vcd_org_vdc.go
Signed-off-by: Vaidotas Bauzys <[email protected]>
Signed-off-by: Vaidotas Bauzys <[email protected]>
Signed-off-by: Vaidotas Bauzys <[email protected]>
Signed-off-by: Vaidotas Bauzys <[email protected]>
Signed-off-by: Vaidotas Bauzys <[email protected]>
…roviders/terraform-provider-vcd into vdc-metadata
Signed-off-by: Vaidotas Bauzys <[email protected]>
Signed-off-by: Vaidotas Bauzys <[email protected]>
Signed-off-by: Vaidotas Bauzys <[email protected]>

# Conflicts:
#	CHANGELOG.md
#	go.mod
#	go.sum
#	vendor/github.com/vmware/go-vcloud-director/v2/govcd/edgegateway.go
#	vendor/modules.txt
Signed-off-by: Vaidotas Bauzys <[email protected]>
@vbauzys vbauzys changed the title Nat ext net support NAT rule support IDs and external or Org networks Jul 15, 2019
Signed-off-by: Vaidotas Bauzys <[email protected]>
Copy link
Collaborator

@Didainius Didainius left a comment

Choose a reason for hiding this comment

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

Round 1

CHANGELOG.md Outdated Show resolved Hide resolved
vcd/resource_vcd_dnat.go Outdated Show resolved Hide resolved
if externalNetwork != nil && externalNetwork != (&govcd.ExternalNetwork{}) {
d.Set("network_type", "ext")
} else {
return fmt.Errorf("issue to find network")
Copy link
Collaborator

Choose a reason for hiding this comment

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

A bit more context would help - network name, network type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

vcd/resource_vcd_dnat.go Outdated Show resolved Hide resolved
vcd/resource_vcd_dnat_test.go Show resolved Hide resolved
website/docs/r/snat.html.markdown Outdated Show resolved Hide resolved
website/docs/r/dnat.html.markdown Outdated Show resolved Hide resolved
website/docs/r/dnat.html.markdown Outdated Show resolved Hide resolved
website/docs/r/snat.html.markdown Outdated Show resolved Hide resolved
vcd/resource_vcd_snat.go Show resolved Hide resolved
vbauzys added 2 commits July 17, 2019 11:49
Signed-off-by: Vaidotas Bauzys <[email protected]>
Signed-off-by: Vaidotas Bauzys <[email protected]>
@ghost ghost added size/XXL and removed size/XL labels Jul 17, 2019
vbauzys added 2 commits July 18, 2019 13:08
Signed-off-by: Vaidotas Bauzys <[email protected]>
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.

Some changes needed. Testing ...

vcd/resource_vcd_dnat.go Outdated Show resolved Hide resolved
vcd/resource_vcd_dnat.go Outdated Show resolved Hide resolved
vcd/resource_vcd_dnat.go Outdated Show resolved Hide resolved
vbauzys added 3 commits July 19, 2019 10:56
Signed-off-by: Vaidotas Bauzys <[email protected]>
Signed-off-by: Vaidotas Bauzys <[email protected]>
Signed-off-by: Vaidotas Bauzys <[email protected]>
CHANGELOG.md Outdated Show resolved Hide resolved
vcd/resource_vcd_dnat.go Show resolved Hide resolved
vbauzys added 2 commits July 19, 2019 13:25
Signed-off-by: Vaidotas Bauzys <[email protected]>
Signed-off-by: Vaidotas Bauzys <[email protected]>
website/docs/r/snat.html.markdown Show resolved Hide resolved
website/docs/r/dnat.html.markdown Show resolved Hide resolved
website/docs/r/snat.html.markdown Show resolved Hide resolved
vcd/resource_vcd_dnat.go Show resolved Hide resolved
vcd/resource_vcd_snat_test.go Outdated Show resolved Hide resolved
vbauzys added 2 commits July 19, 2019 15:38
Signed-off-by: Vaidotas Bauzys <[email protected]>
Signed-off-by: Vaidotas Bauzys <[email protected]>
@vbauzys
Copy link
Contributor Author

vbauzys commented Jul 19, 2019

Bump to tagged version of govcd

@Didainius
Copy link
Collaborator

Didainius commented Jul 19, 2019

I have ext and org networks with the same name.
This test fails in such case:

$ go test -tags functional -v -run "TestAccVcdDNAT_WithExtNetw" .
Connecting to https://192.168.1.109/api
as user administrator@System
Checking resources to create for test suite...
Skipping catalog creation - found preconfigured one: my-catalog
Catalog found successfully
Skipping catalog item creation - found preconfigured one: photon-os
Catalog item found successfully
=== RUN   TestAccVcdDNAT_WithExtNetw
--- FAIL: TestAccVcdDNAT_WithExtNetw (39.59s)
    testing.go:561: Step 1, expected error:

        Check failed: Check 3/8 error: vcd_dnat.TestAccVcdDNAT_WithExtNetw: Attribute 'network_type' expected "ext", got "org"

        To match:

        After applying this step and refreshing, the plan was not empty:


FAIL

If I remove the network in org - tests start passing. This probably hints at incorrect lookup (at least in update case)

@vbauzys
Copy link
Contributor Author

vbauzys commented Jul 22, 2019

TestAccVcdDNAT_WithExtNetw

Well. Resource allows to change network type ext or org - so when I read/refresh I need to check which type it is. Now firstly search for org when for ext network if not found. I think it's ok - if anyone see value, so only one additional approach I see is throw error with complain that org and ext networks with same name exists.

@Didainius
Copy link
Collaborator

TestAccVcdDNAT_WithExtNetw

Well. Resource allows to change network type ext or org - so when I read/refresh I need to check which type it is. Now firstly search for org when for ext network if not found. I think it's ok - if anyone see value, so only one additional approach I see is throw error with complain that org and ext networks with same name exists.

Could we lookup the network based on it's type? If it was an ext then it should find and try to use ext?

@vbauzys
Copy link
Contributor Author

vbauzys commented Jul 22, 2019

TestAccVcdDNAT_WithExtNetw

Well. Resource allows to change network type ext or org - so when I read/refresh I need to check which type it is. Now firstly search for org when for ext network if not found. I think it's ok - if anyone see value, so only one additional approach I see is throw error with complain that org and ext networks with same name exists.

Could we lookup the network based on it's type? If it was an ext then it should find and try to use ext?
e.g.
With terraform we created rule id 6565 org with name1. Using UI we changed rule 6565 tu use ext with name2. And in your case you have also created create network org with name name2. So type can be changed too.

found = true
}
if orgVdcNetwork != nil && extNetwErr == nil {
return fmt.Errorf("find external network or org VCD network with same name: %s", natRule.GatewayNatRule.Interface.Name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("find external network or org VCD network with same name: %s", natRule.GatewayNatRule.Interface.Name)
return fmt.Errorf("found external network or org VCD network with same name: %s", natRule.GatewayNatRule.Interface.Name)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

Signed-off-by: Vaidotas Bauzys <[email protected]>
Copy link
Collaborator

@Didainius Didainius left a comment

Choose a reason for hiding this comment

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

LGTM. Having duplicate networks now fails as I would expect. Test suite works.

Copy link
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

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

LGTM

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

@vbauzys vbauzys merged commit e184043 into vmware:master Jul 22, 2019
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