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

Make json_stream_sampler handle UID, GID, and perm #1281

Closed
wants to merge 6 commits into from

Conversation

nichamon
Copy link
Collaborator

@nichamon nichamon commented Sep 7, 2023

Without the patch, the json_stream_sampler receives the UID, GID, and perm values at the config line, but it does not apply the values to the metric sets.

The patch makes the plugin use the specified UID, GID, and permission; otherwise, it will use the security data from the received stream data. The security data is applied at the set creation time. The configuration command set_sec_mod must be used to modify the sets' security. The plugin does not automatically revise the set security according to the stream data's security changes.

@nichamon
Copy link
Collaborator Author

nichamon commented Sep 7, 2023

@tom95858 Do you think the plugin should revise the sets' UID, GID, or perm according to the stream data's security changes? The patch does not do that. It only makes sense when the UID, GID, or perm are from the received stream data at the set creation time. It could be confusing when the UID, GID, or perm values are specified at the config line.
We can make it happen in the way that the plugin will automatically modify the UID, GID, or perm only when its value is not given at the config line.

@nichamon
Copy link
Collaborator Author

nichamon commented Sep 7, 2023

I'll mark this as ready when I have the test coverage done, and the patch passes the test cases.

@nichamon
Copy link
Collaborator Author

nichamon commented Sep 7, 2023

@narategithub I use the UID, GID, and perm in the ldms_stream_recv_data_s structure inside the ldms_stream_event_s struct. What is the 'perm' in the ldmsd_stream_recv_data_s? Does it make sense to use it as the permission of the metrics sets encoded the stream data?

@narategithub
Copy link
Collaborator

@nichamon I think it makes sense for most cases. The corner case I can think of right now is stream data sending via root's channel (uid0, gid0), but somehow we want to create a set owned by a user (e.g. bob) from it. In that case, the stream data must carry UID/GID/Perm.

@nichamon
Copy link
Collaborator Author

nichamon commented Sep 8, 2023

@nichamon I think it makes sense for most cases. The corner case I can think of right now is stream data sending via root's channel (uid0, gid0), but somehow we want to create a set owned by a user (e.g. bob) from it. In that case, the stream data must carry UID/GID/Perm.

In this case, admins can specify the UID, GID, and perm at the config line of the plugin. The given values take precedent over the values in the stream data.

@baallan
Copy link
Collaborator

baallan commented Sep 8, 2023

@nichamon I think it makes sense for most cases. The corner case I can think of right now is stream data sending via root's channel (uid0, gid0), but somehow we want to create a set owned by a user (e.g. bob) from it. In that case, the stream data must carry UID/GID/Perm.

We're running root-owned app data collection about linux processes, and part of that data is the uid/gid of each process. At the store, admins may use storage policies or set mappings with these uid/gid in setting up databases, but these uid/gid should not be confused with the ownership/group of the stream (who originally published it).

@nichamon nichamon force-pushed the job_stream_sampler branch 4 times, most recently from 051640c to 98658dd Compare September 15, 2023 15:54
Without the patch, the json_stream_sampler ignores the UID, GID, and
perm given at the config line.

With the patch, the plugin creates LDMS sets with the specified UID,
GID, and permission. If one of the values are not given, it will use the
security data from the received stream data.

After the set has been created, the configuration command set_sec_mod
must be used to modify the sets' security (UID, GID, and perm). The
plugin does not automatically revise the set security according to the
stream data's security changes.
The plugin automatically generates the set instance name if none is
given. The generated name is <producer>_<schema name>.
Add metric types to the LDMS sets' JSON objects
@nichamon nichamon changed the title [TEST] Make json_stream_sampler handle UID, GID, and perm Make json_stream_sampler handle UID, GID, and perm Sep 19, 2023
@nichamon
Copy link
Collaborator Author

The patch passes against the test coverage here.

@nichamon nichamon marked this pull request as ready for review September 19, 2023 19:26
@nichamon nichamon marked this pull request as draft September 21, 2023 17:44
@nichamon
Copy link
Collaborator Author

I'm closing this to prevent Github from running the build test and sending emails to people. I'll create another pull request when the patches are ready.

@nichamon nichamon closed this Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants