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

Route-map with set block sets local preference and MED to 0 by default #813

Closed
ens-scmeeu opened this issue Nov 4, 2022 · 2 comments · Fixed by #818
Closed

Route-map with set block sets local preference and MED to 0 by default #813

ens-scmeeu opened this issue Nov 4, 2022 · 2 comments · Fixed by #818
Labels
bug Bug

Comments

@ens-scmeeu
Copy link

ens-scmeeu commented Nov 4, 2022

Describe the bug

When using an nsxt_policy_gateway_route_map resource with a set block, but without explicitly setting local_preference, local preference will be set to 0. It appears from looking at the source code that it is not conditionally set as I would expect, but always set in the presence of a set block. My assumption is that this structure is using a default value of 0 for local preference, which then causes it to be incorrectly modified.

UPDATE: The same issue applies to med. It should not be being reset either unless explicitly set. See below for a possible (but not quite ideal) fix.

Reproduction steps

  1. Create a base NSX-T setup with at least one operational BGP neighbor
  2. Add an nsxt_policy_gateway_route_map with a set block, matching all routes. Do not set local_preference.
  3. Attach route-map to nsxt_policy_bgp_neighbor in route_filtering block via in_router_filter
  4. In T0 BGP table, observe that the received routes have a local preference of 0 (it should be 100)

Expected behavior

When not explicitly set in the route-map, BGP local preference should not be set. All implementations I'm aware of (including NSX-T) will then set local preference to 100 by default on receipt of the route (for eBGP neighbors that is, iBGP neighbors will preserve the existing value of the attribute).

Additional context

No response

@ens-scmeeu ens-scmeeu added the bug Bug label Nov 4, 2022
@ens-scmeeu
Copy link
Author

ens-scmeeu commented Nov 4, 2022

My Go is very rusty, and I know very little of TF providers, but I think a fix could look something like this:

						"local_preference": {
							Type:        schema.TypeInt,
							Optional:    true,
							Description: "Local preference indicates the degree of preference for one BGP route over other BGP routes",
                                                        Default:     -1,
						},
						"med": {
							Type:        schema.TypeInt,
							Optional:    true,
							Description: "A lower Multi exit descriminator (MED) is preferred over a higher value",
                                                        Default:     -1,
						},
		entrySet := model.RouteMapEntrySet{
			PreferGlobalV6NextHop: &globalIPv6,
			Weight:                &weight,
		}

                if med > -1 {
                        entrySet.Med = &med
                }

		if localPreference > -1 {
			entrySet.LocalPreference = &localPreference
		}

It appears the default value for LocalPreference would be nil, so it won't be modified unless explicitly set, so we omit from the struct init. We change the default value to -1 (since 0 is a valid LP) and test for it before setting.

P.S. - Med should not be being reset by default either. The default value for Med IS zero, but if it were set already and passing through, I think it would also be reset to zero, and it shouldn't be.

@ens-scmeeu ens-scmeeu changed the title Route-map with set block sets local preference to 0 by default Route-map with set block sets local preference and MED to 0 by default Nov 4, 2022
@annakhm annakhm linked a pull request Nov 30, 2022 that will close this issue
@annakhm
Copy link
Collaborator

annakhm commented Nov 30, 2022

Thanks for reporting this @ens-scmeeu! Also appreciate your fix suggestion, however I'd rather not set a -1 default on terraform side to avoid non-empty diff. A fix I suggest would mark these attributes as Computed and not set those unless explicitly specified.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants