Merge ~ines-almeida/launchpad:fetch-service-option-model-update into launchpad:master

[Ines Almeida]

Add Snap.use_fetch_service field to model and API

The field will only be updatable by admins. Although we can't hide the API endpoint itself, we are hidding the endpoint setting and getting behind a new feature flag "snap.fetch_service.enable"

UI changes in another MP: https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/461649

[Simone Pelosi (pelpsi)]

LGTM!

review: Approve

[Simone Pelosi (pelpsi)]

Could you also add the parameter to the `makeSnap` function in `lp\testing\factory.py` ?

[Ines Almeida (ines-almeida)]

Sure, done

[Jürgen Gmach (jugmac00)]

Thank you! I left a couple of comments. I think the missing assert statements need to be handled, the other things are optional or questions.

review: Needs Fixing

[Ines Almeida (ines-almeida)]

Comments addressed, thank you for the review!

[Jürgen Gmach (jugmac00)]

review: Approve

[Ines Almeida (ines-almeida)]

Waiting for DB change to be ready for deployment before merging this change.

If this goes in before the DB change, any other deployments will be blocked while the DB change isn't ready to be deployed.

[Ines Almeida (ines-almeida)]

Opened a new MP to address the post-merge comments from this MP: https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/462338

diff --git a/lib/lp/services/features/flags.py b/lib/lp/services/features/flags.py
index f15a6df..ae0bf52 100644
--- a/lib/lp/services/features/flags.py
+++ b/lib/lp/services/features/flags.py
@@ -304,6 +304,16 @@ flag_info = sorted(
             "",
             "",
         ),
+        (
+            "snap.fetch_service.enable",
+            "boolean",
+            "If set, allow admins to set snap.use_fetch_service field, which "
+            "sets a snap build to use the fetch-service instead of the "
+            "builder-proxy",
+            "",
+            "",
+            "",
+        ),
     ]
 )
 
diff --git a/lib/lp/snappy/interfaces/snap.py b/lib/lp/snappy/interfaces/snap.py
index ae2e3df..8364c58 100644
--- a/lib/lp/snappy/interfaces/snap.py
+++ b/lib/lp/snappy/interfaces/snap.py
@@ -23,6 +23,7 @@ __all__ = [
     "NoSourceForSnap",
     "NoSuchSnap",
     "SNAP_SNAPCRAFT_CHANNEL_FEATURE_FLAG",
+    "SNAP_USE_FETCH_SERVICE_FEATURE_FLAG",
     "SnapAuthorizationBadGeneratedMacaroon",
     "SnapBuildAlreadyPending",
     "SnapBuildArchiveOwnerMismatch",
@@ -101,6 +102,7 @@ from lp.soyuz.interfaces.archive import IArchive
 from lp.soyuz.interfaces.distroarchseries import IDistroArchSeries
 
 SNAP_SNAPCRAFT_CHANNEL_FEATURE_FLAG = "snap.channels.snapcraft"
+SNAP_USE_FETCH_SERVICE_FEATURE_FLAG = "snap.fetch_service.enable"
 
 
 @error_status(http.client.BAD_REQUEST)
@@ -1189,6 +1191,18 @@ class ISnapAdminAttributes(Interface):
         )
     )
 
+    use_fetch_service = exported(
+        Bool(
+            title=_("Use fetch service"),
+            required=True,
+            readonly=False,
+            description=_(
+                "If set, Snap builds will use the fetch-service instead "
+                "of the builder-proxy to access external resources."
+            ),
+        )
+    )
+
     def subscribe(person, subscribed_by):
         """Subscribe a person to this snap recipe."""
 
@@ -1267,6 +1281,7 @@ class ISnapSet(Interface):
         store_channels=None,
         project=None,
         pro_enable=None,
+        use_fetch_service=False,
     ):
         """Create an `ISnap`."""
 
diff --git a/lib/lp/snappy/model/snap.py b/lib/lp/snappy/model/snap.py
index eeeb056..cf18262 100644
--- a/lib/lp/snappy/model/snap.py
+++ b/lib/lp/snappy/model/snap.py
@@ -124,6 +124,7 @@ from lp.services.database.stormexpr import (
     IsDistinctFrom,
     NullsLast,
 )
+from lp.services.features import getFeatureFlag
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
 from lp.services.librarian.model import LibraryFileAlias, LibraryFileContent
@@ -137,6 +138,7 @@ from lp.services.webhooks.interfaces import IWebhookSet
 from lp.services.webhooks.model import WebhookTargetMixin
 from lp.snappy.adapters.buildarch import determine_architectures_to_build
 from lp.snappy.interfaces.snap import (
+    SNAP_USE_FETCH_SERVICE_FEATURE_FLAG,
     BadMacaroon,
     BadSnapSearchContext,
     BadSnapSource,
@@ -394,6 +396,8 @@ class Snap(StormBase, WebhookTargetMixin):
 
     _pro_enable = Bool(name="pro_enable", allow_none=True)
 
+    _use_fetch_service = Bool(name="use_fetch_service", allow_none=False)
+
     def __init__(
         self,
         registrant,
@@ -419,6 +423,7 @@ class Snap(StormBase, WebhookTargetMixin):
         store_channels=None,
         project=None,
         pro_enable=False,
+        use_fetch_service=False,
     ):
         """Construct a `Snap`."""
         super().__init__()
@@ -454,6 +459,7 @@ class Snap(StormBase, WebhookTargetMixin):
         self.store_secrets = store_secrets
         self.store_channels = store_channels
         self._pro_enable = pro_enable
+        self.use_fetch_service = use_fetch_service
 
     def __repr__(self):
         return "<Snap ~%s/+snap/%s>" % (self.owner.name, self.name)
@@ -693,6 +699,17 @@ class Snap(StormBase, WebhookTargetMixin):
     def pro_enable(self, value):
         self._pro_enable = value
 
+    @property
+    def use_fetch_service(self):
+        if getFeatureFlag(SNAP_USE_FETCH_SERVICE_FEATURE_FLAG):
+            return self._use_fetch_service
+        return None
+
+    @use_fetch_service.setter
+    def use_fetch_service(self, value):
+        if getFeatureFlag(SNAP_USE_FETCH_SERVICE_FEATURE_FLAG):
+            self._use_fetch_service = value
+
     def getAllowedInformationTypes(self, user):
         """See `ISnap`."""
         if user_has_special_snap_access(user):
@@ -1534,6 +1551,7 @@ class SnapSet:
         store_channels=None,
         project=None,
         pro_enable=None,
+        use_fetch_service=False,
     ):
         """See `ISnapSet`."""
         if not registrant.inTeam(owner):
@@ -1618,6 +1636,7 @@ class SnapSet:
             store_channels=store_channels,
             project=project,
             pro_enable=pro_enable,
+            use_fetch_service=use_fetch_service,
         )
         store.add(snap)
         snap._reconcileAccess()
diff --git a/lib/lp/snappy/tests/test_snap.py b/lib/lp/snappy/tests/test_snap.py
index 8f35aa1..07fc8d2 100644
--- a/lib/lp/snappy/tests/test_snap.py
+++ b/lib/lp/snappy/tests/test_snap.py
@@ -69,7 +69,7 @@ from lp.services.database.sqlbase import (
     flush_database_caches,
     get_transaction_timestamp,
 )
-from lp.services.features.testing import MemoryFeatureFixture
+from lp.services.features.testing import FeatureFixture, MemoryFeatureFixture
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.runner import JobRunner
 from lp.services.log.logger import BufferLogger
