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 support for NAT rules using proxied NSX-V API #241

Merged
merged 25 commits into from
Sep 19, 2019

Conversation

Didainius
Copy link
Collaborator

@Didainius Didainius commented Sep 11, 2019

This PR aims to add support for methods to allow configuring Edge Gateway NAT rules using proxied NSX-V API.

It introduces a few new methods: CreateNsxvNatRule, UpdateNsxvNatRule, GetNsxvNatRuleById and DeleteNsxvNatRuleById.

The creation of above mentioned NAT rules requires to specify a vNic (network interface on edge gateway). To easen lookup of vNic and replicating similar behaviour as vCD UI two more methods are added: GetVnicIndexByNetworkNameAndType and GetNetworkNameAndTypeByVnicIndex. They allow to convert between vNic index number and network type and network name.

It should also be compatible with subinterfaces when we implement them in other places: https://github.com/terraform-providers/terraform-provider-vcd/issues/321

govcd/edgegateway.go Outdated Show resolved Hide resolved
@Didainius Didainius self-assigned this Sep 11, 2019
@Didainius Didainius added the enhancement New feature or request label Sep 11, 2019
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.

Very nice. Before merge, please confirm two functional aspects:

  1. That NAT rule(s) created by this method may be updated via H5 UI and vice versa (that these operations don't collide anymore).
  2. That it will be possible from Terraform side to create a DNAT rule on an external network with an org admin connection (with current DNAT rule that's not possible though in UI it is).

@Didainius
Copy link
Collaborator Author

1. That NAT rule(s) created by this method may be updated via H5 UI and vice versa (that these operations don't collide anymore).

Yes, they can be updated. II have manually tested it using terraform resources and "read" capabilities, which hint updates.

2. That it will be possible from Terraform side to create a DNAT rule on an _external_ network with an _org_ admin connection (with current DNAT rule that's not possible though in UI it is).

Yes, it must be so. It does not try to access any other resources outside of edge gateway. If the edge gateway has an external network attached - it uses it.

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.

Nice work, overall looks good. Some comments for improvement or discussion

govcd/nat.go Outdated Show resolved Hide resolved
govcd/nat.go Outdated Show resolved Hide resolved
govcd/nat.go Outdated Show resolved Hide resolved
govcd/nat.go Outdated Show resolved Hide resolved
govcd/nat_test.go Show resolved Hide resolved
govcd/nat_test.go Show resolved Hide resolved
types/v56/types.go Outdated Show resolved Hide resolved
types/v56/types.go Show resolved Hide resolved
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

CHANGELOG.md Show resolved Hide resolved
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

@Didainius Didainius merged commit e2356f3 into vmware:master Sep 19, 2019
@Didainius Didainius deleted the edge-nat branch September 19, 2019 04:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-not-required enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants