Add inter VRF routing resource #1167
Conversation
ksamoray
force-pushed
the
inter_vrf_routing
branch
from
0e242f9 to
6853a8e
Compare
|
/test-all |
ksamoray
force-pushed
the
inter_vrf_routing
branch
from
6853a8e to
3669681
Compare
|
/test-all |
| Type: schema.TypeList, | ||
| Description: "route map path for IN direction", | ||
| Optional: true, | ||
| MaxItems: 1, |
There was a problem hiding this comment.
Is there a reason those lists with size 1 are still lists? If we're following the API that expects to grow in future, maybe we shouldn't code the MaxItems in the provider to prevent the requirement for future change.
There was a problem hiding this comment.
IDK why it's defined as it is - they're lists in the spec. Do you think that we can remove MaxItems and let NSX enforce the limit?
There was a problem hiding this comment.
Otherwise I could flatten these attributes
There was a problem hiding this comment.
I'm in favor of letting NSX enforce the limit
| d.Set("nsx_id", d.Id()) | ||
| gwPath, err := getParameterFromPolicyPath("", "/inter-vrf-routing/", importID) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
I think this error message should be something like incorrect policy path for inter vrf routing resource: %v
| connector := getPolicyConnector(m) | ||
| client := tier_0s.NewInterVrfRoutingClient(connector) | ||
|
|
||
| gwPolicyPath := d.Get("gateway_path").(string) |
There was a problem hiding this comment.
I think its more reliable to rely on path here and in update
There was a problem hiding this comment.
Do we have a utility func to parse the gwID from path?
There was a problem hiding this comment.
Not this one specifically..
| * `description` - (Optional) Description of the resource. | ||
| * `tag` - (Optional) A list of scope + tag pairs to associate with this resource. | ||
| * `nsx_id` - (Optional) The NSX ID of this resource. If set, this ID will be used to create the policy resource. | ||
| * `gateway_path` - (Required) The NSX Policy path to the Tier0 or Tier1 Gateway for this NAT Rule. |
There was a problem hiding this comment.
This description doesn't seem right
There was a problem hiding this comment.
Indeed, it's only T0
ksamoray
force-pushed
the
inter_vrf_routing
branch
2 times, most recently
from
3b6d517 to
ceee384
Compare
| * `description` - (Optional) Description of the resource. | ||
| * `tag` - (Optional) A list of scope + tag pairs to associate with this resource. | ||
| * `nsx_id` - (Optional) The NSX ID of this resource. If set, this ID will be used to create the policy resource. | ||
| * `gateway_path` - (Required) The NSX Policy path to the Tier0 Gateway for this NAT Rule. |
There was a problem hiding this comment.
for this NAT Rule => for this resource
| if id == "" { | ||
| id = newUUID() | ||
| } | ||
|
|
There was a problem hiding this comment.
We usually check that object with this nsx_id does not exist yet, here this check is missing, is this intentional?
There was a problem hiding this comment.
No, I guess that I've copy-pasted from the wrong place :)
Seems that there are quite a lot of cases where we do not check this. Maybe it's worthwhile to add this to some TODO list?
|
I think this resource is also relevant for GM? |
Fixes: vmware#1051 Signed-off-by: Kobi Samoray <[email protected]>
ksamoray
force-pushed
the
inter_vrf_routing
branch
from
ceee384 to
5c42c7b
Compare
Indeed looks like support for GM is different - |
Makes sense |
|
/test-all |
Fixes: #1051