Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #28922: Deploy Candlepin on localhost by default #142

Merged
merged 1 commit into from
Mar 4, 2020

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Feb 4, 2020

No description provided.

@ehelms
Copy link
Member Author

ehelms commented Feb 4, 2020

Associated changes:

Generally speaking, Candlepin does not need to be exposed publicly and this decreases the set of ports exposed. Further, all calls to Candlepin in a Katello scenario are proxied through Katello. This still allows a future state of pulling CP off to it's own host for scaling purposes if need be; but optimizes for the 99% case.

@jturel
Copy link
Contributor

jturel commented Feb 11, 2020

I tested all the related PRs (not #143) and things seem to work, with one quirk. The candlepin URL in the katello.yaml still references the VM hostname. I don't see how it's possible.

[root@centos7-katello-devel /]# grep candlepin_url /usr/share/foreman-installer/modules/katello/manifests/params.pp
# @param candlepin_url
  Stdlib::Httpsurl $candlepin_url = "https://localhost:8443/candlepin",

The installer failed, but once I updated the config to use localhost things are working:

[vagrant@centos7-katello-devel foreman]$ curl localhost:3000/katello/api/v2/ping
  {"status":"ok","services":{"pulp":{"status":"ok","duration_ms":"41"},"pulp_auth":{"status":"ok","duration_ms":"55"},"candlepin":{"status":"ok","duration_ms":"342"},"candlepin_auth":{"statu
s":"ok","duration_ms":"24"},"foreman_tasks":{"status":"ok","duration_ms":"11552"},"katello_events":{"status":"ok","message":"0 Processed, 0 Failed","duration_ms":"1"},"candlepin_events":{"st
atus":"ok","message":"0 Processed, 0 Failed","duration_ms":"1"}}}

My client registered successfully as well

@jturel
Copy link
Contributor

jturel commented Feb 11, 2020

my box definition was:

centos7-katello-devel:                                                                                                                                                                        
  primary: true                                                                                                                                                                               
  box: centos7                                                                                                                                                                                
  ansible:                                                                                                                                                                                    
    playbook: 'playbooks/katello_devel.yml'                                                                                                                                                   
    group: 'devel'                                                                                                                                                                            
    variables:                                                                                                                                                                                
      ssh_forward_agent: true                                                                                                                                                                 
      foreman_devel_github_push_ssh: True                                                                                                                                                     
      katello_devel_github_username: 'jturel'                                                                                                                                                 
      foreman_installer_module_prs:                                                                                                                                                           
        #        - theforeman/candlepin/143                                                                                                                                                   
        - theforeman/candlepin/142                                                                                                                                                            
        - theforeman/certs/273                                                                                                                                                                
        - theforeman/katello/321

@ehelms
Copy link
Member Author

ehelms commented Feb 12, 2020

For discussion I will add our deployment options. First a reminder of how requests to Candlepin are handled.

Client -> Apache -> Katello -> Candlepin

That is, clients never talk directly to Candlepin, all APIs are proxied through Katello for authentication and authorization.

Given that, I see our options as:

  1. Deploy on localhost with Tomcat handling SSL; Katello talks to Candlepin via localhost
  2. Deploy on localhost with Tomcat handling no SSL; Katello talks to Candlepin via localhost
  3. Deploy on localhost with Apache handling SSL and proxying to Candlepin; Katello talks to Candlepin via Apache

#1 keeps the current model while simplifying certificates and simplifying ports exposure and usage (i.e. frees up 8443) while keeping a secure connection.

#2 simplifies deployment the most by reducing overall certificate requirements and frees up resources inside Tomcat as it no longer has to handle SSL for each connection. Of which, Candlepin via /rhsm API has the highest number of requests handled.

#3 Standardizes on Apache layer reverse proxying connections and terminating SSL for services on the host. This also frees up Tomcat resources to no longer need to handle SSL. In theory, this provides a standardized model for large user bases that would extract Candlepin to a separate host. This last point is more theoretical than tested or used. This model also creates the following for a single request on the same box:

Client -> Apache -> Katello -> Apache -> Candlepin

@ehelms
Copy link
Member Author

ehelms commented Feb 12, 2020

@jturel Good catch, puppet-katello_devel will need its own update:

https://github.com/theforeman/puppet-katello_devel/blob/master/manifests/init.pp#L129

@ehelms
Copy link
Member Author

ehelms commented Feb 18, 2020

@laugmanuel @timogoebel -- any concerns on your end about changing the default to localhost for the module? Please do read through #142 (comment) for my thinking here

@ekohl
Copy link
Member

ekohl commented Feb 18, 2020

FWIW, IMHO it would make a lot of sense to have a smart proxy module that indicates the location of candlepin. Right now Katello assumes they share a hostname but in a multi homed setup this actually fails. It'd be a lot more explicit. This is a similar model to how we work with Pulp 3. It's not strictly the same as this effort, but it could help with further isolation the individual components of the stack.

@ehelms
Copy link
Member Author

ehelms commented Feb 18, 2020

I've discussed that idea with @jlsherrill before and he tends to shy away from it. Pulp is different in that we deploy it to external proxies for mirroring and having one way to communicate with it makes sense. Candlepin is a bit more special in that it is always hidden behind Katello. I'll let him weigh in since I am paraphrasing him a bit.

@timogoebel
Copy link
Member

any concerns on your end about changing the default to localhost for the module?

Go ahead. I believe we're good as long as there is an option to make candlepin listen on another interface.

@ehelms
Copy link
Member Author

ehelms commented Feb 19, 2020

This is ready for a round of review alongside theforeman/puppet-certs#273

@jlsherrill
Copy link
Contributor

@ekohl i understand the desire to make it smart proxy aware, but that adds a lot of complexity at this point that i don't think is worth it. Unless something drastic changes, we'll only ever have 1 candlepin.

@ekohl
Copy link
Member

ekohl commented Feb 19, 2020

@jlsherrill that's not what I meant. I was talking about the endpoint exposed to client systems (the Apache reverse proxy) and we do have 1 of those for every content proxy.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Without this change we listened on any host, right? That would make this breaking so I've added the Backwards Incompatible label.

manifests/init.pp Outdated Show resolved Hide resolved
@ekohl
Copy link
Member

ekohl commented Feb 27, 2020

Just thinking out loud: should we make it optional and only link it up to localhost in puppet-katello?

@ehelms
Copy link
Member Author

ehelms commented Feb 27, 2020

I personally prefer to have this reflect our standard deployment model and provide the option to configure it.

@ehelms
Copy link
Member Author

ehelms commented Mar 2, 2020

I saw this got the backwards incompatible label. What about this change is backwards incompatible? The default value changing?

@ekohl
Copy link
Member

ekohl commented Mar 2, 2020

Yes. It's also a signal that the next version is a major version bump. That prevents this change from accidentally making it into a stable version.

@ehelms
Copy link
Member Author

ehelms commented Mar 2, 2020

OK, I was just not aware changing a default value (since you can still configure this to your deployment) was a backwards incompatible change. I could be persuaded to keep hostname and just introduce the parameter if that made things easier.

@ekohl
Copy link
Member

ekohl commented Mar 3, 2020

In this case it doesn't matter that much because 504ed85 was already breaking (since it changed the behavior if you didn't pass in any variables). Might as well stack them while we're at it. If it wasn't the case, I'd feel stronger about it.

@ehelms
Copy link
Member Author

ehelms commented Mar 4, 2020

@ekohl good to go on this change now?

@ekohl
Copy link
Member

ekohl commented Mar 4, 2020

Merging this since the katello change is pretty good to go and needs this to be green.

@ekohl ekohl merged commit 191154f into theforeman:master Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants