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.

Sign up for GitHub

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

Jump to bottom

Exposing FRR's config to host FS and adjusting build-infra accordingly. #2008

Closed
wants to merge 1 commit into from
Closed

Exposing FRR's config to host FS and adjusting build-infra accordingly. #2008

wants to merge 1 commit into from

Conversation

rodnymolina
Copy link
Contributor

[ PR not for merging purposes. My intention here is to simply answer the various questions that we have been getting from SONiC community concerning the approach we (Linkedin) follow to interact with FRR stack. Hope it helps. ]

Functionally speaking, these are the changes being introduced in this patch:

  • FRR's config is now exposed to the host FS.
  • FRR is now relying on the unified-configuration model -- one single config file for all the FRR daemons -- frr.conf
  • FRR is now bypassing all minigraph.xml/config_db.json parsing logic, so FRR's docker entry-point logic is considerably simplified.
  • A new 'sonic' user is created to allow critical applications to operate with non-root privileges.
  • Corresponding 'uid-gid' is being exposed in rules/config to allow users to define their desired values.
  • FRR applications now run as 'sonic:sonic'.

Signed-off-by: Rodny Molina [email protected]

Exposing FRR's config to host FS and adjusting build-infra accordingly.
922b7b6 
[ PR not for merging purposes. My intention here is to simply answer the various questions that we have been getting from SONiC community concerning the approach we (Linkedin) follow to interact with FRR stack. Hope it helps. ]

Functionally speaking, these are the changes being introduced in this patch:

  * FRR's config is now exposed to the host FS.
  * FRR is now relying on the unified-configuration model -- one single config file for all the FRR daemons -- frr.conf
  * FRR is now bypassing all minigraph.xml/config_db.json parsing logic, so FRR's docker entry-point logic is considerably simplified.
  * A new 'sonic' user is created to allow critical applications to operate with non-root privileges.
  * Corresponding 'uid-gid' is being exposed in rules/config to allow users to define their desired values.
  * FRR applications now run as 'sonic:sonic'.

Signed-off-by: Rodny Molina <[email protected]>
@lguohan
Copy link
Collaborator

lguohan commented Sep 1, 2018

what is the motivation of exposing the config within docker to host FS?

@rodnymolina
Copy link
Contributor Author

As you know, one of the main issues we have with the routing configuration is the reduced set of features that SONiC supports -- both our jinja2 templates and our configDB schemas would need to be extended to support the richer set of functionalities provided by FRR/Quagga. Till we get there, the obvious solution is to place FRR configuration somewhere else, and have FRR daemons parse this file directly, without SONiC involvement.

Now, it's true that, in principle, you don't need to place frr.conf file in the host to achieve this goal -- we could also place it within the bgp container, but i see a few problems with that:

  • Inconsistency: Users make use of config_db.json for most of the system's configuration except for the routing-stack, which is now not only kept in a different file, but also in an entirely different context. While placing frr.conf in the host FS, we are somewhat mitigating this 'inconsistency' factor.

  • Security: User would need to be able to login into the routing container to be able to directly edit frr.conf file, which is something that many of us don't feel comfortable with, specially since most of our backend daemons run with admin/root privileges.

  • No config backup: Keeping the routing config within a single file, and having that file placed inside the same execution context that parse/edit it, could cause some headaches down the road. If you inadvertently remove your docker container, or if you ever perform a docker-image upgrade, you could easily loose your routing configuration.

@lguohan lguohan closed this May 9, 2019
stepanblyschak added a commit to stepanblyschak/sonic-buildimage that referenced this pull request Nov 11, 2021
@stepanblyschak
[sonic-swss] update submodule
198d136 
```
5f8ebfa (HEAD, origin/master, origin/HEAD, master) [AclOrch] move ACL counters to flex counter infrastructure (sonic-net#1943)
8119ec0 [bfdorch] Orchagent support hardware BFD (sonic-net#1883)
15074ac [sonic-swss]:enable unconfiguring PFC on last TC on a port (sonic-net#1962)
05c7c05 [Mux orch] set default as standby, change mux orch priority (sonic-net#2010)
fe5b2a9 [pytest]: Ignore errors deleting host ifs (sonic-net#2005)
70da9af [ci]: use native arm64 and armhf pool (sonic-net#2013)
e14a071 [qos] Add EXP to TC map support (sonic-net#1954)
c91a7f2 [switchorch] Implement VXLAN src port range feature  (sonic-net#1959)
b20f0f4 Gcov for swss daemon (sonic-net#1737)
01c243a [CRM][MPLS] Fix the mpls nexthop CRM attribute (sonic-net#2008)
8448a60 [vs tests]Migrating sonic-swss tests to use hwsku instead of fakeplatform (sonic-net#1978)
faa26db Fix random failure in PR/CI build. (sonic-net#2006)
e03edb6 Allow interface type value none (sonic-net#1991)
71b9650 [orchagent] Fix group name of port-buffer-drop in flexcounterorch.cpp (sonic-net#1967)
facdef5 [VS test] Skip flaky virtual chassis test (sonic-net#2004)
8261c1f [pytest]: Increase timeout when checking services (sonic-net#2000)
67278be [teammgrd]: Handle LAGs cleanup gracefully on Warm/Fast reboot. (sonic-net#1934)
e92c1df Enable FEC statistics collection for Ethernet ports (sonic-net#1994)
9f30ca1 VxLAN Tunnel Counters and Rates implementation (sonic-net#1859)
ac3103a Add missing neighbor resolution for MPLS route programming (sonic-net#1968)
bfba0ad [vlanmgr]Fix for STATE_DB port check logic (sonic-net#1980)
9ef2ba4 [vlanmgr]: Update VLAN removal code to work with 5.10 kernel and newer iproute2 versions (sonic-net#1970)
41fb26c [Mux orch] Handle setting unknown mux state (sonic-net#1984)
ac09bde [azp]: Increase timeout for VS tests (sonic-net#1988)
da8a43e [pytest]: Check if appl DB exists before deleting (sonic-net#1983)
553d75a [tunnel decap] Change tunnel orch order (sonic-net#1977)
7444e96 [macsecmgr]: Add rekey period in macsec mgr (sonic-net#1958)
d95823d [Buffermgr]Graceful handling of buffer model change (sonic-net#1956)
b0aa6a0 EVPN VxLAN enhancement to support P2MP tunnel based programming for Layer2 extension (sonic-net#1858)
85bdf54 Fix the option missing in kernel config issue (sonic-net#1973)
6b15584 Orchagent validates mirror session queue parameter against maximum value from SAI (sonic-net#1957)
fc9ffb9 [copp] Add ISIS, LDP and micro-BFD trap types to CoPP manager (sonic-net#1890)
452cbc1 [macsecorch]: Add IPG adjusting for MACsec gearbox model (sonic-net#1925)
```

Signed-off-by: Stepan Blyschak <[email protected]>
stepanblyschak added a commit to stepanblyschak/sonic-buildimage that referenced this pull request Nov 11, 2021
@stepanblyschak
[sonic-swss] update submodule
aef49e3 
```
5f8ebfa (HEAD, origin/master, origin/HEAD, master) [AclOrch] move ACL counters to flex counter infrastructure (sonic-net#1943)
8119ec0 [bfdorch] Orchagent support hardware BFD (sonic-net#1883)
15074ac [sonic-swss]:enable unconfiguring PFC on last TC on a port (sonic-net#1962)
05c7c05 [Mux orch] set default as standby, change mux orch priority (sonic-net#2010)
fe5b2a9 [pytest]: Ignore errors deleting host ifs (sonic-net#2005)
70da9af [ci]: use native arm64 and armhf pool (sonic-net#2013)
e14a071 [qos] Add EXP to TC map support (sonic-net#1954)
c91a7f2 [switchorch] Implement VXLAN src port range feature  (sonic-net#1959)
b20f0f4 Gcov for swss daemon (sonic-net#1737)
01c243a [CRM][MPLS] Fix the mpls nexthop CRM attribute (sonic-net#2008)
```

