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

added managed redistribution in ISIS #15617

Closed
wants to merge 1 commit into from

Conversation

squirrelking57
Copy link

@squirrelking57 squirrelking57 commented Mar 25, 2024

added command "route_leaking ipv4/ipv6 level's route-map name" in cli according to RFC5302

Copy link
Contributor

@idryzhov idryzhov left a comment

Choose a reason for hiding this comment

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

This is a good addition to FRR, but the code must be improved. I didn't look into the actual redistribution implementation yet, but I have some comments from the configuration point of view. Please, take a look.

isisd/isis_cli.c Outdated
@@ -85,13 +85,13 @@ void cli_show_router_isis(struct vty *vty, const struct lyd_node *dnode,
{
const char *vrf = NULL;

vrf = yang_dnode_get_string(dnode, "vrf");
vrf = yang_dnode_get_string(dnode, "./vrf");
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, remove all these unrelated changes.

isisd/isis_cli.c Outdated
/*
* XPath: /frr-isisd:isis/instance/route_leanking
*/
DEFPY_YANG(isis_leanking, isis_leanking_cmd,
Copy link
Contributor

@idryzhov idryzhov Mar 27, 2024

Choose a reason for hiding this comment

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

First, this should be leaking, not leaNking. This needs to be fixed everywhere in the code.

Second, I think it should be a part of the regular redistribute command. For example, that's how you configure this on Cisco:

redistribute isis ip level-2 into level-1 route-map r1

IMO we should just use the same command, there's no need to introduce new commands and new terminology.

@@ -585,6 +690,85 @@ void isis_redist_area_finish(struct isis_area *area)
}

#ifdef FABRICD
DEFUN(isis_leanking, isis_leanking_cmd,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comments about the command here.

@@ -104,6 +104,16 @@ module frr-isisd {
description
"This enum indicates capability for both levels.";
}
enum "level2_to_level1" {
Copy link
Contributor

Choose a reason for hiding this comment

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

There are no such levels in IS-IS, it shouldn't be modeled like this. More comments below.

@@ -2078,6 +2109,45 @@ module frr-isisd {
}
}

container route_leanking {
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need this new container to model IS-IS to IS-IS redistribution. It is enough to modify the existing redistribute container:

  • remove must ". != \"isis\""; from leaf protocol to allow redistribution from IS-IS
  • add a new from-level leaf to configure from which level to redistribute:
    leaf from-level {
      type level;
      when "../protocol = \"isis\"";
      mandatory true;
      must "(. != \"level-1-2\") and ((../../../is-type = \"level-1-2\") and (. != ../level))";
      description
        "IS-IS level from which the routes should be redistributed.";
    }
    

These two changes would be enough to model what you need without introducing a lot of new NB callbacks.

1) added command "redistribute ipv4/ipv6 isis level-m into level-n route-map name"
in cli according to RFC5302
2) added adding prefixes with the best metric from the level-1 db to the level-2 db on l1_l2 end system

Signed-off-by: squirrelking57 <1248756005hfh@gmail.com>
@squirrelking57
Copy link
Author

There is no need for a review at the moment, it needs improvement

@squirrelking57 squirrelking57 deleted the multiarea branch April 27, 2024 16:02
@squirrelking57 squirrelking57 restored the multiarea branch April 27, 2024 16:02
@squirrelking57
Copy link
Author

created a new beautiful and good pull request #15863

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.

None yet

3 participants