Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Proposal to replace fixtures with docker-compose #35651

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .ci/packer_cache.sh
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,4 @@ while [ -h "$SCRIPT" ] ; do
done

source $(dirname "${SCRIPT}")/java-versions.properties
JAVA_HOME="${HOME}"/.java/${ES_BUILD_JAVA} ./gradlew resolveAllDependencies --parallel
JAVA_HOME="${HOME}"/.java/${ES_BUILD_JAVA} ./gradlew --parallel resolveAllDependencies composePull
3 changes: 3 additions & 0 deletions TESTING.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,9 @@ If you want to just run the precommit checks:
./gradlew precommit
---------------------------------------------------------------------------

Some of these checks will require `docker-compose` installed for bringing up
test fixtures. If it's not present those checks will be skipped automatically.

== Testing the REST layer

The available integration tests make use of the java API to communicate with
Expand Down
1 change: 1 addition & 0 deletions buildSrc/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ dependencies {
compile "org.elasticsearch:jna:4.5.1"
compile 'com.github.jengelman.gradle.plugins:shadow:2.0.4'
compile 'de.thetaphi:forbiddenapis:2.6'
compile 'com.avast.gradle:docker-compose-gradle-plugin:0.4.5'
testCompile "junit:junit:${props.getProperty('junit')}"
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.elasticsearch.gradle.testfixtures;

import org.gradle.api.NamedDomainObjectContainer;
import org.gradle.api.Project;

public class TestFixtureExtension {

private final Project project;
final NamedDomainObjectContainer<Project> fixtures;

public TestFixtureExtension(Project project) {
this.project = project;
this.fixtures = project.container(Project.class);
}

public void useFixture(String path) {
Project fixtureProject = this.project.findProject(path);
if (fixtureProject == null) {
throw new IllegalArgumentException("Could not find test fixture " + fixtureProject);
}
if (fixtureProject.file(TestFixturesPlugin.DOCKER_COMPOSE_YML).exists() == false) {
throw new IllegalArgumentException(
"Project " + path + " is not a valid test fixture: missing " + TestFixturesPlugin.DOCKER_COMPOSE_YML
);
}
fixtures.add(fixtureProject);
}


}
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.elasticsearch.gradle.testfixtures;

import com.avast.gradle.dockercompose.ComposeExtension;
import com.avast.gradle.dockercompose.DockerComposePlugin;
import org.elasticsearch.gradle.precommit.JarHellTask;
import org.elasticsearch.gradle.precommit.ThirdPartyAuditTask;
import org.gradle.api.DefaultTask;
import org.gradle.api.Plugin;
import org.gradle.api.Project;
import org.gradle.api.Task;
import org.gradle.api.plugins.BasePlugin;
import org.gradle.api.tasks.TaskContainer;

import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.util.Collections;

public class TestFixturesPlugin implements Plugin<Project> {

static final String DOCKER_COMPOSE_YML = "docker-compose.yml";

@Override
public void apply(Project project) {
TaskContainer tasks = project.getTasks();

TestFixtureExtension extension = project.getExtensions().create(
"testFixtures", TestFixtureExtension.class, project
);

// Don't look for docker-compose on the PATH yet that would pick up on Windows as well
if (project.file("/usr/local/bin/docker-compose").exists() == false &&
project.file("/usr/bin/docker-compose").exists() == false
) {
project.getLogger().warn(
"Tests require docker-compose at /usr/local/bin/docker-compose or /usr/bin/docker-compose " +
"but none could not be found so these will be skipped"
);
tasks.withType(getTaskClass("com.carrotsearch.gradle.junit4.RandomizedTestingTask"), task ->
task.setEnabled(false)
);
return;
}

if (project.file(DOCKER_COMPOSE_YML).exists()) {
project.apply(spec -> spec.plugin(BasePlugin.class));
project.apply(spec -> spec.plugin(DockerComposePlugin.class));
ComposeExtension composeExtension = project.getExtensions().getByType(ComposeExtension.class);
composeExtension.setUseComposeFiles(Collections.singletonList(DOCKER_COMPOSE_YML));
composeExtension.setRemoveContainers(true);
composeExtension.setExecutable(
project.file("/usr/local/bin/docker-compose").exists() ?
"/usr/local/bin/docker-compose" : "/usr/bin/docker-compose"
);

project.getTasks().getByName("clean").dependsOn("composeDown");

// convenience boilerplate with build plugin
project.getPluginManager().withPlugin("elasticsearch.build", (appliedPlugin) -> {
// Can't reference tasks that are implemented in Groovy, use reflection instead
disableTaskByType(tasks, getTaskClass("org.elasticsearch.gradle.precommit.LicenseHeadersTask"));
disableTaskByType(tasks, getTaskClass("com.carrotsearch.gradle.junit4.RandomizedTestingTask"));
disableTaskByType(tasks, ThirdPartyAuditTask.class);
disableTaskByType(tasks, JarHellTask.class);
});
} else {
tasks.withType(getTaskClass("com.carrotsearch.gradle.junit4.RandomizedTestingTask"), task ->
extension.fixtures.all(fixtureProject -> {
task.dependsOn(fixtureProject.getTasks().getByName("composeUp"));
task.finalizedBy(fixtureProject.getTasks().getByName("composeDown"));
// Configure ports for the tests as system properties.
// We only know these at execution time so we need to do it in doFirst
task.doFirst(it ->
fixtureProject.getExtensions().getByType(ComposeExtension.class).getServicesInfos()
.forEach((service, infos) ->
infos.getPorts()
.forEach((container, host) -> setSystemProperty(
it,
"test.fixtures." + fixtureProject.getName() + "." + service + "." + container,
host
))
));
}));
}
}

private void setSystemProperty(Task task, String name, Object value) {
try {
Method systemProperty = task.getClass().getMethod("systemProperty", String.class, Object.class);
systemProperty.invoke(task, name, value);
} catch (NoSuchMethodException e) {
throw new IllegalArgumentException("Could not find systemProperty method on RandomizedTestingTask", e);
} catch (IllegalAccessException | InvocationTargetException e) {
throw new IllegalArgumentException("Could not call systemProperty method on RandomizedTestingTask", e);
}
}

private void disableTaskByType(TaskContainer tasks, Class<? extends Task> type) {
tasks.withType(type, task -> task.setEnabled(false));
}

@SuppressWarnings("unchecked")
private Class<? extends DefaultTask> getTaskClass(String type) {
Class<?> aClass;
try {
aClass = Class.forName(type);
if (DefaultTask.class.isAssignableFrom(aClass) == false) {
throw new IllegalArgumentException("Not a task type: " + type);
}
} catch (ClassNotFoundException e) {
throw new IllegalArgumentException("No such task: " + type);
}
return (Class<? extends DefaultTask>) aClass;
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
implementation-class=org.elasticsearch.gradle.testfixtures.TestFixturesPlugin
22 changes: 4 additions & 18 deletions x-pack/qa/third-party/active-directory/build.gradle
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
Project smbFixtureProject = xpackProject("test:smb-fixture")
evaluationDependsOn(smbFixtureProject.path)

apply plugin: 'elasticsearch.vagrantsupport'
apply plugin: 'elasticsearch.standalone-test'
apply plugin: 'elasticsearch.test.fixtures'

dependencies {
testCompile project(xpackModule('security'))
testCompile project(path: xpackModule('security'), configuration: 'testArtifacts')
}

testFixtures.useFixture ":x-pack:test:smb-fixture"

// add test resources from security, so tests can use example certs
sourceSets.test.resources.srcDirs(project(xpackModule('security')).sourceSets.test.resources.srcDirs)
compileTestJava.options.compilerArgs << "-Xlint:-deprecation,-rawtypes,-serial,-try,-unchecked"
Expand All @@ -31,17 +30,4 @@ test {
}

// these are just tests, no need to audit
thirdPartyAudit.enabled = false

task smbFixture {
dependsOn "vagrantCheckVersion", "virtualboxCheckVersion", smbFixtureProject.up
}

if (project.rootProject.vagrantSupported) {
if (project.hasProperty('useExternalAD') == false) {
test.dependsOn smbFixture
test.finalizedBy smbFixtureProject.halt
}
} else {
test.enabled = false
}
thirdPartyAudit.enabled = false
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ private MockSecureSettings newSecureSettings(String key, String value) {
return secureSettings;
}

@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/35738")
public void testUserSearchWithActiveDirectory() throws Exception {
String groupSearchBase = "DC=ad,DC=test,DC=elasticsearch,DC=com";
String userSearchBase = "CN=Users,DC=ad,DC=test,DC=elasticsearch,DC=com";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,14 @@ public abstract class AbstractActiveDirectoryTestCase extends ESTestCase {
// as we cannot control the URL of the referral which may contain a non-resolvable DNS name as
// this name would be served by the samba4 instance
public static final Boolean FOLLOW_REFERRALS = Booleans.parseBoolean(getFromEnv("TESTS_AD_FOLLOW_REFERRALS", "false"));
public static final String AD_LDAP_URL = getFromEnv("TESTS_AD_LDAP_URL", "ldaps://localhost:61636");
public static final String AD_LDAP_GC_URL = getFromEnv("TESTS_AD_LDAP_GC_URL", "ldaps://localhost:63269");
public static final String AD_LDAP_URL = getFromEnv("TESTS_AD_LDAP_URL", "ldaps://localhost:" + getFromProperty("636"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe name them rather than use the original port number? As much as I love port numbers I think it'd be nicer to call them LDAP and LDAP_GC or something.

public static final String AD_LDAP_GC_URL = getFromEnv("TESTS_AD_LDAP_GC_URL", "ldaps://localhost:" + getFromProperty("3269"));
public static final String PASSWORD = getFromEnv("TESTS_AD_USER_PASSWORD", "Passw0rd");
public static final String AD_LDAP_PORT = getFromEnv("TESTS_AD_LDAP_PORT", "61389");
public static final String AD_LDAPS_PORT = getFromEnv("TESTS_AD_LDAPS_PORT", "61636");
public static final String AD_GC_LDAP_PORT = getFromEnv("TESTS_AD_GC_LDAP_PORT", "63268");
public static final String AD_GC_LDAPS_PORT = getFromEnv("TESTS_AD_GC_LDAPS_PORT", "63269");
public static final String AD_LDAP_PORT = getFromEnv("TESTS_AD_LDAP_PORT", getFromProperty("389"));

public static final String AD_LDAPS_PORT = getFromEnv("TESTS_AD_LDAPS_PORT", getFromProperty("636"));
public static final String AD_GC_LDAP_PORT = getFromEnv("TESTS_AD_GC_LDAP_PORT", getFromProperty("3268"));
public static final String AD_GC_LDAPS_PORT = getFromEnv("TESTS_AD_GC_LDAPS_PORT", getFromProperty("3269"));
public static final String AD_DOMAIN = "ad.test.elasticsearch.com";

protected SSLService sslService;
Expand Down Expand Up @@ -151,4 +152,11 @@ private static String getFromEnv(String envVar, String defaultValue) {
final String value = System.getenv(envVar);
return value == null ? defaultValue : value;
}

private static String getFromProperty(String port) {
String key = "test.fixtures.smb-fixture.fixture." + port;
final String value = System.getProperty(key);
assertNotNull("Expected the actual value for " + port + " to be in system property " + key, value);
return value;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ public void setReferralFollowing() {
ldapConnection.getConnectionOptions().setFollowReferrals(AbstractActiveDirectoryTestCase.FOLLOW_REFERRALS);
}

@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/35738")
@SuppressWarnings("unchecked")
public void testResolveSubTree() throws Exception {
Settings settings = Settings.builder()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ protected Settings nodeSettings(int nodeOrdinal) {
return builder.build();
}

@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/35738")
public void testRunAs() throws Exception {
String avenger = realmConfig.loginWithCommonName ? "Natasha Romanoff" : "blackwidow";
final AuthenticateRequest request = new AuthenticateRequest(avenger);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import com.unboundid.ldap.sdk.LDAPException;
import com.unboundid.ldap.sdk.ResultCode;
import org.apache.lucene.util.LuceneTestCase;
import org.elasticsearch.action.support.PlainActionFuture;
import org.elasticsearch.common.settings.SecureString;
import org.elasticsearch.common.settings.Settings;
Expand Down Expand Up @@ -42,6 +43,7 @@
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.is;

@LuceneTestCase.AwaitsFix(bugUrl = "ActiveDirectorySessionFactoryTests")
public class ActiveDirectorySessionFactoryTests extends AbstractActiveDirectoryTestCase {

private static final String REALM_NAME = "ad-test";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,17 @@
*/
package org.elasticsearch.xpack.security.authc.ldap;

import org.apache.lucene.util.LuceneTestCase;

import java.io.IOException;

/**
* This tests the group to role mappings from LDAP sources provided by the super class - available from super.realmConfig.
* The super class will provide appropriate group mappings via configGroupMappings()
*/
@LuceneTestCase.AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/35738")
public class GroupMappingIT extends AbstractAdLdapRealmTestCase {

public void testAuthcAuthz() throws IOException {
String avenger = realmConfig.loginWithCommonName ? "Natasha Romanoff" : "blackwidow";
assertAccessAllowed(avenger, "avengers");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ protected String configRoles() {
" privileges: [ all ]\n";
}

@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/35738")
public void testGroupMapping() throws IOException {
String asgardian = "odin";
String securityPhilanthropist = realmConfig.loginWithCommonName ? "Bruce Banner" : "hulk";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ protected Settings nodeSettings(int nodeOrdinal) {
* Because one realm is using "common name" (cn) for login, and the other uses the "userid" (sAMAccountName) [see
* {@link #setupSecondaryRealm()}], this is simply a matter of checking that we can authenticate with both identifiers.
*/
@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/35738")
public void testCanAuthenticateAgainstBothRealms() throws IOException {
assertAccessAllowed("Natasha Romanoff", "avengers");
assertAccessAllowed("blackwidow", "avengers");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import com.unboundid.ldap.sdk.Attribute;
import com.unboundid.ldap.sdk.SearchRequest;
import com.unboundid.ldap.sdk.SearchScope;
import org.apache.lucene.util.LuceneTestCase;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.xpack.core.security.authc.RealmConfig;
Expand All @@ -22,6 +23,7 @@
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.hasItems;

@LuceneTestCase.AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/35738")
public class UserAttributeGroupsResolverTests extends GroupsResolverTestCase {

public static final String BRUCE_BANNER_DN = "cn=Bruce Banner,CN=Users,DC=ad,DC=test,DC=elasticsearch,DC=com";
Expand Down
12 changes: 12 additions & 0 deletions x-pack/test/smb-fixture/Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
FROM ubuntu:16.04
RUN apt-get update -qqy && apt-get install -qqy samba ldap-utils
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that installsmb.sh is being run in the docker file, I think installing samba and ldap-utils should move back into there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion on that, and don't mind doing it in a follow up.
One advantage of keeping these separate is that one can iterate on the install script without waiting for apt all the time, but having everything in the same script is also nice.

ADD . /fixture
RUN chmod +x /fixture/src/main/resources/provision/installsmb.sh
RUN /fixture/src/main/resources/provision/installsmb.sh

EXPOSE 389
EXPOSE 636
EXPOSE 3268
EXPOSE 3269

CMD service samba-ad-dc restart && sleep infinity
Loading