Signed-off-by: Stepan Blyschak <[email protected]>
lguohan pushed a commit that referenced this pull request Nov 15, 2021
@stepanblyschak
[sonic-swss] update submodule (#9232)
0e057a4 
```
5f8ebfa (HEAD, origin/master, origin/HEAD, master) [AclOrch] move ACL counters to flex counter infrastructure (#1943)
8119ec0 [bfdorch] Orchagent support hardware BFD (#1883)
15074ac [sonic-swss]:enable unconfiguring PFC on last TC on a port (#1962)
05c7c05 [Mux orch] set default as standby, change mux orch priority (#2010)
fe5b2a9 [pytest]: Ignore errors deleting host ifs (#2005)
70da9af [ci]: use native arm64 and armhf pool (#2013)
e14a071 [qos] Add EXP to TC map support (#1954)
c91a7f2 [switchorch] Implement VXLAN src port range feature  (#1959)
b20f0f4 Gcov for swss daemon (#1737)
01c243a [CRM][MPLS] Fix the mpls nexthop CRM attribute (#2008)
```

Signed-off-by: Stepan Blyschak <[email protected]>
@dprital dprital mentioned this pull request Jan 27, 2022
Merged
5 tasks
theasianpianist pushed a commit to theasianpianist/sonic-buildimage that referenced this pull request Feb 5, 2022
@smaheshm
[CRM][MPLS] Fix the mpls nexthop CRM attribute (sonic-net#2008)
01c243a
liat-grozovik pushed a commit that referenced this pull request Feb 7, 2022
@dprital
[submodule] Update sonic-utilities submodule (#9871)
cd77efa 
Update submodule sonic-utilities that contains the following commits:

[build] allowPartiallySucceededBuilds: true (#2043)
[system-health] Remove booting stage in system health service (#2022)
[GCU] Mark children of bgp_neighbor as create-only (#2008)
[generic_config_updater] Minor update - No logical code change (#2028)
[generic-config-updater] Handle failed service restarts (#2020)
[debug dump] Missing Dict Key handled in the MatchOptimizer (#2014)
[Auto Techsupport] Added Event Driven TS to Command Reference (#1985)
taras-keryk pushed a commit to taras-keryk/sonic-buildimage that referenced this pull request Apr 28, 2022
@ghooo
[GCU] Mark children of bgp_neighbor as create-only (sonic-net#2008)
01dfb9c 
#### What I did

Fixes sonic-net#2007 

Most of the children of `/BGP_NEIGHBOR/*` except `admin_status` are create-only field i.e. they can only be created with the neighbor but cannot be modified later.

Validated each attribute is read-only by the following steps:
* Delete a neighbor
* Add the neighbor back without the attribute under test e.g. `holdtime`
* show running config for the neighbor
* show neighbor config using `show ip bgp neighbor <ip>`
* Add just the attribute under test e.g. `holdtime`
* show running config for the neighbor -- we can see the attribute is added
* show neighbor config using `show ip bgp neighbor <ip>` -- we can see the attribute change did not take effect

Example for `holdtime`:
```sh
admin@vlab-01:~$ sudo config apply-patch remove-bgp-neighbor.json -i '' 
.
.
.
Patch applied successfully.
admin@vlab-01:~$ sudo config apply-patch remove-bgp-neighbor.json -i ''
.
.
.
Error: can't remove a non-existent object '10.0.0.57'
admin@vlab-01:~$ sudo config apply-patch add-bgp-neighbor-without-holdtime.json -i ''
Patch Applier: Patch application starting.
Patch Applier: Patch: [{"op": "add", "path": "/BGP_NEIGHBOR/10.0.0.57", "value": {"admin_status": "up", "asn": "64600", "keepalive": "3", "local_addr": "10.0.0.56", "name": "ARISTA01T1", "nhopself": "0", "rrclient": "0"}}]
.
.
.
Patch applied successfully.
admin@vlab-01:~$ show runningconfiguration all | grep 10.0.0.57 -A8
        "10.0.0.57": {
            "admin_status": "up",
            "asn": "64600",
            "keepalive": "3",
            "local_addr": "10.0.0.56",
            "name": "ARISTA01T1",
            "nhopself": "0",
            "rrclient": "0"
        },
admin@vlab-01:~$ show ip bgp neighbors 10.0.0.57
.
.
. 
  Hold time is 180, keepalive interval is 3 seconds
.
.
. 
admin@vlab-01:~$ sudo config apply-patch add-holdtime.json -i ''
Patch Applier: Patch application starting.
Patch Applier: Patch: [{"op": "add", "path": "/BGP_NEIGHBOR/10.0.0.57/holdtime", "value": "10"}]
.
.
. 
Patch applied successfully.
admin@vlab-01:~$ show runningconfiguration all | grep 10.0.0.57 -A10
        "10.0.0.57": {
            "admin_status": "up",
            "asn": "64600",
            "holdtime": "10",
            "keepalive": "3",
            "local_addr": "10.0.0.56",
            "name": "ARISTA01T1",
            "nhopself": "0",
            "rrclient": "0"
        },
        "10.0.0.59": {
admin@vlab-01:~$ show ip bgp neighbors 10.0.0.57
BGP neighbor is 10.0.0.57, remote AS 64600, local AS 65100, external link
.
.
. 
  Hold time is 180, keepalive interval is 3 seconds
.
.
. 
admin@vlab-01:~$ 
```

Also added a validation to `create-only` fields to reject moves that add their parents without them, because we would have to delete their parents again later and add it back. There is no point.
Example assume we have 2 fields marked with create-only namely x,y and they are under c. 
The patch would be:
```
{"op":"add", "path":"/a/b/c", "value":{"x":"value_x", "y":"value_y"}}
```
The generated moves would be:
```
{"op":"add", "path":"/a/b/c", "value":{"x":"value_x"}}
{"op":"remove", "path":"/a/b/c"}
{"op":"add", "path":"/a/b/c", "value":{"x":"value_x", "y":"value_y"}}
```

There is no point of the first 2 moves, because the `y` is create only and it will require the object to be deleted again then added. 


#### How I did it
Marked the fields as create only

#### How to verify it
unit-test

#### Previous command output (if the output of a command-line utility has changed)

#### New command output (if the output of a command-line utility has changed)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Enhancement ➕
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rodnymolina @lguohan