Skip to content

Commit

Permalink
Quota must be checked with conditions over incoming status, not curre… (
Browse files Browse the repository at this point in the history 
eclipse-hawkbit#1847)

* Quota must be checked with conditions over incoming status, not current persisted in db

* Fix Download_Only case where DOWNLOADED is threated as 'final'.
Fix ci build tests.

* Review findings
  • Loading branch information
@vasilchev
vasilchev authored Sep 24, 2024
1 parent 1edc957 commit 3d93547
Show file tree
Hide file tree
Showing 3 changed files with 116 additions and 89 deletions.
21 changes: 12 additions & 9 deletions ...-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/management/JpaActionManagement.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import java.util.stream.Collectors;

import static org.eclipse.hawkbit.repository.model.Action.ActionType.DOWNLOAD_ONLY;
import static org.eclipse.hawkbit.repository.model.Action.Status.ERROR;
import static org.eclipse.hawkbit.repository.model.Action.Status.FINISHED;

/**
Expand Down Expand Up @@ -111,13 +112,12 @@ protected Action addActionStatus(final JpaActionStatusCreate statusCreate) {
*/
private boolean isUpdatingActionStatusAllowed(final JpaAction action, final JpaActionStatus actionStatus) {

final boolean isIntermediateFeedback = (FINISHED != actionStatus.getStatus())
&& (Action.Status.ERROR != actionStatus.getStatus());
final boolean intermediateStatus = isIntermediateStatus(actionStatus);

final boolean isAllowedByRepositoryConfiguration = !repositoryProperties.isRejectActionStatusForClosedAction()
&& isIntermediateFeedback;
final boolean isAllowedByRepositoryConfiguration = intermediateStatus && !repositoryProperties.isRejectActionStatusForClosedAction();

final boolean isAllowedForDownloadOnlyActions = isDownloadOnly(action) && !isIntermediateFeedback;
//in case of download_only action Status#DOWNLOADED is treated as 'final' already, so we accept one final status from device in case it sends
final boolean isAllowedForDownloadOnlyActions = isDownloadOnly(action) && action.getStatus() == Action.Status.DOWNLOADED && !intermediateStatus;

return action.isActive() || isAllowedByRepositoryConfiguration || isAllowedForDownloadOnlyActions;
}
Expand All @@ -141,7 +141,7 @@ protected static boolean isDownloadOnly(final JpaAction action) {
*/
private Action handleAddUpdateActionStatus(final JpaActionStatus actionStatus, final JpaAction action) {
// information status entry - check for a potential DOS attack
assertActionStatusQuota(action);
assertActionStatusQuota(actionStatus, action);
assertActionStatusMessageQuota(actionStatus);
actionStatus.setAction(action);

Expand All @@ -157,9 +157,8 @@ protected void onActionStatusUpdate(final Action.Status updatedActionStatus, fin
// can be overwritten to intercept the persistence of the action status
}

protected void assertActionStatusQuota(final JpaAction action) {
final boolean intermediateStatus = FINISHED != action.getStatus() && Action.Status.ERROR != action.getStatus();
if (intermediateStatus) {// check for quota only for intermediate statuses
protected void assertActionStatusQuota(final JpaActionStatus newActionStatus, final JpaAction action) {
if (isIntermediateStatus(newActionStatus)) {// check for quota only for intermediate statuses
QuotaHelper.assertAssignmentQuota(action.getId(), 1, quotaManagement.getMaxStatusEntriesPerAction(),
ActionStatus.class, Action.class, actionStatusRepository::countByActionId);
}
Expand All @@ -169,4 +168,8 @@ protected void assertActionStatusMessageQuota(final JpaActionStatus actionStatus
QuotaHelper.assertAssignmentQuota(actionStatus.getId(), actionStatus.getMessages().size(),
quotaManagement.getMaxMessagesPerActionStatus(), "Message", ActionStatus.class.getSimpleName(), null);
}

private static boolean isIntermediateStatus(final JpaActionStatus actionStatus) {
return FINISHED != actionStatus.getStatus() && ERROR != actionStatus.getStatus();
}
}
53 changes: 26 additions & 27 deletions .../src/main/java/org/eclipse/hawkbit/repository/jpa/management/JpaControllerManagement.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,37 +9,12 @@
*/
package org.eclipse.hawkbit.repository.jpa.management;

import static org.eclipse.hawkbit.repository.model.Action.Status.DOWNLOADED;
import static org.eclipse.hawkbit.repository.model.Action.Status.FINISHED;
import static org.eclipse.hawkbit.repository.model.Target.CONTROLLER_ATTRIBUTE_KEY_SIZE;
import static org.eclipse.hawkbit.repository.model.Target.CONTROLLER_ATTRIBUTE_VALUE_SIZE;

import java.net.URI;
import java.time.Duration;
import java.time.ZonedDateTime;
import java.time.temporal.ChronoUnit;
import java.time.temporal.TemporalUnit;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.BlockingDeque;
import java.util.concurrent.LinkedBlockingDeque;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;

import jakarta.persistence.EntityManager;
import jakarta.persistence.Query;
import jakarta.persistence.criteria.CriteriaBuilder;
import jakarta.persistence.criteria.CriteriaQuery;
import jakarta.persistence.criteria.Root;
import jakarta.validation.constraints.NotEmpty;

import lombok.extern.slf4j.Slf4j;
import org.apache.commons.collections4.ListUtils;
import org.eclipse.hawkbit.repository.ConfirmationManagement;
Expand Down Expand Up @@ -115,6 +90,30 @@
import org.springframework.util.StringUtils;
import org.springframework.validation.annotation.Validated;

import java.net.URI;
import java.time.Duration;
import java.time.ZonedDateTime;
import java.time.temporal.ChronoUnit;
import java.time.temporal.TemporalUnit;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.BlockingDeque;
import java.util.concurrent.LinkedBlockingDeque;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;

import static org.eclipse.hawkbit.repository.model.Action.Status.DOWNLOADED;
import static org.eclipse.hawkbit.repository.model.Action.Status.FINISHED;
import static org.eclipse.hawkbit.repository.model.Target.CONTROLLER_ATTRIBUTE_KEY_SIZE;
import static org.eclipse.hawkbit.repository.model.Target.CONTROLLER_ATTRIBUTE_VALUE_SIZE;

/**
* JPA based {@link ControllerManagement} implementation.
*/
Expand Down Expand Up @@ -613,7 +612,7 @@ public Action addCancelActionStatus(final ActionStatusCreate c) {
break;
default:
// information status entry - check for a potential DOS attack
assertActionStatusQuota(action);
assertActionStatusQuota(actionStatus, action);
assertActionStatusMessageQuota(actionStatus);
break;
}
Expand Down Expand Up @@ -891,7 +890,7 @@ public ActionStatus addInformationalActionStatus(final ActionStatusCreate c) {
final JpaActionStatus statusMessage = create.build();
statusMessage.setAction(action);

assertActionStatusQuota(action);
assertActionStatusQuota(statusMessage, action);
assertActionStatusMessageQuota(statusMessage);

return actionStatusRepository.save(statusMessage);
Expand Down
131 changes: 78 additions & 53 deletions ...src/test/java/org/eclipse/hawkbit/repository/jpa/management/ControllerManagementTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,33 +9,11 @@
*/
package org.eclipse.hawkbit.repository.jpa.management;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.eclipse.hawkbit.im.authentication.SpPermission.SpringEvalExpressions.CONTROLLER_ROLE;
import static org.eclipse.hawkbit.im.authentication.SpPermission.SpringEvalExpressions.CONTROLLER_ROLE_ANONYMOUS;
import static org.eclipse.hawkbit.im.authentication.SpPermission.SpringEvalExpressions.IS_CONTROLLER;
import static org.eclipse.hawkbit.repository.jpa.configuration.Constants.TX_RT_MAX;
import static org.eclipse.hawkbit.repository.model.Action.ActionType.DOWNLOAD_ONLY;
import static org.eclipse.hawkbit.repository.test.util.TestdataFactory.DEFAULT_CONTROLLER_ID;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import java.io.ByteArrayInputStream;
import java.net.URISyntaxException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.stream.Collectors;
import java.util.stream.IntStream;

import io.qameta.allure.Description;
import io.qameta.allure.Feature;
import io.qameta.allure.Step;
import io.qameta.allure.Story;
import jakarta.validation.ConstraintViolationException;

import org.apache.commons.lang3.RandomStringUtils;
import org.apache.commons.lang3.RandomUtils;
import org.assertj.core.api.Assertions;
Expand All @@ -55,8 +33,8 @@
import org.eclipse.hawkbit.repository.event.remote.entity.SoftwareModuleCreatedEvent;
import org.eclipse.hawkbit.repository.event.remote.entity.SoftwareModuleUpdatedEvent;
import org.eclipse.hawkbit.repository.event.remote.entity.TargetCreatedEvent;
import org.eclipse.hawkbit.repository.event.remote.entity.TargetUpdatedEvent;
import org.eclipse.hawkbit.repository.event.remote.entity.TargetTypeCreatedEvent;
import org.eclipse.hawkbit.repository.event.remote.entity.TargetUpdatedEvent;
import org.eclipse.hawkbit.repository.exception.AssignmentQuotaExceededException;
import org.eclipse.hawkbit.repository.exception.CancelActionNotAllowedException;
import org.eclipse.hawkbit.repository.exception.EntityAlreadyExistsException;
Expand All @@ -80,19 +58,38 @@
import org.eclipse.hawkbit.repository.model.TargetUpdateStatus;
import org.eclipse.hawkbit.repository.test.matcher.Expect;
import org.eclipse.hawkbit.repository.test.matcher.ExpectEvents;
import org.eclipse.hawkbit.repository.test.util.TargetTestData;
import org.eclipse.hawkbit.repository.test.util.SecurityContextSwitch;
import org.eclipse.hawkbit.repository.test.util.TargetTestData;
import org.eclipse.hawkbit.repository.test.util.WithUser;

import org.junit.jupiter.api.Test;
import org.mockito.Mockito;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.dao.ConcurrencyFailureException;

import io.qameta.allure.Description;
import io.qameta.allure.Feature;
import io.qameta.allure.Step;
import io.qameta.allure.Story;
import java.io.ByteArrayInputStream;
import java.net.URISyntaxException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.stream.Collectors;
import java.util.stream.IntStream;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.assertj.core.api.Assertions.assertThatNoException;
import static org.eclipse.hawkbit.im.authentication.SpPermission.SpringEvalExpressions.CONTROLLER_ROLE;
import static org.eclipse.hawkbit.im.authentication.SpPermission.SpringEvalExpressions.CONTROLLER_ROLE_ANONYMOUS;
import static org.eclipse.hawkbit.repository.jpa.configuration.Constants.TX_RT_MAX;
import static org.eclipse.hawkbit.repository.model.Action.ActionType.DOWNLOAD_ONLY;
import static org.eclipse.hawkbit.repository.test.util.TestdataFactory.DEFAULT_CONTROLLER_ID;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

@Feature("Component Tests - Repository")
@Story("Controller Management")
Expand Down Expand Up @@ -1455,30 +1452,42 @@ void quotaExceptionWhencontrollerReportsTooManyDownloadedMessagesForDownloadOnly
@Expect(type = SoftwareModuleCreatedEvent.class, count = 3),
@Expect(type = DistributionSetUpdatedEvent.class, count = 1), // implicit lock
@Expect(type = SoftwareModuleUpdatedEvent.class, count = 3), // implicit lock
@Expect(type = ActionCreatedEvent.class, count = 1), @Expect(type = TargetUpdatedEvent.class, count = 2),
@Expect(type = TargetAttributesRequestedEvent.class, count = 9),
@Expect(type = ActionUpdatedEvent.class, count = 1),
@Expect(type = ActionCreatedEvent.class, count = 1), @Expect(type = TargetUpdatedEvent.class, count = 3),
@Expect(type = TargetAttributesRequestedEvent.class, count = 10),
@Expect(type = ActionUpdatedEvent.class, count = 2),
@Expect(type = TargetAssignDistributionSetEvent.class, count = 1) })
void quotaExceededExceptionWhenControllerReportsTooManyUpdateActionStatusMessagesForDownloadOnlyAction() {
final int maxMessages = quotaManagement.getMaxMessagesPerActionStatus();
testdataFactory.createTarget();
final Long actionId = createAndAssignDsAsDownloadOnly("downloadOnlyDs", DEFAULT_CONTROLLER_ID);
assertThat(actionId).isNotNull();

//assert that too many intermediate statuses will throw quota exception
assertThatExceptionOfType(AssignmentQuotaExceededException.class)
.as("No QuotaExceededException thrown for too many DOWNLOADED updateActionStatus updates").isThrownBy(
() -> IntStream.range(0, maxMessages).forEach(i -> controllerManagement.addUpdateActionStatus(
entityFactory.actionStatus().create(actionId).status(Status.DOWNLOADED))));

assertThatExceptionOfType(AssignmentQuotaExceededException.class)
.as("No QuotaExceededException thrown for too many ERROR updateActionStatus updates")
.isThrownBy(() -> IntStream.range(0, maxMessages).forEach(i -> controllerManagement
.addUpdateActionStatus(entityFactory.actionStatus().create(actionId).status(Status.ERROR))));

assertThatExceptionOfType(AssignmentQuotaExceededException.class)
.as("No QuotaExceededException thrown for too many FINISHED updateActionStatus updates")
.isThrownBy(() -> IntStream.range(0, maxMessages).forEach(i -> controllerManagement
.addUpdateActionStatus(entityFactory.actionStatus().create(actionId).status(Status.FINISHED))));
//assert that Final result is accepted even if quota is reached
assertThatNoException().isThrownBy(() -> {
Action updatedAction = controllerManagement.addUpdateActionStatus(entityFactory.actionStatus().create(actionId).status(Status.FINISHED));
// check if action really finished
assertThat(updatedAction.isActive()).isFalse();
// check if final status is updated accordingly
assertThat(updatedAction.getStatus()).isEqualTo(Status.FINISHED);
});


//assert that additional final result is not accepted
assertThatNoException().isThrownBy(() -> {
Action updatedAction = controllerManagement
.addUpdateActionStatus(entityFactory.actionStatus().create(actionId).status(Status.ERROR));
// check if action really finished
assertThat(updatedAction.isActive()).isFalse();
// check if final status is not changed - e.g. ERROR is not updated because action has already finished
assertThat(updatedAction.getStatus()).isEqualTo(Status.FINISHED);
});
}

@Test
Expand All @@ -1488,27 +1497,43 @@ void quotaExceededExceptionWhenControllerReportsTooManyUpdateActionStatusMessage
@Expect(type = SoftwareModuleCreatedEvent.class, count = 3),
@Expect(type = DistributionSetUpdatedEvent.class, count = 1), // implicit lock
@Expect(type = SoftwareModuleUpdatedEvent.class, count = 3), // implicit lock
@Expect(type = ActionCreatedEvent.class, count = 1), @Expect(type = TargetUpdatedEvent.class, count = 1),
@Expect(type = ActionCreatedEvent.class, count = 1),
@Expect(type = TargetAttributesRequestedEvent.class, count = 1),
@Expect(type = ActionUpdatedEvent.class, count = 1),
@Expect(type = TargetUpdatedEvent.class, count = 2),
@Expect(type = TargetAssignDistributionSetEvent.class, count = 1) })
void quotaExceededExceptionWhenControllerReportsTooManyUpdateActionStatusMessagesForForced() {
final int maxMessages = quotaManagement.getMaxMessagesPerActionStatus();
final Long actionId = createTargetAndAssignDs();
assertThat(actionId).isNotNull();

//assert that too many intermediate statuses will throw quota exception
assertThatExceptionOfType(AssignmentQuotaExceededException.class)
.as("No QuotaExceededException thrown for too many DOWNLOADED updateActionStatus updates").isThrownBy(
() -> IntStream.range(0, maxMessages).forEach(i -> controllerManagement.addUpdateActionStatus(
entityFactory.actionStatus().create(actionId).status(Status.DOWNLOADED))));

assertThatExceptionOfType(AssignmentQuotaExceededException.class)
.as("No QuotaExceededException thrown for too many ERROR updateActionStatus updates")
.isThrownBy(() -> IntStream.range(0, maxMessages).forEach(i -> controllerManagement
.addUpdateActionStatus(entityFactory.actionStatus().create(actionId).status(Status.ERROR))));

assertThatExceptionOfType(AssignmentQuotaExceededException.class)
.as("No QuotaExceededException thrown for too many FINISHED updateActionStatus updates")
.isThrownBy(() -> IntStream.range(0, maxMessages).forEach(i -> controllerManagement
.addUpdateActionStatus(entityFactory.actionStatus().create(actionId).status(Status.FINISHED))));
//assert that Final result is accepted even if quota is reached
assertThatNoException().isThrownBy(() -> {
Action updatedAction = controllerManagement
.addUpdateActionStatus(entityFactory.actionStatus().create(actionId).status(Status.FINISHED));
// check if action really finished
assertThat(updatedAction.isActive()).isFalse();
// check if final status is updated accordingly
assertThat(updatedAction.getStatus()).isEqualTo(Status.FINISHED);
});


//assert that additional final result is not accepted
assertThatNoException().isThrownBy(() -> {
Action updatedAction = controllerManagement
.addUpdateActionStatus(entityFactory.actionStatus().create(actionId).status(Status.ERROR));
// check if action really finished
assertThat(updatedAction.isActive()).isFalse();
// check if final status is not changed - e.g. ERROR is not updated because action has already finished
assertThat(updatedAction.getStatus()).isEqualTo(Status.FINISHED);
});
}

@Test
Expand Down

0 comments on commit 3d93547

Please sign in to comment.