Re: [openstack-dev] [keystone] Adding foreign keys between subsystems

[David Stanek]

On 12-Apr 15:25, Rodrigo Duarte wrote:
> On Wed, Apr 12, 2017 at 2:47 PM, David Stanek  wrote:
> 
> > On 12-Apr 14:30, Rodrigo Duarte wrote:
> > > Just to illustrate the discussion, we have a bug fix that currently tries
> > > to drop a FK between the federation and identity subsystems [1].
> > >
> > > [1] https://review.openstack.org/#/c/445505/
> >
> > I think this highlights one of my problems with the current architecture.
> > I see that
> > you've removed the FK and added delete logic to do what the data layer
> > would be doing
> > for you. I didn't see any added get_user() checks to make sure the user_id
> > being used
> > in creates/updates is valid. Are we already checking that somewhere else
> > or is this
> > introducing a new bug?
> >
> 
> The review [1] is dropping the idp_id and idp_id + protocol_id FKs, not the
> user_id one.
> 

Right, misremembered. Just run s/user_id/idp_id/ and s/get_user/get_idp/ and 
you'll
have the same issues.

-- 
david stanek
web: https://dstanek.com
twitter: https://twitter.com/dstanek

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [keystone] Adding foreign keys between subsystems

[Rodrigo Duarte]

On Wed, Apr 12, 2017 at 2:47 PM, David Stanek  wrote:

> On 12-Apr 14:30, Rodrigo Duarte wrote:
> > Just to illustrate the discussion, we have a bug fix that currently tries
> > to drop a FK between the federation and identity subsystems [1].
> >
> > [1] https://review.openstack.org/#/c/445505/
>
> I think this highlights one of my problems with the current architecture.
> I see that
> you've removed the FK and added delete logic to do what the data layer
> would be doing
> for you. I didn't see any added get_user() checks to make sure the user_id
> being used
> in creates/updates is valid. Are we already checking that somewhere else
> or is this
> introducing a new bug?
>

The review [1] is dropping the idp_id and idp_id + protocol_id FKs, not the
user_id one.


>
> --
> david stanek
> web: https://dstanek.com
> twitter: https://twitter.com/dstanek
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>



-- 
Rodrigo Duarte Sousa
Senior Quality Engineer @ Red Hat
MSc in Computer Science
http://rodrigods.com
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [keystone] Adding foreign keys between subsystems

[David Stanek]

On 12-Apr 14:30, Rodrigo Duarte wrote:
> Just to illustrate the discussion, we have a bug fix that currently tries
> to drop a FK between the federation and identity subsystems [1].
> 
> [1] https://review.openstack.org/#/c/445505/

I think this highlights one of my problems with the current architecture. I see 
that
you've removed the FK and added delete logic to do what the data layer would be 
doing
for you. I didn't see any added get_user() checks to make sure the user_id 
being used
in creates/updates is valid. Are we already checking that somewhere else or is 
this
introducing a new bug?

-- 
david stanek
web: https://dstanek.com
twitter: https://twitter.com/dstanek

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [keystone] Adding foreign keys between subsystems

[Rodrigo Duarte]

Just to illustrate the discussion, we have a bug fix that currently tries
to drop a FK between the federation and identity subsystems [1].

The previous fix for this bug (which has been merged and had the backport
abandoned) took advantage of the FK in order to cascade delete federated
users when a protocol or an identity provider is deleted [2].

[1] https://review.openstack.org/#/c/445505/
[2] https://review.openstack.org/#/c/420893/

On Wed, Apr 12, 2017 at 11:58 AM, Lance Bragstad 
wrote:

>
>
> On Wed, Apr 12, 2017 at 9:28 AM, David Stanek  wrote:
>
>> [tl;dr I want to remove the artificial restriction of not allowing FKs
>> between
>> subsystems and I want to stop FK enforcement in code.]
>>
>> The keystone code architecture is pretty simple. The data and
>> functionality are
>> divided up into subsystems. Each subsystem can be configured to use a
>> different
>> backend datastore. Of course, there are always exceptions to the rule
>> like how
>> the federation and identity subsystems are highly coupled in the data
>> model.
>>
>> On the surface this flexible model sounds good, but there are some
>> interesting
>> consequences. First, you can't tell from looking at the data model that
>> there
>> actually is a lot of coupling between the subsystems. So instead of
>> looking at
>> our sqlalchemy models to see relationships, we must look throughout the
>> code
>> for where a particular primary key is used and look for enforcement.
>> (Hopefully
>> we enforce it in all of the right places.) Additionally, a DBA/data
>> architect/
>> whenever can't see the relationship at all by looking at the database.
>>
>> Second, this has polluted our code and causes erroneous API errors. We
>> have added
>> lots of get_*() calls in our code that checks for the existence of IDs in
>> another subsystem. In some cases we probably don't do the check and thus
>> would
>> allow bad data to be stored. The check often causes 404s instead of 400s
>> when
>> bad data is provided.
>>
>
> Having these cleaned up would be awesome. I imagine we'd also see some
> sort of performance benefit as a result, too.
>
>
>>
>> So I'd like us to be more deliberate in defining which parts of the data
>> model
>> are truly independent and a separate backend datastore would make sense.
>> For
>> instance, we know we want to support LDAP for identity (although this
>> still puts
>> identity info into a SQL database) and catalog is very isolated from the
>> rest of
>> the data model.
>>
>> As a side effect this means that if deployers wished to use a separate
>> backend
>> for something they would need to also implement it for the other highly
>> coupled
>> subsystems. They would also have to provide any FK enforcement that their
>> own
>> datastore does not provide.
>>
>
> So for deployers, this would mean that if today they only deploy keystone
> backed with LDAP for *only* identity, tomorrow they will have to ensure
> that LDAP has all the proper things for other subsystems that use to have
> an in-code constraint with identity (i.e. assignment). I wonder how many
> folks that would be? What would an upgrade look like for deployments
> wishing to stick to LDAP? I imagine we'd be raising the bar for that
> particular upgrade.
>
>
>>
>> Thoughts?
>>
>> --
>> david stanek
>> web: https://dstanek.com
>> twitter: https://twitter.com/dstanek
>>
>> 
>> __
>> OpenStack Development Mailing List (not for usage questions)
>> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscrib
>> e
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
>


-- 
Rodrigo Duarte Sousa
Senior Quality Engineer @ Red Hat
MSc in Computer Science
http://rodrigods.com
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [keystone] Adding foreign keys between subsystems

[Lance Bragstad]

On Wed, Apr 12, 2017 at 9:28 AM, David Stanek  wrote:

> [tl;dr I want to remove the artificial restriction of not allowing FKs
> between
> subsystems and I want to stop FK enforcement in code.]
>
> The keystone code architecture is pretty simple. The data and
> functionality are
> divided up into subsystems. Each subsystem can be configured to use a
> different
> backend datastore. Of course, there are always exceptions to the rule like
> how
> the federation and identity subsystems are highly coupled in the data
> model.
>
> On the surface this flexible model sounds good, but there are some
> interesting
> consequences. First, you can't tell from looking at the data model that
> there
> actually is a lot of coupling between the subsystems. So instead of
> looking at
> our sqlalchemy models to see relationships, we must look throughout the
> code
> for where a particular primary key is used and look for enforcement.
> (Hopefully
> we enforce it in all of the right places.) Additionally, a DBA/data
> architect/
> whenever can't see the relationship at all by looking at the database.
>
> Second, this has polluted our code and causes erroneous API errors. We
> have added
> lots of get_*() calls in our code that checks for the existence of IDs in
> another subsystem. In some cases we probably don't do the check and thus
> would
> allow bad data to be stored. The check often causes 404s instead of 400s
> when
> bad data is provided.
>

Having these cleaned up would be awesome. I imagine we'd also see some sort
of performance benefit as a result, too.


>
> So I'd like us to be more deliberate in defining which parts of the data
> model
> are truly independent and a separate backend datastore would make sense.
> For
> instance, we know we want to support LDAP for identity (although this
> still puts
> identity info into a SQL database) and catalog is very isolated from the
> rest of
> the data model.
>
> As a side effect this means that if deployers wished to use a separate
> backend
> for something they would need to also implement it for the other highly
> coupled
> subsystems. They would also have to provide any FK enforcement that their
> own
> datastore does not provide.
>

So for deployers, this would mean that if today they only deploy keystone
backed with LDAP for *only* identity, tomorrow they will have to ensure
that LDAP has all the proper things for other subsystems that use to have
an in-code constraint with identity (i.e. assignment). I wonder how many
folks that would be? What would an upgrade look like for deployments
wishing to stick to LDAP? I imagine we'd be raising the bar for that
particular upgrade.


>
> Thoughts?
>
> --
> david stanek
> web: https://dstanek.com
> twitter: https://twitter.com/dstanek
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev