tls: add aggregated apiserver certs. #2850

diegs · 2018-01-29T20:28:53Z

This adds a new CA and certificate pair to support running aggregated
api-servers in the cluster. This is required for deploying
metrics-server (heapster replacement) and other aggregated api-server
components that will start to appear in 1.9+.

In the self-signed scenario these are derived from the kube_ca, which
happens to also be the apiserver serving ca. In practice the kube_ca
should be the parent of both this CA and the one that is used by the
--tls-ca-file flag in the apiserver. I added a TODO to fix that when
we implement the "single CA is the parent of all generated certs" change
for track 2.

diegs · 2018-01-29T20:32:18Z

cc @brancz

ericchiang · 2018-01-29T21:00:01Z

contrib/user-provided-certs/kube/self-signed/api_proxy.tf

+    organization = "kube-master"
+  }
+
+  dns_names = [


I don't think this should be used as a serving cert, just a client cert.

So, remove the dns names and ip addresses?

Yes. Allowed usages bellow should also drop "server_auth"

ericchiang · 2018-01-29T21:00:19Z

contrib/user-provided-certs/kube/self-signed/api_proxy.tf

+  private_key_pem = "${tls_private_key.apiserver_proxy.private_key_pem}"
+
+  subject {
+    common_name  = "kube-apiserver-proxy"


Common name has to match the value specified by:

--requestheader-allowed-names

Ah, I thought that was the CN in the CA. The docs are very unclear, and the metrics server appeared to be working. But I'll change the flag to this one in the operators-PR.

diegs · 2018-01-29T21:18:38Z

@cpanato @enxebre any hints on how to debug the failed custom_tls tests? Do I need to generate the new certs somehow? The user_provided_tls stuff in the smoke test directory doesn't have much documentation.

diegs · 2018-01-29T23:35:41Z

Ah, think I found the change I needed to make for smoke tests.

diegs · 2018-01-30T00:24:54Z

zomg ruby linter doesn't like methods with more than 30 lines?

diegs · 2018-01-30T02:24:14Z

I think azure flaked.

diegs · 2018-01-30T02:38:17Z

In azure it looks like some of the pods never came up, not sure if this is typical:

        	smoke_test.go:119: retrying in 3s
        	cluster_test.go:99: pod tectonic-system/alm-operator-56d954fc68-77b9q not running
        	cluster_test.go:99: pod tectonic-system/catalog-operator-78d4cdb8db-ksj4x not running
        	cluster_test.go:99: pod tectonic-system/prometheus-k8s-0 not running
        	cluster_test.go:99: pod tectonic-system/prometheus-operator-7445bc9f7f-gtml6 not running
        	cluster_test.go:99: pod tectonic-system/tectonic-monitoring-auth-prometheus-7746db47d8-qzt9j not running
        	smoke_test.go:118: failed with error: pods are not all ready
        	smoke_test.go:119: retrying in 3s
        	cluster_test.go:83: Timed out waiting for pods to be ready.

brancz · 2018-01-30T09:24:28Z

On a high level this looks good to me. I'm wondering how aggregated APIs are going to get their server certs, seems like we want something like the certificates API, but for multi CA (kubernetes components, etcd, aggregated APIs,...). Do we have a solid idea how to solve that?

ericchiang · 2018-01-30T17:31:20Z

On a high level this looks good to me. I'm wondering how aggregated APIs are going to get their server certs, seems like we want something like the certificates API, but for multi CA (kubernetes components, etcd, aggregated APIs,...). Do we have a solid idea how to solve that?

Aggregated API servers get to advertise their own CA, which the API server then trusts https://github.com/kubernetes/kubernetes/blob/v1.9.2/staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/types.go#L56

So the metrics server can generate a self-signed one and that should be fine.

brancz · 2018-01-30T18:08:48Z

lgtm

diegs · 2018-01-30T18:34:00Z

@brancz @ericchiang plot twist: apparently we don't want the metrics server to generate self-signed certs?

kubernetes-sigs/metrics-server#37 (comment)

diegs · 2018-01-30T18:49:52Z

retest this please. azure and bare metal.

diegs · 2018-01-30T19:51:10Z

All tests passed!

diegs · 2018-01-31T18:26:01Z

May I please get a review on the installer side? @enxebre? @squat?

ericchiang · 2018-02-01T06:29:35Z

We probably want to provide the self-signed certs through a secret.

Mentioned this on JIRA, but I'm going to follow up on kubernetes/kubernetes#54960 to see if there are any specific concerns around this strategy. I'm not familiar with the subtitles.

enxebre · 2018-02-01T08:52:37Z

platforms/aws/tectonic.tf

+  aggregator_ca_cert_pem   = "${module.kube_certs.aggregator_ca_cert_pem}"
+  apiserver_cert_pem       = "${module.kube_certs.apiserver_cert_pem}"
+  apiserver_key_pem        = "${module.kube_certs.apiserver_key_pem}"
+  apiserver_proxy_cert_pem = "${module.kube_certs.apiserver_cert_pem}"


wouldn't this need to be module.kube_certs.apiserver_proxy_cert_pem and module.kube_certs.apiserver_proxy_key_pem? same question for all platforms

Great catch, thank you! Changed to the correct value.

enxebre · 2018-02-01T08:53:04Z

modules/tls/kube/self-signed/aggregator_ca.tf

@@ -0,0 +1,35 @@
+# Kubernetes Aggregated API Server CA (resources/generated/tls/aggregator-ca.crt)
+#
+# TODO(diegs): this should be a sibling of the `--tls-ca-file` CA. However the


shall we add same todo in contrib module?

diegs · 2018-02-02T20:23:26Z

retest this please. govcloud and azure.

diegs · 2018-02-02T21:19:49Z

govcloud:

Error: Error applying plan:

1 error(s) occurred:

* aws_vpn_gateway.vpg (destroy): 1 error(s) occurred:

* aws_vpn_gateway.vpg: IncorrectState: The VPN gateway is in use.
	status code: 400, request id: 1d88c56e-d27f-4c6f-a01e-7f6e83624ab8

azure:

some pods never came up.

diegs · 2018-02-02T21:19:54Z

retest this please.

diegs · 2018-02-02T21:48:32Z

I give up.

cpanato · 2018-02-03T11:41:23Z

all green now @diegs

squat · 2018-02-05T14:37:07Z

modules/tls/kube/self-signed/outputs.tf

+  value = "${tls_locally_signed_cert.aggregator_ca.cert_pem}"
+}
+
+output "aggregator_ca_key_pem" {


I'm curious: why does this module output an aggregator_ca_key but not the tls/kube/user-provided?

It seems like this output is not consumed anywhere either.

You're right, it's not a required output. Got copy-paste happy. Removed.

squat · 2018-02-05T14:40:23Z

contrib/user-provided-certs/kube/self-signed/aggregator_ca.tf

+    "digital_signature",
+    "client_auth",
+  ]
+}


Should this module also create local files for the aggregated ca key and crt just like modules/tls/kube/self-signed does? i.e.:

resource "local_file" "aggregator_ca_key" { ... } resource "local_file" "aggregator_ca_crt" { ... }

diegs

@squat thanks for catching those mistakes, should be fixed now.

diegs · 2018-02-06T05:16:56Z

modules/tls/kube/self-signed/outputs.tf

+  value = "${tls_locally_signed_cert.aggregator_ca.cert_pem}"
+}
+
+output "aggregator_ca_key_pem" {


You're right, it's not a required output. Got copy-paste happy. Removed.

diegs · 2018-02-06T05:19:44Z

contrib/user-provided-certs/kube/self-signed/aggregator_ca.tf

+    "digital_signature",
+    "client_auth",
+  ]
+}


squat · 2018-02-06T14:33:38Z

dang, we have some merge conflicts. Conflicts are only on markdown files. All tests have passed for TF. I'm removing the labels and pushing an extra commit to fix the conflicts with the docs.

cpanato · 2018-02-06T14:35:04Z

ok

This commit ports the documentation that was originally part of coreos/tectonic-installer#2850.

This adds a new CA and certificate pair to support running aggregated api-servers in the cluster. This is required for deploying metrics-server (heapster replacement) and other aggregated api-server components that will start to appear in 1.9+. In the self-signed scenario these are derived from the kube_ca, which happens to also be the apiserver serving ca. In practice the kube_ca should be the parent of both this CA and the one that is used by the --tls-ca-file flag in the apiserver. I added a TODO to fix that when we implement the "single CA is the parent of all generated certs" change for track 2.

squat · 2018-02-06T14:57:15Z

@diegs: the tls module docs you wrote have been PRd to coreos/tectonic-docs#135 now that they no longer exist in this repo since #2678

diegs · 2018-02-06T16:34:42Z

@squat thanks!

tls: add aggregated apiserver certs.

diegs added platform/azure platform/aws platform/bare-metal run-smoke-tests platform/govcloud labels Jan 29, 2018

diegs self-assigned this Jan 29, 2018

diegs requested review from sym3tri, enxebre and ericchiang January 29, 2018 20:28

ericchiang reviewed Jan 29, 2018

View reviewed changes

diegs requested a review from squat January 30, 2018 01:50

diegs removed platform/aws platform/govcloud labels Jan 30, 2018

enxebre reviewed Feb 1, 2018

View reviewed changes

squat reviewed Feb 5, 2018

View reviewed changes

diegs commented Feb 6, 2018

View reviewed changes

diegs added platform/aws platform/bare-metal labels Feb 6, 2018

squat removed platform/bare-metal platform/aws labels Feb 6, 2018

squat previously approved these changes Feb 6, 2018

View reviewed changes

squat removed platform/azure platform/govcloud run-smoke-tests labels Feb 6, 2018

squat added a commit to squat/tectonic-docs that referenced this pull request Feb 6, 2018

Documentation/tls: add aggregated CA

15f641d

This commit ports the documentation that was originally part of coreos/tectonic-installer#2850.

squat mentioned this pull request Feb 6, 2018

Documentation/tls: add aggregated CA coreos/tectonic-docs#135

Merged

squat dismissed their stale review via acbf516 February 6, 2018 14:54

squat approved these changes Feb 6, 2018

View reviewed changes

squat merged commit 96ca618 into coreos:master Feb 6, 2018

diegs deleted the crt branch February 6, 2018 16:34

abhinavdahiya pushed a commit to abhinavdahiya/tectonic-installer that referenced this pull request Feb 6, 2018

Merge pull request coreos#2850 from diegs/crt

0190675

tls: add aggregated apiserver certs.

abhinavdahiya mentioned this pull request Feb 6, 2018

Kubelet tls bootstrap #2809

Merged

aaronlevy mentioned this pull request Mar 19, 2018

Setting requestheader flags on kube-apiserver #2234

Closed

tls: add aggregated apiserver certs. #2850

tls: add aggregated apiserver certs. #2850

Conversation

diegs commented Jan 29, 2018

diegs commented Jan 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegs commented Jan 29, 2018

diegs commented Jan 29, 2018

diegs commented Jan 30, 2018

diegs commented Jan 30, 2018

diegs commented Jan 30, 2018

brancz commented Jan 30, 2018

ericchiang commented Jan 30, 2018

brancz commented Jan 30, 2018

diegs commented Jan 30, 2018

diegs commented Jan 30, 2018

diegs commented Jan 30, 2018

diegs commented Jan 31, 2018

ericchiang commented Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegs commented Feb 2, 2018

diegs commented Feb 2, 2018

diegs commented Feb 2, 2018

diegs commented Feb 2, 2018

cpanato commented Feb 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squat commented Feb 6, 2018

cpanato commented Feb 6, 2018

squat commented Feb 6, 2018

diegs commented Feb 6, 2018

ericchiang commented Feb 1, 2018 •

edited

Loading