Add performance index for token resolution #2932
Conversation
* add external_key to identity_provider * add index and optional search in retrieveByIssuer * add external_key for OAuth/OIDC and SAML IdP
|
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/187807023 The labels on this github issue will be updated when the story is started. |
…821/addExternId # Conflicts: # server/src/main/java/org/cloudfoundry/identity/uaa/provider/JdbcIdentityProviderProvisioning.java
hint: we do not support RFC 7523 on server side. will come after PR #2932 . We need the index for resolution of iss in case of support of RFC 7523
only postgres supports them
- shorten the index name. Similar name pattern then existing
* Fix documentation for OpenID connect clientJWT hint: we do not support RFC 7523 on server side. will come after PR #2932 . We need the index for resolution of iss in case of support of RFC 7523 * update * review
|
I am a bit concerned about the performance impact for searching by and having a unique index on a text column. Have we done any performance testing for this change? |
There was a problem hiding this comment.
Should we not use CONCURRENTLY for PostgreSQL?
There was a problem hiding this comment.
In postgresq we have the partial index feature, means, the index is growing because at the begin we dont have values in external_key so I dont see an issue with the index creation .
In large tables the concurrently helps to create the index outside of flyway transaction....
| @@ -0,0 +1,3 @@ | |||
| -- add column external_key for oauth2,oidc,saml2 IdPs | |||
| ALTER TABLE identity_provider ADD COLUMN external_key TEXT DEFAULT NULL; | |||
| CREATE UNIQUE INDEX external_key_in_zone on identity_provider (identity_zone_id(36),type(8),external_key(724)); | |||
There was a problem hiding this comment.
you see, mysql cannot use the full text index but I had to limit it....
Will think about to change to VARCHAR(512) maybe...
yes, of course I have the explain and index is used in both DBs. The loop over all IdP is configurable because I dont want to change the behavior for all, but we will move to new one. |
|
@torsten-sap Should we limit the size of external id to maybe 512 ? |
You mean the uniqueness was already being checked in Java runtime, and we are adding the index in DB this time? So there is no added performance degradation with this change in that regard? |
yes, in OIDC and SAML there are checks, but currently these checks loop. and this is a performance issuer right now ... if you have many IdP, see OIDC runtime as example I will change the text field into VARCHAR(512) so that mysql does not have an issue with it. |
here is the SAML check for duplicate That is the reason why I added the index as unique, because we have the checks in runtime are there but without a performant access and simply via select * in DB.... but this is a problem in case of many IdPs |
Verified on DB level with explain that index is used.
Add unique index with constrains: identity zone, type, external-id, means that you can have a SAML IDP and OIDC IDP with same issuer / entityID. This can happen and should be allowed. What should be prevented to have 2 IdPs with same entityID or issuer in configuration.
To stay compatible, introduce allowAllOrigins option. Currently we loop in REST calls and check uniqueness of issuer/entityId. Admin can decide to remove this loop in some time.