op-mode: T4038: Python rewrite of image tools #2507

erkin · 2023-11-20T05:06:20Z

Change Summary

Rewrite of vyatta-image-tools.pl. Brings with it a minor change to the template schema to add a special case for comptype. Please see the corresponding PR on vyatta-op below.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Code style update (formatting, renaming)
Refactoring (no functional changes)
Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
Other (please describe):

Related Task(s)

https://vyos.dev/T4038

Related PR(s)

op-mode: T4038: Remove legacy vyatta-image-tools.pl vyatta-op#84

Component(s) name

op-mode, image tools

Proposed changes

It should be functionally identical with the Perl implementation, except it eschews external programs like rsync and curl in favour of in-house routines and there are a few user-facing cosmetic changes.
It adds the <imagePath/> empty tag to the XML schema as an option for <completionType>. It should theoretically work with others like <list> and <script> too, but I haven't tried it yet. (I'll try it with add system image later.)
A certain is_interactive() check for known hosts is replaced with isatty() since it can be gracefully answered by automated scripts and because it was preventing the SFTP operations of copy file.
I tested all routines, except for some cases of copy file upload but it would definitely benefit from more eyes if we're going to backport this to 1.4.

How to test

$ copy file [<image>://<path>|<url>] to [<image>://<path>|<url>]
$ delete file <image>://<path>
$ show file [<image>://<path>|<url>]
$ clone system config <image>
$ clone system config <image> from <image>

Smoketest result

Checklist:

I have read the CONTRIBUTING document
I have linked this PR to one or more Phabricator Task(s)
I have run the components SMOKETESTS if applicable
My commit headlines contain a valid Task id
My change requires a change to the documentation
I have updated the documentation accordingly

erkin · 2023-11-20T05:10:22Z

Before you approve/merge, I have a very important question to those familiar with vyatta-op's workings: There are checks for admin permissions in the deleted template definitions. Is this still necessary? I omitted it because $VYATTA_USER_LEVEL_DIR seems to be set to /opt/vyatta/etc/shell/level/admin for all users by default.
There are no such checks in the current implementation and there are no calls to sudo in the new template definitions. If it is necessary, please let me know because its omission could be a huge security hole.

dmbaturin · 2023-11-23T15:31:17Z

@erkin Since it's no longer possible to set the user level to operator (we removed that option), I believe it's safe to just set it to admin for all users.

jestabro · 2023-12-04T21:15:37Z

src/op_mode/file.py

+                if os.path.isdir(destination_path):
+                    destination_path = os.path.join(destination_path, os.path.basename(source))
+                shutil.copytree(source, destination_path, copy_function=zealous_copy)
+            else:


A missing check here causes problems, in applying 'chown' to the destination directory:

diff --git a/src/op_mode/file.py b/src/op_mode/file.py index b09631ba2..b675afebf 100755 --- a/src/op_mode/file.py +++ b/src/op_mode/file.py @@ -244,6 +244,8 @@ def copy(source_type: str, source_path: str, destination_path = os.path.join(destination_path, os.path.basename(source)) shutil.copytree(source, destination_path, copy_function=zealous_copy) else: + if os.path.isdir(destination_path): + destination_path = os.path.join(destination_path, os.path.basename(source)) zealous_copy(source, destination_path) else: print_error(f'Unknown destination type: {source_type}')

I moved the shared check outside the conditional for simplicity.

jestabro · 2023-12-07T04:31:52Z

This one is a simplification using some of the utils from the image-tools revision and, for consideration, dropping the 'disk-install' case --- my impression is that it is antiquated/obsolete, but I may be missing something; we should check with @dmbaturin

diff --git a/src/completion/list_images.py b/src/completion/list_images.py
index 2f22c2aab..e5af8b7ae 100755
--- a/src/completion/list_images.py
+++ b/src/completion/list_images.py
@@ -16,25 +16,25 @@
 
 import argparse
 import os
-import re
 import sys
 
+from vyos.system.image import is_live_boot
+from vyos.system.image import get_running_image
+
 parser = argparse.ArgumentParser(description='list available system images')
 parser.add_argument('--no-running', action='store_true',
                     help='do not display the currently running image')
 
-def get_current_image() -> str:
-    with open('/proc/cmdline', 'r') as f:
-        return re.match(r'BOOT_IMAGE=/boot/([^/]+)/vmlinuz', f.readline()).group(1)
-
 def get_images(omit_running: bool = False) -> list[str]:
+    if is_live_boot():
+        return []
     images = os.listdir("/lib/live/mount/persistence/boot")
-    # Check if we're running on a live disk
-    if os.path.isdir("/lib/live/mount/persistence/config") and not omit_running:
-        images += 'disk-install'
-    elif omit_running:
-        images.remove(get_current_image())
-    images.remove('grub')
+    if omit_running:
+        images.remove(get_running_image())
+    if 'grub' in images:
+        images.remove('grub')
+    if 'efi' in images:
+        images.remove('efi')
     return sorted(images)
 
 if __name__ == '__main__':

jestabro · 2023-12-15T18:32:04Z

src/op_mode/file.py

+
+def show(type: str, path: str) -> None:
+    if type == 'remote':
+        with tempfile.NamedTemporaryFile(delete=False) as temp:


Since using as a context manager here, one does not want delete=False

jestabro · 2023-12-16T02:34:18Z

Comment added here with reference to issue discussed with Erkin; patch is for the companion PR vyos/vyatta-op#84 and cited below.

Unless op-mode file.py is excluded from piping through VYATTA_PAGER, prompts to input, when called from remote.download(...), are not printed to stdout --- input is still accepted, but the user is at a loss.
Current behavior:

vyos@vyos:~$ show file scp://vyos:[email protected]/home/vyos/files/test-file
Host '192.168.1.5' not found in known hosts.
Fingerprint: b4a8c0f8a8777137a03966fcf25ba58d

... and that's it: 'y'+'enter' or 'enter' is accepted, but prompt is missing.
After excluding from paging:

vyos@vyos:~$ show file scp://vyos:[email protected]/home/vyos/files/test-file
Host '192.168.1.5' not found in known hosts.
Fingerprint: b4a8c0f8a8777137a03966fcf25ba58d
Do you wish to continue? [y/N] y
########## FILE INFO ##########
Path:           /tmp/tmphlsbic_0
Type:           ASCII text
Owner:          root:root
Permissions:    rw-------
Modified:       2023-12-16 02:21:20

########## FILE DATA ##########

slug test-file: vyos on sirius; some data ...

patch for vyatta-op:

diff --git a/functions/interpreter/vyatta-op-run b/functions/interpreter/vyatta-op-run
index ee4cb1c..3260781 100644
--- a/functions/interpreter/vyatta-op-run
+++ b/functions/interpreter/vyatta-op-run
@@ -218,7 +218,7 @@ _vyatta_op_run ()
     local run_cmd=$(_vyatta_op_get_node_def_field $tpath/node.def run)
     run_cmd=$(_vyatta_op_conv_run_cmd "$run_cmd") # convert the positional parameters
     local ret=0
-    local cmd_regex="^(LESSOPEN=|less|pager|tail|/opt/vyatta/bin/vyatta-tshark-interface-port.pl).*"
+    local cmd_regex="^(.*/file.py.*|LESSOPEN=|less|pager|tail|/opt/vyatta/bin/vyatta-tshark-interface-port.pl).*"
     if [ -n "$run_cmd" ]; then
       eval $restore_shopts
       if [[ -t 1 &&  "${args[1]}" == "show" && ! $run_cmd =~ $cmd_regex ]] ; then

There are two things to note:
(1) as op-mode file.py show file does its own paging, piping through VYATTA_PAGER is not recommended nor needed.
(2) The previous fact is not the cause of the bug, as isolating to a simple download is sufficient to trigger; rather this has to do with paramiko's MissingHostKeyPolicy, though details not yet clear..

jestabro · 2024-01-09T17:27:41Z

cf. comment in
vyos/vyatta-op#84

jestabro · 2024-01-19T21:02:57Z

A typo in parse_image_path prevents show/copy across images from working (as it leaves a leading slash in the second arg, so os.path.join is not what one wants):
3804447
Also, one can consider the following for manually setting perms on a file from remote download: it's a bit crude, and up to you whether/how to implement ... just a thought.
212f5d6

github-actions bot added the current label Nov 20, 2023

github-actions bot assigned erkin Nov 20, 2023

vyosbot requested review from a team, dmbaturin, sarthurdev, zdc, jestabro, sever-sever and c-po and removed request for a team November 20, 2023 05:06

erkin mentioned this pull request Nov 20, 2023

op-mode: T4038: Remove legacy vyatta-image-tools.pl vyos/vyatta-op#84

Merged

jestabro reviewed Dec 4, 2023

View reviewed changes

erkin force-pushed the image-tools branch from 1951f29 to 42e6901 Compare December 14, 2023 13:03

jestabro reviewed Dec 15, 2023

View reviewed changes

erkin force-pushed the image-tools branch from 42e6901 to 9274767 Compare December 21, 2023 16:13

op-mode: T4038: Python rewrite of image tools

b76e4c8

erkin force-pushed the image-tools branch from 9274767 to b76e4c8 Compare January 25, 2024 14:39

dmbaturin approved these changes Feb 8, 2024

View reviewed changes

jestabro self-requested a review February 8, 2024 16:10

jestabro approved these changes Feb 8, 2024

View reviewed changes

dmbaturin merged commit 632c9d9 into vyos:current Feb 8, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

op-mode: T4038: Python rewrite of image tools #2507

op-mode: T4038: Python rewrite of image tools #2507

erkin commented Nov 20, 2023

erkin commented Nov 20, 2023 •

edited

Loading

dmbaturin commented Nov 23, 2023

jestabro Dec 4, 2023

erkin Dec 14, 2023

jestabro commented Dec 7, 2023

jestabro Dec 15, 2023

jestabro commented Dec 16, 2023 •

edited

Loading

jestabro commented Jan 9, 2024

jestabro commented Jan 19, 2024

op-mode: T4038: Python rewrite of image tools #2507

op-mode: T4038: Python rewrite of image tools #2507

Conversation

erkin commented Nov 20, 2023

Change Summary

Types of changes

Related Task(s)

Related PR(s)

Component(s) name

Proposed changes

How to test

Smoketest result

Checklist:

erkin commented Nov 20, 2023 • edited Loading

dmbaturin commented Nov 23, 2023

jestabro Dec 4, 2023

Choose a reason for hiding this comment

erkin Dec 14, 2023

Choose a reason for hiding this comment

jestabro commented Dec 7, 2023

jestabro Dec 15, 2023

Choose a reason for hiding this comment

jestabro commented Dec 16, 2023 • edited Loading

jestabro commented Jan 9, 2024

jestabro commented Jan 19, 2024

erkin commented Nov 20, 2023 •

edited

Loading

jestabro commented Dec 16, 2023 •

edited

Loading