cloudfoundry / bosh-cli

BOSH CLI v2+
Apache License 2.0
178 stars 160 forks source link

Adds --system and --all-logs flags to logs action #614

Closed klakin-pivotal closed 1 year ago

klakin-pivotal commented 1 year ago

This commit adds the --system and --all-logs flags to the "bosh logs" action.

The --system flag allows an operator to fetch or follow logs in the '/var/log' directory in a Bosh-deployed VM. The --all-logs flag allows an operator to fetch or follow logs provided by all of the --jobs (the default flag if none are specified), --agent, and --system flags in one convenient operation.

The code for the --system flag in the --follow operation adds a dependency on the 'find' command, but we believe that we currently include that command in every stemcell that we ship, so this should not be a problem. Because /var/log typically contains rotated logs, as well as files that are binary data, we had file-exclusion requirements that were too complex to be expressed with a glob pattern.

This commit also contains a rename of the "job" function parameter in Client.FetchLogs to "instance". The name for this concept has been "instance" for at least five years, and this discrepancy was Very Confusing to us when we were trying to understand that section of the code. So, we renamed the variable.

It's also important to note that use of the --follow flag should work with any version of the Bosh Agent, but downloading the --system or --all-logs logs (done by omitting the --follow flag) requires Bosh Agent changes to handle the new log types. This change is backwards compatible- incompatible Agents will return an error if either the --system or --all-logs flag is used, and will work correctly if neither one is used.

klakin-pivotal commented 1 year ago

NOTE: This PR is related to but does not require either of these two bosh-agent PRs: https://github.com/cloudfoundry/bosh-agent/pull/305 https://github.com/cloudfoundry/bosh-agent/pull/304

klakin-pivotal commented 1 year ago

Yeah, I was definitely not all that happy with building a string to pass to bash -c ''. I was hoping to find some flavor of shellescape in the packed-in golang libraries, but it either doesn't exist, or I missed it.

I also thought about building the list of arguments to find [(rather than specifying them as one long string)], but it seemed to me that the list of arguments was sufficiently short that putting that much distance between the input data and where it was getting used wasn't justifiable yet.

I'm looking at this embed library, and if we used it, we'd have to scp the script over to the Agent VM(s), and then run it, right? (Because otherwise, we'd cat it, and pass that to bash -c, yeah?)

aramprice commented 1 year ago

You could scp the code, or you could use the externalized embed file as a holding place for a string which you (eventually) pass to bash -c ....

This wouldn't necessarily change the complexity of the command string that needs to be sent, though it might make it easier (as a single .sh file) to reason about.

The code might look something like:

//go:embed tail-command.sh
var tailCommand string

func (c LogsCmd) buildTailCmd(opts LogsOpts) []string {
    //....
    return []string{"sudo", "bash", "-c", "exec", tailCommand}
}

... the snipped above presupposes some way of signaling the code contained in tail-command.sh which behavior it should exhibit. This might be accomplished by env-vars, or by breaking up a monolithic file into separate chunks to be included/excluded the way the buildTailCmd currently works.

The thought being that if all/most the logic in the buildTailCmd function could be expressed in a bash script which could be passed to bash -c exec ... then the overall logic might be easier to comprehend.

Or possibly not 😬

I (also) recently discovered that Golang doesn't appear to have any sort of build it shell-escape library 😭

klakin-pivotal commented 1 year ago

I might be being dense (and please correct me if I am), but I don't see the advantage of this over embedding a string... if we're not going to be scping a file over and then executing it. (And I don't like that, because it requires us to clean up afterwards.)

I do like that it's a directive to the compiler to build in a "file" or "directory tree" into the program itself. That's neat (though sure to explode the size of the program if your files are large, which is not neat (albeit irrelevant in our case)).

But given that (I assume) we'd be converting it to a string and passing to bash -c '' anyway (with all the shell escaping that would require), I think that'd be putting way too much distance between the "Oh, you have to do all the work to make the stuff in here -er- 'bash -c safe'" knowledge and where you actually have to use it. (I guess we could build the file with \ line-continuators (or whatever they are called) so that we could cram a comment in there to remind folks of that requirement...)

aramprice commented 1 year ago

I think bash -c will be fine if the code is passed as a single arg in a syscall.Exec method. Example here in cloudfoundry/buildpackapplifecycle/launcher/launcher_unix.go