RIPE NCC has merged fffd040fc

* chore(deps): update dependency org.postgresql:postgresql to v42.7.2 [79c1ed1b0]
* Fix after rebase [df830cb78]
* Simplify [6e853d9de]
* Code smells [1815eb9eb]
* Fixes [f4b703da4]
* Code smells [236ad94f0]
* Refactor duplicate ROA validations [76fb40114]
* chore(deps): update dependency gradle to v8.6 [2e2addc0c]

.gitlab-ci.yml

@@ -1,5 +1,5 @@
 default:
-  image: gradle:8.4-jdk11
+  image: gradle:8.6-jdk11
 
 # Explicit version of the Mergerequests-Pipelines workflow, with the main branch
 # added.

build.gradle

@@ -53,7 +53,7 @@ dependencies {
     implementation 'org.springdoc:springdoc-openapi-ui:1.7.0'
 
     runtimeOnly 'io.micrometer:micrometer-registry-prometheus'
-    implementation 'org.postgresql:postgresql:42.6.0'
+    implementation 'org.postgresql:postgresql:42.7.2'
     runtimeOnly 'org.springframework.boot:spring-boot-starter-tomcat'
 
     implementation 'com.google.code.gson:gson:2.10.1'

gradle/wrapper/gradle-wrapper.properties

@@ -1,6 +1,6 @@
 distributionBase=GRADLE_USER_HOME
 distributionPath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-8.4-bin.zip
+distributionUrl=https\://services.gradle.org/distributions/gradle-8.6-bin.zip
 networkTimeout=10000
 validateDistributionUrl=true
 zipStoreBase=GRADLE_USER_HOME

gradlew.bat

@@ -43,11 +43,11 @@ set JAVA_EXE=java.exe
 %JAVA_EXE% -version >NUL 2>&1
 if %ERRORLEVEL% equ 0 goto execute
 
-echo.
-echo ERROR: JAVA_HOME is not set and no 'java' command could be found in your PATH.
-echo.
-echo Please set the JAVA_HOME variable in your environment to match the
-echo location of your Java installation.
+echo. 1>&2
+echo ERROR: JAVA_HOME is not set and no 'java' command could be found in your PATH. 1>&2
+echo. 1>&2
+echo Please set the JAVA_HOME variable in your environment to match the 1>&2
+echo location of your Java installation. 1>&2
 
 goto fail
 
@@ -57,11 +57,11 @@ set JAVA_EXE=%JAVA_HOME%/bin/java.exe
 
 if exist "%JAVA_EXE%" goto execute
 
-echo.
-echo ERROR: JAVA_HOME is set to an invalid directory: %JAVA_HOME%
-echo.
-echo Please set the JAVA_HOME variable in your environment to match the
-echo location of your Java installation.
+echo. 1>&2
+echo ERROR: JAVA_HOME is set to an invalid directory: %JAVA_HOME% 1>&2
+echo. 1>&2
+echo Please set the JAVA_HOME variable in your environment to match the 1>&2
+echo location of your Java installation. 1>&2
 
 goto fail
 

src/main/java/net/ripe/rpki/rest/service/CaRoaConfigurationService.java

@@ -185,13 +185,20 @@ public ResponseEntity<?> stageRoaChanges(@PathVariable("caName") final CaName ca
 
         final Set<AllowedRoute> currentRoutes = new HashSet<>(currentRoaConfiguration.toAllowedRoutes());
         final Set<AllowedRoute> futureRoutes = new HashSet<>(futureRoas.size());
+
         IpResourceSet affectedRanges;
         try {
+            // This will fill up futureRoutes as well
             affectedRanges = buildAffectedRanges(futureRoas, futureRoutes, currentRoutes);
         } catch (IllegalArgumentException e) {
             return ResponseEntity.status(BAD_REQUEST).body(Map.of(ERROR, e.getMessage()));
         }
 
+        Optional<String> validationError = Roas.validateRoaUpdate(futureRoutes);
+        if (validationError.isPresent()) {
+            return ResponseEntity.status(BAD_REQUEST).body(Map.of(ERROR, validationError.get()));
+        }
+
         final List<BgpAnnouncementChange> bgpAnnouncementChanges = getBgpAnnouncementChanges(
                 ca, currentRoutes, futureRoutes, bgpAnnouncements, affectedRanges);
 
@@ -210,26 +217,9 @@ private IpResourceSet buildAffectedRanges(List<ROA> futureRoas, Set<AllowedRoute
             if (!currentRoutes.contains(route))
                 affectedRanges.add(roaIpRange);
         }
-
-        final Map<AnnouncedRoute, List<Integer>> futureRoaMap = Utils.makeROAMap(futureRoas);
-        Optional<String> e = Utils.validateUniqueROAs("Error in future ROAs", futureRoaMap);
-        if (e.isPresent()) {
-            throw new IllegalArgumentException(e.get());
-        }
-
         for (AllowedRoute route : currentRoutes) {
             if (!futureRoutes.contains(route))
                 affectedRanges.add(route.getPrefix());
-
-            final AnnouncedRoute key = new AnnouncedRoute(route.getAsn(), route.getPrefix());
-            if (futureRoaMap.containsKey(key)) {
-                futureRoaMap.get(key).forEach(futureMaxLength -> {
-                    if (!Objects.equals(futureMaxLength, route.getMaximumLength())) {
-                        // the same ASN+prefix and different max length
-                        throw new IllegalArgumentException(Utils.getSameROAErrorMessage(route, key, futureMaxLength));
-                    }
-                });
-            }
         }
         return affectedRanges;
     }
@@ -299,9 +289,10 @@ public ResponseEntity<?> publishROAs(@PathVariable("caName") final CaName caName
         final String ifMatch = StringUtils.defaultIfEmpty(ifMatchHeader, publishSet.getIfMatch());
 
         try {
-            Utils.validateNoIdenticalROAs(
-                            roaViewService.getRoaConfiguration(ca.getId()),
-                            publishSet.getAdded(), publishSet.getDeleted())
+            var currentRoas = new HashSet<>(roaViewService.getRoaConfiguration(ca.getId()).toAllowedRoutes());
+            var futureRoas = Roas.applyDiff(currentRoas, Roas.toDiff(publishSet));
+
+            Roas.validateRoaUpdate(futureRoas)
                     .ifPresent(rc -> {
                         throw new IllegalArgumentException(rc);
                     });

src/main/java/net/ripe/rpki/rest/service/Roas.java

@@ -0,0 +1,66 @@
+package net.ripe.rpki.rest.service;
+
+import com.google.common.collect.Streams;
+import lombok.AccessLevel;
+import lombok.NoArgsConstructor;
+import lombok.Value;
+import net.ripe.ipresource.Asn;
+import net.ripe.ipresource.IpRange;
+import net.ripe.rpki.commons.validation.roa.AllowedRoute;
+import net.ripe.rpki.commons.validation.roa.AnnouncedRoute;
+import net.ripe.rpki.rest.pojo.PublishSet;
+import net.ripe.rpki.rest.pojo.ROA;
+
+import java.util.*;
+import java.util.stream.Collectors;
+
+@NoArgsConstructor(access = AccessLevel.PRIVATE)
+public class Roas {
+
+    public static Optional<String> validateUniqueROAs(String prefix, Map<AnnouncedRoute, List<Integer>> newOnes) {
+        for (var e : newOnes.entrySet()) {
+            var maxLengths = e.getValue();
+            if (maxLengths.size() > 1) {
+                var sorted = maxLengths.stream().sorted().collect(Collectors.toList());
+                return Optional.of(String.format("%s: there are more than one pair (%s, %s), max lengths: %s",
+                        prefix, e.getKey().getOriginAsn(), e.getKey().getPrefix(), sorted));
+            }
+        }
+        return Optional.empty();
+    }
+
+    public static RoaDiff toDiff(PublishSet publishSet) {
+        return new RoaDiff(
+                publishSet.getAdded().stream().map(Roas::toAllowedRoute).collect(Collectors.toSet()),
+                publishSet.getDeleted().stream().map(Roas::toAllowedRoute).collect(Collectors.toSet())
+        );
+    }
+
+    private static AllowedRoute toAllowedRoute(ROA roa) {
+        var roaIpRange = IpRange.parse(roa.getPrefix());
+        var maxLength = roa.getMaxLength() != null ? roa.getMaxLength() : roaIpRange.getPrefixLength();
+        return new AllowedRoute(Asn.parse(roa.getAsn()), roaIpRange, maxLength);
+    }
+
+    public static Set<AllowedRoute> applyDiff(Set<AllowedRoute> currentRoas, RoaDiff diff) {
+        var futureRoas = new HashSet<>(currentRoas);
+        diff.getDeleted().forEach(futureRoas::remove);
+        futureRoas.addAll(diff.getAdded());
+        return futureRoas;
+    }
+
+    public static Optional<String> validateRoaUpdate(Set<AllowedRoute> futureRoutes) {
+        var futureMap = futureRoutes.stream().collect(Collectors.toMap(
+                r -> new AnnouncedRoute(r.getAsn(), r.getPrefix()),
+                r -> Collections.singletonList(r.getMaximumLength()),
+                (a, b) -> Streams.concat(a.stream(), b.stream()).collect(Collectors.toList())));
+
+        return validateUniqueROAs("Error in future ROAs", futureMap);
+    }
+
+    @Value
+    public static class RoaDiff {
+        Set<AllowedRoute> added;
+        Set<AllowedRoute> deleted;
+    }
+}

src/main/java/net/ripe/rpki/rest/service/Utils.java

@@ -20,8 +20,6 @@
 import net.ripe.rpki.rest.pojo.ROA;
 import net.ripe.rpki.server.api.dto.BgpRisEntry;
 import net.ripe.rpki.server.api.dto.RoaAlertConfigurationData;
-import net.ripe.rpki.server.api.dto.RoaConfigurationData;
-import net.ripe.rpki.server.api.dto.RoaConfigurationPrefixData;
 import net.ripe.rpki.server.api.services.read.RoaAlertConfigurationViewService;
 import org.springframework.http.ResponseEntity;
 
@@ -161,103 +159,4 @@ protected static ResponseEntity<Object> badRequestError(Exception e) {
     protected static ResponseEntity<Object> badRequestError(String errorMessage) {
         return ResponseEntity.badRequest().body(Map.of("error", errorMessage));
     }
-
-    static Optional<String> validateNoIdenticalROAs(RoaConfigurationData roaConfigurationData, List<ROA> newRoas, List<ROA> roasToDelete) {
-        final List<ExistingROA> existingROAs = roaConfigurationData.getPrefixes().stream()
-                .map(p -> new ExistingROA(p.getAsn(), p.getPrefix(), p.getNullableMaxLength()))
-                .collect(Collectors.toList());
-        return validateNoIdenticalROAs(existingROAs, newRoas, roasToDelete);
-    }
-
-    /**
-     * Validate that there are not existing ROAs having the same AS and Prefix
-     * but different Max Length fields.
-     *
-     * @return Optional error text for the first validation error that was found.
-     */
-    static Optional<String> validateNoIdenticalROAs(List<ExistingROA> existingROAs, List<ROA> newRoas, List<ROA> roasToDelete) {
-
-        final Map<AnnouncedRoute, List<Integer>> newOnes = makeROAMap(newRoas);
-        final Map<AnnouncedRoute, List<Integer>> deletedOnes = makeROAMap(roasToDelete);
-
-        Optional<String> e = validateUniqueROAs("Error in new ROAs", newOnes);
-        if (e.isPresent()) return e;
-
-        e = validateUniqueROAs("Error in deleted ROAs", deletedOnes);
-        if (e.isPresent()) return e;
-
-        final Map<AnnouncedRoute, Optional<Integer>> newOnesUnique = uniqueEntries(newOnes);
-        final Map<AnnouncedRoute, Optional<Integer>> deletedOnesUnique = uniqueEntries(deletedOnes);
-
-        for (var existingRoa : existingROAs) {
-            var key = new AnnouncedRoute(existingRoa.getAsn(), existingRoa.getPrefix());
-            if (deletedOnesUnique.containsKey(key)) {
-                var maxLengthToDelete = deletedOnesUnique.get(key).orElse(null);
-                if (Objects.equals(existingRoa.getMaximumLength(), maxLengthToDelete)) {
-                    newOnesUnique.remove(key);
-                }
-            }
-        }
-
-        for (var existingRoa : existingROAs) {
-            var key = new AnnouncedRoute(existingRoa.getAsn(), existingRoa.getPrefix());
-            if (newOnesUnique.containsKey(key)) {
-                final Integer newMaxLength = newOnesUnique.get(key).orElse(null);
-                if (!Objects.equals(existingRoa.getMaximumLength(), newMaxLength)) {
-                    // we are not going to delete existing one
-                    return Optional.of(getSameROAErrorMessage(existingRoa, key, newMaxLength));
-                }
-            }
-        }
-        return Optional.empty();
-    }
-
-    public static Map<AnnouncedRoute, Optional<Integer>> uniqueEntries(Map<AnnouncedRoute, List<Integer>> m) {
-        var newM = new HashMap<AnnouncedRoute, Optional<Integer>>(m.size());
-        m.forEach((k, list) -> newM.put(k, Optional.ofNullable(list.get(0))));
-        return newM;
-    }
-
-    public static Optional<String> validateUniqueROAs(String prefix, Map<AnnouncedRoute, List<Integer>> newOnes) {
-        for (var e : newOnes.entrySet()) {
-            if (e.getValue().size() > 1) {
-                return Optional.of(String.format("%s: there are more than one pair (%s, %s), max lengths: %s",
-                        prefix, e.getKey().getOriginAsn(), e.getKey().getPrefix(), e.getValue()));
-            }
-        }
-        return Optional.empty();
-    }
-
-    public static String getSameROAErrorMessage(Object existingRoa, AnnouncedRoute key, Integer newMaxLength) {
-        return String.format(
-                "There is an overlap in ROAs: existing %s has the same (ASN, prefix) as added %s",
-                existingRoa,
-                new ROA(key.getOriginAsn().toString(), key.getPrefix().toString(), newMaxLength));
-    }
-
-    public static Map<AnnouncedRoute, List<Integer>> makeROAMap(List<ROA> newRoas) {
-        return newRoas.stream().map(r -> {
-            AnnouncedRoute announcedRoute = new AnnouncedRoute(Asn.parse(r.getAsn()), IpRange.parse(r.getPrefix()));
-            return Pair.of(announcedRoute, Collections.singletonList(r.getMaxLength()));
-        }).collect(Collectors.toMap(Pair::getLeft, Pair::getRight,
-                (a, b) -> Streams.concat(a.stream(), b.stream())
-                        .collect(Collectors.toList())));
-    }
-
-    @Value
-    public static class ExistingROA {
-        Asn asn;
-        IpRange prefix;
-        Integer maximumLength;
-
-        @Override
-        public String toString() {
-            return "ROA{" +
-                    "asn=" + asn +
-                    ", prefix=" + prefix +
-                    ", maximumLength=" + maximumLength +
-                    '}';
-        }
-    }
-
 }

src/test/java/net/ripe/rpki/rest/service/CaRoaConfigurationServiceTest.java

@@ -30,11 +30,7 @@
 
 import javax.security.auth.x500.X500Principal;
 import javax.ws.rs.core.HttpHeaders;
-import java.util.Arrays;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.Map;
+import java.util.*;
 
 import static net.ripe.rpki.rest.service.AbstractCaRestService.API_URL_PREFIX;
 import static org.assertj.core.api.Assertions.assertThat;
@@ -115,7 +111,26 @@ public void shouldPostROAtoStageNoRealChange() throws Exception {
     }
 
     @Test
-    public void shouldPreventFromStaginBreakingChanges() throws Exception {
+    public void shouldNotRejectSameAsnPrefixAndDifferentMaxLengthWhenItsAReplacements() throws Exception {
+        when(roaViewService.getRoaConfiguration(CA_ID)).thenReturn(new RoaConfigurationData(List.of(
+                new RoaConfigurationPrefixData(new Asn(10), IpRange.parse("193.0.24.0/21"), 21))));
+
+        ImmutableResourceSet ipResourceSet = ImmutableResourceSet.parse("127.0.0.1, ::1");
+        when(certificateAuthorityData.getResources()).thenReturn(ipResourceSet);
+
+        Map<Boolean, Collection<BgpRisEntry>> bgpRisEntries = new HashMap<>();
+        bgpRisEntries.put(false, Collections.singletonList(new BgpRisEntry(new Asn(10), IpRange.parse("193.0.24.0/21"), 21)));
+        when(bgpRisEntryViewService.findMostSpecificContainedAndNotContained(ipResourceSet)).thenReturn(bgpRisEntries);
+
+        // try to replace one maxLength with the other
+        mockMvc.perform(Rest.post(API_URL_PREFIX + "/123/roas/stage")
+                        .content("[{\"asn\" : \"AS10\", \"prefix\" : \"193.0.24.0/21\", \"maximalLength\": 22 }]"))
+                .andExpect(status().isOk())
+                .andExpect(jsonPath("$.length()").value("1"));
+    }
+
+    @Test
+    public void shouldRejectSameAsnPrefixAndDifferentMaxLengthWhenItsExtraROA() throws Exception {
         when(roaViewService.getRoaConfiguration(CA_ID)).thenReturn(new RoaConfigurationData(Collections.singletonList(
                 new RoaConfigurationPrefixData(new Asn(10), IpRange.parse(TESTNET_1), 32))));
 
@@ -131,12 +146,13 @@ public void shouldPreventFromStaginBreakingChanges() throws Exception {
         when(bgpRisEntryViewService.findMostSpecificContainedAndNotContained(ipResourceSet)).thenReturn(bgpRisEntries);
 
         mockMvc.perform(Rest.post(API_URL_PREFIX + "/123/roas/stage")
-                .content("[{\"asn\" : \"AS10\", \"prefix\" : \"" + TESTNET_1 + "\", \"maximalLength\" : \"24\"}]"))
+                        .content("[" +
+                                "{\"asn\" : \"AS10\", \"prefix\" : \"" + TESTNET_1 + "\", \"maximalLength\" : \"24\"}," +
+                                "{\"asn\" : \"AS10\", \"prefix\" : \"" + TESTNET_1 + "\", \"maximalLength\" : \"25\"}]"
+                        ))
                 .andExpect(status().isBadRequest())
                 .andExpect(jsonPath("$.error")
-                        .value("There is an overlap in ROAs: existing " +
-                                "AllowedRoute[asn=AS10,maximumLength=32,prefix=192.0.2.0/24] has the same (ASN, prefix) as added " +
-                                "ROA{asn='AS10', prefix='192.0.2.0/24', maximalLength=24}"));
+                        .value("Error in future ROAs: there are more than one pair (AS10, 192.0.2.0/24), max lengths: [24, 25]"));
     }
 
     /**