fixes #24070 - add registry to katello.yaml #243

thomasmckay · 2018-05-21T17:54:32Z

theforeman/puppet-katello_devel#168
#243
Katello/katello#7543

thomasmckay · 2018-05-22T00:29:55Z

I don't like that the server errors out if there is something in the settings conf that is unrecognized, is that purposeful?

Also, will upgrades get the new stanza I've added or is there something else I need to do for that?

ekohl

I don't like that the server errors out if there is something in the settings conf that is unrecognized, is that purposeful?

Yes, we explicitly test for the full config so we don't miss anything or there's extra stuff in there. Please add it to the test.

Also, will upgrades get the new stanza I've added or is there something else I need to do for that?

Puppet will ensure the content is there after running so there should be no difference between installs and upgrades. It just ensures the end state.

ehelms · 2018-06-04T16:33:28Z

templates/katello.yaml.erb

+    :url: <%= @crane_url %>
+    :ca_cert_file: <%= @crane_ca_cert %>
+
+


Nitpick - extra line here.

ehelms · 2018-06-04T16:34:28Z

manifests/application.pp

@@ -15,6 +15,7 @@
  String $candlepin_oauth_key = $::katello::candlepin_oauth_key,
  String $candlepin_oauth_secret = $::katello::candlepin_oauth_secret,
  Stdlib::Httpsurl $pulp_url = $::katello::pulp_url,
+  Stdlib::Httpsurl $crane_url = $::katello::crane_url,


The settings file generically referes to things as a regristry but this is specific to crane which is a bit of disconnect to me.

ehelms · 2018-06-04T16:35:22Z

@thomasmckay SETTINGS is just a generic hash nothing special about it hence why it errors if something is missing. You'd need to write your code to handle the case where its not present or configured which is a good model to follow.

thomasmckay · 2018-06-04T16:49:48Z

I thought about making it a crane section except I have plans to avoid crane completely at some point and also will add some registry-specific settings unrelated to crane. Perhaps I should change the settings themselves to reflect their related to crane.

Another question, should I just be using existing settings available in katello for crane comms? Maybe they are not needed at all.

@parthaa @jlsherrill - What do you think?

ehelms · 2018-06-05T13:43:30Z

You could still make this generic to registry unless you think that will cause confusion.

ehelms · 2018-06-05T15:12:10Z

BTW, this is what currently breaks nightly so arriving at a solution soon would be ideal.

ekohl · 2018-06-05T15:20:57Z

I think the parameter should match the one in the config so registry_url would be best. I'm not a big fan of the generic registry though. container_registry would be better.

ekohl · 2018-06-22T18:02:09Z

Ping?

thomasmckay · 2018-06-25T20:40:46Z

@ekohl @ehelms - Updated PR and opened new issue to align with changes needed in katello

thomasmckay · 2018-06-26T00:36:02Z

Updated. Pointers on tests?

thomasmckay · 2018-06-29T15:24:58Z

@ekohl @ehelms
I changed the stanza to

:container_image_registry:
  :crane_url: https://foo.example.com:5000
  :crane_ca_cert_file: /etc/pki/katello/certs/katello-server-ca.crt

If you approve of the naming, let me know here and I'll open a matching PR for katello (I'll open one today and link here).

In terms of commit comment, should I make the katello one say "fixes #" and this one say "refs #" or some other combination?

stbenjam · 2018-06-29T15:45:03Z

Please don't forget about puppet-katello_devel, which will need similar changes.

ekohl

Will this always be configured? I think it was an optional configuration.

ekohl · 2018-06-29T15:49:26Z

templates/katello.yaml.erb

@@ -31,6 +31,10 @@
    :url: <%= @qpid_url %>
    :subscriptions_queue_address: <%= @candlepin_event_queue %>

+  :container_image_registry:
+    :crane_url: <%= @crane_url %>


Why are these prefixed with crane_? I'd expect them to be url and ca_cert_file (similar to others above).

These are the configs that katello uses to find the crane registry. These are not configs that are exposed to the users of the registry.

Well, they are exposed through this module.

Also it doesn't answer why this is pinned to crane. If this option only supports crane and not a generic registry then I'd expect that the section is named crane. The alternative is that it's generic and any registry is supported. In both cases it doesn't make sense to me to prefix options with crane_.

This section is to config the aspects of the newly implemented registry endpoint. As part of the implementation, katello needs to call pulp's crane; that's what these two configs are for. Removing the crane prefix would then not convey what the url is meant to point at.

If preferred, I could add another entry to indicate the type of backend registry it is pointing at and then remove the crane prefix from the existing two entries. Please suggest a name for this new entry.

It would help me to see how this section and its values are used. The PR mentioned in the opening message is merged but since then I think it has changed. Could you describe how it's used or link to the current code?

Usage:
Don't expose registry if settings absent
https://github.com/Katello/katello/blob/master/app/controllers/katello/api/registry/registry_proxies_controller.rb#L410-L415

Pull objects from crane
https://github.com/Katello/katello/blob/master/app/lib/katello/resources/registry.rb#L22-L27

ekohl · 2018-06-29T15:50:32Z

templates/katello.yaml.erb

@@ -31,6 +31,10 @@
    :url: <%= @qpid_url %>
    :subscriptions_queue_address: <%= @candlepin_event_queue %>

+  :container_image_registry:


I'm thinking about container image registry vs container registry but I'm not sure which would be better.

I prefer as is but either is fine with me. We call the resource "container image" throughout the UI now (as we've tried to downplay the term docker).

ekohl · 2018-06-29T15:51:47Z

manifests/application.pp

@@ -34,6 +35,7 @@
  $post_sync_url = "${::foreman::foreman_url}${deployment_url}/api/v2/repositories/sync_complete?token=${post_sync_token}"
  $candlepin_ca_cert = $::certs::ca_cert
  $pulp_ca_cert = $::certs::katello_server_ca_cert
+  $crane_ca_cert = $::certs::katello_server_ca_cert


This can probably also be a Stdlib::Absolutepath. I also think this should be kept together with the URL setting since they're related. We should have done the same with the other settings but that can be a separate PR.

@ekohl I don't understand what is meant to change here.

Ignore this one, I was mistakenly thinking that it was a class parameter.

thomasmckay · 2018-07-19T20:43:33Z

I've edited the PR description to link all the relevant PRs. If there is more to do, please let me know. Thanks for your patience!

thomasmckay · 2018-07-30T13:07:58Z

Is the following acceptable?

--- katello.yaml.example
 
  # Internal configuration for communication from server to pulp crane service.
  :container_image_registry:
    :backend_registry: crane  # must be crane
    :crane_url: https://localhost:5000
    :crane_ca_cert_file: /etc/pki/katello/certs/katello-server-ca.crt


--- puppet
    
  :container_image_registry:
    :backend_registry: crane  # must be crane
    :crane_url: <%= @crane_url %>
    :crane_ca_cert_file: <%= @crane_ca_cert %>

ekohl · 2018-07-31T12:51:53Z

If backend_registry is not yet read by katello you can leave it out. It's just good to know where it's future proof and we don't need to change it again in a later release.

I'd also keep the comment in the puppet template. That way you have a minimal diff when you apply it for the first time.

thomasmckay · 2018-08-03T21:08:07Z

I don't have merge permission in this repo, if someone else could do that.

chris1984 · 2018-08-03T21:41:02Z

Thanks @thomasmckay

theforeman-bot added Needs testing Not yet reviewed labels May 21, 2018

thomasmckay force-pushed the 22951-registry branch from e13c925 to 66180e1 Compare May 21, 2018 17:57

ekohl requested changes Jun 2, 2018

View reviewed changes

ehelms reviewed Jun 4, 2018

View reviewed changes

templates/katello.yaml.erb Outdated

:url: <%= @crane_url %>

:ca_cert_file: <%= @crane_ca_cert %>

Copy link

Member

ehelms Jun 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick - extra line here.

ehelms reviewed Jun 4, 2018

View reviewed changes

thomasmckay force-pushed the 22951-registry branch from 66180e1 to f0f9b5c Compare June 25, 2018 20:39

thomasmckay changed the title ~~refs #22951 - add registry to katello.yaml~~ fixes #24070 - add registry to katello.yaml Jun 26, 2018

thomasmckay force-pushed the 22951-registry branch from f0f9b5c to c858cc4 Compare June 29, 2018 14:23

ekohl requested changes Jun 29, 2018

View reviewed changes

This was referenced Jul 19, 2018

fixes #24305 - container image registry config Katello/katello#7543

Merged

refs #24070 - add registry to katello.yaml theforeman/puppet-katello_devel#168

Merged

thomasmckay force-pushed the 22951-registry branch 2 times, most recently from a82a56e to e3739c1 Compare July 31, 2018 14:03

fixes #24070 - config crane for registry

d2f1be9

thomasmckay force-pushed the 22951-registry branch from e3739c1 to d2f1be9 Compare August 1, 2018 11:32

ekohl approved these changes Aug 3, 2018

View reviewed changes

chris1984 merged commit 16a2588 into theforeman:master Aug 3, 2018

ekohl added the Enhancement label Oct 18, 2018

ekohl mentioned this pull request Aug 13, 2019

Adding pulp3 container registry config #296

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #24070 - add registry to katello.yaml #243

fixes #24070 - add registry to katello.yaml #243

thomasmckay commented May 21, 2018 •

edited

Loading

thomasmckay commented May 22, 2018

ekohl left a comment

ehelms Jun 4, 2018

ehelms Jun 4, 2018

ehelms commented Jun 4, 2018

thomasmckay commented Jun 4, 2018

ehelms commented Jun 5, 2018

ehelms commented Jun 5, 2018

ekohl commented Jun 5, 2018

ekohl commented Jun 22, 2018

thomasmckay commented Jun 25, 2018

thomasmckay commented Jun 26, 2018

thomasmckay commented Jun 29, 2018

stbenjam commented Jun 29, 2018

ekohl left a comment

ekohl Jun 29, 2018

thomasmckay Jul 2, 2018

ekohl Jul 2, 2018

thomasmckay Jul 2, 2018

ekohl Jul 2, 2018

thomasmckay Jul 2, 2018

ekohl Jun 29, 2018

thomasmckay Jul 2, 2018

ekohl Jun 29, 2018

thomasmckay Jul 19, 2018

ekohl Jul 19, 2018

thomasmckay commented Jul 19, 2018

thomasmckay commented Jul 30, 2018

ekohl commented Jul 31, 2018

thomasmckay commented Aug 3, 2018

chris1984 commented Aug 3, 2018

fixes #24070 - add registry to katello.yaml #243

fixes #24070 - add registry to katello.yaml #243

Conversation

thomasmckay commented May 21, 2018 • edited Loading

thomasmckay commented May 22, 2018

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehelms commented Jun 4, 2018

thomasmckay commented Jun 4, 2018

ehelms commented Jun 5, 2018

ehelms commented Jun 5, 2018

ekohl commented Jun 5, 2018

ekohl commented Jun 22, 2018

thomasmckay commented Jun 25, 2018

thomasmckay commented Jun 26, 2018

thomasmckay commented Jun 29, 2018

stbenjam commented Jun 29, 2018

ekohl 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasmckay commented Jul 19, 2018

thomasmckay commented Jul 30, 2018

ekohl commented Jul 31, 2018

thomasmckay commented Aug 3, 2018

chris1984 commented Aug 3, 2018

thomasmckay commented May 21, 2018 •

edited

Loading