Skip to content
This repository has been archived by the owner on Feb 5, 2020. It is now read-only.

*: fix cert and env binding for k8s-node-boostrapper #2757

Closed
wants to merge 1 commit into from

Conversation

squat
Copy link
Contributor

@squat squat commented Jan 18, 2018

Cherry-pick #2460 into track-1
Fixes: https://jira.coreos.com/browse/INST-859

This change adds a bindmount of /etc/ssl
It will also load /etc/profile.env into the env to
The /etc/profile.env file is populated by ignition
based on: https://tinyurl.com/zme96wx
that we can operate in proxy environments.

cc @lucab @crawford @coresolve

@squat
Copy link
Contributor Author

squat commented Jan 18, 2018

As written in #2460, all tests fail because /etc/profile.env does not exist, so systemd fails to start k8s-node-bootstrapper, so kubelet.env is never created, so kubelet fails to start. kubelet.service:

Jan 18 11:49:08 ip-10-0-4-128 systemd[1]: kubelet.service: Failed to load environment files: No such file or directory
Jan 18 11:49:08 ip-10-0-4-128 systemd[1]: kubelet.service: Failed to run 'start-pre' task: No such file or directory
Jan 18 11:49:08 ip-10-0-4-128 systemd[1]: Failed to start Kubelet via Hyperkube ACI.
Jan 18 11:49:08 ip-10-0-4-128 systemd[1]: kubelet.service: Unit entered failed state.
Jan 18 11:49:08 ip-10-0-4-128 systemd[1]: kubelet.service: Failed with result 'resources'.
Jan 18 11:49:18 ip-10-0-4-128 systemd[1]: kubelet.service: Service hold-off time over, scheduling restart.
Jan 18 11:49:18 ip-10-0-4-128 systemd[1]: Stopped Kubelet via Hyperkube ACI.

@squat squat force-pushed the k8s_proxy branch 4 times, most recently from c6eb280 to c830fc0 Compare January 18, 2018 17:29
@squat
Copy link
Contributor Author

squat commented Jan 20, 2018

ok to test

@@ -24,6 +25,7 @@ ExecStartPre=/usr/bin/docker run --rm \
-v /etc/coreos:/etc/coreos:ro \
-v /etc/torcx:/etc/torcx \
-v /etc/kubernetes:/etc/kubernetes \
-v /etc/ssl:/etc/ssl:ro \
Copy link
Contributor

@coresolve coresolve Jan 22, 2018

Choose a reason for hiding this comment

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

Turns out this needs to be /etc/ssl/certs/ca-certificates.crt

The go binary can't read all the certs it needs the cert bundle specifically.
We need /etc/ssl/certs/ca-certificates.crt as opposed to /usr/share/certs/ca-certificates.crt as we need to be able to trust certificates that are added to the bundle at install time. The /usr/share/cert I think is immutable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was cherry-picked from your PR. Should I make that change for you?

Copy link
Contributor

@coresolve coresolve left a comment

Choose a reason for hiding this comment

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

Need to change the cert path.

@squat
Copy link
Contributor Author

squat commented Jan 23, 2018

ping @coresolve

This change adds a bindmount of /etc/ssl
It will also load /etc/profile.env into the env to
The /etc/profile.env file is populated by ignition
based on: https://tinyurl.com/zme96wx
that we can operate in proxy environments.
@coresolve
Copy link
Contributor

The change looks good. I think I am still trying to figure out where to get the proxy env vars from. I was using /etc/profile.env because of this guidance:
https://coreos.com/os/docs/latest/using-environment-variables-in-systemd-units.html#system-wide-environment-variables

With this doc floating around I think we need to agree on an approach.

ExecStartPre=/usr/bin/docker run --rm \
--env-file /etc/profile.env \
Copy link
Contributor

@euank euank Jan 23, 2018

Choose a reason for hiding this comment

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

Would it be bettre to use --env HTTP_PROXY --env NO_PROXY (and any other proxy-related variables; those are the ones the go stdlib understands iirc)?

The profile.env file is meant for full shell parsing and for interactive user shells, so it's perfectly possible a user will, for valid reasons, have set it to include more complex statements than docker's --env-file may understand.

Minimizing the set of environment variables getting passed through is also good from a security perspective.

With the --env suggestion above, using a dropin for specifically this service or globally via /etc/systemd/system.conf.d/ should work, and it won't require touching anything in the default case since docker doesn't error if an --env value doesn't exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

the problem tho is that the docker container needs the env vars. I am fine with pointing at a diff file. In most cases I am blindly expecting the env vars in /etc/profile.env to apply cleanly here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this makes sense. Basically extend the k8s-node-bootstrapper.service with the proxy vars and feed them in with --env This is probably cleaner cause the --env-file doesn't have to exist.

@sym3tri
Copy link
Contributor

sym3tri commented Jan 24, 2018

This is planned to be superseded by forthcoming PRs.

@sym3tri sym3tri closed this Jan 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants