Merge ~chad.smith/ubuntu/+source/ubuntu-advantage-tools:pkg-upload-24.4 into ubuntu/+source/ubuntu-advantage-tools:ubuntu/devel

[Chad Smith]

bug-fix-only release 24.4 for upload into groovy

cherry-pick/backport one upstream commit cfde367[1] to fix one bug on Ubuntu PRO FIPS images[2]

 - Honor additionalPackages directives as provided by the contract server machine-token

Known issues still present:
tox -p auto will still exhibit a known upstream flake issue[3]

references:

[1] https://github.com/canonical/ubuntu-advantage-client/commit/cfde367
  <- upstream commit which was backported, some part was inapplicable to release-24 branch
[2] https://github.com/canonical/ubuntu-advantage-client/issues/1173
[3] known upstream flake issue with tox https://github.com/canonical/ubuntu-advantage-client/issues/1062

Groovy package from this branch is available adding this PPA:
sudo add-apt-repository ppa:chad.smith/ua-client-uploads

[Bryce Harrington (bryce)]

I've had a chance to study the code and think I understand it. I have a couple questions to clarify before approving. I also have several code suggestions but these are just code style so not required or expected for this upload, and provided just for future refactoring ideas if you wish.

### Questions ###

In cc.py and cis.py the packages data member is dropped, allowing these classes to rely on the base class' packages() property, which provides a list of packages from the configuration. fips.py does something similar but overrides the base packages() prop with its own, dropping the fips_packages dict. Two questions:

  a. The old code specified two packages "ubuntu-commoncriteria" and "ubuntu-cisbenchmark-16.04". My assumption is that these are going to be loaded out of a config file going forward? Guessing this is the "contract server" mentioned in bug 1173?

  b. Is there a risk if a user already had a config file that did not have these values, of things breaking? Or do we 100% control the contract server and already have the required data in place?

### Code Suggestions (Optional) ###

1. I notice in the test cases, test_cc.py uses "ubuntu-commoncriteria" as its sample data, but test_cis.py uses "pkg1". Would it be more convenient to use "pkg1" in both cases, or alternatively use "ubuntu-cisbenchmark-16.04" in test_cis.py

2. repo.py's packages() prop adds logic to lookup "entitlement"->"directives"->"additionalPackages" from the configuration and then return it, or an empty list if not found. It strikes me this might be a situation where the code would be clearer to just wrap in a try block:

    try:
        return self.cfg.entitlements[self.name]['entitlement']['directives']['additionalPackages']
    except:
        return []

3. The packages() prop in repo.py could use a bit more explanation in its code doc. E.g.:

        def packages(self) -> "List[str]":
           """
           Packages to install on enablement.

           Returns the list of packages specified in the additionalPackages
           directive, or an empty list if none were configured.
           """

4. fips.py's packages() prop has an interesting use of groupby (I had to look that up, wasn't familiar with it - nice). But as the logic here is a bit more sophisticated it would be worthwhile to give this packages() property its own code docs. For example, maybe something like:

     @property
     def packages(self) -> "List[str]":
         """
         Packages to install on enablement.

         Returns only packages from additionalPackages that are
         either already installed or required for fips.
         """

5. Also in fips.py's packages() prop, the list assembler at the end could be shortened by combining the if statements since they have the same body:

    for pkg_name, pkg_list in pkg_groups:
        if pkg_name in installed_packages + self.fips_required_packages:
            packages += pkg_list

review: Needs Information

[Bryce Harrington (bryce)]

