Fixes #37302 - push containers through Katello #10952

ianballou · 2024-04-01T22:09:25Z

What are the changes introduced in this pull request?

Adds support for pushing content through Katello to Pulp. Does not index any information from Pulp. As such, only pushing is possible through Katello.

Considerations taken when implementing this change?

The REMOTE_USER header must be set to perform remote auth with the Pulpcore registry. Apache will set this rather than Katello, so it's left out here.

What are the testing steps for this pull request?

Cherry-pick @sjha4 's changes here before pulling in this PR and upgrade to Pulpcore nightly: Pulpcore 3.49 bindings theforeman/foreman-packaging#10658

Pulpcore Nightly repo: https://stagingyum.theforeman.org/pulpcore/nightly/

Due to RegistryAuthentication fails to fall back to PulpRemoteUserAuthentication if no authorization header exists pulp/pulp_container#1577, hacking is required for auth with Pulp to work. In /usr/lib/python3.11/site-packages/pulp_container/app/token_verification.py, add return RemoteUserRegistryAuthentication().authenticate(request) to the top of RegistryAuthentication.authenticate.

Don't forget to restart Pulpcore after making the changes.

Apache must also be configured to send the REMOTE_USER header over to Pulp. Edit /etc/httpd/conf.d/05-foreman-ssl.conf so that the /pulpcore_registry/v2/ location loses RequestHeader set REMOTE_USER "%{SSL_CLIENT_S_DN_CN}s" env=SSL_CLIENT_S_DN_CN and gains RequestHeader set REMOTE_USER "admin".

Fixes #37308 - set REMOTE_USER properly for pulpcore registry theforeman/puppet-pulpcore#337

Add the following to foreman/config/settings.plugins.d/katello.yaml:

# The katello section should exist already.
:katello:
  :container_image_registry:
    :allow_push: true

Pull some image to your system using podman pull so there is something to push.
Push to Katello:

# Example -- 76435261c6d3 must point to an image on your machine. Find it with 'podman image ls' after pulling.
podman push 76435261c6d3 centos8-katello-devel.manicotto.example.com/organization/product/quarkus-micro-image

Check that a container push repo was created in Pulp:

pulp container repository -t push list

Make sure that actual content was uploaded.

Try a few images. Try some big ones!

qcjames53 · 2024-04-04T20:53:37Z

Hi Ian, thanks for the earlier fix. It looks like small/medium images are pushed properly but larger images run into issues. I have been seeing 502 bad gateway errors on pushes for images like this one (407 MB). Rails console output shows the following:

HTTP parse error, malformed request ("PATCH /v2/organization/product/keycloak/blobs/uploads/018eaadd-7f75-7ae8-958c-5db6a1ea456c" - (192.168.121.94)): #<Puma::HttpParserError: maximum size of chunk header exceeded>

qcjames53

A few comments...

qcjames53 · 2024-04-04T20:56:15Z

app/controllers/katello/api/registry/registry_proxies_controller.rb

@@ -9,7 +9,9 @@ class Api::Registry::RegistryProxiesController < Api::V2::ApiController
    before_action :optional_authorize, only: [:token, :catalog]
    before_action :registry_authorize, except: [:token, :v1_search, :catalog]
    before_action :authorize_repository_read, only: [:pull_manifest, :tags_list]
-    before_action :authorize_repository_write, only: [:push_manifest]
+    # authorize_repository_write commented out until Katello indexes Pulp container push repositories.


I know some of our automation looks for "TODO" and "FIXME" flags in comments. Perhaps this would be a good spot to indicate something.

qcjames53 · 2024-04-04T21:10:11Z

config/routes/api/registry.rb

@@ -11,15 +11,15 @@ class ActionDispatch::Routing::Mapper
      match '/v2/token' => 'registry_proxies#token', :via => :post
      match '/v2/*repository/manifests/:tag' => 'registry_proxies#pull_manifest', :via => :get
      # Push-related routes are disabled until there is support for pushing to Pulp 3.


I believe this comment should be removed.

qcjames53 · 2024-04-04T21:16:49Z

app/controllers/katello/api/registry/registry_proxies_controller.rb

@@ -182,15 +184,8 @@ def pull_manifest
    end

    def check_blob


I have a feeling the image size issue is something with this method. We were setting a content length value in the response header that doesn't seem to exist anymore? Qualifier on this advice: I may not know what I'm talking about.

Our code actually errors out before we even reach this method, the issue is between podman and puma rather than our code. The MAX_CHUNK_HEADER_SIZE is so small that puma rejects the request that podman sends to us. I'm going to report an issue to puma to see what they say.

puma/puma#3362

Good to know, thanks! Interesting issue. Are you wanting to keep this on the back burner until they get back to you or push this through under the pretense larger image sizes should be fine in the future?

