refine configuration provisioning

[hellt]

Hi @kellerza @karimra
I merged the #383. Sorry it took so long, @kellerza, I wish I could get to it sooner... Thank you for stepping in and delivering the feature implementation of the config generation and provisioning. It was a lot of work, and you made a very important first step!

I propose to open this forum to discuss how do we take it from here, because although it is a working solution, it has quite a few things which are somewhat opinionated and not generic.

Let me summarize here what I did on top of the #383 after the last commit of @kellerza:

1. the way to provide variables for templation

I followed the original idea of providing variables for nodes and links to not introduce any breaking change. But we need to discuss this, as we may have a stronger design here. @karimra that is the issue we discussed the other day.

Originally the labels attribute of nodes and links was used to keep variables that are used in config templation.
I changed that to the following:

1. Node variables are defined with .topology.nodes.<node>.config.vars. This is a map[string]string structure which allows flat variables pairs in the form of var-name:var-value.
2. link variables are provided with .topology.links[].vars

That change required me to change the original .topology.nodes.<node>.config to ``.topology.nodes..startup-configto free up theconfig` key.

The .topology.nodes.<node>.config is of type *ConfigDispatcher. It is not a simple map for a single reason that it was envisioned to grow and incorporate transport selection, transport options, template names to use.

Consequently, I had to create a few methods for Topology and NodeDefinition struct, namely GetConfigDispatcher

An example of the variables defined in clab file:

name: srlcfg

topology:
  nodes:
    srl1:
      kind: srl
      image: registry.srlinux.dev/pub/srlinux:21.3.1-410
      license: license.key
      config:
        vars:
          systemip: 10.0.0.1/32

    srl2:
      kind: srl
      image: registry.srlinux.dev/pub/srlinux:21.3.1-410
      license: license.key
      config:
        vars:
          systemip: 10.0.0.2/32

  links:
    - endpoints: ["srl1:e1-1", "srl2:e1-1"]
      vars:
        port: ethernet-1/1, ethernet-1/1

2 providing templates to use for templation

The templates that will be rendered on a per-node basis are provided via two flags of the config subcommand

• --template-path - recieves a list of paths where the actual template files will be looked at
• --template-list - takes a list of names of the templates files that will be searched in paths provided with the flag above

The --template-list requires a bit of an explanation. The pattern that the templates of the current code are using is <name>__<role>.tmpl.

The name is a free formed string, whereas the role is a suffix that denotes to which role this template belongs to. The idea here is that in a multivendor lab or in a lab with multiple roles the devices would require different templates.
The role by default is assumed to be the kind of the node, but it is configurable.

Let's imagine we want to create a template that configures eBGP between SRL and SR OS, we could create the following files

• bgp__srl.tmpl
• bgp__vr-sros.tmpl

and then by doing clab config -t <topo> --template-path <path> --template-list bgp we are able to pick the right template per each node of the lab, based on the kind selector.

A very basic example of the template for interface provisioning on SRL can look like this:

{{ expect .systemip "ip" }}
{{ range .links }}
    {{ expect .port "^(ethernet-\\d+/|e\\d+-)\\d+$" }}
    {{ expect .name "string" }}
{{ end }}

/interface lo0 {
    admin-state enable
    subinterface 0 {
        ipv4 {
            address {{ .systemip }} {
            }
        }
        ipv6 {
            address ::c1ab:{{ ip .systemip }}/128 {
            }
        }
    }
}

/network-instance default {
    router-id {{ ip .systemip }}
    interface lo0.0 {
    }
}

/system lldp admin-state enable

/ network-instance mgmt {
    description "set from clab"
}

{{ range .links }}
/interface {{ .port }} {
    admin-state enable
}
{{ end }}

If, say, you have this file under ./private/iface__srl.tmpl, then you can configure your SRL nodes of the lab shown above with the following command

# by omitting --template-list the code will take all files available in the search path
clab config -t srl-cfg-demo.clab.yml --tempalte-path ./private 

Open points

1. I know that @karimra wanted to re-eval the way the vars are split between the nodes and links. Our ultimate goal is to be able to specify tree based vars, to avoid the limitations of a flat variables structure.

2. @kellerza It would be interesting to see what warranted to create a wrapper around stdlib text/template. I am not a fan of using custom wrappers until necessary. I remember before you had a functions map that you supplied to text/template

3. regardless of 1) there are a few hard-coded assumptions in the code that does variables preparation for links. For example, it is expected that system ip is always provided in the vars -

   containerlab/clab/config/utils.go

   Line 75 in 929abda

   ipA, ipB, err := linkIPfromSystemIP(link)

   
   I would rather assume that we should let users decide which variables need to be present. The code should only merge variables and pass them to the template

[kellerza]

🎉 !

open points

2 template wrapper

This contains various helper functions (like ip & expect used in your example template above splitting&testing used in the sros template) and respective tests for these. I also overwrite standard text/template functions to enable them to be piped & at the same time keep them backward compatible to text/template.

It was another 800-1000 lines of code that is purely used to make text/template usable for this purpose, and adding some functions that one would expect coming from ansible/jinja. So during the review we decided to split it out

in the future functions for assigning IPs from pools can be added here as well

3 variable prep

This is quite important for links, else you have to explicitly specify an IP on each, but with some more thought I can probably move this to template functions as well

[kellerza]

The majority of the feedback so far is that people have used/has templates with Jinja syntax, so sprig based on underscore function are not ideal. They also don't overwrite standard functions. There are some other, but in all cases it will also need extending. Things like negative indexes is important for me, etc. The SROS example in the PR uses a lot more of these

They also do not have the use case for expect/default, to supply mandatory/optional parameters with validation.

The entire wrapper today only adds the functions, but my intention is that is can convert variables to not be flat, as in the following example, to provide a cleaner template syntax

Today:

{{ expect .systemip "ip" }}
ip {{ ip .systemip }}
mask {{ mask .systemip }}

can become

{{ expect .systemip "ip" }}
ip {{ .systemip.ip }}
mask {{ .systemip.mask }}

[karimra]

I'm not suggesting to use sprig specifically, I'm saying the std template package can be augmented with functions,
there is no need to create a wrapper package.
You can add multiple function maps as well, your own custom functions as well as sprig.

If the wrapper adds only functions why have it?
I can see it adds some restrictions, like assuming the variables are always map[string]interface{}, and not a list.

if we go this way, we are creating a containerlab specific templating syntax, I don't think this is a good idea.
There was multiple attempts at porting jinja2 or twig to Go, or creating new templating syntax, they all failed or stopped being supported. The only safe bet is to stick with Go template.

[kellerza]

I don't have any problem if we just load the template functions. We wont be able to expand the expect function in that case, but we can live without it.

It might also limit autogen functions (like IP/number pools) if you don't control the entire template rendering, but lets see when we get there

[hellt]

I am in favor of using the stdlib template engine over the wrapper and using functions as the means of extension.
The reason is to keep the template design language as used in Go and not deviate towards a bespoke solution. Having custom functions should get us pretty far in making the template feature-rich, but at the same time the overall language is still Go and users will be familiar with it inherently.

I wouldn't look too much on jinja at the same time, it is defo there and many people know it, but so does Go templates getting more and more popular.

So I propose we either

1. move the template functions in a separate package if you wish, but without wrapping the template engine
2. move the functions to clab

going with 1) can be easier if you plan to reuse those functions in other packages (which I see can be a valuable thing)

[kellerza]

Indeed (1) can be re-used. This is already possible today, by using template.Funcs. All template functions are public in order to have them visible with godocs

To unwrap the template package is a clab only change. I'll move some of the wrapper code from the package to clab in a future PR

[karimra]

1. the way to provide variables for templation

Having two sections to define variables can cause some issues,

• What happens if the same key is defined under config.vars and links.[].vars ? which one takes precedence ?
• You will also have 2 distinct parts to keep track of for each node.
• What distinguishes a node variable vs a link variable? looks like an unnecessary subjective decision.

I think it's better to keep variables in one place. The link's labels can be used to influence how an interface is configured, e.g:
( not proper template syntax )

{{ range $link := .Links }}
  {{ if hasLabel $link "label1" }}
    CfgA
  {{ else if hasLabel $link "label2" }}
    CfgB
  {{ end}}
{{ end}}

2. providing templates to use for templation

The templates can be a global variable, a path that is recursively parsed to load all the templates.
the loaded templated are then matched with the nodes using the file name, according to the same proposed schema __.tmpl

Open points

1. same as what is written in 1. above, flat variables structure can be avoided with map[string]interface{}. We just have to decide how deep a merge should go, how to merge lists and how to handle duplicated items in a list.

[karimra]

I don't get why you say it's not possible.
Given a node name and an interface name, the far node can be retrieved. That can be one of the functions added to the template.

Can you explain why it's not possible?

[kellerza]

Well maybe you can do it if that template function uses global vars, but is this really a good design? (This is in my view similar to trying to bind some context to the template function)

I would much prefer a design where all vars needed for execution is passed to the template render function

When you render a node it does not have access to other nodes or the topo file. Only what you passed to it, which is the labels, links (+labels) and these _far end variables I refer to

[karimra]

There is no need to use global vars.

Together with the node, the whole topology can be passed to the template.
something like:

type templateInput struct {
   Node     *types.NodeConfig   // the node for which the template is rendered, even if it can be considered redundant
   Topology *types.Topology
}

[kellerza]

So now we have gone full circle.

So lets just keep on adding variables to the map we pass today. Not enforcing any specific keys to be present and leave it up to the template designer to use what they need

[karimra]

There is no enforcement of any key to be present, I'm not sure where you see this.

If the struct above is used as is, the variables are just accessed in a different way. .Node.Config.Vars
To the same struct, a Vars map[string]interface{} can be added, the vars will be access with .Vars
If you don't like the .Vars notation, add a function vars or config that returns .Vars

I'm not saying it should be this way, just saying that there are other ways to achieve the same things

[kellerza]

#495 addresses the issues of required systemip and all additional vars should now be optional. Also posted a set of slide on the Teams channel

@hellt you want me to work on # 4?

Introduce the free-formed variables structure map[string]interface{} instead of a flat list of key-val pairs.

[hellt]

thanks @kellerza
will have a look at #495

sure, if you can, please feel free to "Introduce the free-formed variables structure map[string]interface{} instead of a flat list of key-val pairs."