Fix / Handling of unmodified secrets in config updates (#551)

* Fix handling of unmodified secrets in config updates

* Explicit error handling for inconsistent secret alias in the orchestrator

* Add Python license aliases for compliance

* In orchestrator, filter repositories for a job to just what the job requires

* Handle non-standard license alias for greenlet

* Restrict versions due to compliance issues around support of PEP 639

* Bring in repo resources used for import model jobs

* Update Python requirements for compliance tools

Author: martin-traverse

dev/compliance/license-config-python.sh

@@ -24,6 +24,9 @@ ALLOWED_LICENSES="${ALLOWED_LICENSES};Python Software Foundation License"
 ALLOWED_LICENSES="${ALLOWED_LICENSES};ISC License (ISCL)"
 ALLOWED_LICENSES="${ALLOWED_LICENSES};The Unlicense (Unlicense)"
 
+# License for greenlet is specified this way
+ALLOWED_LICENSES="${ALLOWED_LICENSES};MIT AND Python-2.0"
+
 # The "certifi" package is a dependency of Python Safety, licensed under MPL 2.0
 # It is OK to use since the compliance tools are not distributed
 # So, we can exclude the package from our license report

tracdap-runtime/python/requirements.txt

@@ -58,6 +58,7 @@ polars >= 1.0.0
 
 idna >= 3.7
 typing_extensions < 4.13
+urllib3 < 2.4.0
 
 
 # ----------------------------------------------------------------------------------------------------------------------
@@ -81,8 +82,10 @@ packaging >= 23.0
 
 # Compliance dependencies
 
-# Safety 3.3 introduces a GPL dependency, excluding for now
-safety >= 3.2, < 3.3
+# Safety 3.3.0 introduces a GPL dependency, fixed in 3.3.1
+# Safety uses marhmallow, which has breaking API changes in 4.0
+safety != 3.3.0
+marshmallow < 4.0
 
 # Series 3.x does not work with Python 3.11
 # Series 4.x does not work with Python 3.7

tracdap-runtime/python/setup.cfg

@@ -72,8 +72,9 @@ install_requires =
     pyyaml == 6.0.2
     dulwich == 0.22.7
     requests == 2.32.3
-    # Bad release, excluded until fixed
+    # Not usable yet due to compliance issues around PEP 639
     typing_extensions < 4.13
+    urllib3 < 2.4.0
 
 
 [options.extras_require]

tracdap-services/tracdap-svc-admin/src/main/java/org/finos/tracdap/svc/admin/services/ConfigService.java

@@ -22,6 +22,7 @@
 import org.finos.tracdap.api.internal.ConfigUpdateType;
 import org.finos.tracdap.api.internal.InternalMetadataApiGrpc;
 import org.finos.tracdap.common.config.ConfigKeys;
+import org.finos.tracdap.common.exception.EConfigParse;
 import org.finos.tracdap.common.metadata.MetadataUtil;
 import org.finos.tracdap.common.middleware.GrpcConcern;
 
@@ -236,7 +237,7 @@ private ConfigWriteRequest processSecrets(
 
         if (objectType == ObjectType.RESOURCE) {
 
-            var secureResource = processSecrets(
+            var secureResource = processResourceSecrets(
                     request.getDefinition().getResource(),
                     prior.getDefinition().getResource(),
                     secrets, secretsUpdated);
@@ -245,14 +246,16 @@ private ConfigWriteRequest processSecrets(
 
             return request.toBuilder().setDefinition(secureObject).build();
         }
+        else {
 
-        // Other object types are not processed for secrets
-        secretsUpdated.setResult(false);
+            // Other object types are not processed for secrets
+            secretsUpdated.setResult(false);
 
-        return request;
+            return request;
+        }
     }
 
-    private ResourceDefinition processSecrets(
+    private ResourceDefinition processResourceSecrets(
             ResourceDefinition newResource, ResourceDefinition oldResource,
             ISecretService secrets, SimpleResult<Boolean> secretsUpdated) {
 
@@ -265,18 +268,34 @@ private ResourceDefinition processSecrets(
 
             if (secretValue != null && !secretValue.isEmpty()) {
 
+                // Secret value is supplied - update the secret store
                 var secretAlias = secrets.storePassword(secretKey, secretValue);
                 secureResource.putSecrets(secretKey, secretAlias);
 
                 if (!secretsUpdated.isDone())
                     secretsUpdated.setResult(true);
             }
+            else if (oldResource.containsSecrets(secretKey)) {
+
+                // If secret value is blank and a previous version exists, carry the secret over
+                var secretAlias = oldResource.getSecretsOrThrow(secretKey);
+                secureResource.putSecrets(secretKey, secretAlias);
+            }
+            else {
+
+                // Do not allow setting blank secrets
+                var message = String.format("No value supplied for config secret [%s]", secretKey);
+                log.error(message);
+                throw new EConfigParse(message);
+            }
         }
 
         for (var secretKey : oldResource.getSecretsMap().keySet()) {
             if (!newResource.containsSecrets(secretKey)) {
 
-                secrets.deleteSecret(secretKey);
+                // Delete any secrets that have been removed since the prior version
+                var secretAlias = oldResource.getSecretsOrThrow(secretKey);
+                secrets.deleteSecret(secretAlias);
 
                 if (!secretsUpdated.isDone())
                     secretsUpdated.setResult(true);

tracdap-services/tracdap-svc-orch/src/main/java/org/finos/tracdap/svc/orch/service/JobProcessorHelpers.java

@@ -20,8 +20,10 @@
 import org.finos.tracdap.api.*;
 import org.finos.tracdap.api.internal.InternalMetadataApiGrpc;
 import org.finos.tracdap.common.config.ConfigHelpers;
+import org.finos.tracdap.common.config.ConfigKeys;
 import org.finos.tracdap.common.config.ConfigManager;
 import org.finos.tracdap.common.config.IDynamicResources;
+import org.finos.tracdap.common.exception.EConsistencyValidation;
 import org.finos.tracdap.common.exception.EUnexpected;
 import org.finos.tracdap.common.metadata.MetadataBundle;
 import org.finos.tracdap.common.metadata.MetadataCodec;
@@ -306,7 +308,7 @@ JobState buildJobConfig(JobState jobState) {
 
         for (var storageEntry : internalStorage.entrySet()) {
             var storageKey = storageEntry.getKey();
-            var storage = translateResourceConfig(storageEntry.getValue());
+            var storage = translateResourceConfig(storageKey, storageEntry.getValue(), jobState);
             storageConfig.putBuckets(storageKey, storage);
         }
 
@@ -315,7 +317,7 @@ JobState buildJobConfig(JobState jobState) {
 
         for (var storageEntry : externalStorage.entrySet()) {
             var storageKey = storageEntry.getKey();
-            var storage = translateResourceConfig(storageEntry.getValue());
+            var storage = translateResourceConfig(storageKey, storageEntry.getValue(), jobState);
             storageConfig.putExternal(storageKey, storage);
         }
 
@@ -336,27 +338,71 @@ JobState buildJobConfig(JobState jobState) {
                 resource -> resource.getResourceType() == ResourceType.MODEL_REPOSITORY);
 
         for (var repoEntry : repositories.entrySet()) {
+
             var repoKey = repoEntry.getKey();
-            var repoConfig = translateResourceConfig(repoEntry.getValue());
-            sysConfig.putRepositories(repoKey, repoConfig);
+
+            // Only translate repositories required for the job
+            if (jobUsesRepository(repoKey, jobState)) {
+                var repoConfig = translateResourceConfig(repoKey, repoEntry.getValue(), jobState);
+                sysConfig.putRepositories(repoKey, repoConfig);
+            }
         }
 
         jobState.sysConfig = sysConfig.build();
 
         return jobState;
     }
 
-    private PluginConfig translateResourceConfig(ResourceDefinition resource) {
+    private boolean jobUsesRepository(String repoKey, JobState jobState) {
+
+        // This method filters repo resources for the currently known job types
+        // TODO: Generic resource filtering in the IJobLogic interface
+
+        // Import model jobs can refer to repositories directly
+        if (jobState.jobType == JobType.IMPORT_MODEL) {
+            var importModelJob = jobState.definition.getImportModel();
+            if (importModelJob.getRepository().equals(repoKey)) {
+                return true;
+            }
+        }
+
+        // Check if any resources refer to the repo key
+        for (var object : jobState.resources.values()) {
+            if (object.getObjectType() == ObjectType.MODEL) {
+                if (object.getModel().getRepository().equals(repoKey)) {
+                    return true;
+                }
+            }
+        }
+
+        return false;
+    }
+
+    private PluginConfig translateResourceConfig(String resourceKey, ResourceDefinition resource, JobState jobState) {
 
         var pluginConfig = ConfigHelpers.resourceToPluginConfig(resource).toBuilder();
 
+        var tenantScope = String.format("/%s/%s/", ConfigKeys.TENANT_SCOPE, jobState.tenant);
+
         for (var secretEntry : pluginConfig.getSecretsMap().entrySet()) {
 
-            var propKey = secretEntry.getKey();
-            var secretKey = secretEntry.getValue();
-            var secret = configManager.loadPassword(secretKey);
+            var propertyName = secretEntry.getKey();
+            var secretAlias = secretEntry.getValue();
+
+            // Secret loader will not load the secret if the alias is not valid
+            // This handling provides more meaningful errors
+            if (secretAlias.isBlank() || !secretAlias.startsWith(tenantScope)) {
+
+                var message = String.format("Resource configuration for [%s] is not valid", resourceKey);
+                var detail = String.format("Inconsistent secret alias for [%s]", propertyName);
+
+                log.error("{}: {}", message, detail);
+                throw new EConsistencyValidation(message);
+            }
+
+            var secret = configManager.loadPassword(secretAlias);
 
-            pluginConfig.putProperties(propKey, secret);
+            pluginConfig.putProperties(propertyName, secret);
         }
 
         pluginConfig.clearSecrets();