libcephfs API changes for credential rework

libcephfs API changes for credential rework

[Jeff Layton]

(tl;dr: we need to make some changes to the libcephfs API to allow
 proper credential handling. What's the right approach to take?)

Greg Farnum has posted a pull request to rework how credentials are
handled in cephfs:

    https://github.com/ceph/ceph/pull/11218

...the patch pile is rather large, but the upshot is basically to
change the code not to carve out '-1' in the uid and gid ranges, and to
allow callers to pass down lists of supplementary groups. This is
mainly needed for nfs-ganesha, but it also allows ceph-fuse to delegate
permissions checks to the MDS.

That PR adds most of the low-level plumbing that's needed, but we still
need to expose this capability to applications via libcephfs. Many
libcephfs calls either take "int uid, int gid" or don't allow the
caller to send down creds at all.

I think what we'll want to do is allow applications to create a new
credential object and pass that down to the library in some fashion.
The question is: what's the best way to handle those API changes?

We have several options here:

1) make a clean break with the old API: just change the arguments to
the existing functions, bump the library's major version and set the
"age" field in it to 0. If you built against old headers, it'll fail to
load the library at runtime. This is clearly the simplest option, but
requires everyone to rebuild applications that were built against the
old headers.

2) create a "parallel" API that takes pointers to UserPerm objects, and
add new new calls for them. The old API then becomes a wrapper around
the new code. This would work, but it's more work -- we'd need to add a
whole swath of new calls that take this pointer. This has the advantage
of allowing existing programs to continue working through a lib
upgrade. For this, we'd bump the major lib version, and "age" field to
indicate that the library has a new API, but is backward compatible
with the old one. If we want to go this route, how should the new
functions be named?

3) get tricky with global and thread local variables. Add calls to
allow users to set libcephfs creds at the process and thread level.
When the API is called, we'll check to see if creds have been installed
in either spot and use those if they're present. If they're not, then
we'll grab them from the current task (much like we do today). This is
somewhat analogous to using setuid/setgid/setgroups before making a
syscall. It's less intuitive than option #2 and requires callers to
manage their creds carefully, but means that we don't need to explode
out the API _and_ preserves the ability to run programs built against
the old headers.

It's a fair bit of work any way we approach it, so I want to be clear
on a direction before I dive in too deeply.

Thoughts?
-- 
Jeff Layton <jlayton@redhat.com>

Re: libcephfs API changes for credential rework

[Sage Weil]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4161 bytes --]

On Tue, 27 Sep 2016, Jeff Layton wrote:
> (tl;dr: we need to make some changes to the libcephfs API to allow
>  proper credential handling. What's the right approach to take?)
> 
> Greg Farnum has posted a pull request to rework how credentials are
> handled in cephfs:
> 
>     https://github.com/ceph/ceph/pull/11218
> 
> ...the patch pile is rather large, but the upshot is basically to
> change the code not to carve out '-1' in the uid and gid ranges, and to
> allow callers to pass down lists of supplementary groups. This is
> mainly needed for nfs-ganesha, but it also allows ceph-fuse to delegate
> permissions checks to the MDS.
> 
> That PR adds most of the low-level plumbing that's needed, but we still
> need to expose this capability to applications via libcephfs. Many
> libcephfs calls either take "int uid, int gid" or don't allow the
> caller to send down creds at all.
> 
> I think what we'll want to do is allow applications to create a new
> credential object and pass that down to the library in some fashion.
> The question is: what's the best way to handle those API changes?
> 
> We have several options here:
> 
> 1) make a clean break with the old API: just change the arguments to
> the existing functions, bump the library's major version and set the
> "age" field in it to 0. If you built against old headers, it'll fail to
> load the library at runtime. This is clearly the simplest option, but
> requires everyone to rebuild applications that were built against the
> old headers.

I think this is the way to go.  We'd bump the SONAME (libcephfs2!).  The 
packaging infrastructure is such that you can have the old version of the 
package (libcephfs1) installed at the same time as libcephfs2 (and 
-devel), so stuff that's linked to the old API can still work and 
coexist with stuff linked to the new one.  The only restriction (on debian 
at least; I assume in rpm-land it is the same) is that you can only have 
one version of the -dev package (headers) installed at a time, so you can 
only compile new code against one version at a time.

IIRC with the SONAME stuff you can specify the oldest compatible 
version.  We can take the lazy route and skip #2 below and make that 2 as 
well, which would mean installing libcephfs1 and libcephfs2 concurrently 
for compatibility.

It seems like the key downside here is that any project building against 
libcephfs that isn't updated will not be able to use newer versions of the 
client until they move to the new API...

sage



> 
> 2) create a "parallel" API that takes pointers to UserPerm objects, and
> add new new calls for them. The old API then becomes a wrapper around
> the new code. This would work, but it's more work -- we'd need to add a
> whole swath of new calls that take this pointer. This has the advantage
> of allowing existing programs to continue working through a lib
> upgrade. For this, we'd bump the major lib version, and "age" field to
> indicate that the library has a new API, but is backward compatible
> with the old one. If we want to go this route, how should the new
> functions be named?
> 
> 3) get tricky with global and thread local variables. Add calls to
> allow users to set libcephfs creds at the process and thread level.
> When the API is called, we'll check to see if creds have been installed
> in either spot and use those if they're present. If they're not, then
> we'll grab them from the current task (much like we do today). This is
> somewhat analogous to using setuid/setgid/setgroups before making a
> syscall. It's less intuitive than option #2 and requires callers to
> manage their creds carefully, but means that we don't need to explode
> out the API _and_ preserves the ability to run programs built against
> the old headers.
> 
> It's a fair bit of work any way we approach it, so I want to be clear
> on a direction before I dive in too deeply.
> 
> Thoughts?
> -- 
> Jeff Layton <jlayton@redhat.com>
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: libcephfs API changes for credential rework

[Jeff Layton]

On Tue, 2016-09-27 at 15:13 +0000, Sage Weil wrote:
> On Tue, 27 Sep 2016, Jeff Layton wrote:
> > (tl;dr: we need to make some changes to the libcephfs API to allow
> >  proper credential handling. What's the right approach to take?)
> > 
> > Greg Farnum has posted a pull request to rework how credentials are
> > handled in cephfs:
> > 
> >     https://github.com/ceph/ceph/pull/11218
> > 
> > ...the patch pile is rather large, but the upshot is basically to
> > change the code not to carve out '-1' in the uid and gid ranges, and to
> > allow callers to pass down lists of supplementary groups. This is
> > mainly needed for nfs-ganesha, but it also allows ceph-fuse to delegate
> > permissions checks to the MDS.
> > 
> > That PR adds most of the low-level plumbing that's needed, but we still
> > need to expose this capability to applications via libcephfs. Many
> > libcephfs calls either take "int uid, int gid" or don't allow the
> > caller to send down creds at all.
> > 
> > I think what we'll want to do is allow applications to create a new
> > credential object and pass that down to the library in some fashion.
> > The question is: what's the best way to handle those API changes?
> > 
> > We have several options here:
> > 
> > 1) make a clean break with the old API: just change the arguments to
> > the existing functions, bump the library's major version and set the
> > "age" field in it to 0. If you built against old headers, it'll fail to
> > load the library at runtime. This is clearly the simplest option, but
> > requires everyone to rebuild applications that were built against the
> > old headers.
> 
> I think this is the way to go.  We'd bump the SONAME (libcephfs2!).  The 
> packaging infrastructure is such that you can have the old version of the 
> package (libcephfs1) installed at the same time as libcephfs2 (and 
> -devel), so stuff that's linked to the old API can still work and 
> coexist with stuff linked to the new one.  The only restriction (on debian 
> at least; I assume in rpm-land it is the same) is that you can only have 
> one version of the -dev package (headers) installed at a time, so you can 
> only compile new code against one version at a time.
> 

Ok, so basically we just rely on the linker picking the right lib at
build time. That would work too.

That said, the RPMs are currently named libcephfs1 and
libcephfs1-devel. I guess then we'd end up with a libcephfs2-devel. I
think we could add a "conflicts" directive in the specfile for it to
make sure we have a conflict with libcephfs1-devel.

I can live with that approach if that's the consensus. It sucks for
anyone who happens to have apps built against the old version, but
that's life...

> IIRC with the SONAME stuff you can specify the oldest compatible 
> version.  We can take the lazy route and skip #2 below and make that 2 as 
> well, which would mean installing libcephfs1 and libcephfs2 concurrently 
> for compatibility.
> 

You can do this. In libtool versioning parlance what you do is bump
"current" and "age", and set revision to 0:

https://www.gnu.org/software/libtool/manual/html_node/Updating-version-info.html

That was what I was figuring we'd do if we took option #2 below. If we
go with option #1 though, then we just bump "current" and set the other
two fields to 0.

> It seems like the key downside here is that any project building against 
> libcephfs that isn't updated will not be able to use newer versions of the 
> client until they move to the new API...
> 

Exactly.

AIUI though, there aren't really many apps in the field that use
libcephfs. The only two major ones I know of that aren't part of ceph
itself are nfs-ganesha and samba. I intend to fix both of them to use
the new API when it's available.

Are there other consumers of this library that I should be aware of?

> 
> 
> 
> > 
> > 2) create a "parallel" API that takes pointers to UserPerm objects, and
> > add new new calls for them. The old API then becomes a wrapper around
> > the new code. This would work, but it's more work -- we'd need to add a
> > whole swath of new calls that take this pointer. This has the advantage
> > of allowing existing programs to continue working through a lib
> > upgrade. For this, we'd bump the major lib version, and "age" field to
> > indicate that the library has a new API, but is backward compatible
> > with the old one. If we want to go this route, how should the new
> > functions be named?
> > 
> > 3) get tricky with global and thread local variables. Add calls to
> > allow users to set libcephfs creds at the process and thread level.
> > When the API is called, we'll check to see if creds have been installed
> > in either spot and use those if they're present. If they're not, then
> > we'll grab them from the current task (much like we do today). This is
> > somewhat analogous to using setuid/setgid/setgroups before making a
> > syscall. It's less intuitive than option #2 and requires callers to
> > manage their creds carefully, but means that we don't need to explode
> > out the API _and_ preserves the ability to run programs built against
> > the old headers.
> > 
> > It's a fair bit of work any way we approach it, so I want to be clear
> > on a direction before I dive in too deeply.
> > 
> > Thoughts?
> > -- 
> > Jeff Layton <jlayton@redhat.com>
> > --
> > To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > 

-- 
Jeff Layton <jlayton@redhat.com>

Re: libcephfs API changes for credential rework

[Nathan Cutler]

> That said, the RPMs are currently named libcephfs1 and
> libcephfs1-devel. I guess then we'd end up with a libcephfs2-devel. I
> think we could add a "conflicts" directive in the specfile for it to
> make sure we have a conflict with libcephfs1-devel.

Actually, in RPM we recently dropped the major version number from the 
devel package names, so in the kraken release we will have 
"libcephfs-devel" instead of "libcephfs1-devel". Same goes for the other 
lib packages like librados2, librbd1, etc.

It would make sense to do the same for the debian packages, right? The 
only reason to have the major version number in the package name is to 
enable multiple major versions of a library to be installed at the same 
time. But with the devel packages this is not possible - having the 
major version in the package name there is just confusing.

At least, this is how it was explained to me ;-)

Nathan

Re: libcephfs API changes for credential rework

[Jeff Layton]

On Tue, 2016-09-27 at 22:40 +0200, Nathan Cutler wrote:
> > 
> > That said, the RPMs are currently named libcephfs1 and
> > libcephfs1-devel. I guess then we'd end up with a libcephfs2-devel. I
> > think we could add a "conflicts" directive in the specfile for it to
> > make sure we have a conflict with libcephfs1-devel.
> 
> Actually, in RPM we recently dropped the major version number from the 
> devel package names, so in the kraken release we will have 
> "libcephfs-devel" instead of "libcephfs1-devel". Same goes for the other 
> lib packages like librados2, librbd1, etc.
> 
> It would make sense to do the same for the debian packages, right? The 
> only reason to have the major version number in the package name is to 
> enable multiple major versions of a library to be installed at the same 
> time. But with the devel packages this is not possible - having the 
> major version in the package name there is just confusing.
> 
> At least, this is how it was explained to me ;-)
> 
> Nathan

I think that should be fine. I don't think you'd want to have more than
one devel package installed. That just contains the headers and a
symlink anyway, and the headers would most likely conflict.

If you want to build vs. the old API then you'd need an older set of
devel packages (and binary libs to build against).

-- 
Jeff Layton <jlayton@redhat.com>

Re: libcephfs API changes for credential rework

[Jeff Layton]

On Tue, 2016-09-27 at 15:13 +0000, Sage Weil wrote:
> On Tue, 27 Sep 2016, Jeff Layton wrote:
> > (tl;dr: we need to make some changes to the libcephfs API to allow
> >  proper credential handling. What's the right approach to take?)
> > 
> > Greg Farnum has posted a pull request to rework how credentials are
> > handled in cephfs:
> > 
> >     https://github.com/ceph/ceph/pull/11218
> > 
> > ...the patch pile is rather large, but the upshot is basically to
> > change the code not to carve out '-1' in the uid and gid ranges, and to
> > allow callers to pass down lists of supplementary groups. This is
> > mainly needed for nfs-ganesha, but it also allows ceph-fuse to delegate
> > permissions checks to the MDS.
> > 
> > That PR adds most of the low-level plumbing that's needed, but we still
> > need to expose this capability to applications via libcephfs. Many
> > libcephfs calls either take "int uid, int gid" or don't allow the
> > caller to send down creds at all.
> > 
> > I think what we'll want to do is allow applications to create a new
> > credential object and pass that down to the library in some fashion.
> > The question is: what's the best way to handle those API changes?
> > 
> > We have several options here:
> > 
> > 1) make a clean break with the old API: just change the arguments to
> > the existing functions, bump the library's major version and set the
> > "age" field in it to 0. If you built against old headers, it'll fail to
> > load the library at runtime. This is clearly the simplest option, but
> > requires everyone to rebuild applications that were built against the
> > old headers.
> 
> I think this is the way to go.  We'd bump the SONAME (libcephfs2!).  The 
> packaging infrastructure is such that you can have the old version of the 
> package (libcephfs1) installed at the same time as libcephfs2 (and 
> -devel), so stuff that's linked to the old API can still work and 
> coexist with stuff linked to the new one.  The only restriction (on debian 
> at least; I assume in rpm-land it is the same) is that you can only have 
> one version of the -dev package (headers) installed at a time, so you can 
> only compile new code against one version at a time.
> 
> IIRC with the SONAME stuff you can specify the oldest compatible 
> version.  We can take the lazy route and skip #2 below and make that 2 as 
> well, which would mean installing libcephfs1 and libcephfs2 concurrently 
> for compatibility.
> 
> It seems like the key downside here is that any project building against 
> libcephfs that isn't updated will not be able to use newer versions of the 
> client until they move to the new API...
> 
> sage
> 

This brings up another point...

When I added the struct ceph_statx interfaces, I left the old struct
stat interfaces intact. So we now have ceph_statx and ceph_ll_getattrx,
etc. If we're making a clean API break where we change args on existing
function names, then we don't really need those new calls. 

Anyone have objections to me just converting the old struct stat based
functions to use struct ceph_statx instead, and dumping the new
function prototypes that I added?

It'll mean some code churn in the short term, but I think it'll make
for a simpler API later.


> 
> 
> > 
> > 2) create a "parallel" API that takes pointers to UserPerm objects, and
> > add new new calls for them. The old API then becomes a wrapper around
> > the new code. This would work, but it's more work -- we'd need to add a
> > whole swath of new calls that take this pointer. This has the advantage
> > of allowing existing programs to continue working through a lib
> > upgrade. For this, we'd bump the major lib version, and "age" field to
> > indicate that the library has a new API, but is backward compatible
> > with the old one. If we want to go this route, how should the new
> > functions be named?
> > 
> > 3) get tricky with global and thread local variables. Add calls to
> > allow users to set libcephfs creds at the process and thread level.
> > When the API is called, we'll check to see if creds have been installed
> > in either spot and use those if they're present. If they're not, then
> > we'll grab them from the current task (much like we do today). This is
> > somewhat analogous to using setuid/setgid/setgroups before making a
> > syscall. It's less intuitive than option #2 and requires callers to
> > manage their creds carefully, but means that we don't need to explode
> > out the API _and_ preserves the ability to run programs built against
> > the old headers.
> > 
> > It's a fair bit of work any way we approach it, so I want to be clear
> > on a direction before I dive in too deeply.
> > 
> > Thoughts?
> > -- 
> > Jeff Layton <jlayton@redhat.com>
> > --
> > To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > 

-- 
Jeff Layton <jlayton@redhat.com>

Re: libcephfs API changes for credential rework

[Gregory Farnum]

On Tue, Sep 27, 2016 at 10:39 AM, Jeff Layton <jlayton@redhat.com> wrote:
> On Tue, 2016-09-27 at 15:13 +0000, Sage Weil wrote:
>> On Tue, 27 Sep 2016, Jeff Layton wrote:
>> > (tl;dr: we need to make some changes to the libcephfs API to allow
>> >  proper credential handling. What's the right approach to take?)
>> >
>> > Greg Farnum has posted a pull request to rework how credentials are
>> > handled in cephfs:
>> >
>> >     https://github.com/ceph/ceph/pull/11218
>> >
>> > ...the patch pile is rather large, but the upshot is basically to
>> > change the code not to carve out '-1' in the uid and gid ranges, and to
>> > allow callers to pass down lists of supplementary groups. This is
>> > mainly needed for nfs-ganesha, but it also allows ceph-fuse to delegate
>> > permissions checks to the MDS.
>> >
>> > That PR adds most of the low-level plumbing that's needed, but we still
>> > need to expose this capability to applications via libcephfs. Many
>> > libcephfs calls either take "int uid, int gid" or don't allow the
>> > caller to send down creds at all.
>> >
>> > I think what we'll want to do is allow applications to create a new
>> > credential object and pass that down to the library in some fashion.
>> > The question is: what's the best way to handle those API changes?
>> >
>> > We have several options here:
>> >
>> > 1) make a clean break with the old API: just change the arguments to
>> > the existing functions, bump the library's major version and set the
>> > "age" field in it to 0. If you built against old headers, it'll fail to
>> > load the library at runtime. This is clearly the simplest option, but
>> > requires everyone to rebuild applications that were built against the
>> > old headers.
>>
>> I think this is the way to go.  We'd bump the SONAME (libcephfs2!).  The
>> packaging infrastructure is such that you can have the old version of the
>> package (libcephfs1) installed at the same time as libcephfs2 (and
>> -devel), so stuff that's linked to the old API can still work and
>> coexist with stuff linked to the new one.  The only restriction (on debian
>> at least; I assume in rpm-land it is the same) is that you can only have
>> one version of the -dev package (headers) installed at a time, so you can
>> only compile new code against one version at a time.
>>
>> IIRC with the SONAME stuff you can specify the oldest compatible
>> version.  We can take the lazy route and skip #2 below and make that 2 as
>> well, which would mean installing libcephfs1 and libcephfs2 concurrently
>> for compatibility.
>>
>> It seems like the key downside here is that any project building against
>> libcephfs that isn't updated will not be able to use newer versions of the
>> client until they move to the new API...
>>
>> sage
>>
>
> This brings up another point...
>
> When I added the struct ceph_statx interfaces, I left the old struct
> stat interfaces intact. So we now have ceph_statx and ceph_ll_getattrx,
> etc. If we're making a clean API break where we change args on existing
> function names, then we don't really need those new calls.
>
> Anyone have objections to me just converting the old struct stat based
> functions to use struct ceph_statx instead, and dumping the new
> function prototypes that I added?
>
> It'll mean some code churn in the short term, but I think it'll make
> for a simpler API later.

Yeah, that makes sense as long as we're going incompatible.

Here's another question, though: should we actually copy the source
code to create libcephfs2, and generate libcephfs1 through the
Luminous release to give anybody who is running stuff out there a
window to change over smoothly? I don't expect we'd need to actually
change the libcephfs1 source code; certainly it won't take much...
-Greg

Re: libcephfs API changes for credential rework

[Jeff Layton]

On Tue, 2016-09-27 at 11:17 -0700, Gregory Farnum wrote:
> On Tue, Sep 27, 2016 at 10:39 AM, Jeff Layton <jlayton@redhat.com> wrote:
> > 
> > On Tue, 2016-09-27 at 15:13 +0000, Sage Weil wrote:
> > > 
> > > On Tue, 27 Sep 2016, Jeff Layton wrote:
> > > > 
> > > > (tl;dr: we need to make some changes to the libcephfs API to allow
> > > >  proper credential handling. What's the right approach to take?)
> > > > 
> > > > Greg Farnum has posted a pull request to rework how credentials are
> > > > handled in cephfs:
> > > > 
> > > >     https://github.com/ceph/ceph/pull/11218
> > > > 
> > > > ...the patch pile is rather large, but the upshot is basically to
> > > > change the code not to carve out '-1' in the uid and gid ranges, and to
> > > > allow callers to pass down lists of supplementary groups. This is
> > > > mainly needed for nfs-ganesha, but it also allows ceph-fuse to delegate
> > > > permissions checks to the MDS.
> > > > 
> > > > That PR adds most of the low-level plumbing that's needed, but we still
> > > > need to expose this capability to applications via libcephfs. Many
> > > > libcephfs calls either take "int uid, int gid" or don't allow the
> > > > caller to send down creds at all.
> > > > 
> > > > I think what we'll want to do is allow applications to create a new
> > > > credential object and pass that down to the library in some fashion.
> > > > The question is: what's the best way to handle those API changes?
> > > > 
> > > > We have several options here:
> > > > 
> > > > 1) make a clean break with the old API: just change the arguments to
> > > > the existing functions, bump the library's major version and set the
> > > > "age" field in it to 0. If you built against old headers, it'll fail to
> > > > load the library at runtime. This is clearly the simplest option, but
> > > > requires everyone to rebuild applications that were built against the
> > > > old headers.
> > > 
> > > I think this is the way to go.  We'd bump the SONAME (libcephfs2!).  The
> > > packaging infrastructure is such that you can have the old version of the
> > > package (libcephfs1) installed at the same time as libcephfs2 (and
> > > -devel), so stuff that's linked to the old API can still work and
> > > coexist with stuff linked to the new one.  The only restriction (on debian
> > > at least; I assume in rpm-land it is the same) is that you can only have
> > > one version of the -dev package (headers) installed at a time, so you can
> > > only compile new code against one version at a time.
> > > 
> > > IIRC with the SONAME stuff you can specify the oldest compatible
> > > version.  We can take the lazy route and skip #2 below and make that 2 as
> > > well, which would mean installing libcephfs1 and libcephfs2 concurrently
> > > for compatibility.
> > > 
> > > It seems like the key downside here is that any project building against
> > > libcephfs that isn't updated will not be able to use newer versions of the
> > > client until they move to the new API...
> > > 
> > > sage
> > > 
> > 
> > This brings up another point...
> > 
> > When I added the struct ceph_statx interfaces, I left the old struct
> > stat interfaces intact. So we now have ceph_statx and ceph_ll_getattrx,
> > etc. If we're making a clean API break where we change args on existing
> > function names, then we don't really need those new calls.
> > 
> > Anyone have objections to me just converting the old struct stat based
> > functions to use struct ceph_statx instead, and dumping the new
> > function prototypes that I added?
> > 
> > It'll mean some code churn in the short term, but I think it'll make
> > for a simpler API later.
> 
> Yeah, that makes sense as long as we're going incompatible.
> 
> Here's another question, though: should we actually copy the source
> code to create libcephfs2, and generate libcephfs1 through the
> Luminous release to give anybody who is running stuff out there a
> window to change over smoothly? I don't expect we'd need to actually
> change the libcephfs1 source code; certainly it won't take much...
> -Greg

Sure, I don't think it'd be too hard to do that, just a little shim
layer on top of the new API.

Here's another thing too though. We have both java and python bindings
in the tree as well. They don't really deal with creds, but they do
wrap ceph_stat() and friends...should I convert those over to use the
statx-based APIs as well?

-- 
Jeff Layton <jlayton@redhat.com>

Re: libcephfs API changes for credential rework

[Gregory Farnum]

On Tue, Sep 27, 2016 at 12:10 PM, Jeff Layton <jlayton@redhat.com> wrote:
> On Tue, 2016-09-27 at 11:17 -0700, Gregory Farnum wrote:
>> On Tue, Sep 27, 2016 at 10:39 AM, Jeff Layton <jlayton@redhat.com> wrote:
>> >
>> > On Tue, 2016-09-27 at 15:13 +0000, Sage Weil wrote:
>> > >
>> > > On Tue, 27 Sep 2016, Jeff Layton wrote:
>> > > >
>> > > > (tl;dr: we need to make some changes to the libcephfs API to allow
>> > > >  proper credential handling. What's the right approach to take?)
>> > > >
>> > > > Greg Farnum has posted a pull request to rework how credentials are
>> > > > handled in cephfs:
>> > > >
>> > > >     https://github.com/ceph/ceph/pull/11218
>> > > >
>> > > > ...the patch pile is rather large, but the upshot is basically to
>> > > > change the code not to carve out '-1' in the uid and gid ranges, and to
>> > > > allow callers to pass down lists of supplementary groups. This is
>> > > > mainly needed for nfs-ganesha, but it also allows ceph-fuse to delegate
>> > > > permissions checks to the MDS.
>> > > >
>> > > > That PR adds most of the low-level plumbing that's needed, but we still
>> > > > need to expose this capability to applications via libcephfs. Many
>> > > > libcephfs calls either take "int uid, int gid" or don't allow the
>> > > > caller to send down creds at all.
>> > > >
>> > > > I think what we'll want to do is allow applications to create a new
>> > > > credential object and pass that down to the library in some fashion.
>> > > > The question is: what's the best way to handle those API changes?
>> > > >
>> > > > We have several options here:
>> > > >
>> > > > 1) make a clean break with the old API: just change the arguments to
>> > > > the existing functions, bump the library's major version and set the
>> > > > "age" field in it to 0. If you built against old headers, it'll fail to
>> > > > load the library at runtime. This is clearly the simplest option, but
>> > > > requires everyone to rebuild applications that were built against the
>> > > > old headers.
>> > >
>> > > I think this is the way to go.  We'd bump the SONAME (libcephfs2!).  The
>> > > packaging infrastructure is such that you can have the old version of the
>> > > package (libcephfs1) installed at the same time as libcephfs2 (and
>> > > -devel), so stuff that's linked to the old API can still work and
>> > > coexist with stuff linked to the new one.  The only restriction (on debian
>> > > at least; I assume in rpm-land it is the same) is that you can only have
>> > > one version of the -dev package (headers) installed at a time, so you can
>> > > only compile new code against one version at a time.
>> > >
>> > > IIRC with the SONAME stuff you can specify the oldest compatible
>> > > version.  We can take the lazy route and skip #2 below and make that 2 as
>> > > well, which would mean installing libcephfs1 and libcephfs2 concurrently
>> > > for compatibility.
>> > >
>> > > It seems like the key downside here is that any project building against
>> > > libcephfs that isn't updated will not be able to use newer versions of the
>> > > client until they move to the new API...
>> > >
>> > > sage
>> > >
>> >
>> > This brings up another point...
>> >
>> > When I added the struct ceph_statx interfaces, I left the old struct
>> > stat interfaces intact. So we now have ceph_statx and ceph_ll_getattrx,
>> > etc. If we're making a clean API break where we change args on existing
>> > function names, then we don't really need those new calls.
>> >
>> > Anyone have objections to me just converting the old struct stat based
>> > functions to use struct ceph_statx instead, and dumping the new
>> > function prototypes that I added?
>> >
>> > It'll mean some code churn in the short term, but I think it'll make
>> > for a simpler API later.
>>
>> Yeah, that makes sense as long as we're going incompatible.
>>
>> Here's another question, though: should we actually copy the source
>> code to create libcephfs2, and generate libcephfs1 through the
>> Luminous release to give anybody who is running stuff out there a
>> window to change over smoothly? I don't expect we'd need to actually
>> change the libcephfs1 source code; certainly it won't take much...
>> -Greg
>
> Sure, I don't think it'd be too hard to do that, just a little shim
> layer on top of the new API.
>
> Here's another thing too though. We have both java and python bindings
> in the tree as well. They don't really deal with creds, but they do
> wrap ceph_stat() and friends...should I convert those over to use the
> statx-based APIs as well?

I dunno about the python ones. The Java API is really only used by our
Hadoop bindings AFAIK so it'd be dictated by that system's needs. I
imagine we'd want to swap them to use our new interfaces for ease of
maintenance, but if it's simpler we don't need to swap out their
exterior APIs to expose statx until there's demand?

Re: libcephfs API changes for credential rework

[Jeff Layton]

On Tue, 2016-09-27 at 12:13 -0700, Gregory Farnum wrote:
> On Tue, Sep 27, 2016 at 12:10 PM, Jeff Layton <jlayton@redhat.com> wrote:
> > 
> > On Tue, 2016-09-27 at 11:17 -0700, Gregory Farnum wrote:
> > > 
> > > On Tue, Sep 27, 2016 at 10:39 AM, Jeff Layton <jlayton@redhat.com> wrote:
> > > > 
> > > > 
> > > > On Tue, 2016-09-27 at 15:13 +0000, Sage Weil wrote:
> > > > > 
> > > > > 
> > > > > On Tue, 27 Sep 2016, Jeff Layton wrote:
> > > > > > 
> > > > > > 
> > > > > > (tl;dr: we need to make some changes to the libcephfs API to allow
> > > > > >  proper credential handling. What's the right approach to take?)
> > > > > > 
> > > > > > Greg Farnum has posted a pull request to rework how credentials are
> > > > > > handled in cephfs:
> > > > > > 
> > > > > >     https://github.com/ceph/ceph/pull/11218
> > > > > > 
> > > > > > ...the patch pile is rather large, but the upshot is basically to
> > > > > > change the code not to carve out '-1' in the uid and gid ranges, and to
> > > > > > allow callers to pass down lists of supplementary groups. This is
> > > > > > mainly needed for nfs-ganesha, but it also allows ceph-fuse to delegate
> > > > > > permissions checks to the MDS.
> > > > > > 
> > > > > > That PR adds most of the low-level plumbing that's needed, but we still
> > > > > > need to expose this capability to applications via libcephfs. Many
> > > > > > libcephfs calls either take "int uid, int gid" or don't allow the
> > > > > > caller to send down creds at all.
> > > > > > 
> > > > > > I think what we'll want to do is allow applications to create a new
> > > > > > credential object and pass that down to the library in some fashion.
> > > > > > The question is: what's the best way to handle those API changes?
> > > > > > 
> > > > > > We have several options here:
> > > > > > 
> > > > > > 1) make a clean break with the old API: just change the arguments to
> > > > > > the existing functions, bump the library's major version and set the
> > > > > > "age" field in it to 0. If you built against old headers, it'll fail to
> > > > > > load the library at runtime. This is clearly the simplest option, but
> > > > > > requires everyone to rebuild applications that were built against the
> > > > > > old headers.
> > > > > 
> > > > > I think this is the way to go.  We'd bump the SONAME (libcephfs2!).  The
> > > > > packaging infrastructure is such that you can have the old version of the
> > > > > package (libcephfs1) installed at the same time as libcephfs2 (and
> > > > > -devel), so stuff that's linked to the old API can still work and
> > > > > coexist with stuff linked to the new one.  The only restriction (on debian
> > > > > at least; I assume in rpm-land it is the same) is that you can only have
> > > > > one version of the -dev package (headers) installed at a time, so you can
> > > > > only compile new code against one version at a time.
> > > > > 
> > > > > IIRC with the SONAME stuff you can specify the oldest compatible
> > > > > version.  We can take the lazy route and skip #2 below and make that 2 as
> > > > > well, which would mean installing libcephfs1 and libcephfs2 concurrently
> > > > > for compatibility.
> > > > > 
> > > > > It seems like the key downside here is that any project building against
> > > > > libcephfs that isn't updated will not be able to use newer versions of the
> > > > > client until they move to the new API...
> > > > > 
> > > > > sage
> > > > > 
> > > > 
> > > > This brings up another point...
> > > > 
> > > > When I added the struct ceph_statx interfaces, I left the old struct
> > > > stat interfaces intact. So we now have ceph_statx and ceph_ll_getattrx,
> > > > etc. If we're making a clean API break where we change args on existing
> > > > function names, then we don't really need those new calls.
> > > > 
> > > > Anyone have objections to me just converting the old struct stat based
> > > > functions to use struct ceph_statx instead, and dumping the new
> > > > function prototypes that I added?
> > > > 
> > > > It'll mean some code churn in the short term, but I think it'll make
> > > > for a simpler API later.
> > > 
> > > Yeah, that makes sense as long as we're going incompatible.
> > > 
> > > Here's another question, though: should we actually copy the source
> > > code to create libcephfs2, and generate libcephfs1 through the
> > > Luminous release to give anybody who is running stuff out there a
> > > window to change over smoothly? I don't expect we'd need to actually
> > > change the libcephfs1 source code; certainly it won't take much...
> > > -Greg
> > 
> > Sure, I don't think it'd be too hard to do that, just a little shim
> > layer on top of the new API.
> > 
> > Here's another thing too though. We have both java and python bindings
> > in the tree as well. They don't really deal with creds, but they do
> > wrap ceph_stat() and friends...should I convert those over to use the
> > statx-based APIs as well?
> 
> I dunno about the python ones. The Java API is really only used by our
> Hadoop bindings AFAIK so it'd be dictated by that system's needs. I
> imagine we'd want to swap them to use our new interfaces for ease of
> maintenance, but if it's simpler we don't need to swap out their
> exterior APIs to expose statx until there's demand?

Maybe, but it might be good to go ahead and convert them too.

The older interfaces were pretty heavy on the caps requirements. The
ceph_statx APIs allow clients to get away with only the caps they need
for the fields that they are interested in. I imagine hadoop workloads
might benefit from that.

Fixing the bindings themselves looks pretty simple, but I'd need
someone else to drive fixing hadoop and the python consumers of them.

-- 
Jeff Layton <jlayton@redhat.com>