Integration of FRR ISIS Routing protocol to DANOS. #3

KaushikNiral · 2020-03-16T17:08:35Z

Integrated FRR ISIS Routing protocol to DANOS.

Added new set file. and operational file for isis under yang/
Added json script for isis under script/frr/configs/commands/
Added isis install file under debian/
Modified this files debian/control and debian/vyatta-frr-vci.component for isis

1. Added new set file. and operational file for isis under yang/ 2. Added json script for isis under script/frr/configs/commands/ 3. Added isis install file under debian/ 4. Modified this files debian/control and debian/vyatta-frr-vci.component for isis Signed-off-by: kaushik <[email protected]>

odd22 · 2020-03-17T08:22:40Z

@KaushikNiral I think before adding ISIS to Vyatta/DANOS CLI, we must first update FRR included in DANOS. Current FRR version is 7.3 https://github.com/FRRouting/frr/releases while FRR version integrated within DANOS is the 6.0.

The main point here, is that since 6.0, FRR team has added a new northbound API for CLI based on tang model i.e. usable with NetConf. Not all daemons have been ported to this new NorthBound API, but ISIS yes. See here: https://github.com/FRRouting/frr/tree/master/yang

So, instead of creating a new yang model for ISIS, it will be better to adopt the yang models provided by the FRR release. As other daemons e.g zebra, bfd, rip, eigrp, ... and base modules e.g. interface, route-map, filter ... have also been ported to this new Northbound API, and thus, with corresponding yang model, I think it will also be necessary to adapt DANOS yang models to the FRR yang models. In addition, FRR aims to endorse and follow as much as possible IETF yang models. Deviations are explicitly release in additional yang model as augmentation to IETF yang model. Currently a great effort is made to port BGP, OSPF & LDP to this new Northbound API, thus with respective yang models.

JammyStuff · 2020-03-17T11:20:02Z

@odd22 Maybe @jsouthworth will have some thoughts as well, but it's really unclear to me how the FRR YANG models avoid the need for DANOS to have YANG models to integrate with FRR.

At some point, it would probably be desirable that DANOS starts configuring FRR via your new northbound API, which would obviously be using your YANG, but at the user interface level (CLI, NETCONF, etc) I really don't see how your YANG models directly would integrate cleanly into the configuration for the rest of the system. For example, how would interface configuration work? The FRR protocols YANG models augment the FRR interface YANG models, which are distinct from the DANOS interface YANG models. The only way that I can see this working is for there to be DANOS YANG models and infrastructure that would translate that config to fit the FRR YANG models and pass that over your new northbound API.

We don't have the infrastructure for it in place at the moment, but we do have an idea about "multi-model support", whereby it would be possible to configure the system through different YANG models (e.g. OpenConfig). Even with that though, it's not immediately obvious how a subsystem specific YANG model such as the FRR one would really work. How would the rest of the system be configured?

odd22 · 2020-03-17T13:49:28Z

@JammyStuff Perhaps, I was not sufficiently clear with the new NorthBound API. In fact, for daemon which adopt the new Northbound API, the old CLI was removed except for the show command family. When a new command is introduce, we start by adding the corresponding entry/parameters in the yang model, then create the entry in the northbound API and then in the CLI a new call to this new NorthBound API. If DANOS used its own yang model, this introduce a double yang <-> CLI exchange which is not necessary: DANOS yang -> vtysh -> FRR yang -> daemon call

Just have a look for example to https://github.com/FRRouting/frr/blob/master/isisd/isis_nb_config.c and https://github.com/FRRouting/frr/blob/master/isisd/isis_cli.c

I agree that the work is not complete on FRR side and that what DANOS provided is great for the moment. I'm just suggesting to go to the FRR yang model when introducing new Daemon that already support the new Northbound API. But, this means that DANOS must first upgrade FRR to latest release.

odd22 · 2020-03-17T13:56:13Z

yang/vyatta-protocols-frr-isis-v1.yang

@@ -0,0 +1,878 @@
+module vyatta-protocols-frr-isis-v1 {


Please use this file instead: https://github.com/FRRouting/frr/blob/master/yang/frr-isisd.yang

It will help further integration of next FRR release

JammyStuff · 2020-03-17T14:05:58Z

@odd22 Just to clarify (both in general and in your suggested change), are you suggesting that the FRR YANG is used verbatim, or that a DANOS YANG file should be created that is similar to the FRR YANG file but modified to fit with the rest of the DANOS YANG model in the hope that would minimise the work required by the layer to translate DANOS config to FRR config?

I was under the impression that you were suggesting to use the FRR YANG verbatim, in which case I still don't understand how that is going to fit with the rest of the DANOS config model.

odd22 · 2020-03-17T14:19:28Z

@JammyStuff I'm suggesting to take the FRR YANG verbatim for the configuration part and keep the DANOS Yang for operational part as this one doesn't exist for FRR (Show CLI stay with the old CLI system). If you look to both proposed yang/vyatta-protocols-frr-isis-v1.yang model and https://github.com/FRRouting/frr/blob/master/yang/frr-isisd.yang model, they are almost identical. And, the FRR model contains all available configuration for ISIS.

JammyStuff · 2020-03-17T14:46:13Z

The frr-isisd YANG model doesn't stand on it's own. It's augmenting the frr-interface YANG model for the IS-IS interface configuration for example. If we're taking the model verbatim, how do we avoid needing to also take the frr-interface model and having 2 separate interface models?

odd22 · 2020-03-17T16:20:17Z

That's the tricky point. We need to manage the transition phase in the smoother way to avoid problem and double work. It is exactly why I suggest to first upgrade to FRR 7.3 before integrating new daemon such isis in DANOS.

jsouthworth · 2020-03-17T16:49:14Z

Integrating YANG modules from individual projects is going to be extremely problematic. DANOS presents a full system data-model that is highly integrated. This is used to render all user facing interfaces. One of the driving intentions behind DANOS is to provide first a reasonable model to a human operators. This will necessitate some translation to happen somewhere in the system.

FRR presents a model for the subsystem it controls which is self-contained and highly integrated. We cannot pull the FRR models in to DANOS directly and present them as the interface model as this will require that an operator create objects in two places for shared configuration (interfaces, etc.).

Work is scheduled for a future release that will support a data-model translation framework that will allow one to translate between an interface model and another data-model that will also expose both models via the northbound interfaces. This may allow us to have NETCONF or gNMI speak the FRR model natively, but the infrastructure does not exist at this time and so we cannot integrate this in this way today. This does not remove the need to do a translation, it only moves it to a different place.

In the shorter term, when integrating a future FRR release it is highly likely that the gRPC based Northbound API would be used. The integration component for FRR would need to be modified to use this interface instead of using vtysh. We are aware of the need to do this and would welcome the addition if someone did the work. It is quite unfair, however, to expect someone adding integration for a single feature to do this work, which is far outside the scope of the current review. In my view, updating FRR and adding ISIS support are orthogonal. ISIS can be added with fairly little change to what is already here, of which this PR is evidence, and doesn't create much additional burden when porting to a newer FRR in the future.

Obviously, the closer the DANOS YANG is to the FRR YANG, the easier it will be to translate from one to the other in the future, but there is no way to get around needing to do it. FRR's YANG wants to bring in other FRR YANG and DANOS has duplication for it already, and at this time, this cannot be supported in a way that meet the goals of DANOS as a whole.

In general, all YANG models will have this same limitation, Openconfig is highly dependent on other Openconfig models, the IETF models are highly dependent on other IETF models, etc. Bringing in one will bring in the lot of other things. This makes utilizing modules in a modular way nearly impossible. If the YANG were written slightly differently, one could avoid some of this but that requires a lot of abstraction for no real benefit to the organization doing the work. As such, we are stuck with fragmentation in the data-model space in general. DANOS is no exception to this.

jsouthworth · 2020-03-17T16:52:25Z

yang/vyatta-protocols-frr-isis-v1.yang

+    		}
+  	}
+
+  	augment /if:interfaces/interfaces-dataplane:dataplane {


We generally do interface augmentations as separate YANG files that import a grouping in the main file for the feature. We do this for modularity reasons so that not all interface types must be available on all builds.

Thanks @jsouthworth , for the review.
Hi @jsouthworth ,
We followed the "vyatta-protocols-frr-ospf-v1.yang" to perform the augmentations in the new file named "vyatta-protocols-frr-isis-v1.yang".
Since you are suggesting a different approach, can you please share a relevant file or example of it.

OK, what is here should be sufficient then. Our guys didn't follow the convention when doing integration for the rest of the routing suite. It is highly unlike that an image would ever be built with out dataplane interfaces anyway. If support for other interface types are added in future release we'll need to follow the "tertiary model" convention for them.

jsouthworth · 2020-03-17T16:53:34Z

yang/vyatta-protocols-frr-isis-v1.yang

+        		must "count(../isis/tagnode) <= 1" {
+          			error-message "Another ISIS instance is already running" ;
+        		}
+        		key "tagnode";


There is no reason to continue the "tagnode" naming convention, we only have this in some YANG due to an automated conversion from Vyatta's original data-modeling language.

We have fixed this issue.
We will send review after resolving the 1st review comment on augmentation.

jsouthworth · 2020-03-17T16:59:52Z

yang/vyatta-protocols-frr-isis-v1.yang

@@ -0,0 +1,878 @@
+module vyatta-protocols-frr-isis-v1 {


There seems to be a lot of whitespace errors in this file. For reference, the DANOS convention is to use Tabs to do indentation in YANG files.

We have fixed this issue.

KaushikNiral · 2020-03-18T10:42:25Z

Integrating YANG modules from individual projects is going to be extremely problematic. DANOS presents a full system data-model that is highly integrated. This is used to render all user facing interfaces. One of the driving intentions behind DANOS is to provide first a reasonable model to a human operators. This will necessitate some translation to happen somewhere in the system.

FRR presents a model for the subsystem it controls which is self-contained and highly integrated. We cannot pull the FRR models in to DANOS directly and present them as the interface model as this will require that an operator create objects in two places for shared configuration (interfaces, etc.).

Work is scheduled for a future release that will support a data-model translation framework that will allow one to translate between an interface model and another data-model that will also expose both models via the northbound interfaces. This may allow us to have NETCONF or gNMI speak the FRR model natively, but the infrastructure does not exist at this time and so we cannot integrate this in this way today. This does not remove the need to do a translation, it only moves it to a different place.

In the shorter term, when integrating a future FRR release it is highly likely that the gRPC based Northbound API would be used. The integration component for FRR would need to be modified to use this interface instead of using vtysh. We are aware of the need to do this and would welcome the addition if someone did the work. It is quite unfair, however, to expect someone adding integration for a single feature to do this work, which is far outside the scope of the current review. In my view, updating FRR and adding ISIS support are orthogonal. ISIS can be added with fairly little change to what is already here, of which this PR is evidence, and doesn't create much additional burden when porting to a newer FRR in the future.

Obviously, the closer the DANOS YANG is to the FRR YANG, the easier it will be to translate from one to the other in the future, but there is no way to get around needing to do it. FRR's YANG wants to bring in other FRR YANG and DANOS has duplication for it already, and at this time, this cannot be supported in a way that meet the goals of DANOS as a whole.

In general, all YANG models will have this same limitation, Openconfig is highly dependent on other Openconfig models, the IETF models are highly dependent on other IETF models, etc. Bringing in one will bring in the lot of other things. This makes utilizing modules in a modular way nearly impossible. If the YANG were written slightly differently, one could avoid some of this but that requires a lot of abstraction for no real benefit to the organization doing the work. As such, we are stuck with fragmentation in the data-model space in general. DANOS is no exception to this.

Thank you @JammyStuff , @jsouthworth and @odd22 for taking your time to review the PR.

jsouthworth · 2020-03-20T16:13:42Z

yang/vyatta-protocols-frr-isis-v1.yang

+  	typedef level {
+    		type enumeration {
+      			enum "level-1" {
+        			value 1;


These values still have whitespace problems (the indentation is off by one).

We did not update the review changes yet, will share it in sometime.

1. All review comments are resolved. 2. Removed tagnode and corrections done for the indentation. Signed-off-by: kaushik <[email protected]>

deastoe · 2020-03-23T15:12:05Z

Thanks for your contribution @KaushikNiral.

The indentation in the new YANG modules is quite inconsistent; some parts use double space while others use tab. Please could you update the modules to use a consistent tab indent?

deastoe · 2020-03-23T15:16:40Z

yang/vyatta-protocols-frr-isis-v1.yang

+    of subnetwork being used. As an example, for an ethernet subnetwork, the SNPA is 
+    encoded as a MAC address like '00aa.bbcc.ddee'.";
+
+    description"This type defines the Subnetwork Point of Attachment (SNPA) format.


Please add a space after the description keyword. This prevents a pyang error:

vyatta-protocols-frr-isis-v1.yang:148: error: syntax error: expected separator, got: ""This ..."

Actually it looks like this typedef is unused? It could be removed.

This is Resolved.

deastoe · 2020-03-23T15:18:22Z

yang/vyatta-op-protocols-frr-isis-v1.yang

+	import vyatta-types-v1 {
+		prefix types;
+	}


This import can be removed as it is unused. Removing it will suppress a warning from pyang:

vyatta-op-protocols-frr-isis-v1.yang:5: warning: imported module vyatta-types-v1 not used

This is Resolved.

deastoe · 2020-03-23T15:21:16Z

yang/vyatta-protocols-frr-isis-v1.yang

+	grouping redistribute-protocol {
+		leaf kernel {
+      type level;
+      must ". != \"isis\"";


The must statements in this grouping can be removed, since the isis is not defined by the level enumeration.

This is Resolved.

deastoe · 2020-03-23T15:47:01Z

yang/vyatta-protocols-frr-isis-v1.yang

+  grouping redistribute-attributes {
+    configd:help "Common optional attributes of any redistribute entry.";
+    description  "Common optional attributes of any redistribute entry.";
+    choice attribute {
+      list route-map {
+        configd:help "Route map reference.";
+        description "Route map reference.";
+        key "rm-name";
+        leaf rm-name {
+          type string {
+            length "1..max";
+          }
+          configd:help "Applies the conditions of the specified route-map to routes that
+          are redistributed into this routing instance.";
+          description "Applies the conditions of the specified route-map to routes that
+          are redistributed into this routing instance.";
+        }
+        leaf metric {
+          configd:help "Metric for default route.";
+          description "Metric for default route.";
+          type uint32 {
+            range "0..16777215";
+          }
+          default "0";
+        }
+      }
+      list metric {
+        configd:help "Metric for default route.";
+        description "Metric for default route.";
+        key "metric-id";
+        leaf metric-id {
+          type uint32 {
+            range "0..16777215";
+          }
+          default "0";
+          configd:help "Metric used for the redistributed route. If 0,
+          the default-metric attribute is used instead.";
+          description "Metric used for the redistributed route. If 0,
+          the default-metric attribute is used instead.";
+        }
+        leaf route-map {
+          type string {
+            length "1..max";
+          }
+          configd:help "Route map reference.";
+          description "Route map reference";
+        }
+      }			
+    }
+  }
+
+  grouping redistribute-default {
+    container always {
+      configd:help "Always advertise default route.";
+      description "Always advertise default route.";
+      uses redistribute-attributes;
+    }
+    uses redistribute-attributes;
+  }


It looks like these groupings aren't currently used? I'd suggest only adding the groupings which are currently in use/supported.

This is Resolved.

deastoe · 2020-03-23T15:49:23Z

yang/vyatta-protocols-frr-isis-v1.yang

+  typedef if-state-type {
+    type enumeration {
+      enum "up" {
+        value 0;
+        configd:help "Up state."; 
+        description "Up state.";
+      }
+      enum "down" {
+        value 1;
+        configd:help "Down state";
+        description "Down state";
+      }
+    }
+    configd:help "This type defines the state of an interface";
+    description "This type defines the state of an interface";
+  }
+
+  typedef adj-state-type {
+    type enumeration {
+      enum "up" {
+        value 0;
+        configd:help "State indicates the adjacency is established.";
+        description "State indicates the adjacency is established.";
+      }
+      enum "down" {
+        value 1;
+        configd:help "State indicates the adjacency is NOT established.";
+        description "State indicates the adjacency is NOT established.";
+      }
+      enum "init" {
+        value 2;
+        configd:help "State indicates the adjacency is establishing.";
+        description "State indicates the adjacency is establishing.";
+      }
+      enum "failed" {
+        value 3;
+        configd:help "State indicates the adjacency is failed.";
+        description "State indicates the adjacency is failed.";
+      }
+    }
+    configd:help "This type defines states of an adjacency";
+    description "This type defines states of an adjacency";
+  }


Looks like the adj-state-type and if-state-type types are also currently unused.

This is Resolved.

deastoe · 2020-03-23T16:00:30Z

yang/vyatta-protocols-frr-isis-v1.yang

+  typedef lsp-id {
+    type string {
+      pattern "[0-9A-Fa-f]{4}\\.[0-9A-Fa-f]{4}\\.[0-9A-Fa-f]{4}\\.[0-9][0-9]-[0-9][0-9]";
+    }
+    configd:help  "This type defines the IS-IS LSP ID format using a 
+    pattern, An example LSP ID is 0143.0438.AeF0.02-01"; 
+    description "This type defines the IS-IS LSP ID format using a
+    pattern, An example LSP ID is 0143.0438.AeF0.02-01";
+  }


This looks like it is not used currently, I'd recommend removing it until required.

This is Resolved.

deastoe · 2020-03-23T16:00:37Z

yang/vyatta-protocols-frr-isis-v1.yang

+  typedef extended-circuit-id {
+    type uint32;
+    configd:help "This type defines the extended circuit ID associated with an interface.";
+    description "This type defines the extended circuit ID associated with an interface.";
+  }


This looks like it is not used currently, I'd recommend removing it until required.

This is Resolved.

deastoe · 2020-03-23T16:01:23Z

yang/vyatta-protocols-frr-isis-v1.yang

+  typedef system-id {
+    type string {
+      pattern "[0-9A-Fa-f]{4}\\.[0-9A-Fa-f]{4}\\.[0-9A-Fa-f]{4}";
+    }
+    configd:help "This type defines IS-IS system-id using a pattern,
+    An example system-id is 0143.0438.AeF0";
+    description "This type defines IS-IS system-id using a pattern,
+    An example system-id is 0143.0438.AeF0";
+  }


This looks like it is not used currently, I'd recommend removing it until required.

This is Resolved.

deastoe · 2020-03-23T16:07:47Z

yang/vyatta-protocols-frr-isis-v1.yang

+  }
+
+  grouping isis-intf-password {
+    list clear {


I'm not sure clear and md5 should be lists. Does isisd support configuring multiple passwords of the same type?

This is Resolved.

deastoe · 2020-03-23T16:54:07Z

yang/vyatta-protocols-frr-isis-v1.yang

+      presence "Present if IETF SPF back-off delay is enabled.";
+      configd:help "IETF SPF delay algorithm.";
+      description "IETF SPF delay algorithm.";
+      list init-delay {


Please could you share the reasoning for using the nested set of lists? IMO it would be simpler and cleaner to have all these delay values as leaves at the top level of the container. Doing this would also prevent the user from configuring multiple of the same delay value (which can be done with lists, using max-elements, but I think using leaves at the top level is a more straightforward and readable approach).

Got it. Thanks @deastoe .
This is Resolved.

deastoe · 2020-03-23T17:07:33Z

yang/vyatta-protocols-frr-isis-v1.yang

+        }
+      }
+    }
+    leaf net {


The description suggests this is a list of addresses, but a leaf will only allow a single value. Should this be a leaf-list instead?

This is Resolved.

KaushikNiral · 2020-03-27T15:06:38Z

Thank you @KaushikNiral, that makes these modules easier to read and review.

I personally prefer a deeper (such as 4 spaces or a Tab character) indent as it is easier for me to visualise the structure. We don't yet have this documented, but DANOS does have a convention to use Tab characters for indentation in YANG modules.

The existing YANG modules in vyatta-protocols-frr don't all follow this convention (some use 4 space indents). Given this and the current lack of documentation I won't insist on any more widespread whitespace changes for this PR - we can always re-format all the modules in future.

I've asked some of the DANOS YANG experts to take a look this PR, and I will also give it another pass over the next few days.

Thanks for your patience while we review this :)

Thanks @deastoe :)
Looking forward to it.

wivory · 2020-03-30T08:16:09Z

yang/vyatta-op-protocols-frr-isis-v1.yang

+
+    This module implements the ISIS operational CLI.";
+
+  revision 2018-11-22 {


This should be the current date. It's important that revision dates are accurate, and updated when the file is changed, or NETCONF clients may continue using an older cached version of the file instead.

This is resolved.

wivory · 2020-03-30T08:16:49Z

yang/vyatta-protocols-frr-isis-v1.yang

+    SPDX-License-Identifier: BSD-3-Clause
+    The YANG module package for vyatta-protocols-frr-isis-v1";
+
+  revision 2018-11-15 {


Should be current date

This is resolved.
Thanks.

1. Revision date is modified to current date. Signed-off-by: harios <[email protected]>

wivory · 2020-04-02T10:48:40Z

yang/vyatta-protocols-frr-isis-v1.yang

+
+      leaf network {
+        type network-type;
+        must "(. = \"point-to-point\") or (. = \"broadcast\")";


Use single quotes - 'point-to-point' - instead of escaped double quotes. Much cleaner.

Hi Wivory,

We tried using single quote in this below mentioned way.
must "(. = 'point-to-point') or (. = 'broadcast')";

But "configd" process crashes if we do it in this manner.

Thanks,

Well, I can compile this file just fine with single quotes, and I have written countless unit tests proving single quotes work in general here. Are you definitely using the correct single quote character, as we've had problems with the wrong one being used before now. Can you tell me what error you get in configd (traceback / log error), and attach a copy of the file that causes the problem so I can test it, as if this is really failing, that's a bug that needs fixing.

Sorry, actually we tried using backslash with single quote that's why we got the error, the screenshot is attached.
Now we have applied the changes and found working. It is also committed.

Thanks.

Thanks - looks much cleaner

wivory · 2020-04-02T10:49:53Z

yang/vyatta-protocols-frr-isis-v1.yang

+  }
+
+  grouping isis-password {
+    choice type-choice {


DANOS does not yet support choice - it will compile, but is ignored. So, this isn't going to work. It also raises the question of what testing has been done on this?

Hi Wivory,

We have tested "choice" and found it to be working before sending for review.
We have attached 5 screenshot of the following below.

Screenshot of the DANOS CLI.

Screenshot of the frr.json file from location:- /etc/vyatta-routing/frr.json

Screenshot of the frr.conf file from location:- /etc/vyatta-routing/frr.conf

And finally from the frr.conf file from location:- /etc/frr/frr.conf

Screenshot of the set command from DANOS CLI.

Are you suggesting any other way to validate that it is working ?

Thanks,

Sorry - perhaps I wasn't clear what I meant by 'ignored'. We ignore the 'choice' element, and allow all choices to be configured as shown here:

[edit] vyatta@vm-at-1# show protocols isis +isis 1 { + area-password { + clear foo + } +} [edit] vyatta@vm-at-1# set protocols isis 1 area-password md5 bar [edit] vyatta@vm-at-1# show protocols isis +isis 1 { + area-password { + clear foo + md5 bar + } +}

If we had implemented choice correctly, setting the md5 option would have removed the clear option. You certainly couldn't commit both options with choice implemented correctly - but you will find that on DANOS currently, you can.

So, I guess you can keep the YANG as-is, but for now you will be able to configure both 'clear' and 'md5' at the same time.

Okay,
Thanks.

Actually you must not keep this yang or there is a risk that someone configures both options, and then when we fix choice, the config becomes invalid, ie this would be backwards incompatible.

Hi Wivory,

We have found a way which does not use "choice" and hence it should be good.
We are committing this change now.

grouping isis-password {
leaf clear {
configd:help "Clear-text authentication type.";
description "Clear-text authentication type.";
type string {
length "1..254";
}
must "not(../md5)" {
error-message "Can't configure both clear and md5 for password";
}
}
leaf md5 {
configd:help "MD5 authentication type.";
description "MD5 authentication type.";
type string {
length "1..254";
}
must "not(../clear)" {
error-message "Can't configure both clear and md5 for password";
}
}
}

Yes, that will do the trick. You only need it on one of the 2 leaves though - no need to check both ways.

Okay, we rectified it and committed.

Thanks.

1. The correction done on the quotation. Signed-off-by: harios <[email protected]>

1. Replacing choice in yang to prevent future backwards incompatibility. Signed-off-by: harios <[email protected]>

wivory · 2020-04-03T15:47:24Z

yang/vyatta-protocols-frr-isis-v1.yang

+      type string {
+        length "1..254";
+      }
+      must "not(../clear)" {


You don't need to check both ways round - with both you will get 2 warnings rather than one.

Okay, it is resolved now.

Thanks.

1.Rectified double warning into single warning for password. Signed-off-by: harios <[email protected]>

wivory · 2020-04-07T09:34:54Z

yang/vyatta-protocols-frr-isis-v1.yang

+      description "area password.";
+      configd:help "area password.";
+      uses isis-password;
+      must "not(clear and md5)" {


Is this an inherent property of the grouping, or of the 2 specific cases where it is used? If the former, you should put the must statement in the grouping on one of the two leaves.

Okay, we have kept the must statement in the grouping on one of the two leaves.

Thanks,

wivory · 2020-04-07T09:36:54Z

yang/vyatta-protocols-frr-isis-v1.yang

+      must "(as-num != 23456)" {
+        error-message "Invalid AS Number, 23456 (Reserved)";
+      }
+      must "count(../isis/as-num) <= 1" {


Better expressed as 'max-elements 1'

We found that ISIS allows multiple instances so we have removed the "must" statement as it is not required.

Thanks,

We found that ISIS allows multiple instances so we have removed the "must" statement as it is not required.

Thanks,

Approved YANG files as part of data model review process.

Thanks for the approval.

wivory · 2020-04-07T09:38:17Z

yang/vyatta-protocols-frr-isis-v1.yang

+        configd:help "Delay used while in QUIET state";
+        description "Delay used while in QUIET state";
+        default 500;	
+        type uint16 {


You could usefully define some common types for delays and timers as this will make the YANG more concise and more readable (and ensure consistency across different nodes)

This is resolved.

Thanks,

1. Added common types for delays and timers. 2. Correction for the must statement is done. 3. Minor changes done related to ISIS instance creation. Signed-off-by: harios <[email protected]>

wivory

Approved YANG files as part of data model review process.

KaushikNiral · 2020-04-15T13:40:57Z

Approved YANG files as part of data model review process.

Thanks for the approval.

dewi-morgan · 2020-05-22T09:13:12Z

As a general rule when introducing new yang models, taking account of any existing IETF yang models is a good idea. I was wondering if you were aware of draft-ietf-isis-yang-isis-cfg-42 model and if your model is based upon it or another yang model ? Does it make sense to accommodate the structure of the draft IETF ISIS yang model where possible (or makes sense) into your existing yang model ?

Thank you,

deastoe · 2020-05-22T09:49:07Z

yang/vyatta-protocols-frr-isis-v1.yang

+
+  grouping isis-password {
+    leaf clear {
+      configd:help "Clear-text authentication type.";


Please could you add a configd:secret "true"; statement to this leaf. This instructs the system to handle these values more carefully, for example redacting the value in audit logs.

Sure will do that and update.
Thanks,

This is Resolved.

deastoe · 2020-05-22T09:49:34Z

yang/vyatta-protocols-frr-isis-v1.yang

+        error-message "Can't configure both clear and md5 for password";
+      }
+    }
+    leaf md5 {


Please also add configd:secret "true"; to this leaf.

Sure, will add it.

Thanks,

This is Resolved.

1. Review comment on adding "configd:secret "true"" is done. 2. One minor cleanup in debian/control file. 3. One new typedef is added for ISIS level. This is done to separate the usage of ISIS three levels and two levels. Signed-off-by: harios <[email protected]>

odd22 · 2020-10-02T16:03:05Z

Is it possible to also add the Segment Routing configuration stuff introduce with FRR-7.3 release?

KaushikNiral force-pushed the niral_isis_integration branch from ce0adce to 855429c Compare March 16, 2020 17:14

odd22 suggested changes Mar 17, 2020

View reviewed changes

jsouthworth reviewed Mar 17, 2020

View reviewed changes

jsouthworth reviewed Mar 20, 2020

View reviewed changes

KaushikNiral force-pushed the niral_isis_integration branch from eb45cf0 to 44e5031 Compare March 20, 2020 20:28

Integration of FRR ISIS Routing protocol to DANOS.

44e5031

1. All review comments are resolved. 2. Removed tagnode and corrections done for the indentation. Signed-off-by: kaushik <[email protected]>

deastoe reviewed Mar 23, 2020

View reviewed changes

wivory reviewed Mar 30, 2020

View reviewed changes

Integration of FRR ISIS Routing protocol to DANOS.

626ca01

1. Revision date is modified to current date. Signed-off-by: harios <[email protected]>

hariosniral force-pushed the niral_isis_integration branch from f340adc to 626ca01 Compare March 30, 2020 14:42

wivory reviewed Apr 2, 2020

View reviewed changes

Integration of FRR ISIS Routing protocol to DANOS.

4a5a641

1. The correction done on the quotation. Signed-off-by: harios <[email protected]>

hariosniral force-pushed the niral_isis_integration branch from 0149c0b to 4a5a641 Compare April 3, 2020 11:46

Integration of FRR ISIS Routing protocol to DANOS.

1cefac4

1. Replacing choice in yang to prevent future backwards incompatibility. Signed-off-by: harios <[email protected]>

hariosniral force-pushed the niral_isis_integration branch from 7aeac0f to 1cefac4 Compare April 3, 2020 14:43

wivory reviewed Apr 3, 2020

View reviewed changes

Integration of FRR ISIS Routing protocol to DANOS.

5407485

1.Rectified double warning into single warning for password. Signed-off-by: harios <[email protected]>

hariosniral force-pushed the niral_isis_integration branch from 78da1dc to 5407485 Compare April 3, 2020 17:14

wivory reviewed Apr 7, 2020

View reviewed changes

Integration of FRR ISIS Routing protocol to DANOS.

0efb0af

1. Added common types for delays and timers. 2. Correction for the must statement is done. 3. Minor changes done related to ISIS instance creation. Signed-off-by: harios <[email protected]>

hariosniral force-pushed the niral_isis_integration branch from 4112c94 to 0efb0af Compare April 7, 2020 14:06

wivory approved these changes Apr 14, 2020

View reviewed changes

deastoe reviewed May 22, 2020

View reviewed changes

hariosniral force-pushed the niral_isis_integration branch from e0f285f to 196d460 Compare May 26, 2020 10:31

deastoe self-requested a review October 6, 2020 10:44

deastoe approved these changes Oct 9, 2020

View reviewed changes

VyattaInternal merged commit 195cda2 into danos:master Oct 9, 2020


		This module implements the ISIS operational CLI.";

		revision 2018-11-22 {

Integration of FRR ISIS Routing protocol to DANOS. #3

Integration of FRR ISIS Routing protocol to DANOS. #3

Conversation

KaushikNiral commented Mar 16, 2020

odd22 commented Mar 17, 2020

JammyStuff commented Mar 17, 2020

odd22 commented Mar 17, 2020

Choose a reason for hiding this comment

JammyStuff commented Mar 17, 2020

odd22 commented Mar 17, 2020

JammyStuff commented Mar 17, 2020

odd22 commented Mar 17, 2020

jsouthworth commented Mar 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaushikNiral commented Mar 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deastoe commented Mar 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deastoe Mar 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaushikNiral commented Mar 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wivory Apr 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wivory left a comment

Choose a reason for hiding this comment

KaushikNiral commented Apr 15, 2020

dewi-morgan commented May 22, 2020

jsouthworth commented Mar 17, 2020 •

edited

Loading

deastoe Mar 23, 2020 •

edited

Loading

wivory Apr 3, 2020 •

edited

Loading