-
Notifications
You must be signed in to change notification settings - Fork 42
netascode#533-BFD_Multihop_Policy_L3O_Node_Profile #288
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
netascode#533-BFD_Multihop_Policy_L3O_Node_Profile #288
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comments
validation { | ||
condition = ( | ||
var.bfd_multihop.auth_key == null || | ||
can(regex("^[a-zA-Z0-9_.:-]{0,64}$", var.bfd_multihop.auth_key)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
verify length of variable. Seems to be wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been corrected. The variable length is 1 to 20. Updated this.
} | ||
} | ||
|
||
resource "aci_rest_managed" "bfd_multihop" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep resource name as class_name if possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- If object already exists, try to add some name after dash like eg. bfdMhNodeP-tenant.
- cannot we reuse existing resource bfdMhNodeP with condition var.tenant != "infra"?
sr-mpls node-profile supports same values like keyId, type, key.
@juchowan , what do you think? does it make sense to enable 2 ways of configuring bfd for infra sr-mpls and remove one of it in future via breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed the resources with _ The resource name is now as below
bfdMhNodeP_
bfdRsMhNodePol_
} | ||
} | ||
|
||
resource "aci_rest_managed" "bfd_multihop_node_policy" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
} | ||
} | ||
|
||
variable "bfd_multihop" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have following config:
apic:
tenants:
- name: sansalun
l3outs:
- name: L3OUT1
vrf: VRF1
domain: ROUTED1
node_profiles:
- name: NP_103
nodes:
- node_id: 103
router_id: 5.5.5.7
and it creates me bfd_multihop policy even if i don't define it. Something is wrong
aci_tenants.tf
Outdated
bgp_protocol_profile_name = try(np.bgp.name, "") | ||
bgp_timer_policy = try("${np.bgp.timer_policy}${local.defaults.apic.tenants.policies.bgp_timer_policies.name_suffix}", "") | ||
bgp_as_path_policy = try("${np.bgp.as_path_policy}${local.defaults.apic.tenants.policies.bgp_best_path_policies.name_suffix}", "") | ||
bfd_multihop = try("${np.bfd_multihop}${local.defaults.apic.tenants.policies.bfd_multihop_node_policies.name_suffix}", "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add also for node_profiles_auto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also I do not understand why we try to apply "string" + "name_suffix" value into "object".
For some reason this code works, however I have no idea how. I don't know what variables do we read in bfd_multihop from yaml code.
Doesn't it make sense to make it bfd_multihop = { all attributes/variables here? }
class_name = "bfdMhNodeP" | ||
content = { | ||
keyId = var.bfd_multihop.auth_key_id | ||
key = var.bfd_multihop.auth_key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requires
lifecycle { ignore_changes = [content["key"]}
see ldap module example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been added
class_name = "bfdMhNodeP" | ||
} | ||
|
||
resource "aci_rest_managed" "bfdRsMhNodePol" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't work for following code, why?
apic:
tenants:
- name: sansalun
l3outs:
- name: L3OUT1
vrf: VRF1
domain: ROUTED1
node_profiles:
- name: NP_101
nodes:
- node_id: 101
router_id: 5.5.5.5
bfd_multihop:
auth_type: sha1
auth_key_id: 1
auth_key: C1sco123
bfd_multihop_node_policy: BFD_MHOP_POL
Please folow up this yaml structure:
|
Description
This pull request introduces - Add the option for configuring a multihop BFD multihop profile under a BGP node profile.
ACI Object
bfdMhNodeP is a child of the L3out node profile and bfdRsMhNodePol is a child of the bfdMhNodeP.
For example:
uni/tn-mytenant/out-myl3out/lnodep-mynodeprofile/bfdMhNodeP is of class bfdMhNodeP
uni/tn-mytenant/out-myl3out/lnodep-mynodeprofile/bfdMhNodeP/bfdRsMhNodePol is of class bfdRsMhNodePol
The bfdMhNodePol contains a tnBfdMhNodePolName field that must point to the existing BFD multihop policy name.
BFD multihop policies were already implemented in AAC for a different use case, so are not required for this enhancement.
Motivation
The additional option to configure BFD multihop under a BGP node profile.
Related Issues
https://wwwin-github.cisco.com/netascode/nac-aci/issues/533
Proposed Syntax
l3outs:
- name:
node_profiles:
- name:
nodes:
- node_id:
bfd_multihop:
auth_type:
auth_key_id:
auth_key:
bfd_multihop_node_policy: