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

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

[David Stanek]

[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.

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.

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