*: add support for journal-gatewayd #967

crawford · 2018-12-20T23:34:30Z

This allows the user to stream logs from the bootstrap node without the
need for SSH. This read-only view into the bootstrap process is
convenient for those who wish not to use SSH in their cluster. Right
now, this requires the user to manually invoke curl to fetch the logs,
but this could be done automatically by the installer in the future.

abhinavdahiya · 2018-12-21T00:15:09Z

pkg/asset/ignition/bootstrap/bootstrap.go

@@ -234,23 +234,68 @@ func (a *Bootstrap) addSystemdUnits(uri string, templateData *bootstrapTemplateD
 	}

 	for _, childInfo := range children {
-		name := childInfo.Name()
-		file, err := data.Assets.Open(path.Join(uri, name))
+		dir := path.Join(uri, childInfo.Name())


Is it possible to add a unit test to test this part of the code?

pkg/asset/cluster/cluster.go

abhinavdahiya · 2018-12-21T00:19:37Z

data/data/bootstrap/systemd/units/systemd-journal-gatewayd.service.d/certs.conf

+[Service]
+ExecStart=
+ExecStart=/usr/lib/systemd/systemd-journal-gatewayd \
+  --key=/opt/openshift/tls/journal-gatewayd-ca.key \


since we are using the same set of cert/key pair for server,client-authn and client-authz can we drop the -ca

abhinavdahiya · 2018-12-21T00:22:14Z

pkg/asset/ignition/bootstrap/bootstrap.go

@@ -357,6 +359,10 @@ func (a *Bootstrap) addParentFiles(dependencies asset.Parents) {
 		dependencies.Get(asset)
 		a.Config.Storage.Files = append(a.Config.Storage.Files, ignition.FilesFromAsset(rootDir, 0600, asset)...)
 	}
+
+	journalCertKey := &tls.JournalCA{}


this feels out of place in addSystemdUnits it seems like it belongs here 907a22d#diff-2d9b655c8052ca2e3bdd9004778c75bcR357

It can't go there because the key and cert need to be readable by journal-gatewayd (which doesn't run as root). I should probably also change the owner of these files... I hope that works on RHCOS...

wking · 2019-01-04T04:42:32Z

Can you pull 0bc0117 out into it's own PRs (and maybe split the auto-generated SVG rebuild into a separate commit?)? Those look like orthogonal changes that we can land without further discussion (once CI gets working again).

pkg/asset/ignition/bootstrap/bootstrap.go

wking · 2019-01-04T04:53:41Z

pkg/asset/ignition/bootstrap/bootstrap.go

-		unit := igntypes.Unit{Name: name, Contents: string(data)}
-		if _, ok := enabled[name]; ok {
-			unit.Enabled = util.BoolToPtr(true)
+		if info.IsDir() {


Can we add a check here that says "if the directory name doesn't end with .d we ignore it" (possibly with a debug-level log)? From the systemd docs, it looks like they require all drop-in directories to have a .d suffix. But I'm not all that familiar with systemd, maybe I'm just misreading the docs.

I'm going to add a trace-level log line for that case. Trace should really only be used by developers which seems appropriate for this kind of message. Should I mention the "trace" level in the help output, or just keep that goodie for ourselves?

Should I mention the "trace" level in the help output, or just keep that goodie for ourselves?

I'd mention it. Users should recognize it as "the level after debug" even if they're non-devs, so I don't expect it will be too distracting.

wking · 2019-01-04T05:00:20Z

docs/user/troubleshooting.md

-journalctl --unit=bootkube.service
-```
+1. If SSH is available, the following command can be run on the bootstrap node: `journalctl --unit=bootkube.service`
+2. Regardless of whether or not SSH is available, the following command can be run: `curl --insecure --cert ${INSTALL_DIR}/tls/journal-gatewayd-client.crt --key ${INSTALL_DIR}/tls/journal-gatewayd-client.key 'https://${BOOTSTRAP_IP}:19531/entries?follow&_SYSTEMD_UNIT=bootkube.service'`


Should the installer connect to this automatically and stream it into the installer logs (like we do for Terraform) instead of polling for the API? I'm fine leaving that to follow-up work, but I don't think we need to document this manual approach if we're planning on adding a built-in approach in the future.

That's a good question. I'd prefer to always stream these logs, but I'm not sure how to handle the case where a network connection is not available. Do we just optimistically stream them and ignore it otherwise?

... but I'm not sure how to handle the case where a network connection is not available.

We currently rely on network connections to the load balancers for our Kubernetes API and event waits. When those work, I expect this access to the bootstrap journal will usually work as well. When it doesn't (access from inside a restrictive firewall?), I'm fine logging a warning and falling back to the Kubernetes API wait.

I once wrote a simple package to talk to gatewayd, in case you ever do want to do that: coreos/go-systemd#86

Also: Having an installer sub-command to that does the equivalent curl would be pretty nice.

pkg/asset/ignition/bootstrap/bootstrap.go

crawford · 2019-01-04T17:57:40Z

Can you pull 0bc0117 out into it's own PRs

@wking done in #993.

crawford · 2019-01-10T21:57:09Z

Let's see if the registry gateway issue has been resolved...
/retest

wking · 2019-01-10T23:45:08Z

pkg/asset/ignition/bootstrap/bootstrap.go

@@ -205,7 +208,7 @@ func (a *Bootstrap) addStorageFiles(base string, uri string, templateData *boots
 	} else {
 		mode = 0600
 	}
-	ign := ignition.FileFromBytes(strings.TrimSuffix(base, ".template"), mode, data)
+	ign := ignition.FileFromBytes(strings.TrimSuffix(base, ".template"), "root", mode, data)
 	if filename == ".bash_history" {
 		ign.User = &igntypes.NodeUser{Name: ignitionUser}


You could set a variable for the user name and override for .bash_history before feeding into FileFromBytes. But certainly not a blocker; we can always do something like that later if we want ;).

crawford · 2019-01-11T04:14:15Z

/retest

crawford · 2019-01-11T15:28:39Z

2019/01/11 05:24:22 Build installer succeeded after 30m50s
2019/01/11 05:24:23 Ran for 37m45s
error: could not run steps: could not copy stable imagestreamtag: Timeout: request did not complete within allowed duration

That's not one I've seen before, but it sounds related to the recent registry failures we've seen.

/retest

sttts · 2019-01-11T16:10:56Z

This looks awesome!

crawford · 2019-01-11T16:11:19Z

level=error msg="Error: Error applying plan:"
level=error
level=error msg="1 error occurred:"
level=error msg="\t* aws_route53_record.etcd_a_nodes[2]: 1 error occurred:"
level=error msg="\t* aws_route53_record.etcd_a_nodes.2: [ERR]: Error building changeset: timeout while waiting for state to become 'accepted' (timeout: 5m0s)"

/retest

This allows systemd drop-ins to be included in the bootstrap data directory. The drop-ins must be located under an appropriately-named directory, the same way that systemd requires on the host.

This gives the ability to specify the owner of the files. So far, only the root user is used, but support for other users will be needed in order to support journal-gatewayd. This service doesn't run as root, so in order to read its TLS assets, they either need to be world-readable or they need to be owned by the correct user.

The key for the root CA should not end up in the cluster. This is most easily prevented by ensuring that it doesn't end up on the bootstrap node at all. This change also makes the root CA certificate world-readable.

This allows the user to stream logs from the bootstrap node without the need for SSH. This read-only view into the bootstrap process is convenient for those who wish not to use SSH in their cluster. Right now, this requires the user to manually invoke curl to fetch the logs, but this could be done automatically by the installer in the future.

crawford · 2019-01-11T18:49:28Z

Rebased onto master. @wking can I get another one of those /lgtms?

abhinavdahiya · 2019-01-11T18:50:22Z

/lgtm

openshift-ci-robot · 2019-01-11T18:50:38Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,crawford]

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

wking · 2019-01-12T04:37:49Z

@sferich888, there were some troubleshooting-doc changes here.

sferich888 · 2019-01-12T11:20:47Z

data/data/bootstrap/systemd/units/systemd-journal-gatewayd.service.d/certs.conf

@@ -0,0 +1,6 @@
+[Service]
+ExecStart=
+ExecStart=/usr/lib/systemd/systemd-journal-gatewayd \


Where is this service defined? Where is it's source.

Where is this service defined? Where is it's source.

The service this is a drop-in for? I'd guess in a package RHCOS pulls in.

I thought this was offered by the kubelet? And it's debug port.

This is journal-gatewayd, which is included (at my request) in RHCOS.

wking · 2019-01-12T15:47:39Z

data/data/bootstrap/systemd/units/systemd-journal-gatewayd.service.d/certs.conf

@@ -0,0 +1,6 @@
+[Service]
+ExecStart=


Should this line be dropped?

All systemd directives act as lists and each entry adds another item to the list. Simple services (like journal-gatewayd) can only have a single item for ExecStart, so we have to clear the list with an empty entry.

openshift-ci-robot requested review from flaper87 and tomassedovic December 20, 2018 23:34

abhinavdahiya reviewed Dec 21, 2018

View reviewed changes

pkg/asset/cluster/cluster.go Outdated Show resolved Hide resolved

abhinavdahiya reviewed Dec 21, 2018

View reviewed changes

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 4, 2019

wking reviewed Jan 4, 2019

View reviewed changes

pkg/asset/ignition/bootstrap/bootstrap.go Outdated Show resolved Hide resolved

wking reviewed Jan 4, 2019

View reviewed changes

pkg/asset/ignition/bootstrap/bootstrap.go Show resolved Hide resolved

wking mentioned this pull request Jan 4, 2019

e2e-aws: stream bootkube and 'kubectl get all' to artifacts openshift/release#2064

Closed

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 4, 2019

crawford mentioned this pull request Jan 5, 2019

Installer creates instances without SSH key-pair #862

Closed

crawford added this to the Freeze milestone Jan 10, 2019

crawford changed the title ~~WIP: add support for journal-gatewayd~~ *: add support for journal-gatewayd Jan 10, 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 Jan 10, 2019

wking reviewed Jan 10, 2019

View reviewed changes

crawford added 4 commits January 11, 2019 10:48

asset/ignition: allow systemd drop-ins to be used

36e127e

This allows systemd drop-ins to be included in the bootstrap data directory. The drop-ins must be located under an appropriately-named directory, the same way that systemd requires on the host.

asset/*: remove root CA key from bootstrap node

81f4445

The key for the root CA should not end up in the cluster. This is most easily prevented by ensuring that it doesn't end up on the bootstrap node at all. This change also makes the root CA certificate world-readable.

openshift-ci-robot assigned abhinavdahiya Jan 11, 2019

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

openshift-merge-robot merged commit 7880721 into openshift:master Jan 11, 2019

crawford deleted the gatewayd branch January 11, 2019 23:33

wking mentioned this pull request Jan 12, 2019

Official Debugging Instructions (Informational) #1003

Closed

sferich888 reviewed Jan 12, 2019

View reviewed changes

wking reviewed Jan 12, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: add support for journal-gatewayd #967

*: add support for journal-gatewayd #967

crawford commented Dec 20, 2018 •

edited

Loading

abhinavdahiya Dec 21, 2018

abhinavdahiya Dec 21, 2018

abhinavdahiya Dec 21, 2018

crawford Jan 4, 2019

wking commented Jan 4, 2019

wking Jan 4, 2019 •

edited

Loading

crawford Jan 4, 2019 •

edited

Loading

wking Jan 4, 2019

wking Jan 4, 2019

crawford Jan 4, 2019

wking Jan 4, 2019

chancez Jan 8, 2019

crawford commented Jan 4, 2019

crawford commented Jan 10, 2019

wking Jan 10, 2019

crawford commented Jan 11, 2019

crawford commented Jan 11, 2019

sttts commented Jan 11, 2019

crawford commented Jan 11, 2019

crawford commented Jan 11, 2019

abhinavdahiya commented Jan 11, 2019

openshift-ci-robot commented Jan 11, 2019

wking commented Jan 12, 2019

sferich888 Jan 12, 2019

wking Jan 12, 2019

sferich888 Jan 12, 2019

crawford Jan 13, 2019

wking Jan 12, 2019

crawford Jan 13, 2019

		@@ -0,0 +1,6 @@
		[Service]
		ExecStart=

*: add support for journal-gatewayd #967

*: add support for journal-gatewayd #967

Conversation

crawford commented Dec 20, 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

wking commented Jan 4, 2019

wking Jan 4, 2019 • edited Loading

Choose a reason for hiding this comment

crawford Jan 4, 2019 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crawford commented Jan 4, 2019

crawford commented Jan 10, 2019

Choose a reason for hiding this comment

crawford commented Jan 11, 2019

crawford commented Jan 11, 2019

sttts commented Jan 11, 2019

crawford commented Jan 11, 2019

crawford commented Jan 11, 2019

abhinavdahiya commented Jan 11, 2019

openshift-ci-robot commented Jan 11, 2019

wking commented Jan 12, 2019

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

crawford commented Dec 20, 2018 •

edited

Loading

wking Jan 4, 2019 •

edited

Loading

crawford Jan 4, 2019 •

edited

Loading