diff options
author | Bjørn Christian Seime <bjorncs@yahooinc.com> | 2023-02-03 13:04:44 +0100 |
---|---|---|
committer | Bjørn Christian Seime <bjorncs@yahooinc.com> | 2023-02-03 13:04:44 +0100 |
commit | f22e00a151a7071a4c2f36295679e08215202b46 (patch) | |
tree | 0d566b99581657fef7607c3180a52b0ccf6c1b4c /athenz-identity-provider-service | |
parent | 90892ba4d2a302b1a262fdd1198fac8c6724e44f (diff) |
Make unit tests more resilient
Introduce a validation exception. Verify in unit test that validation fails of expected reason.
Diffstat (limited to 'athenz-identity-provider-service')
2 files changed, 56 insertions, 27 deletions
diff --git a/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/InstanceValidator.java b/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/InstanceValidator.java index b316d9fb0b4..15c7b620e44 100644 --- a/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/InstanceValidator.java +++ b/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/InstanceValidator.java @@ -24,6 +24,7 @@ import java.util.Collection; import java.util.List; import java.util.Optional; import java.util.Set; +import java.util.function.Supplier; import java.util.logging.Level; import java.util.logging.Logger; import java.util.stream.Collectors; @@ -72,34 +73,42 @@ public class InstanceValidator { } public boolean isValidInstance(InstanceConfirmation instanceConfirmation) { - SignedIdentityDocument signedIdentityDocument = EntityBindingsMapper.toSignedIdentityDocument(instanceConfirmation.signedIdentityDocument); + try { + validateInstance(instanceConfirmation); + return true; + } catch (ValidationException e) { + log.log(e.logLevel(), e.messageSupplier()); + return false; + } + } + + public void validateInstance(InstanceConfirmation req) throws ValidationException { + SignedIdentityDocument signedIdentityDocument = EntityBindingsMapper.toSignedIdentityDocument(req.signedIdentityDocument); VespaUniqueInstanceId providerUniqueId = signedIdentityDocument.providerUniqueId(); ApplicationId applicationId = ApplicationId.from( providerUniqueId.tenant(), providerUniqueId.application(), providerUniqueId.instance()); - VespaUniqueInstanceId csrProviderUniqueId = getVespaUniqueInstanceId(instanceConfirmation); + VespaUniqueInstanceId csrProviderUniqueId = getVespaUniqueInstanceId(req); if(! providerUniqueId.equals(csrProviderUniqueId)) { - log.log(Level.WARNING, String.format("Instance %s has invalid provider unique ID in CSR (%s)", providerUniqueId, csrProviderUniqueId)); - return false; + var msg = String.format("Instance %s has invalid provider unique ID in CSR (%s)", providerUniqueId, csrProviderUniqueId); + throw new ValidationException(Level.WARNING, () -> msg); } - if (! isSameIdentityAsInServicesXml(applicationId, instanceConfirmation.domain, instanceConfirmation.service)) { - return false; + if (! isSameIdentityAsInServicesXml(applicationId, req.domain, req.service)) { + Supplier<String> msg = () -> "Invalid identity '%s.%s' in services.xml".formatted(req.domain, req.service); + throw new ValidationException(Level.FINE, msg); } log.log(Level.FINE, () -> String.format("Validating instance %s.", providerUniqueId)); PublicKey publicKey = keyProvider.getPublicKey(signedIdentityDocument.signingKeyVersion()); if (! signer.hasValidSignature(signedIdentityDocument, publicKey)) { - log.log(Level.SEVERE, () -> String.format("Instance %s has invalid signature.", providerUniqueId)); - return false; + var msg = String.format("Instance %s has invalid signature.", providerUniqueId); + throw new ValidationException(Level.SEVERE, () -> msg); } - if(validateAttributes(instanceConfirmation, providerUniqueId)) { - log.log(Level.FINE, () -> String.format("Instance %s is valid.", providerUniqueId)); - return true; - } - return false; + validateAttributes(req, providerUniqueId); + log.log(Level.FINE, () -> String.format("Instance %s is valid.", providerUniqueId)); } // TODO Add actual validation. Cannot reuse isValidInstance as identity document is not part of the refresh request. @@ -111,7 +120,11 @@ public class InstanceValidator { confirmation.provider, confirmation.attributes.get(SAN_DNS_ATTRNAME))); try { - return validateAttributes(confirmation, getVespaUniqueInstanceId(confirmation)); + validateAttributes(confirmation, getVespaUniqueInstanceId(confirmation)); + return true; + } catch (ValidationException e) { + log.log(e.logLevel(), e.messageSupplier()); + return false; } catch (Exception e) { log.log(Level.WARNING, "Encountered exception while refreshing certificate for confirmation: " + confirmation, e); return false; @@ -132,10 +145,11 @@ public class InstanceValidator { .orElse(null); } - private boolean validateAttributes(InstanceConfirmation confirmation, VespaUniqueInstanceId vespaUniqueInstanceId) { + private void validateAttributes(InstanceConfirmation confirmation, VespaUniqueInstanceId vespaUniqueInstanceId) + throws ValidationException { if(vespaUniqueInstanceId == null) { - log.log(Level.WARNING, "Unable to find unique instance ID in refresh request: " + confirmation.toString()); - return false; + var msg = "Unable to find unique instance ID in refresh request: " + confirmation.toString(); + throw new ValidationException(Level.WARNING, () -> msg); } // Find node matching vespa unique id @@ -145,8 +159,8 @@ public class InstanceValidator { .findFirst() // Should be only one .orElse(null); if(node == null) { - log.log(Level.WARNING, "Invalid InstanceConfirmation, No nodes matching uniqueId: " + vespaUniqueInstanceId); - return false; + var msg = "Invalid InstanceConfirmation, No nodes matching uniqueId: " + vespaUniqueInstanceId; + throw new ValidationException(Level.WARNING, () -> msg); } // Find list of ipaddresses @@ -163,8 +177,8 @@ public class InstanceValidator { // Validate that ipaddresses in request are valid for node if(! nodeIpAddresses.containsAll(ips)) { - log.log(Level.WARNING, "Invalid InstanceConfirmation, wrong ip in : " + vespaUniqueInstanceId); - return false; + var msg = "Invalid InstanceConfirmation, wrong ip in : " + vespaUniqueInstanceId; + throw new ValidationException(Level.WARNING, () -> msg); } var urisCommaSeparated = confirmation.attributes.get(SAN_URI_ATTRNAME); @@ -173,17 +187,15 @@ public class InstanceValidator { requestedUris = Optional.ofNullable(urisCommaSeparated).stream() .flatMap(s -> Arrays.stream(s.split(","))).map(URI::create).collect(Collectors.toSet()); } catch (IllegalArgumentException e) { - log.log(Level.WARNING, "Invalid SAN URIs: " + urisCommaSeparated); - return false; + throw new ValidationException(Level.WARNING, () -> "Invalid SAN URIs: " + urisCommaSeparated, e); } var clusterType = node.allocation().map(a -> a.membership().cluster().type()).orElse(null); Set<URI> allowedUris = clusterType != null ? Set.of(URI.create("vespa://cluster-type/%s".formatted(clusterType.name()))) : Set.of(); if (!allowedUris.containsAll(requestedUris)) { - log.log(Level.WARNING, "Illegal SAN URIs: expected '%s' found '%s'".formatted(allowedUris, requestedUris)); - return false; + Supplier<String> msg = () -> "Illegal SAN URIs: expected '%s' found '%s'".formatted(allowedUris, requestedUris); + throw new ValidationException(Level.WARNING, msg); } - return true; } private boolean nodeMatchesVespaUniqueId(Node node, VespaUniqueInstanceId vespaUniqueInstanceId) { @@ -237,4 +249,16 @@ public class InstanceValidator { return true; } + + public static class ValidationException extends Exception { + private final Level logLevel; + private final Supplier<String> msg; + + public ValidationException(Level logLevel, Supplier<String> msg) { this(logLevel, msg, null); } + public ValidationException(Level logLevel, Supplier<String> msg, Throwable cause) { super(cause); this.logLevel = logLevel; this.msg = msg; } + + @Override public String getMessage() { return msg.get(); } + public Level logLevel() { return logLevel; } + public Supplier<String> messageSupplier() { return msg; } + } } diff --git a/athenz-identity-provider-service/src/test/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/InstanceValidatorTest.java b/athenz-identity-provider-service/src/test/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/InstanceValidatorTest.java index fbaa57d9694..26da66d4ac6 100644 --- a/athenz-identity-provider-service/src/test/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/InstanceValidatorTest.java +++ b/athenz-identity-provider-service/src/test/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/InstanceValidatorTest.java @@ -19,6 +19,7 @@ import com.yahoo.vespa.athenz.identityprovider.api.IdentityType; import com.yahoo.vespa.athenz.identityprovider.api.SignedIdentityDocument; import com.yahoo.vespa.athenz.identityprovider.api.VespaUniqueInstanceId; import com.yahoo.vespa.athenz.identityprovider.client.IdentityDocumentSigner; +import com.yahoo.vespa.hosted.athenz.instanceproviderservice.InstanceValidator.ValidationException; import com.yahoo.vespa.hosted.provision.Node; import com.yahoo.vespa.hosted.provision.NodeList; import com.yahoo.vespa.hosted.provision.NodeRepository; @@ -42,7 +43,9 @@ import java.util.stream.Stream; import static com.yahoo.vespa.hosted.athenz.instanceproviderservice.InstanceValidator.SERVICE_PROPERTIES_DOMAIN_KEY; import static com.yahoo.vespa.hosted.athenz.instanceproviderservice.InstanceValidator.SERVICE_PROPERTIES_SERVICE_KEY; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; @@ -142,7 +145,9 @@ public class InstanceValidatorTest { var instanceValidator = new InstanceValidator(keyProvider, provider, mockNodeRepo(), new IdentityDocumentSigner(), vespaTenantDomain); var instanceConfirmation = createRegisterInstanceConfirmation(applicationId, domain, service); instanceConfirmation.set("sanURI", "vespa://cluster-type/content"); - assertFalse(instanceValidator.isValidInstance(instanceConfirmation)); + var exception = assertThrows(ValidationException.class, () -> instanceValidator.validateInstance(instanceConfirmation)); + var expectedMsg = "Illegal SAN URIs: expected '[vespa://cluster-type/container]' found '[vespa://cluster-type/content]'"; + assertEquals(expectedMsg, exception.getMessage()); } @Test |