DNAT: allow different src/translated port #14437

Boran · 2017-05-12T14:25:08Z

Initial work on #12384.

Building with UKCloud/govcloudair@81750aa

The corresponding issue in govair is vmware-archive/govcloudair#37 and UKCloud/govcloudair#3

Added a new (optional) param translated_port, keeping the existing port as the src port.
If translated_port is not specified use port - allows backward compatibility.
The name translated_port was selected because it is closer to the api and is less confusing than internal/external for example.

Does the output have to have a specific format?
vcd_dnat.test-ssh1: Creation complete (ID: 193.1.1.1:22 > 172.16.8.36:40022)
How/where does one define the version of govcloudair to pull in, so that travis tests work?

Boran · 2017-05-22T13:09:27Z

@robcoward could you have a look please, anything else needed?

FYI
15:04 $ make test TEST=./builtin/providers/vcd
.....
echo ./builtin/providers/vcd |
xargs -t -n4 go test -timeout=60s -parallel=4
go test -timeout=60s -parallel=4 ./builtin/providers/vcd
ok github.com/hashicorp/terraform/builtin/providers/vcd 0.031s

Boran · 2017-05-23T13:27:27Z

@ewypych could you look too please? I integrated your code for the test, as that part was missing.

ewypych · 2017-05-23T15:31:48Z

@Boran LGTM. If everything works properly, I don't mind merging. 👍 Hope it will be merged. 😄

bobbydeveaux · 2017-05-26T10:36:26Z

Awesome- looks great. @robcoward has left UKCloud now so not sure he'll be checking these alerts.

Would be great if you could merge please @ewypych

robcoward · 2017-05-26T10:48:21Z

@Boran LGTM. Like @bobbydeveaux said, I no longer work for UKCloud so dont have the infrastructure to test this against, but the logic looks sound.
👍

ewypych · 2017-05-26T15:04:03Z

@robcoward @bobbydeveaux If you mean whether it works with vCloud, I can compile and test it on vCloud Director (8.2 and 8.0 for SP, have access only to these versions). Unfortunately, Sunday evening is the first possible date.

I've made my version (didn't notice that @Boran made it first) and it works fine with the infrastructure (you can look at the code here #14709). Code is similar, so I assume that everything is great and there should be no problems.

And Bobby - bad news, I haven't permissions to merge, I'm just an ordinary contributor. 😄 But I have the same view - merging would be great!

Boran · 2017-05-29T09:35:15Z

I have tested on vCloud Director 8.02.4348775

@bobbydeveaux: Have you merge rights?

bobbydeveaux · 2017-05-29T20:00:46Z

Afraid not. I can merge anything UKCloud, but not Hashicorp! :)

Boran · 2017-05-31T07:18:51Z

@jbardin Could you help, or know who could help us get this PR merged?

jbardin

Thanks for the contribution!

There's a few comments in-line. We also need the output from an acceptance test run.

I'll hand this off to someone who does more providers to give it the final review and merge.!

jbardin · 2017-05-31T14:04:28Z

builtin/providers/vcd/resource_vcd_dnat.go

@@ -77,7 +89,7 @@ func resourceVcdDNATCreate(d *schema.ResourceData, meta interface{}) error {
 		return fmt.Errorf("Error completing tasks: %#v", err)
 	}

-	d.SetId(d.Get("external_ip").(string) + "_" + portString)
+	d.SetId(d.Get("external_ip").(string) + ":" + portString + " > " + d.Get("internal_ip").(string) + ":" + trlateportString)


Is changing the ID going effect existing resources?

Yes, but the current ID is not unique and is a bug if left so (in my opinion).

jbardin · 2017-05-31T14:09:27Z

builtin/providers/vcd/resource_vcd_dnat.go

@@ -49,6 +55,10 @@ func resourceVcdDNATCreate(d *schema.ResourceData, meta interface{}) error {
 	vcdClient.Mutex.Lock()
 	defer vcdClient.Mutex.Unlock()
 	portString := getPortString(d.Get("port").(int))
+	trlateportString := portString // default


Can we make this variable a little easier to read? translatedPortString isn't much longer.

sure, change committed.

jbardin · 2017-05-31T14:15:42Z

builtin/providers/vcd/resource_vcd_dnat_test.go

 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckVcdDNATExists("vcd_dnat.bar", &e),
 					resource.TestCheckResourceAttr(
 						"vcd_dnat.bar", "external_ip", os.Getenv("VCD_EXTERNAL_IP")),
 					resource.TestCheckResourceAttr(
-						"vcd_dnat.bar", "port", "77"),
+						"vcd_dnat.bar", "port", "7777"),


Since existing configs are still valid, I'd rather leave the existing test and add a new test case with a translated port.

Boran · 2017-06-01T09:03:49Z

@bobbydeveaux I merged the latest terraform master + govcloudair before starting acceptance tests. On compiling now getting

Stderr: # github.com/hashicorp/terraform/vendor/github.com/ukcloud/govcloudair/types/v56
vendor/github.com/ukcloud/govcloudair/types/v56/types.go:69: undefined: LinkList

Any idea where govair is expecting that type to come from?

bobbydeveaux · 2017-06-01T09:16:00Z

@Boran I had this before.. it's a pain and I'm unsure why go vendor requires this step, but you need to fetch all 3 directories in govcloudair:

govendor fetch github.com/ukcloud/govcloudair
govendor fetch github.com/ukcloud/govcloudair/types
govendor fetch github.com/ukcloud/govcloudair/types/v56

I'm guessing you only did the first one, as the LinkList is in types/v56/link.go

HTH! :)

Boran · 2017-06-01T10:42:01Z

Hmm. It took 20mins to get those, but it helped thanks!
are there other types to get ?

vendor/github.com/ukcloud/govcloudair/api_vcd.go:15: undefined: Org
vendor/github.com/ukcloud/govcloudair/vdc.go:17: undefined: Client

bobbydeveaux · 2017-06-01T10:44:19Z

Very odd. You've done all three?

Boran · 2017-06-01T11:49:06Z

yes, and doing a "grep -R Client github.com/hashicorp/terraform/vendor/github.com/ukcloud/govcloudair" I do not see where Client is defined.
(and Govendor take 30 minutes).

13:11 $ govendor fetch github.com/UKCloud/govcloudair && govendor fetch github.com/ukcloud/govcloudair/types && govendor fetch github.com/ukcloud/govcloudair/types/v56

13:39 $ XC_OS="darwin" XC_ARCH="amd64" make bin
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/06/01 13:39:33 Generated command/internal_plugin_list.go
==> Removing old directory...
==> Building...
Number of parallel builds: 7
-->    darwin/amd64: github.com/hashicorp/terraform
1 errors occurred:
--> darwin/amd64 error: exit status 2
Stderr: # github.com/hashicorp/terraform/vendor/github.com/ukcloud/govcloudair
vendor/github.com/ukcloud/govcloudair/api_vcd.go:15: undefined: Org
vendor/github.com/ukcloud/govcloudair/vdc.go:17: undefined: Client

bobbydeveaux · 2017-06-01T11:59:50Z

terraform/vendor/github.com/ukcloud/govcloudair/api.go:19

// Client provides a client to vCloud Air, values can be populated automatically using the Authenticate method.
type Client struct {
APIVersion string // The API version required
VCDToken string // Access Token (authorization header)
VCDAuthHeader string // Authorization header
VCDVDCHREF url.URL // HREF of the backend VDC you're using
Http http.Client // HttpClient is the client to use. Default will be used if not provided.
}

bobbydeveaux · 2017-06-01T12:01:31Z

Does make plugin-dev PLUGIN=provider-vcd work for you?

Personally, I use; make dev

Boran · 2017-06-01T12:30:44Z

There is no api.go
ls vendor/github.com/ukcloud/govcloudair/
LICENSE Makefile api_vcd.go edgegateway.go types vapp.go vdc.go

I realise now how I did this before:
mv vendor/github.com/ukcloud/govcloudair vendor/github.com/ukcloud/govcloudair.$$
and checkout govcloudair into ~/gocode/src/github.com/UKCloud/govcloudair. (in that folder I can see api.go and terraform compile just fine)
So govendor is probably not picking up the right (latest) version... maybe you have an idea why?

…dDNAT_tlate

Boran · 2017-06-01T13:00:34Z

I understand that one uses "make plugin-dev" to build only the plugin and link it in dynamically on startup. I prefer to build the full binary.

@jbardin, @catsby: So I have now been able to run acceptance tests and as requested, have separated the TestAccVcdDNAT_Basic into TestAccVcdDNAT_Basic and TestAccVcdDNAT_tlate.

14:47 $ make testacc TEST=./builtin/providers/vcd
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/06/01 14:47:37 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/vcd -v -timeout 120m
=== RUN TestProvider
--- PASS: TestProvider (0.00s)
=== RUN TestProvider_impl
--- PASS: TestProvider_impl (0.00s)
=== RUN TestAccVcdDNAT_Basic
--- PASS: TestAccVcdDNAT_Basic (134.08s)
=== RUN TestAccVcdDNAT_tlate
--- PASS: TestAccVcdDNAT_tlate (287.34s)
=== RUN TestAccVcdFirewallRules_basic
--- PASS: TestAccVcdFirewallRules_basic (236.37s)
=== RUN TestAccVcdNetwork_Basic
--- PASS: TestAccVcdNetwork_Basic (137.87s)
=== RUN TestAccVcdSNAT_Basic
--- PASS: TestAccVcdSNAT_Basic (172.24s)

There is one fail, but not related to this PR
=== RUN TestAccVcdVApp_PowerOff
--- FAIL: TestAccVcdVApp_PowerOff (201.88s)
testing.go:280: Step 0 error: Error applying: 1 error(s) occurred:
* vcd_vapp.foobar: 1 error(s) occurred:
* vcd_vapp.foobar: Error finding catalog: &errors.errorString{s:"can't find catalog: NubesLab"}
FAIL

pasikarkkainen · 2017-06-07T10:49:12Z

@Boran : Thanks a lot! Sounds like this PR is ready to go then?

bobbydeveaux · 2017-06-07T10:54:09Z

