SLES remote support with authentication token #1729

pavelpicka · 2020-05-29T17:23:33Z

New remote type for Suse Enterprise Linux Server. If user provides his
repository authentication token it is possible to sync SLES official repository.

closes: #6729
https://pulp.plan.io/issues/6729

[nocoverage]

pulpbot · 2020-05-29T17:28:52Z

Attached issue: https://pulp.plan.io/issues/6729

pulp_rpm/app/downloaders.py

pulp_rpm/app/tasks/synchronizing.py

jlsherrill · 2020-06-08T18:12:36Z

What was the reasoning to use a different remote rather than the existing remote? This is functionality identical, but just has a uniqe auth mechanism

bmbouter · 2020-06-09T15:18:40Z

I can think of two motivating reasons.

Another remote type is more explicit for both users and developers, especially as requirements diverge over time. Additionally, we want reduce user confusion by having all remote options to be meaningful in all cases; attaching the auth options to a Remote that uses them prevents users from configuring Suse-specific options for non-Suse Remotes.

jlsherrill · 2020-06-09T15:47:31Z

Another remote type is more explicit for both users and developers, especially as requirements diverge over time.
I can't speak for future requirements that aren't defined yet, but i think today a new remote type is not more clear in this case. A specialized auth method seems pretty straight forward for a user or developer to understand if it is named correctly.

To me a remote should be defined by the technology that defines it, in this case Yum (i'd actually rename it to Yum Remote if i had a magic wand). Would/Could it be used for syncing 3rd party repos for Suse content? No Would/Could it be used for syncing SLES repos from some other source (like another pulp server)? No What other special logic would be in this remote that justifies its existence?

we want reduce user confusion by having all remote options to be meaningful in all cases; attaching the auth options to a Remote that uses them prevents users from configuring Suse-specific options for non-Suse Remotes.

Given my previous statements, does it actually help them? I would think a user would create a sles remote for some other sles repo and be confused why an auth token is required. If it was more clear that the user should always use SLES for SLES content (like a special content type) i could see needing a new remote, but for the user to know to select a Remote only if they are syncing SLES and only if they are syncing from one specific source of content seems more confusing than just having a SLES auth token option IMO.

bmbouter · 2020-06-09T16:32:27Z

The idea is that SLES content should always use the SLES remote even if it does not require the auth to work. This came up in the pulp_rpm discussion also. The explicit over implicit I mentioned in the design helps the user answer the question: which of these are for sles content? Without having a separate remote type that's not practical to answer. Maybe that's not valuable to katello (or users in general) but that's the idea that was discussed.

What I'm wondering is: what are the downsides of having a clearer separation with a new remote?

jlsherrill · 2020-06-09T17:43:17Z

@bmbouter thanks for explaining a bit, i def. had some misunderstandings. So auth token isn't required?

I'm still trying to grok the vision, as that seems more of a labeling/tag mechanism than a remote type.
The only downside i can think is if you need to change the type to get the added functionality of some other remote (but i'm still trying to think through the reasons that someone would choose this remote if it existed). To change the 'type' you'd have to delete the remote and add a new one which is a tad cumbersome on the user and might have other implications.

To me it was more about the fact that I thought i knew what a remote was and what it meant to be a remote and this seemed to violate that. So its possible if the meaning is changing, or i'm wrong in my thinking, it may need some adjustment. However I think it should be clearly defined if this is the route to go, and it kinda feels like that hasn't been done yet?

bmbouter · 2020-06-09T17:58:49Z

I am having second thoughts about this. Despite a PR being available I'd like to discuss the design once more at the pulp_rpm meeting on June 11th. @pavelpicka @goosemania @dkliban

goosemania · 2020-06-10T07:43:59Z

@bmbouter , +1 to discuss it.

FWIW, there was a broad agreement that if we create a new remote for SLES repos it will cover only cases with auth token. There was even a strong suggestion to call it AuthTokenRemote and we agreed to come up with the name on the PR. Otherwise, there is no difference from RpmRemote, users can randomly use any Remote, migration is not able to identify SLES repos without auth token, so it will be a messy user experience.

abr1x · 2020-06-15T10:18:45Z

What was the reasoning to use a different remote rather than the existing remote? This is functionality identical, but just has a uniqe auth mechanism

I totally agree on @jlsherrill

FWIW, there was a broad agreement that if we create a new remote for SLES repos it will cover only cases with auth token. There was even a strong suggestion to call it AuthTokenRemote and we agreed to come up with the name on the PR. Otherwise, there is no difference from RpmRemote, users can randomly use any Remote, migration is not able to identify SLES repos without auth token, so it will be a messy user experience.

As a user, I'm even more confused when I need to use an additional RpmRemote when I only want to use an auth_token especially when I have another 3rd party rpm repo which is not related SLES but needs an auth token.

@bmbouter thanks for explaining a bit, i def. had some misunderstandings. So auth token isn't required?

No, since SLES12 an auth token is always required for the official SLES repositories.

pavelpicka · 2020-06-15T13:33:17Z

After another discussion, we decided to take the way with one RpmRemote and optional argument of auth token. As mainly targeting SLES its name is 'sles_auth_token'.

docs/workflows/create_sync_publish.rst

pulp_rpm/app/downloaders.py

bmbouter

This looks good to me, but get another ack maybe from @goosemania or @ipanova before merging.

dralley

Looks good!

docs/workflows/create_sync_publish.rst

goosemania · 2020-06-15T15:30:43Z

pulp_rpm/app/downloaders.py

+from logging import getLogger
+from aiohttp.client_exceptions import ClientResponseError
+from pulpcore.plugin.download import HttpDownloader
+from urllib.parse import urljoin


Please check other files or pep https://www.python.org/dev/peps/pep-0008/#imports to sort imports appropriately.
We usually leave a newline between different categories.

pulp_rpm/app/downloaders.py

goosemania · 2020-06-15T15:32:50Z

pulp_rpm/app/downloaders.py

+        Initialize the downloader.
+        """
+        if 'sles_auth_token' in kwargs.keys():
+            self.sles_auth_token = kwargs.pop('sles_auth_token')


Why do you need to remove sles_auth_token from the kwargs?

no need to remove but still need to "transfer" to class variable, as another class method using it. In this case I think doesn't matter if kwargs.pop() or kwargs['sles...'] is used for assign it.

Fair enough. I try not to change any incoming objects if I don't really need to.

pulp_rpm/app/downloaders.py

pulp_rpm/app/models/repository.py

pulp_rpm/app/tasks/synchronizing.py

goosemania · 2020-06-16T07:40:07Z

pulp_rpm/app/models/repository.py

@@ -148,6 +151,61 @@ class RpmRemote(Remote):
    """

    TYPE = 'rpm'
+    sles_auth_token = models.CharField(


@bmbouter , @dralley , should auth token be mutually exclusive with username/password or client cert/key?

These things aren't intended to be configured together, but allowing them to be is probably ok. For example multi-factor authentication promotes the idea that two types of authentication are used at once, so if any server side decides to do that, our helpful feature would prevent users from using it.

I don't have any strong feeling on this, ^ is just my general take in this situation.

Added support for SLES repository with use of authentication token. Also updated docs how to use it. closes: #6729 https://pulp.plan.io/issues/6729 [nocoverage]

dkliban reviewed May 29, 2020

View reviewed changes

pulp_rpm/app/downloaders.py Show resolved Hide resolved

pavelpicka force-pushed the 6729-suse-auth-token branch 3 times, most recently from 9091957 to 61e153a Compare May 29, 2020 20:05

dralley reviewed May 31, 2020

View reviewed changes

pulp_rpm/app/tasks/synchronizing.py Outdated Show resolved Hide resolved

pavelpicka force-pushed the 6729-suse-auth-token branch 4 times, most recently from 4f0893a to 59a482e Compare June 1, 2020 11:50

pavelpicka changed the title ~~[WIP] SLES remote support with authentication token~~ SLES remote support with authentication token Jun 1, 2020

pavelpicka changed the title ~~SLES remote support with authentication token~~ [WIP] SLES remote support with authentication token Jun 11, 2020

pavelpicka force-pushed the 6729-suse-auth-token branch from 59a482e to 9be4174 Compare June 15, 2020 13:23

bmbouter reviewed Jun 15, 2020

View reviewed changes

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

bmbouter reviewed Jun 15, 2020

View reviewed changes

pulp_rpm/app/downloaders.py Show resolved Hide resolved

bmbouter approved these changes Jun 15, 2020

View reviewed changes

pavelpicka force-pushed the 6729-suse-auth-token branch from 9be4174 to 9cdd442 Compare June 15, 2020 15:24

dralley approved these changes Jun 15, 2020

View reviewed changes

goosemania reviewed Jun 15, 2020

View reviewed changes

pulp_rpm/app/tasks/synchronizing.py Outdated Show resolved Hide resolved

goosemania reviewed Jun 16, 2020

View reviewed changes

SLES support with authentication token

3f84d87

Added support for SLES repository with use of authentication token. Also updated docs how to use it. closes: #6729 https://pulp.plan.io/issues/6729 [nocoverage]

pavelpicka force-pushed the 6729-suse-auth-token branch from 9cdd442 to 3f84d87 Compare June 16, 2020 12:12

goosemania changed the title ~~[WIP] SLES remote support with authentication token~~ SLES remote support with authentication token Jun 16, 2020

goosemania merged commit 546da71 into pulp:master Jun 16, 2020

SLES remote support with authentication token #1729

SLES remote support with authentication token #1729

pavelpicka commented May 29, 2020

pulpbot commented May 29, 2020

jlsherrill commented Jun 8, 2020

bmbouter commented Jun 9, 2020

jlsherrill commented Jun 9, 2020

bmbouter commented Jun 9, 2020

jlsherrill commented Jun 9, 2020

bmbouter commented Jun 9, 2020

goosemania commented Jun 10, 2020

abr1x commented Jun 15, 2020

pavelpicka commented Jun 15, 2020

bmbouter left a comment

dralley left a comment

goosemania Jun 15, 2020

goosemania Jun 15, 2020

pavelpicka Jun 16, 2020

goosemania Jun 16, 2020

goosemania Jun 16, 2020

bmbouter Jun 16, 2020

SLES remote support with authentication token #1729

SLES remote support with authentication token #1729

Conversation

pavelpicka commented May 29, 2020

pulpbot commented May 29, 2020

jlsherrill commented Jun 8, 2020

bmbouter commented Jun 9, 2020

jlsherrill commented Jun 9, 2020

bmbouter commented Jun 9, 2020

jlsherrill commented Jun 9, 2020

bmbouter commented Jun 9, 2020

goosemania commented Jun 10, 2020

abr1x commented Jun 15, 2020

pavelpicka commented Jun 15, 2020

bmbouter left a comment

Choose a reason for hiding this comment

dralley left a comment

Choose a reason for hiding this comment

goosemania Jun 15, 2020

Choose a reason for hiding this comment

goosemania Jun 15, 2020

Choose a reason for hiding this comment

pavelpicka Jun 16, 2020

Choose a reason for hiding this comment

goosemania Jun 16, 2020

Choose a reason for hiding this comment

goosemania Jun 16, 2020

Choose a reason for hiding this comment

bmbouter Jun 16, 2020

Choose a reason for hiding this comment