This repository has been archived by the owner on Dec 7, 2022. It is now read-only.
Allow Accept header to send multiple values #391
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hello @johnpmitsch! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2019-08-06 15:35:13 UTC |
|
Can one of the admins verify this patch? |
1 similar comment
|
Can one of the admins verify this patch? |
johnpmitsch
force-pushed
the
registry_fix
branch
from
ffd3bec to
e422f22
Compare
ipanova
reviewed
Aug 6, 2019
pulp_docker/app/registry.py
Outdated
| if header == b'Accept': | ||
| accepted_media_types.append(value.decode('UTF-8')) | ||
| values = [v.strip().decode('UTF-8') for v in values.split(b", ")] |
There was a problem hiding this comment.
please remove the space after comma split(',')
I think normally per HTTP spec when providing multiple values in the header,each value is delimited by a comma, not sure about the presence of the space.
There was a problem hiding this comment.
In [15]: x='multiple, types'
In [16]: y= x.split(', ')
In [17]: y[0].strip()
Out[17]: 'multiple'
In [18]: x='multiple,types'
In [19]: z= x.split(', ')
In [20]: z[0].strip()
Out[20]: 'multiple,types'
In [21]:
There was a problem hiding this comment.
tested and it works!
http HEAD 'http://localhost:24816/v2/foo/manifests/musl' 'Accept:application/vnd.docker.distribution.manifest.list.v2+json, application/vnd.docker.distribution.manifest.v2+json'
HTTP/1.1 200 OK
Accept-Ranges: bytes
Content-Disposition: attachment; filename=dbab3ccf5ebe2d1c79131966766ff2156df89ed538e0c8fb9a1f087b503a65
Content-Length: 1638
Content-Type: application/vnd.docker.distribution.manifest.list.v2+json
Date: Tue, 06 Aug 2019 15:22:43 GMT
Docker-Content-Digest: sha256:2edbab3ccf5ebe2d1c79131966766ff2156df89ed538e0c8fb9a1f087b503a65
Docker-Distribution-API-Version: registry/2.0
Last-Modified: Tue, 06 Aug 2019 14:55:50 GMT
Server: Python/3.7 aiohttp/3.5.4
(pulp) [vagrant@pulp3-source-fedora29 pulp_docker]$ http HEAD 'http://localhost:24816/v2/foo/manifests/musl' 'Accept:application/vnd.docker.distribution.manifest.list.v2+json,application/vnd.docker.distribution.manifest.v2+json'
HTTP/1.1 200 OK
Accept-Ranges: bytes
Content-Disposition: attachment; filename=dbab3ccf5ebe2d1c79131966766ff2156df89ed538e0c8fb9a1f087b503a65
Content-Length: 1638
Content-Type: application/vnd.docker.distribution.manifest.list.v2+json
Date: Tue, 06 Aug 2019 15:22:50 GMT
Docker-Content-Digest: sha256:2edbab3ccf5ebe2d1c79131966766ff2156df89ed538e0c8fb9a1f087b503a65
Docker-Distribution-API-Version: registry/2.0
Last-Modified: Tue, 06 Aug 2019 14:55:50 GMT
Server: Python/3.7 aiohttp/3.5.4
(pulp) [vagrant@pulp3-source-fedora29 pulp_docker]$
closes #5211 https://pulp.plan.io/issues/5211 This will allow the Accept header to use multiple values. I tested this out with pulling directly with docker, which seems to send one value, and Katello, where we send multiple values. Both scenarios were able to pull successfully. This will make it easy for Katello to substitute pulp3 registry url instead of crane where it is supported.
johnpmitsch
force-pushed
the
registry_fix
branch
from
e422f22 to
f553dd7
Compare
ipanova
approved these changes
Aug 6, 2019
johnpmitsch
pushed a commit
to johnpmitsch/katello
that referenced
this pull request
Aug 7, 2019
This supports docker pull with pulp3. Crane (pulp2) is still supported and the
registry URL is dynamically determined based on the support for Docker blobs.
Previously, the RegistryResource class would run code when loaded, setting up
the inherited class variables. This commit moves this and the new logic to a class
method. Since there are now backend calls (to check for pulp3) to set up the class
variables, it seems like this should be a more deliberate action. This also makes testing
much easier.
I'm open to other suggestions on how to handle the class inheritance and the class variables.
To test:
- Modify foreman/config/settings.plugins.d/katello.yaml, adding the pulp_registry_url
```yaml
:container_image_registry:
:crane_url: https://localhost:5000
:pulp_registry_url: http://localhost:24816
:registry_ca_cert_file: /etc/pki/katello/certs/katello-default-ca.crt
```
- Set up docker on pulp3 box. You will need an up-to-date box since pulp/pulp_docker#391 was merged
- Sync repos, you can use https://cloud.docker.com/u/jomitsch/repository/docker/jomitsch/workflow-test for a small one.
- docker login $HOSTNAME
- docker pull $HOSTNAME/default_organization-docker-workflow_test (check name in docker repo details)
- docker pull should be successful
It's worth trying out some other repos or registries (like quay.io) and also we should ensure pulp2 functionality is not expected
There will be installer changes to come that will update both dev and prod's katello.yml, these will need to be merged along
with this PR
I also updated the rubocop_todo as it was throwing warnings for duplicate entries.
johnpmitsch
pushed a commit
to johnpmitsch/katello
that referenced
this pull request
Aug 7, 2019
This supports docker pull with pulp3. Crane (pulp2) is still supported and the
registry URL is dynamically determined based on the support for Docker blobs.
Previously, the RegistryResource class would run code when loaded, setting up
the inherited class variables. This commit moves this and the new logic to a class
method. Since there are now backend calls (to check for pulp3) to set up the class
variables, it seems like this should be a more deliberate action. This also makes testing
much easier.
I'm open to other suggestions on how to handle the class inheritance and the class variables.
To test:
- Modify foreman/config/settings.plugins.d/katello.yaml, adding the pulp_registry_url
```yaml
:container_image_registry:
:crane_url: https://localhost:5000
:pulp_registry_url: http://localhost:24816
:registry_ca_cert_file: /etc/pki/katello/certs/katello-default-ca.crt
```
- Set up docker on pulp3 box. You will need an up-to-date box since pulp/pulp_docker#391 was merged
- Sync repos, you can use https://cloud.docker.com/u/jomitsch/repository/docker/jomitsch/workflow-test for a small one.
- docker login $HOSTNAME
- docker pull $HOSTNAME/default_organization-docker-workflow_test (check name in docker repo details)
- docker pull should be successful
It's worth trying out some other repos or registries (like quay.io) and also we should ensure pulp2 functionality is not expected
There will be installer changes to come that will update both dev and prod's katello.yml, these will need to be merged along
with this PR
I also updated the rubocop_todo as it was throwing warnings for duplicate entries.
johnpmitsch
pushed a commit
to johnpmitsch/katello
that referenced
this pull request
Aug 7, 2019
This supports docker pull with pulp3. Crane (pulp2) is still supported and the
registry URL is dynamically determined based on the support for Docker blobs.
Previously, the RegistryResource class would run code when loaded, setting up
the inherited class variables. This commit moves this and the new logic to a class
method. Since there are now backend calls (to check for pulp3) to set up the class
variables, it seems like this should be a more deliberate action. This also makes testing
much easier.
I'm open to other suggestions on how to handle the class inheritance and the class variables.
To test:
- Modify foreman/config/settings.plugins.d/katello.yaml, adding the pulp_registry_url
```yaml
:container_image_registry:
:crane_url: https://localhost:5000
:pulp_registry_url: http://localhost:24816
:registry_ca_cert_file: /etc/pki/katello/certs/katello-default-ca.crt
```
- Set up docker on pulp3 box. You will need an up-to-date box since pulp/pulp_docker#391 was merged
- Sync repos, you can use https://cloud.docker.com/u/jomitsch/repository/docker/jomitsch/workflow-test for a small one.
- docker login $HOSTNAME
- docker pull $HOSTNAME/default_organization-docker-workflow_test (check name in docker repo details)
- docker pull should be successful
It's worth trying out some other repos or registries (like quay.io) and also we should ensure pulp2 functionality is not expected
There will be installer changes to come that will update both dev and prod's katello.yml, these will need to be merged along
with this PR
I also updated the rubocop_todo as it was throwing warnings for duplicate entries.
johnpmitsch
pushed a commit
to johnpmitsch/katello
that referenced
this pull request
Aug 7, 2019
This supports docker pull with pulp3. Crane (pulp2) is still supported and the
registry URL is dynamically determined based on the support for Docker blobs.
Previously, the RegistryResource class would run code when loaded, setting up
the inherited class variables. This commit moves this and the new logic to a class
method. Since there are now backend calls (to check for pulp3) to set up the class
variables, it seems like this should be a more deliberate action. This also makes testing
much easier.
I'm open to other suggestions on how to handle the class inheritance and the class variables.
To test:
- Modify foreman/config/settings.plugins.d/katello.yaml, adding the pulp_registry_url
```yaml
:container_image_registry:
:crane_url: https://localhost:5000
:pulp_registry_url: http://localhost:24816
:registry_ca_cert_file: /etc/pki/katello/certs/katello-default-ca.crt
```
- Set up docker on pulp3 box. You will need an up-to-date box since pulp/pulp_docker#391 was merged
- Sync repos, you can use https://cloud.docker.com/u/jomitsch/repository/docker/jomitsch/workflow-test for a small one.
- docker login $HOSTNAME
- docker pull $HOSTNAME/default_organization-docker-workflow_test (check name in docker repo details)
- docker pull should be successful
It's worth trying out some other repos or registries (like quay.io) and also we should ensure pulp2 functionality is not expected
There will be installer changes to come that will update both dev and prod's katello.yml, these will need to be merged along
with this PR
I also updated the rubocop_todo as it was throwing warnings for duplicate entries.
johnpmitsch
pushed a commit
to johnpmitsch/katello
that referenced
this pull request
Aug 23, 2019
This supports docker pull with pulp3. Crane (pulp2) is still supported and the
registry URL is dynamically determined based on the support for Docker blobs.
Previously, the RegistryResource class would run code when loaded, setting up
the inherited class variables. This commit moves this and the new logic to a class
method. Since there are now backend calls (to check for pulp3) to set up the class
variables, it seems like this should be a more deliberate action. This also makes testing
much easier.
I'm open to other suggestions on how to handle the class inheritance and the class variables.
To test:
- Modify foreman/config/settings.plugins.d/katello.yaml, adding the pulp_registry_url
```yaml
:container_image_registry:
:crane_url: https://localhost:5000
:pulp_registry_url: http://localhost:24816
:registry_ca_cert_file: /etc/pki/katello/certs/katello-default-ca.crt
```
- Set up docker on pulp3 box. You will need an up-to-date box since pulp/pulp_docker#391 was merged
- Sync repos, you can use https://cloud.docker.com/u/jomitsch/repository/docker/jomitsch/workflow-test for a small one.
- docker login $HOSTNAME
- docker pull $HOSTNAME/default_organization-docker-workflow_test (check name in docker repo details)
- docker pull should be successful
It's worth trying out some other repos or registries (like quay.io) and also we should ensure pulp2 functionality is not expected
There will be installer changes to come that will update both dev and prod's katello.yml, these will need to be merged along
with this PR
I also updated the rubocop_todo as it was throwing warnings for duplicate entries.
johnpmitsch
pushed a commit
to johnpmitsch/katello
that referenced
this pull request
Aug 23, 2019
This supports docker pull with pulp3. Crane (pulp2) is still supported and the
registry URL is dynamically determined based on the support for Docker blobs.
Previously, the RegistryResource class would run code when loaded, setting up
the inherited class variables. This commit moves this and the new logic to a class
method. Since there are now backend calls (to check for pulp3) to set up the class
variables, it seems like this should be a more deliberate action. This also makes testing
much easier.
I'm open to other suggestions on how to handle the class inheritance and the class variables.
To test:
- Modify foreman/config/settings.plugins.d/katello.yaml, adding the pulp_registry_url
```yaml
:container_image_registry:
:crane_url: https://localhost:5000
:pulp_registry_url: http://localhost:24816
:registry_ca_cert_file: /etc/pki/katello/certs/katello-default-ca.crt
```
- Set up docker on pulp3 box. You will need an up-to-date box since pulp/pulp_docker#391 was merged
- Sync repos, you can use https://cloud.docker.com/u/jomitsch/repository/docker/jomitsch/workflow-test for a small one.
- docker login $HOSTNAME
- docker pull $HOSTNAME/default_organization-docker-workflow_test (check name in docker repo details)
- docker pull should be successful
It's worth trying out some other repos or registries (like quay.io) and also we should ensure pulp2 functionality is not expected
There will be installer changes to come that will update both dev and prod's katello.yml, these will need to be merged along
with this PR
I also updated the rubocop_todo as it was throwing warnings for duplicate entries.
johnpmitsch
pushed a commit
to johnpmitsch/katello
that referenced
this pull request
Aug 23, 2019
This supports docker pull with pulp3. Crane (pulp2) is still supported and the
registry URL is dynamically determined based on the support for Docker blobs.
Previously, the RegistryResource class would run code when loaded, setting up
the inherited class variables. This commit moves this and the new logic to a class
method. Since there are now backend calls (to check for pulp3) to set up the class
variables, it seems like this should be a more deliberate action. This also makes testing
much easier.
I'm open to other suggestions on how to handle the class inheritance and the class variables.
To test:
- Modify foreman/config/settings.plugins.d/katello.yaml, adding the pulp_registry_url
```yaml
:container_image_registry:
:crane_url: https://localhost:5000
:pulp_registry_url: http://localhost:24816
:registry_ca_cert_file: /etc/pki/katello/certs/katello-default-ca.crt
```
- Set up docker on pulp3 box. You will need an up-to-date box since pulp/pulp_docker#391 was merged
- Sync repos, you can use https://cloud.docker.com/u/jomitsch/repository/docker/jomitsch/workflow-test for a small one.
- docker login $HOSTNAME
- docker pull $HOSTNAME/default_organization-docker-workflow_test (check name in docker repo details)
- docker pull should be successful
It's worth trying out some other repos or registries (like quay.io) and also we should ensure pulp2 functionality is not expected
There will be installer changes to come that will update both dev and prod's katello.yml, these will need to be merged along
with this PR
I also updated the rubocop_todo as it was throwing warnings for duplicate entries.
johnpmitsch
pushed a commit
to johnpmitsch/katello
that referenced
this pull request
Aug 23, 2019
This supports docker pull with pulp3. Crane (pulp2) is still supported and the
registry URL is dynamically determined based on the support for Docker blobs.
Previously, the RegistryResource class would run code when loaded, setting up
the inherited class variables. This commit moves this and the new logic to a class
method. Since there are now backend calls (to check for pulp3) to set up the class
variables, it seems like this should be a more deliberate action. This also makes testing
much easier.
I'm open to other suggestions on how to handle the class inheritance and the class variables.
To test:
- Modify foreman/config/settings.plugins.d/katello.yaml, adding the pulp_registry_url
```yaml
:container_image_registry:
:crane_url: https://localhost:5000
:pulp_registry_url: http://localhost:24816
:registry_ca_cert_file: /etc/pki/katello/certs/katello-default-ca.crt
```
- Set up docker on pulp3 box. You will need an up-to-date box since pulp/pulp_docker#391 was merged
- Sync repos, you can use https://cloud.docker.com/u/jomitsch/repository/docker/jomitsch/workflow-test for a small one.
- docker login $HOSTNAME
- docker pull $HOSTNAME/default_organization-docker-workflow_test (check name in docker repo details)
- docker pull should be successful
It's worth trying out some other repos or registries (like quay.io) and also we should ensure pulp2 functionality is not expected
There will be installer changes to come that will update both dev and prod's katello.yml, these will need to be merged along
with this PR
I also updated the rubocop_todo as it was throwing warnings for duplicate entries.
johnpmitsch
pushed a commit
to johnpmitsch/katello
that referenced
this pull request
Aug 23, 2019
This supports docker pull with pulp3. Crane (pulp2) is still supported and the
registry URL is dynamically determined based on the support for Docker blobs.
Previously, the RegistryResource class would run code when loaded, setting up
the inherited class variables. This commit moves this and the new logic to a class
method. Since there are now backend calls (to check for pulp3) to set up the class
variables, it seems like this should be a more deliberate action. This also makes testing
much easier.
I'm open to other suggestions on how to handle the class inheritance and the class variables.
To test:
- Modify foreman/config/settings.plugins.d/katello.yaml, adding the pulp_registry_url
```yaml
:container_image_registry:
:crane_url: https://localhost:5000
:pulp_registry_url: http://localhost:24816
:registry_ca_cert_file: /etc/pki/katello/certs/katello-default-ca.crt
```
- Set up docker on pulp3 box. You will need an up-to-date box since pulp/pulp_docker#391 was merged
- Sync repos, you can use https://cloud.docker.com/u/jomitsch/repository/docker/jomitsch/workflow-test for a small one.
- docker login $HOSTNAME
- docker pull $HOSTNAME/default_organization-docker-workflow_test (check name in docker repo details)
- docker pull should be successful
It's worth trying out some other repos or registries (like quay.io) and also we should ensure pulp2 functionality is not expected
There will be installer changes to come that will update both dev and prod's katello.yml, these will need to be merged along
with this PR
I also updated the rubocop_todo as it was throwing warnings for duplicate entries.
johnpmitsch
pushed a commit
to Katello/katello
that referenced
this pull request
Aug 26, 2019
This supports docker pull with pulp3. Crane (pulp2) is still supported and the
registry URL is dynamically determined based on the support for Docker blobs.
Previously, the RegistryResource class would run code when loaded, setting up
the inherited class variables. This commit moves this and the new logic to a class
method. Since there are now backend calls (to check for pulp3) to set up the class
variables, it seems like this should be a more deliberate action. This also makes testing
much easier.
I'm open to other suggestions on how to handle the class inheritance and the class variables.
To test:
- Modify foreman/config/settings.plugins.d/katello.yaml, adding the pulp_registry_url
```yaml
:container_image_registry:
:crane_url: https://localhost:5000
:pulp_registry_url: http://localhost:24816
:registry_ca_cert_file: /etc/pki/katello/certs/katello-default-ca.crt
```
- Set up docker on pulp3 box. You will need an up-to-date box since pulp/pulp_docker#391 was merged
- Sync repos, you can use https://cloud.docker.com/u/jomitsch/repository/docker/jomitsch/workflow-test for a small one.
- docker login $HOSTNAME
- docker pull $HOSTNAME/default_organization-docker-workflow_test (check name in docker repo details)
- docker pull should be successful
It's worth trying out some other repos or registries (like quay.io) and also we should ensure pulp2 functionality is not expected
There will be installer changes to come that will update both dev and prod's katello.yml, these will need to be merged along
with this PR
I also updated the rubocop_todo as it was throwing warnings for duplicate entries.
|
I am trying to pull docker image from foreman but showing error , although all images are showing on foreman UI with tags docker pull fooreman2.test.com:5000/test-airo-ko:v6 Error response from daemon: unknown: Not Found Should I need to install some plugin ? I just need foreman to handle all my images and tags. ################################### 10.32.7.210 - - [19/Oct/2020:12:59:38 -0400] “GET /v2/ HTTP/1.1” 200 2 “-” “docker/19.03.12 go/go1.13.10 git-commit/48a66213fe kernel/3.10.0-1127.18.2.el7.x86_64 os/linux arch/amd64 UpstreamClient(Docker-Client/19.03.12 (linux))” 10.32.7.210 - - [19/Oct/2020:12:59:38 -0400] “GET /v2/abc-production-airo-airo-keycloak/manifests/latest HTTP/1.1” 404 53 “-” “docker/19.03.12 go/go1.13.10 git-commit/48a66213fe kernel/3.10.0-1127.18.2.el7.x86_64 os/linux arch/amd64 UpstreamClient(Docker-Client/19.03.12 (linux))” ####################### 10.32.9.206 - - [19/Oct/2020:13:35:28 -0400] “GET /notification_recipients HTTP/1.1” 200 40 “https://host2.xyz.com/content_views/3/versions/4/docker?page=1&per_page=20” “Mozilla/5.0 (Windows NT 6.3; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/86.0.4240.75 Safari/537.36” 10.32.9.206 - - [19/Oct/2020:13:35:38 -0400] “GET /notification_recipients HTTP/1.1” 200 40 “https://host2.xyz.com/content_views/3/versions/4/docker?page=1&per_page=20” “Mozilla/5.0 (Windows NT 6.3; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/86.0.4240.75 Safari/537.36” 10.32.9.206 - - [19/Oct/2020:13:35:48 -0400] “GET /notification_recipients HTTP/1.1” 200 40 “https://fountain2.xyz.com/content_views/3/versions/4/docker?page=1&per_page=20” “Mozilla/5.0 (Windows NT 6.3; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/86.0.4240.75 Safari/537.36” ==> /var/log/httpd/error_log <== [Mon Oct 19 17:35:56.990633 2020] [:error] [pid 22824] [INFO] crane.data: loading metadata from /var/lib/pulp/published/docker/v2/app [Mon Oct 19 17:35:56.991311 2020] [:error] [pid 22824] [INFO] crane.data: finished loading metadata ##################### |
|
hey @masadrasheed, how about you open up a support thread on https://community.theforeman.org/ and we can discuss further? |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #5211
https://pulp.plan.io/issues/5211
This will allow the Accept header to use multiple values.
I tested this out with pulling directly with docker, which seems to send one value,
and Katello, where we send multiple values. Both scenarios were able to pull successfully.
This will make it easy for Katello to substitute pulp3 registry url instead of crane where it is supported.