Merge pull request #11837 from vespa-engine/bratseth/dont-allow-wrong-serviceport-take-2

Bratseth/dont allow wrong serviceport take 2

11 files changed, 144 insertions, 84 deletions

diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/http/Binding.java b/config-model/src/main/java/com/yahoo/vespa/model/container/http/Binding.java
index 09ac5f580ed..28f4949f210 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/http/Binding.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/http/Binding.java
@@ -10,6 +10,7 @@ import java.util.logging.Level;
  * @author bjorncs
  */
 public class Binding {
+
     private final ComponentSpecification filterId;
     private final String binding;
 
@@ -20,10 +21,9 @@ public class Binding {
 
     public static Binding create(ComponentSpecification filterId, String binding, DeployLogger logger) {
         if (binding.startsWith("https://")) {
-            logger.log(Level.WARNING, String.format(
-                    "For binding '%s' on '%s': 'https' bindings are deprecated, " +
-                            "use 'http' instead to bind to both http and https traffic.",
-                    binding, filterId));
+            logger.log(Level.WARNING, String.format("For binding '%s' on '%s': 'https' bindings are deprecated, " +
+                                                    "use 'http' instead to bind to both http and https traffic.",
+                                                    binding, filterId));
         }
         return new Binding(filterId, binding);
     }
@@ -35,4 +35,5 @@ public class Binding {
     public String binding() {
         return binding;
     }
+
 }
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/http/Http.java b/config-model/src/main/java/com/yahoo/vespa/model/container/http/Http.java
index 94694d9f737..400ddf80cf9 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/http/Http.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/http/Http.java
@@ -13,7 +13,7 @@ import java.util.List;
 import java.util.Optional;
 
 /**
- * Represents the http servers and filters of a Jdisc cluster.
+ * Represents the http servers and filters of a container cluster.
  *
  * @author Tony Vaagenes
  */
@@ -81,11 +81,10 @@ public class Http extends AbstractConfigProducer<AbstractConfigProducer<?>> impl
 
     @Override
     public void getConfig(ServerConfig.Builder builder) {
-        for (final Binding binding : bindings) {
-            builder.filter(
-                    new ServerConfig.Filter.Builder()
-                            .id(binding.filterId().stringValue())
-                            .binding(binding.binding()));
+        for (Binding binding : bindings) {
+            builder.filter(new ServerConfig.Filter.Builder()
+                                   .id(binding.filterId().stringValue())
+                                   .binding(binding.binding()));
         }
     }
 
@@ -95,17 +94,17 @@ public class Http extends AbstractConfigProducer<AbstractConfigProducer<?>> impl
     }
 
     void validate(Collection<Binding> bindings) {
-        if (!bindings.isEmpty()) {
-            if (filterChains == null)
-                throw new IllegalArgumentException("Null FilterChains is not allowed when there are filter bindings!");
+        if (bindings.isEmpty()) return;
 
-            ComponentRegistry<ChainedComponent<?>> filters = filterChains.componentsRegistry();
-            ComponentRegistry<Chain<Filter>> chains = filterChains.allChains();
+        if (filterChains == null)
+            throw new IllegalArgumentException("Null FilterChains are not allowed when there are filter bindings");
 
-            for (Binding binding: bindings) {
-                if (filters.getComponent(binding.filterId()) == null && chains.getComponent(binding.filterId()) == null)
-                    throw new RuntimeException("Can't find filter " + binding.filterId() + " for binding " + binding.binding());
-            }
+        ComponentRegistry<ChainedComponent<?>> filters = filterChains.componentsRegistry();
+        ComponentRegistry<Chain<Filter>> chains = filterChains.allChains();
+
+        for (Binding binding: bindings) {
+            if (filters.getComponent(binding.filterId()) == null && chains.getComponent(binding.filterId()) == null)
+                throw new RuntimeException("Can't find filter " + binding.filterId() + " for binding " + binding.binding());
         }
     }
 
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/HttpBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/HttpBuilder.java
index 8cf430741f0..b37caf22216 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/HttpBuilder.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/HttpBuilder.java
@@ -8,8 +8,9 @@ import com.yahoo.config.model.deploy.DeployState;
 import com.yahoo.config.model.producer.AbstractConfigProducer;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.ApplicationName;
-import com.yahoo.log.LogLevel;
 import com.yahoo.text.XML;
+import com.yahoo.vespa.defaults.Defaults;
+import com.yahoo.vespa.model.builder.xml.dom.ModelElement;
 import com.yahoo.vespa.model.builder.xml.dom.VespaDomBuilder;
 import com.yahoo.vespa.model.container.Container;
 import com.yahoo.vespa.model.container.ApplicationContainerCluster;
@@ -23,6 +24,7 @@ import org.w3c.dom.Element;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Optional;
+import java.util.logging.Level;
 
 import static com.yahoo.vespa.model.container.http.AccessControl.ACCESS_CONTROL_CHAIN_ID;
 
@@ -127,20 +129,24 @@ public class HttpBuilder extends VespaDomBuilder.DomConfigProducerBuilder<Http>
         http.setHttpServer(new JettyHttpServerBuilder().build(deployState, ancestor, spec));
     }
 
-    static int readPort(Element spec, boolean isHosted, DeployLogger deployLogger) {
-        String portString = spec.getAttribute("port");
+    static int readPort(ModelElement spec, boolean isHosted, DeployLogger logger) {
+        Integer port = spec.integerAttribute("port");
+        if (port == null)
+            return Defaults.getDefaults().vespaWebServicePort();
 
-        int port = Integer.parseInt(portString);
         if (port < 0)
-            throw new IllegalArgumentException(String.format("Invalid port %d.", port));
+            throw new IllegalArgumentException("Invalid port " + port);
 
         int legalPortInHostedVespa = Container.BASEPORT;
-        if (isHosted && port != legalPortInHostedVespa) {
-            deployLogger.log(LogLevel.WARNING,
-                    String.format("Trying to set port to %d for http server with id %s. You cannot set port to anything else than %s",
-                            port, spec.getAttribute("id"), legalPortInHostedVespa));
+        if (isHosted && port != legalPortInHostedVespa && ! spec.booleanAttribute("required", false)) {
+            // TODO: After January 2020:
+            // - Set required='true' for the http server on port 4443 in the tester services.xml in InternalStepRunner
+            // - Enable 2 currently ignored tests in this module
+            // - throw IllegalArgumentException here instead of warning
+            logger.log(Level.WARNING, "Illegal port " + port + " in http server '" +
+                                      spec.stringAttribute("id") + "'" +
+                                      ": Port must be set to " + legalPortInHostedVespa);
         }
-
         return port;
     }
 }
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/JettyConnectorBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/JettyConnectorBuilder.java
index 1b457b1250a..db831a1ec2f 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/JettyConnectorBuilder.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/JettyConnectorBuilder.java
@@ -5,6 +5,7 @@ import com.yahoo.config.model.builder.xml.XmlHelper;
 import com.yahoo.config.model.deploy.DeployState;
 import com.yahoo.config.model.producer.AbstractConfigProducer;
 import com.yahoo.text.XML;
+import com.yahoo.vespa.model.builder.xml.dom.ModelElement;
 import com.yahoo.vespa.model.builder.xml.dom.VespaDomBuilder;
 import com.yahoo.vespa.model.container.component.SimpleComponent;
 import com.yahoo.vespa.model.container.http.ConnectorFactory;
@@ -24,7 +25,7 @@ public class JettyConnectorBuilder extends VespaDomBuilder.DomConfigProducerBuil
     @Override
     protected ConnectorFactory doBuild(DeployState deployState, AbstractConfigProducer ancestor, Element serverSpec) {
         String name = XmlHelper.getIdString(serverSpec);
-        int port = HttpBuilder.readPort(serverSpec, deployState.isHosted(), deployState.getDeployLogger());
+        int port = HttpBuilder.readPort(new ModelElement(serverSpec), deployState.isHosted(), deployState.getDeployLogger());
 
         SimpleComponent sslProviderComponent = getSslConfigComponents(name, serverSpec);
         return new ConnectorFactory(name, port, sslProviderComponent);
@@ -53,4 +54,5 @@ public class JettyConnectorBuilder extends VespaDomBuilder.DomConfigProducerBuil
             return new DefaultSslProvider(serverName);
         }
     }
+
 }
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/JettyHttpServerBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/JettyHttpServerBuilder.java
index d5d1de37444..3f38b2b16fa 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/JettyHttpServerBuilder.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/JettyHttpServerBuilder.java
@@ -11,10 +11,10 @@ import com.yahoo.vespa.model.container.http.JettyHttpServer;
 import org.w3c.dom.Element;
 
 /**
- * @author <a href="mailto:einarmr@yahoo-inc.com">Einar M R Rosenvinge</a>
- * @since 5.17.0
+ * @author Einar M R Rosenvinge
  */
 public class JettyHttpServerBuilder extends VespaDomBuilder.DomConfigProducerBuilder<JettyHttpServer> {
+
     @Override
     protected JettyHttpServer doBuild(DeployState deployState, AbstractConfigProducer ancestor, Element http) {
         JettyHttpServer jettyHttpServer = new JettyHttpServer(new ComponentId("jdisc-jetty"));
@@ -25,4 +25,5 @@ public class JettyHttpServerBuilder extends VespaDomBuilder.DomConfigProducerBui
 
         return jettyHttpServer;
     }
+
 }
diff --git a/config-model/src/main/resources/schema/containercluster.rnc b/config-model/src/main/resources/schema/containercluster.rnc
index 142abb5c63b..6e4346d96ee 100644
--- a/config-model/src/main/resources/schema/containercluster.rnc
+++ b/config-model/src/main/resources/schema/containercluster.rnc
@@ -64,7 +64,8 @@ Filtering = element filtering {
 }
 
 HttpServer =  element server {
-    attribute port { xsd:nonNegativeInteger } &
+    attribute port { xsd:nonNegativeInteger }? &
+    attribute required { xsd:boolean }? &
     ComponentId &
     (Ssl | SslProvider)? &
     GenericConfig*
diff --git a/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java b/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java
index 6461ae1aaa4..7b4b650295c 100644
--- a/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java
+++ b/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java
@@ -1297,6 +1297,7 @@ public class ModelProvisioningTest {
         assertEquals(1, model.getRoot().hostSystem().getHosts().size());
         assertEquals(1, model.getAdmin().getSlobroks().size());
     }
+
     @Test
     public void testThatStandaloneSyntaxWorksOnHostedVespa() {
         String services =
@@ -1314,6 +1315,29 @@ public class ModelProvisioningTest {
     }
 
     @Test
+    @Ignore // TODO: Enable when turning the port check on
+    public void testThatStandaloneSyntaxOnHostedVespaRequiresDefaultPort() {
+        try {
+            String services =
+                    "<?xml version='1.0' encoding='utf-8' ?>" +
+                    "<container id='foo' version='1.0'>" +
+                    "  <http>" +
+                    "    <server id='server1' port='8095' />" +
+                    "  </http>" +
+                    "</container>";
+            VespaModelTester tester = new VespaModelTester();
+            tester.addHosts(1);
+            VespaModel model = tester.createModel(services, true);
+            fail("Expected exception");
+        }
+        catch (IllegalArgumentException e) {
+            // Success
+            assertEquals("Illegal port 8095 in http server 'server1': Port must be set to " +
+                         getDefaults().vespaWebServicePort(), e.getMessage());
+        }
+    }
+
+    @Test
     public void testThatStandaloneSyntaxWorksOnHostedManuallyDeployed() {
         String services =
                 "<?xml version='1.0' encoding='utf-8' ?>" +
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/search/ImplicitIndexingClusterTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/search/ImplicitIndexingClusterTest.java
index 30a36715a17..c7118618003 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/container/search/ImplicitIndexingClusterTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/container/search/ImplicitIndexingClusterTest.java
@@ -6,6 +6,7 @@ import com.yahoo.config.model.deploy.DeployState;
 import com.yahoo.config.model.deploy.TestProperties;
 import com.yahoo.config.model.provision.InMemoryProvisioner;
 import com.yahoo.config.model.test.MockApplicationPackage;
+import com.yahoo.vespa.defaults.Defaults;
 import com.yahoo.vespa.model.VespaModel;
 import com.yahoo.vespa.model.container.ContainerCluster;
 import com.yahoo.vespa.model.test.utils.VespaModelCreatorWithMockPkg;
@@ -18,6 +19,7 @@ import static org.junit.Assert.assertNotNull;
  * @author ollivir
  */
 public class ImplicitIndexingClusterTest {
+
     @Test
     public void existing_jdisc_is_used_as_indexing_cluster_when_multitenant() {
         final String servicesXml = "<services version=\"1.0\">\n" + //
@@ -25,7 +27,7 @@ public class ImplicitIndexingClusterTest {
                 "    <search />\n" + //
                 "    <nodes count=\"1\" />\n" + //
                 "    <http>\n" + //
-                "      <server id=\"bar\" port=\"4080\" />\n" + //
+                "      <server id=\"bar\" port=\"" + Defaults.getDefaults().vespaWebServicePort() + "\" />\n" + //
                 "    </http>\n" + //
                 "  </container>\n" + //
                 "  <content id=\"music\" version=\"1.0\">\n" + //
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilderTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilderTest.java
index 5a1befd3eb0..54d1c1c9793 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilderTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilderTest.java
@@ -35,6 +35,7 @@ import com.yahoo.net.HostName;
 import com.yahoo.path.Path;
 import com.yahoo.prelude.cluster.QrMonitorConfig;
 import com.yahoo.search.config.QrStartConfig;
+import com.yahoo.vespa.defaults.Defaults;
 import com.yahoo.vespa.model.AbstractService;
 import com.yahoo.vespa.model.VespaModel;
 import com.yahoo.vespa.model.container.ApplicationContainer;
@@ -46,6 +47,7 @@ import com.yahoo.vespa.model.container.http.ConnectorFactory;
 import com.yahoo.vespa.model.content.utils.ContentClusterUtils;
 import com.yahoo.vespa.model.test.utils.VespaModelCreatorWithFilePkg;
 import org.hamcrest.Matchers;
+import org.junit.Ignore;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
@@ -133,29 +135,50 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase {
     }
 
     @Test
-    public void fail_if_http_port_is_not_4080_in_hosted_vespa() throws Exception {
-        String servicesXml =
-                "<services>" +
-                "<admin version='3.0'>" +
-                "    <nodes count='1'/>" +
-                "</admin>" +
-                "<container version='1.0'>" +
-                "  <http>" +
-                "    <server port='9000' id='foo' />" +
-                "  </http>" +
-                nodesXml +
-                "</container>" +
-                "</services>";
-        ApplicationPackage applicationPackage = new MockApplicationPackage.Builder().withServices(servicesXml).build();
-        // Need to create VespaModel to make deploy properties have effect
-        final TestLogger logger = new TestLogger();
-        new VespaModel(new NullConfigModelRegistry(), new DeployState.Builder()
-                .applicationPackage(applicationPackage)
-                .deployLogger(logger)
-                .properties(new TestProperties().setHostedVespa(true))
-                .build());
-        assertFalse(logger.msgs.isEmpty());
-        assertThat(logger.msgs.get(0).getSecond(), containsString(String.format("You cannot set port to anything else than %d", Container.BASEPORT)));
+    public void omitting_http_server_port_gives_default() {
+        Element clusterElem = DomBuilderTest.parse(
+                "<container version='1.0'>",
+                "  <http>",
+                "    <server id='foo'/>",
+                "  </http>",
+                nodesXml,
+                "</container>" );
+        createModel(root, clusterElem);
+        AbstractService container = (AbstractService)root.getProducer("container/container.0");
+        assertEquals(Defaults.getDefaults().vespaWebServicePort(), container.getRelativePort(0));
+    }
+
+    @Test
+    @Ignore // TODO: Enable when turning the port check on
+    public void fail_if_http_port_is_not_default_in_hosted_vespa() throws Exception {
+        try {
+            String servicesXml =
+                    "<services>" +
+                    "<admin version='3.0'>" +
+                    "    <nodes count='1'/>" +
+                    "</admin>" +
+                    "<container version='1.0'>" +
+                    "  <http>" +
+                    "    <server port='9000' id='foo' />" +
+                    "  </http>" +
+                    nodesXml +
+                    "</container>" +
+                    "</services>";
+            ApplicationPackage applicationPackage = new MockApplicationPackage.Builder().withServices(servicesXml).build();
+            // Need to create VespaModel to make deploy properties have effect
+            TestLogger logger = new TestLogger();
+            new VespaModel(new NullConfigModelRegistry(), new DeployState.Builder()
+                                                                  .applicationPackage(applicationPackage)
+                                                                  .deployLogger(logger)
+                                                                  .properties(new TestProperties().setHostedVespa(true))
+                                                                  .build());
+            fail("Expected exception");
+        }
+        catch (IllegalArgumentException e) {
+            // Success
+            assertEquals("Illegal port 9000 in http server 'foo': Port must be set to " + Defaults.getDefaults().vespaWebServicePort(),
+                         e.getMessage());
+        }
     }
 
     @Test
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/JettyContainerModelBuilderTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/JettyContainerModelBuilderTest.java
index 929e520f984..863781073f8 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/JettyContainerModelBuilderTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/JettyContainerModelBuilderTest.java
@@ -234,7 +234,7 @@ public class JettyContainerModelBuilderTest extends ContainerModelBuilderTestBas
         Element clusterElem = DomBuilderTest.parse(
                 "<container id='default' version='1.0' jetty='true'>",
                 "    <http>",
-                "        <server port='9000' id='ssl'>",
+                "        <server port='8080' id='ssl'>",
                 "            <ssl>",
                 "                <private-key-file>/foo/key</private-key-file>",
                 "                <certificate-file>/foo/cert</certificate-file>",
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/test/ModelAmendingTestCase.java b/config-model/src/test/java/com/yahoo/vespa/model/test/ModelAmendingTestCase.java
index 8c4552ba117..da17044528f 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/test/ModelAmendingTestCase.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/test/ModelAmendingTestCase.java
@@ -11,6 +11,7 @@ import com.yahoo.config.model.admin.AdminModel;
 import com.yahoo.config.model.builder.xml.ConfigModelBuilder;
 import com.yahoo.config.model.builder.xml.ConfigModelId;
 import com.yahoo.config.model.producer.AbstractConfigProducer;
+import com.yahoo.vespa.defaults.Defaults;
 import com.yahoo.vespa.model.AbstractService;
 import com.yahoo.vespa.model.HostResource;
 import com.yahoo.vespa.model.PortAllocBridge;
@@ -46,32 +47,32 @@ public class ModelAmendingTestCase {
                                                                                       new ContainerModelAmenderBuilder(),
                                                                                       new ContentModelAmenderBuilder());
         String services =
-                                             "<services version='1.0'>" +
-                                             "    <admin version='4.0'/>" +
-                                             "    <container id='test1' version='1.0'>" +
-                                             "        <search/>" +
-                                             "        <nodes count='2'/>" +
-                                             "    </container>" +
-                                             "    <container id='test2' version='1.0'>" +
-                                             "        <http><server id='server1' port='19110'/></http>" +
-                                             "        <document-api/>" +
-                                             "        <nodes count='2'/>" +
-                                             "    </container>" +
-                                             "    <content id='test3' version='1.0'>" +
-                                             "        <redundancy>1</redundancy>" +
-                                             "        <documents>" +
-                                             "            <document mode='index' type='type1'/>" +
-                                             "        </documents>" +
-                                             "        <nodes count='2'/>" +
-                                             "    </content>" +
-                                             "    <content id='test4' version='1.0'>" +
-                                             "        <redundancy>1</redundancy>" +
-                                             "        <documents>" +
-                                             "            <document mode='index' type='type1'/>" +
-                                             "        </documents>" +
-                                             "        <nodes count='3'/>" +
-                                             "    </content>" +
-                                             "</services>";
+                "<services version='1.0'>" +
+                "    <admin version='4.0'/>" +
+                "    <container id='test1' version='1.0'>" +
+                "        <search/>" +
+                "        <nodes count='2'/>" +
+                "    </container>" +
+                "    <container id='test2' version='1.0'>" +
+                "        <http><server id='server1' port='" + Defaults.getDefaults().vespaWebServicePort() + "'/></http>" +
+                "        <document-api/>" +
+                "        <nodes count='2'/>" +
+                "    </container>" +
+                "    <content id='test3' version='1.0'>" +
+                "        <redundancy>1</redundancy>" +
+                "        <documents>" +
+                "            <document mode='index' type='type1'/>" +
+                "        </documents>" +
+                "        <nodes count='2'/>" +
+                "    </content>" +
+                "    <content id='test4' version='1.0'>" +
+                "        <redundancy>1</redundancy>" +
+                "        <documents>" +
+                "            <document mode='index' type='type1'/>" +
+                "        </documents>" +
+                "        <nodes count='3'/>" +
+                "    </content>" +
+                "</services>";
         VespaModelTester tester = new VespaModelTester(amendingModelRepo);
         tester.addHosts(10);
         VespaModel model = tester.createModel(services);