Looks like Puma already has a fix for this: puma/puma#3338

I think as such we can add that to the list of things that need fixing for container push to be fully functional and press forward.

Feel free to patch in that fix, it's a very small change.

Awesome, thanks!

ianballou · 2024-04-05T20:02:09Z

Reminder for myself to see about adding / modifying tests.

ianballou · 2024-04-08T17:17:42Z

[test katello]

sjha4 · 2024-04-09T02:48:01Z

app/controllers/katello/api/registry/registry_proxies_controller.rb

+        key.match("^HTTP_.*")
+      end
+      env.each do |header|
+        current_headers[header[0].split('_')[1..-1].join('-')] = header[1]


@ianballou : Could you explain why this is needed?

This is one bit of the code that was created a while back. I believe it's here because request.env contains environment variables, and some of which translate to headers. The headers as env vars have underscores and are prefixed with "HTTP_". To turn them into true headers, they need the underscores turned into hyphens also the HTTP prefix needs removing.

I need to read up more on it to verify, but one example I saw was that our apache must set the Remote-User header, which eventually reaches Pulp/Django as "HTTP_REMOTE_USER".

I was trying to remember why start_upload_blob doesn't use translated_headers_for_proxy, and remembered that uploading a blob doesn't require special headers, unlike actually uploading the blob. For example: https://specs.opencontainers.org/distribution-spec/?v=v1.0.0#DISTRIBUTION-SPEC-93

Some more helpful material about the web server environment variables https://medium.com/@cipeinado/unlock-http-info-in-rails-with-rack-request-env-and-uri-c7835397dc14
It clarifies my thinking a bit about how the HTTP_ variables relate to headers.

Another thing that's interesting -- so request.headers doesn't list anything by default. You need to look up a specific header to get info. If you type request.headers.to_a, for example, you end up just getting the normal request.env information more or less. It would need to be verified, but I think request.headers['key'] looks for headers by various naming conventions. For example:

[13] pry(#<Katello::Api::Registry::RegistryProxiesController>)> request.headers => #<ActionDispatch::Http::Headers:0x000055afdecc0450 @req= #<ActionDispatch::Request POST "https://centos8-katello-devel.manicotto.example.com/v2/organization/product/hello-world/blobs/uploads/" for 192.168.122.147>> [14] pry(#<Katello::Api::Registry::RegistryProxiesController>)> request.headers['Authorization'] => "Bearer $2a$09$1b6453892473a467d0737uL4crR75S3rqz44qQcdMGK68gYLHX/Ba" [15] pry(#<Katello::Api::Registry::RegistryProxiesController>)> request.headers['AUTHORIZATION'] => "Bearer $2a$09$1b6453892473a467d0737uL4crR75S3rqz44qQcdMGK68gYLHX/Ba" [16] pry(#<Katello::Api::Registry::RegistryProxiesController>)> request.headers['HTTP_AUTHORIZATION'] => "Bearer $2a$09$1b6453892473a467d0737uL4crR75S3rqz44qQcdMGK68gYLHX/Ba" [17] pry(#<Katello::Api::Registry::RegistryProxiesController>)> request.headers['HTT_AUTHORIZATION'] => nil [18] pry(#<Katello::Api::Registry::RegistryProxiesController>)> request.headers['HTTP_AUTHORIZATION'] => "Bearer $2a$09$1b6453892473a467d0737uL4crR75S3rqz44qQcdMGK68gYLHX/Ba" [19] pry(#<Katello::Api::Registry::RegistryProxiesController>)> request.headers['x-authorization'] => nil [20] pry(#<Katello::Api::Registry::RegistryProxiesController>)> request.headers['X-Authorization'] => nil

I did realize that some headers, like the Content-Length, don't make it in with the HTTP_ env vars. So I'll add explicit checks for those.

ianballou · 2024-04-09T16:26:04Z

@qcjames53 my PR is ready for another round of reviews.

qcjames53

Hi Ian. I was able to test this PR again with a large image (I used keycloak again, 461MB). Using the puma patch you mentioned, I'm unfortunately still seeing 502 bad gateway errors. Still the same puma error as well: #<Puma::HttpParserError: maximum size of chunk header exceeded>. I'm hoping I'm doing something incorrect with this testing; please let me know if this should be working on my end.

Thanks for adding test cases for the new functionality; good addition. I was unable to get them to run locally but it looks like the GH automation was happy. As of now I don't think we need a test for large images since you are mocking the pulp responses anyways.

Thanks for the other small formatting/comment changes. Outside of the one comment below, everything else looks pretty good to me.

qcjames53 · 2024-04-10T01:56:05Z

app/controllers/katello/api/registry/registry_proxies_controller.rb

-      File.open(tmp_file("#{params[:uuid]}.tar"), 'ab', 0600) do |file|
-        file.write request.body.read
+      headers = translated_headers_for_proxy
+      headers['Content-Type'] = request.headers['Content-Type'] if request.headers['Content-Type']


It really feels like there should be a more concise way to do all these calls but neither ||= or &&= works. Perhaps you could split all three of these test blocks into a method? I don't think fancier syntax is necessarily required; this is very easy to understand as-is. It just bugs me at the kernel level.

Lemme take a look about if there's a less verbose way to do this that doesn't lose readability.

Based on our video call I'm happy to call this ok. It's very readable and quick as you wrote it.

I didn't find a less verbose way to write this, that check for nil seems like a requirement. C'mon Ruby!

qcjames53 · 2024-04-10T14:05:02Z

Can confirm that the new puma patch works to allow pushes for larger images. Thank you for the help with that config, Ian.

qcjames53

ACK

pr-processor bot added the Not yet reviewed label Apr 1, 2024

ianballou force-pushed the 37302-container-push-testing branch 2 times, most recently from eb8c7f3 to a94868d Compare April 4, 2024 15:05

qcjames53 requested changes Apr 4, 2024

View reviewed changes

pr-processor bot added Waiting on contributor and removed Not yet reviewed labels Apr 4, 2024

ianballou force-pushed the 37302-container-push-testing branch from a94868d to b01de38 Compare April 5, 2024 20:01

pr-processor bot added Needs re-review and removed Waiting on contributor labels Apr 5, 2024

github-actions bot added the Packaging Change label Apr 5, 2024

ianballou force-pushed the 37302-container-push-testing branch from b01de38 to b726cd4 Compare April 5, 2024 20:05

github-actions bot removed the Packaging Change label Apr 5, 2024

ianballou force-pushed the 37302-container-push-testing branch from b726cd4 to 2649255 Compare April 8, 2024 21:39

sjha4 reviewed Apr 9, 2024

View reviewed changes

Fixes #37302 - push containers through Katello

d4c16dc

ianballou force-pushed the 37302-container-push-testing branch from 2649255 to d4c16dc Compare April 9, 2024 16:25

ianballou requested a review from qcjames53 April 9, 2024 16:25

qcjames53 requested changes Apr 10, 2024

View reviewed changes

pr-processor bot added Waiting on contributor and removed Needs re-review labels Apr 10, 2024

qcjames53 approved these changes Apr 10, 2024

View reviewed changes

ianballou merged commit ac91651 into Katello:master Apr 10, 2024
21 of 22 checks passed

ianballou deleted the 37302-container-push-testing branch April 10, 2024 16:05

evgeni mentioned this pull request Sep 4, 2024

add test for container push on Katello 4.14+ theforeman/forklift#1853

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #37302 - push containers through Katello #10952

Fixes #37302 - push containers through Katello #10952

ianballou commented Apr 1, 2024 •

edited

Loading

qcjames53 commented Apr 4, 2024

qcjames53 left a comment

qcjames53 Apr 4, 2024

qcjames53 Apr 4, 2024

qcjames53 Apr 4, 2024

ianballou Apr 4, 2024

ianballou Apr 4, 2024

qcjames53 Apr 4, 2024

ianballou Apr 5, 2024

ianballou Apr 5, 2024

qcjames53 Apr 5, 2024

ianballou commented Apr 5, 2024

ianballou commented Apr 8, 2024

sjha4 Apr 9, 2024

ianballou Apr 9, 2024

ianballou Apr 9, 2024

ianballou Apr 9, 2024 •

edited

Loading

ianballou Apr 9, 2024

ianballou Apr 9, 2024 •

edited

Loading

ianballou commented Apr 9, 2024

qcjames53 left a comment

qcjames53 Apr 10, 2024 •

edited

Loading

ianballou Apr 10, 2024

qcjames53 Apr 10, 2024

ianballou Apr 10, 2024

qcjames53 commented Apr 10, 2024

qcjames53 left a comment

Fixes #37302 - push containers through Katello #10952

Fixes #37302 - push containers through Katello #10952

Conversation

ianballou commented Apr 1, 2024 • edited Loading

What are the changes introduced in this pull request?

Considerations taken when implementing this change?

What are the testing steps for this pull request?

qcjames53 commented Apr 4, 2024

qcjames53 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianballou commented Apr 5, 2024

ianballou commented Apr 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianballou Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianballou Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

ianballou commented Apr 9, 2024

qcjames53 left a comment

Choose a reason for hiding this comment

qcjames53 Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qcjames53 commented Apr 10, 2024

qcjames53 left a comment

Choose a reason for hiding this comment

ianballou commented Apr 1, 2024 •

edited

Loading

ianballou Apr 9, 2024 •

edited

Loading

ianballou Apr 9, 2024 •

edited

Loading

qcjames53 Apr 10, 2024 •

edited

Loading