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 NSX-T NAT rule support #382

Merged
merged 15 commits into from
Jun 25, 2021
Merged

Conversation

Didainius
Copy link
Collaborator

@Didainius Didainius commented Jun 9, 2021

Implements NSX-T NAT rule support by adding NsxtNatRule and types.NsxtNatRule as well as methods edge.GetAllNsxtNatRules,edge.GetNsxtNatRuleByName, edge.GetNsxtNatRuleById, edge.CreateNatRule, nsxtNatRule.Update, nsxtNatRule.Delete, nsxtNatRule.IsEqualTo

A few caveats:

  • API does not return ID of created object therefore it must be manually looked up by comparing as much fields as it is possible. To aid this nsxtNatRule.IsEqualTo is introduced to perform additional validations. edge.CreateNatRule also has non-standard task tracking and then uses nsxtNatRule.IsEqualTo internally to find ID of newly created rule.
  • There are two new fields introduced in API V35.2+ (VCD 10.2.2+) - FirewalMatch and Priority. In order to support these fields all methods above will elevevate API version to exactly V35.2 (not higher) so that those fields can be used. There is a specific check Test_NsxtNatDnatFirewallMatchPriority to test exactly that
  • New NAT rule type REFLEXIVE is supported in API V36.0. It also requires rule type to be specified in Type field instead of RuleType
  • There is one field which was deprecated InternalPort and was changed to DnatExternalPort between existing versions. Because these functions automatically elevate API - one must know which field to use. Tests Test_NsxtNatDnatInternalPort and Test_NsxtNatDnatExternalPort test out this functionality.

Additionally:

  • Functions queryOrgVdcByName, queryOrgVdcById, queryCatalogByName and queryCatalogById are improved to include orgName as filter (improves filtering accuracy in cases when org ID field is not accepted)

Note. Tests pass with nsxt tag on all NSX-T supported environments.

@Didainius Didainius self-assigned this Jun 9, 2021
@Didainius Didainius force-pushed the nsxt-nat-rules-pr branch 9 times, most recently from 2128714 to 3b05b3b Compare June 9, 2021 11:55
Signed-off-by: Dainius Serplis <[email protected]>
@Didainius Didainius force-pushed the nsxt-nat-rules-pr branch from 3b05b3b to ea37317 Compare June 9, 2021 12:01
Signed-off-by: Dainius Serplis <[email protected]>
Signed-off-by: Dainius Serplis <[email protected]>
@Didainius Didainius force-pushed the nsxt-nat-rules-pr branch from 66422b3 to dc36e30 Compare June 11, 2021 19:19
Signed-off-by: Dainius Serplis <[email protected]>
@Didainius Didainius marked this pull request as ready for review June 12, 2021 19:04
Signed-off-by: Dainius Serplis <[email protected]>
@Didainius Didainius requested a review from dataclouder June 22, 2021 12:33
Signed-off-by: Dainius Serplis <[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 questions to answer


// IsEqualTo allows to check if a rule has exactly the same fields (except ID) to the supplied rule
// This validation is very tricky because minor version changes impact how fields are return.
// This function relies on most common and stable fields:
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that Type and RuleType may not both be there, but a comparison should be still viable. What am I missing?

Also, could you make a note on why these fields are not reliably comparable?

  • InternalPort,
  • FirewallMatch,
  • Priority,
  • Version,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, could you make a note on why these fields are not reliably comparable?

Good question which led me to another round of thinking. I was carefully trying to match as many fields as possible (while still having comprehensible comparison code). Main problem for many fields is - behavior changes across API versions which can make this matching very messy - value comparisons differ based on version:

  • Type and RuleType - the trick is - on API V36.0 one can set RuleType (deprecated) or Type field value (except the new REFLEXIVE which explicitly requires Type field). After READ it would return both RuleType and Type being set with the same value, unless it is REFLEXIVE(in that case it returns ruleType=null and type=REFLEXIVE)
  • FirewallMatch - it exists only since API 35.2+ and has a default starting this version
  • InternalPort - is deprecated since API V35.0+ and is replaced by DnatExternalPort
  • Priority - is available only in API V35.2+
  • Version - it is something that is automatically handled by API. When creating - you must specify none, but it sets version to 0. When updating one must specify the last version read, and again it will automatically increment this value after update. (probably it is meant to avoid concurrent updates)

We could probably even write smart rules for tested versions, but what I am more afraid of is that even using a known API version might break in future VCD versions as we already saw a few times before. I know that in future versions NAT should return ID after POST and that should not be required at all, but now we still have it.

For now I have put this comment as well into function docs.

Any other ideas? I will sleep on it to see if anything more pops up now as I refreshed this context.

apiVersion = "36.0"
}

return apiVersion
Copy link
Contributor

Choose a reason for hiding this comment

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

This function assumes that apiVersion contains the valid version to be used when the client version is < 35.2.
But if apiVersion is empty or invalid, we will have an unrecoverable error.
Possibly we should retrieve apiVersion within elevateNsxtNatRuleApiVersion and return (string error).
Or you may have a better idea.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The point with this function was that I would always feed it with output of

endpoint := types.OpenApiPathVersion1_0_0 + types.OpenApiEndpointNsxtNatRules
apiVersion, err := client.checkOpenApiEndpointCompatibility(endpoint)

However. I had in mind that one day - if such approach of "stepped" elevation (elevation to known verified API versions when possible) could be more generic for those resources that need it.

I have made a second attempt and introduced a function client.getOpenApiHighestElevatedVersion for internal use which is similar to checkOpenApiEndpointCompatibility, but is capable of picking highest possible version from a predefined list endpointElevatedApiVersions.

We could just use "highest" version supported by VCD API, but that is risky as pointing it to future VCD version might just break it (if some field is removed in future API version).

Have a look and share if it looks better. This is yet hard to comprehend if it is a silver bullet as there is just one use case, but this was my early vision of how it could look in a more generic fashion.

Signed-off-by: Dainius Serplis <[email protected]>
@Didainius Didainius merged commit 222985e into vmware:master Jun 25, 2021
@Didainius Didainius deleted the nsxt-nat-rules-pr branch June 25, 2021 09:03
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