-
Notifications
You must be signed in to change notification settings - Fork 79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#641: Specify and remember the Signing Services we want to use for each Repo / Release. #689
Conversation
7d9dc69
to
3f05a3c
Compare
3c4b567
to
12bc925
Compare
a84d748
to
7da9495
Compare
@mdellweg ping, I believe this is ready for merging. We have been using this patch without problem for a month now. |
Can you squash the commits and maybe the migrations too? |
7da9495
to
e883d93
Compare
Sure, done, thanks! |
Hmm, we'll I'll try to figure out what the automation is complaining about, but also it occurs to me that we probably update this to use |
e883d93
to
77ea3c8
Compare
I was wrong previously, the version of this PR that we have been running had diverged from this code and there were a couple of things I had to patch up to get it working right. It should be better now though. |
18cb978
to
e33a76a
Compare
e8a0a6f
to
dc7b0e8
Compare
I added some basic tests to exersize this functionality. I don't know why all the github checks are failing; it doesn't seem to be related to my changes and everything works locally here. |
dc7b0e8
to
3321549
Compare
a4d452f
to
29ef41d
Compare
An example api workflow setting signing service on a specific release:
|
An example api workflow setting signing service on the whole repo:
|
6e1b113
to
0560da9
Compare
…itory or Release creation time. closes pulp#641
0560da9
to
60a3abc
Compare
LGTM. Will merge this when I get back to work (so probably monday) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not done a full review like @hstct, but had a look over the code and performed the following test workflow:
Set up a signing service:
oci-env shell
curl -L https://github.com/pulp/pulp-fixtures/raw/master/common/GPG-PRIVATE-KEY-pulp-qe | gpg --import
echo "6EDF301256480B9B801EBA3D05A5E6DA269D9D98:6:" | gpg --import-ownertrust
SIGNING_SERVICE_NAME='Pulp_QE'
pulpcore-manager add-signing-service --class 'deb:AptReleaseSigningService' "${SIGNING_SERVICE_NAME}" /src/pulp_deb/pulp_deb/tests/functional/sign_deb_release.sh '6EDF301256480B9B801EBA3D05A5E6DA269D9D98'
Main workflow using Pulp CLI and http:
SIGNING_SERVICE_NAME='Pulp_QE'
ENTITIES_NAME='test'
REMOTE_OPTIONS='--url=https://fixtures.pulpproject.org/debian/ --distribution=ragnarok --distribution=ginnungagap --distribution=nosuite --policy=on_demand'
SIGNING_SERVICE_HREF=$(${PULP_CLI_CMD} signing-service show --name=${SIGNING_SERVICE_NAME} | jq -r .pulp_href)
REMOTE_HREF=$(${PULP_CLI_CMD} deb remote create --name=${ENTITIES_NAME} ${REMOTE_OPTIONS} | jq -r .pulp_href)
${HTTP_CMD} post ${PULP_URL}/pulp/api/v3/repositories/deb/apt/ remote=${REMOTE_HREF} name=${ENTITIES_NAME} signing_service_release_overrides:="{\"ragnarok\": \"$SIGNING_SERVICE_HREF\", \"default\": \"$SIGNING_SERVICE_HREF\"}"
${PULP_CLI_CMD} deb repository sync --name=${ENTITIES_NAME}
APT_PUBLICATION_HREF=$(${PULP_CLI_CMD} deb publication create --simple --structured --repository=${ENTITIES_NAME} | jq -r .pulp_href)
${PULP_CLI_CMD} deb distribution create --name=${ENTITIES_NAME} --base-path=${ENTITIES_NAME} --publication=${APT_PUBLICATION_HREF}
This has worked, and I think the PR can be merged as is, I do have a few closing remarks to add:
- Great job on adding some basic documentation to
docs/feature_overview.rst
and the strings used for the API docs! - It looks like adding a
signing_service_release_overrides = {"default": ...
along withsimple=True
on the publication does not interact. I am fine with this, since we want to deprecate usage ofsimple=True
anyway. (This is just something to be aware of). - We should create an issue at pulp-cli-deb to add the
signing_service_release_overrides
andsinging_service
parameters to thepulp deb repository create
command!
I created the issue: pulp/pulp-cli-deb#54 |
…ice overrides Previously we had implemented a pulp_deb patch locally (0008) where we tied the SigningService directly to an Apt Repository or Release via foreign keys to set the "default" for the repo/release. [We submitted it upstream](pulp/pulp_deb#689), and they requested changes where instead of tying Release<->SigningService directly we instead do it via indirect reference to avoid a problem that they've had for forever where if you make any local changes to a Release then syncing from another repo creates a duplicate release. So that's what the pulp patch here is, it's kind of the second half of the 0008 patch that brings us in sync with what pulp_deb has merged upstream. This PR also has server/cli support for setting that field so we don't have to do it via the pulp api directly, like we did last time when 0008 was first released. Because of the database changes in pulp when we deploy this we'll have to reset the overrides on azurecore and azurecore-test: ``` pmc repo update azurecore-apt --add-release-signing-services 'bionic=legacy;xenial=legacy' pmc repo update azurecore-test-apt --add-release-signing-services 'bionic=legacy;xenial=legacy;trusty=legacy' ``` Related work items: #16950389
closes #641