-
Notifications
You must be signed in to change notification settings - Fork 266
modules/*: trust CA certificates on the nodes #2362
Conversation
Can one of the admins verify this patch? |
2740e4e
to
552ecfd
Compare
@squat PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great overall. I found one issue that I think breaks GCP. PTAL
modules/gcp/worker-igm/ignition.tf
Outdated
@@ -3,6 +3,7 @@ data "ignition_config" "main" { | |||
"${data.ignition_file.kubeconfig.id}", | |||
"${var.ign_max_user_watches_id}", | |||
"${var.ign_installer_kubelet_env_id}", | |||
"${var.ign_ca_cert_id_list}", | |||
] | |||
|
|||
systemd = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The worker is missing the var.ign_update_ca_certificates_dropin_id
service
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, thanks!
@@ -38,6 +38,11 @@ systemd: | |||
- name: bootkube.service | |||
enable: false | |||
contents: {{.ign_bootkube_service_json}} | |||
- name: update-ca-certificates.service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not for this pr but we really need to refactor this platform so that it uses the same ignition format as all the other platforms
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totally agreed 👍
552ecfd
to
c82dfb9
Compare
ok to test |
c82dfb9
to
bcffef6
Compare
This builds on top of #2421 |
This also bumps the ignition provider to 1.0.0 to support S3 downloads via ignition. Fixes #INST-67
Currently etcd certificate ignition files are created in their respective platform module. This unifies it by declaring the ignition file units centrally in the ignition module.
Currently the etcd TLS module also generates a zip file which is only used on AWS to reduce the userdata size to be <20k (hard limit on AWS). Since the etcd TLS assets will be provisioned via S3 this optimization/hack is not needed any more.
We hit the limits of the AWS userdata limit (20k) constantly. This fixes it by provisioning a minimal ignition configuration only which points to a replacement ignition configuration hosted on S3. This also removes workarounds/hacks to keep the userdata size small, especially for provisioning TLS assets on etcd nodes.
bcffef6
to
6d66b86
Compare
retest this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once green
|
||
[Service] | ||
ExecStart= | ||
ExecStart=/usr/sbin/update-ca-certificates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @lucab This works, because we are enforcing rehashing all files. Rather the default service should work, but openssl is pointing to the wrong default bundle.
all green, merging |
"Action": [ | ||
"ecr:DescribeRepositories", | ||
"ecr:ListImages", | ||
"ecr:BatchGetImage" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@s-urbaniak Why did you need to add ECR permissions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might've been a copy&pasta oversight, but without this the machine didn't seem to boot.
* modules/*: trust CA certificates on the nodes This also bumps the ignition provider to 1.0.0 to support S3 downloads via ignition. Fixes #INST-67 * modules/ignition: unify etcd certificate generation Currently etcd certificate ignition files are created in their respective platform module. This unifies it by declaring the ignition file units centrally in the ignition module. * modules/tls/etcd: remove zip generation Currently the etcd TLS module also generates a zip file which is only used on AWS to reduce the userdata size to be <20k (hard limit on AWS). Since the etcd TLS assets will be provisioned via S3 this optimization/hack is not needed any more. * */aws: use S3 for ignition provisioning We hit the limits of the AWS userdata limit (20k) constantly. This fixes it by provisioning a minimal ignition configuration only which points to a replacement ignition configuration hosted on S3. This also removes workarounds/hacks to keep the userdata size small, especially for provisioning TLS assets on etcd nodes. * Documentation/examples: regenerate * */azure: use unified etcd TLS ignition files * */gcp: use unified etcd TLS ignition files * */openstack: use unified etcd TLS ignition files * */vmware: use unified etcd TLS ignition files
* modules/*: trust CA certificates on the nodes This also bumps the ignition provider to 1.0.0 to support S3 downloads via ignition. Fixes #INST-67 * modules/ignition: unify etcd certificate generation Currently etcd certificate ignition files are created in their respective platform module. This unifies it by declaring the ignition file units centrally in the ignition module. * modules/tls/etcd: remove zip generation Currently the etcd TLS module also generates a zip file which is only used on AWS to reduce the userdata size to be <20k (hard limit on AWS). Since the etcd TLS assets will be provisioned via S3 this optimization/hack is not needed any more. * */aws: use S3 for ignition provisioning We hit the limits of the AWS userdata limit (20k) constantly. This fixes it by provisioning a minimal ignition configuration only which points to a replacement ignition configuration hosted on S3. This also removes workarounds/hacks to keep the userdata size small, especially for provisioning TLS assets on etcd nodes. * Documentation/examples: regenerate * */azure: use unified etcd TLS ignition files * */gcp: use unified etcd TLS ignition files * */openstack: use unified etcd TLS ignition files * */vmware: use unified etcd TLS ignition files
@@ -0,0 +1,25 @@ | |||
resource "aws_s3_bucket_object" "ignition_etcd" { | |||
count = "${length(var.external_endpoints) == 0 ? var.instance_count : 0}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm new to Terraform and was scratching my head about this after not finding count
in the aws_s3_bucket_object
docs. It turns out it is a meta-parameter (more here). I thought I'd add this note in case it helps any other Terraform newbies ;).
The local variable is from cd71f30 (Ignition assets, 2018-02-14, coreos/tectonic-installer#2940), but it doesn't scale beyond three etcd nodes. This commit pivots to an approach that scales to an arbitrary number of nodes, matching the scalable generation logic we've had since 0c7ac90 (modules/*: trust CA certificates on the nodes, 2017-11-22, coreos/tectonic-installer#2362).
The local variable is from cd71f30 (Ignition assets, 2018-02-14, coreos/tectonic-installer#2940), but it doesn't scale beyond three etcd nodes. This commit pivots to an approach that scales to an arbitrary number of nodes, matching the scalable generation logic we've had since 0c7ac90 (modules/*: trust CA certificates on the nodes, 2017-11-22, coreos/tectonic-installer#2362).
This also bumps the ignition provider to 1.0.0 to support S3 downloads
via ignition.
Fixes #INST-67