Auto-connect request for OpenSearch snap

medib · September 14, 2022, 1:23pm

Hello,

I am working on a Snap for OpenSearch, and the latter makes heavy use of the local filesystem, in addition to reading kernel values, making use of coredump_filter etc…

I would therefore like to request the auto-connect permissions for the following:

- log-observe
- mount-observe
- process-control
- procsys-read   # custom plug
- system-observe

The procsys_read plug reads from the system-files interface, and is described here.

The snap can be found here.

Thank you

alexmurray · September 15, 2022, 1:58am

Wow that procsys-read interface seems huge… in the past we have created custom interfaces like greengrass-support and docker-support for snaps which require these sorts of more privileged accesses - however I guess now with system-files perhaps we have a better solution - @pedronis could you comment on whether you think a new opensearch-support interface would be a better solution here compared to using system-files for these accesses? Whilst this may work I wonder that it violates the spirit / intention of system-files.

pedronis · September 15, 2022, 2:15pm

no, that’s definitely too much for system-files, it’s very brittle to review at that level. I think we need to consider an opensearch-support interface or see if there’s a pattern there? it seems it’s trying to read its own cgroup hierarchy?

a couple of /proc things there are already covered by system-observe or could be added there (swappiness)

ogra · September 15, 2022, 3:31pm

well, the system-files interface could as well just be a one liner like: /sys/fs/cgroup/system.slice/snap.opensearch.daemon.service/

and the other bits added to the respective interfaces …

medib · September 16, 2022, 9:15am

So what would be the way to proceed from here? Do I need to create a new interface or do we proceed as is given it’s only a read access to its own cgroup?

ps: here is the version I’m trying to upload to the store

pedronis · September 19, 2022, 1:02pm

So if I understand these are the read accesses:

      - /proc/cgroups
      - /proc/self/coredump_filter
      - /proc/sys/vm/max_map_count
      - /proc/sys/vm/swappiness
      - /proc/sys/net/ipv4/tcp_retries2
      - /sys/fs/cgroup/system.slice/snap.opensearch.daemon.service/*

/proc/self/coredump_filter is taken care by process-control once the PR land.

/proc/sys/net/ipv4/tcp_retries2 should be accessible with network-observe afaict?

proc/sys/vm/max_map_countis available under system-observe already

/proc/sys/vm/swappiness is not available anywhere but could be added to system-observe I think

We are left with:

/sys/fs/cgroup/system.slice/snap.opensearch.daemon.service/ that if it was the last thing needing access could be a system-files

/proc/cgroups could be added to system-observe?

@alexmurray what are your thoughts on these?

alexmurray · September 19, 2022, 9:29pm

Thanks for the analysis @pedronis - yep that sounds good to me.

medib · September 22, 2022, 9:18pm

Thank you for the details @pedronis! You’re right, /proc/sys/net/ipv4/tcp_retries2 is part of network-observe. I made a PR to include /proc/sys/vm/swappiness and /proc/cgroups in system-observe.

Do I understand correctly that /sys/fs/cgroup/system.slice/snap.opensearch.daemon.service/ currently stays as is?

pedronis · September 26, 2022, 10:02am

yes, reduced to one entry though. Until we find more use cases like this, this seems a reasonable approach.

emitorino · November 3, 2022, 10:35am

@medib,

Could you please update your snap with the alternatives suggested? The latest I see still contains the procsys-read iface reference with huge list of system-files entries.

I see the PR to extend system-observe was merged, so we should be good there. Meanwhile, I am +1 for auto-connect log-observe, mount-observe and system-observe ifaces since all seem required for searching purposes on the system.

Also, whats the reason to auto-connect process-control?

alexmurray · November 3, 2022, 7:39pm

+1 from me too for auto-connect of log-observe, mount-observe and system-observe as Emi outlined above.

medib · November 4, 2022, 10:13am

Thank you both!

I proceeded with the changes, they can be found here.

@emitorino the reason behind the process-control use is the fact OpenSearch needs to read/write in the /proc/self_pid/coredump_filter directory (here is the PR where I patched it on snapd for the said use)

Could I also get the approval for the 2 remaining ones?

process-control
cgroup-service-read (previously named procsys-read - but now with a much smaller scope)

emitorino · November 4, 2022, 11:28am

+2 votes for, 0 votes against, granting auto-connection of log-observe and system-observe. This is now live. (mount-observe was already granted for revision # 1)

@medib thanks for sharing the explanation about the need for auto-connecting process-control and pointing to the PR. But since process-control allows system-wide process management and this is not an expected opensearch functionality, I would prefer the user has a voice on granting such broad access so I am -1 for auto-connecting process-control to this snap. Can other @reviewers please vote?

I am +1 for auto-connect system-files with read access to /sys/fs/cgroup/system.slice/snap.opensearch.daemon.service. If possible, could you please rename the iface reference to be sys-fs-cgroup instead of just cgroup so the access being granted is clearer? Can other @reviewers please vote?

medib · November 11, 2022, 2:27pm

@emitorino I renamed the cgroup-service-read interface to sys-fs-cgroup-service-read, landing on my next PR.

@review-team could you please vote?

alexmurray · November 15, 2022, 12:11am

+1 from me for use of and auto-connect of system-files named sys-fs-cgroup-service (can you please drop the redundant read part of the name?) for read access to /sys/fs/cgroup/system.slice/snap.opensearch.daemon.service.

I also tend to agree on process-control being a bit too much authority for opensearch to be granted auto-connect for this - does the snap still function if this is not connected?

medib · November 16, 2022, 12:01pm

Thank you @alexmurray! I dropped the read from the interface name.

For process-control, it is needed for:

setting various ulimits, required by OpenSearch to start, such as ulimit -n 65535
for the coredump_filter part, it is needed by the jvm - But OpenSearch does work without it, I assume as long as there is no jvm related crash.

I could also make it a requirement for users of the snap to connect process-control, what do you think?

alexmurray · November 16, 2022, 8:24pm

If the snap can function without process-control (but in say a degraded state) then I think it is fine for it to remain as manually connected. However, if the snap is not even able to say start up without this being connected then it would be more appropriate for auto-connect - can you clarify?

medib · November 17, 2022, 4:04pm

I tested multiple scenarios and it seems that the accurate representation is “the snap can function without process-control (but in say a degraded state)”, the degraded state would be with regards to ulimit -l unlimited and coredump_filter - but opensearch is able to run and be used.

I think we’re fine for now, if in the future we come across a use case where it turns out to be crucial, I’ll let you know.

Thank you Alex and everyone for all the support, it was very helpful!

alexmurray · November 18, 2022, 4:06am

Ok let’s leave process-control to remain as manually connected for now.

+2 votes for, 0 votes against auto-connect of system-files named sys-fs-cgroup-service or read access to /sys/fs/cgroup/system.slice/snap.opensearch.daemon.service. This is now live.