Skip to content

idtools: add support for libsubid #882

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

Merged
merged 2 commits into from
Aug 4, 2021

Conversation

giuseppe
Copy link
Member

when building with cgo, add support for libsubid to read the
additional sub IDs for the user instead of parsing the /etc/sub?id
files.

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

@rhatdan
Copy link
Member

rhatdan commented Apr 27, 2021

Thanks @giuseppe When do you think libsubuid will show up?

@giuseppe
Copy link
Member Author

Thanks @giuseppe When do you think libsubuid will show up?

I am not sure about that, AFAIK there wasn't even a release upstream yet.

@alexey-tikhonov do you know when a new shadow-utils can land in Fedora?

@alexey-tikhonov
Copy link

It is actually in Fedora 35 already: https://bodhi.fedoraproject.org/updates/FEDORA-2021-8483b52301

(but most probably will need an update when shadow-maint/shadow#325 is fixed)

@giuseppe
Copy link
Member Author

thanks! Sorry I've missed it before.

I've tested the current patch with the package available for F35 and it works fine

@rhatdan
Copy link
Member

rhatdan commented Jun 6, 2021

What do we need to do to get this in and able to work on distros without libsubid?

@giuseppe
Copy link
Member Author

giuseppe commented Jun 7, 2021

What do we need to do to get this in and able to work on distros without libsubid?

Probably using a build tag would be the easiest and we can use it in Rawhide

@alexey-tikhonov
Copy link

Btw, there were several changes in libsubid (including shadow-maint/shadow@3d670ba)
We will rebuild package in Rawhide soon (probably after shadow-utils upstream release)

@rhatdan
Copy link
Member

rhatdan commented Jun 23, 2021

Can you add something like:

cat hack/libsubid_tag.sh
#!/usr/bin/env bash
if test $(${GO:-go} env GOOS) != "linux" ; then
	exit 0
fi
tmpdir="$PWD/tmp.$RANDOM"
mkdir -p "$tmpdir"
trap 'rm -fr "$tmpdir"' EXIT
cc -o "$tmpdir"/libdm_tag -ldevmapper -x c - > /dev/null 2> /dev/null << EOF
#include <"shadow/subid.h">
int main() {
        subid_range *ranges;
	err := get_subuid_ranges("root", &ranges);
	return 0;
}
EOF
if test $? -eq 0 ; then
	echo libsubid
fi

@rhatdan
Copy link
Member

rhatdan commented Jun 23, 2021

Also modify the Makefile with

AUTOTAGS := ( s h e l l . / h a c k / b t r f s t a g . s h ) (shell ./hack/libdm_tag.sh) $(shell ./hack/libsubid_tag.sh)

@rhatdan
Copy link
Member

rhatdan commented Jun 23, 2021

Perhaps we should make the Tag a negative as well so it will always get compiled in unless users specify don't use it.

@giuseppe giuseppe force-pushed the support-libsubid branch 2 times, most recently from 69d0932 to 4518890 Compare June 25, 2021 11:10
@giuseppe
Copy link
Member Author

switched the logic

@@ -0,0 +1,19 @@
#!/usr/bin/env bash
if test $(${GO:-go} env GOOS) != "linux" ; then
exit 0
Copy link
Member

Choose a reason for hiding this comment

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

This should also do
echo no_libsubid

Copy link
Member Author

Choose a reason for hiding this comment

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

amended

@rhatdan
Copy link
Member

rhatdan commented Jun 25, 2021

One issue, and then LGTM
Is this still a WIP, or something we can merge now?

@giuseppe
Copy link
Member Author

Is this still a WIP, or something we can merge now?

it is a WIP because the libsubid API were not released yet.

@alexey-tikhonov is there any risk the API will change again or can we merge this PR?

@giuseppe giuseppe changed the title [WIP] idtools: add support for libsubid idtools: add support for libsubid Jun 25, 2021
@rhatdan
Copy link
Member

rhatdan commented Jun 25, 2021

@rhatdan
Copy link
Member

rhatdan commented Jun 25, 2021

It would be nice to get a version of Podman into Rawhide that used libsubid, to make sure all of the plumbing works.

@rhatdan
Copy link
Member

rhatdan commented Jun 25, 2021

@alexey-tikhonov PTAL

Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

Just a non-blocking nit, otherwise LGTM

@alexey-tikhonov
Copy link

it is a WIP because the libsubid API were not released yet.

@alexey-tikhonov is there any risk the API will change again or can we merge this PR?

To the best of my knowledge, I don't anticipate libsubid API changes in a near future. (And this is already merged in C9S: https://gitlab.com/redhat/centos-stream/rpms/shadow-utils/-/commit/fbf9d3a3ea5ea94a96eb747034c5ca9a25095633)

But that's true that there is no upstream release yet and I'm not authoritative source here.
@hallyn, what's your current plan with regards to upstream release of shadow-utils?

@alexey-tikhonov PTAL

Looks good to me.

@alexey-tikhonov
Copy link

@ikerexxe
Copy link

ikerexxe commented Aug 2, 2021

I'm working on rebasing Fedora rawhide to the new version of shadow-utils. Once it is ready I'll let you know.

@rhatdan
Copy link
Member

rhatdan commented Aug 2, 2021

Awesome news, Are you planning on releasing this to Fedora 34? We will need this released/backported to RHEL8.6, also.

@alexey-tikhonov
Copy link

Awesome news, Are you planning on releasing this to Fedora 34? We will need this released/backported to RHEL8.6, also.

RHEL8.6 - sure, Fedora 34 - not planned at the moment.

@rhatdan
Copy link
Member

rhatdan commented Aug 3, 2021

@giuseppe Now that libsubuid is in Rawhide, can we move this out of draft?

@giuseppe giuseppe marked this pull request as ready for review August 4, 2021 08:13
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
when building with cgo, add support for libsubid to read the
additional sub IDs for the user instead of parsing the /etc/sub?id
files.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe
Copy link
Member Author

giuseppe commented Aug 4, 2021

rebased

@rhatdan rhatdan merged commit 7a4d2bc into containers:main Aug 4, 2021
@ikerexxe
Copy link

ikerexxe commented Aug 5, 2021

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.

None yet

5 participants