-
Notifications
You must be signed in to change notification settings - Fork 266
Conversation
Can one of the admins verify this patch? |
1 similar comment
Can one of the admins verify this patch? |
platforms/metal/variables.tf
Outdated
@@ -185,3 +185,8 @@ SSH public key to use as an authorized key. | |||
Example: `ssh-rsa AAAB3N...` | |||
EOF | |||
} | |||
|
|||
variable "tectonic_ssh_private_key" { |
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 believe this is unrelated to this PR.
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.
Additionally I would like to avoid encoding private keys in the state. As you are using this currently only to transfer assets I would prefer not using the private key but rather rely on the host's SSH agent.
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.
oop, good call. That bled over from the rebase I was doing for #625. I'll pull that out.
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.
Thanks a lot for the PR! I have two questions/comments though.
modules/openstack/nodes/ignition.tf
Outdated
|
||
[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.
That would execute update-ca-certificates
at every boot, no? I would like to recheck with the OS team (/cc @crawford @euank) whether this is the canonical way of adding public CAs using ignition as I think https://github.com/coreos/coreos-overlay/blob/master/app-misc/ca-certificates/files/update-ca-certificates.service#L7 does this already at first boot.
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.
Sounds good. As it currently is, the ConditionPathIsSymbolicLink
and the --skip-rehash
can cause it not to pick up the new certificate, so I removed those, but I'm open to a more canonical way.
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.
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.
Apologies for not responding; this dropped off my radar.
Do you know of any specific cases where the existing ExecStart
line actually does miss new certificates? It looks like it should generally do the right thing already with its timestamp comparison.
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 not 100% clear on the reasoning, but it definitely requires both to get this to work. Here's what happens if I remove the drop-in, as well as with the drop-in changes:
# This is what we're looking for in the CA list
core@terraform-master-0 ~ $ sudo cat /etc/ssl/certs/kube_ca.pem | openssl x509 -noout -subject
subject= /C=/ST=/L=/postalCode=/O=bootkube/OU=/CN=kube-ca
# update-ca-certificates didn't run because of the precondition (maybe already ran during the image build?)
core@terraform-master-0 ~ $ systemctl status update-ca-certificates
● update-ca-certificates.service - Update CA bundle at /etc/ssl/certs/ca-certificates.crt
Loaded: loaded (/usr/lib/systemd/system/update-ca-certificates.service; static; vendor preset: disabled)
Active: inactive (dead)
Condition: start condition failed at Wed 2017-07-26 19:33:02 UTC; 18min ago
└─ ConditionPathIsSymbolicLink=!/etc/ssl/certs/ca-certificates.crt was not met
core@terraform-master-0 ~ $ sudo mkdir /etc/systemd/system/update-ca-certificates.service.d/
# Just reset ConditionPathIsSymbolicLink in the drop-in
core@terraform-master-0 ~ $ sudo vi /etc/systemd/system/update-ca-certificates.service.d/edits.conf
core@terraform-master-0 ~ $ sudo systemctl daemon-reload
core@terraform-master-0 ~ $ sudo systemctl start update-ca-certificates
# update-ca-certificates runs now but doesn't do much
core@terraform-master-0 ~ $ systemctl status update-ca-certificates
● update-ca-certificates.service - Update CA bundle at /etc/ssl/certs/ca-certificates.crt
Loaded: loaded (/usr/lib/systemd/system/update-ca-certificates.service; static; vendor preset: disabled)
Drop-In: /etc/systemd/system/update-ca-certificates.service.d
└─edits.conf
Active: inactive (dead) since Wed 2017-07-26 20:06:42 UTC; 6s ago
Process: 6327 ExecStart=/usr/sbin/update-ca-certificates --skip-rehash (code=exited, status=0/SUCCESS)
Main PID: 6327 (code=exited, status=0/SUCCESS)
CPU: 34ms
Jul 26 20:06:42 terraform-master-0 systemd[1]: Starting Update CA bundle at /etc/ssl/certs/ca-certificates.crt...
Jul 26 20:06:42 terraform-master-0 update-ca-certificates[6327]: Recreating certificate bundle /etc/ssl/certs/ca-certificates.crt
Jul 26 20:06:42 terraform-master-0 systemd[1]: Started Update CA bundle at /etc/ssl/certs/ca-certificates.crt.
# Still no certificate in the list
core@terraform-master-0 ~ $ awk -v cmd='openssl x509 -noout -subject' '/BEGIN/{close(cmd)};{print | cmd}' < /etc/ssl/certs/ca-certificates.crt | grep kube
# Change the ExecStart too
core@terraform-master-0 ~ $ sudo vi /etc/systemd/system/update-ca-certificates.service.d/edits.conf
core@terraform-master-0 ~ $ sudo systemctl daemon-reload
core@terraform-master-0 ~ $ sudo systemctl start update-ca-certificates
core@terraform-master-0 ~ $ systemctl status update-ca-certificates
● update-ca-certificates.service - Update CA bundle at /etc/ssl/certs/ca-certificates.crt
Loaded: loaded (/usr/lib/systemd/system/update-ca-certificates.service; static; vendor preset: disabled)
Drop-In: /etc/systemd/system/update-ca-certificates.service.d
└─edits.conf
Active: inactive (dead) since Wed 2017-07-26 20:07:43 UTC; 2min 52s ago
Process: 6691 ExecStart=/usr/sbin/update-ca-certificates (code=exited, status=0/SUCCESS)
Main PID: 6691 (code=exited, status=0/SUCCESS)
CPU: 5.597s
Jul 26 20:07:43 terraform-master-0 update-ca-certificates[6691]: WoSign_China.pem => 5d63b0ae.0
Jul 26 20:07:43 terraform-master-0 update-ca-certificates[6691]: XRamp_Global_CA_Root.pem => 706f604c.0
Jul 26 20:07:43 terraform-master-0 update-ca-certificates[6691]: certSIGN_ROOT_CA.pem => 8d86cdd1.0
Jul 26 20:07:43 terraform-master-0 update-ca-certificates[6691]: ePKI_Root_Certification_Authority.pem => ca6e4ad9.0
Jul 26 20:07:43 terraform-master-0 update-ca-certificates[6691]: kube_ca.pem => dc214808.0
Jul 26 20:07:43 terraform-master-0 update-ca-certificates[6691]: thawte_Primary_Root_CA.pem => 2e4eed3c.0
Jul 26 20:07:43 terraform-master-0 update-ca-certificates[6691]: thawte_Primary_Root_CA_-_G2.pem => c089bbbd.0
Jul 26 20:07:43 terraform-master-0 update-ca-certificates[6691]: thawte_Primary_Root_CA_-_G3.pem => ba89ed3b.0
Jul 26 20:07:43 terraform-master-0 update-ca-certificates[6691]: Recreating certificate bundle /etc/ssl/certs/ca-certificates.crt
Jul 26 20:07:43 terraform-master-0 systemd[1]: Started Update CA bundle at /etc/ssl/certs/ca-certificates.crt.
# There it is
core@terraform-master-0 ~ $ awk -v cmd='openssl x509 -noout -subject' '/BEGIN/{close(cmd)};{print | cmd}' < /etc/ssl/certs/ca-certificates.crt | grep kube
subject= /C=/ST=/L=/postalCode=/O=bootkube/OU=/CN=kube-ca
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.
Thanks for checking, sorry for the bit of a runaround there.
This still seems a bit sloppy, but since I don't have a better suggestion, I'm okay with it.
platforms/metal/variables.tf
Outdated
@@ -185,3 +185,8 @@ SSH public key to use as an authorized key. | |||
Example: `ssh-rsa AAAB3N...` | |||
EOF | |||
} | |||
|
|||
variable "tectonic_ssh_private_key" { |
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.
Additionally I would like to avoid encoding private keys in the state. As you are using this currently only to transfer assets I would prefer not using the private key but rather rely on the host's SSH agent.
@@ -101,6 +101,23 @@ storage: | |||
contents: | |||
inline: | | |||
fs.inotify.max_user_watches=16184 | |||
- path: /etc/systemd/system/update-ca-certificates.service.d/10-alwaysrun.conf |
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.
This should be part of the systemd:
section, not the storage:
section
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.
Oop, good catch. Moved in the controller too.
modules/openstack/nodes/ignition.tf
Outdated
|
||
[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.
Thanks for checking, sorry for the bit of a runaround there.
This still seems a bit sloppy, but since I don't have a better suggestion, I'm okay with it.
coreos#1351 From 013-install-kube-ca.patch
@coreypobrien @s-urbaniak Is this good to go after a rebase? |
Can one of the admins verify this patch? |
1 similar comment
Can one of the admins verify this patch? |
@lblackstone Rebased and should be good to go. Thanks! |
@s-urbaniak Any further objections? |
Ping @s-urbaniak |
Can one of the admins verify this patch? |
1 similar comment
Can one of the admins verify this patch? |
@lblackstone thanks a lot for the patience. This generally LGTM to me. Note though, that we did a major refactoring which unifies ignition across all platforms in #1743. I would therefore advocate to put all the systemd units in the shiny new |
@s-urbaniak No problem, I understand things are moving fast! :) @coreypobrien Given the extra effort required at this point, want me to take over this PR? |
@lblackstone thanks a lot! 👍 |
@lblackstone I think I've got it rebased, but I haven't had a chance to test any of the platforms yet. Let's see how CI looks and if you wouldn't mind testing OpenStack that'd be great. |
@@ -28,6 +40,22 @@ data "ignition_systemd_unit" "docker_dropin" { | |||
] | |||
} | |||
|
|||
data "template_file" "update_ca_certificates_dropin" { | |||
template = "${file("${path.module}/resources/dropins/10-dockeropts.conf")}" |
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 believe this should be 10-always-update-ca-certificates.conf
, no?
just fyi: I am actively working on this. My plan is to extend the functionality of this PR and not only inject the kubelet CA, but also any number of additional CA files. |
@s-urbaniak Thanks for the info. I haven't had a chance to test this out anyway, so I'll just let you run with your changes. |
@s-urbaniak Is this change in place now? Looks like d38ebc9 may have included it? |
Can one of the admins verify this patch? |
1 similar comment
Can one of the admins verify this patch? |
@s-urbaniak #2362 fixes this, correct? |
@lblackstone correct, closing in favor of #2362 |
Fixes #736
This is specifically useful for running a private Docker registry that uses the same signing chain as Kubernetes since Docker uses the system certificate store and needs to trust the CA for the registry.
Because Docker uses the system certificate store, installing the certificate during the ignition process avoids needing to restart Docker to pick up the changes later.
I've only manually tested the
openstack/neutron
andmetal
platforms. Theaws
,azure
, andvmware
changes were just a copy-paste fromopenstack/neutron
, but if someone could check those more closely I'd appreciate it.