New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg/daemon: detect ssh accesses before daemon started #381

Merged
merged 1 commit into from Feb 13, 2019

Conversation

Projects
None yet
8 participants
@runcom
Copy link
Member

runcom commented Feb 5, 2019

Reads the journal checking for logind message ID and annotates the node
with the ssh accessed label if it finds out there were previous
accesses.

Close #379

Signed-off-by: Antonio Murdaca runcom@linux.com

@runcom runcom force-pushed the runcom:early-ssh-accesses branch 3 times, most recently from 049ffe4 to ce09d42 Feb 5, 2019

@runcom

This comment has been minimized.

Copy link
Member Author

runcom commented Feb 5, 2019

/test unit

@runcom runcom force-pushed the runcom:early-ssh-accesses branch from ce09d42 to afb2de2 Feb 5, 2019

@runcom

This comment has been minimized.

Copy link
Member Author

runcom commented Feb 5, 2019

so tests all fail because we're missing the systemd-devel dependency, does anybody know how to add that in the CI environment? /cc @cgwalters @jlebon et all

@runcom runcom force-pushed the runcom:early-ssh-accesses branch 3 times, most recently from 93dcf8f to 8613c23 Feb 5, 2019

@jlebon

This comment has been minimized.

Copy link
Member

jlebon commented Feb 5, 2019

I think right now it's failing because it's trying to link it for all the container builds but the dep was only added for the daemon Dockerfiles. @abhinavdahiya mentioned in #335 (comment) that we should only turn on CGO_ENABLED for the MCD. Doing that should fix it, I think?

(Also, can you do the vendoring part as a separate commit; can probably just cherry-pick 1068940 :)).

@cgwalters

This comment has been minimized.

Copy link
Contributor

cgwalters commented Feb 5, 2019

I believe the build container is derived from https://github.com/openshift/release/blob/a30cd48207d5f7244ed492450e111c6fbde5b757/projects/origin-release/golang-1.10/Dockerfile

I'm not sure about precedent for either adding to it or making a new one (or a derived container).

@runcom

This comment has been minimized.

Copy link
Member Author

runcom commented Feb 5, 2019

@abhinavdahiya mentioned in #335 (comment) that we should only turn on CGO_ENABLED for the MCD. Doing that should fix it, I think?

@jlebon I did that but still fails cause we're probably using another Dockerfile and image in tests (???)

(Also, can you do the vendoring part as a separate commit; can probably just cherry-pick 1068940 :)).

@jlebon is dep able to pull in vendors w/o a specific import in the code? in case it's not, I'd just wait for either PR to land and rebase the other one I guess. Otherwise, I'll create a separate commit.

I'm not sure about precedent for either adding to it or making a new one (or a derived container).

thanks, I'll check that out

@cgwalters

This comment has been minimized.

Copy link
Contributor

cgwalters commented Feb 5, 2019

While it may seem like a hack honestly I'd say we just fork off that journalctl command as a subprocess.

@runcom

This comment has been minimized.

Copy link
Member Author

runcom commented Feb 5, 2019

While it may seem like a hack honestly I'd say we just fork off that journalctl command as a subprocess.

that's fine with me, let's see how it goes when adding a build dep on the release image openshift/release#2783

}
// this is so fragile, but from testing, it's pretty accurate as a way
// to detect if there were sessions created before we took over with the daemon
if entry != nil && strings.Contains(entry.Fields["MESSAGE"], "New session") {

This comment has been minimized.

@jlebon

jlebon Feb 5, 2019

Member

We don't even need to check the MESSAGE here. The msg id already represents the event we're looking for.

This comment has been minimized.

@jlebon

jlebon Feb 5, 2019

Member

Also, there's no need for a for loop here, right? We can just .Next() and then .GetEntry() sequentially.

This comment has been minimized.

@runcom

runcom Feb 5, 2019

Author Member

for the second point, just 1 entry is already enough to mark the node ssh/accessed so yeah, no need for a loop or sequentially calling GetEntry

Show resolved Hide resolved pkg/daemon/daemon.go
@jlebon

This comment has been minimized.

Copy link
Member

jlebon commented Feb 5, 2019

@jlebon is dep able to pull in vendors w/o a specific import in the code?

I've just been doing git add vendor/ and committing that separately first. GitHub does a good job of not expanding the vendoring churn in the "Files changed" tab, but in the terminal it's a pain to wade through.

@yuqi-zhang

This comment has been minimized.

Copy link
Contributor

yuqi-zhang commented Feb 5, 2019

Running this locally shows:

# WHAT=machine-config-daemon ./hack/build-go.sh
Using version from git...
Building github.com/openshift/machine-config-operator/cmd/machine-config-daemon (v3.11.0-573-g10b54e42)
# github.com/openshift/machine-config-operator/vendor/github.com/coreos/go-systemd/sdjournal
vendor/github.com/coreos/go-systemd/sdjournal/journal.go:27:11: fatal error: systemd/sd-journal.h: No such file or directory
 // #include <systemd/sd-journal.h>
           ^~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
@runcom

This comment has been minimized.

Copy link
Member Author

runcom commented Feb 5, 2019

yes, we're waiting for openshift/release#2783 or go raw journalctl

@runcom runcom force-pushed the runcom:early-ssh-accesses branch 3 times, most recently from 43060b1 to 26f97f6 Feb 5, 2019

@openshift-merge-robot

This comment has been minimized.

Copy link
Contributor

openshift-merge-robot commented Feb 6, 2019

/retest

1 similar comment
@openshift-merge-robot

This comment has been minimized.

Copy link
Contributor

openshift-merge-robot commented Feb 6, 2019

/retest

@ashcrow

This comment has been minimized.

Copy link
Member

ashcrow commented Feb 7, 2019

@ashcrow

This comment has been minimized.

Copy link
Member

ashcrow commented Feb 7, 2019

/retest

1 similar comment
@ashcrow

This comment has been minimized.

Copy link
Member

ashcrow commented Feb 7, 2019

/retest

@cgwalters

This comment has been minimized.

Copy link
Contributor

cgwalters commented Feb 7, 2019

So rhel-images will fail until the upstream change makes it there. Not sure how long that will take.

@@ -31,4 +31,4 @@ fi
mkdir -p ${BIN_PATH}

echo "Building ${REPO}/cmd/${WHAT} (${VERSION_OVERRIDE})"
CGO_ENABLED=0 GOOS=${GOOS} GOARCH=${GOARCH} go build ${GOFLAGS} -ldflags "${GLDFLAGS}" -o ${BIN_PATH}/${WHAT} ${REPO}/cmd/${WHAT}
CGO_ENABLED=1 GOOS=${GOOS} GOARCH=${GOARCH} go build ${GOFLAGS} -ldflags "${GLDFLAGS}" -o ${BIN_PATH}/${WHAT} ${REPO}/cmd/${WHAT}

This comment has been minimized.

@jlebon

jlebon Feb 8, 2019

Member

I made this conditional on the MCD only in c02eb5d#diff-9283775b0feecc10455ec28bd08983b0.

This comment has been minimized.

@jlebon

jlebon Feb 8, 2019

Member

Hmm, actually that might not work. It looks like other components will then fail to build. From #335:

--> RUN WHAT=machine-config-controller ./hack/build-go.sh
Using version from git...
Building github.com/openshift/machine-config-operator/cmd/machine-config-controller (v3.11.0-589-gf62a5cf6-dirty)
vendor/github.com/coreos/go-systemd/sdjournal/functions.go...machine-config-operator/vendor/github.com/coreos/pkg/dlopen
error: build error: running 'WHAT=machine-config-controller ./hack/build-go.sh' failed with exit code 1

I'm not familiar enough with the golang build system, but it seems like it's trying to bundle all the vendored packages for all the targets instead of only the ones that actually need them?

This comment has been minimized.

@runcom

runcom Feb 8, 2019

Author Member

it's because the daemon package imports other packages (controller|server) and the build system bundle them together and that's why it's required for other components as well.

This comment has been minimized.

@abhinavdahiya

abhinavdahiya Feb 8, 2019

Member

You can use build tags to include / build files only when those tags are set...
https://golang.org/pkg/go/build/#hdr-Build_Constraints

This comment has been minimized.

@runcom

runcom Feb 8, 2019

Author Member

yeah, but that's gonna require some refactor as well

This comment has been minimized.

@runcom

runcom Feb 8, 2019

Author Member

my point is yeah, we're going to refactor for build tags but I guess for this PR we can ship with CGO_ENABLED=1 anyway, I'm creating a new issue to track this.

This comment has been minimized.

@runcom

runcom Feb 8, 2019

Author Member

@runcom runcom force-pushed the runcom:early-ssh-accesses branch from 26f97f6 to a75271c Feb 8, 2019

@ashcrow

This comment has been minimized.

Copy link
Member

ashcrow commented Feb 8, 2019

could not wait for build: the build machine-config-controller failed after 2m16s with reason DockerBuildFailed: Docker build strategy has failed.

@runcom runcom force-pushed the runcom:early-ssh-accesses branch from a75271c to ad90c63 Feb 12, 2019

@runcom

This comment has been minimized.

Copy link
Member Author

runcom commented Feb 12, 2019

rebased and removed the vendoring since #335 pulled it already

This should be ready

@runcom runcom force-pushed the runcom:early-ssh-accesses branch from ad90c63 to 0a7bdad Feb 12, 2019

@runcom

This comment has been minimized.

Copy link
Member Author

runcom commented Feb 12, 2019

/retest

1 similar comment
@runcom

This comment has been minimized.

Copy link
Member Author

runcom commented Feb 13, 2019

/retest

@jlebon
Copy link
Member

jlebon left a comment

Just one minor nit, otherwise LGTM!

return err
}
// if the journal cursor has something
if r == 1 {

This comment has been minimized.

@jlebon

jlebon Feb 13, 2019

Member

Minor: could avoid the indentation here by checking for 0 and returning early.

This comment has been minimized.

@runcom

runcom Feb 13, 2019

Author Member

thanks, fixed

pkg/daemon: detect ssh accesses before daemon started
Reads the journal checking for logind message ID and annotates the node
with the ssh accessed label if it finds out there were previous
accesses.

Signed-off-by: Antonio Murdaca <runcom@linux.com>

@runcom runcom force-pushed the runcom:early-ssh-accesses branch from 0a7bdad to 77cd586 Feb 13, 2019

@jlebon

This comment has been minimized.

Copy link
Member

jlebon commented Feb 13, 2019

/lgtm

@openshift-ci-robot

This comment has been minimized.

Copy link

openshift-ci-robot commented Feb 13, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlebon, runcom

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:

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

@runcom

This comment has been minimized.

Copy link
Member Author

runcom commented Feb 13, 2019

/retest

1 similar comment
@runcom

This comment has been minimized.

Copy link
Member Author

runcom commented Feb 13, 2019

/retest

@openshift-merge-robot openshift-merge-robot merged commit 7d2cb71 into openshift:master Feb 13, 2019

6 checks passed

ci/prow/e2e-aws Job succeeded.
Details
ci/prow/e2e-aws-op Job succeeded.
Details
ci/prow/images Job succeeded.
Details
ci/prow/rhel-images Job succeeded.
Details
ci/prow/unit Job succeeded.
Details
tide In merge pool.
Details

@runcom runcom deleted the runcom:early-ssh-accesses branch Feb 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment