Skip to content

Commit

Permalink
Fix DELETED reaction check & shadows cleanup task
Browse files Browse the repository at this point in the history 
1. DELETED reaction check now treats ObjectNotFoundExceptions
gracefully. This fixes some failing model-impl tests.

2. !!! BEHAVIOR CHANGE !!! Shadows cleanup activity now checks the real
existence of a resource object before sending a "shadow deleted" event.
This may break the functionality for connectors not providing the READ
capability. (But the original use case is now obsolete, anyway.) See
also MID-8350.
  • Loading branch information
@mederly
mederly committed Nov 25, 2022
1 parent 646f8e5 commit 9402fd3
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 36 deletions.
31 changes: 18 additions & 13 deletions ...n/java/com/evolveum/midpoint/model/impl/sync/reactions/SynchronizationActionExecutor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ private void executeReaction(@NotNull SynchronizationReactionDefinition reaction
*/
private boolean isDeleteReactionApplicable(OperationResult result)
throws SchemaException, ExpressionEvaluationException, CommunicationException, SecurityViolationException,
ConfigurationException, ObjectNotFoundException {
ConfigurationException {
F owner = syncCtx.getLinkedOwner();
if (owner == null) {
LOGGER.trace("Cannot consider the 'realness' of the DELETE situation because there's no linked owner");
Expand All @@ -211,19 +211,24 @@ private boolean isDeleteReactionApplicable(OperationResult result)
var options = GetOperationOptionsBuilder.create()
.noFetch()
.futurePointInTime()
.allowNotFound()
.build();
ShadowType shadow =
provisioningService
.getObject(ShadowType.class, linkRef.getOid(), options, syncCtx.getTask(), result)
.asObjectable();
ResourceObjectTypeIdentification type = syncCtx.getTypeIdentification();
if (ShadowUtil.getResourceOidRequired(shadow).equals(syncCtx.getResourceOid())
&& shadow.getKind() == type.getKind()
&& Objects.equals(shadow.getIntent(), type.getIntent())
&& !ShadowUtil.isDead(shadow)) {
LOGGER.debug("Found non-dead compatible shadow, the DELETE reaction will not be executed for {} of {}",
syncCtx.getShadowedResourceObject(), owner);
return false;
try {
ShadowType shadow =
provisioningService
.getObject(ShadowType.class, linkRef.getOid(), options, syncCtx.getTask(), result)
.asObjectable();
ResourceObjectTypeIdentification type = syncCtx.getTypeIdentification();
if (ShadowUtil.getResourceOidRequired(shadow).equals(syncCtx.getResourceOid())
&& shadow.getKind() == type.getKind()
&& Objects.equals(shadow.getIntent(), type.getIntent())
&& !ShadowUtil.isDead(shadow)) {
LOGGER.debug("Found non-dead compatible shadow, the DELETE reaction will not be executed for {} of {}",
syncCtx.getShadowedResourceObject(), owner);
return false;
}
} catch (ObjectNotFoundException e) {
LOGGER.trace("Shadow not existing (or disappeared during provisioning get operation): {}", linkRef.getOid());
}
}
LOGGER.trace("No non-dead compatible shadow found, the DELETE reaction will be executed");
Expand Down
48 changes: 31 additions & 17 deletions ...pl/src/main/java/com/evolveum/midpoint/model/impl/tasks/ShadowCleanupActivityHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/
package com.evolveum.midpoint.model.impl.tasks;

import static com.evolveum.midpoint.prism.xml.XmlTypeConverter.createXMLGregorianCalendar;
import static com.evolveum.midpoint.util.MiscUtil.argCheck;
import static com.evolveum.midpoint.xml.ns._public.common.common_3.ResourceObjectSetQueryApplicationModeType.APPEND;

Expand All @@ -21,11 +22,8 @@
import com.evolveum.midpoint.model.impl.sync.tasks.ProcessingScope;
import com.evolveum.midpoint.model.impl.tasks.simple.SimpleActivityHandler;
import com.evolveum.midpoint.prism.PrismContext;
import com.evolveum.midpoint.prism.PrismObject;
import com.evolveum.midpoint.prism.query.ObjectFilter;
import com.evolveum.midpoint.prism.query.ObjectQuery;
import com.evolveum.midpoint.prism.xml.XmlTypeConverter;
import com.evolveum.midpoint.provisioning.api.ResourceObjectShadowChangeDescription;
import com.evolveum.midpoint.repo.common.activity.definition.AbstractWorkDefinition;
import com.evolveum.midpoint.repo.common.activity.definition.ResourceObjectSetSpecificationProvider;
import com.evolveum.midpoint.repo.common.activity.definition.WorkDefinitionFactory.WorkDefinitionSupplier;
Expand All @@ -35,6 +33,7 @@
import com.evolveum.midpoint.repo.common.activity.run.SearchBasedActivityRun;
import com.evolveum.midpoint.repo.common.activity.run.processing.ItemProcessingRequest;
import com.evolveum.midpoint.schema.GetOperationOptions;
import com.evolveum.midpoint.schema.GetOperationOptionsBuilder;
import com.evolveum.midpoint.schema.SelectorOptions;
import com.evolveum.midpoint.schema.constants.SchemaConstants;
import com.evolveum.midpoint.schema.result.OperationResult;
Expand All @@ -44,14 +43,26 @@
import com.evolveum.midpoint.schema.util.task.work.WorkDefinitionSource;
import com.evolveum.midpoint.schema.util.task.work.WorkDefinitionWrapper.TypedWorkDefinitionWrapper;
import com.evolveum.midpoint.task.api.RunningTask;
import com.evolveum.midpoint.task.api.Task;
import com.evolveum.midpoint.util.DebugUtil;
import com.evolveum.midpoint.util.exception.CommonException;
import com.evolveum.midpoint.util.exception.ObjectNotFoundException;
import com.evolveum.midpoint.util.logging.Trace;
import com.evolveum.midpoint.util.logging.TraceManager;
import com.evolveum.midpoint.xml.ns._public.common.common_3.*;

/**
* The original idea behind this activity was to treat shadows on (asynchronous) Kafka resources that did not support
* "read" operation (or did that in a very limited way). So the only way how to know what shadows are really existing
* was to send regular account update events that would keep "fullSynchronizationTimestamp" up to date. Shadows that
* were not updated were considered to be dead.
*
* However, such an approach is a bit brittle. In particular, if used for a regular resource, it may be possible that
* such a shadow really exists. Hence, in 4.7 the behavior was changed to call explicit provisioning "getObject"
* operation instead of simply assuming the shadow is gone. This conflicts with the original use case, and if that
* should be usable again, the code would need to be improved somehow.
*
* TODO Decide on the fate of this activity (MID-8350)
*
* @author skublik
*/
@Component
Expand Down Expand Up @@ -105,7 +116,7 @@ public String getIdentifierPrefix() {
return "shadow-cleanup";
}

public static final class MyRun extends
public final class MyRun extends
SearchBasedActivityRun<ShadowType, MyWorkDefinition, ShadowCleanupActivityHandler, AbstractActivityWorkStateType> {

private ProcessingScope processingScope;
Expand Down Expand Up @@ -149,7 +160,7 @@ public ObjectQuery customizeQuery(ObjectQuery configuredQuery, OperationResult r
PrismContext prismContext = getActivityHandler().prismContext;

ObjectFilter syncTimestampFilter = prismContext.queryFor(ShadowType.class)
.item(ShadowType.F_FULL_SYNCHRONIZATION_TIMESTAMP).le(XmlTypeConverter.createXMLGregorianCalendar(deletingDate))
.item(ShadowType.F_FULL_SYNCHRONIZATION_TIMESTAMP).le(createXMLGregorianCalendar(deletingDate))
.or().item(ShadowType.F_FULL_SYNCHRONIZATION_TIMESTAMP).isNull()
.buildFilter();

Expand All @@ -167,19 +178,22 @@ public Collection<SelectorOptions<GetOperationOptions>> customizeSearchOptions(

@Override
public boolean processItem(@NotNull ShadowType shadow,
@NotNull ItemProcessingRequest<ShadowType> request, RunningTask workerTask, OperationResult result) {
deleteShadow(shadow.asPrismObject(), workerTask, result);
@NotNull ItemProcessingRequest<ShadowType> request, RunningTask workerTask, OperationResult result)
throws CommonException {
var options = GetOperationOptionsBuilder.create()
.forceRefresh()
.forceRetry()
.allowNotFound()
.build();
try {
// TODO what if the resource does not support "read" capability?
provisioningService.getObject(ShadowType.class, shadow.getOid(), options, workerTask, result);
// The "shadow dead" or even "shadow deleted" event should be emitted by the provisioning service
} catch (ObjectNotFoundException e) {
LOGGER.trace("Shadow is no longer there - OK, that makes sense: {}", shadow);
}
return true;
}

private void deleteShadow(PrismObject<ShadowType> shadow, Task workerTask, OperationResult result) {
ResourceObjectShadowChangeDescription change = new ResourceObjectShadowChangeDescription();
change.setObjectDelta(shadow.createDeleteDelta());
change.setResource(processingScope.getResource().asPrismObject());
change.setShadowedResourceObject(shadow);
change.setSourceChannel(SchemaConstants.CHANNEL_CLEANUP_URI);
getActivityHandler().synchronizationService.notifyChange(change, workerTask, result);
}
}

public static class MyWorkDefinition extends AbstractWorkDefinition implements ResourceObjectSetSpecificationProvider {
Expand Down
19 changes: 14 additions & 5 deletions ...est/src/test/java/com/evolveum/midpoint/model/intest/sync/TestSyncStoryUsingLiveSync.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,18 +96,22 @@ public void test999GreenShadowsCleanup() throws Exception {
getGreenResource().addAccount(accountJack);
runSyncTasks(GREEN, GREEN, GREEN); // To stabilize the situation, and eat all secondary changes related to jack's account

when("sleeping 10 seconds to make jack's account rot");
and("sleeping 10 seconds to make jack's account rot");
TimeUnit.SECONDS.sleep(10);

when("carol account is added and sync is run");
and("carol account is added and sync is run");
DummyAccount accountCarol = new DummyAccount(ACCOUNT_CAROL_DUMMY_USERNAME);
accountCarol.setEnabled(true);
accountCarol.addAttributeValues(DummyResourceContoller.DUMMY_ACCOUNT_ATTRIBUTE_FULLNAME_NAME, "Carol Seepgood");
accountCarol.addAttributeValues(DummyResourceContoller.DUMMY_ACCOUNT_ATTRIBUTE_LOCATION_NAME, "Melee Island");
getGreenResource().addAccount(accountCarol);
runSyncTasks(GREEN);

when("cleanup task is run");
and("jack's account is deleted from the resource");
// This is to check that shadows for existing accounts will not be removed by the cleanup task
getGreenResource().deleteAccountByName(accountJack.getName());

and("cleanup task is run");
TASK_SHADOW_CLEANUP_GREEN.rerun(result);
TASK_SHADOW_CLEANUP_GREEN.assertAfter();

Expand All @@ -116,10 +120,15 @@ public void test999GreenShadowsCleanup() throws Exception {
display("User carol", userCarol);
assertNotNull("No carol user", userCarol);

and("jack should be gone, as the shadow is too old");
and("jack should be gone, as the shadow is too old and no longer on the resource");
PrismObject<UserType> userJack = findUserByUsername(ACCOUNT_JACK_DUMMY_USERNAME);
display("User jack", userJack);
assertNull("User jack is not null", userJack);
assertNull("User jack is still there!", userJack);

and("jojo should be still there, as the Xjojo account was not deleted on the resource");
PrismObject<UserType> userJojo = findUserByUsername("jojo");
display("User jojo", userJojo);
assertNotNull("No jojo user", userJojo);

displayAllNotifications();
notificationManager.setDisabled(true);
Expand Down
3 changes: 2 additions & 1 deletion ...mpl/src/main/java/com/evolveum/midpoint/provisioning/impl/shadows/ShadowGetOperation.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,8 @@ private void doQuickShadowRefresh(OperationResult result) throws ObjectNotFoundE
throw new ObjectNotFoundException(
"Resource object not found (after quick refresh)",
ShadowType.class,
oid);
oid,
ctx.isAllowNotFound());
}
updateShadowState();
}
Expand Down

0 comments on commit 9402fd3

Please sign in to comment.