-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Test fixtures krb5 #40297
Test fixtures krb5 #40297
Conversation
Add two separate fixtures for the different configs used by this fixture.
This breaks on windows where TMP dir default to C:\Windows and startup fails with a permission error. I tried to create a tmp dir and pass in `TMP` env, but it lead to a class not found error, and since testclusers is already independent of the calling environment I stopped there.
We only ever connect on localhost. This entry just causes a missleading connection error
turns out we need both local and mapped port in the file
We need to disable these too when docker is not supproted.
Pinging @elastic/es-core-infra |
@elasticmachine test this please |
|
||
// Create HDFS File System Testing Fixtures for HA/Secure combinations | ||
for (String fixtureName : ['hdfsFixture', 'haHdfsFixture', 'secureHdfsFixture', 'secureHaHdfsFixture']) { | ||
project.tasks.create(fixtureName, org.elasticsearch.gradle.test.AntFixture) { | ||
dependsOn project.configurations.hdfsFixture | ||
dependsOn project.configurations.hdfsFixture, project(':test:fixtures:krb5kdc-fixture').tasks.postProcessFixture |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a better abstraction for this. This doesn't need to be done in this PR but something as simple as "this task needs this fixture to be done being setup" seems a common enough pattern to deserve it's own DSL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. We already add this dependency to testing tasks (RandomizedTestingTask
that is, we'll have to add Gradle Test
too there ).
This is the first example of a task in the "client" project other than a test needing to have the fixture up. I was thinking of creating a postProcessFixture
task on the "client" project identical to the one on the fixture to allow for dumping ports to files, and depending on the fixture. What do you think ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was envisioning something similar to what we do for the rest tests. That is, we decorate tasks on the consuming project with some DSL which folks can use to indicate that task requires a given test fixture.
// Set the keytab files in the classpath so that we can access them from test code without the security manager | ||
// freaking out. | ||
project.dependencies { | ||
testRuntime fileTree(dir: project(':test:fixtures:krb5kdc-fixture').ext.krb5Keytabs("hdfs","hdfs_hdfs.build.elastic.co.keytab").parent, include: ['*.keytab']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, this kind of cross-project coupling makes me very uneasy. It's just so easy to break this kind of thing. We really should model these kinds of things as proper project dependencies. At the very least then we can ask Gradle "who uses this thing?".
We don't need to address this as part of the PR. Similar to my comment above, I think we need to provide better abstractions around things a fixture exposes to a dependent project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM 👍
Closes #39977 since the fixture is no longer vagrant based. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick sanity question, everything else LGTM
@@ -19,6 +19,9 @@ | |||
|
|||
set -e | |||
|
|||
krb5kdc | |||
kadmind |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be in the hdfs.sh instead? Do these commands have any issues with being executed multiple times when adding multiple principals for a fixture?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify "in hdfs.sh": I mean "in provision/hdfs.sh and provision/peppa.sh"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it but the containers don't come online. since the tests work as-si I didn't look further at it, we can perhaps address this lather on.
@elasticmachine run elasticsearch-ci/2 |
@elasticmachine run elasticsearch-ci/1 |
@elasticmachine elasticsearch-ci/2 |
@elasticmachine run elasticsearch-ci/2 |
@elasticmachine run elasticsearch-ci/2 |
Replaces the vagrant based kerberos fixtures with docker based test fixtures plugin. The configuration is now entirely static on the docker side and no longer driven by Gradle, also two different services are being configured since there are two different consumers of the fixture that can run in parallel and require different configurations.
* master: (25 commits) [DOCS] Correct keystore commands for Email and Jira actions in Watcher (elastic#40417) [DOCS] Document common settings for snapshot repository plugins (elastic#40475) Remove with(out)-system-key tests (elastic#40547) Geo Point parse error fix (elastic#40447) Handle null retention leases in WaitForNoFollowersStep (elastic#40477) [DOCS] Adds anchors for ruby client (elastic#39867) Mute DataFrameAuditorIT#testAuditorWritesAudits Disable integTest when Docker is not available (elastic#40585) Add randomScore function in script_score query (elastic#40186) Test fixtures krb5 (elastic#40297) Correct ILM metadata minimum compatibility version (elastic#40569) SQL: Centralize SQL test dependencies version handling (elastic#40551) Mute testTracerLog Mute testHttpInput Include functions' aliases in the list of functions (elastic#40584) Optimise rejection of out-of-range `long` values (elastic#40325) Add docs for cluster.remote.*.proxy setting (elastic#40281) Migrate systemd packaging tests from bats to java (elastic#39954) Move top-level pipeline aggs out of QuerySearchResult (elastic#40319) Use openjdk 12 in packer cache script (elastic#40498) ...
@atorok, can we backport this to 7.0 and 6.7 too ? We'll still run these for some time in CI and parts of the changes here also affect/resolve issues with other test fixtures (idp-fixture) |
Replaces the vagrant based kerberos fixtures with docker based test fixtures plugin. The configuration is now entirely static on the docker side and no longer driven by Gradle, also two different services are being configured since there are two different consumers of the fixture that can run in parallel and require different configurations.
Replaces the vagrant based kerberos fixtures with docker based test fixtures plugin. The configuration is now entirely static on the docker side and no longer driven by Gradle, also two different services are being configured since there are two different consumers of the fixture that can run in parallel and require different configurations.
* elastic/7.0: (50 commits) Fix more broken links in plugins docs Fix archives links in plugins docs Disable integTest when Docker is not available (elastic#40585) Add docs for bundled jdk (elastic#40487) [DOCS] Correct keystore commands for Email and Jira actions in Watcher (elastic#40417) (elastic#40613) Add usage indicators for the bundled JDK (elastic#40616) Add ability to mute and mute flaky fixture (elastic#40630) Test fixtures krb5 (elastic#40297) Update docs for the DFR similarity (elastic#40579) Update ingest jdocs that a null return value will drop the current document. (elastic#40359) [DOCS] Document common settings for snapshot repository plugins (elastic#40475) (elastic#40607) [DOCS] Fixes formatting in breaking changes Handle null retention leases in WaitForNoFollowersStep (elastic#40477) Correct ILM metadata minimum compatibility version (elastic#40569) Mute SpecificMasterNodesIT.testElectOnlyBetweenMasterNodes() Mute testHttpInput Include functions' aliases in the list of functions (elastic#40584) Optimise rejection of out-of-range `long` values (elastic#40325) Add docs for cluster.remote.*.proxy setting (elastic#40281) Mute WatchAckTests.testAckAllActions ...
Replaces the vagrant based kerberos fixtures with docker based test fixtures plugin.
The configuration is now entirely static on the docker side and no longer driven by Gradle,
also two different services are being configured since there are two different consumers of the fixture that can run in parallel and require different configurations.
@jbaiera the checksum error was due to the fact that both the local and the mapped ports need to be in the config since depending on where this is called upon one or the other will be used.
Closes #34095