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

TLS verification of backend services #852

Closed

Conversation

robbiemcmichael
Copy link
Contributor

@davecheney It was good to meet you at the YOW! Conference. I briefly talked to you about implementing TLS verification for backend services; I finally got around to finishing off a sample implementation which verifies both the CA and hostname of the certificate.

This PR addresses issue #813. The implementation is similar to the design I suggested in that issue, however the ingress route schema has changed slightly to accommodate hostname verification. The question I had around the granularity of specifying the TLS verification settings (i.e. should it be one per ingress route, route or service?) is still my main concern and it would be good to get your thoughts. I'm happy to rework this PR based on any design concerns or code review comments.

@robbiemcmichael
Copy link
Contributor Author

Updated my branch to remove some modifications to the deployment YAML file that were used for testing and accidentally left in as well as to add sign-offs to all commit messages.

@davecheney davecheney added this to the 0.10.0 milestone Jan 25, 2019
Copy link
Contributor

@davecheney davecheney left a comment

Choose a reason for hiding this comment

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

Thank you for this change. Apart from the unrelated cleanups in the PR, I would prefer to not merge this until a discussion on the implementation via #813 is complete.

I'm sorry that it has taken so long to reply, we're a very small team.

v.clusters[c.Name] = c
}
case *dag.TCPService:
case dag.Service:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is good cleanup, but unrelated to this change. Can you please send it as its own PR. Thank you.

Copy link
Contributor Author

@robbiemcmichael robbiemcmichael Jan 29, 2019

Choose a reason for hiding this comment

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

It wasn't completely unrelated to this PR, I had to change Clustername to take a Service as a parameter since the hash needed to include fields that are only present in an HTTPService.

The cleanup here was made possible as a byproduct of changing that function, but I can still split those parts out into a separate PR if you'd like.

@davecheney
Copy link
Contributor

Thank you for working on this. I'm sorry that it took so long to resolve and that we ended up duplicating some of your work.

I'm going to close this as a duplicate of #1045

@davecheney davecheney closed this May 8, 2019
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.

2 participants