cli: add --system param to allow validating system user-data on a machine by blackboxsw · Pull Request #575 · canonical/cloud-init

blackboxsw · 2020-09-15T22:42:31Z

Proposed Commit Message

cli: add --system param schema to allow validating system user-data

Allow root user to validate the userdata provided to the launched a machine using:
cloud-init devel schema --system

Test Steps

lxc launch ubuntu-daily:xenial dev-x
lxc file push cloudinit/config/schema.py dev-x/usr/lib/python3/dist-packages/cloudinit/config/
lxc exec dev-x bash
$ cloud-init devel schema --system

Checklist:

My code follows the process laid out in the documentation
I have updated or added any unit tests accordingly
I have updated or added any documentation accordingly: NOTE: separating auto-generated documentation for all CLI commands to a separate PR

mitechie · 2020-09-16T13:08:07Z

    parser.add_argument('-c', '--config-file',
                        help='Path of the cloud-config yaml file to validate')
+    parser.add_argument('--system', action='store_true', default=False,
+                        help='Validate the system cloud-config userdata')


shouldn't there be associated doc updates with adding a new argument to UI?

mitechie · 2020-09-16T13:09:27Z

-Another option is to run the following on an instance when debugging:
+Another option is to run the following on an instance to debug userdata
+provided to the system:



This is the doc update but it seemed an edit to an existing doc and didn't seem clear to me.

Yeah, devel commands, and general CLI documentation, isn't great. In the devel case I had originally left it fairly undocumented as these devel commands will likely change form or break compatibility before we decide to make them fully fledged top-level (non-devel) subcommands of cloud-init. But, now that we are referencing subcommand switches in docs in other places. it would be helpful to have a more full rendering of cli docs for this devel subcommands. I ultimately think we should autogeerate docs from the argparse configs so that we can keep that up to date any time the cli is changed. I think sphinx-argparse extension could do this for us, but I'd like to leave that to a separate PR as it'll potentially involve re-organizing the top-level CLI docs docs. As I think we could try to modify how the CLI docs in general are exposed and documented.

That said, I did just push a commit that renders cmdline devel schema to this branch. I think we can build on this if an acceptable format for all CLI commands.

OddBloke

Thanks Chad, this is a nice little UX improvement. A couple of inline comments.

OddBloke · 2020-09-16T13:56:14Z

 SCHEMA_EXAMPLES_HEADER = '\n**Examples**::\n\n'
 SCHEMA_EXAMPLES_SPACER_TEMPLATE = '\n    # --- Example{0} ---'

+SYSTEM_USERDATA_FILE = "/var/lib/cloud/instance/user-data.txt"


This should really be using a Paths object rather than hardcoding a path.

@mitechie @OddBloke OOPS while the code no longer relies on this static var, I had forgotten to remove it.
Done.

github-actions · 2020-10-27T00:30:28Z

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging mitechie, and he will ensure that someone takes a look soon.

(If the pull request is closed, please do feel free to reopen it if you wish to continue working on it.)

blackboxsw · 2020-11-04T02:38:51Z

@OddBloke @mitechie I've updated this branch and added auto-generated docs for review for only the devel subcommand using sphinx-argparse. If this documentation approach is palatable. I can add a followup PR to auto-gen docs for our entire CLI as a followup PR. (I could separate that from this PR too).

blackboxsw · 2020-11-05T15:07:03Z

@OddBloke @mitechie separated doc changes into a followup PR as discussed in standup yesterday

mitechie

With a +1 to Dan's comment on the file path at the start of the PR.

OddBloke

Thanks Chad! Overall this looks good, I have one inline comment that I think needs addressing before this can land (and another one which you can ignore because it's a nit 👍).

Platform userdata could be configured to live in a differnt place. Use Paths object to determine that userdata_raw file path. Also add a getuid check if --system param is provided to schema because non-root users cannot access system userdata.

OddBloke

Thanks Chad! Some nits inline, which you can fix in a follow-up if you so desire.

OddBloke · 2020-11-17T21:52:29Z

+    @pytest.mark.parametrize("params", exclusive_combinations)
+    def test_main_exclusive_args(self, params, capsys):
+        """Main exits non-zero and error on required exclusive args."""
+        params = list(itertools.chain(*[a.split() for a in params]))


A nit: it took me a while to grok what this was doing; I think params = " ".join(params).split() has the same effect?

In [12]: params = ('--docs all', '--config-file something') In [13]: list(itertools.chain(*[a.split() for a in params])) Out[13]: ['--docs', 'all', '--config-file', 'something'] In [14]: " ".join(params).split() Out[14]: ['--docs', 'all', '--config-file', 'something']

OddBloke · 2020-11-17T21:57:47Z

+        """Main exits non-zero and error on required exclusive args."""
+        params = list(itertools.chain(*[a.split() for a in params]))
+        with mock.patch('sys.argv', ['mycmd'] + params):
+            with pytest.raises(SystemExit) as context_manager:


A nit: the pytest docs use excinfo for these, and we follow that pattern elsewhere in the codebase:

$ a -s pytest.raises.\*as cloudinit/tests/test_conftest.py 28: with pytest.raises(AssertionError) as excinfo: 35: with pytest.raises(AssertionError) as excinfo:

We also broadly follow the pattern in UA client and pycloudlib, though there is more divergence there.

OddBloke · 2020-11-17T22:00:37Z

+        )
+        assert expected == err
+
+    def test_main_missing_args(self, capsys):


A nit (unless I'm wrong, then not even that): you could potentially fold this into the above definition by making the parameters list(exclusive_combinations) + [()].

OddBloke · 2020-11-17T22:20:27Z

+        ["--system", "--docs all", "--config-file something"], 2
+    )
+
+    @pytest.mark.parametrize("params", exclusive_combinations)


Nit: a not-particularly-important gap here is all three being specified at once.

OddBloke · 2020-11-17T22:36:04Z

-    content = load_file(config_path, decode=False)
+    if config_path is None:
+        # Use system's raw userdata path
+        if os.getuid() != 0:


This file is always root:root-owned by default and not world-readable, so this is not even a nit: we could instead rely on the user's permissions to determine if they can use this path.

Ensure `cloud-init devel schema` indicates valid userdata is valid and invalid userdata is invalid.

This currently covers functionality added in canonical#575

This currently covers functionality added in #575

mitechie reviewed Sep 16, 2020

View reviewed changes

OddBloke suggested changes Sep 16, 2020

View reviewed changes

OddBloke self-assigned this Sep 17, 2020

github-actions Bot added the stale-pr Pull request is stale; will be auto-closed soon label Oct 8, 2020

mitechie removed the stale-pr Pull request is stale; will be auto-closed soon label Oct 12, 2020

canonical deleted a comment from github-actions Bot Oct 12, 2020

github-actions Bot added the stale-pr Pull request is stale; will be auto-closed soon label Oct 27, 2020

blackboxsw force-pushed the schema-system branch from 314ca95 to 10b4174 Compare November 3, 2020 20:51

blackboxsw removed the stale-pr Pull request is stale; will be auto-closed soon label Nov 4, 2020

blackboxsw force-pushed the schema-system branch 2 times, most recently from 20aebc0 to 2c1eb9d Compare November 5, 2020 14:48

mitechie approved these changes Nov 10, 2020

View reviewed changes

blackboxsw force-pushed the schema-system branch from 2c1eb9d to 55245a3 Compare November 10, 2020 22:16

OddBloke suggested changes Nov 13, 2020

View reviewed changes

Comment thread cloudinit/config/schema.py Outdated

Comment thread cloudinit/config/schema.py Outdated

blackboxsw added 3 commits November 16, 2020 09:14

cli: add --system to allow validating system user-data on a machine

48c7e79

schema: correct exclusive args handling docs, config-file, system

b3d7bcd

blackboxsw force-pushed the schema-system branch from 55245a3 to b3d7bcd Compare November 16, 2020 19:37

blackboxsw requested a review from OddBloke November 16, 2020 20:35

OddBloke approved these changes Nov 17, 2020

View reviewed changes

Merge branch 'master' into schema-system

5187f62

OddBloke merged commit f680114 into canonical:master Nov 18, 2020

TheRealFalcon added a commit to TheRealFalcon/cloud-init that referenced this pull request Dec 15, 2020

Integration test for GH canonical#575

1bf3474

Ensure `cloud-init devel schema` indicates valid userdata is valid and invalid userdata is invalid.

TheRealFalcon added a commit to TheRealFalcon/cloud-init that referenced this pull request Dec 15, 2020

Add integration tests for CLI functionality

d08a670

This currently covers functionality added in canonical#575

TheRealFalcon mentioned this pull request Dec 15, 2020

Integration test for #575 #729

Merged

3 tasks

TheRealFalcon added a commit that referenced this pull request Dec 15, 2020

Add integration tests for CLI functionality (#729)

9e89ca7

This currently covers functionality added in #575

This was referenced May 12, 2023

cloud-init regenerating ssh-keys #3753

Closed

Release 20.4 #3811

Closed

Conversation

blackboxsw commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Commit Message

Test Steps

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OddBloke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Oct 27, 2020

Uh oh!

blackboxsw commented Nov 4, 2020

Uh oh!

blackboxsw commented Nov 5, 2020

Uh oh!

mitechie left a comment

Choose a reason for hiding this comment

Uh oh!

OddBloke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

OddBloke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

blackboxsw commented Sep 15, 2020 •

edited

Loading