@@ -80,6 +80,7 @@ from lp.services.webapp.publisher import canonical_url
 from lp.services.webapp.snapshot import notify_modified
 from lp.services.webhooks.testing import LogsScheduledWebhooks
 from lp.snappy.interfaces.snap import (
+    SNAP_USE_FETCH_SERVICE_FEATURE_FLAG,
     BadSnapSearchContext,
     CannotFetchSnapcraftYaml,
     CannotModifySnapProcessor,
@@ -2318,6 +2319,7 @@ class TestSnapSet(TestCaseWithFactory):
         self.assertTrue(snap.allow_internet)
         self.assertFalse(snap.build_source_tarball)
         self.assertFalse(snap.pro_enable)
+        self.assertFalse(snap.use_fetch_service)
 
     def test_creation_git(self):
         # The metadata entries supplied when a Snap is created for a Git
@@ -2342,6 +2344,7 @@ class TestSnapSet(TestCaseWithFactory):
         self.assertTrue(snap.allow_internet)
         self.assertFalse(snap.build_source_tarball)
         self.assertFalse(snap.pro_enable)
+        self.assertFalse(snap.use_fetch_service)
 
     def test_creation_git_url(self):
         # A Snap can be backed directly by a URL for an external Git
@@ -2355,6 +2358,55 @@ class TestSnapSet(TestCaseWithFactory):
         self.assertEqual(ref.path, snap.git_path)
         self.assertEqual(ref, snap.git_ref)
 
+    def test_auth_to_edit_admin_only_fields(self):
+        # The admin fields can only be updated by an admin
+        self.useFixture(
+            FeatureFixture({SNAP_USE_FETCH_SERVICE_FEATURE_FLAG: "on"})
+        )
+
+        non_admin = self.factory.makePerson()
+        [ref] = self.factory.makeGitRefs(owner=non_admin)
+        components = self.makeSnapComponents(git_ref=ref)
+        snap = getUtility(ISnapSet).new(**components)
+
+        admin_fields = [
+            "allow_internet",
+            "pro_enable",
+            "require_virtualized",
+            "use_fetch_service",
+        ]
+
+        for field_name in admin_fields:
+            # exception is raised when user tries updating admin-only fields
+            with person_logged_in(non_admin):
+                self.assertRaises(
+                    Unauthorized, setattr, snap, field_name, True
+                )
+            # exception isn't raised when an admin does the same
+            with admin_logged_in():
+                setattr(snap, field_name, True)
+                self.assertTrue(getattr(snap, field_name))
+
+    def test_snap_use_fetch_service_feature_flag(self):
+        # The snap.use_fetch_service API only works when feature flag is set
+        [ref] = self.factory.makeGitRefs()
+        components = self.makeSnapComponents(git_ref=ref)
+        snap = getUtility(ISnapSet).new(**components)
+
+        # admin cannot get the actual value or set a new value to the
+        # use_fetch_service field when the feature flag is OFF
+        login_admin()
+        self.assertEqual(None, snap.use_fetch_service)
+        snap.use_fetch_service = True
+        self.assertEqual(None, snap.use_fetch_service)
+
+        # when feature flag is ON, admin can see the real value of
+        # `use_fetch_service` and opt in and out of it
+        with MemoryFeatureFixture({SNAP_USE_FETCH_SERVICE_FEATURE_FLAG: "on"}):
+            self.assertFalse(snap.use_fetch_service)
+            snap.use_fetch_service = True
+            self.assertTrue(snap.use_fetch_service)
+
     def test_create_private_snap_with_open_team_as_owner_fails(self):
         components = self.makeSnapComponents()
         with admin_logged_in():
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index 61e4f6e..57d1d45 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -6209,6 +6209,7 @@ class LaunchpadObjectFactory(ObjectFactory):
         store_channels=None,
         project=_DEFAULT,
         pro_enable=False,
+        use_fetch_service=False,
     ):
         """Make a new Snap."""
         assert information_type is None or private is None
@@ -6290,6 +6291,7 @@ class LaunchpadObjectFactory(ObjectFactory):
             store_channels=store_channels,
             project=project,
             pro_enable=pro_enable,
+            use_fetch_service=use_fetch_service,
         )
         if is_stale is not None:
             removeSecurityProxy(snap).is_stale = is_stale