Merge pull request #12708 from vespa-engine/bjorncs/access-control

Bjorncs/access control

6 files changed, 116 insertions, 91 deletions

diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/Container.java b/config-model/src/main/java/com/yahoo/vespa/model/container/Container.java
index 7e2d6680827..764b42c0ea2 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/Container.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/Container.java
@@ -140,7 +140,7 @@ public abstract class Container extends AbstractService implements
         if (http == null) {
             return defaultHttpServer;
         } else {
-            return http.getHttpServer();
+            return http.getHttpServer().orElse(null);
         }
     }
 
@@ -228,10 +228,10 @@ public abstract class Container extends AbstractService implements
             // XXX unused - remove:
             from.allocatePort("http/1");
             portsMeta.on(offset++).tag("http").tag("external");
-        } else if (getHttp().getHttpServer() == null) {
+        } else if (getHttp().getHttpServer().isEmpty()) {
             // no http server ports
         } else {
-            for (ConnectorFactory connectorFactory : getHttp().getHttpServer().getConnectorFactories()) {
+            for (ConnectorFactory connectorFactory : getHttp().getHttpServer().get().getConnectorFactories()) {
                 int port = getPort(connectorFactory);
                 String name = "http/" + connectorFactory.getName();
                 from.requirePort(port, name);
@@ -280,7 +280,7 @@ public abstract class Container extends AbstractService implements
         final Http http = getHttp();
         if (http != null) {
             // TODO: allow the user to specify health port manually
-            if (http.getHttpServer() == null) {
+            if (http.getHttpServer().isEmpty()) {
                 return -1;
             } else {
                 return getRelativePort(0);
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/configserver/ConfigserverCluster.java b/config-model/src/main/java/com/yahoo/vespa/model/container/configserver/ConfigserverCluster.java
index f3758def2b1..9b8d138a98a 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/configserver/ConfigserverCluster.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/configserver/ConfigserverCluster.java
@@ -116,8 +116,8 @@ public class ConfigserverCluster extends AbstractConfigProducer
         }
 
         builder.serverId(HostName.getLocalhost());
-        if (!containerCluster.getHttp().getHttpServer().getConnectorFactories().isEmpty()) {
-            builder.httpport(containerCluster.getHttp().getHttpServer().getConnectorFactories().get(0).getListenPort());
+        if (!containerCluster.getHttp().getHttpServer().get().getConnectorFactories().isEmpty()) {
+            builder.httpport(containerCluster.getHttp().getHttpServer().get().getConnectorFactories().get(0).getListenPort());
         }
         if (options.useVespaVersionInRequest().isPresent()) {
             builder.useVespaVersionInRequest(options.useVespaVersionInRequest().get());
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 400ddf80cf9..4742ebfcf89 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
@@ -8,104 +8,112 @@ import com.yahoo.vespa.model.container.component.chain.Chain;
 import com.yahoo.vespa.model.container.component.chain.ChainedComponent;
 
 import java.util.Collection;
-import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
+import java.util.concurrent.CopyOnWriteArrayList;
 
 /**
  * Represents the http servers and filters of a container cluster.
  *
  * @author Tony Vaagenes
+ * @author bjorncs
  */
 public class Http extends AbstractConfigProducer<AbstractConfigProducer<?>> implements ServerConfig.Producer {
 
-    private FilterChains filterChains;
-    private JettyHttpServer httpServer;
-    private List<Binding> bindings;
-    private final Optional<AccessControl> accessControl;
-
-    public Http(List<Binding> bindings) {
-        this(bindings, null);
-    }
+    private final Object monitor = new Object();
 
-    public Http(List<Binding> bindings, AccessControl accessControl) {
-        super( "http");
-        this.bindings = Collections.unmodifiableList(bindings);
-        this.accessControl = Optional.ofNullable(accessControl);
-    }
+    private final FilterChains filterChains;
+    private final List<Binding> bindings = new CopyOnWriteArrayList<>();
+    private JettyHttpServer httpServer;
+    private AccessControl accessControl;
 
-    public void setFilterChains(FilterChains filterChains) {
-        this.filterChains = filterChains;
+    public Http(FilterChains chains) {
+        super("http");
+        this.filterChains = chains;
     }
 
-    public void setBindings(List<Binding> bindings) {
-        this.bindings = Collections.unmodifiableList(bindings);
+    public void setAccessControl(AccessControl accessControl) {
+        synchronized (monitor) {
+            if (this.accessControl != null) throw new IllegalStateException("Access control already assigned");
+            this.accessControl = accessControl;
+        }
     }
 
     public FilterChains getFilterChains() {
-        return filterChains;
+        synchronized (monitor) {
+            return filterChains;
+        }
     }
 
-    public JettyHttpServer getHttpServer() {
-        return httpServer;
+    public Optional<JettyHttpServer> getHttpServer() {
+        synchronized (monitor) {
+            return Optional.ofNullable(httpServer);
+        }
     }
 
     public void setHttpServer(JettyHttpServer newServer) {
-        JettyHttpServer oldServer = this.httpServer;
-        this.httpServer = newServer;
-
-        if (oldServer == null && newServer != null) {
-            addChild(newServer);
-        } else if (newServer == null && oldServer != null) {
-            removeChild(oldServer);
-        } else if (newServer == null && oldServer == null) {
-            //do nothing
-        } else {
-            //none of them are null
-            removeChild(oldServer);
-            addChild(newServer);
+        synchronized (monitor) {
+            JettyHttpServer oldServer = this.httpServer;
+            this.httpServer = newServer;
+
+            if (oldServer == null && newServer != null) {
+                addChild(newServer);
+            } else if (newServer == null && oldServer != null) {
+                removeChild(oldServer);
+            } else if (newServer == null && oldServer == null) {
+                //do nothing
+            } else {
+                //none of them are null
+                removeChild(oldServer);
+                addChild(newServer);
+            }
         }
     }
 
     public void removeAllServers() {
-        setHttpServer(null);
+        synchronized (monitor) {
+            setHttpServer(null);
+        }
     }
 
     public List<Binding> getBindings() {
-        return bindings;
+        synchronized (monitor) {
+            return bindings;
+        }
     }
 
     public Optional<AccessControl> getAccessControl() {
-        return accessControl;
+        synchronized (monitor) {
+            return Optional.ofNullable(accessControl);
+        }
     }
 
     @Override
     public void getConfig(ServerConfig.Builder builder) {
-        for (Binding binding : bindings) {
-            builder.filter(new ServerConfig.Filter.Builder()
-                                   .id(binding.filterId().stringValue())
-                                   .binding(binding.binding()));
+        synchronized (monitor) {
+            for (Binding binding : bindings) {
+                builder.filter(new ServerConfig.Filter.Builder()
+                                       .id(binding.filterId().stringValue())
+                                       .binding(binding.binding()));
+            }
         }
     }
 
     @Override
     public void validate() {
-        validate(bindings);
-    }
-
-    void validate(Collection<Binding> bindings) {
-        if (bindings.isEmpty()) return;
+        synchronized (monitor) {
+            if (((Collection<Binding>) bindings).isEmpty()) return;
 
-        if (filterChains == null)
-            throw new IllegalArgumentException("Null FilterChains are not allowed when there are filter bindings");
+            if (filterChains == null)
+                throw new IllegalArgumentException("Null FilterChains are not allowed when there are filter bindings");
 
-        ComponentRegistry<ChainedComponent<?>> filters = filterChains.componentsRegistry();
-        ComponentRegistry<Chain<Filter>> chains = filterChains.allChains();
+            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());
+            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 61a588fb716..bfde9b9add1 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
@@ -54,11 +54,10 @@ public class HttpBuilder extends VespaDomBuilder.DomConfigProducerBuilder<Http>
             filterChains = new FilterChainsBuilder().newChainsInstance(ancestor);
         }
 
-        Http http = new Http(bindings, accessControl);
-        http.setFilterChains(filterChains);
-
-        buildHttpServers(deployState, ancestor, http, spec);
-
+        Http http = new Http(filterChains);
+        http.getBindings().addAll(bindings);
+        http.setAccessControl(accessControl);
+        http.setHttpServer(new JettyHttpServerBuilder().build(deployState, ancestor, spec));
         return http;
     }
 
@@ -131,10 +130,6 @@ public class HttpBuilder extends VespaDomBuilder.DomConfigProducerBuilder<Http>
         return result;
     }
 
-    private void buildHttpServers(DeployState deployState, AbstractConfigProducer ancestor, Http http, Element spec) {
-        http.setHttpServer(new JettyHttpServerBuilder().build(deployState, ancestor, spec));
-    }
-
     static int readPort(ModelElement spec, boolean isHosted, DeployLogger logger) {
         Integer port = spec.integerAttribute("port");
         if (port == null)
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 c1f793e255d..cd292da6fa1 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
@@ -324,13 +324,14 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
             cluster.setHttp(buildHttp(deployState, cluster, httpElement));
         }
         if (isHostedTenantApplication(context)) {
-            addHostedImplicitHttpIfNotPresent(deployState, cluster);
+            addHostedImplicitHttpIfNotPresent(cluster);
+            addHostedImplicitAccessControlIfNotPresent(deployState, cluster);
             addAdditionalHostedConnector(deployState, cluster);
         }
     }
 
     private void addAdditionalHostedConnector(DeployState deployState, ApplicationContainerCluster cluster) {
-        JettyHttpServer server = cluster.getHttp().getHttpServer();
+        JettyHttpServer server = cluster.getHttp().getHttpServer().get();
         String serverName = server.getComponentId().getName();
 
         String proxyProtocol = deployState.getProperties().proxyProtocol();
@@ -356,39 +357,31 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
         return deployState.isHosted() && context.getApplicationType() == ApplicationType.DEFAULT && !isTesterApplication;
     }
 
-    private static void addHostedImplicitHttpIfNotPresent(DeployState deployState, ApplicationContainerCluster cluster) {
+    private static void addHostedImplicitHttpIfNotPresent(ApplicationContainerCluster cluster) {
         if(cluster.getHttp() == null) {
-            Http http = deployState.getProperties().athenzDomain()
-                    .map(tenantDomain -> createHostedImplicitHttpWithAccessControl(deployState, tenantDomain, cluster))
-                    .orElseGet(() -> createHostedImplicitHttpWithoutAccessControl(cluster));
-            cluster.setHttp(http);
+            cluster.setHttp(new Http(new FilterChains(cluster)));
         }
-        if(cluster.getHttp().getHttpServer() == null) {
+        if(cluster.getHttp().getHttpServer().isEmpty()) {
             JettyHttpServer defaultHttpServer = new JettyHttpServer(new ComponentId("DefaultHttpServer"));
             cluster.getHttp().setHttpServer(defaultHttpServer);
             defaultHttpServer.addConnector(new ConnectorFactory("SearchServer", Defaults.getDefaults().vespaWebServicePort()));
         }
     }
 
-    private static Http createHostedImplicitHttpWithAccessControl(
-            DeployState deployState, AthenzDomain tenantDomain, ApplicationContainerCluster cluster) {
+    private void addHostedImplicitAccessControlIfNotPresent(DeployState deployState, ApplicationContainerCluster cluster) {
+        Http http = cluster.getHttp();
+        if (http.getAccessControl().isPresent()) return; // access control added explicitly
+        AthenzDomain tenantDomain = deployState.getProperties().athenzDomain().orElse(null);
+        if (tenantDomain == null) return; // tenant domain not present, cannot add access control. this should eventually be a failure.
         AccessControl accessControl =
                 new AccessControl.Builder(tenantDomain.value(), deployState.getDeployLogger())
                         .setHandlers(cluster)
                         .readEnabled(false)
                         .writeEnabled(false)
                         .build();
-        Http http = new Http(accessControl.getBindings(), accessControl);
-        FilterChains filterChains = new FilterChains(cluster);
-        filterChains.add(new Chain<>(FilterChains.emptyChainSpec(ACCESS_CONTROL_CHAIN_ID)));
-        http.setFilterChains(filterChains);
-        return http;
-    }
-
-    private static Http createHostedImplicitHttpWithoutAccessControl(ApplicationContainerCluster cluster) {
-        Http http = new Http(Collections.emptyList());
-        http.setFilterChains(new FilterChains(cluster));
-        return http;
+        http.getFilterChains().add(new Chain<>(FilterChains.emptyChainSpec(ACCESS_CONTROL_CHAIN_ID)));
+        http.setAccessControl(accessControl);
+        http.getBindings().addAll(accessControl.getBindings());
     }
 
     private Http buildHttp(DeployState deployState, ApplicationContainerCluster cluster, Element httpElement) {
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 73db6e35428..8fcd743cb2d 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
@@ -46,6 +46,7 @@ import com.yahoo.vespa.model.container.SecretStore;
 import com.yahoo.vespa.model.container.component.Component;
 import com.yahoo.vespa.model.container.http.AccessControl;
 import com.yahoo.vespa.model.container.http.ConnectorFactory;
+import com.yahoo.vespa.model.container.http.Http;
 import com.yahoo.vespa.model.content.utils.ContentClusterUtils;
 import com.yahoo.vespa.model.test.utils.VespaModelCreatorWithFilePkg;
 import org.hamcrest.Matchers;
@@ -811,7 +812,7 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase {
         ApplicationContainer container = (ApplicationContainer)root.getProducer("container/container.0");
 
         // Verify that there are two connectors
-        List<ConnectorFactory> connectorFactories = container.getHttp().getHttpServer().getConnectorFactories();
+        List<ConnectorFactory> connectorFactories = container.getHttp().getHttpServer().get().getConnectorFactories();
         assertEquals(2, connectorFactories.size());
         List<Integer> ports = connectorFactories.stream()
                 .map(ConnectorFactory::getListenPort)
@@ -857,6 +858,34 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase {
         assertThat(accessControl.domain, equalTo(tenantDomain.value()));
     }
 
+    @Test
+    public void access_control_is_implicitly_added_for_hosted_apps_with_existing_http_element() {
+        Element clusterElem = DomBuilderTest.parse(
+                "<container version='1.0'>",
+                "  <http>",
+                "    <server port='" + getDefaults().vespaWebServicePort() + "' id='main' />",
+                "    <filtering>",
+                "      <filter id='outer' />",
+                "      <request-chain id='myChain'>",
+                "        <filter id='inner' />",
+                "      </request-chain>",
+                "    </filtering>",
+                "  </http>",
+                nodesXml,
+                "</container>" );
+        AthenzDomain tenantDomain = AthenzDomain.from("my-tenant-domain");
+        DeployState state = new DeployState.Builder().properties(
+                new TestProperties()
+                        .setAthenzDomain(tenantDomain)
+                        .setHostedVespa(true))
+                .build();
+        createModel(root, state, null, clusterElem);
+        Http http = ((ApplicationContainer) root.getProducer("container/container.0")).getHttp();
+        assertThat(http.getAccessControl().isPresent(), is(true));
+        assertThat(http.getFilterChains().hasChain(AccessControl.ACCESS_CONTROL_CHAIN_ID), is(true));
+        assertThat(http.getFilterChains().hasChain(ComponentId.fromString("myChain")), is(true));
+    }
+
 
     private Element generateContainerElementWithRenderer(String rendererId) {
         return DomBuilderTest.parse(