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 #9674 - Handle ProxyAPI exceptions on PuppetCA controller #2229

Closed
wants to merge 1 commit into from

Conversation

dLobatog
Copy link
Member

@dLobatog dLobatog commented Mar 7, 2015

Currently if any error happens when signing a certificate request, Foreman will throw a 500 error page.

See the following example for a error of a client sending a broken cert request (incompatible digest algorithm):

ERF12-9815 [ProxyAPI::ProxyException]: Unable to sign PuppetCA certificate for samplehost ([RestClient::NotAcceptable]: 406 Not Acceptable) for proxy https://mydomain:9090/puppet/ca

We can handle this through an alert and return back to the smart proxy PuppetCA index page.

Same goes for any other ProxyAPI::ProxyException that happens within the PuppetCA controller, they all throw a nasty 500 as they are not rescued at all.

process_success({ :success_redirect => smart_proxy_puppetca_index_path(@proxy, :state => params[:state]), :object_name => cert.to_s })
else
process_error({ :redirect => smart_proxy_puppetca_index_path(@proxy) })
proxy_command do
Copy link
Member

Choose a reason for hiding this comment

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

I think we could use rescue_from rather than wrapping every action in proxy_command

@dLobatog
Copy link
Member Author

dLobatog commented Mar 9, 2015

@ares Updated, thanks for the review 🙇

when "create" then hash[:render] ||= "new"
when "update" then hash[:render] ||= "edit"
else
hash[:redirect] ||= send("#{controller_name}_url")
Copy link
Member

Choose a reason for hiding this comment

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

nit - hash[:redirect] can't be blank, same applies to hash[:render] in both when branches so ||= should be just =

@ares
Copy link
Member

ares commented Mar 10, 2015

When an error appears on index page (e.g. proxy not running) we have redirect loop, because user is always redirected index page.

@dLobatog
Copy link
Member Author

@ares Fixed, I redirect to 🔙 instead, I think that should more or less ensure no loops.

@@ -1,15 +1,13 @@
class PuppetcaController < ApplicationController
rescue_from ProxyAPI::ProxyException do |exception|
Copy link
Contributor

Choose a reason for hiding this comment

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

Why only in this controller? I somewhat see your point about the 500 page (noting that the behaviour is different between prod and dev too, prod is prettier), but shouldn't this be in app controller and handled application-wide?

@dLobatog
Copy link
Member Author

dLobatog commented Apr 6, 2015

@domcleal I updated it to move the rescue_from to application_controller. I didn't do it before because I don't know what other parts of the application it may affect badly.

I think tests for the rescue fit better in application_controller_subclass_test but since I didn't see any other rescue tests there, I'll leave it just testing the behavior this issue is reporting keeps fixed.

@domcleal
Copy link
Contributor

domcleal commented May 1, 2015

[test] feeling paranoid about tests becoming broken without merge conflicts.

@domcleal
Copy link
Contributor

domcleal commented May 1, 2015

Thanks @elobato, merged as 1ced4fb.

@domcleal domcleal closed this May 1, 2015
This pull request was closed.
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.

4 participants