keybase / go-keybase-chat-bot

Official Go Keybase chat bot SDK
BSD 3-Clause "New" or "Revised" License
111 stars 43 forks source link

Command.stdoutpipe fails on GCP Cloud Run #58

Open vladionescu opened 4 years ago

vladionescu commented 4 years ago

Breaking this out from #55

Google Cloud's serverless container environment is called Cloud Run.

I'm trying to run a bot here in a container, but it kept crashing on calls to kbc.GetUsername(). Eventually this was tracked down to be a problem with the Command("status") call.

The issue is that Cloud Run captures STDOUT and STDERR without duplicating the file descriptors, so those original STDOUT/STDERR FDs are no longer doing what we expect. getUsername() expects to shell out to keybase status and read its STDOUT, but since the FDs are being hogged by Cloud Run's logging, the output doesn't come back and it ends up timing out as noted in #55.

In that issue I recommended 2 potential fixes: using a different FD that we know Cloud Run isn't hogging, or using another API altogether that doesn't involve shelling out to the keybase CLI.

@malware-unicorn PoC'd the first option successfully.

malware-unicorn commented 4 years ago

Yep I confirmed the fix, switching from StdoutPipe and StderrPipe to a custom os.Pipe() solved the output issues when in google cloud run.

I had to change the pipes in func getUsername, func (a *API) startPipes() and func (a *API) Listen and it worked out great.

Fix in my fork: https://github.com/malware-unicorn/go-keybase-chat-bot/blob/master/kbchat/kbchat.go#L37

Also os.Pipe does not work in Windows environments so there might need to be a better work around in the future.

joshblum commented 4 years ago

@malware-unicorn calling Run on the command blocks until completion, defeating the timeout in play later in the function https://github.com/malware-unicorn/go-keybase-chat-bot/blob/master/kbchat/kbchat.go#L48. In https://github.com/keybase/go-keybase-chat-bot/pull/57 I added the output pipe to ExtraFiles, are you able to get the command running with these fixes?

malware-unicorn commented 4 years ago

@joshblum yeah the Run is not needed but what is needed is the actual assignment for stdout because the command needs a different file descriptor since the container's stdout file descriptor is being locked/used by google cloud run's logging.

pipeR, pipeW, _ := os.Pipe()
    p.Stdout = pipeW <-------This is needed
    p.ExtraFiles = []*os.File{
        pipeW,
    }

and

scanner := bufio.NewScanner(pipeR)

Anywhere StdoutPipe is called would need to be changed to p.Stdout = pipeW

joshblum commented 4 years ago

@malware-unicorn calling StdoutPipe creates a pipe, it doesn't use os.Stdout so our changes should be equivalent with the ExtraFiles call https://golang.org/src/os/exec/exec.go?s=17635:17684#L604