Implement flatpak index view #1301

stbergmann · 2023-06-06T14:28:57Z

This implements the protocol documented at https://github.com/owtaylor/flagstate/blob/master/docs/protocol.md, with the following notes:

It does not announce any Annotations, only Labels.
It does not announce any Lists of images, only individual Images.

Both of those simplifications are in line with how upstream flatpak indexers (e.g., https://github.com/owtaylor/flatpak-indexer serving https://registry.fedoraproject.org/) behave and with what clients actually expect and make use of.

(This fixes https://issues.redhat.com/browse/SAT-4416 "Create a JSON index to allow installing via Flatpak client".)

closes #1315

(The Pulp folks asked for that clarification in response to my work-in-progress implementation of this protocol for Pulp at <pulp/pulp_container#1301> "Implement flatpak index view".)

lubosmj

Would you mind further squashing all the commits into a single one and fixing the lint errors: https://github.com/pulp/pulp_container/actions/runs/5201618714/jobs/9384837157?pr=1301?

For caching, you will need to override the make_key method.

pulp_container/pulp_container/app/cache.py

Line 39 in a424671

def make_key(self, request):

pulp_container/app/registry_api.py

stbergmann · 2023-06-08T12:25:30Z

Would you mind further squashing all the commits into a single one

@ipanova requested that "you can keep the commits with all the extra info you've put there since that will help". Please let me know what's preferred.

and fixing the lint errors: https://github.com/pulp/pulp_container/actions/runs/5201618714/jobs/9384837157?pr=1301?

done, stbergmann@2ce9aeb "Manually reformat according to black 23.3.0"

pulp_container/app/cache.py

pulp_container/app/models.py

ipanova

As discussed on slack with @stbergmann there is some undocumented/outdated information on the specs that will need to be fixed.
Another question i have, is about specs themselves, they do not really seem to be official or finalized. They are not accepted by registry specs either. Last mention I found about them is that they are in DRAFT https://opencontainers.org/posts/blog/2018-11-07-bringing-oci-images-to-the-desktop-with-flatpak/

With this reasoning we need to introduce a so called feature flag, probably a pulp setting that will enabling or disabling the flatpack index support in the pulp-container registry. By default it should be disabled.

@stbergmann can you provide multiple various examples of how client request would look like?
We also will need to add docs as part of this PR.

pulp_container/app/models.py

ipanova · 2023-06-20T13:56:31Z

pulp_container/app/cache.py

+    def make_key(self, request):
+        """Make a key composed of the request's query."""
+        all_keys = {
+            USER_KEY: request.user.get_username(),


don't you need to also have CacheKeys.host as key?

reading specs Requests to this endpoint are expected to be repeated with exactly the same parameters, possibly by many different clients. It mean different clients expect to repeat same query .Does it mean that it makes sense to cache the query params but not the user==client? :)

from the recent info we got, since index is unauthenticated then probably it does not make sense to register the user key?

don't you need to also have CacheKeys.host as key?

I have really no idea. And I don't even know what the semantics of CacheKeys.host are supposed to be. My naive assumption is that it would be related to some form of multi-tenancy support, and I already tried to tangentially ask about this in an earlier comment at #1301 (comment).

Does it mean that it makes sense to cache the query params but not the user==client? :)

I'm not sure I understand what you mean with the above. Is this perhaps already covered by the next item now?

from the recent info we got, since index is unauthenticated then probably it does not make sense to register the user key?

done, 2818e70 "Only index public repositories"

pulp_container/app/registry_api.py

ipanova · 2023-06-20T16:47:16Z

Another question, do flatpack clients account for the registry auth? In that case you need to make it work with token auth, the view needs to have ContainerRegistryApiMixin

stbergmann · 2023-06-21T11:21:42Z

As discussed on slack with @stbergmann there is some undocumented/outdated information on the specs that will need to be fixed.

I had created owtaylor/flagstate#6 "protocol.md: Fix note formatting and add a note about annotations and image lists". I'm not aware there's further "undocumented/outdated information" apart from that "note that annotations and image lists are currently unused" (and thus not implemented by this pulp_container PR). Let me know if you think more is missing.

Another question i have, is about specs themselves, they do not really seem to be official or finalized. They are not accepted by registry specs either. Last mention I found about them is that they are in DRAFT https://opencontainers.org/posts/blog/2018-11-07-bringing-oci-images-to-the-desktop-with-flatpak/

Can you be more specific what exactly you mean with "registry specs"?

On a RH-internal slack channel, @owtaylor mentioned that the "docs [...] at https://github.com/owtaylor/flagstate/blob/master/docs/protocol.md are as official as it gets. I perhaps should move that protocol doc to a separate repository under github.com/flatpak, or into the flatpak project, since the "flagstate" project is dead, but you can certainly count it as official"

With this reasoning we need to introduce a so called feature flag, probably a pulp setting that will enabling or disabling the flatpack index support in the pulp-container registry. By default it should be disabled.

OK, I will look into implementing that.

@stbergmann can you provide multiple various examples of how client request would look like? We also will need to add docs as part of this PR.

Clients here are the flatpak command line interface and (IIUC) integration into applications like GNOME Software through libflatpak (both the flatpak command line tool and libflatpak are provided by https://github.com/flatpak/flatpak). A typical usage scenario is to set up an OCI-based remote,

$ flatpak remote-add some-name oci+https://...

pointing at the https://... base URL relative to which the ./index/static endpoint is served. Then,

$ flatpak remote-ls some-name

retrieves information from that endpoint via some https://.../index/static?label:org.flatpak.ref:exists=1&architecture=...&os=linux&tag=latest. Then, given the identifier of some flatpak app advertised through the above,

$ flatpak install some-name identifier

pulls the relevant https://.../base-path/blobs/....

pulp_container/app/registry_api.py

ipanova · 2023-06-21T11:35:33Z

I filed story to track this #1315

stbergmann · 2023-06-21T13:04:00Z

Another question, do flatpack clients account for the registry auth? In that case you need to make it work with token auth, the view needs to have ContainerRegistryApiMixin

I don't really know. Maybe @owtaylor can give some information here?

From how I implemented things in this PR and tried them out in a local oci-env instance, I naively assumed that per the design of Pulp the /index/static endpoint will by necessity be behind some kind of authentication. I found that in my local oci-env instance it can be reached via http://admin:changeme@localhost:5001/index/static (and I patched my local flatpak command-line client to make such URLs containing user:password work there, see flatpak/flatpak#5446 "[Bug]: Support oci remotes with URLs containing user:password"). At https://matrix.to/#/!RfXaBjokqHAbzZrgHz:matrix.org/$reAbU4QuJaBhruvvjtAAZvJtp-3iIEHavOzFMYoudK4?via=matrix.org&via=gnome.org&via=kde.org, I noticed: "I see there's some --authenticator-name etc. arguments to flatpak remote-add, but can't find any documentation about that, so no idea it could help me. Any ideas?" But didn't get much of an authoritative response.

So, first question presumably is whether, per the design of Pulp and Satellite, these /index/dynamic and /index/static endpoints should be behind some kind of authentication in the first place?

ipanova · 2023-06-21T13:25:54Z

Another question, do flatpack clients account for the registry auth? In that case you need to make it work with token auth, the view needs to have ContainerRegistryApiMixin

I don't really know. Maybe @owtaylor can give some information here?

From how I implemented things in this PR and tried them out in a local oci-env instance, I naively assumed that per the design of Pulp the /index/static endpoint will by necessity be behind some kind of authentication. I found that in my local oci-env instance it can be reached via http://admin:changeme@localhost:5001/index/static (and I patched my local flatpak command-line client to make such URLs containing user:password work there, see flatpak/flatpak#5446 "[Bug]: Support oci remotes with URLs containing user:password"). At https://matrix.to/#/!RfXaBjokqHAbzZrgHz:matrix.org/$reAbU4QuJaBhruvvjtAAZvJtp-3iIEHavOzFMYoudK4?via=matrix.org&via=gnome.org&via=kde.org, I noticed: "I see there's some --authenticator-name etc. arguments to flatpak remote-add, but can't find any documentation about that, so no idea it could help me. Any ideas?" But didn't get much of an authoritative response.

So, first question presumably is whether, per the design of Pulp and Satellite, these /index/dynamic and /index/static endpoints should be behind some kind of authentication in the first place?

Per pulp-container design, as long as this endpoint is considered a registry endpoint it should be backed up by token auth.
Satellite has it's own auth layer, so this is a separate unrelated question to answer, for now.

owtaylor · 2023-06-21T14:58:26Z

Another question, do flatpack clients account for the registry auth? In that case you need to make it work with token auth, the view needs to have ContainerRegistryApiMixin

I don't really know. Maybe @owtaylor can give some information here?

The design of the Flatpak client is that the index is unauthenticated and authentication is only needed to access the actually container data. This design is meant to handle things like registry.redhat.io having subscription-only content, and also (in other contexts) the idea that a Flatpak remote might have paid content, but is not meant to provide privacy for registry data.

It's not terribly easy to change this because of the split between the user and system sessions for system-wide Flatpak installs.

The download of the index is done by the system, so it can verify what is being installed against the digests in the index.
But the auth workflow and content download is done in the user session (so the user can be prompted for passwords or other credentials, so that downloads can be interrupted, etc)

So the ideal thing would be if we could just make the index unauthenticated - if that doesn't violate pulp principles too badly. If we are doing this, then requiring label:org.freedesktop.exists=1 might make sense to not leak information about non Flatpak containers in the registry.

The alternative, I guess, would be to add standard registry authentication (with what scope?) and then until we can add appropriate support in the Flatpak client, just document that Flatpak access only works for non-authenticated (but likely VPN protected) repositories.

stbergmann · 2023-06-22T11:50:17Z

So the ideal thing would be if we could just make the index unauthenticated - if that doesn't violate pulp principles too badly. If we are doing this, then requiring label:org.freedesktop.exists=1 might make sense to not leak information about non Flatpak containers in the registry.

@ipanova Would that be possible, to "make the index unauthenticated"?

@owtaylor I assume you meant label:org.flatpak.ref:exists=1 instead of label:org.freedesktop.exists=1.

ipanova · 2023-06-22T12:47:19Z

@owtaylor if it is expected to have index unauthenticated then the assumption that the index is going to expose only public repos. Public repos can be are accessible by everyone( aka unauthenticated, anonymous pull). It does not make sense to go through all the content repos( perf. overhead) , expose them( information leak) and then leave it to the user session to provide it's credentials which might or might not give a 200 on content pull.
If my read on this is same as your then we're on the same page.

ipanova · 2023-06-22T13:40:22Z

Here's my interpretation and summary of the outstanding work:

Add docs
Add new boolean setting FLATPACK_INDEX=False, it will make flatpack index url conditional
move https://github.com/owtaylor/flagstate/blob/master/docs/protocol.md to https://github.com/flatpack/flagstate/blob/master/docs/protocol.md
update flagstate protocol.md with the recent info/changes
tailor the code to the Flatpack client. i.e:
1. it is expected that client sends always only one of (not repeated query param): repo, tag, label, os, arch, annotation. however in case query params are repeated follow the OR logic Implement flatpak index view #1301 (comment)
2. query param values are not input validated i.e. os=blabla would not raise error rather return a []
3. query param value has always a single value (comma separated list is not understood), otherwise raise 400, not sure on this one, @owtaylor
4. if flatpack label is not present in the query raise 400
5. for unknown query param raise 400 (only repo, tag, label, os, arch, annotation are understood)
index/static and index/dynamic are non authed urls. It means only public repo are exposed.
figure out how to treat annotations query param Implement flatpak index view #1301 (comment)

stbergmann · 2023-06-22T13:54:07Z

  1. client sends always only _one_ (not repeated query param): repo, tag, label, os, arch, annotation. If query param is repeated raise 400

It was easy to address that properly (i.e., by not raising 400 but rather implementing the intended semantics) with 719a9f5 "Fix logic for repeated query parameters", so I at least would be fine with leaving the code that way.

ipanova · 2023-06-22T14:02:08Z

  1. client sends always only _one_ (not repeated query param): repo, tag, label, os, arch, annotation. If query param is repeated raise 400
It was easy to address that properly (i.e., by not raising 400 but rather implementing the intended semantics) with 719a9f5 "Fix logic for repeated query parameters", so I at least would be fine with leaving the code that way.

I am fine either way as long as we all agree what to follow. Updated my previous comment, check it out

owtaylor · 2023-06-22T14:23:52Z

@owtaylor if it is expected to have index unauthenticated then the assumption that the index is going to expose only public repos. Public repos can be are accessible by everyone( aka unauthenticated, anonymous pull). It does not make sense to go through all the content repos( perf. overhead) , expose them( information leak) and then leave it to the user session to provide it's credentials which might or might not give a 200 on content pull.
If my read on this is same as your then we're on the same page.

I agree that public index, private content is probably not needed for the expected use of Flatpak in pulp_container - an organization publishing content to a deployment of workstations and desktops. And defaulting to that would definitely violate the principle of least surprise.

I think we can start off with only indexing public repos, and then that doesn't work for some users, then we have an actual use case, and can decide whether we need:

Public index, private content (as an opt-in boolean per repo?)
Private index, private content (requires Flatpak client changes.)

owtaylor · 2023-06-22T14:36:22Z

Here's my interpretation and summary of the outstanding work:

move https://github.com/owtaylor/flagstate/blob/master/docs/protocol.md to https://github.com/flatpack/flagstate/blob/master/docs/protocol.md

That doesn't need to block this PR, bu tyes, I'll try to move the protocol to some repository under the flatpak organization.

Other than that and @stbergmann's comment, the list looks about right to me. Responded to the questions about private content and annotations separately.

stbergmann · 2023-06-23T13:00:33Z

[...]

With this reasoning we need to introduce a so called feature flag, probably a pulp setting that will enabling or disabling the flatpack index support in the pulp-container registry. By default it should be disabled.

i.e.,

[...]

Add new boolean setting FLATPACK_INDEX=False, it will make flatpack index url conditional

I assume I'd add a line FLATPAK_INDEX = False to pulp_container's pulp_container/app/settings.py. But then, how would I set that to true in my local oci_env? Adding a line PULP_FLATPAK_INDEX=true to oci_envs base/pulp_config.env and/or profiles/pulp_container_base/pulp_config.env apparently doesn't enable it. (And https://github.com/pulp/oci_env#profiles leaves me clueless. What is a profile, exactly? How do I use one? Do I use one per plugin? ...)

stbergmann · 2023-06-26T08:26:10Z

[...]

With this reasoning we need to introduce a so called feature flag, probably a pulp setting that will enabling or disabling the flatpack index support in the pulp-container registry. By default it should be disabled.

i.e.,

[...]

Add new boolean setting FLATPACK_INDEX=False, it will make flatpack index url conditional

I assume I'd add a line FLATPAK_INDEX = False to pulp_container's pulp_container/app/settings.py. But then, how would I set that to true in my local oci_env? Adding a line PULP_FLATPAK_INDEX=true to oci_envs base/pulp_config.env and/or profiles/pulp_container_base/pulp_config.env apparently doesn't enable it. (And https://github.com/pulp/oci_env#profiles leaves me clueless. What is a profile, exactly? How do I use one? Do I use one per plugin? ...)

done now with 097f8cd "Make support for FLATPAK_INDEX conditional", and enabling it in oci_env by adding PULP_FLATPAK_INDEX=true to base/pulp_config.env works as intended (no idea why it didn't work for me initially)

pulp_container/app/registry_api.py

docs/workflows/flatpak.rst

docs/workflows/index.rst

template_config.yml

pulp_container/app/registry_api.py

.github/workflows/update_ci.yml

lubosmj

Thanks for working on this! Please, consider this comment once again: #1301 (comment).

ipanova · 2023-07-24T11:32:14Z

@owtaylor do you want to give another look before this gets merged?

stbergmann · 2023-07-31T07:58:16Z

Please, consider this comment once again: #1301 (comment).

Which I had addressed with "But anyway, if it's better to be explicit here, done that now in d8ad7a6" at #1301 (comment), no?

closes pulp#1315

ipanova marked this pull request as draft June 6, 2023 14:31

lubosmj reviewed Jun 7, 2023

View reviewed changes

pulp_container/app/registry_api.py Outdated Show resolved Hide resolved

pulp_container/app/registry_api.py Outdated Show resolved Hide resolved

pulp_container/app/registry_api.py Outdated Show resolved Hide resolved

pulp_container/app/registry_api.py Outdated Show resolved Hide resolved

stbergmann commented Jun 14, 2023

View reviewed changes

pulp_container/app/cache.py Outdated Show resolved Hide resolved

stbergmann commented Jun 19, 2023

View reviewed changes

pulp_container/app/models.py Outdated Show resolved Hide resolved

keplersj mentioned this pull request Jun 19, 2023

Automatically Generate Summary File pulp/pulp_ostree#269

Closed

ipanova reviewed Jun 20, 2023

View reviewed changes

ipanova reviewed Jun 21, 2023

View reviewed changes

pulp_container/app/registry_api.py Outdated Show resolved Hide resolved

lubosmj mentioned this pull request Jun 22, 2023

protocol.md: Fix note formatting and add a note about annotations and image lists owtaylor/flagstate#6

Closed

ipanova reviewed Jun 28, 2023

View reviewed changes

pulp_container/app/registry_api.py Show resolved Hide resolved

ipanova reviewed Jun 28, 2023

View reviewed changes

pulp_container/app/registry_api.py Show resolved Hide resolved

ipanova reviewed Jun 28, 2023

View reviewed changes

pulp_container/app/registry_api.py Outdated Show resolved Hide resolved

ipanova reviewed Jun 28, 2023

View reviewed changes

docs/workflows/flatpak.rst Outdated Show resolved Hide resolved

ipanova reviewed Jun 28, 2023

View reviewed changes

docs/workflows/flatpak.rst Outdated Show resolved Hide resolved

stbergmann force-pushed the flatpak-wip branch 5 times, most recently from 593c538 to 46db69e Compare July 18, 2023 07:55

ipanova reviewed Jul 18, 2023

View reviewed changes

docs/workflows/index.rst Outdated Show resolved Hide resolved

template_config.yml Show resolved Hide resolved

pulp_container/app/registry_api.py Outdated Show resolved Hide resolved

ipanova reviewed Jul 18, 2023

View reviewed changes

pulp_container/app/registry_api.py Outdated Show resolved Hide resolved

stbergmann force-pushed the flatpak-wip branch 12 times, most recently from dc92a8d to d8ad7a6 Compare July 20, 2023 13:34

stbergmann commented Jul 21, 2023

View reviewed changes

.github/workflows/update_ci.yml Outdated Show resolved Hide resolved

stbergmann force-pushed the flatpak-wip branch 3 times, most recently from 2e790f5 to f08a44f Compare July 21, 2023 14:50

lubosmj approved these changes Jul 24, 2023

View reviewed changes

ipanova mentioned this pull request Jul 24, 2023

As a user I host and expose flatpack content via flatpack index #1315

Closed

Support for Flatpak index endpoints

86fa8a5

closes pulp#1315

stbergmann force-pushed the flatpak-wip branch from f08a44f to 86fa8a5 Compare July 31, 2023 11:23

lubosmj merged commit eec5133 into pulp:main Aug 2, 2023
9 of 10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement flatpak index view #1301

Implement flatpak index view #1301

stbergmann commented Jun 6, 2023 •

edited by lubosmj

Loading

lubosmj left a comment

stbergmann commented Jun 8, 2023

ipanova left a comment

ipanova Jun 20, 2023

ipanova Jun 20, 2023

ipanova Jun 22, 2023

stbergmann Jun 23, 2023

ipanova commented Jun 20, 2023 •

edited

Loading

stbergmann commented Jun 21, 2023

ipanova commented Jun 21, 2023

stbergmann commented Jun 21, 2023

ipanova commented Jun 21, 2023

owtaylor commented Jun 21, 2023

stbergmann commented Jun 22, 2023

ipanova commented Jun 22, 2023 •

edited

Loading

ipanova commented Jun 22, 2023 •

edited

Loading

stbergmann commented Jun 22, 2023

ipanova commented Jun 22, 2023

owtaylor commented Jun 22, 2023

owtaylor commented Jun 22, 2023

stbergmann commented Jun 23, 2023

stbergmann commented Jun 26, 2023

lubosmj left a comment

ipanova commented Jul 24, 2023

stbergmann commented Jul 31, 2023

Implement flatpak index view #1301

Implement flatpak index view #1301

Conversation

stbergmann commented Jun 6, 2023 • edited by lubosmj Loading

lubosmj left a comment

Choose a reason for hiding this comment

stbergmann commented Jun 8, 2023

ipanova left a comment

Choose a reason for hiding this comment

ipanova Jun 20, 2023

Choose a reason for hiding this comment

ipanova Jun 20, 2023

Choose a reason for hiding this comment

ipanova Jun 22, 2023

Choose a reason for hiding this comment

stbergmann Jun 23, 2023

Choose a reason for hiding this comment

ipanova commented Jun 20, 2023 • edited Loading

stbergmann commented Jun 21, 2023

ipanova commented Jun 21, 2023

stbergmann commented Jun 21, 2023

ipanova commented Jun 21, 2023

owtaylor commented Jun 21, 2023

stbergmann commented Jun 22, 2023

ipanova commented Jun 22, 2023 • edited Loading

ipanova commented Jun 22, 2023 • edited Loading

stbergmann commented Jun 22, 2023

ipanova commented Jun 22, 2023

owtaylor commented Jun 22, 2023

owtaylor commented Jun 22, 2023

stbergmann commented Jun 23, 2023

stbergmann commented Jun 26, 2023

lubosmj left a comment

Choose a reason for hiding this comment

ipanova commented Jul 24, 2023

stbergmann commented Jul 31, 2023

stbergmann commented Jun 6, 2023 •

edited by lubosmj

Loading

ipanova commented Jun 20, 2023 •

edited

Loading

ipanova commented Jun 22, 2023 •

edited

Loading

ipanova commented Jun 22, 2023 •

edited

Loading