Enable SSL Certificate authentication for etcd endpoint #677
Conversation
|
In our cluster, we need certificate authentication. Can you review this pull request ? Thanks. |
| @@ -11,6 +11,9 @@ import ( | |||
| type Config struct { | |||
| EtcdServers []string | |||
| EtcdKeyPrefix string | |||
| KeyFile string | |||
There was a problem hiding this comment.
Please prefix these new flags with "etcd", just like "etcd_servers" and "etcd_key_prefix"
|
Please document these new flags in https://github.com/coreos/fleet/blob/master/Documentation/configuration.md and add them to https://github.com/coreos/fleet/blob/master/fleet.conf.sample |
| @@ -356,3 +359,50 @@ func (ar *actionResolver) one(req *http.Request, cancel <-chan bool) (resp *http | |||
| log.V(1).Infof("etcd: recv response from %s %s: %s", req.Method, req.URL, resp.Status) | |||
| return | |||
| } | |||
|
|
|||
| func GetTlsClientConfig(cafile string, keyfile string, certfile string) (*tls.Config, error) { | |||
There was a problem hiding this comment.
"Get" is unnecessary, just call this TLSClientConfig
There was a problem hiding this comment.
Please add some unit tests of this new function.
|
Thanks for your review. I made the corrections. |
| @@ -356,3 +359,50 @@ func (ar *actionResolver) one(req *http.Request, cancel <-chan bool) (resp *http | |||
| log.V(1).Infof("etcd: recv response from %s %s: %s", req.Method, req.URL, resp.Status) | |||
| return | |||
| } | |||
|
|
|||
| func GetTLSClientConfig(cafile string, keyfile string, certfile string) (*tls.Config, error) { | |||
There was a problem hiding this comment.
I think you missed my comments on this function. I'll reiterate:
- Drop the
Getfrom the function name - Add some unittests of this function
|
Sorry, you are right I had missed your comment. |
| @@ -356,3 +359,50 @@ func (ar *actionResolver) one(req *http.Request, cancel <-chan bool) (resp *http | |||
| log.V(1).Infof("etcd: recv response from %s %s: %s", req.Method, req.URL, resp.Status) | |||
| return | |||
| } | |||
|
|
|||
| func TLSClientConfig(cafile string, keyfile string, certfile string) (*tls.Config, error) { | |||
There was a problem hiding this comment.
It would help testability if you were passing in io.Readers rather than filenames. At that point, you can just generate objects on the fly and assert the proper *tls.Config is generated.
|
Thanks. I have added 3 unit tests. |
| @@ -8,6 +8,11 @@ | |||
| # by the underlying go-etcd library. | |||
| # etcd_servers=["http://127.0.0.1:4001"] | |||
|
|
|||
| # Provide TLS configuration when SSL certificate authentication is enabled in etcd endpoints | |||
| # etcd_cafile=path/to/CAfile | |||
There was a problem hiding this comment.
Can you prepend / to "path/to/CAfile" and "path/to/keyfile"?
|
Just about there, thanks @oalbiez |
|
@bcwaldon, do you agree on the generating files in unit tests ? |
|
Sure, that'll work. Squash all of your commits down and we'll merge. |
|
Commits squashed. Ready to be merged :) |
|
@oalbiez One last thing. Please format your commit message per https://github.com/coreos/fleet/blob/master/CONTRIBUTING.md#format-of-the-commit-message |
Provide TLS configuration when SSL certificate authentication is enabled in etcd endpoints. Add configuration option for keyfile, certfile and cafile on both fleet and fleetctl.
|
Commit message updated. |
|
@oalbiez thanks, looks much better. I'm going to functionally test this over the weekend and get it in to the v0.6.0 release on Monday. |
Enable SSL Certificate authentication for etcd endpoint
This pull request allows to connect etcd with certificate authentication.
Change are made for fleetctl and fleet
Some code could be factored between etcd and fleet... but I need this functionality now.
Could you merge before refactoring?