Skip to content

Target Group Quota introduction #3337

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

Open
strailov wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Target Group Quota introduction #3337

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fb0e903
Target Group Quota introduction
strailov Sep 3, 2026
378170c
address review comments
strailov Sep 3, 2026
120e1d0
remove redundant null check guards
strailov Sep 3, 2026
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
4 changes: 4 additions & 0 deletions hawkbit-core/src/main/java/org/eclipse/hawkbit/security/HawkbitSecurityProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,10 @@ public static class Dos {
* Maximum number of allowed groups per Rollout.
*/
private int maxRolloutGroupsPerRollout = 500;
/**
* Maximum number of allowed distinct target groups.
*/
private int maxTargetGroups = 100;
/**
* Maximum number of messages per ActionStatus
*/
Expand Down
5 changes: 5 additions & 0 deletions .../hawkbit-repository-api/src/main/java/org/eclipse/hawkbit/repository/QuotaManagement.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,11 @@ public interface QuotaManagement {
*/
int getMaxTargetsPerRolloutGroup();

/**
* @return the maximum number of distinct target groups
*/
int getMaxTargetGroups();

/**
* @return the maximum number of target distribution set assignments resulting from a manual assignment
*/
Expand Down
5 changes: 5 additions & 0 deletions ...pository-core/src/main/java/org/eclipse/hawkbit/repository/PropertiesQuotaManagement.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@ public int getMaxTargetsPerRolloutGroup() {
return securityProperties.getDos().getMaxTargetsPerRolloutGroup();
}

@Override
public int getMaxTargetGroups() {
return securityProperties.getDos().getMaxTargetGroups();
}

@Override
public int getMaxTargetDistributionSetAssignmentsPerManualAssignment() {
return securityProperties.getDos().getMaxTargetDistributionSetAssignmentsPerManualAssignment();
Expand Down
88 changes: 88 additions & 0 deletions ...-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/management/JpaTargetManagement.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.SortedSet;
import java.util.TreeSet;
import java.util.function.BiFunction;
import java.util.function.Consumer;
import java.util.function.Function;
Expand All @@ -34,6 +36,7 @@
import jakarta.validation.constraints.NotEmpty;

import lombok.extern.slf4j.Slf4j;
import org.eclipse.hawkbit.context.AccessContext;
import org.eclipse.hawkbit.ql.jpa.QLSupport;
import org.eclipse.hawkbit.repository.QuotaManagement;
import org.eclipse.hawkbit.repository.TargetManagement;
Expand Down Expand Up @@ -102,6 +105,50 @@ protected JpaTargetManagement(
this.targetTagRepository = targetTagRepository;
}

@Override
@Transactional
@Retryable(includes = ConcurrencyFailureException.class, maxRetriesString = Constants.RETRY_MAX, delayString = Constants.RETRY_DELAY)
public JpaTarget create(final TargetManagement.Create create) {
assertTargetGroupQuota(Collections.singletonList(create.getGroup()));

@avgustinmm avgustinmm Sep 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why here we don't check for group == null, but on some places we do check>?
same on update

@strailov strailov Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We actually do on later point - on wanted.isEmpty() in the assertTargetGroupQuota. It is possible to directly check in the create method, but I have decided the code would become more complex to read idk ...

@strailov strailov Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So I guess the two guards in assign methods are redundant here ... Will remove them in order to comply with all the others.

return super.create(create);
}

@Override
@Transactional
@Retryable(includes = ConcurrencyFailureException.class, maxRetriesString = Constants.RETRY_MAX, delayString = Constants.RETRY_DELAY)
public List<JpaTarget> create(final Collection<TargetManagement.Create> create) {
assertTargetGroupQuota(create.stream().map(TargetManagement.Create::getGroup).toList());
return super.create(create);
}

@Override
@Transactional
@Retryable(includes = ConcurrencyFailureException.class, maxRetriesString = Constants.RETRY_MAX, delayString = Constants.RETRY_DELAY)
public JpaTarget update(final TargetManagement.Update update) {
try {
assertTargetGroupQuota(Collections.singletonList(update.getGroup()));
} catch (final Exception ex) {
// target existence check in order to throw EntityNotFound instead of AssignmentQuotaException if both applicable
getValid(update.getId());
throw ex;
}
return super.update(update);
}

@Override
@Transactional
@Retryable(includes = ConcurrencyFailureException.class, maxRetriesString = Constants.RETRY_MAX, delayString = Constants.RETRY_DELAY)
public Map<Long, JpaTarget> update(final Collection<TargetManagement.Update> update) {
try {
assertTargetGroupQuota(update.stream().map(TargetManagement.Update::getGroup).toList());
} catch (final Exception ex) {
// target existence check in order to throw EntityNotFound instead of AssignmentQuotaException if both applicable
get(update.stream().map(TargetManagement.Update::getId).toList());
throw ex;
}
return super.update(update);
}

@Override
public Map<String, String> getControllerAttributes(final String controllerId) {
return getMap(controllerId, JpaTarget_.controllerAttributes);
Expand Down Expand Up @@ -339,6 +386,10 @@ public Target unassignType(final String controllerId) {
@Transactional
@Retryable(includes = ConcurrencyFailureException.class, maxRetriesString = Constants.RETRY_MAX, delayString = Constants.RETRY_DELAY)
public void assignTargetGroupWithRsql(String group, String rsql) {
// Quota check
if (group != null) {
assertTargetGroupQuota(Collections.singletonList(group));
}

// Switch back to UpdateAllQuery if switching back to hibernate. (EclipseLink does not work well with UpdateAllQuery)
// EclipseLink: using subquery approach — applying predicate directly to the UPDATE root
Expand Down Expand Up @@ -401,6 +452,11 @@ private void assignTargetGroupOnChunks(final String group, final String rsql) {
@Transactional
@Retryable(includes = ConcurrencyFailureException.class, maxRetriesString = Constants.RETRY_MAX, delayString = Constants.RETRY_DELAY)
public void assignTargetsWithGroup(String group, List<String> controllerIds) {
// Quota check
if (group != null) {
assertTargetGroupQuota(Collections.singletonList(group));
}

final CriteriaBuilder cb = entityManager.getCriteriaBuilder();
CriteriaUpdate<JpaTarget> criteriaQuery = cb.createCriteriaUpdate(JpaTarget.class);
Root<JpaTarget> root = criteriaQuery.from(JpaTarget.class);
Expand Down Expand Up @@ -552,4 +608,36 @@ private void throwEntityNotFoundExceptionIfTagDoesNotExist(final Long tagId) {
throw new EntityNotFoundException(TargetTag.class, tagId);
}
}

private void assertTargetGroupQuota(final Collection<String> requested) {
final long limit = quotaManagement.getMaxTargetGroups();

@avgustinmm avgustinmm Sep 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if wanted is empty - no need to getMaxTargetGroups, which at some point could become db managed

@strailov strailov Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fair point, will switch the order

if (limit <= 0) {
return;
}

final SortedSet<String> wanted = requested.stream()
.filter(Objects::nonNull)
.collect(Collectors.toCollection(() -> new TreeSet<>(String.CASE_INSENSITIVE_ORDER)));

@avgustinmm avgustinmm Sep 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

db could be case sensitive?

@strailov strailov Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, agreed - I'll let the db collation do the job without specifying in java for it


if (wanted.isEmpty()) {
return; // no group(s), skip findDistinctGroups db call
}

// one group, already present -> allowed
if (wanted.size() == 1 && jpaRepository.existsByGroup(wanted.first())) {
return;
}
final List<String> existing = jpaRepository.findDistinctGroups(AccessContext.tenant());

@avgustinmm avgustinmm Sep 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it could be the case that updating a group other is removed, e.g.
update group x -> y, if device is the only in x - then x is removed y is created - no need to check.
I wonder if it will be better if we do a "relaxed" check - i.e. if there are already 100 groups - we don't allow not existing, though - this also could be wrong if we "remove" groups

@strailov strailov Sep 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Didn't quite get the second one - don't we actually currently do that ? We don't allow not existing groups (currently) when the limit is reached .
Fair point for the first case with x and y, however not quite sure currently how could we handle it painlessly.
Most of those cases some devices are left with group x which could lead to hitting the limit, however if exactly those devices that has x goes to y this is like more renaming and we won't work in that case, yes. Not sure if we should mitigate this - workaround is present, just unassign X and assign Y.

final Set<String> existingCi = new TreeSet<>(String.CASE_INSENSITIVE_ORDER);
existingCi.addAll(existing);

wanted.removeAll(existingCi);
if (wanted.isEmpty()) {
return; // no growth -> allowed
}

QuotaHelper.assertAssignmentQuota(
AccessContext.tenant(), wanted.size(), limit, "target group", "tenant",
tenant -> existing.size());
}
}
8 changes: 8 additions & 0 deletions ...ory-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/repository/TargetRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,4 +110,12 @@ void setAssignedAndInstalledDistributionSetAndUpdateStatus(
*/
@Query(value = "SELECT DISTINCT target_group FROM sp_target WHERE tenant = ?1 AND target_group IS NOT NULL", nativeQuery = true)
List<String> findDistinctGroups(@Param("tenant") String tenant);

/**
* Checks if a target group is present
*
* @param group to check existence
* @return whether a target group is present or not
*/
boolean existsByGroup(String group);
}
Loading
Loading