Repository navigation
Replies: 49 comments
|
To generalize this a bit, this is a problem for multi-tenant realms–where the same clients are used across many tenants, and each tenant may configure its own IdP. We might expect up to several thousand IdPs for example. To generalize even further, what else is included in a realm representation in the admin console or in admin events? If there are any other unbounded arrays like IdPs, it becomes a soft limit on scaling those objects within a realm. I see many other high cardinality lists (e.g. groups, users) in the realm representation, but of course as users don't cause this problem, it's not clear why some are populated (IdPs) and not others (users) in the representation. |
|
@jsorah @alechenninger I suppose that when you are updating realm, you are using admin console. Is it correct? It seems that what is going is:
Options for fixing this
My vote is (2.a) as this is the easiest option and IMO most proper. We may need to document this in the upgrading guide that realm GET endpoint won't return IDP and IDP mappers anymore. If we cannot break admin REST API (I am not 100% sure what the general contract is for admin REST API), then we can go 2.b. BTV. If we have versioning support for admin REST API, then this will be easy as we can go 2.a and increase version without any side-effect. Workaround optionI think the easiest workaround is to handle this at admin console level. You can create your own theme (and/or possibly directly update "themes" folder in the distribution, I am not 100% sure if this works, so you can try first...) and update some file (I think it is in Another workaround is to use admin REST API directly and make sure that you don't sent IDP and IDP mappers in the realm PUT request |
@alechenninger One question to this: Is this few thousand IdPs per realm or per whole deployment? If it is per whole deployment, then how many IdPs do you expect per realm? I think Keycloak actually does not expect that realm will contain thousands (or even tens) of IdPs and it is possible there are more similar issues. For example there is no yet pagination for IdPs (at admin console, REST API and model layer). I suggest to try test with some bigger number of IdPs in your stage/test environment to avoid some unexpected surprises as more bugs can happen once you start increasing count of IdPs in production :-/ |
|
Thank you @mposolda . Very helpful! I agree with your suggestion (2a), but 2b seems also like a reasonable compatible fix. Might need @stianst to weigh in on what the policy is around admin API compatibility from Keycloak to Keycloak release, but based on some internal discussion I think we want to avoid breaking changes. Let's continue that discussion on options internally for now.
It is per realm. We are just starting to look at the scaling issues here. Right now we administratively still control how many we have configured, so we can stop adding them if there are problems. We are investigating what issues we might see with self-service which is what we expect will take us into the 100s or 1000s. We will work on discovering what else may come up as we increase this number well before prod of course :). |
|
I believe this is a more general problem - events cannot store detail, including representation, longer than what the underlying engine supports. Even if it was fixed for one REST endpoint, same issue could occur for any other endpoint that updates an object with representation longer than that limit. The only way out IMO is to enable limit for the event detail, i.e. representation in this case. There is already a similar functionality for login events implemented in #6576, and I'd argue the same limit (set via Technically speaking, it means applying Or perhaps better, introducing a new parameter which would set the maximum field length |
|
I agree but I think both things are true–the API is asymmetric: it "GETs" what it does not equivalently "PUT." This is surprising. Additionally, the "GET" returns a potentially high cardinality array of objects which you can otherwise query separately. So even ignoring event storage, that is a problem. I agree with your point also, @hmlnarik , about the event storage generally. |
|
Would it make sense to break this issue up into these separate concerns, so the conversations don't get too confused, as they are both valid?
|
Absolutely, these are two issues. The first one is a no-brainer for me; I'm very reluctant to implementing the second as proposed though since this is a breaking change. I'd rather define a new REST API (v2) and migration path off the current one. |
|
I agree that it will be ideal to address both issues (trimming the events as well as stop sending IDP in GET-realm request). I wasn't aware of The One way to handle REST API in backwards compatible way is to introduce some flag like At the same time, there will be likely much more things to address if we want to support thousands of IDP per realm? Looks like IDP should be handled same way like the other objects, which are supported for long count (like clients, groups, roles etc). So looks like we may need pagination, better searching support, not sure if also something like So are we in agreement, that as "milestone 1", we will just trim the admin events, which will solve this particular issue? Then as "milestone 2+", we can handle the other things related to IDP scaling (which can be much more work...). |
|
From what I can infer there are actually two issues here:
One open question: how are realms updated when seeing this issue? My guess is that it is only through the admin console? In terms of what team on the Keycloak side should handle this it seems it should be the core features team, as from what we've discussed so far no one has suggested increasing the support for the database to store larger representations in the events. Interestingly enough we have a relevant PR from the community that introduces pagination for IdPs in the old admin console: We could pick up this PR and likely also include an option in the get realm endpoint (&excludeIdps=true or something like that) to make this a non-breaking change. The admin console would always use this, and would need to use a paginated endpoint to fetch the list of IdPs. Another tweak we could make is to clear out the IdPs list before storing the admin event. It doesn't make much sense to include the list of IdPs in the admin event as this list is ignored while updating the realm anyways. Long term for sure we need a new version of admin endpoints that handle these (and many other cases) more consistently. However, that is not something that is going to happen anytime soon, so we should patch up the current approach as cleanly as we can. |
I'd agree, but I would go as far as considering backwards-compatible REST API change you suggested in milestone 1.
I agree that this could be a short-term workaround and thus think this could be a separate issue for the first milestone. I rely on your statement that admin console doesn't need list of IDPs in the GET-realm response, I have not checked that. @ssilvert could perhaps confirm that is the case. So in total, I can see 3 issues:
I would think this is not exactly necessary, and API-wise the potentially needed changes can still fit the |
|
I wouldn't truncate admin event rep, but rather do the simple thing here which is to remove the list of IdPs, as these are not considered when updating the realm. For the admin console for this to scale we would need the ability to paginate these, and have just checked that the new admin console is not fetching any separate list of IdPs, so I presume it is currently just using the list from the realm rep. |
Not truncating admin event rep would cause the whole transaction to roll back, so the realm would not be updatable, just as stated in the bug report. Is this desired? |
As mentioned we don't need to include the list of IdPs in the admin realm update event as this field is ignored when updating a realm. Once that is been removed the size of the event shouldn't be an issue, and there's no need for a separate truncating option. |
|
Do we know that there is no other place suffering from this issue? |
What does not solve it for any other long fields? If it's removing list of IdPs from the rep, it doesn't have to solve it for other fields. If it's for the follow-up, I suggest we discuss that as a follow-up and get this issue out of the way first. |
There are more callers of If there is storing representation enabled, then the any of those operations would be effectively blocked until it is fixed by a fix of yet another keycloak bug. I struggle to see why there is such an objection against trimming, especially given that we already have the trimming of details available to the admin. Do we prefer having the admins being effectively blocked on MySQL / MariaDB until next Keycloak version is released? |
|
Based on today's meeting between myself, @mposolda and @stianst, the trimming has been agreed on to be included, to be used by admin for emergencies. This does not mean it should be used by default but as a stopgap solution until the cause is properly resolved |
|
Removing storage area label (trimming implemented as #14891) from this issue since it now only relates admin API and UI. |
|
We currently have an in-progress API implementation here https://github.com/agandhew/keycloak/tree/fix-realm-api It is not completely ready for review or merge but it is being worked on @agandhew tagging you for visibility |
|
@pedroigor It's from sso.rh team and hence I consider it as 22 for now. |
|
I've created a PR to add the brief representation to the GET path for the realm resource, however I want to make sure I'm addressing all concerns Reading through this entire thread it seems that there's also talk about a couple of other solutions:
It seems that the second of these bullets has been dropped in favor of the brief representation param, but I'm wondering about the first. Reading through the code, |
|
@mposolda @Redhat-Alice Isn't this issue now solved by #14891? What I understand from the discussions is that trimming the details should be the solution. @Redhat-Alice The PR you sent does not seem to help here because the admin console also needs to use the FTR, I would also say this issue is related to supporting PATCH operations in our REST APIs. The administration console relies on the full realm representation to manage settings from different tabs. Although it makes things easier, it has some drawbacks. At the same time, I'm not sure if we should be so strict and provide endpoints for the different aspects you can manage from a realm. The last points are a different discussion. Just wanted to expose them here as I think it is related to the core issue we are trying to solve. |
|
This issue thread was incredibly long with a lot of ideas on how to solve the issue that went in different directions. I made my PR based on these points above #14851 (comment) With the issue splitting in mind, my focus was on providing a non-breaking way of electing to ignore the identity providers when calling this endpoint which I believe also allows for the admin-console to call this endpoint and get the brief representation and thus never log it in the admin event. If this is a flawed approach to that then I can adjust |
|
@Redhat-Alice Not saying it is flawed but to properly leverage your change we also need updates to the admin console. Otherwise, it is still fetching/sending the full representation. Do you think that the already existing support for trimming the representation when persisting the event is enough for sso.redhat.com? That is basically what I'm trying to figure out. |
|
I believe that this does technically solve the initial issue, though I do agree with others that throwing away data when given another option is not ideal. I think #14891 will be fine in the short term but in the long term subverting this issue without potentially sacrificing important data would be ideal! |
|
@jsorah do you want to weigh in since you were the original issue reporter? |
|
I've converted into discussion as handling of many Idps is complicated task, which may be done in stages. And this issue already contains long discussion, which touched multiple aspects of "handle many identity providers". Creating epic #21071 with various subtasks for the individual pieces. We can possibly continue discussion here if needed (or on individual PR/issues as they come around). |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Describe the bug
Realm update fails when a realm has many Identity Providers configured and also is saving Admin Events with the representation of the request.
Concretely, our Keycloak instance has approximately 34 IDPs configured - when I attempt to update another, unrelated configuration item (such as the Content-Security-Policy) - the request fails. When I disable saving the representation as part of the Admin Event, the request succeeds.
The error we see in our logs indicates it is unable to fit the representation (what appears to be the raw JSON sent by the front end to the backend) - inspecting the request payload to the server, I see it is 70988 characters when I copied from Chrome devtools.
Interestingly, across database vendors the size of the REPRESENTATION field can vary widely
It appears for some reason the front-end always includes the IDP metadata with its payload, even if you are changing unrelated configuration settings, which means even if you resolve the backend storage issue, the payload would always grow with each new IDP added. So I would think any change to ADMIN_EVENT_TABLE would just be deferring this other unbounded growth issue.
Long Stack Trace Warning
Version
18
Expected behavior
Realm is able to be saved with Admin Events having representation enabled.
Actual behavior
Realm is not able to be saved with Admin Events having representation enabled.
How to Reproduce?
Our Keycloak Setup:
Authenticate to the admin console once this set up is configured, and attempt to change a security configuration for the realm such as the
Content-Security-PolicyAnything else?
No response
All reactions