launchpad-reviewers team mailing list archive

[Ines Almeida <mp+461552@xxxxxxxxxxxxxxxxxx>]

Ines Almeida has proposed merging ~ines-almeida/launchpad:fetch-service-option-model-update into launchpad:master.

Commit message:
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"

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/461552
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:fetch-service-option-model-update into launchpad:master.

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..c3dc03c 100644
--- a/lib/lp/snappy/tests/test_snap.py
+++ b/lib/lp/snappy/tests/test_snap.py
@@ -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,41 @@ class TestSnapSet(TestCaseWithFactory):
         self.assertEqual(ref.path, snap.git_path)
         self.assertEqual(ref, snap.git_ref)
 
+    def test_edit_admin_only_fields(self):
+        # The admin fields can only be updated by an admin
+        [ref] = self.factory.makeGitRefs()
+        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:
+            login(ANONYMOUS)
+            self.assertRaises(Unauthorized, setattr, snap, field_name, True)
+            login_admin()
+            setattr(snap, field_name, True)
+
+    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)
+
+        login_admin()
+        self.assertEqual(None, snap.use_fetch_service)
+        snap.use_fetch_service = True
+        self.assertEqual(None, snap.use_fetch_service)
+
+        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():