Skip to content
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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+323 −76
Open

RHCLOUD-37430 | fix: integrations are not being returned sometimes #3256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d2128d2
fix: integrations are not being returned sometimes
MikelAlejoBR Jan 28, 2025
9f3f2c3
feature: implement post-filtering methods for Kessel
MikelAlejoBR Jan 28, 2025
7b43dfe
refactor: remove debugging log traces
MikelAlejoBR Jan 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
149 changes: 112 additions & 37 deletions backend/src/main/java/com/redhat/cloud/notifications/auth/kessel/KesselAuthorization.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import com.redhat.cloud.notifications.auth.kessel.permission.WorkspacePermission;
import com.redhat.cloud.notifications.auth.principal.rhid.RhIdentity;
import com.redhat.cloud.notifications.config.BackendConfig;
import com.redhat.cloud.notifications.models.Endpoint;
import com.redhat.cloud.notifications.routers.SecurityContextUtil;
import io.micrometer.core.instrument.MeterRegistry;
import io.micrometer.core.instrument.Tags;
Expand All @@ -22,12 +23,15 @@
import org.project_kessel.api.relations.v1beta1.LookupResourcesResponse;
import org.project_kessel.api.relations.v1beta1.ObjectReference;
import org.project_kessel.api.relations.v1beta1.ObjectType;
import org.project_kessel.api.relations.v1beta1.RequestPagination;
import org.project_kessel.api.relations.v1beta1.SubjectReference;
import org.project_kessel.relations.client.CheckClient;
import org.project_kessel.relations.client.LookupClient;

import java.util.ArrayList;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Set;
import java.util.UUID;

