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

Allow SSL certs to be updated on ELB's in ECS backend #700

Merged
merged 5 commits into from
Dec 22, 2015
Merged

Conversation

ejholmes
Copy link
Contributor

This allows the ECS backend to update the SSL cert on the associated ELB if it changes.

A better approach would be to provision resources using CloudFormation like talked about in #696, but that's a ways off and will require a substantial migration.

}

func (m *ELBManager) updateSSLCert(ctx context.Context, name, certID string) error {
_, err := m.elb.SetLoadBalancerListenerSSLCertificate(&elb.SetLoadBalancerListenerSSLCertificateInput{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should actually ensure that there's a 443 listener in case there was previously no cert attached.

@ejholmes
Copy link
Contributor Author

Just realized that #662 actually still prevents this. I wonder if it would just be better if the UI for adding an SSL cert was more of an "attachment" flow:

$ aws iam upload-server-certificate --server-certificate-name ProdCert
$ emp cert-attach ProdCert -a <app>

This would also fix #471, and make it more obvious what cert is attached to a load balancer.

@phobologic
Copy link
Contributor

I'm all for changing the behavior to something like you're talking about, for all the reasons you mention.

@mwildehahn
Copy link
Contributor

huge +1, this would be amazing

ejholmes added a commit that referenced this pull request Dec 22, 2015
Allow SSL certs to be updated on ELB's in ECS backend
@ejholmes ejholmes merged commit 4573b33 into master Dec 22, 2015
@ejholmes ejholmes deleted the update-elb branch December 22, 2015 02:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants