Skip to content

chore: add parent PID to coder ssh log file name #16080

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

Merged
merged 1 commit into from
Jan 14, 2025
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 19 additions & 11 deletions cli/ssh.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,18 +124,26 @@ func (r *RootCmd) ssh() *serpent.Command {
if err != nil {
return xerrors.Errorf("generate nonce: %w", err)
}
logFilePath := filepath.Join(
logDirPath,
fmt.Sprintf(
"coder-ssh-%s-%s.log",
// The time portion makes it easier to find the right
// log file.
time.Now().Format("20060102-150405"),
// The nonce prevents collisions, as SSH invocations
// frequently happen in parallel.
nonce,
),
logFileBaseName := fmt.Sprintf(
"coder-ssh-%s-%s",
// The time portion makes it easier to find the right
// log file.
time.Now().Format("20060102-150405"),
// The nonce prevents collisions, as SSH invocations
// frequently happen in parallel.
nonce,
)
if stdio {
Copy link
Contributor

Choose a reason for hiding this comment

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

In setStatsCallback, we use the parent pid as the filename regardless of whether or not we're in stdio mode: https://github.com/coder/coder/pull/16078/files#diff-e288cc849ca774d47294f4f6c71c4ec171f6e0b15491e7220a0144dbfc1a43bcR1213

Should we just include it in either case here as well? Trying to get a handle on how these files relate I suppose.

Copy link
Contributor

Choose a reason for hiding this comment

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

For now I think this is good for compatibility though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking we only want it for stdio mode because including the parent PID is only meaningful when coder ssh is invoked via ProxyCommand (which would use --stdio). Happy to change this if you prefer to be consistent, though.

There isn't a use case where we would use --network-info-dir without --stdio, but I thought I should be consistent and support it in either case. I could make it return an error when --stdio is not also specified.

// The VS Code extension obtains the PID of the SSH process to
// find the log file associated with a SSH session.
//
// We get the parent PID because it's assumed `ssh` is calling this
// command via the ProxyCommand SSH option.
logFileBaseName += fmt.Sprintf("-%d", os.Getppid())
}
logFileBaseName += ".log"

logFilePath := filepath.Join(logDirPath, logFileBaseName)
logFile, err := os.OpenFile(
logFilePath,
os.O_CREATE|os.O_APPEND|os.O_WRONLY|os.O_EXCL,
Expand Down
Loading