launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Thiago F. Pappacena has proposed merging ~pappacena/launchpad:upgrade-boto into launchpad:master.

Commit message:
Upgrading boto lib and removing workarounds for public ECR image uploads

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/400419

Dependencies MP: https://code.launchpad.net/~pappacena/lp-source-dependencies/+git/lp-source-dependencies/+merge/400418
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:upgrade-boto into launchpad:master.

diff --git a/lib/lp/oci/model/ociregistryclient.py b/lib/lp/oci/model/ociregistryclient.py
index 4d51843..923f670 100644
--- a/lib/lp/oci/model/ociregistryclient.py
+++ b/lib/lp/oci/model/ociregistryclient.py
@@ -1,4 +1,4 @@
-# Copyright 2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2020-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Client for talking to an OCI registry."""
@@ -59,8 +59,6 @@ proxy_urlfetch = partial(urlfetch, use_proxy=True)
 
 
 OCI_AWS_BEARER_TOKEN_DOMAINS_FLAG = 'oci.push.aws.bearer_token_domains'
-OCI_AWS_BOT_EXTRA_MODEL_PATH = 'oci.push.aws.boto.extra_paths'
-OCI_AWS_BOT_EXTRA_MODEL_NAME = 'oci.push.aws.boto.extra_model_name'
 
 
 def is_aws_bearer_token_domain(domain):
@@ -68,7 +66,9 @@ def is_aws_bearer_token_domain(domain):
     instead of basic auth."""
     domains = getFeatureFlag(OCI_AWS_BEARER_TOKEN_DOMAINS_FLAG)
     if not domains:
-        return False
+        # We know that public ECR default domain is bearer token. If the
+        # flag is not set, force it.
+        domains = 'public.ecr.aws'
     return any(domain.endswith(i) for i in domains.split())
 
 
@@ -680,23 +680,13 @@ class AWSAuthenticatorMixin:
 
     def _getBotoClient(self):
         params = self._getClientParameters()
-        if not self.should_use_aws_extra_model:
-            return boto3.client('ecr', **params)
-        model_path = getFeatureFlag(OCI_AWS_BOT_EXTRA_MODEL_PATH)
-        model_name = getFeatureFlag(OCI_AWS_BOT_EXTRA_MODEL_NAME)
-        if not model_path or not model_name:
-            log.warning(
-                "%s or %s feature rules are not set. Using default model." %
-                (OCI_AWS_BOT_EXTRA_MODEL_PATH, OCI_AWS_BOT_EXTRA_MODEL_NAME))
-            return boto3.client('ecr', **params)
-        session = boto3.Session()
-        session._loader.search_paths.extend([model_path])
-        return session.client(model_name, **params)
+        client_type = 'ecr-public' if self.is_public_ecr else 'ecr'
+        return boto3.client(client_type, **params)
 
     @property
-    def should_use_aws_extra_model(self):
-        """Returns True if the given registry domain requires extra boto API
-        model.
+    def is_public_ecr(self):
+        """Returns True if the given registry domain is a public ECR. False
+        otherwhise.
         """
         domain = urlparse(self.push_rule.registry_url).netloc
         return is_aws_bearer_token_domain(domain)
diff --git a/lib/lp/oci/tests/test_ociregistryclient.py b/lib/lp/oci/tests/test_ociregistryclient.py
index e30a470..5fe9f2f 100644
--- a/lib/lp/oci/tests/test_ociregistryclient.py
+++ b/lib/lp/oci/tests/test_ociregistryclient.py
@@ -1,4 +1,4 @@
-# Copyright 2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2020-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the OCI Registry client."""
@@ -1045,7 +1045,7 @@ class TestRegistryHTTPClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
 
         instance = RegistryHTTPClient.getInstance(push_rule)
         self.assertEqual(AWSRegistryHTTPClient, type(instance))
-        self.assertFalse(instance.should_use_aws_extra_model)
+        self.assertFalse(instance.is_public_ecr)
         self.assertIsInstance(instance, RegistryHTTPClient)
 
     @responses.activate
@@ -1066,7 +1066,7 @@ class TestRegistryHTTPClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
 
         instance = RegistryHTTPClient.getInstance(push_rule)
         self.assertEqual(AWSRegistryBearerTokenClient, type(instance))
