diff options
12 files changed, 44 insertions, 91 deletions
diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/dns/AliasTarget.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/dns/AliasTarget.java index 7ccbcf2a954..9a33113728d 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/dns/AliasTarget.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/dns/AliasTarget.java @@ -58,11 +58,11 @@ public sealed abstract class AliasTarget permits LatencyAliasTarget, WeightedAli /** Unpack target from given record data */ public static AliasTarget unpack(RecordData data) { String[] parts = data.asString().split("/"); - switch (parts[0]) { - case LatencyAliasTarget.TARGET_TYPE: return LatencyAliasTarget.unpack(data); - case WeightedAliasTarget.TARGET_TYPE: return WeightedAliasTarget.unpack(data); - } - throw new IllegalArgumentException("Unknown alias type '" + parts[0] + "'"); + return switch (parts[0]) { + case LatencyAliasTarget.TARGET_TYPE -> LatencyAliasTarget.unpack(data); + case WeightedAliasTarget.TARGET_TYPE -> WeightedAliasTarget.unpack(data); + default -> throw new IllegalArgumentException("Unknown alias type '" + parts[0] + "'"); + }; } } diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/dns/LatencyAliasTarget.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/dns/LatencyAliasTarget.java index 00e5218dead..b33a5e942fa 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/dns/LatencyAliasTarget.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/dns/LatencyAliasTarget.java @@ -48,7 +48,7 @@ public final class LatencyAliasTarget extends AliasTarget { @Override public String toString() { - return "latency target for " + name() + "[id=" + id() + ",dnsZone=" + dnsZone() + "]"; + return "latency target for " + name() + " [id=" + id() + ",dnsZone=" + dnsZone() + "]"; } /** Unpack latency alias from given record data */ diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/dns/MemoryNameService.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/dns/MemoryNameService.java index 0ca4ae66fb3..4ad54ea14b0 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/dns/MemoryNameService.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/dns/MemoryNameService.java @@ -7,9 +7,7 @@ import java.util.Comparator; import java.util.List; import java.util.Optional; import java.util.Set; -import java.util.TreeSet; import java.util.concurrent.ConcurrentSkipListSet; -import java.util.stream.Collectors; /** * An in-memory name service for testing purposes. @@ -85,28 +83,6 @@ public class MemoryNameService implements NameService { } @Override - public List<Record> findRecords(Record.Type type, RecordData data) { - if (type == Record.Type.ALIAS && data.asString().contains("/")) { - // Validate the same expectation as of a real name service - throw new IllegalArgumentException("Finding " + Record.Type.ALIAS + " record by data should only include " + - "the FQDN name"); - } - return records.stream() - .filter(record -> { - if (record.type() == type) { - if (type == Record.Type.ALIAS) { - // Unpack ALIAS record and compare FQDN of data part - return RecordData.fqdn(AliasTarget.unpack(record.data()).name().value()) - .equals(data); - } - return record.data().equals(data); - } - return false; - }) - .toList(); - } - - @Override public void updateRecord(Record record, RecordData newData) { var records = findRecords(record.type(), record.name()); if (records.isEmpty()) { diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/dns/NameService.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/dns/NameService.java index 72e983680d9..a0612fda038 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/dns/NameService.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/dns/NameService.java @@ -51,9 +51,6 @@ public interface NameService { /** Find all records matching given type and name */ List<Record> findRecords(Record.Type type, RecordName name); - /** Find all records matching given type and data */ - List<Record> findRecords(Record.Type type, RecordData data); - /** Update existing record */ void updateRecord(Record record, RecordData newData); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/RoutingController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/RoutingController.java index 4ccebfa11e0..120d3e7e762 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/RoutingController.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/RoutingController.java @@ -56,7 +56,6 @@ import java.util.Set; import java.util.TreeMap; import java.util.stream.Collectors; -import static java.util.stream.Collectors.groupingBy; import static java.util.stream.Collectors.toMap; /** @@ -145,7 +144,7 @@ public class RoutingController { .map(zone -> new DeploymentId(instance, ZoneId.from(Environment.prod, zone.region().get()))) .toList(); RoutingId routingId = RoutingId.of(instance, EndpointId.defaultId()); - endpoints.addAll(computeGlobalEndpoints(routingId, ClusterSpec.Id.from(clusterId), deployments, deploymentSpec)); + endpoints.addAll(computeGlobalEndpoints(routingId, ClusterSpec.Id.from(clusterId), deployments)); }); // Add endpoints declared with current syntax spec.endpoints().forEach(declaredEndpoint -> { @@ -154,7 +153,7 @@ public class RoutingController { .map(region -> new DeploymentId(instance, ZoneId.from(Environment.prod, region))) .toList(); - endpoints.addAll(computeGlobalEndpoints(routingId, ClusterSpec.Id.from(declaredEndpoint.containerId()), deployments, deploymentSpec)); + endpoints.addAll(computeGlobalEndpoints(routingId, ClusterSpec.Id.from(declaredEndpoint.containerId()), deployments)); }); } // Add application endpoints @@ -347,7 +346,7 @@ public class RoutingController { .map(region -> new DeploymentId(instance.id(), ZoneId.from(Environment.prod, region))) .toList(); endpointsToRemove.addAll(computeGlobalEndpoints(RoutingId.of(instance.id(), rotation.endpointId()), - rotation.clusterId(), deployments, application.deploymentSpec())); + rotation.clusterId(), deployments)); } endpointsToRemove.forEach(endpoint -> controller.nameServiceForwarder() .removeRecords(Record.Type.CNAME, @@ -392,7 +391,7 @@ public class RoutingController { } /** Compute global endpoints for given routing ID, application and deployments */ - private List<Endpoint> computeGlobalEndpoints(RoutingId routingId, ClusterSpec.Id cluster, List<DeploymentId> deployments, DeploymentSpec deploymentSpec) { + private List<Endpoint> computeGlobalEndpoints(RoutingId routingId, ClusterSpec.Id cluster, List<DeploymentId> deployments) { var endpoints = new ArrayList<Endpoint>(); var directMethods = 0; var availableRoutingMethods = routingMethodsOfAll(deployments); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/dns/NameServiceForwarder.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/dns/NameServiceForwarder.java index df0d8877e07..9885740b2b5 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/dns/NameServiceForwarder.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/dns/NameServiceForwarder.java @@ -16,7 +16,6 @@ import java.util.Objects; import java.util.Set; import java.util.logging.Level; import java.util.logging.Logger; -import java.util.stream.Collectors; /** * This adds name service requests to the {@link NameServiceQueue}. @@ -77,11 +76,6 @@ public class NameServiceForwarder { forward(new RemoveRecords(type, name), priority); } - /** Remove all records of given type and data */ - public void removeRecords(Record.Type type, RecordData data, NameServiceQueue.Priority priority) { - forward(new RemoveRecords(type, data), priority); - } - /** Remove all records of given type, name and data */ public void removeRecords(Record.Type type, RecordName name, RecordData data, NameServiceQueue.Priority priority) { forward(new RemoveRecords(type, name, data), priority); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/dns/RemoveRecords.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/dns/RemoveRecords.java index 6fa7473ad1e..24841c53c3c 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/dns/RemoveRecords.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/dns/RemoveRecords.java @@ -8,7 +8,6 @@ import com.yahoo.vespa.hosted.controller.api.integration.dns.Record; import com.yahoo.vespa.hosted.controller.api.integration.dns.RecordData; import com.yahoo.vespa.hosted.controller.api.integration.dns.RecordName; -import java.util.ArrayList; import java.util.List; import java.util.Objects; import java.util.Optional; @@ -18,36 +17,28 @@ import java.util.Optional; * * - name and data * - only name - * - only data * * @author mpolden */ public class RemoveRecords implements NameServiceRequest { private final Record.Type type; - private final Optional<RecordName> name; + private final RecordName name; private final Optional<RecordData> data; public RemoveRecords(Record.Type type, RecordName name) { - this(type, Optional.of(name), Optional.empty()); - } - - public RemoveRecords(Record.Type type, RecordData data) { - this(type, Optional.empty(), Optional.of(data)); + this(type, name, Optional.empty()); } public RemoveRecords(Record.Type type, RecordName name, RecordData data) { - this(type, Optional.of(name), Optional.of(data)); + this(type, name, Optional.of(data)); } /** DO NOT USE. Public for serialization purposes */ - public RemoveRecords(Record.Type type, Optional<RecordName> name, Optional<RecordData> data) { + public RemoveRecords(Record.Type type, RecordName name, Optional<RecordData> data) { this.type = Objects.requireNonNull(type, "type must be non-null"); this.name = Objects.requireNonNull(name, "name must be non-null"); this.data = Objects.requireNonNull(data, "data must be non-null"); - if (name.isEmpty() && data.isEmpty()) { - throw new IllegalArgumentException("at least one of name and data must be non-empty"); - } } public Record.Type type() { @@ -55,7 +46,7 @@ public class RemoveRecords implements NameServiceRequest { } public Optional<RecordName> name() { - return name; + return Optional.of(name); } public Optional<RecordData> data() { @@ -64,32 +55,18 @@ public class RemoveRecords implements NameServiceRequest { @Override public void dispatchTo(NameService nameService) { - List<Record> records = new ArrayList<>(); - if (name.isPresent() && data.isPresent()) { - nameService.findRecords(type, name.get()) - .stream() - .filter(record -> { - // Records to remove must match both name and data fields - String dataValue = switch (record.type()) { - case ALIAS -> AliasTarget.unpack(record.data()).name().value(); - case DIRECT -> DirectTarget.unpack(record.data()).recordData().asString(); - default -> record.data().asString(); - }; - return fqdn(dataValue).equals(fqdn(data.get().asString())); - }) - .forEach(records::add); - } else { - name.ifPresent(n -> records.addAll(nameService.findRecords(type, n))); - data.ifPresent(d -> records.addAll(nameService.findRecords(type, d))); - } - nameService.removeRecords(records); + // Deletions require all records fields to match exactly, data may be incomplete even if present. To ensure + // completeness we search for the record(s) first + List<Record> completeRecords = nameService.findRecords(type, name).stream() + .filter(record -> data.isEmpty() || matchingFqdnIn(data.get(), record)) + .toList(); + nameService.removeRecords(completeRecords); } @Override public String toString() { - return "remove records of type " + type + ", by " + - name.map(n -> "name " + n).orElse("") + - data.map(d -> "data " + d).orElse(""); + return "remove records of type " + type + ", by name " + name + + data.map(d -> " data " + d).orElse(""); } @Override @@ -107,6 +84,15 @@ public class RemoveRecords implements NameServiceRequest { return Objects.hash(type, name, data); } + private static boolean matchingFqdnIn(RecordData data, Record record) { + String dataValue = switch (record.type()) { + case ALIAS -> AliasTarget.unpack(record.data()).name().value(); + case DIRECT -> DirectTarget.unpack(record.data()).recordData().asString(); + default -> record.data().asString(); + }; + return fqdn(dataValue).equals(fqdn(data.asString())); + } + private static String fqdn(String name) { return name.endsWith(".") ? name : name + "."; } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/NameServiceQueueSerializer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/NameServiceQueueSerializer.java index 3fd2a262e2e..05b69054151 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/NameServiceQueueSerializer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/NameServiceQueueSerializer.java @@ -108,7 +108,7 @@ public class NameServiceQueueSerializer { private RemoveRecords removeRecordsFromSlime(Inspector object) { var type = Record.Type.valueOf(object.field(typeField).asString()); - var name = SlimeUtils.optionalString(object.field(nameField)).map(RecordName::from); + var name = RecordName.from(object.field(nameField).asString()); var data = SlimeUtils.optionalString(object.field(dataField)).map(RecordData::from); return new RemoveRecords(type, name, data); } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java index 25ad7da98e7..a116396b224 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java @@ -210,7 +210,8 @@ public class RoutingPolicies { controller.nameServiceForwarder().createAlias(RecordName.from(endpoint.dnsName()), latencyTargets, Priority.normal); inactiveLatencyTargets.forEach(t -> controller.nameServiceForwarder() .removeRecords(Record.Type.ALIAS, - RecordData.fqdn(t.name().value()), + RecordName.from(endpoint.dnsName()), + RecordData.from(t.name().value()), Priority.normal)); } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/dns/NameServiceQueueTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/dns/NameServiceQueueTest.java index efb04615125..fcc0e9c9e6c 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/dns/NameServiceQueueTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/dns/NameServiceQueueTest.java @@ -39,8 +39,8 @@ public class NameServiceQueueTest { var req2 = new CreateRecords(List.of(r2)); var req3 = new CreateRecords(r3); var req4 = new RemoveRecords(r3.get(0).type(), r3.get(0).name()); - var req5 = new RemoveRecords(r2.type(), r2.data()); - var req6 = new RemoveRecords(Record.Type.CNAME, r1.data()); + var req5 = new RemoveRecords(r2.type(), r2.name(), r2.data()); + var req6 = new RemoveRecords(Record.Type.CNAME, r1.name(), r1.data()); // Add requests with different priorities and dispatch first one var queue = NameServiceQueue.EMPTY.with(req2).with(req1, Priority.high); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/NameServiceQueueSerializerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/NameServiceQueueSerializerTest.java index c42d5621a46..abab802576e 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/NameServiceQueueSerializerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/NameServiceQueueSerializerTest.java @@ -15,6 +15,7 @@ import com.yahoo.vespa.hosted.controller.dns.RemoveRecords; import org.junit.jupiter.api.Test; import java.util.List; +import java.util.Optional; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -50,7 +51,7 @@ public class NameServiceQueueSerializerTest { ZoneId.from("prod", "us-north-1"), 100).pack()))), new RemoveRecords(record1.type(), record1.name()), - new RemoveRecords(record2.type(), record2.data()) + new RemoveRecords(record2.type(), record2.name(), Optional.of(record2.data())) ); var queue = new NameServiceQueue(requests); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java index b553c8eb4c9..9c1253c7520 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java @@ -58,7 +58,6 @@ import java.util.stream.Collectors; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.junit.jupiter.api.Assertions.fail; /** * @author mortent @@ -1070,13 +1069,13 @@ public class RoutingPoliciesTest { deploymentsByDnsName.computeIfAbsent(dnsName, (k) -> new ArrayList<>()) .add(deployment); } - assertTrue(1 <= deploymentsByDnsName.size(), "Found " + endpointId + " for " + application); + assertTrue(deploymentsByDnsName.size() >= 1, "Found " + endpointId + " for " + application); deploymentsByDnsName.forEach((dnsName, deployments) -> { Set<String> weightedTargets = deployments.stream() - .map(d -> "weighted/lb-" + loadBalancerId + "--" + - d.applicationId().toFullString() + "--" + d.zoneId().value() + - "/dns-zone-1/" + d.zoneId().value() + "/" + deploymentWeights.get(d)) - .collect(Collectors.toSet()); + .map(d -> "weighted/lb-" + loadBalancerId + "--" + + d.applicationId().toFullString() + "--" + d.zoneId().value() + + "/dns-zone-1/" + d.zoneId().value() + "/" + deploymentWeights.get(d)) + .collect(Collectors.toSet()); assertEquals(weightedTargets, aliasDataOf(dnsName), dnsName + " has expected targets"); }); } |