PEP 694: Amend scanning in staged releases and add legacy API changes to use staged releases - #5070
PEP 694: Amend scanning in staged releases and add legacy API changes to use staged releases#5070cjames23 wants to merge 3 commits into
Conversation
…API changes to use staged releases
|
All commit authors signed the Contributor License Agreement. |
Documentation build overview
21 files changed ·
|
hugovk
changed the title
hugovk
changed the title
warsaw
left a comment
warsaw
left a comment
There was a problem hiding this comment.
This is a great addition to the PEP. I have some comments for a few things that need clarification, but otherwise +1. And welcome aboard as a co-author!
| so that (for example) a CI job can install-test a staged release without project upload credentials. | ||
|
|
||
| As one such stricter policy, an index **MAY** require *additional* authorization, beyond upload permission, to | ||
| :ref:`publish <publishing-session-completion>` or :ref:`cancel <publishing-session-cancellation>` a session, |
There was a problem hiding this comment.
+1 for publishing, but can you provide some additional scenarios about why cancelation should require additional authorization?
There was a problem hiding this comment.
I see how this reads a little weird. I was trying to state that an index MAY require a separate set of permissions for moving publish state to either completed or cancelled.
| **MAY** treat it as though it had completed without an adverse result and publish the session, so that a | ||
| backlogged or unavailable review system does not indefinitely prevent publication. If the review produces an | ||
| adverse result, the session resolves to ``error`` with the reason reported in the session's ``notices``; the | ||
| server **MAY** decline to publish such a session on any subsequent retry, in which case it is eventually |
There was a problem hiding this comment.
This section (if not covered below), should provide details on what happens if files in the session are deleted and reuploaded. It could be:
- Continue to await human review
- Rescan newly uploaded files
- Reset the entire malware scan state
It probably would also be helpful to say something about denial of service attacks on the scanner. E.g. bad actor starts an upload session, uploads known malware knowing it will get flagged. Bad Actor deletes the files and reuploads them to trigger another scan. Rinse and repeat until the scanning service is overwhelmed.
|
|
||
| .. _legacy-interop: | ||
|
|
||
| Legacy Upload API Interoperability |
There was a problem hiding this comment.
I wonder if it makes sense to be more explicit about the "control plane" API for managing the session (and file uploads), and/or lump this and the explicit session creation together? Maybe add a top-level "Session Creation" section, putting this and the original session creation request together, then the rest of it would be the same control plane API.
It also occurs to me that we might want to update the state diagrams so that it's obvious that there are two entry points "open" state. However, if I'm reading this section correctly, the index can also auto-publish if the conditions allow (no adverse scan or scan timeout). Maybe that'll make the diagram too cluttered in which case we could skip it or add a second diagram. LMK what you think and I can take a crack at updating the diagram once this PR lands.
The other thing this section should discuss is the interaction between legacy and 2.0 sessions. E.g. what happens if there's an open 2.0 session? Can someone use the legacy API to add a file to that existing session? I would think that would get rejected with a 409. Also vice versa (meaning: do we allow mixing of session creation requests?). We just need to be explicit about how these mix.
There was a problem hiding this comment.
IN terms of a top level Session Creation section I had that in another branch but was not sure, this is the signal it should be a separated out top level section which would then be the same control plane API.
I will be explicit about the mixing of session creation requests which I am aligned on an attempt to mix would be rejected with a 409.
warsaw
left a comment
warsaw
left a comment
There was a problem hiding this comment.
I really like this addition to the PEP. It provides a fantastic transition period from legacy to upload-2.0. I have a few questions and suggestions.
| * entering the publishing session workflow from the existing legacy upload API, so that staging is | ||
| available to publishers before their tooling adopts this API; |
There was a problem hiding this comment.
| * entering the publishing session workflow from the existing legacy upload API, so that staging is | |
| available to publishers before their tooling adopts this API; | |
| * entering the publishing session workflow from the existing legacy upload API, so that staging is | |
| available to publishers before their tooling fully adopts this API; |
I struggled a little bit with this text, since publishers will have to update their tooling either way. Even twine will have to grow the inclusion of a staged field. I think what you're trying to get at is that we can get the staging benefits without the full new control plane protocol. I couldn't find an elegant way to say that so I just added the word "fully".
| system alone can neither publish a release nor discard a pending one. | ||
|
|
||
| Cancellation is protected because it is destructive: it discards the staged files and frees the name-version | ||
| pair. An index that left it at the upload-permission level would let a compromised upload credential delete a |
There was a problem hiding this comment.
Cancelation also frees any temporary new project name reservation, which would allow an attacker with the compromised credential to "steal" the name. So that's yet another reason why an index might want to separate these permissions.
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| Publishers cannot use the features of this API until their upload tooling adopts it, and the legacy API is | ||
| expected to remain available as this pep does not propose a deprecatioin schedule. To make staging available to those publishers sooner, an index |
There was a problem hiding this comment.
| expected to remain available as this pep does not propose a deprecatioin schedule. To make staging available to those publishers sooner, an index | |
| expected to remain available anyway, as this PEP does not propose a deprecation schedule. To make staging available to those publishers sooner, an index |
The presence of a staged field would technically require tooling to change, but minimally. This section is also proposing that an index can create a staged release even without the staged field, which is interesting because then no tools actually need to be changed.
I wonder if that distinction should be called out more explicitly?
| version **SHOULD** be added to the same open session when that session was itself created through a legacy | ||
| upload, so that the release is still published as a unit. | ||
|
|
||
| A session's creation path is fixed when it is created, and the two paths are not mixed. If a non-terminal |
There was a problem hiding this comment.
| A session's creation path is fixed when it is created, and the two paths are not mixed. If a non-terminal | |
| A session's creation path is fixed when it is created, and the two paths **MUST NOT** be mixed. If a non-terminal |
| contribute to it through the *other* path **MUST** be rejected with a ``409 Conflict``: a legacy | ||
| ``staged=true`` upload for a pair that already has an open session created through the Upload 2.0 API is | ||
| rejected, and an Upload 2.0 request that would add to a session created through a legacy upload is likewise | ||
| rejected. |
There was a problem hiding this comment.
I'm confused by the requirements here. I would totally expect rejection for a path where a legacy upload without the staged=true flag when an Upload 2.0 session is already created for the name-version pair, because that's mixing legacy-legacy with upload-2.0. But legacy+staged into an Upload 2.0 stage could be allowed without any semantic confusion -- I think! Or is that also a problem? And vice versa of course (legacy+staged followed by Upload 2.0 ... okay or not okay?). If I'm confused maybe some additional rationale is warranted?
2 participants
Amendments based on DPO discussion https://discuss.python.org/t/pre-pep-staged-releases-separated-from-pep-694/107804/59
@warsaw - here are my proprosed amendments which I would like your sign off on as well.