(PF-2437) Allow r10k to authenticate to the Forge #1192
Conversation
binford2k
force-pushed
the
support_forge_auth
branch
from
91dad73 to
565dafc
Compare
|
Oh cool, good timing on us adding this support then. |
| custom Forge server. When set, 'baseurl' must also be set. | ||
| You will need to prepend your token with 'Bearer ' if using Artifactory as your Forge server. | ||
| The 'authorization_token' setting allows you to provide a token for authenticating to a Forge server. | ||
| You will need to prepend your token with 'Bearer ' to authenticate to the Forge or when using your own Artifactory server. |
There was a problem hiding this comment.
| You will need to prepend your token with 'Bearer ' to authenticate to the Forge or when using your own Artifactory server. | |
| You will need to prepend your token with 'Bearer ' to authenticate to the Forge or when using Artifactory as your Forge server. |
There was a problem hiding this comment.
I suggest waiting to merge this until puppetlabs/forge-ruby#89 is merged and released, since it obviates the need to include 'Bearer'
There was a problem hiding this comment.
Looks like that should work for Artifactory-based Forge too?
There was a problem hiding this comment.
I think so, although I haven't checked that manually - I did manually verify just now that the version of forge-ruby I just released (3.1.0) will allow this to work with the forge API without prepending Bearer in the r10k.yaml setting. Otherwise, the only thing stopping r10k from authenticating before was the check for a baseurl that's removed in this PR.
Unless I'm missing something, it seems like the options here are to:
- Update this README to suggest specifying
Bearer <your-key>as theauthorization_tokensetting for now (regardless of whether you're authenticating to the forge or to artifactory), and leave the forge-ruby dependency as-is
or - Update the forge-ruby dependency to 3.1.0 and don't mention
Bearerat all - the biggest impact from this would probably be that the default forge API url set by forge-ruby would update from https://forgeapi.puppetlabs.com to https://forgeapi.puppet.com.
There was a problem hiding this comment.
I guess a third option would be to add new logic in r10k to prepend Bearer to the key as needed - it wouldn't affect forge-ruby behavior in 3.1.0, and it would let people omit Bearer in their r10k.yaml file if they're using an older forge-ruby version.
There was a problem hiding this comment.
We talked about that and decided we didn't know enough about what token requirements other custom Forge implementations might have, and didn't want to introduce magic that was going to bite us. Though I guess if the Forge gem is doing that now, that point is kinda moot.
There was a problem hiding this comment.
@caseywilliams Would the URL update in 3.1.0 that you mentioned cause breakages? I know we had some trouble updating that URL in r10k, so we're still using the puppetlabs one.
There was a problem hiding this comment.
Yeah, it might - if I remember correctly, the PE acceptance tests were breaking last time we tested using the updated URL in r10k. forgeapi.puppet.com is really the current canonical place to find the API, so everyone should switch over eventually, but the two biggest issues in updating would be (as far as I can tell) (1) if a user had allowlisted access in their network to forgeapi.puppetlabs.com and then suddenly lost access when the url updated but their rules had not, and (2) needing to schedule the acceptance test update work
There was a problem hiding this comment.
looks like bumping the puppet_forge gem to 3.1.0 will involve packaging a bunch of additional dependencies in pe-r10k-vanagon, since faraday 1.3+ has a bunch more dependencies than the version we're currently on.
I'm thinking it might be best to open a separate ticket/issue for bumping puppet_forge, removing the docs around Bearer , and updating the Forge API url. Would you mind doing that @caseywilliams?
There was a problem hiding this comment.
Sure, wasn't sure exactly which jira project was appropriate but I filed PE-32770.
| @@ -230,14 +230,6 @@ def call | |||
| runner.setup_settings | |||
| runner.setup_authorization | |||
| end | |||
|
|
|||
| it 'errors if no custom forge URL is set' do | |||
There was a problem hiding this comment.
Would it be valuable to test that setting authorization_token now does work if baseurl isn't set?
There was a problem hiding this comment.
Considering how mocked up this all is, not really IMO.
|
Is it even possible to do manual testing right now, or does the forge not yet support token auth? |
mwaggett
added
awaiting user feedback
feature
has jira ticket
|
There is Forge WIP that will need to land before this is actually functional. It shouldn't actually break anything, so it won't hurt anything to release early, but I'd wait until @carabasdaniel gives the go-ahead. |
|
okay cool. yeah @binford2k I'd like to wait for that to land so that this can be manually tested before merging. |
|
This won't be ready for testing until PF-2447 is done - I've updated the jira ticket for this PR to reflect that. I'd prefer to wait until we can test with real data before we merge these changes. |
|
@binford2k and @mwaggett - when y'all get a chance to take a look at this again, it looks like PF-2447 was resolved yesterday! |
|
awesome, @binford2k would you mind rebasing and then doing some manual testing? |
In the near future, the Forge will be adding support for authenticated module downloads. This extends support for token based authentication to that use case.
binford2k
force-pushed
the
support_forge_auth
branch
from
565dafc to
f982cc0
Compare
|
@mwaggett manually tested, works as expected 👍 |
In the near future, the Forge will be adding support for authenticated
module downloads. This extends support for token based authentication to
that use case.