Skip to content

Commit

Permalink
feature: improve resilience when deleting the secrets from Sources (#…
Browse files Browse the repository at this point in the history
…3211)

Before this patch, if reporting the integration deletion to the Kessel
Inventory failed, the secrets were deleted from Sources but the
integration deletion would be rolled back in the database, which left it
in an invalid state unable to fetch the associated secrets from Sources.

With this patch:

- If deleting the integration from the database goes wrong, the
  transaction gets rolled back and nothing gets changed.
- If deleting the integration from Kessel goes wrong, the transaction is
  rolled back and the above point applies.
- If deleting the integration's secrets' from Sources goes wrong, the
  first point still applies and we make sure that the integration gets
  recreated in Inventory.

This should give us enough resiliency to avoid invalid states with
integrations in Notifications and the services we depend upon.

RHCLOUD-35158
  • Loading branch information
MikelAlejoBR authored Dec 20, 2024
1 parent 5febd2c commit bf71508
Show file tree
Hide file tree
Showing 2 changed files with 105 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -728,7 +728,6 @@ private Response internalDeleteEndpoint(final SecurityContext securityContext, f

// Clean up the secrets in Sources.
final Endpoint endpoint = endpointRepository.getEndpoint(orgId, id);
this.secretUtils.deleteSecretsForEndpoint(endpoint);

endpointRepository.deleteEndpoint(orgId, id);

Expand All @@ -741,6 +740,26 @@ private Response internalDeleteEndpoint(final SecurityContext securityContext, f
this.kesselAssets.deleteIntegration(securityContext, workspaceId.toString(), id.toString());
}

// Attempt deleting the secrets for the given endpoint. In the case
// that the secrets deletion goes wrong:
//
// - The transaction will be rolled back and the integration will not
// be deleted.
// - The secrets will not have been deleted from Sources.
// - We need to recreate the integration in Kessel Inventory, so that
// everything stays in sync.
try {
this.secretUtils.deleteSecretsForEndpoint(endpoint);
} catch (final Exception e) {
if (this.backendConfig.isKesselInventoryEnabled(orgId)) {
final UUID workspaceId = this.workspaceUtils.getDefaultWorkspaceId(orgId);

this.kesselAssets.createIntegration(securityContext, workspaceId.toString(), id.toString());
}

throw e;
}

return Response.noContent().build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
import org.eclipse.microprofile.config.inject.ConfigProperty;
import org.eclipse.microprofile.rest.client.inject.RestClient;
import org.hamcrest.Matchers;
import org.jboss.resteasy.reactive.ClientWebApplicationException;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
Expand All @@ -86,6 +87,7 @@
import org.project_kessel.relations.client.LookupClient;

import java.net.URI;
import java.time.LocalDateTime;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
Expand Down Expand Up @@ -4628,6 +4630,89 @@ void testInventoryDeleteIntegrationFailFailIntegrationNotRemovedFromDatabase(fin
this.assertIntegrationExists(identityHeader, integration);
}

/**
* Tests that when we attempt to remove an integration from our database,
* if the Sources integration throws an exception when deleting the
* secrets, the whole transaction is rolled back, the integration does not
* get deleted from our database, and that the integration gets recreated
* in Kessel's Inventory.
*/
@ParameterizedTest
@ValueSource(booleans = {false, true})
void testInventoryDeleteIntegrationSourcesFail(final boolean isKesselRelationsApiEnabled) {
// Enable the Inventory API.
Mockito.when(this.backendConfig.isKesselInventoryEnabled(anyString())).thenReturn(true);

// Conditionally enable the Relations API to test this in both
// scenarios.
Mockito.when(this.backendConfig.isKesselRelationsEnabled(anyString())).thenReturn(isKesselRelationsApiEnabled);

// Mock the reporter instance id.
Mockito.when(this.backendConfig.getKesselInventoryReporterInstanceId()).thenReturn("service-account-notifications");

// Create the integration we are going to attempt to delete.
final WebhookProperties webhookProperties = new WebhookProperties();
webhookProperties.setDisableSslVerification(false);
webhookProperties.setMethod(POST);
webhookProperties.setSecretToken("my-super-secret-token");
webhookProperties.setSecretTokenSourcesId(new Random().nextLong());
webhookProperties.setUrl(getMockServerUrl());

final Endpoint integration = this.resourceHelpers.createEndpoint(
DEFAULT_ACCOUNT_ID,
DEFAULT_ORG_ID,
WEBHOOK,
null,
"integration-name",
"integration-description",
webhookProperties,
false,
LocalDateTime.now()
);

// Return a default workspace for the organization.
this.kesselTestHelper.mockDefaultWorkspaceId(DEFAULT_ORG_ID);

// Simulate that Sources throws an exception when attempting to delete
// the secrets.
Mockito.doThrow(new ClientWebApplicationException()).when(this.sourcesServiceMock).delete(Mockito.anyString(), Mockito.anyString(), Mockito.anyLong());

// Create the identity header to be used in the request.
final String identityHeaderValue = TestHelpers.encodeRHIdentityInfo(DEFAULT_ACCOUNT_ID, DEFAULT_ORG_ID, DEFAULT_USER);
final Header identityHeader = TestHelpers.createRHIdentityHeader(identityHeaderValue);
MockServerConfig.addMockRbacAccess(identityHeaderValue, FULL_ACCESS);

// Mock the Kessel permission to be able to delete the integration.
this.kesselTestHelper.mockKesselPermission(DEFAULT_USER, IntegrationPermission.DELETE, ResourceType.INTEGRATION, integration.getId().toString());

// Attempt deleting the integration.
given()
.header(identityHeader)
.pathParam("id", integration.getId())
.when()
.delete("/endpoints/{id}")
.then()
.statusCode(HttpStatus.SC_INTERNAL_SERVER_ERROR);

// The integration assertion triggers a call to Sources to fetch the
// secret token, so we need to make sure a secret is returned so that
// the code doesn't break.
final Secret secret = new Secret();
secret.password = webhookProperties.getSecretToken();

Mockito
.when(sourcesServiceMock.getById(Mockito.anyString(), Mockito.anyString(), Mockito.eq(webhookProperties.getSecretTokenSourcesId())))
.thenReturn(secret);

// Assert that the transaction was rolled back and that the integration
// was not deleted.
this.assertIntegrationExists(identityHeader, integration);

// Since the Sources' secrets' deletion failed, we should have
// recreated the integration in Kessel's Inventory.
Mockito.verify(this.notificationsIntegrationClient, Mockito.times(1)).CreateNotificationsIntegration(Mockito.any(CreateNotificationsIntegrationRequest.class));
}

/**
* Asserts that no integrations are present in the database by issuing a
* query directly and by calling the "/endpoints" REST endpoint.
Expand Down

0 comments on commit bf71508

Please sign in to comment.