Page MenuHomeVyOS Platform
Create Task
Maniphest T1846

Make session_config not depend on the current edit level
Closed, ResolvedPublic
Actions

Description

It seems that now after T1758 / d9ee0b95d1020b6d5412dd011ebb1ef7f6ef3fc7 the session config is not taken from the root level but the current edit level which means that trying to commit will fail if the user is not in the top level.

The config is determined using self._run([self._cli_shell_api, '--show-working-only', '--show-show-defaults', 'showConfig']) which is edit-level dependent:

[edit interfaces ethernet eth0]
vyos@vyos# /bin/cli-shell-api --show-working-only --show-show-defaults showConfig
address 10.0.0.1/24
hw-id 52:54:00:3f:49:25
[edit interfaces ethernet eth0]
vyos@vyos# top
[edit]
vyos@vyos# /bin/cli-shell-api --show-working-only --show-show-defaults showConfig | head
interfaces {
    ethernet eth0 {
        address 10.0.0.1/24
        hw-id 52:54:00:3f:49:25

Originally found out / debugged in T1844.

Details

Difficulty level
Unknown (require assessment)
Version
-
Why the issue appeared?
Will be filled on close
Is it a breaking change?
Unspecified (possibly destroys the router)

Related Objects
Search...

Event Timeline

varesa created this task.Dec 4 2019, 10:06 AM
varesa created this object in space S1 VyOS Public.
varesa added a comment.Dec 4 2019, 10:16 AM

It would be pretty simple to pass VYATTA_{TEMPLATE,EDIT}_LEVEL=/ as environment variables in Config._run():

def _run(self, cmd):    
    if self.__session_env:    
        p = subprocess.Popen(cmd, stdout=subprocess.PIPE, env=self.__session_env)    
    else:    
        p = subprocess.Popen(cmd, stdout=subprocess.PIPE)

However I'm not sure if something else might actually depend on it being edit level aware.

Mainly these:

out = self._run(self._make_command('showConfig', path))
self._run(self._make_command('isMulti', path))
self._run(self._make_command('isTag', path))
self._run(self._make_command('isLeaf', path))

Is path supposed to be absolute or relative to the current edit level?

varesa updated the task description. (Show Details)Dec 4 2019, 10:17 AM
varesa added a comment.Dec 4 2019, 10:21 AM

Seems like that paths are absolute regardless of the environment variables:

[edit interfaces ethernet eth0]
vyos@vyos# /bin/cli-shell-api isLeaf interfaces ethernet eth0 hw-id; echo $?
0
varesa added a comment.Dec 4 2019, 10:37 AM

Seems based on the uses of the Config-class, for example this in vyos-http-api-server:

session = ConfigSession(os.getpid())    
env = session.get_session_env()    
config = vyos.config.Config(session_env=env)

that the intention was to have a clean environment unless called with session_env=something.

However by default the Popen call uses the surrounding environment instead of a clean one.

So the appropriate fix to me seems to be:

diff --git a/python/vyos/config.py b/python/vyos/config.py
index 13b2c10..c9c5ea0 100644
--- a/python/vyos/config.py
+++ b/python/vyos/config.py
@@ -137,7 +137,7 @@ class Config(object):
         if self.__session_env:
             p = subprocess.Popen(cmd, stdout=subprocess.PIPE, env=self.__session_env)
         else:
-            p = subprocess.Popen(cmd, stdout=subprocess.PIPE)
+            p = subprocess.Popen(cmd, stdout=subprocess.PIPE, env={})
         out = p.stdout.read()
         p.wait()
         if p.returncode != 0:
varesa added a comment.Dec 4 2019, 10:49 AM

Okay, that doesn't work. It likely requires some variables like these:

VYATTA_ACTIVE_CONFIGURATION_DIR=/opt/vyatta/config/active
VYATTA_CONFIG_TMP=/opt/vyatta/config/tmp/tmp_1929
VYATTA_CHANGES_ONLY_DIR=/opt/vyatta/config/tmp/changes_only_1929
VYATTA_TEMP_CONFIG_DIR=/opt/vyatta/config/tmp/new_config_1929
varesa added a comment.Dec 4 2019, 10:50 AM

Simple fix could be to just override VYATTA_TEMPLATE_LEVEL and VYATTA_EDIT_LEVEL.

A nicer one would be to whitelist the required variables and otherwise use a clean environment

varesa added a comment.Dec 4 2019, 10:56 AM

This works better:

diff --git a/python/vyos/config.py b/python/vyos/config.py
index 13b2c10..82483cb 100644
--- a/python/vyos/config.py
+++ b/python/vyos/config.py
@@ -137,7 +137,10 @@ class Config(object):
         if self.__session_env:
             p = subprocess.Popen(cmd, stdout=subprocess.PIPE, env=self.__session_env)
         else:
-            p = subprocess.Popen(cmd, stdout=subprocess.PIPE)
+            env = os.environ
+            env['VYATTA_TEMPLATE_LEVEL'] = '/'
+            env['VYATTA_EDIT_LEVEL'] = '/'
+            p = subprocess.Popen(cmd, stdout=subprocess.PIPE, env=env)
         out = p.stdout.read()
         p.wait()
         if p.returncode != 0:
varesa added a comment.Dec 4 2019, 11:23 AM

Created a PR with the above change: https://github.com/vyos/vyos-1x/pull/174

varesa mentioned this in T1844: Fix interface configuration which was broken in T1762 .Dec 4 2019, 11:25 AM
pasik added a subscriber: pasik.Dec 4 2019, 3:23 PM
runar mentioned this in T1863: daemon config lost after commit changes.Dec 8 2019, 4:40 PM
qiuchengxuan mentioned this in T1861: hosts lost after modified static-host-mapping.Dec 9 2019, 9:10 AM
jestabro claimed this task.Dec 9 2019, 5:29 PM
jestabro changed the task status from Open to In progress.Dec 9 2019, 6:19 PM

A parsimonious fix to this issue has been committed; I will move the status to "In progress" until the related issues mentioned here are sorted through.

jestabro added a subtask: T1844: Fix interface configuration which was broken in T1762 .Dec 9 2019, 7:38 PM
jestabro added a subtask: T1863: daemon config lost after commit changes.Dec 9 2019, 7:40 PM
jestabro added a subtask: T1861: hosts lost after modified static-host-mapping.
jestabro closed this task as Resolved.Dec 9 2019, 7:55 PM
jestabro added a subtask: T1851: wireguard - changing the pubkey on an existing peer seems to destroy the running config..
hagbard changed the status of subtask T1851: wireguard - changing the pubkey on an existing peer seems to destroy the running config. from Open to Needs testing.Dec 10 2019, 5:38 PM
hagbard mentioned this in T1851: wireguard - changing the pubkey on an existing peer seems to destroy the running config..
varesa removed a subscriber: varesa.Dec 10 2019, 9:58 PM
jestabro reopened this task as Open.Dec 11 2019, 3:47 PM
hagbard changed the status of subtask T1851: wireguard - changing the pubkey on an existing peer seems to destroy the running config. from Needs testing to Backport pending.Dec 11 2019, 3:55 PM
hagbard closed subtask T1851: wireguard - changing the pubkey on an existing peer seems to destroy the running config. as Resolved.
jestabro added a comment.Dec 11 2019, 4:39 PM

This requirement needs to be applied to 'running_config' as well, in case of (1) editing at the level of an unset node (parse error); (2) reading effective_value(s) in configuration mode (lacking full paths).

jestabro closed this task as Resolved.Dec 11 2019, 4:39 PM
qiuchengxuan closed subtask T1861: hosts lost after modified static-host-mapping as Resolved.Dec 13 2019, 9:02 AM
qiuchengxuan closed subtask T1863: daemon config lost after commit changes as Resolved.
syncer reopened subtask T1851: wireguard - changing the pubkey on an existing peer seems to destroy the running config. as Backport candidate.Jan 1 2020, 2:03 PM
hagbard closed subtask T1851: wireguard - changing the pubkey on an existing peer seems to destroy the running config. as Resolved.Feb 8 2020, 4:15 PM
jestabro added a subtask: T2168: Config edit level affecting the show_config() function.Mar 27 2020, 8:27 PM
dmbaturin closed subtask T1844: Fix interface configuration which was broken in T1762 as Resolved.Jun 18 2020, 10:28 PM