-        self.assertTrue(instance.should_use_aws_extra_model)
+        self.assertTrue(instance.is_public_ecr)
         self.assertIsInstance(instance, RegistryHTTPClient)
 
     @responses.activate
@@ -1336,26 +1336,58 @@ class TestAWSAuthenticator(OCIConfigHelperMixin, TestCaseWithFactory):
             auth.push_rule = push_rule
             self.assertRaises(OCIRegistryAuthenticationError, auth._getRegion)
 
-    def test_should_use_extra_model(self):
+    def test_should_use_public_ecr_boto_model(self):
         self.setConfig({
             OCI_AWS_BEARER_TOKEN_DOMAINS_FLAG: 'bearertoken.example.com'})
+        boto_client_mock = self.useFixture(
+            MockPatch('lp.oci.model.ociregistryclient.boto3.client')).mock
         cred = self.factory.makeOCIRegistryCredentials(
-            url="https://myregistry.bearertoken.example.com";)
+            url="https://myregistry.bearertoken.example.com";,
+            credentials=dict(
+                region='us-east-1', username='user1', password='passwd1'))
         push_rule = self.factory.makeOCIPushRule(registry_credentials=cred)
 
         with admin_logged_in():
             auth = AWSAuthenticatorMixin()
             auth.push_rule = push_rule
-            self.assertTrue(auth.should_use_aws_extra_model)
+            self.assertTrue(auth.is_public_ecr)
 
-    def test_should_not_use_extra_model(self):
+            client = auth._getBotoClient()
+            self.assertEqual(boto_client_mock.return_value, client)
+            self.assertEqual(1, boto_client_mock.call_count)
+            call = boto_client_mock.call_args
+            self.assertEqual(
+                mock.call(
+                    'ecr-public', config=mock.ANY,
+                    region_name='us-east-1', aws_secret_access_key='passwd1',
+                    aws_access_key_id='user1'), call)
+            self.assertThat(
+                call[1]['config'], MatchesStructure.byEquality(proxies=None))
+
+    def test_should_not_use_public_ecr_boto_model(self):
         self.setConfig({
             OCI_AWS_BEARER_TOKEN_DOMAINS_FLAG: 'bearertoken.example.com'})
+        boto_client_mock = self.useFixture(
+            MockPatch('lp.oci.model.ociregistryclient.boto3.client')).mock
         cred = self.factory.makeOCIRegistryCredentials(
-            url="https://123456789.dkr.ecr.sa-west-1.amazonaws.com";)
+            url="https://123456789.dkr.ecr.sa-west-1.amazonaws.com";,
+            credentials=dict(
+                region='us-east-1', username='user1', password='passwd1'))
         push_rule = self.factory.makeOCIPushRule(registry_credentials=cred)
 
         with admin_logged_in():
             auth = AWSAuthenticatorMixin()
             auth.push_rule = push_rule
-            self.assertFalse(auth.should_use_aws_extra_model)
+            self.assertFalse(auth.is_public_ecr)
+
+            client = auth._getBotoClient()
+            self.assertEqual(boto_client_mock.return_value, client)
+            self.assertEqual(1, boto_client_mock.call_count)
+            call = boto_client_mock.call_args
+            self.assertEqual(
+                mock.call(
+                    'ecr', config=mock.ANY, region_name='us-east-1',
+                    aws_secret_access_key='passwd1',
+                    aws_access_key_id='user1'), call)
+            self.assertThat(
+                call[1]['config'], MatchesStructure.byEquality(proxies=None))
diff --git a/requirements/launchpad.txt b/requirements/launchpad.txt
index 581b579..f1bafe7 100644
--- a/requirements/launchpad.txt
+++ b/requirements/launchpad.txt
@@ -22,8 +22,8 @@ billiard==3.5.0.5
 bleach==3.1.0
 breezy==3.0.1
 bson==0.5.9
-boto3==1.16.2
-botocore==1.19.2
+boto3==1.17.40
+botocore==1.20.40
 celery==4.1.1
 Chameleon==3.6.2
 configobj==5.0.6
@@ -134,7 +134,7 @@ rabbitfixture==0.5.0
 requests-file==1.4.3
 requests-toolbelt==0.9.1
 responses==0.9.0
-s3transfer==0.3.3
+s3transfer==0.3.6
 scandir==1.7
 secure-cookie==0.1.0
 service-identity==18.1.0