Sorry @Boran didn't see your other replies. Glad you're up and running and this LGTM!

Boran · 2017-06-09T08:28:00Z

@catsby : have you had a chance to review/look? :-)

Boran · 2017-06-16T08:10:34Z

Move this PR to the new vcd provider repo.
https://github.com/terraform-providers/terraform-provider-vcd/pull/19

ghost · 2020-04-08T02:34:45Z

I'm going to lock this issue 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 similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

initial idea: add dest_port parameter

1031521

Boran changed the title ~~DNAT: all different src/translated port~~ DNAT: allow different src/translated port May 12, 2017

Boran added 2 commits May 12, 2017 17:03

change name to translated_port

ed10701

translated_port optional, defaults to port (backwarsd compatibility

42613ba

paddycarver added enhancement provider/vcloud labels May 12, 2017

ewypych mentioned this pull request May 22, 2017

provider/vcd: Add port mapping to DNAT rule #14709

Closed

Boran added 2 commits May 22, 2017 15:07

RemoveNATMapping > RemoveNATPortMapping

e116d3c

test for translated port

22aac4f

fix test

db4923c

jbardin reviewed May 31, 2017

View reviewed changes

jbardin assigned catsby May 31, 2017

Boran added 2 commits May 31, 2017 18:09

trlateportString > translatedPortString

bbb2441

Merge branch 'master' of https://github.com/hashicorp/terraform

d27c7fc

Merge branch master

5304599

Boran added 2 commits June 1, 2017 14:32

Merge branch master

234789b

separate TestAccVcdDNAT_Basic into TestAccVcdDNAT_Basic and TestAccVc…

1eae19b

…dDNAT_tlate

Boran mentioned this pull request Jun 16, 2017

dnat: allow different src/translated port vmware/terraform-provider-vcd#19

Merged

Boran closed this Jun 16, 2017

ghost locked and limited conversation to collaborators Apr 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DNAT: allow different src/translated port #14437

DNAT: allow different src/translated port #14437

Boran commented May 12, 2017 •

edited

Loading

Boran commented May 22, 2017 •

edited

Loading

Boran commented May 23, 2017

ewypych commented May 23, 2017

bobbydeveaux commented May 26, 2017

robcoward commented May 26, 2017

ewypych commented May 26, 2017

Boran commented May 29, 2017

bobbydeveaux commented May 29, 2017

Boran commented May 31, 2017

jbardin left a comment

jbardin May 31, 2017

Boran May 31, 2017

jbardin May 31, 2017

Boran May 31, 2017

jbardin May 31, 2017

Boran commented Jun 1, 2017 •

edited

Loading

bobbydeveaux commented Jun 1, 2017 •

edited

Loading

Boran commented Jun 1, 2017

bobbydeveaux commented Jun 1, 2017

Boran commented Jun 1, 2017

bobbydeveaux commented Jun 1, 2017

bobbydeveaux commented Jun 1, 2017

Boran commented Jun 1, 2017

Boran commented Jun 1, 2017 •

edited

Loading

pasikarkkainen commented Jun 7, 2017

bobbydeveaux commented Jun 7, 2017 •

edited

Loading

Boran commented Jun 9, 2017

Boran commented Jun 16, 2017

ghost commented Apr 8, 2020

DNAT: allow different src/translated port #14437

DNAT: allow different src/translated port #14437

Conversation

Boran commented May 12, 2017 • edited Loading

Boran commented May 22, 2017 • edited Loading

Boran commented May 23, 2017

ewypych commented May 23, 2017

bobbydeveaux commented May 26, 2017

robcoward commented May 26, 2017

ewypych commented May 26, 2017

Boran commented May 29, 2017

bobbydeveaux commented May 29, 2017

Boran commented May 31, 2017

jbardin left a comment

Choose a reason for hiding this comment

jbardin May 31, 2017

Choose a reason for hiding this comment

Boran May 31, 2017

Choose a reason for hiding this comment

jbardin May 31, 2017

Choose a reason for hiding this comment

Boran May 31, 2017

Choose a reason for hiding this comment

jbardin May 31, 2017

Choose a reason for hiding this comment

Boran commented Jun 1, 2017 • edited Loading

bobbydeveaux commented Jun 1, 2017 • edited Loading

Boran commented Jun 1, 2017

bobbydeveaux commented Jun 1, 2017

Boran commented Jun 1, 2017

bobbydeveaux commented Jun 1, 2017

bobbydeveaux commented Jun 1, 2017

Boran commented Jun 1, 2017

Boran commented Jun 1, 2017 • edited Loading

pasikarkkainen commented Jun 7, 2017

bobbydeveaux commented Jun 7, 2017 • edited Loading

Boran commented Jun 9, 2017

Boran commented Jun 16, 2017

ghost commented Apr 8, 2020

Boran commented May 12, 2017 •

edited

Loading

Boran commented May 22, 2017 •

edited

Loading

Boran commented Jun 1, 2017 •

edited

Loading

bobbydeveaux commented Jun 1, 2017 •

edited

Loading

Boran commented Jun 1, 2017 •

edited

Loading

bobbydeveaux commented Jun 7, 2017 •

edited

Loading