Skip to content

Use "getent group" instead of reading "/etc/group" to get group information#14316

Merged
Dim-P merged 4 commits into
netdata:masterfrom
Dim-P:use-getent-group
Jan 27, 2023
Merged

Use "getent group" instead of reading "/etc/group" to get group information#14316
Dim-P merged 4 commits into
netdata:masterfrom
Dim-P:use-getent-group

Conversation

@Dim-P
Copy link
Copy Markdown
Contributor

@Dim-P Dim-P commented Jan 23, 2023

Summary

SSIA, Fixes #13711 .

Test Plan
  1. Install Netdata using netdata-installer.sh.
  2. Uninstall Netdata using netdata-uninstaller.sh
  3. Observe group creation, user assignment and group/user deletion runs as expected.
Additional Information
For users: How does this change affect me?

@Dim-P Dim-P requested a review from a team January 23, 2023 18:42
@Dim-P Dim-P requested a review from Ferroin as a code owner January 23, 2023 18:42
@github-actions github-actions Bot added the area/packaging Packaging and operating systems support label Jan 23, 2023
Comment thread packaging/installer/functions.sh
Comment thread packaging/installer/functions.sh Outdated
@Dim-P Dim-P requested review from ilyam8 and thiagoftsm January 24, 2023 18:59
Comment on lines +400 to +403
if command -v getent > /dev/null 2>&1; then
getent group "${1:-""}"
else
cat /etc/group | grep "^${1}:"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suggest dev null stdout and stderr in this function so a caller doesn't need to do it

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I will leave it as is due to the following call that needs to evaluate the function return value:
if get_group "${groupname}" | cut -d ':' -f 4 | grep -wq "${username}"; then

But I renamed group_exists() to get_group() to make the function purpose clearer.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As it is now, get_group == getent group. Do we need cat /etc/group (also can be just grep "^${1}:" /etc/group) at all? I see we used getent group before this PR and it wasn't a problem.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's a fallback, see the other thread. You are right about cat, I am just used to do it this way for other reasons and forgot you can do grep directly. Not a big deal so I will merge it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's a fallback, see the other thread.

Yes, I understood that.

My point is - why do we need this fallback? We had no fallback previously and it wasn't a problem.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree but it was suggested by both @thiagoftsm and @Ferroin so I just added it.

@Dim-P Dim-P requested a review from tkatsoulas as a code owner January 26, 2023 21:47
@Dim-P Dim-P requested a review from ilyam8 January 26, 2023 21:49
Copy link
Copy Markdown
Contributor

@thiagoftsm thiagoftsm left a comment

Choose a reason for hiding this comment

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

Installation was successful after changes and netdata is running as expected, LGTM!

@Dim-P Dim-P merged commit 081dbc6 into netdata:master Jan 27, 2023
@Dim-P Dim-P deleted the use-getent-group branch January 27, 2023 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/packaging Packaging and operating systems support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Stop using 'grep' to obtain the user or group information.

5 participants