tox passes for me, modulo the known-issue (#1062) which showed for xenial and bionic:

  py3: commands succeeded
  flake8: commands succeeded
  py3-xenial: commands succeeded
  py3-bionic: commands succeeded
  py3-eoan: commands succeeded
  flake8-trusty: commands succeeded
ERROR: flake8-xenial: commands failed
ERROR: flake8-bionic: commands failed
  flake8-eoan: commands succeeded
  mypy: commands succeeded
  black: commands succeeded

[Chad Smith (chad.smith)]

> I've had a chance to study the code and think I understand it. I have a
> couple questions to clarify before approving. I also have several code
> suggestions but these are just code style so not required or expected for this
> upload, and provided just for future refactoring ideas if you wish.

>
> ### Questions ###
>
> In cc.py and cis.py the packages data member is dropped, allowing these
> classes to rely on the base class' packages() property, which provides a list
> of packages from the configuration.

Correct base class implementation should work for most repo-based subclasses. FIPS* have an unique behavior in that -hmac packages need to additionally be installed only if the corresponding base package is already installed on the system which is why there is a fips-override of that behavior (so it doesn't automatically install all packages listed in additionalPackages directive).

> fips.py does something similar but
> overrides the base packages() prop with its own, dropping the fips_packages
> dict. Two questions:
>
> a. The old code specified two packages "ubuntu-commoncriteria" and "ubuntu-
> cisbenchmark-16.04". My assumption is that these are going to be loaded out
> of a config file going forward? Guessing this is the "contract server"
> mentioned in bug 1173?

Correct, here is an excerpt of that "contract" and cis service itself is a beta service that isn't even supported/installable at the moment, so no risk there at the moment.

               "type" : "cc-eal",
               "directives" : {
                  "additionalPackages" : [
                     "ubuntu-commoncriteria"
                  ],
                  "aptURL" : "https://esm.ubuntu.com/cc",
                  "aptKey" : "9F912DADD99EE1CC6BFFFF243A186E733F491C46",
                  "suites" : [
                     "xenial"
                  ]
               },
...

>
> b. Is there a risk if a user already had a config file that did not have
> these values, of things breaking? Or do we 100% control the contract server
> and already have the required data in place?
>

Any connected machine will already have the /var/lib/ubuntu-advantage/private/machine-token.json present with these additionalPackages directives already present. This has directive been in place for 1.5 years I believe. That said, Groovy doesn't offer/install said services, so this is really a noop for groovy. And yes we (online services team) 100% control the contract server details and have had this in place for a long time. ua-tools pkg just didn't use it in release-24 branch yet.

>
>
> ### Code Suggestions (Optional) ###
>
> 1. I notice in the test cases, test_cc.py uses "ubuntu-commoncriteria" as its
> sample data, but test_cis.py uses "pkg1". Would it be more convenient to use
> "pkg1" in both cases, or alternatively use "ubuntu-cisbenchmark-16.04" in
> test_cis.py

+1 we can take this to master branch.

>
> 2. repo.py's packages() prop adds logic to lookup
> "entitlement"->"directives"->"additionalPackages" from the configuration and
> then return it, or an empty list if not found. It strikes me this might be a
> situation where the code would be clearer ...

[Bryce Harrington (bryce)]

I saw `tox -e behave` listed in README.md as an integration test, so tried running that too. I don't know if that is an applicable testsuite for acceptance testing, nor if it is currently expected to pass or not, but it failed for me:

~/pkg/UbuntuAdvantageTools/review-mp390109/ubuntu-advantage-tools$ tox -e behave
GLOB sdist-make: /home/bryce/pkg/UbuntuAdvantageTools/review-mp390109/ubuntu-advantage-tools/setup.py
behave create: /home/bryce/pkg/UbuntuAdvantageTools/review-mp390109/ubuntu-advantage-tools/.tox/behave
behave installdeps: -rrequirements.txt, -rtest-requirements.txt
behave inst: /home/bryce/pkg/UbuntuAdvantageTools/review-mp390109/ubuntu-advantage-tools/.tox/.tmp/package/1/ubuntu-advantage-tools-24.4.zip
behave installed: appdirs==1.4.3,attrs==20.1.0,behave==1.2.6,CacheControl==0.12.6,certifi==2019.11.28,chardet==3.0.4,colorama==0.4.3,contextlib2==0.6.0,coverage==5.2.1,distlib==0.3.0,distro==1.4.0,flake8==3.8.3,html5lib==1.0.1,idna==2.8,iniconfig==1.0.1,ipaddr==2.2.0,lockfile==0.12.2,mccabe==0.6.1,mock==4.0.2,more-itertools==8.5.0,msgpack==0.6.2,packaging==20.3,parse==1.17.0,parse-type==0.5.2,pep517==0.8.2,pluggy==0.13.1,progress==1.5,py==1.9.0,pycodestyle==2.6.0,pyflakes==2.2.0,PyHamcrest==2.0.2,pyparsing==2.4.6,pytest==6.0.1,pytest-cov==2.10.1,pytoml==0.1.21,PyYAML==5.3.1,requests==2.22.0,retrying==1.3.3,six==1.14.0,toml==0.10.1,ubuntu-advantage-tools==24.4,urllib3==1.25.8,webencodings==0.5.1
behave run-test-pre: PYTHONHASHSEED='3371048301'
behave run-test: commands[0] | behave --verbose
Loading config defaults from "./tox.ini"
Using defaults:
          color True
  show_snippets True
   show_skipped True
        dry_run False
    show_source True
   show_timings True
 stdout_capture True
 stderr_capture True
    log_capture True
 logging_format %(levelname)s:%(name)s:%(message)s
  logging_level 20
  steps_catalog False
        summary True
          junit False
          stage None
       userdata {}
 default_format pretty
   default_tags
scenario_outline_annotation_schema {name} -- @{row.id} {examples.name}
more_formatters {}
Using default path "./features"
Trying base directory: /home/bryce/pkg/UbuntuAdvantageTools/review-mp390109/ubuntu-advantage-tools/features
Config options:
  image_clean = True
  destroy_instances = True
  contract_token = None
  reuse_image = None
Creating behave-image-build-1598996336365313
Starting behave-image-build-1598996336365313
Unexpected runlevel output:
Unexpected runlevel output:
Unexpected runlevel output:
Unexpected runlevel output:
Unexpected runlevel output:
HOOK-ERROR in before_all: Exception: System did not boot in 10s
  File "/home/bryce/pkg/UbuntuAdvantageTools/review-mp390109/ubuntu-advantage-tools/.tox/behave/lib/python3.8/site-packages/behave/runner.py", line 545, in run_hook
    self.hooks[name](context, *args)
  File "features/environment.py", line 111, in before_all
    create_trusty_uat_lxd_image(context)
  File "features/environment.py", line 167, in create_trusty_uat_lxd_image
    launch_lxd_container(context, "ubuntu:trusty", build_container_name)
  File "/home/bryce/pkg/UbuntuAdvantageTools/review-mp390109/ubuntu-advantage-tools/features/util.py", line 33, in la...

[Chad Smith (chad.smith)]

As mentioned in chat. behave isn't expected to pass on this branch as
.travis.yml marks that job:

  allow_failures:
    - python: 3.7
      env: TOXENV=behave
      script:

[Chad Smith (chad.smith)]

That said, when we release version 25.0 we expect the behave targets to pass

[Bryce Harrington (bryce)]

One other minor niggle, the indentation on the changelog entry is one space too many:

   * New bug-fix-only release 24.4:
      - uaclient.version bump to 24.4
      - fips: honor additionalPackage directive from contract for bionic
        (GH #1173)

I dedented locally for the upload but you may want to update your git tree too.

Otherwise, thanks for confirming my assumptions on the config data, so everything looks good for upload.

review: Approve

[Bryce Harrington (bryce)]

$ dput ubuntu ubuntu-advantage-tools_24.4_source.changes
Checking signature on .changes
gpg: /home/bryce/pkg/UbuntuAdvantageTools/review-mp390109/ubuntu-advantage-tools_24.4_source.changes: Valid signature from E603B2578FB8F0FB
Checking signature on .dsc
gpg: /home/bryce/pkg/UbuntuAdvantageTools/review-mp390109/ubuntu-advantage-tools_24.4.dsc: Valid signature from E603B2578FB8F0FB
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading ubuntu-advantage-tools_24.4.dsc: done.
  Uploading ubuntu-advantage-tools_24.4.tar.xz: done.
  Uploading ubuntu-advantage-tools_24.4_source.buildinfo: done.
  Uploading ubuntu-advantage-tools_24.4_source.changes: done.
Successfully uploaded packages.

Unmerged commits

90f9fae... by Chad Smith on 2020-09-01

changelog: update 24.4 version for bug-fix-release

79e7bd0... by Chad Smith on 2020-09-01

version: update version 24.4

7e391ae... by Lucas Albuquerque Medeiros de Moura on 2020-06-01

Use additionalPackages from service payload (#1064)

Right now, ua-client is not using the additionalPackages field when enabling repo services, like cc-eal and fips. This PR adds the possibility for ua-client to start using these data when enabling repo services.

For fips, we extend packages() to understand which conditional extra packages should be installed when enabling the service.

diff --git a/debian/changelog b/debian/changelog
index 4be1a7a..08a2b57 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,12 @@
+ubuntu-advantage-tools (24.4) groovy; urgency=medium
+
+  * New bug-fix-only release 24.4:
+     - uaclient.version bump to 24.4
+     - fips: honor additionalPackage directive from contract for bionic
+       (GH #1173)
+
+ -- Chad Smith <[e-mail]>  Tue, 01 Sep 2020 11:14:39 -0600
+
 ubuntu-advantage-tools (24.3) groovy; urgency=medium
 
   * New bug-fix-only release 24.3:
diff --git a/uaclient/entitlements/cc.py b/uaclient/entitlements/cc.py
index 961f3a2..628121b 100644
--- a/uaclient/entitlements/cc.py
+++ b/uaclient/entitlements/cc.py
@@ -10,7 +10,6 @@ class CommonCriteriaEntitlement(repo.RepoEntitlement):
     title = "CC EAL2"
     description = "Common Criteria EAL2 Provisioning Packages"
     repo_key_file = "ubuntu-cc-keyring.gpg"
-    packages = ["ubuntu-commoncriteria"]
     messaging = {
         "pre_install": [
             "(This will download more than 500MB of packages, so may take some"
diff --git a/uaclient/entitlements/cis.py b/uaclient/entitlements/cis.py
index 8cddb0a..43330ce 100644
--- a/uaclient/entitlements/cis.py
+++ b/uaclient/entitlements/cis.py
@@ -8,4 +8,3 @@ class CISEntitlement(repo.RepoEntitlement):
     title = "CIS Audit"
     description = "Center for Internet Security Audit Tools"
     repo_key_file = "ubuntu-securitybenchmarks-keyring.gpg"
-    packages = ["ubuntu-cisbenchmark-16.04"]
diff --git a/uaclient/entitlements/fips.py b/uaclient/entitlements/fips.py
index f3ad25f..8503229 100644
--- a/uaclient/entitlements/fips.py
+++ b/uaclient/entitlements/fips.py
@@ -1,3 +1,5 @@
+from itertools import groupby
+
 from uaclient.entitlements import repo
 from uaclient import apt, status, util
 
@@ -12,23 +14,24 @@ class FIPSCommonEntitlement(repo.RepoEntitlement):
 
     repo_pin_priority = 1001
     fips_required_packages = frozenset({"fips-initramfs", "linux-fips"})
-    fips_packages = {
-        "libssl1.0.0": {"libssl1.0.0-hmac"},
-        "openssh-client": {"openssh-client-hmac"},
-        "openssh-server": {"openssh-server-hmac"},
-        "openssl": set(),
-        "strongswan": {"strongswan-hmac"},
-    }  # type: Dict[str, Set[str]]
     repo_key_file = "ubuntu-advantage-fips.gpg"  # Same for fips & fips-updates
 
     @property
     def packages(self) -> "List[str]":
-        packages = list(self.fips_required_packages)
+        packages = []  # type: List[str]
         installed_packages = apt.get_installed_packages()
-        for pkg_name, extra_pkgs in self.fips_packages.items():
+
+        pkg_groups = groupby(
+            super().packages,
+            key=lambda pkg_name: pkg_name.replace("-hmac", ""),
+        )
+
+        for pkg_name, pkg_list in pkg_groups:
             if pkg_name in installed_packages:
-                packages.append(pkg_name)
-                packages.extend(extra_pkgs)
+                packages += pkg_list
+            elif pkg_name in self.fips_required_packages:
+                packages += pkg_list
+
         return packages
 
     def application_status(self) -> "Tuple[status.ApplicationStatus, str]":
diff --git a/uaclient/entitlements/repo.py b/uaclient/entitlements/repo.py
index c288db6..b5dcc3c 100644
--- a/uaclient/entitlements/repo.py
+++ b/uaclient/entitlements/repo.py
@@ -45,7 +45,19 @@ class RepoEntitlement(base.UAEntitlement):
     @property
     def packages(self) -> "List[str]":
         """debs to install on enablement"""
-        return []
+        packages = []
+
+        entitlement = self.cfg.entitlements.get(self.name, {}).get(
+            "entitlement", {}
+        )
+
+        if entitlement:
+            directives = entitlement.get("directives", {})
+            additional_packages = directives.get("additionalPackages", [])
+
+            packages = additional_packages
+
+        return packages
 
     @property
     @abc.abstractmethod
diff --git a/uaclient/entitlements/tests/conftest.py b/uaclient/entitlements/tests/conftest.py
index 542b591..49dc37d 100644
--- a/uaclient/entitlements/tests/conftest.py
+++ b/uaclient/entitlements/tests/conftest.py
@@ -16,7 +16,8 @@ def machine_token(
     directives: "Dict[str, Any]" = None,
     entitled: bool = True,
     obligations: "Dict[str, Any]" = None,
-    suites: "List[str]" = None
+    suites: "List[str]" = None,
+    additional_packages: "List[str]" = None
 ) -> "Dict[str, Any]":
     return {
         "resourceTokens": [
@@ -36,6 +37,7 @@ def machine_token(
                         entitled=entitled,
                         obligations=obligations,
                         suites=suites,
+                        additional_packages=additional_packages,
                     )
                 ]
             }
@@ -50,7 +52,8 @@ def machine_access(
     directives: "Dict[str, Any]" = None,
     entitled: bool = True,
     obligations: "Dict[str, Any]" = None,
-    suites: "List[str]" = None
+    suites: "List[str]" = None,
+    additional_packages: "List[str]" = None
 ) -> "Dict[str, Any]":
     if affordances is None:
         affordances = {"series": []}  # Will match all series
@@ -64,6 +67,9 @@ def machine_access(
             "aptKey": "APTKEY",
             "suites": suites,
         }
+
+        if additional_packages:
+            directives["additionalPackages"] = additional_packages
     return {
         "obligations": obligations,
         "type": entitlement_type,
@@ -90,7 +96,8 @@ def entitlement_factory(tmpdir):
         affordances: "Dict[str, Any]" = None,
         directives: "Dict[str, Any]" = None,
         entitled: bool = True,
-        suites: "List[str]" = None
+        suites: "List[str]" = None,
+        additional_packages: "List[str]" = None
     ):
         cfg = config.UAConfig(cfg={"data_dir": tmpdir.strpath})
         cfg.write_cache(
@@ -101,6 +108,7 @@ def entitlement_factory(tmpdir):
                 directives=directives,
                 entitled=entitled,
                 suites=suites,
+                additional_packages=additional_packages,
             ),
         )
         return cls(cfg)
diff --git a/uaclient/entitlements/tests/test_cc.py b/uaclient/entitlements/tests/test_cc.py
index d1e3419..5115339 100644
--- a/uaclient/entitlements/tests/test_cc.py
+++ b/uaclient/entitlements/tests/test_cc.py
@@ -24,6 +24,7 @@ CC_MACHINE_TOKEN = machine_token(
         "aptURL": "http://CC",
         "aptKey": "APTKEY",
         "suites": ["xenial"],
+        "additionalPackages": ["ubuntu-commoncriteria"],
     },
     affordances={
         "architectures": ["x86_64", "ppc64le", "s390x"],
diff --git a/uaclient/entitlements/tests/test_cis.py b/uaclient/entitlements/tests/test_cis.py
index 821c75d..bb2f905 100644
--- a/uaclient/entitlements/tests/test_cis.py
+++ b/uaclient/entitlements/tests/test_cis.py
@@ -14,7 +14,7 @@ M_REPOPATH = "uaclient.entitlements.repo."
 
 @pytest.fixture
 def entitlement(entitlement_factory):
-    return entitlement_factory(CISEntitlement)
+    return entitlement_factory(CISEntitlement, additional_packages=["pkg1"])
 
 
 class TestCISEntitlementCanEnable:
diff --git a/uaclient/entitlements/tests/test_fips.py b/uaclient/entitlements/tests/test_fips.py
index 84a533a..a74a907 100644
--- a/uaclient/entitlements/tests/test_fips.py
+++ b/uaclient/entitlements/tests/test_fips.py
@@ -11,11 +11,7 @@ import pytest
 from uaclient import apt
 from uaclient import defaults
 from uaclient import status, util
-from uaclient.entitlements.fips import (
-    FIPSCommonEntitlement,
-    FIPSEntitlement,
-    FIPSUpdatesEntitlement,
-)
+from uaclient.entitlements.fips import FIPSEntitlement, FIPSUpdatesEntitlement
 from uaclient import exceptions
 
 
@@ -27,7 +23,25 @@ M_GETPLATFORM = M_REPOPATH + "util.get_platform_info"
 @pytest.fixture(params=[FIPSEntitlement, FIPSUpdatesEntitlement])
 def fips_entitlement_factory(request, entitlement_factory):
     """Parameterized fixture so we apply all tests to both FIPS and Updates"""
-    return partial(entitlement_factory, request.param)
+    additional_packages = [
+        "fips-initramfs",
+        "libssl1.0.0",
+        "libssl1.0.0-hmac",
+        "linux-fips",
+        "openssh-client",
+        "openssh-client-hmac",
+        "openssh-server",
+        "openssh-server-hmac",
+        "openssl",
+        "strongswan",
+        "strongswan-hmac",
+    ]
+
+    return partial(
+        entitlement_factory,
+        request.param,
+        additional_packages=additional_packages,
+    )
 
 
 @pytest.fixture
@@ -234,9 +248,17 @@ class TestFIPSEntitlementEnable:
 
 def _fips_pkg_combinations():
     """Construct all combinations of fips_packages and expected installs"""
+    fips_packages = {
+        "libssl1.0.0": {"libssl1.0.0-hmac"},
+        "openssh-client": {"openssh-client-hmac"},
+        "openssh-server": {"openssh-server-hmac"},
+        "openssl": set(),
+        "strongswan": {"strongswan-hmac"},
+    }
+
     items = [  # These are the items that we will combine together
         (pkg_name, [pkg_name] + list(extra_pkgs))
-        for pkg_name, extra_pkgs in FIPSCommonEntitlement.fips_packages.items()
+        for pkg_name, extra_pkgs in fips_packages.items()
     ]
     # This produces combinations in all possible combination lengths
     combinations = itertools.chain.from_iterable(
@@ -283,7 +305,7 @@ class TestFipsEntitlementPackages:
         full_expected_installs = (
             list(entitlement.fips_required_packages) + expected_installs
         )
-        assert full_expected_installs == entitlement.packages
+        assert sorted(full_expected_installs) == sorted(entitlement.packages)
 
     @mock.patch(M_PATH + "apt.get_installed_packages")
     def test_multiple_packages_calls_dont_mutate_state(
@@ -292,17 +314,11 @@ class TestFipsEntitlementPackages:
         # Make it appear like all packages are installed
         m_get_installed_packages.return_value.__contains__.return_value = True
 
-        before = (
-            copy.deepcopy(entitlement.fips_required_packages),
-            copy.deepcopy(entitlement.fips_packages),
-        )
+        before = copy.deepcopy(entitlement.fips_required_packages)
 
         assert entitlement.packages
 
-        after = (
-            copy.deepcopy(entitlement.fips_required_packages),
-            copy.deepcopy(entitlement.fips_packages),
-        )
+        after = copy.deepcopy(entitlement.fips_required_packages)
 
         assert before == after
 
diff --git a/uaclient/version.py b/uaclient/version.py
index e64f3ae..4e9b7a7 100644
--- a/uaclient/version.py
+++ b/uaclient/version.py
@@ -8,7 +8,7 @@ import os.path
 from uaclient import util
 
 
-__VERSION__ = "24.3"
+__VERSION__ = "24.4"
 PACKAGED_VERSION = "@@PACKAGED_VERSION@@"