pkg/asset/tls: add etcd metrics assets #1291

hexfusion · 2019-02-21T16:46:40Z

This PR is to start a conversation about using etcd grpc-proxy to provide TLS trust separation between etcd server and monitoring. The idea here is to provide monitoring a root CA which they can own giving access to etcd metrics but not etcd server directly. etcd grpc-proxy will only forward GET requests to etcd server /metrics endpoint.

/cc @abhinavdahiya

Blocking: openshift/machine-config-operator#517.

hexfusion · 2019-02-25T15:30:42Z

/test e2e-aws

abhinavdahiya · 2019-02-25T17:18:43Z

pkg/asset/tls/etcdmetricsproxy.go

+)
+
+// EtcdMetricsProxyCA is the asset that generates the etcd-metrics-proxy-ca key/cert pair.
+// [DEPRECATED]


a new asset is already deprecated? ;)

yeah started with your template but I see where you are going now will clean that up, thanks for the feedback!

abhinavdahiya · 2019-02-25T17:20:03Z

we are moving towards dropping root-ca from all chains. some examples #1232

hexfusion · 2019-02-26T18:19:58Z

@abhinavdahiya does this look more inline with what you are expecting?

pkg/asset/tls/etcdmetrics.go

hexfusion · 2019-02-26T19:55:48Z

/test e2e-aws

data/data/manifests/bootkube/etcd-service.yaml

pkg/asset/tls/etcdmetrics.go

hexfusion · 2019-03-01T20:37:28Z

@abhinavdahiya PTAL

abhinavdahiya · 2019-03-01T21:30:24Z

data/data/aws/vpc/sg-master.tf

+  security_group_id = "${aws_security_group.master.id}"
+
+  protocol  = "tcp"
+  from_port = 23790


can you get approval to use that port openshift/origin#21520

abhinavdahiya · 2019-03-01T21:33:12Z

pkg/asset/manifests/operators.go

@@ -145,6 +153,9 @@ func (m *Manifests) generateBootKubeManifests(dependencies asset.Parents) []*ass
 		EtcdCaCert:                      string(etcdCA.Cert()),
 		EtcdClientCert:                  base64.StdEncoding.EncodeToString(etcdClientCertKey.Cert()),
 		EtcdClientKey:                   base64.StdEncoding.EncodeToString(etcdClientCertKey.Key()),
+		EtcdMetricsCaCert:               string(etcdMetricsSignerCertKey.Cert()),


shouldn't you be using
https://github.com/openshift/installer/pull/1291/files#diff-303f2bd7732f182add6f8ae14e9929f1R41

@abhinavdahiya I would like to keep it like this if possible. It feels like we could drop the CA Bundle config but maybe I am missing something?

this seems to used for https://github.com/openshift/installer/pull/1291/files#diff-626d6cfa425c30cb6c099ebc5324d25b
I think we should use the CABundle asset for this for consistency.

ack will adjust

abhinavdahiya · 2019-03-01T21:34:54Z

@hexfusion this is LGTM.

