-
Notifications
You must be signed in to change notification settings - Fork 158
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
Remove existing SSL management. #701
Conversation
@@ -57,12 +57,6 @@ func New(e *empire.Empire, authenticator auth.Authenticator) httpx.Handler { | |||
// OAuth | |||
r.Handle("/oauth/authorizations", &PostAuthorizations{e}).Methods("POST") | |||
|
|||
// SSL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should map these to http handlers that explain that they were removed.
This looks good so far - are you planning to put the new SSL attachment stuff in this same PR before merging? |
Yep, I'll be adding the new endpoint shortly with support in |
return err | ||
} | ||
|
||
if err := s.releaser.ReleaseApp(ctx, app); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of re-releasing, this should probably creat a new release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll follow up with this in a separate PR that makes the API simpler for creating new releases for apps.
Remove existing SSL management.
Fixes #471
Fixes #662
This still needs client support (
emp certs-attach
) and associated API endpoint, but this would change how certs are managed via Empire, in that you don't use Empire to manage certs, just attach existing certs to apps.Note that #700 will still be required to fix the fact that ELB's can't be updated to use new certs.
The basic flow for attaching a cert to an Empire application will be:
I think this will also ultimately work better when we add more backends, since we won't have to try to support varying CertManager implementations.
Also note that this will be backwards compatible with existing certificates inside Empire (see migration).
TODO