Skip to content
This repository was archived by the owner on Jan 21, 2022. It is now read-only.

Loggregator etcd tls #978

Closed
wants to merge 3 commits into from
Closed

Loggregator etcd tls #978

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c62ffca
Adds properties for loggregator to communicate with etcd over TLS
Jun 13, 2016
7133ad8
Adds etcd TLS properties
Jun 13, 2016
1b8ba9d
Sets consul_agent to be first template to be deployed
Jun 13, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 52 additions & 6 deletions templates/cf.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,10 @@ jobs:
properties:
metron_agent:
zone: z1
consul:
agent:
services:
etcd: {name: cf-etcd}
Copy link
Contributor

@wfernandes wfernandes Jun 14, 2016

Choose a reason for hiding this comment

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

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

update:
serial: true
max_in_flight: 1
Expand All @@ -112,6 +116,10 @@ jobs:
properties:
metron_agent:
zone: z2
consul:
agent:
services:
etcd: {name: cf-etcd}
update:
serial: true
max_in_flight: 1
Expand Down Expand Up @@ -568,9 +576,22 @@ properties:

etcd:
machines: (( merge || jobs.etcd_z1.networks.cf1.static_ips jobs.etcd_z2.networks.cf2.static_ips ))
require_ssl: false
peer_require_ssl: false
require_ssl: (( merge || false ))
peer_require_ssl: (( merge || false ))
advertise_urls_dns_suffix: (( merge || "etcd.service.cf.internal" ))
cluster:
- instances: (( jobs.etcd_z1.instances ))
name: etcd_z1
- instances: (( jobs.etcd_z2.instances ))
name: etcd_z2
ca_cert: (( merge || nil ))
server_cert: (( merge || nil ))
server_key: (( merge || nil ))
client_cert: (( merge || nil ))
client_key: (( merge || nil ))
peer_ca_cert: (( merge || nil ))
peer_cert: (( merge || nil ))
peer_key: (( merge || nil ))

etcd_metrics_server:
nats:
Expand Down Expand Up @@ -618,6 +639,8 @@ properties:
tls:
ca_cert: ~
etcd:
ca_cert: (( .properties.etcd.ca_cert ))
require_ssl: (( .properties.etcd.require_ssl ))
Copy link
Contributor

@Amit-PivotalLabs Amit-PivotalLabs Jun 14, 2016

Choose a reason for hiding this comment

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

Don't you also need cluster here?

Copy link
Contributor

@wfernandes wfernandes Jun 14, 2016

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@Amit-PivotalLabs Amit-PivotalLabs Jun 15, 2016

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@wfernandes wfernandes Jun 16, 2016

Choose a reason for hiding this comment

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

Yes. That is correct. The clients don't care about the cluster.

Copy link
Contributor

@Amit-PivotalLabs Amit-PivotalLabs Jun 17, 2016

Choose a reason for hiding this comment

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

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.

machines: (( .properties.etcd.machines ))

loggregator_endpoint:
Expand All @@ -631,6 +654,9 @@ properties:
blacklisted_syslog_ranges: ~
unmarshaller_count: (( merge || 5 ))
port: (( merge || 4443 ))
etcd:
client_cert: ~
client_key: ~
Copy link
Contributor

@Amit-PivotalLabs Amit-PivotalLabs Jun 14, 2016

Choose a reason for hiding this comment

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

I see this in 3 places. For those 3 places, shouldn't it be (( .properties.etcd.client_cert )) instead of just ~?

Copy link
Contributor

@wfernandes wfernandes Jun 14, 2016

Choose a reason for hiding this comment

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

We actually didn't want to use the etcd.client_cert because we wanted separate etcd client certs for our components so that in the event of a leaked cert, we only need to roll the respective components. For example, if Doppler & Metron shared the same etcd client certs and if it were compromised, then we would have to roll the entire CF and Diego deployment.

Also there is another job called the syslog_drain_binder. We didn't add that in this template because it could get pulled in from the stub. So in the stub we would have an additional property block like below:

syslog_drain_binder:
  etcd:
    client_cert: foo
    client_key: bar

Copy link
Contributor

@Amit-PivotalLabs Amit-PivotalLabs Jun 15, 2016

Choose a reason for hiding this comment

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

I see. So we'll need to generate a lot of different etcd client certs. There's a question of what should be possible in the manifest, and what should be possible via the spiff templates. We should be able to configure the client certs independently in the manifest. Whether the spiff workflow should do that is a separate question. The spiff templates have a confusing use cases, they're supposed to be both easy to get started with, but some people also want it to be hardened for production deployments too. We err on the side of simplicity, because there's no way to guarantee spiff templates are production hardened. Let's discuss whether it makes sense, for the purpose of these templates, to just copy around the same client certs.

Copy link
Contributor

@wfernandes wfernandes Jun 16, 2016

Choose a reason for hiding this comment

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

That's fine with us. I just didn't want to make us dependent on the etcd.client_cert and etcd.client_key properties. We can create another etcd client cert and share it across the loggregator components if that is simpler.

Copy link
Contributor

@shalako shalako Jun 17, 2016

Choose a reason for hiding this comment

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

@wfernandes if you intend to use client_cert and client_key namespaced to your client jobs, why does the PR add these properties to the top-level etcd namespace?

Copy link
Contributor

@Amit-PivotalLabs Amit-PivotalLabs Jun 17, 2016

Choose a reason for hiding this comment

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

@wfernandes Makes sense, want to allow different client certs for different jobs. That's all well and good, but for the spiff templates, which are sorta geared to simplicity, they should probably just copy the same client cert around to all clients, rather than ask the new operator to produce N different client certs and keys. So in spiff, I still stand by my original line comment here.

Copy link
Contributor

@wfernandes wfernandes Jun 22, 2016

Choose a reason for hiding this comment

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

@Amit-PivotalLabs Sounds good. We can use the same certs and tie them to the loggregator components that need them.

tls:
server_cert: ~
server_key: ~
Expand All @@ -645,6 +671,9 @@ properties:
preferred_protocol: ~
enable_buffer: ~
buffer_size: ~
etcd:
client_cert: ~
client_key: ~
tls:
client_cert: ~
client_key: ~
Expand All @@ -656,6 +685,9 @@ properties:
outgoing_port: 8080
zone: (( merge || nil ))
disable_access_control: (( merge || nil ))
etcd:
client_cert: ~
client_key: ~
security_event_logging:
enabled: (( merge || false ))

Expand Down Expand Up @@ -1195,6 +1227,8 @@ meta:
release: (( meta.loggregator_release_name ))

clock_templates:
- name: consul_agent
release: (( meta.consul_release_name ))
- name: cloud_controller_clock
release: (( meta.capi_release_name ))
- name: metron_agent
Expand All @@ -1217,6 +1251,8 @@ meta:
release: (( meta.loggregator_release_name ))

etcd_templates:
- name: consul_agent
release: (( meta.consul_release_name ))
- name: etcd
release: (( meta.etcd_release_name ))
- name: etcd_metrics_server
Expand All @@ -1225,12 +1261,12 @@ meta:
release: (( meta.loggregator_release_name ))

ha_proxy_templates:
- name: consul_agent
release: (( meta.consul_release_name ))
- name: haproxy
release: (( meta.cf_release_name ))
- name: metron_agent
release: (( meta.loggregator_release_name ))
- name: consul_agent
release: (( meta.consul_release_name ))

hm9000_routes:
- name: hm9000
Expand All @@ -1252,6 +1288,8 @@ meta:
release: (( meta.cf_release_name ))

loggregator_templates:
- name: consul_agent
release: (( meta.consul_release_name ))
- name: doppler
release: (( meta.loggregator_release_name ))
- name: syslog_drain_binder
Expand All @@ -1272,6 +1310,8 @@ meta:
- (( "loggregator." .properties.system_domain ))

loggregator_trafficcontroller_templates:
- name: consul_agent
release: (( meta.consul_release_name ))
- name: loggregator_trafficcontroller
release: (( meta.loggregator_release_name ))
- name: metron_agent
Expand All @@ -1280,6 +1320,8 @@ meta:
release: (( meta.cf_release_name ))

nats_templates:
- name: consul_agent
release: (( meta.consul_release_name ))
- name: nats
release: (( meta.cf_release_name ))
- name: nats_stream_forwarder
Expand Down Expand Up @@ -1319,6 +1361,8 @@ meta:
release: (( meta.cf_release_name ))

postgres_templates:
- name: consul_agent
release: (( meta.consul_release_name ))
- name: postgres
release: (( meta.cf_release_name ))
- name: metron_agent
Expand All @@ -1333,6 +1377,8 @@ meta:
release: (( meta.loggregator_release_name ))

stats_templates:
- name: consul_agent
release: (( meta.consul_release_name ))
- name: collector
release: (( meta.cf_release_name ))
- name: metron_agent
Expand All @@ -1354,12 +1400,12 @@ meta:
script_path: /var/vcap/jobs/uaa/bin/health_check

uaa_templates:
- name: consul_agent
release: (( meta.consul_release_name ))
- name: uaa
release: (( meta.uaa_release_name ))
- name: metron_agent
release: (( meta.loggregator_release_name ))
- name: consul_agent
release: (( meta.consul_release_name ))
- name: route_registrar
release: (( meta.cf_release_name ))
- name: statsd-injector
Expand Down