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.

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

test: resolve process.setgid error for group 'nobody' on ubuntu #19755

Closed

Conversation

dsinecos
Copy link
Contributor

@dsinecos dsinecos commented Apr 2, 2018

When the tests are run as root in Ubuntu, process.setgid is called with
'nobody' as an argument. This throws an error in Ubuntu. This is because
in Ubuntu the equivalent of 'nobody' group is named as 'nogroup'

This commit sets gid to 'nobody' first and if it throws a group id does not exist error, it attempts to set gid to 'nogroup'. If it still
causes an error, the error is thrown.

Refs: #19594

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 2, 2018
@Trott
Copy link
Member

Trott commented Apr 2, 2018

@Trott
Copy link
Member

Trott commented Apr 2, 2018

This problem is only triggered on Ubuntu if the tests are run as root, which is probably why we almost never see it. But I definitely have gotten more than one report of it from people following https://www.nodetodo.org/getting-started/

@Trott
Copy link
Member

Trott commented Apr 2, 2018

Hi @dsinecos, and welcome! Thanks for the pull request!

} catch (err) {
if (err.message !== 'setgid group id does not exist') {
throw err;
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: You don't need the else here because of the throw in the preceding block. I don't mind it, 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.

Took me a while to figure out why we won't need the else block, got it now :) Will keep it in mind for the future

@Trott
Copy link
Member

Trott commented Apr 3, 2018

@Trott
Copy link
Member

Trott commented Apr 3, 2018

test-node-commit-arm needs to be re-run too but using Rebuild in the CI interface isn't rebuilding that task for me. Odd.

@dsinecos
Copy link
Contributor Author

dsinecos commented Apr 3, 2018

@Trott Thanks Rich, for all the guidance :)

@BridgeAR
Copy link
Member

BridgeAR commented Apr 9, 2018

@dsinecos would you be so kind and rebase this? :-) And while being on it: do you mind addressing the nit? But that is not required.

@dsinecos
Copy link
Contributor Author

dsinecos commented Apr 9, 2018

@BridgeAR Sure :) Give me until tomorrow though, I'm tied up with a few things. I hope that's fine

@BridgeAR
Copy link
Member

BridgeAR commented Apr 9, 2018

@dsinecos sure :-)

When the tests are run as root in Ubuntu, process.setgid is called with
'nobody' as an argument. This throws an error in Ubuntu. This is because
in Ubuntu the equivalent of 'nobody' group is named as 'nogroup'

This commit sets gid to 'nobody' first and if it throws a `group id does
not exist` error, it attempts to set gid to 'nogroup'. If it still
causes an error, the error is thrown.

Refs: nodejs#19594
@dsinecos dsinecos force-pushed the setgid-nobody-error-in-ubuntu branch from 2d83de1 to b1ebb21 Compare April 10, 2018 07:28
@dsinecos
Copy link
Contributor Author

@BridgeAR I've made the changes. I just realized, the commit message is not as per the guidelines, I'll amend that and push again, sorry about this

Else block removed as the `throw` in the preceding block makes it
redundant

Refs: nodejs#19594
@dsinecos dsinecos force-pushed the setgid-nobody-error-in-ubuntu branch from b1ebb21 to 2129edf Compare April 10, 2018 07:37
@dsinecos
Copy link
Contributor Author

@BridgeAR I hope I haven't made any errors around the commits, I did a git push -f origin setgid-nobody-error-in-ubuntu following a stack overflow answer as a normal push was returning the following

hint: Updates were rejected because the tip of your current branch is behind hint: its remote counterpart.

Let me know if something is amiss or if this needs more changes :)

@lpinca
Copy link
Member

lpinca commented Apr 16, 2018

@lpinca lpinca added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 16, 2018
@Trott
Copy link
Member

Trott commented Apr 16, 2018

@Trott
Copy link
Member

Trott commented Apr 16, 2018

@lpinca
Copy link
Member

lpinca commented Apr 21, 2018

One last CI before landing: https://ci.nodejs.org/job/node-test-pull-request/14416/

@lpinca
Copy link
Member

lpinca commented Apr 21, 2018

Landed in cc8a33e.

@lpinca lpinca closed this Apr 21, 2018
lpinca pushed a commit that referenced this pull request Apr 21, 2018
When the tests are run as root in Ubuntu, process.setgid() is called
with 'nobody' as an argument. This throws an error in Ubuntu and is
because, in Ubuntu, the equivalent of 'nobody' group is named as
'nogroup'.

This commit sets gid to 'nobody' first and if it throws a "group id
does not exist" error, it attempts to set gid to 'nogroup'. If it still
causes an error, the error is thrown.

PR-URL: #19755
Refs: #19594
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
jasnell pushed a commit that referenced this pull request Apr 23, 2018
When the tests are run as root in Ubuntu, process.setgid() is called
with 'nobody' as an argument. This throws an error in Ubuntu and is
because, in Ubuntu, the equivalent of 'nobody' group is named as
'nogroup'.

This commit sets gid to 'nobody' first and if it throws a "group id
does not exist" error, it attempts to set gid to 'nogroup'. If it still
causes an error, the error is thrown.

PR-URL: #19755
Refs: #19594
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants