Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[BUGFIX] Not working properly on self-hosted runners #27

Merged
merged 6 commits into from
May 18, 2020
Merged

[BUGFIX] Not working properly on self-hosted runners #27

merged 6 commits into from
May 18, 2020

Conversation

thommyhh
Copy link
Contributor

@thommyhh thommyhh commented Apr 10, 2020 •
edited
Loading

  • remove fixed default auth socket path
  • call ssh-agent with auth socket path, if not explicitly given,
    to let the system choose the path
  • read SSH_AUTH_SOCKET and SSH_AGENT_PID from ssh-agent command output
    and export them as variables
  • update dist/index.js
  • update readme with section about exported variables

Fixes #16

thommyhh added 3 commits April 10, 2020 20:02
@thommyhh
[BUGFIX] Not working properly on self-hosted runner
36fd7e1 
* remove fixed default auth socket path
* call `ssh-agent` with auth socket path, if not explicitly given,
  to let the system choose the path
* read SSH_AUTH_SOCKET and SSH_AGENT_PID from `ssh-agent` command output
  and add the as variables
@thommyhh
[TASK] Update dist/index.js
0e6fdec
@thommyhh
[DOC] Update readme with section about exported variables
bf9b7d9
@rbellamy
Copy link

rbellamy commented May 8, 2020

Maybe I'm reading this incorrectly, but wouldn't this kill the agent for other runners on the same machine?

@rbellamy
Copy link

rbellamy commented May 8, 2020

Adding the run ID seems like a wise choice to ensure the agent is using a unique AUTH_SOCK:

#16 (comment)

@thommyhh
Copy link
Contributor Author

thommyhh commented May 8, 2020

@rbellamy As far as I know, ssh-agent picks a random unique file name for the auth socket, if none is provided. This is why I removed the default. By exporting the process ID $SSH_AGENT_PID, we can kill that one agent. No other agents are killed.

mpdude
mpdude requested changes May 15, 2020
View reviewed changes
Copy link
Member

@mpdude mpdude left a comment

Choose a reason for hiding this comment

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

Thank you @thommyhh for working on this! I really appreciate the effort, in particular when it comes to parsing the agent's output.

Apart from the minor comments I left, could you have a look at https://help.github.com/en/actions/building-actions/metadata-syntax-for-github-actions#post ? I think we could setup a post job that makes this action always clean up its own agent instance automatically.

README.md Outdated
## Exported variables
The action exports `SSH_AUTH_SOCK` and `SSH_AGENT_PID` through the Github Actions core module.
The `$SSH_AUTH_SOCK` is used by several applications like git or rsync to connect to the SSH authentication agent.
The `$SSH_AGENT_PID` can be used to kill the SSH agent, e.g. when using on a self-hosted runner:
Copy link
Member

Choose a reason for hiding this comment

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

I'd shorten this to only say "... contains the process id of the agent running for this job". I think it should be the action's responsibility to kill the agent.

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 added a post action to kill the agent. Seems to work as expected.

dist/index.js
@@ -62,7 +62,7 @@ try {
const home = process.env['HOME'];
const homeSsh = home + '/.ssh';

const privateKey = core.getInput('ssh-private-key').trim();
const privateKey = core.getInput('ssh-private-key');
Copy link
Member

Choose a reason for hiding this comment

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

Could you rebase this PR against master please? Then this change should go away.

dist/index.js
@@ -71,14 +71,27 @@ try {
}

console.log(`Adding github.com keys to ${homeSsh}/known_hosts`);
fs.mkdirSync(homeSsh, { recursive: true});
fs.mkdirSync(homeSsh, { recursive: true });
Copy link
Member

Choose a reason for hiding this comment

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

Same as before, should go away with a rebase.

dist/index.js
if (authSock && authSock.length > 0) {
sshAgentOutput = child_process.execFileSync('ssh-agent', ['-a', authSock]);
} else {
sshAgentOutput = child_process.execFileSync('ssh-agent')
Copy link
Member

Choose a reason for hiding this comment

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

This is a slight BC break since now not providing a value for the ssh-auth-sock input will bind it to a random port, instead of the fixed value we used previously. (Yes, I completely understand that this is part of the goal to be able to run multiple instances in parallel; just sayin').

Copy link
Member

Choose a reason for hiding this comment

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

Since we're still in the 0.x version ranges, IMO nothing we should worry about too much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It binds to a random socket file, just for being precise. I don't think the value matters as long as it written to the $SSH_AUTH_SOCK variable.

thommyhh added 2 commits May 15, 2020 08:34
@thommyhh
Merge remote-tracking branch 'webfactory/master'
8f59108
@thommyhh
[FEATURE] Post step for cleaning up the SSH agent
b05960c 
* add `cleanup.js` for stopping the ssh-agent after the job is done
* add `scripts/build.js` to make it easier to build the files in `dist/`
* update README
* remove meaningless dependency `child_process` (its a node core package)
* update `@actions/core` to `^1.2.4`
@thommyhh thommyhh changed the base branch from master to try-windows May 15, 2020 07:43
@thommyhh thommyhh changed the base branch from try-windows to master May 15, 2020 07:43
@thommyhh thommyhh requested a review from mpdude May 15, 2020 07:46
@thommyhh
Copy link
Contributor Author

@mpdude I updated the branch with the latest changes from master, but the changes did not disappear. I don't know, how to trigger GitHub to re-read the base branch (tried changing base).

I also added the post action to kill the agent automatically, as suggested. I wondered, if there is something like a post job script, to clean things up.

@zhigang1992
Copy link

I think you can merge master and then push?

@thommyhh
Copy link
Contributor Author

@zhigang1992
That's what I did, but GitHub did not update the base branch anyway.

@mpdude mpdude merged commit 4fcb25e into webfactory:master May 18, 2020
@mpdude
Copy link
Member

mpdude commented May 18, 2020

I have released 0.3.0 with this change.

Thank you @thommyhh and everyone involved!

@mkrakowitzer
Copy link

Thanks @thommyhh @mpdude for resolving this!

This was referenced Mar 10, 2021
Merged
Merged
Merged
Merged
Merged
Merged
Closed
Merged
Merged
Merged
Merged
This was referenced Mar 15, 2021
Merged
Closed
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpdude mpdude Awaiting requested review from mpdude

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support non-ephemeral, self-hosted runners
5 participants
@thommyhh @rbellamy @zhigang1992 @mpdude @mkrakowitzer