Loggregator etcd tls #978
Loggregator etcd tls #978
Conversation
[#121255441] Signed-off-by: Sam Nelson <[email protected]>
- Adds consul_agent to all jobs with metron_agent [#121255441] Signed-off-by: Sam Nelson <[email protected]>
|
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/121448173 The labels on this github issue will be updated when the story is started. |
|
Hey nelsam! Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA. |
templates/cf.yml
Outdated
| release: (( meta.capi_release_name )) | ||
| - name: metron_agent | ||
| release: (( meta.loggregator_release_name )) | ||
| - name: consul_agent |
There was a problem hiding this comment.
consul_agent needs to be first everywhere
[#121255441] Signed-off-by: Sam Nelson <[email protected]>
| ca_cert: ~ | ||
| etcd: | ||
| ca_cert: (( .properties.etcd.ca_cert )) | ||
| require_ssl: (( .properties.etcd.require_ssl )) |
There was a problem hiding this comment.
Don't you also need cluster here?
There was a problem hiding this comment.
The cluster configuration is directly under the etcd properties line 582. These properties are to provide our components (Doppler, TrafficController, MetronAgent, SyslogDrainBinder) with the etcd ca_cert, require_ssl, and machines
There was a problem hiding this comment.
It sounds like some of the etcd properties you're copying over into the loggregator.etcd namespace, but stuff like cluster you're taking directly from the etcd namespace?
There was a problem hiding this comment.
Yes. That is correct. The clients don't care about the cluster.
There was a problem hiding this comment.
I see. The clients don't care about cluster, but they do need to know how to talk to the server, which is what "machines" is for. Now this tricky. In non-tls mode, the "machines" should be the list of IPs. But in TLS mode, it should be the single DNS name of the secure etcd cluster, in this case "cf-etcd.service.cf.internal". See e.g. https://github.com/cloudfoundry-incubator/diego-release/blob/develop/jobs/bbs/spec#L59-L61. I'm not sure what the right thing to do here for spiff.
| consul: | ||
| agent: | ||
| services: | ||
| etcd: {name: cf-etcd} |
There was a problem hiding this comment.
Just wanted to point this out, we named this cf-etcd in comparison to other-etcd. The certs should have common name of cf-etcd.service.cf.internal
|
Please make sure the unit tests pass on cf-release with these changes. In its current state, this PR will fail. The best thing to do is to run |
|
@nelsam @wfernandes Did you intend to continue working on this? If not, you could simply wait until the Infrastructure team adds this stuff in anyways. It might be a while, as we're working on the component to support zero-downtime switch to etcd in TLS mode. In that case, we could close out this PR. |
|
@Amit-PivotalLabs We have stopped working on the etcd-tls epic unless/until we need to fix something. So no pressure from us. :) |
|
Closing this PR, we have this story: https://www.pivotaltracker.com/story/show/122070663. Will link to this PR for context. |
etcd.require_sslandetcd.peer_require_sslto merge from stub with a default of false, rather than being forced to falseThe first commit shouldn't have any changes that collide with changes from other teams. The second commit is separate because it has changes related to more global properties, which may have conflicts with changes from other teams.