Merge ~mertkirpici/juju-lint:lp/1987951 into juju-lint:master

[Mert Kirpici]

Close LP #1987951

config: make --dump-state boolean

The command line argument --dump-state is in fact used as a boolean flag
however it expects a dummy argument, causing confusion. With this change
it only using it will be enough. i.e.:

  $ juju-lint --dump-state -d outdir -c ...

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[Eric Chen (eric-chen)]

LGTM

review: Approve

[Mert Kirpici (mertkirpici)]

Lets wait until the MP[0] for the functional tests are merged for this. I want to add a functional test for the --dump-state as well.

[0] https://code.launchpad.net/~mertkirpici/juju-lint/+git/juju-lint/+merge/428809

[Gabriel Cocenza (gabrielcocenza)]

Thanks for filling the bug and sending this patch.

Looking at the source code I think we should get rid off this dump-state flag. IMO it just makes harder to use the CLI. If the user passes the output dir, why should we force use another flag to dump it?

Moreover, I see three "big" problems that we should think about it:

1 - help of "output-dir" in the cli says "The folder to use when saving gathered cloud data and lint reports." I've checked and we are not writing lint reports. The dump contains the content of the following juju comands:
 - juju controllers
 - juju status
 - juju export-bundle

2 - the command juju controllers shows sensible information (e.g: ca-cert of the controller). I see this as a security issue and I think it's not a good standard behavior.

3 - Finally, we are not dealing with wrong path from the user. Passing an invalid path raises FileNotFoundError.

review: Needs Fixing

[Mert Kirpici (mertkirpici)]

Rebased the branch and pushed an update to partially address Gabriel's comments.
A small summary of changes:
- I did get rid of the `--output-dir` flag rather than the `--dump-state` since it felt more intuitive for me to use it like "dump the state to DIR", rather than "output to DIR" which begs the question "What output?"

- Now we are checking the dump directory for permissions and existence before running any sort of linting.

- New functests for this feature.

I am thinking of filing bugs and addressing the issues #1(missing lint results in dumps) and #2(sensitive info in dumps) in separate MPs since they are quite serious bugs within themselves.

Last but not least, here is the functest run after this change:
https://pastebin.ubuntu.com/p/bgsfqGwVVh/

[Gabriel Cocenza (gabrielcocenza)]

The patch LGTM, but unit tests are failing because of the changes and because now we need 100% line coverage.

Regarding the bugs, please fill the first one. The second one after talking with Martin, it's not that bad because it's the public certificate of the controller.

Thanks!

review: Needs Fixing

[Mert Kirpici (mertkirpici)]

Hey again, so first of all I did go ahead and file LP#1989673 for the first item you mentioned.
https://bugs.launchpad.net/juju-lint/+bug/1989673

After some thought I decided to deprecate the `--dump-state` rather than directly removing it, so this newly pushed update is mainly dealing with that. It:

- introduces a new argparse.Action subclass for deprecation
- marks `--dump-state` as nargs="*" therefore no matter with how many arguments it is called, it will be valid(and no-op, just log a warning message)
- fixes breaking unittests and adds a test to ensure %100 coverage
- re-renames some tests and function names for consistency's sake

Here is the `make test` output:
https://pastebin.ubuntu.com/p/rqd5tcDVS2/

I do have one request though. If you think this is good to go, could you let me know before merging so that I can squash the commits and add explanation for the changes in the commit message to clear up the intention behind this change?

[Martin Kalcok (martin-kalcok)]

Just one note and I think this is ready for merge. Please include the warning about sensitive data being dumped, that was previously shown in the "--dump-state" help message, in the "--output-dir".

review: Approve

[Eric Chen (eric-chen)]

Please Gabriel Angelo Sgarbi Cocenza confirm this. And please mert provides the log of lint/unit/func if jenking bot is not ready.

Any MR should be

1) 0 "Need Fixing"
2) 2+ approval
3) approval of jenkins bot or provide log of lint/unit/func manually

[Gabriel Cocenza (gabrielcocenza)]

LGTM, but before merging please see my comments and please provide the logs of the tests.

review: Approve

[Mert Kirpici (mertkirpici)]

updated one last time. changes:
- squashed all commits, re-written the commit message, rebased onto current master
- added a forgotten @pytest.fixture to fix the unit tests
- I really liked Gabriel's idea to parametrize the bad output directory tests. so did that.
- Following review comments from Martin and Gabriel, added the warning about sensitive info into the help string and removed that part that mentions lint reports being dumped. Lint reports are not dumped currently, I'd already filed a LP bug about this.

`make test`:
https://pastebin.ubuntu.com/p/tqggRBjmwb/

all green, ready to land this change.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision d89eef7b64063e543bffb1e292152a9118e1dfe9

diff --git a/jujulint/cli.py b/jujulint/cli.py
index 2ea188a..ac85b2b 100755
--- a/jujulint/cli.py
+++ b/jujulint/cli.py
@@ -17,9 +17,11 @@
 # You should have received a copy of the GNU General Public License
 # along with this program. If not, see <http://www.gnu.org/licenses/>.
 """Main entrypoint for the juju-lint CLI."""
+import errno
 import logging
 import os.path
 import sys
+import tempfile
 
 import pkg_resources
 import yaml
@@ -127,6 +129,7 @@ class Cli:
 
     def audit_all(self):
         """Iterate over clouds and run audit."""
+        self._check_output_folder()
         self.logger.debug("Starting audit")
         for cloud_name in self.config["clouds"].get():
             self.audit(cloud_name)
@@ -178,11 +181,27 @@ class Cli:
 
     def write_yaml(self, data, file_name):
         """Write collected information to YAML."""
-        if "dump" in self.config["output"]:
-            if self.config["output"]["dump"]:
-                folder_name = self.config["output"]["folder"].get()
-                file_handle = open("{}/{}".format(folder_name, file_name), "w")
-                yaml.dump(data, file_handle)
+        folder_name = self.config["output"]["folder"].get()
+        if folder_name:
+            file_handle = open("{}/{}".format(folder_name, file_name), "w")
+            yaml.dump(data, file_handle)
+
+    def _check_output_folder(self):
+        """Check the output folder for permission and existence."""
+        outdir = self.config["output"]["folder"].get()
+        if outdir:
+            try:
+                with tempfile.TemporaryFile(dir=outdir):
+                    pass  # pragma: no cover
+            except FileNotFoundError as err:
+                err.filename = outdir
+                Logger.fubar(msg=str(err), exit_code=errno.ENOENT)
+            except PermissionError as err:
+                err.filename = outdir
+                Logger.fubar(msg=str(err), exit_code=errno.EACCES)
+            except Exception as err:
+                err.filename = outdir
+                Logger.fubar(msg=str(err))
 
 
 def main():
diff --git a/jujulint/config.py b/jujulint/config.py
index a4822ee..45c5618 100644
--- a/jujulint/config.py
+++ b/jujulint/config.py
@@ -22,6 +22,8 @@ from argparse import ArgumentParser
 
 from confuse import Configuration
 
+from jujulint.util import DeprecateAction
+
 
 class Config(Configuration):
     """Helper class for holding parsed config, extending confuse's BaseConfiguraion class."""
@@ -44,18 +46,22 @@ class Config(Configuration):
             "-d",
             "--output-dir",
             type=str,
-            default="output",
-            nargs="?",
-            help="The folder to use when saving gathered cloud data and lint reports.",
+            default="",
+            metavar="DIR",
+            help=(
+                "Dump gathered cloud state data into %(metavar)s. "
+                "Note that %(metavar)s must exist and be writable by the user. "
+                "Use with caution, as dumps will contain sensitve data. "
+                "This feature is disabled by default."
+            ),
             dest="output.folder",
         )
         self.parser.add_argument(
             "--dump-state",
             type=str,
-            help=(
-                "Optionally, dump cloud state as YAML into --output-dir."
-                "Use with caution, as dumps will contain sensitve data."
-            ),
+            nargs="*",
+            action=DeprecateAction,
+            help="DEPRECATED. See --output-dir for the current behavior",
             dest="output.dump",
         )
         self.parser.add_argument(
diff --git a/jujulint/util.py b/jujulint/util.py
index 283fa4b..3b24419 100644
--- a/jujulint/util.py
+++ b/jujulint/util.py
@@ -18,8 +18,11 @@
 # along with this program. If not, see <http://www.gnu.org/licenses/>.
 """Utility library for all helpful functions this project uses."""
 
+import argparse
 import re
 
+from jujulint.logging import Logger
+
 
 class InvalidCharmNameError(Exception):
     """Represents an invalid charm name being processed."""
@@ -82,3 +85,14 @@ def extract_charm_name(charm):
     if not match:
         raise InvalidCharmNameError("charm name '{}' is invalid".format(charm))
     return match.group(1)
+
+
+class DeprecateAction(argparse.Action):  # pragma: no cover
+    """Custom deprecation action to be used with ArgumentParser."""
+
+    def __call__(self, parser, namespace, values, option_string=None):
+        """Print a deprecation warning and remove the attribute from the namespace."""
+        Logger().warn(
+            "The argument {} is deprecated and will be ignored. ".format(option_string)
+        )
+        delattr(namespace, self.dest)
diff --git a/tests/functional/conftest.py b/tests/functional/conftest.py
index d26b318..7c215c1 100644
--- a/tests/functional/conftest.py
+++ b/tests/functional/conftest.py
@@ -2,7 +2,9 @@
 import logging
 import os
 import shutil
+from pathlib import Path
 from subprocess import check_call, check_output
+from tempfile import TemporaryDirectory
 from textwrap import dedent
 
 import pytest
@@ -127,3 +129,22 @@ def local_cloud():
     if backup:
         logging.info("Restoring backup")
         shutil.move(local_config_dir + ".bak", local_config_dir)
+
+
[e-mail]
+def non_existent_directory():
+    """Return a non-existent directory."""
+    dir = Path("/a/path/that/does/not/exist")
+    assert not dir.exists()
+    return dir
+
+
[e-mail]
+def non_writable_directory():
+    """Return a non-writable directory."""
+    dir = TemporaryDirectory()
+    os.chmod(dir.name, mode=0o555)
+
+    yield dir.name
+
+    dir.cleanup()
diff --git a/tests/functional/test_jujulint.py b/tests/functional/test_jujulint.py
index e15e9b6..9d908f1 100644
--- a/tests/functional/test_jujulint.py
+++ b/tests/functional/test_jujulint.py
@@ -47,3 +47,50 @@ async def test_audit_local_cloud(ops_test, local_cloud, rules_file):
         f"controller {ops_test.controller_name}, model {ops_test.model_name}" in stderr
     )
     assert returncode == 0
+
+
[e-mail]
+async def test_output_folder(ops_test, local_cloud, rules_file, tmp_path):
+    """Test juju-lint state output to folder."""
+    all_data_yaml = tmp_path / "all-data.yaml"
+    cloudstate_yaml = tmp_path / f"{local_cloud}-state.yaml"
+    assert not all_data_yaml.exists() and not cloudstate_yaml.exists()
+
+    await ops_test.model.wait_for_idle()
+    returncode, _, stderr = await ops_test.run(
+        *f"juju-lint -d {tmp_path} -c {rules_file}".split()
+    )
+
+    assert (
+        f"[{local_cloud}] Linting model information for {socket.getfqdn()}, "
+        f"controller {ops_test.controller_name}, model {ops_test.model_name}" in stderr
+    )
+    assert returncode == 0
+    assert all_data_yaml.exists() and cloudstate_yaml.exists()
+
+
[e-mail](
+    "bad_output_folder, expected_error",
+    [
+        ("non_existent_directory", "No such file or directory"),
+        ("non_writable_directory", "Permission denied"),
+    ],
+)
[e-mail]
+async def test_bad_output_folder_error(
+    ops_test, local_cloud, rules_file, bad_output_folder, expected_error, request
+):
+    """Test juju-lint fails gracefully for bad output folder values."""
+    output_folder = request.getfixturevalue(bad_output_folder)
+
+    await ops_test.model.wait_for_idle()
+    returncode, _, stderr = await ops_test.run(
+        *f"juju-lint -d {output_folder} -c {rules_file}".split()
+    )
+    assert returncode != 0
+    assert (
+        f"[{local_cloud}] Linting model information for {socket.getfqdn()}, "
+        f"controller {ops_test.controller_name}, model {ops_test.model_name}"
+        not in stderr
+    )
+    assert expected_error in stderr
diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py
index 4d3d3ee..00500ea 100644
--- a/tests/unit/conftest.py
+++ b/tests/unit/conftest.py
@@ -239,6 +239,7 @@ def rules_files():
     ]
 
 
[e-mail]
 def juju_status_relation():
     """Representation of juju status input to test relations checks."""
     return {
diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py
index ed63f67..81d8934 100644
--- a/tests/unit/test_cli.py
+++ b/tests/unit/test_cli.py
@@ -1,5 +1,6 @@
 #!/usr/bin/python3
 """Test the CLI."""
+import errno
 from logging import WARN
 from unittest.mock import MagicMock, call
 
@@ -254,7 +255,11 @@ def test_cli_audit_all(cli_instance, mocker):
     clouds = MagicMock()
     clouds.get.return_value = clouds_value
 
-    config_data = {"clouds": clouds}
+    folder_value = ""
+    folder = MagicMock()
+    folder.get.return_value = folder_value
+
+    config_data = {"clouds": clouds, "output": {"folder": folder}}
     config = MagicMock()
     config.__getitem__.side_effect = config_data.__getitem__
 
@@ -336,7 +341,7 @@ def test_cli_write_yaml(cli_instance, mocker):
     opened_file = MagicMock()
     mock_open = mocker.patch("builtins.open", return_value=opened_file)
 
-    config = {"output": {"dump": True, "folder": output_folder}}
+    config = {"output": {"folder": output_folder}}
 
     cli_instance.config = config
     cli_instance.write_yaml(data, file_name)
@@ -347,6 +352,37 @@ def test_cli_write_yaml(cli_instance, mocker):
     yaml_mock.dump.assert_called_once_with(data, opened_file)
 
 
+def test_check_output_folder(cli_instance, mocker):
+    """Test _check_output_folder() method from Cli class."""
+    folder_value = "/a/non/empty/path/string"
+    folder = MagicMock()
+    folder.get.return_value = folder_value
+
+    config = {"output": {"folder": folder}}
+    cli_instance.config = config
+
+    mock_temporary_file = mocker.patch("tempfile.TemporaryFile")
+    mock_sys_exit = mocker.patch("sys.exit")
+
+    mock_temporary_file.return_value.__enter__.side_effect = FileNotFoundError()
+    cli_instance._check_output_folder()
+    mock_sys_exit.assert_called_once_with(errno.ENOENT)
+
+    mock_temporary_file.reset_mock(return_value=True)
+    mock_sys_exit.reset_mock()
+
+    mock_temporary_file.return_value.__enter__.side_effect = PermissionError()
+    cli_instance._check_output_folder()
+    mock_sys_exit.assert_called_once_with(errno.EACCES)
+
+    mock_temporary_file.reset_mock(return_value=True)
+    mock_sys_exit.reset_mock()
+
+    mock_temporary_file.return_value.__enter__.side_effect = Exception()
+    cli_instance._check_output_folder()
+    mock_sys_exit.assert_called_once_with(1)
+
+
 @pytest.mark.parametrize("audit_type", ["file", "all", None])
 def test_main(cli_instance, audit_type, mocker):
     """Test main entrypoint of jujulint."""