can you squash to more reasonable number of commits. (if you want to do one that's fine too.)
Document reserved ports origin#21520 make sure 23790 is one of the recognized port.

hexfusion · 2019-03-04T16:35:53Z

@abhinavdahiya rebased commits. WRT openshift/origin#21520 would it preferred to drop into the 9xxx port range or just wait for verification. I have no strong feeling on the port.

Signed-off-by: Sam Batschelet <sbatsche@redhat.com>

hexfusion · 2019-03-04T18:39:05Z

[root@ip-10-0-172-22 ~]# ss -ltn | grep -Eo ':[9][0-9]{3}' | sort --unique
:9100
:9101
:9537

hexfusion · 2019-03-05T12:11:56Z

/cc @brancz

brancz · 2019-03-05T13:11:13Z

Generally 👍 from me as not having etcd metrics is a regression from 3.11, and it's by far the most important component to be monitored. This must be in 4.0.

abhinavdahiya

/lgtm

openshift-ci-robot · 2019-03-05T14:54:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, hexfusion

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [abhinavdahiya]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot requested a review from abhinavdahiya February 21, 2019 16:46

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 21, 2019

hexfusion force-pushed the etcd_metrics branch 5 times, most recently from 593858d to ae0953a Compare February 25, 2019 15:04

hexfusion force-pushed the etcd_metrics branch from 83e5e46 to 1737e38 Compare February 25, 2019 16:35

abhinavdahiya reviewed Feb 25, 2019

View reviewed changes

hexfusion force-pushed the etcd_metrics branch 2 times, most recently from 60926f0 to d20c0d3 Compare February 26, 2019 16:20

hexfusion changed the title ~~[WIP] pkg/asset/tls: add etcd metrics proxy assets~~ pkg/asset/tls: add etcd metrics assets Feb 26, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 26, 2019

mrogers950 reviewed Feb 26, 2019

View reviewed changes

pkg/asset/tls/etcdmetrics.go Outdated Show resolved Hide resolved

hexfusion changed the title ~~pkg/asset/tls: add etcd metrics assets~~ [WIP] pkg/asset/tls: add etcd metrics assets Feb 28, 2019

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 28, 2019

hexfusion commented Mar 1, 2019

View reviewed changes

data/data/manifests/bootkube/etcd-service.yaml Show resolved Hide resolved

hexfusion commented Mar 1, 2019

View reviewed changes

pkg/asset/tls/etcdmetrics.go Outdated Show resolved Hide resolved

hexfusion force-pushed the etcd_metrics branch from 6c5b6dd to 471327c Compare March 1, 2019 16:20

hexfusion mentioned this pull request Mar 1, 2019

BUG 1670700: *: add support for etcd metrics proxy openshift/machine-config-operator#517

Merged

hexfusion changed the title ~~[WIP] pkg/asset/tls: add etcd metrics assets~~ pkg/asset/tls: add etcd metrics assets Mar 1, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 1, 2019

abhinavdahiya reviewed Mar 1, 2019

View reviewed changes

hexfusion force-pushed the etcd_metrics branch from 471327c to 69bd28b Compare March 4, 2019 16:01

hexfusion added 2 commits March 4, 2019 12:30

data/data/*: add configs to support etcd metrics proxy

8865eb7

Signed-off-by: Sam Batschelet <sbatsche@redhat.com>

pkg/asset/*: add etcd metrics TLS assets

cb4bbec

Signed-off-by: Sam Batschelet <sbatsche@redhat.com>

hexfusion force-pushed the etcd_metrics branch from 69bd28b to cb4bbec Compare March 4, 2019 17:41

openshift-ci-robot requested a review from brancz March 5, 2019 12:11

abhinavdahiya reviewed Mar 5, 2019

View reviewed changes

openshift-ci-robot assigned abhinavdahiya Mar 5, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 5, 2019

openshift-merge-robot merged commit c91cdbc into openshift:master Mar 5, 2019

hexfusion deleted the etcd_metrics branch March 5, 2019 16:10

abhinavdahiya mentioned this pull request Mar 5, 2019

BUG 1684206: *: store etcd CA and client certs in cluster #1363

Merged

hexfusion mentioned this pull request Mar 6, 2019

data/data/bootstrap: add etcd metrics flags #1377

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/asset/tls: add etcd metrics assets #1291

pkg/asset/tls: add etcd metrics assets #1291

hexfusion commented Feb 21, 2019 •

edited

Loading

hexfusion commented Feb 25, 2019

abhinavdahiya Feb 25, 2019

hexfusion Feb 25, 2019

abhinavdahiya commented Feb 25, 2019

hexfusion commented Feb 26, 2019

hexfusion commented Feb 26, 2019

hexfusion commented Mar 1, 2019

abhinavdahiya Mar 1, 2019

abhinavdahiya Mar 1, 2019

hexfusion Mar 4, 2019

abhinavdahiya Mar 4, 2019

hexfusion Mar 4, 2019

abhinavdahiya commented Mar 1, 2019

hexfusion commented Mar 4, 2019

hexfusion commented Mar 4, 2019

hexfusion commented Mar 5, 2019

brancz commented Mar 5, 2019

abhinavdahiya left a comment

openshift-ci-robot commented Mar 5, 2019

pkg/asset/tls: add etcd metrics assets #1291

pkg/asset/tls: add etcd metrics assets #1291

Conversation

hexfusion commented Feb 21, 2019 • edited Loading

hexfusion commented Feb 25, 2019

abhinavdahiya Feb 25, 2019

Choose a reason for hiding this comment

hexfusion Feb 25, 2019

Choose a reason for hiding this comment

abhinavdahiya commented Feb 25, 2019

hexfusion commented Feb 26, 2019

hexfusion commented Feb 26, 2019

hexfusion commented Mar 1, 2019

abhinavdahiya Mar 1, 2019

Choose a reason for hiding this comment

abhinavdahiya Mar 1, 2019

Choose a reason for hiding this comment

hexfusion Mar 4, 2019

Choose a reason for hiding this comment

abhinavdahiya Mar 4, 2019

Choose a reason for hiding this comment

hexfusion Mar 4, 2019

Choose a reason for hiding this comment

abhinavdahiya commented Mar 1, 2019

hexfusion commented Mar 4, 2019

hexfusion commented Mar 4, 2019

hexfusion commented Mar 5, 2019

brancz commented Mar 5, 2019

abhinavdahiya left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Mar 5, 2019

hexfusion commented Feb 21, 2019 •

edited

Loading