fix: Refine confclient's decision-making regarding version parameter

Refs: XRDDEV-1689
nordic-institute · Oct 28, 2023 · 985c001 · 985c001
1 parent 0e36a82
commit 985c001
Show file tree

Hide file tree

Showing 4 changed files with 49 additions and 27 deletions.
diff --git a/...uration-client/src/main/java/ee/ria/xroad/common/conf/globalconf/ConfigurationClient.java b/...uration-client/src/main/java/ee/ria/xroad/common/conf/globalconf/ConfigurationClient.java
@@ -32,7 +32,9 @@
 
 import java.io.FileNotFoundException;
 import java.nio.file.Files;
+import java.nio.file.Path;
 import java.nio.file.Paths;
+import java.time.OffsetDateTime;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
@@ -51,7 +53,7 @@ class ConfigurationClient {
 
     private ConfigurationSource configurationAnchor;
 
-    ConfigurationClient(String globalConfigurationDir, int version) {
+    ConfigurationClient(String globalConfigurationDir, Integer version) {
         this.globalConfigurationDir = globalConfigurationDir;
         downloader = new ConfigurationDownloader(globalConfigurationDir, version);
     }
@@ -72,7 +74,7 @@ synchronized void execute() throws Exception {
         }
 
         downloadConfigurationFromAnchor();
-        List<ConfigurationSource> configurationSources = getConfigurationSources();
+        List<ConfigurationSource> configurationSources = getAdditionalConfigurationSources();
 
         FederationConfigurationSourceFilter sourceFilter =
                 new FederationConfigurationSourceFilterImpl(configurationAnchor.getInstanceIdentifier());
@@ -82,7 +84,7 @@ synchronized void execute() throws Exception {
         downloadConfigurationFromAdditionalSources(configurationSources, sourceFilter);
     }
 
-    protected List<ConfigurationSource> getConfigurationSources() {
+    protected List<ConfigurationSource> getAdditionalConfigurationSources() {
         PrivateParameters privateParameters = loadPrivateParameters();
         return privateParameters != null ? privateParameters.getConfigurationAnchors() : List.of();
     }
@@ -108,6 +110,7 @@ private void initConfigurationAnchor() throws Exception {
         }
 
         saveInstanceIdentifier();
+
     }
 
     void saveInstanceIdentifier() throws Exception {
@@ -123,13 +126,13 @@ private void downloadConfigurationFromAnchor() throws Exception {
 
     private PrivateParameters loadPrivateParameters() {
         try {
-            VersionableConfigurationDirectory<? extends PrivateParametersProvider, ? extends SharedParametersProvider> dir;
+            Path privateParamsPath = Path.of(globalConfigurationDir, configurationAnchor.getInstanceIdentifier(),
+                    ConfigurationConstants.FILE_NAME_PRIVATE_PARAMETERS);
             if (downloader.getConfigurationVersion() > 2) {
-                dir = new ConfigurationDirectoryV3(globalConfigurationDir);
+                return new PrivateParametersV3(privateParamsPath, OffsetDateTime.MAX).getPrivateParameters();
             } else {
-                dir = new ConfigurationDirectoryV2(globalConfigurationDir);
+                return new PrivateParametersV2(privateParamsPath, OffsetDateTime.MAX).getPrivateParameters();
             }
-            return dir.getPrivate(configurationAnchor.getInstanceIdentifier());
         } catch (Exception e) {
             log.error("Failed to read additional configuration sources from" + globalConfigurationDir, e);
             return null;

diff --git a/...ion-client/src/main/java/ee/ria/xroad/common/conf/globalconf/ConfigurationClientMain.java b/...ion-client/src/main/java/ee/ria/xroad/common/conf/globalconf/ConfigurationClientMain.java
@@ -116,7 +116,7 @@ public static void main(String[] args) throws Exception {
             System.exit(download(actualArgs[0], actualArgs[1], Integer.parseInt(actualArgs[2])));
         } else if (actualArgs.length == NUM_ARGS_FROM_CONF_PROXY) {
             // Run configuration client in one-shot mode downloading the current global configuration version.
-            System.exit(download(actualArgs[0], actualArgs[1], CURRENT_GLOBAL_CONFIGURATION_VERSION));
+            System.exit(download(actualArgs[0], actualArgs[1], null));
         } else if (actualArgs.length == 1) {
             // Run configuration client in validate mode.
             System.exit(validate(actualArgs[0], getParamsValidator(cmd)));
@@ -138,7 +138,7 @@ private static CommandLine getCommandLine(String[] args) throws Exception {
         return parser.parse(options, args);
     }
 
-    private static int download(String configurationAnchorFile, String configurationPath, int version) {
+    private static int download(String configurationAnchorFile, String configurationPath, Integer version) {
         log.debug("Downloading configuration using anchor {} path = {})",
                 configurationAnchorFile, configurationPath);
 

diff --git a/...ion-client/src/main/java/ee/ria/xroad/common/conf/globalconf/ConfigurationDownloader.java b/...ion-client/src/main/java/ee/ria/xroad/common/conf/globalconf/ConfigurationDownloader.java
@@ -32,13 +32,15 @@
 import org.apache.commons.io.IOUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.http.HttpStatus;
+import org.apache.http.client.utils.URIBuilder;
 import org.bouncycastle.operator.DigestCalculator;
 
 import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
 import java.net.HttpURLConnection;
 import java.net.URI;
+import java.net.URISyntaxException;
 import java.net.URL;
 import java.net.URLConnection;
 import java.nio.file.Files;
@@ -56,7 +58,7 @@
 
 import static ee.ria.xroad.common.ErrorCodes.X_IO_ERROR;
 import static ee.ria.xroad.common.ErrorCodes.X_MALFORMED_GLOBALCONF;
-import static ee.ria.xroad.common.conf.globalconf.ConfigurationUtils.generateConfigurationLocation;
+import static ee.ria.xroad.common.SystemProperties.CURRENT_GLOBAL_CONFIGURATION_VERSION;
 import static ee.ria.xroad.common.util.CryptoUtils.createDigestCalculator;
 import static ee.ria.xroad.common.util.CryptoUtils.decodeBase64;
 import static ee.ria.xroad.common.util.CryptoUtils.encodeBase64;
@@ -78,14 +80,16 @@ class ConfigurationDownloader {
 
     public static final int READ_TIMEOUT = 30000;
 
+    private static final String VERSION_QUERY_PARAMETER = "version";
+
     protected final FileNameProvider fileNameProvider;
 
     private final Map<ConfigurationSource, ConfigurationLocation> lastSuccessfulLocation = new HashMap<>();
 
     @Getter
-    private int configurationVersion;
+    private final Integer configurationVersion;
 
-    ConfigurationDownloader(String globalConfigurationDir, int configurationVersion) {
+    ConfigurationDownloader(String globalConfigurationDir, Integer configurationVersion) {
         fileNameProvider = new FileNameProviderImpl(globalConfigurationDir);
         this.configurationVersion = configurationVersion;
     }
@@ -107,6 +111,7 @@ DownloadResult download(ConfigurationSource source, String... contentIdentifiers
 
         for (ConfigurationLocation location : getLocations(source)) {
             try {
+                location = toVersionedLocation(location);
                 Configuration config = download(location, contentIdentifiers);
                 rememberLastSuccessfulLocation(location);
                 return result.success(config);
@@ -147,7 +152,6 @@ private void preferLastSuccessLocation(ConfigurationSource source, List<Configur
     }
 
     Configuration download(ConfigurationLocation location, String[] contentIdentifiers) throws Exception {
-        location = buildVersionedLocation(location);
         log.info("Downloading configuration from {}", location.getDownloadURL());
 
         Configuration configuration = getParser().parse(location, contentIdentifiers);
@@ -269,19 +273,34 @@ boolean shouldDownload(ConfigurationFile configurationFile, Path file) throws Ex
         return true;
     }
 
-    ConfigurationLocation buildVersionedLocation(ConfigurationLocation location) throws IOException {
-        if (configurationVersion > 2) {
-            URL url = new URL(generateConfigurationLocation(location.getDownloadURL(), configurationVersion));
-            HttpURLConnection connection = (HttpURLConnection) url.openConnection();
-            if (connection.getResponseCode() != HttpStatus.SC_OK) {
-                log.info("Global conf V3 not available, defaulting back to V2");
-                configurationVersion = 2;
-            }
-            connection.disconnect();
+    ConfigurationLocation toVersionedLocation(ConfigurationLocation location) throws IOException, URISyntaxException {
+        URIBuilder uriBuilder = new URIBuilder(location.getDownloadURL());
+        if (configurationVersion == null) {
+            chooseVersion(uriBuilder);
+        } else {
+            // Force a.k.a. add "version" query parameter to the URI or replace if already exists
+            uriBuilder.setParameter(VERSION_QUERY_PARAMETER, configurationVersion.toString());
+        }
+        return new ConfigurationLocation(location.getSource(), uriBuilder.build().toString(), location.getVerificationCerts());
+
+    }
+
+    private void chooseVersion(URIBuilder uriBuilder) throws IOException {
+        // Respect preset version
+        if (uriBuilder.getQueryParams().stream().anyMatch(param -> VERSION_QUERY_PARAMETER.equals(param.getName()))) {
+            return;
+        }
+        log.info("Determining whether global conf V3 is available for {}", uriBuilder);
+        uriBuilder.addParameter(VERSION_QUERY_PARAMETER, String.valueOf(CURRENT_GLOBAL_CONFIGURATION_VERSION));
+        URL url = new URL(uriBuilder.toString());
+        HttpURLConnection connection = (HttpURLConnection) url.openConnection();
+        if (connection.getResponseCode() == HttpStatus.SC_NOT_FOUND) {
+            log.info("Global conf V3 query resulted in HTTP {}, defaulting back to V2.", HttpStatus.SC_NOT_FOUND);
+            uriBuilder.setParameter(VERSION_QUERY_PARAMETER, String.valueOf(CURRENT_GLOBAL_CONFIGURATION_VERSION - 1));
+        } else {
+            log.info("Using Global conf V3");
         }
-        return new ConfigurationLocation(location.getSource(),
-                ConfigurationUtils.generateConfigurationLocation(location.getDownloadURL(), configurationVersion),
-                location.getVerificationCerts());
+        connection.disconnect();
     }
 
     byte[] downloadContent(ConfigurationLocation location, ConfigurationFile file) throws Exception {

diff --git a/...client/src/test/java/ee/ria/xroad/common/conf/globalconf/ConfigurationDownloaderTest.java b/...client/src/test/java/ee/ria/xroad/common/conf/globalconf/ConfigurationDownloaderTest.java
@@ -85,7 +85,7 @@ public void rememberLastSuccessfulDownloadLocation() {
         for (int i = 0; i < MAX_ATTEMPTS; i++) {
             // Given
             ConfigurationDownloader downloader =
-                    getDownloader(LOCATION_URL_SUCCESS + "?version=2");
+                    getDownloader(LOCATION_URL_SUCCESS + "?version=3");
             List<String> locationUrls = getMixedLocationUrls();
 
             // When
@@ -128,7 +128,7 @@ private Matcher<List<String>> hasOnlyOneSuccessfulUrl() {
             @Override
             protected boolean matchesSafely(List<String> parsedUrls) {
                 return parsedUrls.size() == 1
-                        && parsedUrls.contains(LOCATION_URL_SUCCESS + "?version=2");
+                        && parsedUrls.contains(LOCATION_URL_SUCCESS + "?version=3");
             }
 
             @Override