VPC N-S security policy resource #1223
Conversation
|
/test-all |
ksamoray
force-pushed
the
vpc-ns-policy
branch
4 times, most recently
from
6d32cc4 to
eb2bf1c
Compare
| @@ -263,7 +264,22 @@ func createChildDomainWithGatewayPolicy(domain string, policyID string, policy m | |||
| return dataValue.(*data.StructValue), nil | |||
| } | |||
|
|
|||
| func gatewayPolicyInfraPatch(context utl.SessionContext, policy model.GatewayPolicy, domain string, m interface{}) error { | |||
| func gatewayPolicyInfraPatch(context utl.SessionContext, policy model.GatewayPolicy, domain string, m interface{}, withDomain bool) error { | |||
There was a problem hiding this comment.
How are you using withDomain in this function?
There was a problem hiding this comment.
It's redundant, I'll remove that argument.
| // GW Policies don't support scope | ||
| delete(secPolicy, "scope") | ||
| secPolicy["category"].ValidateFunc = validation.StringInSlice(gatewayPolicyCategoryWritableValues, false) | ||
| // GW Policy rules require scope to be set | ||
| secPolicy["rule"] = getSecurityPolicyAndGatewayRulesSchema(true, false, true) | ||
| secPolicy["rule"] = getSecurityPolicyAndGatewayRulesSchema(withDomain, false, true) |
There was a problem hiding this comment.
I thought the first parameter is for scope required.
if withDomain = False and then we have a VPC resource, does this make scope optional?
There was a problem hiding this comment.
I guess that I've made it so to make scope optional for VPC - but I'm not sure that it should be so.
| func resourceNsxtPolicyVPCGatewayPolicyCreate(d *schema.ResourceData, m interface{}) error { | ||
| connector := getPolicyConnector(m) | ||
|
|
||
| // Initialize resource Id and verify this ID is not yet used |
There was a problem hiding this comment.
If domain is the only difference between this function and resourceNsxtPolicyGatewayPolicyCreate, would it make sense to parametrize this single value here and below?
There was a problem hiding this comment.
Both funcs barely have any logic. I don't see much value in adding excess of functions which in the end, do nothing, pretty much like in Delete()
ksamoray
force-pushed
the
vpc-ns-policy
branch
5 times, most recently
from
064892c to
3090eef
Compare
| return fmt.Errorf("error obtaining VPC Gateway Policy ID") | ||
| } | ||
|
|
||
| obj, err := getGatewayPolicyInDomain(getSessionContext(d, m), id, "", connector) |
There was a problem hiding this comment.
I understand the reluctance to rename common helpers, but I personally find domain confusing here.. I think a comment explaining that we're not actually looking in domain for VPC would help future code readers.
For a long run, I think we might want to consider moving domain into the context
There was a problem hiding this comment.
I think that moving domain (or generally anything that's in the URL part of the API) into context makes some sense, but I'd rather avoid this now - it's a major effort and a compatibility issue.
For the func name change - I'll change it, I missed that somehow.
|
|
||
| rule { | ||
| display_name = "rule1" | ||
| destination_groups = [nsxt_policy_group.group1.path, nsxt_policy_group.group2.path] |
There was a problem hiding this comment.
nsxt_vpc_group would be the more obvious use case?
| * `rule` (Optional) A repeatable block to specify rules for the Gateway Policy. Each rule includes the following fields: | ||
| * `display_name` - (Required) Display name of the resource. | ||
| * `description` - (Optional) Description of the resource. | ||
| * `destination_groups` - (Optional) Set of group paths that serve as the destination for this rule. IPs, IP ranges, or CIDRs may also be used starting in NSX-T 3.0. An empty set can be used to specify "Any". |
There was a problem hiding this comment.
lets remove 3.0 mention here and below as its irrelevant for VPC
| * `ip_version` - (Optional) The IP Protocol for the rule. Must be one of: `IPV4`, `IPV6` or `IPV4_IPV6`. Defaults to `IPV4_IPV6`. | ||
| * `logged` - (Optional) A boolean flag to enable packet logging. | ||
| * `notes` - (Optional) Text for additional notes on changes for the rule. | ||
| * `profiles` - (Optional) A list of context profiles for the rule. Note: due to platform issue, this setting is only supported with NSX 3.2 onwards. |
There was a problem hiding this comment.
Lets remove NSX version mention here as well. Are context profiles even supported for VPC?
There was a problem hiding this comment.
FQDN network policies should work for VPCs as well.
|
/test-all |
|
/test-all |
1 similar comment
|
/test-all |
| terraform import nsxt_vpc_gateway_policy.policy1 PATH | ||
| ``` | ||
|
|
||
| The above command imports the VPC gateway policy named `policy1` under NSX domain `domain` with the NSX Policy path `PATH`. |
|
/test-all |
|
@ksamoray, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding
|
Implement the resource for north-south VPC security policy. Signed-off-by: Kobi Samoray <[email protected]>
|
/test-all |
Implement the resource for north-south VPC security policy.