diff options
14 files changed, 457 insertions, 126 deletions
diff --git a/config-model-api/abi-spec.json b/config-model-api/abi-spec.json index 841bac213ed..e5c2be4eb43 100644 --- a/config-model-api/abi-spec.json +++ b/config-model-api/abi-spec.json @@ -676,7 +676,8 @@ "public static final enum com.yahoo.config.application.api.ValidationId globalEndpointChange", "public static final enum com.yahoo.config.application.api.ValidationId redundancyIncrease", "public static final enum com.yahoo.config.application.api.ValidationId redundancyOne", - "public static final enum com.yahoo.config.application.api.ValidationId pagedSettingRemoval" + "public static final enum com.yahoo.config.application.api.ValidationId pagedSettingRemoval", + "public static final enum com.yahoo.config.application.api.ValidationId certificateRemoval" ] }, "com.yahoo.config.application.api.ValidationOverrides$Allow" : { diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/ValidationId.java b/config-model-api/src/main/java/com/yahoo/config/application/api/ValidationId.java index 33b7e9e6a57..571cc3c7d5c 100644 --- a/config-model-api/src/main/java/com/yahoo/config/application/api/ValidationId.java +++ b/config-model-api/src/main/java/com/yahoo/config/application/api/ValidationId.java @@ -26,7 +26,8 @@ public enum ValidationId { globalEndpointChange("global-endpoint-change"), // Changing global endpoints redundancyIncrease("redundancy-increase"), // Increasing redundancy - may easily cause feed blocked redundancyOne("redundancy-one"), // redundancy=1 requires a validation override on first deployment - pagedSettingRemoval("paged-setting-removal"); // May cause content nodes to run out of memory + pagedSettingRemoval("paged-setting-removal"), // May cause content nodes to run out of memory + certificateRemoval("certificate-removal"); // Remove data plane certificates private final String id; diff --git a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/CloudDataPlaneFilterValidator.java b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/CloudDataPlaneFilterValidator.java new file mode 100644 index 00000000000..60705ad9b51 --- /dev/null +++ b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/CloudDataPlaneFilterValidator.java @@ -0,0 +1,71 @@ +package com.yahoo.vespa.model.application.validation; + +import com.yahoo.config.application.api.ApplicationPackage; +import com.yahoo.config.model.deploy.DeployState; +import com.yahoo.io.IOUtils; +import com.yahoo.io.reader.NamedReader; +import com.yahoo.path.Path; +import com.yahoo.security.X509CertificateUtils; +import com.yahoo.vespa.model.VespaModel; + +import java.io.IOException; +import java.security.cert.X509Certificate; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.logging.Logger; +import java.util.stream.Collectors; + +public class CloudDataPlaneFilterValidator extends Validator { + + private static final Logger log = Logger.getLogger(CloudDataPlaneFilterValidator.class.getName()); + + @Override + public void validate(VespaModel model, DeployState deployState) { + if (!deployState.isHosted()) return; + if (!deployState.zone().system().isPublic()) return; + if (!deployState.featureFlags().enableDataPlaneFilter()) return; + + validateUniqueCertificates(deployState); + } + + private void validateUniqueCertificates(DeployState deployState) { + List<NamedReader> certFiles = deployState.getApplicationPackage().getFiles(ApplicationPackage.SECURITY_DIR, ".pem"); + + Map<String, List<X509Certificate>> configuredCertificates = certFiles.stream() + .collect(Collectors.toMap(NamedReader::getName, CloudDataPlaneFilterValidator::readCertificates)); + + Set<X509Certificate> duplicates = new HashSet<>(); + Set<X509Certificate> globalUniqueCerts = new HashSet<>(); + for (Map.Entry<String, List<X509Certificate>> certificateFile : configuredCertificates.entrySet()) { + List<X509Certificate> duplicatesInFile = certificateFile.getValue().stream() + .filter(i -> !globalUniqueCerts.add(i)) + .toList(); + duplicates.addAll(duplicatesInFile); + } + if (!duplicates.isEmpty()) { + List<String> filesWithDuplicates = configuredCertificates.entrySet().stream() + .filter(entry -> entry.getValue().stream().anyMatch(duplicates::contains)) + .map(Map.Entry::getKey) + .map(Path::fromString) + .map(Path::getName) + .map(p -> ApplicationPackage.SECURITY_DIR.append(p).getRelative()) + .sorted() + .toList(); + throw new IllegalArgumentException("Duplicate certificate(s) detected in files: %s. Certificate subject of duplicates: %s" + .formatted(filesWithDuplicates.toString(), + duplicates.stream().map(cert -> cert.getSubjectX500Principal().getName()).toList().toString())); + } + } + + private static List<X509Certificate> readCertificates(NamedReader reader) { + try { + return X509CertificateUtils.certificateListFromPem(IOUtils.readAll(reader)); + } catch (IOException e) { + log.warning("Exception reading certificate list from application package. File: %s, exception message: %s" + .formatted(reader.getName(), e.getMessage())); + throw new RuntimeException("Error reading certificates from application package", e); + } + } +} diff --git a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/Validation.java b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/Validation.java index 68b5adcc84f..b81b3524c67 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/Validation.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/Validation.java @@ -86,6 +86,7 @@ public class Validation { new CloudWatchValidator().validate(model, deployState); new QuotaValidator().validate(model, deployState); new UriBindingsValidator().validate(model, deployState); + new CloudDataPlaneFilterValidator().validate(model, deployState); additionalValidators.forEach(v -> v.validate(model, deployState)); diff --git a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/CertificateRemovalChangeValidator.java b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/CertificateRemovalChangeValidator.java new file mode 100644 index 00000000000..50fd6572bcc --- /dev/null +++ b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/CertificateRemovalChangeValidator.java @@ -0,0 +1,47 @@ +package com.yahoo.vespa.model.application.validation.change; + +import com.yahoo.config.application.api.ValidationId; +import com.yahoo.config.application.api.ValidationOverrides; +import com.yahoo.config.model.api.ConfigChangeAction; +import com.yahoo.vespa.model.VespaModel; +import com.yahoo.vespa.model.container.http.Client; + +import java.security.cert.X509Certificate; +import java.time.Instant; +import java.util.Collection; +import java.util.List; + +public class CertificateRemovalChangeValidator implements ChangeValidator { + @Override + public List<ConfigChangeAction> validate(VespaModel current, VespaModel next, ValidationOverrides overrides, Instant now) { + + current.getContainerClusters() + .forEach((clusterId, currentCluster) -> { + validateClients(clusterId, currentCluster.getClients(), next.getContainerClusters().get(clusterId).getClients(), overrides, now); + }); + + return List.of(); + } + + void validateClients(String clusterId, List<Client> current, List<Client> next, ValidationOverrides overrides, Instant now) { + + List<X509Certificate> currentCertificates = current.stream() + .map(Client::certificates) + .flatMap(Collection::stream) + .toList(); + List<X509Certificate> nextCertificates = next.stream() + .map(Client::certificates) + .flatMap(Collection::stream) + .toList(); + + List<X509Certificate> missingCerts = currentCertificates.stream().filter(cert -> !nextCertificates.contains(cert)).toList(); + if (!missingCerts.isEmpty()) { + overrides.invalid(ValidationId.certificateRemoval, + "Data plane certificate(s) from cluster '" + clusterId + "' is removed " + + "(removed certificates: " + missingCerts.stream().map(x509Certificate -> x509Certificate.getSubjectX500Principal().getName()).toList() + ") " + + "This can cause client connection issues.", + now); + } + + } +} diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java index 79d8512e447..007e8401c70 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java @@ -38,6 +38,7 @@ import com.yahoo.config.provision.NodeType; import com.yahoo.config.provision.Zone; import com.yahoo.container.bundle.BundleInstantiationSpecification; import com.yahoo.container.logging.FileConnectionLog; +import com.yahoo.io.IOUtils; import com.yahoo.osgi.provider.model.ComponentModel; import com.yahoo.path.Path; import com.yahoo.schema.OnnxModel; @@ -100,9 +101,8 @@ import org.w3c.dom.Element; import org.w3c.dom.Node; import java.io.IOException; -import java.io.InputStream; +import java.io.Reader; import java.net.URI; -import java.nio.charset.StandardCharsets; import java.security.cert.X509Certificate; import java.util.ArrayList; import java.util.Arrays; @@ -529,10 +529,10 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> { private List<X509Certificate> getCertificates(ApplicationFile file) { try { - InputStream inputStream = file.createInputStream(); - byte[] bytes = inputStream.readAllBytes(); - inputStream.close(); - return X509CertificateUtils.certificateListFromPem(new String(bytes, StandardCharsets.UTF_8)); + Reader reader = file.createReader(); + String certPem = IOUtils.readAll(reader); + reader.close(); + return X509CertificateUtils.certificateListFromPem(certPem); } catch (IOException e) { throw new RuntimeException(e); } diff --git a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/CloudDataPlaneFilterValidatorTest.java b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/CloudDataPlaneFilterValidatorTest.java new file mode 100644 index 00000000000..ed39260bdd2 --- /dev/null +++ b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/CloudDataPlaneFilterValidatorTest.java @@ -0,0 +1,165 @@ +package com.yahoo.vespa.model.application.validation; + +import com.yahoo.config.application.api.ApplicationPackage; +import com.yahoo.config.model.NullConfigModelRegistry; +import com.yahoo.config.model.api.EndpointCertificateSecrets; +import com.yahoo.config.model.deploy.DeployState; +import com.yahoo.config.model.deploy.TestProperties; +import com.yahoo.config.model.test.MockApplicationPackage; +import com.yahoo.config.provision.Environment; +import com.yahoo.config.provision.RegionName; +import com.yahoo.config.provision.SystemName; +import com.yahoo.config.provision.Zone; +import com.yahoo.path.Path; +import com.yahoo.security.KeyAlgorithm; +import com.yahoo.security.KeyUtils; +import com.yahoo.security.SignatureAlgorithm; +import com.yahoo.security.X509CertificateBuilder; +import com.yahoo.security.X509CertificateUtils; +import com.yahoo.vespa.model.VespaModel; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.xml.sax.SAXException; + +import javax.security.auth.x500.X500Principal; +import java.io.File; +import java.io.IOException; +import java.io.StringReader; +import java.math.BigInteger; +import java.security.KeyPair; +import java.security.cert.X509Certificate; +import java.time.Instant; +import java.time.temporal.ChronoUnit; +import java.util.List; +import java.util.Map; +import java.util.Optional; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +public class CloudDataPlaneFilterValidatorTest { + + @TempDir + public File applicationFolder; + + @Test + void validator_accepts_distinct_client_certificates() throws IOException, SAXException { + String certFile1 = "security/foo.pem"; + String certFile2 = "security/bar.pem"; + String servicesXml = """ + <services version='1.0'> + <container version='1.0'> + <clients> + <client id="a" permissions="read,write"> + <certificate file="%s"/> + </client> + <client id="b" permissions="read,write"> + <certificate file="%s"/> + </client> + </clients> + </container> + </services> + """.formatted(certFile1, certFile2); + + DeployState deployState = createDeployState(servicesXml, + Map.of( + certFile1, List.of(createCertificate("foo")), + certFile2, List.of(createCertificate("bar")))); + + VespaModel model = new VespaModel(new NullConfigModelRegistry(), deployState); + new CloudDataPlaneFilterValidator().validate(model, deployState); + } + + @Test + void validator_rejects_duplicate_client_certificates_different_files() throws IOException, SAXException { + String certFile1 = "security/a.pem"; + String certFile2 = "security/b.pem"; + X509Certificate certificate = createCertificate("a"); + String servicesXml = """ + <services version='1.0'> + <container version='1.0'> + <clients> + <client id="a" permissions="read,write"> + <certificate file="%s"/> + </client> + <client id="b" permissions="read,write"> + <certificate file="%s"/> + </client> + </clients> + </container> + </services> + """.formatted(certFile1, certFile2); + + DeployState deployState = createDeployState(servicesXml, + Map.of( + certFile1, List.of(certificate), + certFile2, List.of(certificate))); + + VespaModel model = new VespaModel(new NullConfigModelRegistry(), deployState); + IllegalArgumentException illegalArgumentException = Assertions.assertThrows(IllegalArgumentException.class, () -> + new CloudDataPlaneFilterValidator().validate(model, deployState)); + assertEquals( + "Duplicate certificate(s) detected in files: [%s, %s]. Certificate subject of duplicates: [%s]".formatted(certFile1, certFile2, certificate.getSubjectX500Principal().getName()), + illegalArgumentException.getMessage()); + } + + @Test + void validator_rejects_duplicate_client_certificates_same_file() throws IOException, SAXException { + String certFile1 = "security/a.pem"; + X509Certificate certificate = createCertificate("a"); + String servicesXml = """ + <services version='1.0'> + <container version='1.0'> + <clients> + <client id="a" permissions="read,write"> + <certificate file="%s"/> + </client> + </clients> + </container> + </services> + """.formatted(certFile1); + + DeployState deployState = createDeployState(servicesXml, + Map.of(certFile1, List.of(certificate, certificate))); + + VespaModel model = new VespaModel(new NullConfigModelRegistry(), deployState); + IllegalArgumentException illegalArgumentException = Assertions.assertThrows(IllegalArgumentException.class, () -> + new CloudDataPlaneFilterValidator().validate(model, deployState)); + assertEquals( + "Duplicate certificate(s) detected in files: [%s]. Certificate subject of duplicates: [%s]".formatted(certFile1, certificate.getSubjectX500Principal().getName()), + illegalArgumentException.getMessage()); + } + + + private DeployState createDeployState(String servicesXml, Map<String, List<X509Certificate>> certificates) { + + ApplicationPackage applicationPackage = new MockApplicationPackage.Builder() + .withRoot(applicationFolder) + .withServices(servicesXml) + .build(); + + applicationPackage.getFile(Path.fromString("security")).createDirectory(); + certificates.forEach((file, certList) -> + applicationPackage.getFile(Path.fromString(file)).writeFile(new StringReader(X509CertificateUtils.toPem(certList)))); + + return new DeployState.Builder() + .applicationPackage(applicationPackage) + .properties( + new TestProperties() + .setEndpointCertificateSecrets(Optional.of(new EndpointCertificateSecrets("CERT", "KEY"))) + .setHostedVespa(true) + .setEnableDataPlaneFilter(true)) + .zone(new Zone(SystemName.PublicCd, Environment.dev, RegionName.defaultName())) + .build(); + } + + static X509Certificate createCertificate(String cn) throws IOException { + KeyPair keyPair = KeyUtils.generateKeypair(KeyAlgorithm.EC, 256); + X500Principal subject = new X500Principal("CN=" + cn); + return X509CertificateBuilder + .fromKeypair( + keyPair, subject, Instant.now(), Instant.now().plus(1, ChronoUnit.DAYS), SignatureAlgorithm.SHA512_WITH_ECDSA, BigInteger.valueOf(1)) + .build(); + } + +} diff --git a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/CertificateRemovalChangeValidatorTest.java b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/CertificateRemovalChangeValidatorTest.java new file mode 100644 index 00000000000..f89c75362da --- /dev/null +++ b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/CertificateRemovalChangeValidatorTest.java @@ -0,0 +1,51 @@ +package com.yahoo.vespa.model.application.validation.change; + +import com.yahoo.config.application.api.ValidationOverrides; +import com.yahoo.security.X509CertificateUtils; +import com.yahoo.vespa.model.container.http.Client; +import org.junit.jupiter.api.Test; + +import java.security.cert.X509Certificate; +import java.time.Duration; +import java.time.Instant; +import java.time.LocalDate; +import java.time.ZoneOffset; +import java.time.format.DateTimeFormatter; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertThrows; + +public class CertificateRemovalChangeValidatorTest { + + private static final String validationOverrides = + "<validation-overrides>\n" + + " <allow until='2000-01-14' comment='test override'>certificate-removal</allow>\n" + + "</validation-overrides>\n"; + + @Test + void validate() { + Instant now = LocalDate.parse("2000-01-01", DateTimeFormatter.ISO_DATE).atStartOfDay().atZone(ZoneOffset.UTC).toInstant(); + + Client c1 = new Client("c1", List.of(), List.of(certificate("cn=c1"))); + Client c2 = new Client("c2", List.of(), List.of(certificate("cn=c2"))); + Client c3 = new Client("c3", List.of(), List.of(certificate("cn=c3"))); + + CertificateRemovalChangeValidator validator = new CertificateRemovalChangeValidator(); + + // Adding certs -> ok + validator.validateClients("clusterId", List.of(c1,c2), List.of(c1, c2, c3), ValidationOverrides.empty, now); + + // Removing certs -> fails + assertThrows(ValidationOverrides.ValidationException.class, + () ->validator.validateClients("clusterId", List.of(c1, c2, c3), List.of(c1, c3), ValidationOverrides.empty, now)); + + // Removing certs with validationoverrides -> ok + validator.validateClients("clusterId", List.of(c1, c2, c3), List.of(c1, c3), ValidationOverrides.fromXml(validationOverrides), now); + + } + + static X509Certificate certificate(String cn) { + return X509CertificateUtils.createSelfSigned(cn, Duration.ofHours(1)).certificate(); + } + +} diff --git a/config/src/main/java/com/yahoo/config/subscription/impl/JRTConfigRequester.java b/config/src/main/java/com/yahoo/config/subscription/impl/JRTConfigRequester.java index 0ef99930dde..b4a2a29102b 100644 --- a/config/src/main/java/com/yahoo/config/subscription/impl/JRTConfigRequester.java +++ b/config/src/main/java/com/yahoo/config/subscription/impl/JRTConfigRequester.java @@ -23,6 +23,7 @@ import java.util.concurrent.TimeUnit; import java.util.logging.Level; import java.util.logging.Logger; +import static com.yahoo.jrt.ErrorCode.CONNECTION; import static java.util.logging.Level.FINE; import static java.util.logging.Level.FINEST; import static java.util.logging.Level.SEVERE; @@ -132,7 +133,7 @@ public class JRTConfigRequester implements RequestWaiter { if (sub.isClosed()) return; // Avoid error messages etc. after closing boolean validResponse = jrtReq.validateResponse(); - log.log(FINE, () -> "Request callback " + (validResponse ? "valid" : "invalid") + ". Req: " + jrtReq + "\nSpec: " + connection); + log.log(FINE, () -> "Response " + (validResponse ? "valid" : "invalid") + ". Req: " + jrtReq + "\nSpec: " + connection); Trace trace = jrtReq.getResponseTrace(); trace.trace(TRACELEVEL, "JRTConfigRequester.doHandle()"); log.log(FINEST, () -> trace.toString()); @@ -142,34 +143,26 @@ public class JRTConfigRequester implements RequestWaiter { handleFailedRequest(jrtReq, sub, connection); } - private void logError(JRTClientConfigRequest jrtReq, Connection connection) { - switch (jrtReq.errorCode()) { - case com.yahoo.jrt.ErrorCode.CONNECTION: - log.log(FINE, () -> "Request callback failed: " + jrtReq.errorMessage() + - "\nConnection spec: " + connection); - break; - case ErrorCode.APPLICATION_NOT_LOADED: - case ErrorCode.UNKNOWN_VESPA_VERSION: - logWarning(jrtReq, connection); - break; - default: - log.log(WARNING, "Request callback failed. Req: " + jrtReq + "\nSpec: " + connection.getAddress() + - " . Req error message: " + jrtReq.errorMessage()); - break; - } - } + private void logFailingRequest(JRTClientConfigRequest jrtReq, Connection connection) { + if (closed) return; - private void logWarning(JRTClientConfigRequest jrtReq, Connection connection) { - if ( ! closed && timeForLastLogWarning.isBefore(Instant.now().minus(delayBetweenWarnings))) { - log.log(WARNING, "Request callback failed: " + ErrorCode.getName(jrtReq.errorCode()) + + if (jrtReq.errorCode() == CONNECTION) { + log.log(FINE, () -> "Request failed: " + jrtReq.errorMessage() + + "\nConnection spec: " + connection); + } else if (timeForLastLogWarning.isBefore(Instant.now().minus(delayBetweenWarnings))) { + log.log(WARNING, "Request failed: " + ErrorCode.getName(jrtReq.errorCode()) + ". Connection spec: " + connection.getAddress() + ", error message: " + jrtReq.errorMessage()); timeForLastLogWarning = Instant.now(); + } else { + log.log(FINE, () -> "Request failed: " + ErrorCode.getName(jrtReq.errorCode()) + + ". Connection spec: " + connection.getAddress() + + ", error message: " + jrtReq.errorMessage()); } } private void handleFailedRequest(JRTClientConfigRequest jrtReq, JRTConfigSubscription<ConfigInstance> sub, Connection connection) { - logError(jrtReq, connection); + logFailingRequest(jrtReq, connection); connectionPool.switchConnection(connection); if (failures < 10) diff --git a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java index 52fe91b6997..91904deefd5 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java @@ -283,7 +283,7 @@ public class Flags { public static final UnboundBooleanFlag ENABLE_PROXY_PROTOCOL_MIXED_MODE = defineFeatureFlag( "enable-proxy-protocol-mixed-mode", true, - List.of("tokle"), "2022-05-09", "2022-12-01", + List.of("tokle"), "2022-05-09", "2023-01-31", "Enable or disable proxy protocol mixed mode", "Takes effect on redeployment", APPLICATION_ID); @@ -304,7 +304,7 @@ public class Flags { public static final UnboundBooleanFlag USE_YUM_PROXY_V2 = defineFeatureFlag( "use-yumproxy-v2", false, - List.of("tokle"), "2022-05-05", "2022-12-01", + List.of("tokle"), "2022-05-05", "2023-01-31", "Use yumproxy-v2", "Takes effect on host admin restart", HOSTNAME); diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailer.java index 32eac49a288..51b59ab77eb 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailer.java @@ -286,7 +286,7 @@ public class NodeFailer extends NodeRepositoryMaintainer { public enum ThrottlePolicy { - hosted(Duration.ofDays(1), 0.03, 2), + hosted(Duration.ofDays(1), 0.04, 2), disabled(Duration.ZERO, 0, 0); private final Duration throttleWindow; diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailerTest.java index ede958ef083..47803594148 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailerTest.java @@ -563,33 +563,35 @@ public class NodeFailerTest { NodeList allNodes = tester.nodeRepository.nodes().list(); assertEquals(500, allNodes.size()); - // 2 hours pass, 20 nodes (4%) die + // 2 hours pass, many nodes fail tester.runMaintainers(); + int downNodes = 25; // 5% + int allowedToFail = 20; // 4% allNodes.state(Node.State.active) .nodeType(NodeType.tenant) .stream() - .limit(20) + .limit(downNodes) .forEach(host -> tester.serviceMonitor.setHostDown(host.hostname())); tester.runMaintainers(); tester.clock.advance(Duration.ofHours(2)); tester.runMaintainers(); - // 3% are allowed to fail - assertEquals(15, tester.nodeRepository.nodes().list(Node.State.failed).size()); + // Fails nodes up to throttle limit + assertEquals(allowedToFail, tester.nodeRepository.nodes().list(Node.State.failed).size()); assertEquals("Throttling is indicated by the metric.", 1, tester.metric.values.get(NodeFailer.throttlingActiveMetric)); - assertEquals("Throttled node failures", 5, tester.metric.values.get(NodeFailer.throttledNodeFailuresMetric)); + assertEquals("Throttled node failures", downNodes - allowedToFail, tester.metric.values.get(NodeFailer.throttledNodeFailuresMetric)); // 6 more hours pass, no more nodes are failed tester.clock.advance(Duration.ofHours(6)); tester.runMaintainers(); - assertEquals(15, tester.nodeRepository.nodes().list(Node.State.failed).size()); + assertEquals(allowedToFail, tester.nodeRepository.nodes().list(Node.State.failed).size()); assertEquals("Throttling is indicated by the metric.", 1, tester.metric.values.get(NodeFailer.throttlingActiveMetric)); - assertEquals("Throttled node failures", 5, tester.metric.values.get(NodeFailer.throttledNodeFailuresMetric)); + assertEquals("Throttled node failures", downNodes - allowedToFail, tester.metric.values.get(NodeFailer.throttledNodeFailuresMetric)); - // 18 more hours pass, 24 hours since the first 10 nodes were failed. The remaining 5 are failed + // 18 more hours pass, 24 hours since the first batch of nodes were failed. The remaining nodes are failed tester.clock.advance(Duration.ofHours(18)); tester.runMaintainers(); - assertEquals(20, tester.nodeRepository.nodes().list(Node.State.failed).size()); + assertEquals(downNodes, tester.nodeRepository.nodes().list(Node.State.failed).size()); assertEquals("Throttling is not indicated by the metric, as no throttled attempt is made.", 0, tester.metric.values.get(NodeFailer.throttlingActiveMetric)); assertEquals("No throttled node failures", 0, tester.metric.values.get(NodeFailer.throttledNodeFailuresMetric)); } diff --git a/searchcore/src/tests/proton/attribute/attribute_directory/CMakeLists.txt b/searchcore/src/tests/proton/attribute/attribute_directory/CMakeLists.txt index 36fb6a92ea7..74c46a27b84 100644 --- a/searchcore/src/tests/proton/attribute/attribute_directory/CMakeLists.txt +++ b/searchcore/src/tests/proton/attribute/attribute_directory/CMakeLists.txt @@ -4,5 +4,6 @@ vespa_add_executable(searchcore_attribute_directory_test_app TEST attribute_directory_test.cpp DEPENDS searchcore_attribute + GTest::GTest ) vespa_add_test(NAME searchcore_attribute_directory_test_app COMMAND searchcore_attribute_directory_test_app) diff --git a/searchcore/src/tests/proton/attribute/attribute_directory/attribute_directory_test.cpp b/searchcore/src/tests/proton/attribute/attribute_directory/attribute_directory_test.cpp index 55de4caed80..98dca748533 100644 --- a/searchcore/src/tests/proton/attribute/attribute_directory/attribute_directory_test.cpp +++ b/searchcore/src/tests/proton/attribute/attribute_directory/attribute_directory_test.cpp @@ -3,9 +3,9 @@ #include <vespa/searchcore/proton/attribute/attribute_directory.h> #include <vespa/searchcore/proton/attribute/attributedisklayout.h> #include <vespa/searchlib/test/directory_handler.h> +#include <vespa/vespalib/gtest/gtest.h> #include <vespa/vespalib/stllike/asciistream.h> #include <vespa/vespalib/stllike/string.h> -#include <vespa/vespalib/testkit/testapp.h> #include <filesystem> #include <vespa/log/log.h> @@ -47,8 +47,8 @@ bool hasWriter(const std::unique_ptr<AttributeDirectory::Writer> &writer) { } -struct Fixture : public DirectoryHandler -{ +class Fixture : public DirectoryHandler { +public: std::shared_ptr<AttributeDiskLayout> _diskLayout; @@ -69,7 +69,7 @@ struct Fixture : public DirectoryHandler } void assertAttributeDiskDir(const vespalib::string &name) { - TEST_DO(assertDiskDir(getAttrDir(name))); + assertDiskDir(getAttrDir(name)); } void assertNotDiskDir(const vespalib::string &name) { @@ -77,7 +77,7 @@ struct Fixture : public DirectoryHandler } void assertNotAttributeDiskDir(const vespalib::string &name) { - TEST_DO(assertNotDiskDir(getAttrDir(name))); + assertNotDiskDir(getAttrDir(name)); } vespalib::string getSnapshotDirComponent(SerialNum serialNum) { @@ -92,11 +92,11 @@ struct Fixture : public DirectoryHandler } void assertSnapshotDir(const vespalib::string &name, SerialNum serialNum) { - TEST_DO(assertDiskDir(getSnapshotDir(name, serialNum))); + assertDiskDir(getSnapshotDir(name, serialNum)); } void assertNotSnapshotDir(const vespalib::string &name, SerialNum serialNum) { - TEST_DO(assertNotDiskDir(getSnapshotDir(name, serialNum))); + assertNotDiskDir(getSnapshotDir(name, serialNum)); } void assertSnapshots(const vespalib::string &name, const vespalib::string &exp) { @@ -104,7 +104,7 @@ struct Fixture : public DirectoryHandler IndexMetaInfo info(attrDir); info.load(); vespalib::string act = toString(info); - EXPECT_EQUAL(exp, act); + EXPECT_EQ(exp, act); } auto createAttributeDir(const vespalib::string &name) { return _diskLayout->createAttributeDir(name); } @@ -116,17 +116,17 @@ struct Fixture : public DirectoryHandler void assertNotGetAttributeDir(const vespalib::string &name) { auto dir = getAttributeDir(name); EXPECT_FALSE(static_cast<bool>(dir)); - TEST_DO(assertNotAttributeDiskDir(name)); + assertNotAttributeDiskDir(name); } void assertGetAttributeDir(const vespalib::string &name, std::shared_ptr<AttributeDirectory> expDir) { auto dir = getAttributeDir(name); EXPECT_TRUE(static_cast<bool>(dir)); - EXPECT_EQUAL(expDir, dir); + EXPECT_EQ(expDir, dir); } void assertCreateAttributeDir(const vespalib::string &name, std::shared_ptr<AttributeDirectory> expDir) { auto dir = getAttributeDir(name); EXPECT_TRUE(static_cast<bool>(dir)); - EXPECT_EQUAL(expDir, dir); + EXPECT_EQ(expDir, dir); } void setupFooSnapshots(SerialNum serialNum) { @@ -136,7 +136,7 @@ struct Fixture : public DirectoryHandler writer->createInvalidSnapshot(serialNum); std::filesystem::create_directory(std::filesystem::path(writer->getSnapshotDir(serialNum))); writer->markValidSnapshot(serialNum); - TEST_DO(assertAttributeDiskDir("foo")); + assertAttributeDiskDir("foo"); } void invalidateFooSnapshots(bool removeDir) { @@ -147,7 +147,7 @@ struct Fixture : public DirectoryHandler if (removeDir) { writer->removeDiskDir(); } - TEST_DO(assertGetAttributeDir("foo", dir)); + assertGetAttributeDir("foo", dir); } void makeInvalidSnapshot(SerialNum serialNum) { @@ -166,45 +166,46 @@ struct Fixture : public DirectoryHandler }; -TEST_F("Test that we can create attribute directory", Fixture) +class AttributeDirectoryTest : public Fixture, public testing::Test {}; + +TEST_F(AttributeDirectoryTest, can_create_attribute_directory) { - auto dir = f.createFooAttrDir(); + auto dir = createFooAttrDir(); EXPECT_TRUE(hasAttributeDir(dir)); } - -TEST_F("Test that attribute directory is persistent", Fixture) +TEST_F(AttributeDirectoryTest, attribute_directory_is_persistent) { - TEST_DO(f.assertNotGetAttributeDir("foo")); - auto dir = f.createFooAttrDir(); + assertNotGetAttributeDir("foo"); + auto dir = createFooAttrDir(); EXPECT_TRUE(hasAttributeDir(dir)); - TEST_DO(f.assertGetAttributeDir("foo", dir)); + assertGetAttributeDir("foo", dir); } -TEST_F("Test that we can remove attribute directory", Fixture) +TEST_F(AttributeDirectoryTest, can_remove_attribute_directory) { - auto dir = f.createFooAttrDir(); + auto dir = createFooAttrDir(); EXPECT_TRUE(hasAttributeDir(dir)); - TEST_DO(f.assertGetAttributeDir("foo", dir)); - f.removeFooAttrDir(10); - TEST_DO(f.assertNotGetAttributeDir("foo")); + assertGetAttributeDir("foo", dir); + removeFooAttrDir(10); + assertNotGetAttributeDir("foo"); } -TEST_F("Test that we can create attribute directory with one snapshot", Fixture) +TEST_F(AttributeDirectoryTest, can_create_attribute_directory_with_one_snapshot) { - TEST_DO(f.assertNotGetAttributeDir("foo")); - auto dir = f.createFooAttrDir(); + assertNotGetAttributeDir("foo"); + auto dir = createFooAttrDir(); EXPECT_TRUE(hasAttributeDir(dir)); - TEST_DO(f.assertNotAttributeDiskDir("foo")); + assertNotAttributeDiskDir("foo"); dir->getWriter()->createInvalidSnapshot(1); - TEST_DO(f.assertAttributeDiskDir("foo")); - TEST_DO(f.assertSnapshots("foo", "i1")); + assertAttributeDiskDir("foo"); + assertSnapshots("foo", "i1"); } -TEST_F("Test that we can prune attribute snapshots", Fixture) +TEST_F(AttributeDirectoryTest, can_prune_attribute_snapshots) { - auto dir = f.createFooAttrDir(); - TEST_DO(f.assertNotAttributeDiskDir("foo")); + auto dir = createFooAttrDir(); + assertNotAttributeDiskDir("foo"); auto writer = dir->getWriter(); writer->createInvalidSnapshot(2); std::filesystem::create_directory(std::filesystem::path(writer->getSnapshotDir(2))); @@ -213,53 +214,53 @@ TEST_F("Test that we can prune attribute snapshots", Fixture) std::filesystem::create_directory(std::filesystem::path(writer->getSnapshotDir(4))); writer->markValidSnapshot(4); writer.reset(); - TEST_DO(f.assertAttributeDiskDir("foo")); - TEST_DO(f.assertSnapshots("foo", "v2,v4")); + assertAttributeDiskDir("foo"); + assertSnapshots("foo", "v2,v4"); dir->getWriter()->invalidateOldSnapshots(); - TEST_DO(f.assertSnapshots("foo", "i2,v4")); + assertSnapshots("foo", "i2,v4"); dir->getWriter()->removeInvalidSnapshots(); - TEST_DO(f.assertSnapshots("foo", "v4")); + assertSnapshots("foo", "v4"); } -TEST_F("Test that attribute directory is not removed if valid snapshots remain", Fixture) +TEST_F(AttributeDirectoryTest, attribute_directory_is_not_removed_if_valid_snapshots_remain) { - TEST_DO(f.setupFooSnapshots(20)); - auto dir = f.getFooAttrDir(); + setupFooSnapshots(20); + auto dir = getFooAttrDir(); EXPECT_TRUE(hasAttributeDir(dir)); dir->getWriter()->createInvalidSnapshot(30); - TEST_DO(f.assertSnapshots("foo", "v20,i30")); - TEST_DO(f.removeFooAttrDir(10)); - TEST_DO(f.assertGetAttributeDir("foo", dir)); - TEST_DO(f.assertAttributeDiskDir("foo")); - TEST_DO(f.assertSnapshots("foo", "v20")); + assertSnapshots("foo", "v20,i30"); + removeFooAttrDir(10); + assertGetAttributeDir("foo", dir); + assertAttributeDiskDir("foo"); + assertSnapshots("foo", "v20"); } -TEST_F("Test that attribute directory is removed if no valid snapshots remain", Fixture) +TEST_F(AttributeDirectoryTest, attribute_directory_is_removed_if_no_valid_snapshots_remain) { - TEST_DO(f.setupFooSnapshots(5)); - auto dir = f.getFooAttrDir(); + setupFooSnapshots(5); + auto dir = getFooAttrDir(); EXPECT_TRUE(hasAttributeDir(dir)); dir->getWriter()->createInvalidSnapshot(30); - TEST_DO(f.assertSnapshots("foo", "v5,i30")); - TEST_DO(f.removeFooAttrDir(10)); - TEST_DO(f.assertNotGetAttributeDir("foo")); + assertSnapshots("foo", "v5,i30"); + removeFooAttrDir(10); + assertNotGetAttributeDir("foo"); } -TEST_F("Test that attribute directory is not removed due to pruning and disk dir is kept", Fixture) +TEST_F(AttributeDirectoryTest, attribute_directory_is_not_removed_due_to_pruning_and_disk_dir_is_kept) { - TEST_DO(f.setupFooSnapshots(5)); - TEST_DO(f.invalidateFooSnapshots(false)); - TEST_DO(f.assertAttributeDiskDir("foo")); + setupFooSnapshots(5); + invalidateFooSnapshots(false); + assertAttributeDiskDir("foo"); } -TEST_F("Test that attribute directory is not removed due to pruning but disk dir is removed", Fixture) +TEST_F(AttributeDirectoryTest, attribute_directory_is_not_removed_due_to_pruning_but_disk_dir_is_removed) { - TEST_DO(f.setupFooSnapshots(5)); - TEST_DO(f.invalidateFooSnapshots(true)); - TEST_DO(f.assertNotAttributeDiskDir("foo")); + setupFooSnapshots(5); + invalidateFooSnapshots(true); + assertNotAttributeDiskDir("foo"); } -TEST("Test that initial state tracks disk layout") +TEST(BasicDirectoryTest, initial_state_tracks_disk_layout) { std::filesystem::create_directory(std::filesystem::path("attributes")); std::filesystem::create_directory(std::filesystem::path("attributes/foo")); @@ -272,38 +273,38 @@ TEST("Test that initial state tracks disk layout") barInfo.addSnapshot({false, 5, "snapshot-5"}); barInfo.save(); Fixture f; - TEST_DO(f.assertAttributeDiskDir("foo")); - TEST_DO(f.assertAttributeDiskDir("bar")); + f.assertAttributeDiskDir("foo"); + f.assertAttributeDiskDir("bar"); auto foodir = f.getFooAttrDir(); EXPECT_TRUE(hasAttributeDir(foodir)); auto bardir = f.getAttributeDir("bar"); EXPECT_TRUE(hasAttributeDir(bardir)); - TEST_DO(f.assertNotGetAttributeDir("baz")); - TEST_DO(f.assertSnapshots("foo", "v4,i8")); - TEST_DO(f.assertSnapshots("bar", "i5")); + f.assertNotGetAttributeDir("baz"); + f.assertSnapshots("foo", "v4,i8"); + f.assertSnapshots("bar", "i5"); f.makeInvalidSnapshot(12); f.makeValidSnapshot(16); - TEST_DO(f.assertSnapshots("foo", "v4,i8,i12,v16")); + f.assertSnapshots("foo", "v4,i8,i12,v16"); } -TEST_F("Test that snapshot removal removes correct snapshot directory", Fixture) +TEST_F(AttributeDirectoryTest, snapshot_removal_removes_correct_snapshot_directory) { - TEST_DO(f.setupFooSnapshots(5)); - std::filesystem::create_directory(std::filesystem::path(f.getSnapshotDir("foo", 5))); - std::filesystem::create_directory(std::filesystem::path(f.getSnapshotDir("foo", 6))); - TEST_DO(f.assertSnapshotDir("foo", 5)); - TEST_DO(f.assertSnapshotDir("foo", 6)); - TEST_DO(f.invalidateFooSnapshots(false)); - TEST_DO(f.assertNotSnapshotDir("foo", 5)); - TEST_DO(f.assertSnapshotDir("foo", 6)); - TEST_DO(f.invalidateFooSnapshots(true)); - TEST_DO(f.assertNotSnapshotDir("foo", 5)); - TEST_DO(f.assertNotSnapshotDir("foo", 6)); + setupFooSnapshots(5); + std::filesystem::create_directory(std::filesystem::path(getSnapshotDir("foo", 5))); + std::filesystem::create_directory(std::filesystem::path(getSnapshotDir("foo", 6))); + assertSnapshotDir("foo", 5); + assertSnapshotDir("foo", 6); + invalidateFooSnapshots(false); + assertNotSnapshotDir("foo", 5); + assertSnapshotDir("foo", 6); + invalidateFooSnapshots(true); + assertNotSnapshotDir("foo", 5); + assertNotSnapshotDir("foo", 6); } -TEST_F("Test that we can get nonblocking writer", Fixture) +TEST_F(AttributeDirectoryTest, can_get_nonblocking_writer) { - auto dir = f.createFooAttrDir(); + auto dir = createFooAttrDir(); auto writer = dir->getWriter(); EXPECT_TRUE(hasWriter(writer)); auto writer2 = dir->tryGetWriter(); @@ -317,7 +318,4 @@ TEST_F("Test that we can get nonblocking writer", Fixture) } -TEST_MAIN() -{ - TEST_RUN_ALL(); -} +GTEST_MAIN_RUN_ALL_TESTS() |