Expand Down Expand Up @@ -82,6 +86,33 @@ public class KesselAuthorization {
@Inject
BackendConfig backendConfig;

/**
* 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;
}

Comment on lines +89 to +115
Copy link
Member Author

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.

/**
* Checks if the subject on the security context has permission on the
* given resource. Throws
Expand Down Expand Up @@ -137,7 +168,8 @@ public void hasPermissionOnResource(final SecurityContext securityContext, final

/**
* Looks up the integrations the security context's subject has the given
* permission for.
* permission for. Useful for when we want to "pre-filter" the integrations
* the principal has authorization for.
* @param securityContext the security context holding the subject's
* identity.
* @param integrationPermission the integration's permission we want to use
Expand All @@ -148,43 +180,66 @@ public Set<UUID> lookupAuthorizedIntegrations(final SecurityContext securityCont
// Identify the subject.
final RhIdentity identity = SecurityContextUtil.extractRhIdentity(securityContext);

// Build the lookup request for Kessel.
final LookupResourcesRequest request = this.buildLookupResourcesRequest(identity, integrationPermission);

Log.tracef("[identity: %s][permission: %s][resource_type: %s] Payload for the resource lookup: %s", identity, integrationPermission, ResourceType.INTEGRATION, request);

// Measure the time it takes to perform the lookup operation.
final Timer.Sample lookupTimer = Timer.start(this.meterRegistry);

// Call 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
);
meterRegistry.counter(KESSEL_METRICS_LOOKUP_RESOURCES_COUNTER_NAME, Tags.of(COUNTER_TAG_REQUEST_RESULT, COUNTER_TAG_FAILURES)).increment();

throw e;
} finally {
// Stop the timer.
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())));
}

meterRegistry.counter(KESSEL_METRICS_LOOKUP_RESOURCES_COUNTER_NAME, Tags.of(COUNTER_TAG_REQUEST_RESULT, COUNTER_TAG_SUCCESSES)).increment();

// Process the incoming responses.
// Prepare the set of UUIDs we are going to receive from Kessel.
final Set<UUID> uuids = new HashSet<>();
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()));
}
// Every response coming from Kessel has a continuation token. In order
// to know when to stop querying for resources, we need to keep track
// of the old continuation token and the one from the latest received
// element. Once the old one and the new one are the same, we don't
// have to keep querying for more resources.
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();
}
Comment on lines +194 to +236
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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?


meterRegistry.counter(KESSEL_METRICS_LOOKUP_RESOURCES_COUNTER_NAME, Tags.of(COUNTER_TAG_REQUEST_RESULT, COUNTER_TAG_SUCCESSES)).increment();
} while (!continuationToken.equals(newContinuationToken));

// Stop the timer.
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())));

return uuids;
}
Expand Down Expand Up @@ -258,11 +313,14 @@ protected CheckRequest buildCheckRequest(final RhIdentity identity, final Kessel
* @param identity the subject's identity.
* @param kesselPermission the permission we want to check against the
* subject's integrations.
* @param continuationToken the token that will resume fetching resources
* from Kessel from the last point we left off.
* @return a built lookup request that aims at finding integrations for the
* given subject.
*/
protected LookupResourcesRequest buildLookupResourcesRequest(final RhIdentity identity, final KesselPermission kesselPermission) {
return LookupResourcesRequest.newBuilder()
protected LookupResourcesRequest buildLookupResourcesRequest(final RhIdentity identity, final KesselPermission kesselPermission, final String continuationToken) {
// Build the regular query.
final LookupResourcesRequest.Builder requestBuilder = LookupResourcesRequest.newBuilder()
.setSubject(
SubjectReference.newBuilder()
.setSubject(
Expand All @@ -271,10 +329,27 @@ protected LookupResourcesRequest buildLookupResourcesRequest(final RhIdentity id
.setId(getUserId(identity))
).build()
).setRelation(kesselPermission.getKesselPermissionName())
.setResourceType(ResourceType.INTEGRATION.getKesselObjectType())
.build();
.setResourceType(ResourceType.INTEGRATION.getKesselObjectType());

// Include the continuation token in the request to resume fetching
// resources right where we last left off.
if (continuationToken != null && !continuationToken.isBlank()) {
requestBuilder.setPagination(
RequestPagination.newBuilder()
.setContinuationToken(continuationToken)
.setLimit(Integer.MAX_VALUE)
Copy link
Contributor

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 :)

.build()
);
}

return requestBuilder.build();
}

/**
* Gets the user identifier from the {@link RhIdentity} object.
* @param identity the object to extract the identifier from.
* @return the user ID in the format that Kessel expects.
*/
private String getUserId(RhIdentity identity) {
return backendConfig.getKesselDomain() + "/" + identity.getUserId();
}
Expand Down
14 changes: 0 additions & 14 deletions backend/src/main/java/com/redhat/cloud/notifications/routers/EndpointResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,12 +318,7 @@ public EndpointPage getEndpoints(
@QueryParam("active") Boolean activeOnly,
@QueryParam("name") String name
) {
Log.tracef("[org_id: %s] Called V2 \"list endpoints\" endpoint", getOrgId(sec));
Log.debugf("[org_id: %s][query: %s][target_type: %s][active_only: %s][name: %s] Received request parameters", getOrgId(sec), query, targetType, activeOnly, name);

if (this.backendConfig.isKesselRelationsEnabled(getOrgId(sec))) {
Log.tracef("[org_id: %s] Kessel relations enabled. Looking up authorized integrations", getOrgId(sec));

// Fetch the set of integration IDs the user is authorized to view.
final Set<UUID> authorizedIds = this.kesselAuthorization.lookupAuthorizedIntegrations(sec, IntegrationPermission.VIEW);
if (authorizedIds.isEmpty()) {
Expand All @@ -334,8 +329,6 @@ public EndpointPage getEndpoints(

return internalGetEndpoints(sec, query, targetType, activeOnly, name, authorizedIds, true);
} else {
Log.tracef("[org_id :%s] Legacy RBAC enabled", getOrgId(sec));

return getEndpointsLegacyRBACRoles(sec, query, targetType, activeOnly, name, true);
}
}
Expand Down Expand Up @@ -365,12 +358,7 @@ public EndpointPage getEndpoints(
@QueryParam("active") Boolean activeOnly,
@QueryParam("name") String name
) {
Log.tracef("[org_id: %s] Called V1 \"list endpoints\" endpoint", getOrgId(sec));
Log.debugf("[org_id: %s][query: %s][target_type: %s][active_only: %s][name: %s] Received request parameters", getOrgId(sec), query, targetType, activeOnly, name);

if (this.backendConfig.isKesselRelationsEnabled(getOrgId(sec))) {
Log.tracef("[org_id: %s] Kessel relations enabled. Looking up authorized integrations", getOrgId(sec));

// Fetch the set of integration IDs the user is authorized to view.
final Set<UUID> authorizedIds = this.kesselAuthorization.lookupAuthorizedIntegrations(sec, IntegrationPermission.VIEW);
if (authorizedIds.isEmpty()) {
Expand All @@ -381,8 +369,6 @@ public EndpointPage getEndpoints(

return this.internalGetEndpoints(sec, query, targetType, activeOnly, name, authorizedIds, false);
} else {
Log.tracef("[org_id :%s] Legacy RBAC enabled", getOrgId(sec));

return this.getEndpointsLegacyRBACRoles(sec, query, targetType, activeOnly, name, false);
}
}
Expand Down
13 changes: 4 additions & 9 deletions backend/src/main/resources/application.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,21 +109,16 @@ quarkus.unleash.url=http://localhost:4242
quarkus.log.category."com.redhat.cloud.notifications.routers.internal.kessel.KesselAssetsMigrationService".min-level=TRACE
quarkus.log.category."com.redhat.cloud.notifications.auth.kessel".min-level=TRACE

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
Comment on lines -112 to -121
Copy link
Member Author

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.

relations-api.authn.client.secret=h91qw8bPiDj9R6VSORsI5TYbceGU5PMH
relations-api.authn.mode=oidc-client-credentials
relations-api.is-secure-clients=false
relations-api.target-url=${clowder.endpoints.kessel-relations-api:localhost:9000}

# Enabling "trace" logging level for debugging purposes. Safe to remove these
# lines after RHCLOUD-37430 has been closed.
quarkus.log.category."com.redhat.cloud.notifications.db.builder.Parameters".min-level=TRACE
quarkus.log.category."com.redhat.cloud.notifications.routers.EndpointResource".min-level=TRACE
Loading
Loading