-
Notifications
You must be signed in to change notification settings - Fork 74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RHCLOUD-37430 | fix: integrations are not being returned sometimes #3256
base: master
Are you sure you want to change the base?
Conversation
We were getting reports of integrations not showing up in stage when Kessel Relations was enabled. More specifically, the issue was that after trying to filter integrations by name, some of them did simply not show up in the results even though they were recently created. After investigating the issue, it turned out that the problem was that we were getting an incomplete set of authorized integration IDs from Kessel, which in turn made us filter the database query results incorrectly, which again, in turn, made us return an inaccurate list of integrations for an organization. The QE organization which reported the issue had around 2,500 integrations created, and when a request was made to look up for a very specific integration with name "X", Notifications first had to ask Kessel for the principal's authorized integrations. Since Kessel only returned a batch of a 1,000 we would use just those to apply a "integration ID in" filter in the query, so if the specified name in the request was in that batch, the user was lucky and the integration was returned. These changes introduce a mechanism that keeps firing requests to Kessel whenever there is a continuation token available. That makes sure that we are fetching all the integrations that the user has access to, so that we don't miss any when querying our database. RHCLOUD-37430
ac2c8e6
to
d2128d2
Compare
inventory-api.authn.client.id=insights-notifications | ||
inventory-api.authn.client.id=svc-test | ||
inventory-api.authn.client.issuer=http://localhost:8084/realms/redhat-external | ||
inventory-api.authn.client.secret=development-value-123 | ||
inventory-api.authn.client.secret=h91qw8bPiDj9R6VSORsI5TYbceGU5PMH | ||
inventory-api.authn.mode=oidc-client-credentials | ||
inventory-api.is-secure-clients=false | ||
inventory-api.target-url=${clowder.endpoints.kessel-inventory-api:localhost:9081} | ||
|
||
relations-api.authn.client.id=insights-notifications | ||
relations-api.authn.client.id=svc-test | ||
relations-api.authn.client.issuer=http://localhost:8084/realms/redhat-external | ||
relations-api.authn.client.secret=development-value-123 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are the default values when launching the Kessel's Inventory API with the authentication enabled: https://github.com/project-kessel/inventory-api/blob/e0fc00b00af6af4977f8b4c66230a34f850da4a2/scripts/get-token.sh
In order to simplify things, I think it's just better to leave the credentials like this by default. It eases development and debugging, and since the credentials are public facing, I think it is just safe to have them here.
Even though we are not going to use them now, I already developed the code for the "post-filtering" tests on a different PR. We might consider this approach in the future, so it is worth having the code already developed. RHCLOUD-37430
/** | ||
* Filters the given list of integrations and leaves only the ones for | ||
* which the principal has authorization to view. Useful for | ||
* "post-filtering" the integrations once we have fetched them from the | ||
* database, and we want to remove the ones that the principal does not | ||
* have authorization for. | ||
* @param securityContext the security context from which the principal is | ||
* extracted. | ||
* @param endpoints the list of integrations to check. | ||
* @return a filtered list of integrations that the principal has | ||
* permission to view. The original list is kept untouched to avoid any | ||
* issues to avoid "immutable lists cannot be modified" issues. | ||
*/ | ||
public List<Endpoint> filterUnauthorizedIntegrations(final SecurityContext securityContext, final List<Endpoint> endpoints) { | ||
final List<Endpoint> resultingList = new ArrayList<>(); | ||
|
||
for (final Endpoint endpoint : endpoints) { | ||
try { | ||
this.hasPermissionOnResource(securityContext, IntegrationPermission.VIEW, ResourceType.INTEGRATION, endpoint.getId().toString()); | ||
resultingList.add(endpoint); | ||
} catch (final ForbiddenException ignored) { | ||
} | ||
} | ||
|
||
return resultingList; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though it is not used right now, I thought that we might want to change our approach in the future depending on the usage we make of the Kessel integration. That way we'll have access to both "pre-filtering" and "post-filtering" methods in case we want to get creative.
We are no longer going to use those log traces, so we can safely remove them from the code. RHCLOUD-37430
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
String continuationToken; | ||
String newContinuationToken = ""; | ||
do { | ||
// Replace the old continuation token with the new one. This way | ||
// the token gets used | ||
continuationToken = newContinuationToken; | ||
|
||
// Build the lookup request for Kessel. | ||
final LookupResourcesRequest request = this.buildLookupResourcesRequest(identity, integrationPermission, continuationToken); | ||
|
||
Log.tracef("[identity: %s][permission: %s][resource_type: %s] Payload for the resource lookup: %s", identity, integrationPermission, ResourceType.INTEGRATION, request); | ||
|
||
// Make the request to Kessel. | ||
final Iterator<LookupResourcesResponse> responses; | ||
try { | ||
responses = this.lookupClient.lookupResources(request); | ||
} catch (final Exception e) { | ||
Log.errorf( | ||
e, | ||
"[identity: %s][permission: %s][resource_type: %s] Runtime error when querying Kessel for integration resources with request payload: %s", | ||
identity, integrationPermission, ResourceType.INTEGRATION, request | ||
); | ||
|
||
// Increment the errors counter and stop the timer in case of | ||
// an error. | ||
this.meterRegistry.counter(KESSEL_METRICS_LOOKUP_RESOURCES_COUNTER_NAME, Tags.of(COUNTER_TAG_REQUEST_RESULT, COUNTER_TAG_FAILURES)).increment(); | ||
lookupTimer.stop(this.meterRegistry.timer(KESSEL_METRICS_LOOKUP_RESOURCES_TIMER_NAME, Tags.of(KESSEL_METRICS_TAG_PERMISSION_KEY, integrationPermission.getKesselPermissionName(), Constants.KESSEL_METRICS_TAG_RESOURCE_TYPE_KEY, ResourceType.INTEGRATION.name()))); | ||
|
||
throw e; | ||
} | ||
|
||
// Iterate over the incoming results. | ||
while (responses.hasNext()) { | ||
final LookupResourcesResponse response = responses.next(); | ||
|
||
Log.tracef("[identity: %s][permission: %s][resource_type: %s] Received payload for the resource lookup: %s", identity, integrationPermission, ResourceType.INTEGRATION, response); | ||
|
||
uuids.add(UUID.fromString(response.getResource().getId())); | ||
|
||
// Update the continuation token every time, to make sure we | ||
// grab the last streamed element's continuation token. | ||
newContinuationToken = response.getPagination().getContinuationToken(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String continuationToken; | |
String newContinuationToken = ""; | |
do { | |
// Replace the old continuation token with the new one. This way | |
// the token gets used | |
continuationToken = newContinuationToken; | |
// Build the lookup request for Kessel. | |
final LookupResourcesRequest request = this.buildLookupResourcesRequest(identity, integrationPermission, continuationToken); | |
Log.tracef("[identity: %s][permission: %s][resource_type: %s] Payload for the resource lookup: %s", identity, integrationPermission, ResourceType.INTEGRATION, request); | |
// Make the request to Kessel. | |
final Iterator<LookupResourcesResponse> responses; | |
try { | |
responses = this.lookupClient.lookupResources(request); | |
} catch (final Exception e) { | |
Log.errorf( | |
e, | |
"[identity: %s][permission: %s][resource_type: %s] Runtime error when querying Kessel for integration resources with request payload: %s", | |
identity, integrationPermission, ResourceType.INTEGRATION, request | |
); | |
// Increment the errors counter and stop the timer in case of | |
// an error. | |
this.meterRegistry.counter(KESSEL_METRICS_LOOKUP_RESOURCES_COUNTER_NAME, Tags.of(COUNTER_TAG_REQUEST_RESULT, COUNTER_TAG_FAILURES)).increment(); | |
lookupTimer.stop(this.meterRegistry.timer(KESSEL_METRICS_LOOKUP_RESOURCES_TIMER_NAME, Tags.of(KESSEL_METRICS_TAG_PERMISSION_KEY, integrationPermission.getKesselPermissionName(), Constants.KESSEL_METRICS_TAG_RESOURCE_TYPE_KEY, ResourceType.INTEGRATION.name()))); | |
throw e; | |
} | |
// Iterate over the incoming results. | |
while (responses.hasNext()) { | |
final LookupResourcesResponse response = responses.next(); | |
Log.tracef("[identity: %s][permission: %s][resource_type: %s] Received payload for the resource lookup: %s", identity, integrationPermission, ResourceType.INTEGRATION, response); | |
uuids.add(UUID.fromString(response.getResource().getId())); | |
// Update the continuation token every time, to make sure we | |
// grab the last streamed element's continuation token. | |
newContinuationToken = response.getPagination().getContinuationToken(); | |
} | |
String continuationToken; | |
do { | |
// Build the lookup request for Kessel. | |
final LookupResourcesRequest request = this.buildLookupResourcesRequest(identity, integrationPermission, continuationToken); | |
Log.tracef("[identity: %s][permission: %s][resource_type: %s] Payload for the resource lookup: %s", identity, integrationPermission, ResourceType.INTEGRATION, request); | |
// Make the request to Kessel. | |
final Iterator<LookupResourcesResponse> responses; | |
try { | |
responses = this.lookupClient.lookupResources(request); | |
} catch (final Exception e) { | |
Log.errorf( | |
e, | |
"[identity: %s][permission: %s][resource_type: %s] Runtime error when querying Kessel for integration resources with request payload: %s", | |
identity, integrationPermission, ResourceType.INTEGRATION, request | |
); | |
// Increment the errors counter and stop the timer in case of | |
// an error. | |
this.meterRegistry.counter(KESSEL_METRICS_LOOKUP_RESOURCES_COUNTER_NAME, Tags.of(COUNTER_TAG_REQUEST_RESULT, COUNTER_TAG_FAILURES)).increment(); | |
lookupTimer.stop(this.meterRegistry.timer(KESSEL_METRICS_LOOKUP_RESOURCES_TIMER_NAME, Tags.of(KESSEL_METRICS_TAG_PERMISSION_KEY, integrationPermission.getKesselPermissionName(), Constants.KESSEL_METRICS_TAG_RESOURCE_TYPE_KEY, ResourceType.INTEGRATION.name()))); | |
throw e; | |
} | |
// Iterate over the incoming results. | |
while (responses.hasNext()) { | |
final LookupResourcesResponse response = responses.next(); | |
Log.tracef("[identity: %s][permission: %s][resource_type: %s] Received payload for the resource lookup: %s", identity, integrationPermission, ResourceType.INTEGRATION, response); | |
uuids.add(UUID.fromString(response.getResource().getId())); | |
// Update the continuation token every time, to make sure we | |
// grab the last streamed element's continuation token. | |
continuationToken = response.getPagination().getContinuationToken(); | |
} |
Can't we be more direct by removing newContinuationToken
?
requestBuilder.setPagination( | ||
RequestPagination.newBuilder() | ||
.setContinuationToken(continuationToken) | ||
.setLimit(Integer.MAX_VALUE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we align this limit with the current Kessel's limitation of 1000?
BTW even if it should never happend, requesting 2147483647 results at a time could be little bit risky :)
We were getting reports of integrations not showing up in stage when Kessel Relations was enabled. More specifically, the issue was that after trying to filter integrations by name, some of them did simply not show up in the results even though they were recently created.
After investigating the issue, it turned out that the problem was that we were getting an incomplete set of authorized integration IDs from Kessel, which in turn made us filter the database query results incorrectly, which again, in turn, made us return an inaccurate list of integrations for an organization.
The QE organization which reported the issue had around 2,500 integrations created, and when a request was made to look up for a very specific integration with name "X", Notifications first had to ask Kessel for the principal's authorized integrations. Since Kessel only returned a batch of a 1,000 we would use just those to apply a "integration ID in" filter in the query, so if the specified name in the request was in that batch, the user was lucky and the integration was returned.
These changes introduce a mechanism that keeps firing requests to Kessel whenever there is a continuation token available. That makes sure that we are fetching all the integrations that the user has access to, so that we don't miss any when querying our database.
Jira ticket
[RHCLOUD-37430]