From a6fb10826ba5fc726cb0c593a22b4a6d4ccaa18f Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Wed, 11 Jan 2017 11:23:07 -0500 Subject: [PATCH 01/10] Remove doc links from config template The config template that ships with Elasticsearch distributions contains links to various pieces of documentation. Links go out of date and get broken. This commit removes such links from the config template. Relates #22553 --- .../src/main/resources/config/elasticsearch.yml | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/distribution/src/main/resources/config/elasticsearch.yml b/distribution/src/main/resources/config/elasticsearch.yml index bf806535c9b..15e841fe390 100644 --- a/distribution/src/main/resources/config/elasticsearch.yml +++ b/distribution/src/main/resources/config/elasticsearch.yml @@ -7,8 +7,8 @@ # The primary way of configuring a node is via this file. This template lists # the most important settings you may want to configure for a production cluster. # -# Please see the documentation for further information on configuration options: -# +# Please consult the documentation for further information on configuration options: +# https://www.elastic.co/guide/en/elasticsearch/reference/index.html # # ---------------------------------- Cluster ----------------------------------- # @@ -58,8 +58,7 @@ # #http.port: 9200 # -# For more information, see the documentation at: -# +# For more information, consult the network module documentation. # # --------------------------------- Discovery ---------------------------------- # @@ -68,12 +67,11 @@ # #discovery.zen.ping.unicast.hosts: ["host1", "host2"] # -# Prevent the "split brain" by configuring the majority of nodes (total number of nodes / 2 + 1): +# Prevent the "split brain" by configuring the majority of nodes (total number of master-eligible nodes / 2 + 1): # #discovery.zen.minimum_master_nodes: 3 # -# For more information, see the documentation at: -# +# For more information, consult the zen discovery module documentation. # # ---------------------------------- Gateway ----------------------------------- # @@ -81,8 +79,7 @@ # #gateway.recover_after_nodes: 3 # -# For more information, see the documentation at: -# +# For more information, consult the gateway module documentation. # # ---------------------------------- Various ----------------------------------- # From 389ffc93d8330e65737f744f900af1bb714bc644 Mon Sep 17 00:00:00 2001 From: Ali Beyad Date: Wed, 11 Jan 2017 11:27:25 -0500 Subject: [PATCH 02/10] Adds debugging information for invalid repository data x-content --- .../repositories/blobstore/BlobStoreRepository.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/core/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java b/core/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java index fb77d351db1..65153a9f586 100644 --- a/core/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java +++ b/core/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java @@ -620,6 +620,9 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp // EMPTY is safe here because RepositoryData#fromXContent calls namedObject try (XContentParser parser = XContentHelper.createParser(NamedXContentRegistry.EMPTY, out.bytes())) { repositoryData = RepositoryData.snapshotsFromXContent(parser, indexGen); + } catch (NotXContentException e) { + logger.warn("[{}] index blob is not valid x-content [{} bytes]", snapshotsIndexBlobName, out.bytes().length()); + throw e; } } From fed2a1a8225996f9dbad2ebd5cd3f8c4242f87a8 Mon Sep 17 00:00:00 2001 From: Lee Hinman Date: Wed, 11 Jan 2017 10:08:04 -0700 Subject: [PATCH 03/10] Fix Translog.Delete serialization for sequence numbers (#22543) * Fix Translog.Delete serialization for sequence numbers Translog.Delete used `.writeVLong` instead of `.writeLong` for the sequence number and primary term (and their respective "read" variants). This could lead to issues where a 5.x node sent a translog operation with a negative sequence number (-2 for unassigned seq no) that tripped an assertion serializing a negative number and causing ES to exit. Adds a unit test for serialization and a mixed-cluster REST test, since that was how this was originally caught. * Use more realistic values for random seqNum and primary term * Add comment with TODO for removal in 7.0 * Change comment into an assert --- .../index/translog/Translog.java | 10 +-- .../index/translog/TranslogTests.java | 62 +++++++++++++++++-- .../test/mixed_cluster/10_basic.yaml | 25 +++++++- 3 files changed, 87 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/translog/Translog.java b/core/src/main/java/org/elasticsearch/index/translog/Translog.java index bdbce03bda1..fa41824f4de 100644 --- a/core/src/main/java/org/elasticsearch/index/translog/Translog.java +++ b/core/src/main/java/org/elasticsearch/index/translog/Translog.java @@ -855,7 +855,7 @@ public class Translog extends AbstractIndexShardComponent implements IndexShardC in.readLong(); // ttl } this.versionType = VersionType.fromValue(in.readByte()); - assert versionType.validateVersionForWrites(this.version); + assert versionType.validateVersionForWrites(this.version) : "invalid version for writes: " + this.version; if (format >= FORMAT_AUTO_GENERATED_IDS) { this.autoGeneratedIdTimestamp = in.readLong(); } else { @@ -1036,8 +1036,8 @@ public class Translog extends AbstractIndexShardComponent implements IndexShardC this.versionType = VersionType.fromValue(in.readByte()); assert versionType.validateVersionForWrites(this.version); if (format >= FORMAT_SEQ_NO) { - seqNo = in.readVLong(); - primaryTerm = in.readVLong(); + seqNo = in.readLong(); + primaryTerm = in.readLong(); } } @@ -1100,8 +1100,8 @@ public class Translog extends AbstractIndexShardComponent implements IndexShardC out.writeString(uid.text()); out.writeLong(version); out.writeByte(versionType.getValue()); - out.writeVLong(seqNo); - out.writeVLong(primaryTerm); + out.writeLong(seqNo); + out.writeLong(primaryTerm); } @Override diff --git a/core/src/test/java/org/elasticsearch/index/translog/TranslogTests.java b/core/src/test/java/org/elasticsearch/index/translog/TranslogTests.java index a2a6620a6c2..a3e3f611b21 100644 --- a/core/src/test/java/org/elasticsearch/index/translog/TranslogTests.java +++ b/core/src/test/java/org/elasticsearch/index/translog/TranslogTests.java @@ -23,6 +23,9 @@ import com.carrotsearch.randomizedtesting.generators.RandomPicks; import org.apache.logging.log4j.message.ParameterizedMessage; import org.apache.logging.log4j.util.Supplier; import org.apache.lucene.codecs.CodecUtil; +import org.apache.lucene.document.Field; +import org.apache.lucene.document.NumericDocValuesField; +import org.apache.lucene.document.TextField; import org.apache.lucene.index.Term; import org.apache.lucene.mockfile.FilterFileChannel; import org.apache.lucene.store.AlreadyClosedException; @@ -31,6 +34,7 @@ import org.apache.lucene.store.MockDirectoryWrapper; import org.apache.lucene.util.IOUtils; import org.apache.lucene.util.LineFileDocs; import org.apache.lucene.util.LuceneTestCase; +import org.elasticsearch.Version; import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; @@ -47,6 +51,12 @@ import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.index.VersionType; +import org.elasticsearch.index.engine.Engine; +import org.elasticsearch.index.engine.Engine.Operation.Origin; +import org.elasticsearch.index.mapper.ParseContext.Document; +import org.elasticsearch.index.mapper.ParsedDocument; +import org.elasticsearch.index.mapper.SeqNoFieldMapper; +import org.elasticsearch.index.mapper.UidFieldMapper; import org.elasticsearch.index.seqno.SequenceNumbersService; import org.elasticsearch.index.shard.ShardId; import org.elasticsearch.index.translog.Translog.Location; @@ -67,6 +77,7 @@ import java.nio.file.InvalidPathException; import java.nio.file.Path; import java.nio.file.StandardOpenOption; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.HashSet; @@ -297,14 +308,14 @@ public class TranslogTests extends ESTestCase { { final TranslogStats stats = stats(); assertThat(stats.estimatedNumberOfOperations(), equalTo(2L)); - assertThat(stats.getTranslogSizeInBytes(), equalTo(125L)); + assertThat(stats.getTranslogSizeInBytes(), equalTo(139L)); } translog.add(new Translog.Delete(newUid("3"))); { final TranslogStats stats = stats(); assertThat(stats.estimatedNumberOfOperations(), equalTo(3L)); - assertThat(stats.getTranslogSizeInBytes(), equalTo(153L)); + assertThat(stats.getTranslogSizeInBytes(), equalTo(181L)); } final long seqNo = 1; @@ -313,10 +324,10 @@ public class TranslogTests extends ESTestCase { { final TranslogStats stats = stats(); assertThat(stats.estimatedNumberOfOperations(), equalTo(4L)); - assertThat(stats.getTranslogSizeInBytes(), equalTo(195L)); + assertThat(stats.getTranslogSizeInBytes(), equalTo(223L)); } - final long expectedSizeInBytes = 238L; + final long expectedSizeInBytes = 266L; translog.prepareCommit(); { final TranslogStats stats = stats(); @@ -1993,4 +2004,47 @@ public class TranslogTests extends ESTestCase { public static Translog.Location randomTranslogLocation() { return new Translog.Location(randomLong(), randomLong(), randomInt()); } + + public void testTranslogOpSerialization() throws Exception { + BytesReference B_1 = new BytesArray(new byte[]{1}); + SeqNoFieldMapper.SequenceID seqID = SeqNoFieldMapper.SequenceID.emptySeqID(); + assert Version.CURRENT.major <= 6 : "Using UNASSIGNED_SEQ_NO can be removed in 7.0, because 6.0+ nodes have actual sequence numbers"; + long randomSeqNum = randomBoolean() ? SequenceNumbersService.UNASSIGNED_SEQ_NO : randomNonNegativeLong(); + long randomPrimaryTerm = randomBoolean() ? 0 : randomNonNegativeLong(); + seqID.seqNo.setLongValue(randomSeqNum); + seqID.seqNoDocValue.setLongValue(randomSeqNum); + seqID.primaryTerm.setLongValue(randomPrimaryTerm); + Field uidField = new Field("_uid", "1", UidFieldMapper.Defaults.FIELD_TYPE); + Field versionField = new NumericDocValuesField("_version", 1); + Document document = new Document(); + document.add(new TextField("value", "test", Field.Store.YES)); + document.add(uidField); + document.add(versionField); + document.add(seqID.seqNo); + document.add(seqID.seqNoDocValue); + document.add(seqID.primaryTerm); + ParsedDocument doc = new ParsedDocument(versionField, seqID, "1", "type", null, Arrays.asList(document), B_1, null); + + Engine.Index eIndex = new Engine.Index(newUid("1"), doc, randomSeqNum, randomPrimaryTerm, + 1, VersionType.INTERNAL, Origin.PRIMARY, 0, 0, false); + Engine.IndexResult eIndexResult = new Engine.IndexResult(1, randomSeqNum, true); + Translog.Index index = new Translog.Index(eIndex, eIndexResult); + + BytesStreamOutput out = new BytesStreamOutput(); + index.writeTo(out); + StreamInput in = out.bytes().streamInput(); + Translog.Index serializedIndex = new Translog.Index(in); + assertEquals(index, serializedIndex); + + Engine.Delete eDelete = new Engine.Delete("type", "1", newUid("1"), randomSeqNum, randomPrimaryTerm, + 2, VersionType.INTERNAL, Origin.PRIMARY, 0); + Engine.DeleteResult eDeleteResult = new Engine.DeleteResult(2, randomSeqNum, true); + Translog.Delete delete = new Translog.Delete(eDelete, eDeleteResult); + + out = new BytesStreamOutput(); + delete.writeTo(out); + in = out.bytes().streamInput(); + Translog.Delete serializedDelete = new Translog.Delete(in); + assertEquals(delete, serializedDelete); + } } diff --git a/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/10_basic.yaml b/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/10_basic.yaml index c836ba73fa0..f9475057bc4 100644 --- a/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/10_basic.yaml +++ b/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/10_basic.yaml @@ -26,6 +26,13 @@ - '{"index": {"_index": "test_index", "_type": "test_type"}}' - '{"f1": "v5_mixed", "f2": 9}' + - do: + index: + index: test_index + type: test_type + id: d10 + body: {"f1": "v6_mixed", "f2": 10} + - do: indices.flush: index: test_index @@ -34,7 +41,23 @@ search: index: test_index - - match: { hits.total: 10 } # 5 docs from old cluster, 5 docs from mixed cluster + - match: { hits.total: 11 } # 5 docs from old cluster, 6 docs from mixed cluster + + - do: + delete: + index: test_index + type: test_type + id: d10 + + - do: + indices.flush: + index: test_index + + - do: + search: + index: test_index + + - match: { hits.total: 10 } --- "Verify custom cluster metadata still exists during upgrade": From 0c694b3d19d9c3b54fca2fc92bbefe7f6622ae87 Mon Sep 17 00:00:00 2001 From: Jack Conradson Date: Wed, 11 Jan 2017 09:22:24 -0800 Subject: [PATCH 04/10] Update loop counter to be higher (1000000) instead of (10000). --- .../org/elasticsearch/painless/CompilerSettings.java | 12 +++++++----- .../painless/WhenThingsGoWrongTests.java | 4 ++-- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/CompilerSettings.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/CompilerSettings.java index 9ef1b2ccf12..378cca7f58f 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/CompilerSettings.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/CompilerSettings.java @@ -41,7 +41,7 @@ public final class CompilerSettings { * Constant to be used for enabling additional internal compilation checks (slower). */ public static final String PICKY = "picky"; - + /** * For testing: do not use. */ @@ -49,15 +49,17 @@ public final class CompilerSettings { /** * The maximum number of statements allowed to be run in a loop. + * For now the number is set fairly high to accommodate users + * doing large update queries. */ - private int maxLoopCounter = 10000; + private int maxLoopCounter = 1000000; /** * Whether to throw exception on ambiguity or other internal parsing issues. This option * makes things slower too, it is only for debugging. */ private boolean picky = false; - + /** * For testing. Do not use. */ @@ -102,7 +104,7 @@ public final class CompilerSettings { public void setPicky(boolean picky) { this.picky = picky; } - + /** * Returns initial call site depth. This means we pretend we've already seen N different types, * to better exercise fallback code in tests. @@ -110,7 +112,7 @@ public final class CompilerSettings { public int getInitialCallSiteDepth() { return initialCallSiteDepth; } - + /** * For testing megamorphic fallbacks. Do not use. * @see #getInitialCallSiteDepth() diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java index 4051d8457fa..aaa337ae821 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java @@ -148,10 +148,10 @@ public class WhenThingsGoWrongTests extends ScriptTestCase { public void testLoopLimits() { // right below limit: ok - exec("for (int x = 0; x < 9999; ++x) {}"); + exec("for (int x = 0; x < 999999; ++x) {}"); PainlessError expected = expectScriptThrows(PainlessError.class, () -> { - exec("for (int x = 0; x < 10000; ++x) {}"); + exec("for (int x = 0; x < 1000000; ++x) {}"); }); assertTrue(expected.getMessage().contains( "The maximum number of statements that can be executed in a loop has been reached.")); From 25a5f1869af4d2391b89aa0c30955f08d0244ecb Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Wed, 11 Jan 2017 12:55:23 -0500 Subject: [PATCH 05/10] Improve error message when reindex-from-remote gets bad json (#22536) Adds a message about how the remote is unlikely to be Elasticsearch. This isn't as good as including the whole message from the remote but we can't do that because we are stream parsing it and we don't want to mark the whole request. Closes #22330 --- .../reindex/remote/RemoteScrollableHitSource.java | 9 ++++++++- .../remote/RemoteScrollableHitSourceTests.java | 11 +++++++++++ .../src/test/resources/responses/some_text.txt | 1 + 3 files changed, 20 insertions(+), 1 deletion(-) create mode 100644 modules/reindex/src/test/resources/responses/some_text.txt diff --git a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/remote/RemoteScrollableHitSource.java b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/remote/RemoteScrollableHitSource.java index 03a90bba815..2971319726b 100644 --- a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/remote/RemoteScrollableHitSource.java +++ b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/remote/RemoteScrollableHitSource.java @@ -36,6 +36,7 @@ import org.elasticsearch.client.RestClient; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.ParseFieldMatcher; import org.elasticsearch.common.ParseFieldMatcherSupplier; +import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.Strings; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.unit.TimeValue; @@ -176,9 +177,15 @@ public class RemoteScrollableHitSource extends ScrollableHitSource { try (XContentParser xContentParser = xContentType.xContent().createParser(NamedXContentRegistry.EMPTY, content)) { parsedResponse = parser.apply(xContentParser, () -> ParseFieldMatcher.STRICT); + } catch (ParsingException e) { + /* Because we're streaming the response we can't get a copy of it here. The best we can do is hint that it + * is totally wrong and we're probably not talking to Elasticsearch. */ + throw new ElasticsearchException( + "Error parsing the response, remote is likely not an Elasticsearch instance", e); } } catch (IOException e) { - throw new ElasticsearchException("Error deserializing response", e); + throw new ElasticsearchException("Error deserializing response, remote is likely not an Elasticsearch instance", + e); } listener.accept(parsedResponse); } diff --git a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/remote/RemoteScrollableHitSourceTests.java b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/remote/RemoteScrollableHitSourceTests.java index 780bb716719..495104e53e6 100644 --- a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/remote/RemoteScrollableHitSourceTests.java +++ b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/remote/RemoteScrollableHitSourceTests.java @@ -463,6 +463,17 @@ public class RemoteScrollableHitSourceTests extends ESTestCase { assertThat(e.getCause().getCause().getMessage(), containsString("Response didn't include Content-Type: body={")); } + public void testInvalidJsonThinksRemoveIsNotES() throws IOException { + Exception e = expectThrows(RuntimeException.class, () -> sourceWithMockedRemoteCall("some_text.txt").doStart(null)); + assertEquals("Error parsing the response, remote is likely not an Elasticsearch instance", e.getCause().getCause().getMessage()); + } + + public void testUnexpectedJsonThinksRemoveIsNotES() throws IOException { + // Use the response from a main action instead of a proper start response to generate a parse error + Exception e = expectThrows(RuntimeException.class, () -> sourceWithMockedRemoteCall("main/2_3_3.json").doStart(null)); + assertEquals("Error parsing the response, remote is likely not an Elasticsearch instance", e.getCause().getCause().getMessage()); + } + private RemoteScrollableHitSource sourceWithMockedRemoteCall(String... paths) throws Exception { return sourceWithMockedRemoteCall(true, ContentType.APPLICATION_JSON, paths); } diff --git a/modules/reindex/src/test/resources/responses/some_text.txt b/modules/reindex/src/test/resources/responses/some_text.txt new file mode 100644 index 00000000000..39fe6c971a8 --- /dev/null +++ b/modules/reindex/src/test/resources/responses/some_text.txt @@ -0,0 +1 @@ +I'm just text! From 609d2aab15d7af12bb6b39ce22bdb7b6f4c7cd06 Mon Sep 17 00:00:00 2001 From: Matt Weber Date: Wed, 11 Jan 2017 09:59:43 -0800 Subject: [PATCH 06/10] QueryString and SimpleQueryString Graph Support (#22541) Add support for graph token streams to "query_String" and "simple_query_string" queries. --- .../classic/MapperQueryParser.java | 48 +++++-- .../common/lucene/search/Queries.java | 29 ++++ .../index/query/MatchQueryBuilder.java | 30 +--- .../index/query/QueryStringQueryBuilder.java | 7 +- .../index/query/SimpleQueryStringBuilder.java | 11 +- .../index/search/MultiMatchQuery.java | 7 +- .../query/QueryStringQueryBuilderTests.java | 132 +++++++++++++++++- .../index/query/SimpleQueryParserTests.java | 50 +++++++ .../search/query/QueryStringIT.java | 114 ++++++++++++--- .../synonym-graph-tokenfilter.asciidoc | 9 -- 10 files changed, 342 insertions(+), 95 deletions(-) diff --git a/core/src/main/java/org/apache/lucene/queryparser/classic/MapperQueryParser.java b/core/src/main/java/org/apache/lucene/queryparser/classic/MapperQueryParser.java index 976c4706725..c1998c65000 100644 --- a/core/src/main/java/org/apache/lucene/queryparser/classic/MapperQueryParser.java +++ b/core/src/main/java/org/apache/lucene/queryparser/classic/MapperQueryParser.java @@ -19,6 +19,9 @@ package org.apache.lucene.queryparser.classic; +import static java.util.Collections.unmodifiableMap; +import static org.elasticsearch.common.lucene.search.Queries.fixNegativeQueryIfNeeded; + import org.apache.lucene.analysis.Analyzer; import org.apache.lucene.analysis.TokenStream; import org.apache.lucene.analysis.tokenattributes.CharTermAttribute; @@ -30,6 +33,7 @@ import org.apache.lucene.search.BooleanQuery; import org.apache.lucene.search.BoostQuery; import org.apache.lucene.search.DisjunctionMaxQuery; import org.apache.lucene.search.FuzzyQuery; +import org.apache.lucene.search.GraphQuery; import org.apache.lucene.search.MatchNoDocsQuery; import org.apache.lucene.search.MultiPhraseQuery; import org.apache.lucene.search.PhraseQuery; @@ -55,9 +59,6 @@ import java.util.List; import java.util.Map; import java.util.Objects; -import static java.util.Collections.unmodifiableMap; -import static org.elasticsearch.common.lucene.search.Queries.fixNegativeQueryIfNeeded; - /** * A query parser that uses the {@link MapperService} in order to build smarter * queries based on the mapping information. @@ -739,27 +740,48 @@ public class MapperQueryParser extends AnalyzingQueryParser { private Query applySlop(Query q, int slop) { if (q instanceof PhraseQuery) { - PhraseQuery pq = (PhraseQuery) q; - PhraseQuery.Builder builder = new PhraseQuery.Builder(); - builder.setSlop(slop); - final Term[] terms = pq.getTerms(); - final int[] positions = pq.getPositions(); - for (int i = 0; i < terms.length; ++i) { - builder.add(terms[i], positions[i]); - } - pq = builder.build(); //make sure that the boost hasn't been set beforehand, otherwise we'd lose it assert q instanceof BoostQuery == false; - return pq; + return addSlopToPhrase((PhraseQuery) q, slop); } else if (q instanceof MultiPhraseQuery) { MultiPhraseQuery.Builder builder = new MultiPhraseQuery.Builder((MultiPhraseQuery) q); builder.setSlop(slop); return builder.build(); + } else if (q instanceof GraphQuery && ((GraphQuery) q).hasPhrase()) { + // we have a graph query that has at least one phrase sub-query + // re-build and set slop on all phrase queries + List oldQueries = ((GraphQuery) q).getQueries(); + Query[] queries = new Query[oldQueries.size()]; + for (int i = 0; i < queries.length; i++) { + Query oldQuery = oldQueries.get(i); + if (oldQuery instanceof PhraseQuery) { + queries[i] = addSlopToPhrase((PhraseQuery) oldQuery, slop); + } else { + queries[i] = oldQuery; + } + } + + return new GraphQuery(queries); } else { return q; } } + /** + * Rebuild a phrase query with a slop value + */ + private PhraseQuery addSlopToPhrase(PhraseQuery query, int slop) { + PhraseQuery.Builder builder = new PhraseQuery.Builder(); + builder.setSlop(slop); + final Term[] terms = query.getTerms(); + final int[] positions = query.getPositions(); + for (int i = 0; i < terms.length; ++i) { + builder.add(terms[i], positions[i]); + } + + return builder.build(); + } + private Collection extractMultiFields(String field) { Collection fields; if (field != null) { diff --git a/core/src/main/java/org/elasticsearch/common/lucene/search/Queries.java b/core/src/main/java/org/elasticsearch/common/lucene/search/Queries.java index 8933b56b124..a29b249375e 100644 --- a/core/src/main/java/org/elasticsearch/common/lucene/search/Queries.java +++ b/core/src/main/java/org/elasticsearch/common/lucene/search/Queries.java @@ -20,10 +20,12 @@ package org.elasticsearch.common.lucene.search; import org.apache.lucene.index.Term; +import org.apache.lucene.queries.ExtendedCommonTermsQuery; import org.apache.lucene.search.BooleanClause; import org.apache.lucene.search.BooleanClause.Occur; import org.apache.lucene.search.BooleanQuery; import org.apache.lucene.search.ConstantScoreQuery; +import org.apache.lucene.search.GraphQuery; import org.apache.lucene.search.MatchAllDocsQuery; import org.apache.lucene.search.MatchNoDocsQuery; import org.apache.lucene.search.PrefixQuery; @@ -135,6 +137,33 @@ public class Queries { } } + /** + * Potentially apply minimum should match value if we have a query that it can be applied to, + * otherwise return the original query. + */ + public static Query maybeApplyMinimumShouldMatch(Query query, @Nullable String minimumShouldMatch) { + // If the coordination factor is disabled on a boolean query we don't apply the minimum should match. + // This is done to make sure that the minimum_should_match doesn't get applied when there is only one word + // and multiple variations of the same word in the query (synonyms for instance). + if (query instanceof BooleanQuery && !((BooleanQuery) query).isCoordDisabled()) { + return applyMinimumShouldMatch((BooleanQuery) query, minimumShouldMatch); + } else if (query instanceof ExtendedCommonTermsQuery) { + ((ExtendedCommonTermsQuery)query).setLowFreqMinimumNumberShouldMatch(minimumShouldMatch); + } else if (query instanceof GraphQuery && ((GraphQuery) query).hasBoolean()) { + // we have a graph query that has at least one boolean sub-query + // re-build and set minimum should match value on all boolean queries + List oldQueries = ((GraphQuery) query).getQueries(); + Query[] queries = new Query[oldQueries.size()]; + for (int i = 0; i < queries.length; i++) { + queries[i] = maybeApplyMinimumShouldMatch(oldQueries.get(i), minimumShouldMatch); + } + + return new GraphQuery(queries); + } + + return query; + } + private static Pattern spaceAroundLessThanPattern = Pattern.compile("(\\s+<\\s*)|(\\s*<\\s+)"); private static Pattern spacePattern = Pattern.compile(" "); private static Pattern lessThanPattern = Pattern.compile("<"); diff --git a/core/src/main/java/org/elasticsearch/index/query/MatchQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/MatchQueryBuilder.java index 20e3137b7b7..e7936b43b1c 100644 --- a/core/src/main/java/org/elasticsearch/index/query/MatchQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/MatchQueryBuilder.java @@ -461,35 +461,7 @@ public class MatchQueryBuilder extends AbstractQueryBuilder { matchQuery.setZeroTermsQuery(zeroTermsQuery); Query query = matchQuery.parse(type, fieldName, value); - if (query == null) { - return null; - } - - // If the coordination factor is disabled on a boolean query we don't apply the minimum should match. - // This is done to make sure that the minimum_should_match doesn't get applied when there is only one word - // and multiple variations of the same word in the query (synonyms for instance). - if (query instanceof BooleanQuery && !((BooleanQuery) query).isCoordDisabled()) { - query = Queries.applyMinimumShouldMatch((BooleanQuery) query, minimumShouldMatch); - } else if (query instanceof GraphQuery && ((GraphQuery) query).hasBoolean()) { - // we have a graph query that has at least one boolean sub-query - // re-build and set minimum should match value on all boolean queries - List oldQueries = ((GraphQuery) query).getQueries(); - Query[] queries = new Query[oldQueries.size()]; - for (int i = 0; i < queries.length; i++) { - Query oldQuery = oldQueries.get(i); - if (oldQuery instanceof BooleanQuery) { - queries[i] = Queries.applyMinimumShouldMatch((BooleanQuery) oldQuery, minimumShouldMatch); - } else { - queries[i] = oldQuery; - } - } - - query = new GraphQuery(queries); - } else if (query instanceof ExtendedCommonTermsQuery) { - ((ExtendedCommonTermsQuery)query).setLowFreqMinimumNumberShouldMatch(minimumShouldMatch); - } - - return query; + return Queries.maybeApplyMinimumShouldMatch(query, minimumShouldMatch); } @Override diff --git a/core/src/main/java/org/elasticsearch/index/query/QueryStringQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/QueryStringQueryBuilder.java index da427c9c305..d293199aa5a 100644 --- a/core/src/main/java/org/elasticsearch/index/query/QueryStringQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/QueryStringQueryBuilder.java @@ -1042,12 +1042,7 @@ public class QueryStringQueryBuilder extends AbstractQueryBuilder= 0; i--) { diff --git a/core/src/main/java/org/elasticsearch/index/query/SimpleQueryStringBuilder.java b/core/src/main/java/org/elasticsearch/index/query/SimpleQueryStringBuilder.java index 4e3b9d98b1e..0ea2097e0c2 100644 --- a/core/src/main/java/org/elasticsearch/index/query/SimpleQueryStringBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/SimpleQueryStringBuilder.java @@ -21,6 +21,7 @@ package org.elasticsearch.index.query; import org.apache.lucene.analysis.Analyzer; import org.apache.lucene.search.BooleanQuery; +import org.apache.lucene.search.GraphQuery; import org.apache.lucene.search.Query; import org.elasticsearch.Version; import org.elasticsearch.common.ParseField; @@ -37,6 +38,7 @@ import org.elasticsearch.index.query.SimpleQueryParser.Settings; import java.io.IOException; import java.util.HashMap; +import java.util.List; import java.util.Locale; import java.util.Map; import java.util.Objects; @@ -412,15 +414,8 @@ public class SimpleQueryStringBuilder extends AbstractQueryBuilder { @Override @@ -376,6 +379,121 @@ public class QueryStringQueryBuilderTests extends AbstractQueryTestCase 0); + for (Operator op : Operator.values()) { + BooleanClause.Occur defaultOp = op.toBooleanClauseOccur(); + MapperQueryParser queryParser = new MapperQueryParser(createShardContext()); + QueryParserSettings settings = new QueryParserSettings(""); + settings.defaultField(STRING_FIELD_NAME); + settings.fieldsAndWeights(Collections.emptyMap()); + settings.fuzziness(Fuzziness.AUTO); + settings.analyzeWildcard(true); + settings.rewriteMethod(MultiTermQuery.CONSTANT_SCORE_REWRITE); + settings.defaultOperator(op.toQueryParserOperator()); + settings.forceAnalyzer(new MockSynonymAnalyzer()); + settings.forceQuoteAnalyzer(new MockSynonymAnalyzer()); + queryParser.reset(settings); + + // simple multi-term + Query query = queryParser.parse("guinea pig"); + Query expectedQuery = new GraphQuery( + new BooleanQuery.Builder() + .add(new BooleanClause(new TermQuery(new Term(STRING_FIELD_NAME, "guinea")), defaultOp)) + .add(new BooleanClause(new TermQuery(new Term(STRING_FIELD_NAME, "pig")), defaultOp)) + .build(), + new TermQuery(new Term(STRING_FIELD_NAME, "cavy")) + ); + assertThat(query, Matchers.equalTo(expectedQuery)); + + // simple with additional tokens + query = queryParser.parse("that guinea pig smells"); + expectedQuery = new GraphQuery( + new BooleanQuery.Builder() + .add(new BooleanClause(new TermQuery(new Term(STRING_FIELD_NAME, "that")), defaultOp)) + .add(new BooleanClause(new TermQuery(new Term(STRING_FIELD_NAME, "guinea")), defaultOp)) + .add(new BooleanClause(new TermQuery(new Term(STRING_FIELD_NAME, "pig")), defaultOp)) + .add(new BooleanClause(new TermQuery(new Term(STRING_FIELD_NAME, "smells")), defaultOp)) + .build(), + new BooleanQuery.Builder() + .add(new BooleanClause(new TermQuery(new Term(STRING_FIELD_NAME, "that")), defaultOp)) + .add(new BooleanClause(new TermQuery(new Term(STRING_FIELD_NAME, "cavy")), defaultOp)) + .add(new BooleanClause(new TermQuery(new Term(STRING_FIELD_NAME, "smells")), defaultOp)) + .build() + ); + assertThat(query, Matchers.equalTo(expectedQuery)); + + // complex + query = queryParser.parse("+that -(guinea pig) +smells"); + expectedQuery = new BooleanQuery.Builder() + .add(new BooleanClause(new TermQuery(new Term(STRING_FIELD_NAME, "that")), BooleanClause.Occur.MUST)) + .add(new BooleanClause(new GraphQuery( + new BooleanQuery.Builder() + .add(new BooleanClause(new TermQuery(new Term(STRING_FIELD_NAME, "guinea")), defaultOp)) + .add(new BooleanClause(new TermQuery(new Term(STRING_FIELD_NAME, "pig")), defaultOp)) + .build(), + new TermQuery(new Term(STRING_FIELD_NAME, "cavy")) + ), BooleanClause.Occur.MUST_NOT)) + .add(new BooleanClause(new TermQuery(new Term(STRING_FIELD_NAME, "smells")), BooleanClause.Occur.MUST)) + .build(); + + assertThat(query, Matchers.equalTo(expectedQuery)); + + // no paren should cause guinea and pig to be treated as separate tokens + query = queryParser.parse("+that -guinea pig +smells"); + expectedQuery = new BooleanQuery.Builder() + .add(new BooleanClause(new TermQuery(new Term(STRING_FIELD_NAME, "that")), BooleanClause.Occur.MUST)) + .add(new BooleanClause(new TermQuery(new Term(STRING_FIELD_NAME, "guinea")), BooleanClause.Occur.MUST_NOT)) + .add(new BooleanClause(new TermQuery(new Term(STRING_FIELD_NAME, "pig")), defaultOp)) + .add(new BooleanClause(new TermQuery(new Term(STRING_FIELD_NAME, "smells")), BooleanClause.Occur.MUST)) + .build(); + + assertThat(query, Matchers.equalTo(expectedQuery)); + + // phrase + query = queryParser.parse("\"that guinea pig smells\""); + expectedQuery = new BooleanQuery.Builder() + .setDisableCoord(true) + .add(new BooleanClause(new GraphQuery( + new PhraseQuery.Builder() + .add(new Term(STRING_FIELD_NAME, "that")) + .add(new Term(STRING_FIELD_NAME, "guinea")) + .add(new Term(STRING_FIELD_NAME, "pig")) + .add(new Term(STRING_FIELD_NAME, "smells")) + .build(), + new PhraseQuery.Builder() + .add(new Term(STRING_FIELD_NAME, "that")) + .add(new Term(STRING_FIELD_NAME, "cavy")) + .add(new Term(STRING_FIELD_NAME, "smells")) + .build() + ), BooleanClause.Occur.SHOULD)).build(); + + assertThat(query, Matchers.equalTo(expectedQuery)); + + // phrase with slop + query = queryParser.parse("\"that guinea pig smells\"~2"); + expectedQuery = new BooleanQuery.Builder() + .setDisableCoord(true) + .add(new BooleanClause(new GraphQuery( + new PhraseQuery.Builder() + .add(new Term(STRING_FIELD_NAME, "that")) + .add(new Term(STRING_FIELD_NAME, "guinea")) + .add(new Term(STRING_FIELD_NAME, "pig")) + .add(new Term(STRING_FIELD_NAME, "smells")) + .setSlop(2) + .build(), + new PhraseQuery.Builder() + .add(new Term(STRING_FIELD_NAME, "that")) + .add(new Term(STRING_FIELD_NAME, "cavy")) + .add(new Term(STRING_FIELD_NAME, "smells")) + .setSlop(2) + .build() + ), BooleanClause.Occur.SHOULD)).build(); + + assertThat(query, Matchers.equalTo(expectedQuery)); + } + } + public void testToQueryRegExpQuery() throws Exception { assumeTrue("test runs only when at least a type is registered", getCurrentTypes().length > 0); Query query = queryStringQuery("/foo*bar/").defaultField(STRING_FIELD_NAME) diff --git a/core/src/test/java/org/elasticsearch/index/query/SimpleQueryParserTests.java b/core/src/test/java/org/elasticsearch/index/query/SimpleQueryParserTests.java index 3c9b0f29d75..22fca23c69a 100644 --- a/core/src/test/java/org/elasticsearch/index/query/SimpleQueryParserTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/SimpleQueryParserTests.java @@ -20,10 +20,13 @@ package org.elasticsearch.index.query; import org.apache.lucene.analysis.Analyzer; +import org.apache.lucene.analysis.MockSynonymAnalyzer; import org.apache.lucene.analysis.standard.StandardAnalyzer; import org.apache.lucene.index.Term; import org.apache.lucene.search.BooleanClause; import org.apache.lucene.search.BooleanQuery; +import org.apache.lucene.search.GraphQuery; +import org.apache.lucene.search.PhraseQuery; import org.apache.lucene.search.PrefixQuery; import org.apache.lucene.search.Query; import org.apache.lucene.search.SynonymQuery; @@ -113,6 +116,53 @@ public class SimpleQueryParserTests extends ESTestCase { } } + public void testAnalyzerWithGraph() { + SimpleQueryParser.Settings settings = new SimpleQueryParser.Settings(); + settings.analyzeWildcard(true); + Map weights = new HashMap<>(); + weights.put("field1", 1.0f); + SimpleQueryParser parser = new MockSimpleQueryParser(new MockSynonymAnalyzer(), weights, -1, settings); + + for (Operator op : Operator.values()) { + BooleanClause.Occur defaultOp = op.toBooleanClauseOccur(); + parser.setDefaultOperator(defaultOp); + + // non-phrase won't detect multi-word synonym because of whitespace splitting + Query query = parser.parse("guinea pig"); + + Query expectedQuery = new BooleanQuery.Builder() + .add(new BooleanClause(new TermQuery(new Term("field1", "guinea")), defaultOp)) + .add(new BooleanClause(new TermQuery(new Term("field1", "pig")), defaultOp)) + .build(); + assertThat(query, equalTo(expectedQuery)); + + // phrase will pick it up + query = parser.parse("\"guinea pig\""); + + expectedQuery = new GraphQuery( + new PhraseQuery("field1", "guinea", "pig"), + new TermQuery(new Term("field1", "cavy"))); + + assertThat(query, equalTo(expectedQuery)); + + // phrase with slop + query = parser.parse("big \"guinea pig\"~2"); + + expectedQuery = new BooleanQuery.Builder() + .add(new BooleanClause(new TermQuery(new Term("field1", "big")), defaultOp)) + .add(new BooleanClause(new GraphQuery( + new PhraseQuery.Builder() + .add(new Term("field1", "guinea")) + .add(new Term("field1", "pig")) + .setSlop(2) + .build(), + new TermQuery(new Term("field1", "cavy"))), defaultOp)) + .build(); + + assertThat(query, equalTo(expectedQuery)); + } + } + public void testQuoteFieldSuffix() { SimpleQueryParser.Settings sqpSettings = new SimpleQueryParser.Settings(); sqpSettings.quoteFieldSuffix(".quote"); diff --git a/core/src/test/java/org/elasticsearch/search/query/QueryStringIT.java b/core/src/test/java/org/elasticsearch/search/query/QueryStringIT.java index c003038c7c2..34ba1278538 100644 --- a/core/src/test/java/org/elasticsearch/search/query/QueryStringIT.java +++ b/core/src/test/java/org/elasticsearch/search/query/QueryStringIT.java @@ -19,16 +19,26 @@ package org.elasticsearch.search.query; +import static org.elasticsearch.index.query.QueryBuilders.queryStringQuery; +import static org.elasticsearch.test.StreamsUtils.copyToStringFromClasspath; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoSearchHits; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchHits; +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; + import org.apache.lucene.util.LuceneTestCase; import org.elasticsearch.ExceptionsHelper; import org.elasticsearch.action.admin.indices.create.CreateIndexRequestBuilder; import org.elasticsearch.action.index.IndexRequestBuilder; import org.elasticsearch.action.search.SearchResponse; -import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.index.query.Operator; +import org.elasticsearch.index.query.QueryBuilders; import org.elasticsearch.index.query.QueryStringQueryBuilder; import org.elasticsearch.search.SearchHit; import org.elasticsearch.search.SearchHits; @@ -41,22 +51,6 @@ import java.util.HashSet; import java.util.List; import java.util.Set; -import static org.elasticsearch.index.query.QueryBuilders.queryStringQuery; -import static org.elasticsearch.test.StreamsUtils.copyToStringFromClasspath; -import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; -import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertFirstHit; -import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount; -import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoFailures; -import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchHits; -import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSecondHit; -import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.hasId; -import static org.hamcrest.Matchers.containsInAnyOrder; -import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.lessThan; -import static org.hamcrest.Matchers.notNullValue; -import static org.hamcrest.Matchers.nullValue; - public class QueryStringIT extends ESIntegTestCase { @Before @@ -263,6 +257,92 @@ public class QueryStringIT extends ESIntegTestCase { containsString("unit [D] not supported for date math [-2D]")); } + private void setupIndexWithGraph(String index) throws Exception { + CreateIndexRequestBuilder builder = prepareCreate(index).setSettings( + Settings.builder() + .put(indexSettings()) + .put("index.analysis.filter.graphsyns.type", "synonym_graph") + .putArray("index.analysis.filter.graphsyns.synonyms", "wtf, what the fudge", "foo, bar baz") + .put("index.analysis.analyzer.lower_graphsyns.type", "custom") + .put("index.analysis.analyzer.lower_graphsyns.tokenizer", "standard") + .putArray("index.analysis.analyzer.lower_graphsyns.filter", "lowercase", "graphsyns") + ); + + XContentBuilder mapping = XContentFactory.jsonBuilder().startObject().startObject(index).startObject("properties") + .startObject("field").field("type", "text").endObject().endObject().endObject().endObject(); + + assertAcked(builder.addMapping(index, mapping)); + ensureGreen(); + + List builders = new ArrayList<>(); + builders.add(client().prepareIndex(index, index, "1").setSource("field", "say wtf happened foo")); + builders.add(client().prepareIndex(index, index, "2").setSource("field", "bar baz what the fudge man")); + builders.add(client().prepareIndex(index, index, "3").setSource("field", "wtf")); + builders.add(client().prepareIndex(index, index, "4").setSource("field", "what is the name for fudge")); + builders.add(client().prepareIndex(index, index, "5").setSource("field", "bar two three")); + builders.add(client().prepareIndex(index, index, "6").setSource("field", "bar baz two three")); + + indexRandom(true, false, builders); + } + + public void testGraphQueries() throws Exception { + String index = "graph_test_index"; + setupIndexWithGraph(index); + + // phrase + SearchResponse searchResponse = client().prepareSearch(index).setQuery( + QueryBuilders.queryStringQuery("\"foo two three\"") + .defaultField("field") + .analyzer("lower_graphsyns")).get(); + + assertHitCount(searchResponse, 1L); + assertSearchHits(searchResponse, "6"); + + // and + searchResponse = client().prepareSearch(index).setQuery( + QueryBuilders.queryStringQuery("say what the fudge") + .defaultField("field") + .splitOnWhitespace(false) + .defaultOperator(Operator.AND) + .analyzer("lower_graphsyns")).get(); + + assertHitCount(searchResponse, 1L); + assertSearchHits(searchResponse, "1"); + + // and, split on whitespace means we should not recognize the multi-word synonym + searchResponse = client().prepareSearch(index).setQuery( + QueryBuilders.queryStringQuery("say what the fudge") + .defaultField("field") + .splitOnWhitespace(true) + .defaultOperator(Operator.AND) + .analyzer("lower_graphsyns")).get(); + + assertNoSearchHits(searchResponse); + + // or + searchResponse = client().prepareSearch(index).setQuery( + QueryBuilders.queryStringQuery("three what the fudge foo") + .defaultField("field") + .splitOnWhitespace(false) + .defaultOperator(Operator.OR) + .analyzer("lower_graphsyns")).get(); + + assertHitCount(searchResponse, 6L); + assertSearchHits(searchResponse, "1", "2", "3", "4", "5", "6"); + + // min should match + searchResponse = client().prepareSearch(index).setQuery( + QueryBuilders.queryStringQuery("three what the fudge foo") + .defaultField("field") + .splitOnWhitespace(false) + .defaultOperator(Operator.OR) + .analyzer("lower_graphsyns") + .minimumShouldMatch("80%")).get(); + + assertHitCount(searchResponse, 3L); + assertSearchHits(searchResponse, "1", "2", "6"); + } + private void assertHits(SearchHits hits, String... ids) { assertThat(hits.totalHits(), equalTo((long) ids.length)); Set hitIds = new HashSet<>(); diff --git a/docs/reference/analysis/tokenfilters/synonym-graph-tokenfilter.asciidoc b/docs/reference/analysis/tokenfilters/synonym-graph-tokenfilter.asciidoc index 16758ad6ad2..6f15b27c379 100644 --- a/docs/reference/analysis/tokenfilters/synonym-graph-tokenfilter.asciidoc +++ b/docs/reference/analysis/tokenfilters/synonym-graph-tokenfilter.asciidoc @@ -19,15 +19,6 @@ only. If you want to apply synonyms during indexing please use the standard <>. =============================== -["NOTE",id="synonym-graph-query-note"] -=============================== -The graph token stream created by this token filter requires special -query handling. Currently only the <> and -<> queries can do this. Using -it with any other type of analyzed query will potentially result in -incorrect search results. -=============================== - Synonyms are configured using a configuration file. Here is an example: From 8015fbbf257c7e55210f13cb5dbc311fcf9feaf1 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Wed, 11 Jan 2017 11:19:46 -0800 Subject: [PATCH 07/10] Make s3 repository sensitive settings use secure settings (#22479) * Settings: Make s3 repository sensitive settings use secure settings This change converts repository-s3 to use the new secure settings. In order to support the multiple ways we allow aws creds to be configured, it also moves the main methods for the keystore wrapper into a SecureSettings interface, in order to allow settings prefixing to work. --- .../elasticsearch/bootstrap/Bootstrap.java | 13 +- .../settings/AddStringKeyStoreCommand.java | 2 +- .../common/settings/KeyStoreWrapper.java | 18 +- .../common/settings/SecureSetting.java | 64 ++++-- .../common/settings/SecureSettings.java | 38 ++++ .../common/settings/SecureString.java | 3 +- .../common/settings/Setting.java | 5 +- .../common/settings/Settings.java | 70 +++++-- .../settings/KeyStoreCommandTestCase.java | 4 +- .../elasticsearch/cloud/aws/AwsS3Service.java | 37 ++-- .../cloud/aws/InternalAwsS3Service.java | 41 ++-- .../repositories/s3/S3Repository.java | 15 +- .../cloud/aws/AwsS3ServiceImplTests.java | 198 ++++++++++++++++-- .../repositories/s3/S3RepositoryTests.java | 22 +- .../test/repository_s3/20_repository.yaml | 5 + .../common/settings/MockSecureSettings.java | 54 +++++ .../org/elasticsearch/test/ESTestCase.java | 3 +- 17 files changed, 477 insertions(+), 115 deletions(-) create mode 100644 core/src/main/java/org/elasticsearch/common/settings/SecureSettings.java create mode 100644 test/framework/src/main/java/org/elasticsearch/common/settings/MockSecureSettings.java diff --git a/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java b/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java index 42be65ec22c..33f3f922fa4 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java @@ -40,6 +40,8 @@ import org.elasticsearch.common.logging.LogConfigurator; import org.elasticsearch.common.logging.Loggers; import org.elasticsearch.common.network.IfConfig; import org.elasticsearch.common.settings.KeyStoreWrapper; +import org.elasticsearch.common.settings.SecureSetting; +import org.elasticsearch.common.settings.SecureSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.transport.BoundTransportAddress; import org.elasticsearch.env.Environment; @@ -227,7 +229,7 @@ final class Bootstrap { }; } - private static KeyStoreWrapper loadKeyStore(Environment initialEnv) throws BootstrapException { + private static SecureSettings loadSecureSettings(Environment initialEnv) throws BootstrapException { final KeyStoreWrapper keystore; try { keystore = KeyStoreWrapper.load(initialEnv.configFile()); @@ -246,16 +248,17 @@ final class Bootstrap { return keystore; } + private static Environment createEnvironment(boolean foreground, Path pidFile, - KeyStoreWrapper keystore, Settings initialSettings) { + SecureSettings secureSettings, Settings initialSettings) { Terminal terminal = foreground ? Terminal.DEFAULT : null; Settings.Builder builder = Settings.builder(); if (pidFile != null) { builder.put(Environment.PIDFILE_SETTING.getKey(), pidFile); } builder.put(initialSettings); - if (keystore != null) { - builder.setKeyStore(keystore); + if (secureSettings != null) { + builder.setSecureSettings(secureSettings); } return InternalSettingsPreparer.prepareEnvironment(builder.build(), terminal, Collections.emptyMap()); } @@ -297,7 +300,7 @@ final class Bootstrap { INSTANCE = new Bootstrap(); - final KeyStoreWrapper keystore = loadKeyStore(initialEnv); + final SecureSettings keystore = loadSecureSettings(initialEnv); Environment environment = createEnvironment(foreground, pidFile, keystore, initialEnv.settings()); try { LogConfigurator.configure(environment); diff --git a/core/src/main/java/org/elasticsearch/common/settings/AddStringKeyStoreCommand.java b/core/src/main/java/org/elasticsearch/common/settings/AddStringKeyStoreCommand.java index e684e925057..7ac78c15b9a 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/AddStringKeyStoreCommand.java +++ b/core/src/main/java/org/elasticsearch/common/settings/AddStringKeyStoreCommand.java @@ -80,7 +80,7 @@ class AddStringKeyStoreCommand extends EnvironmentAwareCommand { } try { - keystore.setStringSetting(setting, value); + keystore.setString(setting, value); } catch (IllegalArgumentException e) { throw new UserException(ExitCodes.DATA_ERROR, "String value must contain only ASCII"); } diff --git a/core/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java b/core/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java index 7d0e5a8212b..f77b63d88fa 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java +++ b/core/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java @@ -54,6 +54,7 @@ import org.apache.lucene.store.IndexInput; import org.apache.lucene.store.IndexOutput; import org.apache.lucene.store.SimpleFSDirectory; import org.apache.lucene.util.SetOnce; +import org.elasticsearch.ElasticsearchException; /** * A wrapper around a Java KeyStore which provides supplements the keystore with extra metadata. @@ -64,7 +65,7 @@ import org.apache.lucene.util.SetOnce; * in a single thread. Once decrypted, keys may be read with the wrapper in * multiple threads. */ -public class KeyStoreWrapper implements Closeable { +public class KeyStoreWrapper implements SecureSettings { /** The name of the keystore file to read and write. */ private static final String KEYSTORE_FILENAME = "elasticsearch.keystore"; @@ -159,7 +160,7 @@ public class KeyStoreWrapper implements Closeable { } } - /** Returns true iff {@link #decrypt(char[])} has been called. */ + @Override public boolean isLoaded() { return keystore.get() != null; } @@ -225,20 +226,25 @@ public class KeyStoreWrapper implements Closeable { } } - /** Returns the names of all settings in this keystore. */ public Set getSettings() { return settingNames; } + @Override + public boolean hasSetting(String setting) { + return settingNames.contains(setting); + } + // TODO: make settings accessible only to code that registered the setting /** Retrieve a string setting. The {@link SecureString} should be closed once it is used. */ - SecureString getStringSetting(String setting) throws GeneralSecurityException { + @Override + public SecureString getString(String setting) throws GeneralSecurityException { KeyStore.Entry entry = keystore.get().getEntry(setting, keystorePassword.get()); if (entry instanceof KeyStore.SecretKeyEntry == false) { throw new IllegalStateException("Secret setting " + setting + " is not a string"); } // TODO: only allow getting a setting once? - KeyStore.SecretKeyEntry secretKeyEntry = (KeyStore.SecretKeyEntry)entry; + KeyStore.SecretKeyEntry secretKeyEntry = (KeyStore.SecretKeyEntry) entry; PBEKeySpec keySpec = (PBEKeySpec) secretFactory.getKeySpec(secretKeyEntry.getSecretKey(), PBEKeySpec.class); SecureString value = new SecureString(keySpec.getPassword()); keySpec.clearPassword(); @@ -250,7 +256,7 @@ public class KeyStoreWrapper implements Closeable { * * @throws IllegalArgumentException if the value is not ASCII */ - void setStringSetting(String setting, char[] value) throws GeneralSecurityException { + void setString(String setting, char[] value) throws GeneralSecurityException { if (ASCII_ENCODER.canEncode(CharBuffer.wrap(value)) == false) { throw new IllegalArgumentException("Value must be ascii"); } diff --git a/core/src/main/java/org/elasticsearch/common/settings/SecureSetting.java b/core/src/main/java/org/elasticsearch/common/settings/SecureSetting.java index fb4073482f3..1a69896dcc1 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/SecureSetting.java +++ b/core/src/main/java/org/elasticsearch/common/settings/SecureSetting.java @@ -25,6 +25,9 @@ import java.util.HashSet; import java.util.Objects; import java.util.Set; +import org.elasticsearch.common.Strings; +import org.elasticsearch.common.util.ArrayUtils; + /** * A secure setting. @@ -36,8 +39,16 @@ public abstract class SecureSetting extends Setting { Arrays.asList(Property.Deprecated, Property.Shared) ); - private SecureSetting(String key, Setting.Property... properties) { - super(key, (String)null, null, properties); + private static final Property[] FIXED_PROPERTIES = { + Property.NodeScope + }; + + private static final Property[] LEGACY_PROPERTIES = { + Property.NodeScope, Property.Deprecated, Property.Filtered + }; + + private SecureSetting(String key, Property... properties) { + super(key, (String)null, null, ArrayUtils.concat(properties, FIXED_PROPERTIES, Property.class)); assert assertAllowedProperties(properties); } @@ -65,22 +76,28 @@ public abstract class SecureSetting extends Setting { throw new UnsupportedOperationException("secure settings are not strings"); } + @Override + public boolean exists(Settings settings) { + final SecureSettings secureSettings = settings.getSecureSettings(); + return secureSettings != null && secureSettings.hasSetting(getKey()); + } + @Override public T get(Settings settings) { checkDeprecation(settings); - final KeyStoreWrapper keystore = Objects.requireNonNull(settings.getKeyStore()); - if (keystore.getSettings().contains(getKey()) == false) { + final SecureSettings secureSettings = settings.getSecureSettings(); + if (secureSettings == null || secureSettings.hasSetting(getKey()) == false) { return getFallback(settings); } try { - return getSecret(keystore); + return getSecret(secureSettings); } catch (GeneralSecurityException e) { throw new RuntimeException("failed to read secure setting " + getKey(), e); } } /** Returns the secret setting from the keyStoreReader store. */ - abstract T getSecret(KeyStoreWrapper keystore) throws GeneralSecurityException; + abstract T getSecret(SecureSettings secureSettings) throws GeneralSecurityException; /** Returns the value from a fallback setting. Returns null if no fallback exists. */ abstract T getFallback(Settings settings); @@ -92,18 +109,41 @@ public abstract class SecureSetting extends Setting { * * This may be any sensitive string, e.g. a username, a password, an auth token, etc. */ - public static SecureSetting stringSetting(String name, Setting fallback, Property... properties) { + public static SecureSetting secureString(String name, SecureSetting fallback, + boolean allowLegacy, Property... properties) { + final Setting legacy; + if (allowLegacy) { + Property[] legacyProperties = ArrayUtils.concat(properties, LEGACY_PROPERTIES, Property.class); + legacy = Setting.simpleString(name, legacyProperties); + } else { + legacy = null; + } return new SecureSetting(name, properties) { @Override - protected SecureString getSecret(KeyStoreWrapper keystore) throws GeneralSecurityException { - return keystore.getStringSetting(getKey()); + protected SecureString getSecret(SecureSettings secureSettings) throws GeneralSecurityException { + return secureSettings.getString(getKey()); } @Override SecureString getFallback(Settings settings) { - if (fallback != null) { - return new SecureString(fallback.get(settings).toCharArray()); + if (legacy != null && legacy.exists(settings)) { + return new SecureString(legacy.get(settings).toCharArray()); } - return null; + if (fallback != null) { + return fallback.get(settings); + } + return new SecureString(new char[0]); // this means "setting does not exist" + } + @Override + protected void checkDeprecation(Settings settings) { + super.checkDeprecation(settings); + if (legacy != null) { + legacy.checkDeprecation(settings); + } + } + @Override + public boolean exists(Settings settings) { + // handle legacy, which is internal to this setting + return super.exists(settings) || legacy != null && legacy.exists(settings); } }; } diff --git a/core/src/main/java/org/elasticsearch/common/settings/SecureSettings.java b/core/src/main/java/org/elasticsearch/common/settings/SecureSettings.java new file mode 100644 index 00000000000..d810e1ec4da --- /dev/null +++ b/core/src/main/java/org/elasticsearch/common/settings/SecureSettings.java @@ -0,0 +1,38 @@ +/* + * 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.common.settings; + +import java.io.Closeable; +import java.security.GeneralSecurityException; + +/** + * An accessor for settings which are securely stored. See {@link SecureSetting}. + */ +public interface SecureSettings extends Closeable { + + /** Returns true iff the settings are loaded and retrievable. */ + boolean isLoaded(); + + /** Returns true iff the given setting exists in this secure settings. */ + boolean hasSetting(String setting); + + /** Return a string setting. The {@link SecureString} should be closed once it is used. */ + SecureString getString(String setting) throws GeneralSecurityException; +} diff --git a/core/src/main/java/org/elasticsearch/common/settings/SecureString.java b/core/src/main/java/org/elasticsearch/common/settings/SecureString.java index 9ce820952e1..3d423dca637 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/SecureString.java +++ b/core/src/main/java/org/elasticsearch/common/settings/SecureString.java @@ -19,13 +19,14 @@ package org.elasticsearch.common.settings; +import java.io.Closeable; import java.util.Arrays; import java.util.Objects; /** * A String implementations which allows clearing the underlying char array. */ -public final class SecureString implements CharSequence, AutoCloseable { +public final class SecureString implements CharSequence, Closeable { private char[] chars; diff --git a/core/src/main/java/org/elasticsearch/common/settings/Setting.java b/core/src/main/java/org/elasticsearch/common/settings/Setting.java index d3bc4ebaf0b..5f067f27e90 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/core/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -122,7 +122,8 @@ public class Setting extends ToXContentToBytes { private Setting(Key key, @Nullable Setting fallbackSetting, Function defaultValue, Function parser, Property... properties) { - assert parser.apply(defaultValue.apply(Settings.EMPTY)) != null || this.isGroupSetting(): "parser returned null"; + assert this instanceof SecureSetting || parser.apply(defaultValue.apply(Settings.EMPTY)) != null || this.isGroupSetting() + : "parser returned null"; this.key = key; this.fallbackSetting = fallbackSetting; this.defaultValue = defaultValue; @@ -293,7 +294,7 @@ public class Setting extends ToXContentToBytes { * Returns true iff this setting is present in the given settings object. Otherwise false */ public boolean exists(Settings settings) { - return settings.contains(getKey()); + return settings.getAsMap().containsKey(getKey()); } /** diff --git a/core/src/main/java/org/elasticsearch/common/settings/Settings.java b/core/src/main/java/org/elasticsearch/common/settings/Settings.java index dbd305bb890..ef9ff00a1f0 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/Settings.java +++ b/core/src/main/java/org/elasticsearch/common/settings/Settings.java @@ -43,6 +43,7 @@ import java.io.InputStreamReader; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.security.GeneralSecurityException; import java.util.AbstractMap; import java.util.AbstractSet; import java.util.ArrayList; @@ -80,26 +81,21 @@ public final class Settings implements ToXContent { /** The raw settings from the full key to raw string value. */ private Map settings; - /** The keystore storage associated with these settings. */ - private KeyStoreWrapper keystore; + /** The secure settings storage associated with these settings. */ + private SecureSettings secureSettings; - Settings(Map settings, KeyStoreWrapper keystore) { + Settings(Map settings, SecureSettings secureSettings) { // we use a sorted map for consistent serialization when using getAsMap() this.settings = Collections.unmodifiableSortedMap(new TreeMap<>(settings)); - this.keystore = keystore; + this.secureSettings = secureSettings; } /** - * Retrieve the keystore that contains secure settings. + * Retrieve the secure settings in these settings. */ - KeyStoreWrapper getKeyStore() { + SecureSettings getSecureSettings() { // pkg private so it can only be accessed by local subclasses of SecureSetting - return keystore; - } - - /** Returns true if the setting exists, false otherwise. */ - public boolean contains(String key) { - return settings.containsKey(key) || keystore != null && keystore.getSettings().contains(key); + return secureSettings; } /** @@ -205,10 +201,10 @@ public final class Settings implements ToXContent { /** * A settings that are filtered (and key is removed) with the specified prefix. - * Secure settings may not be access through the prefixed settings. */ public Settings getByPrefix(String prefix) { - return new Settings(new FilteredMap(this.settings, (k) -> k.startsWith(prefix), prefix), null); + return new Settings(new FilteredMap(this.settings, (k) -> k.startsWith(prefix), prefix), + secureSettings == null ? null : new PrefixedSecureSettings(secureSettings, prefix)); } /** @@ -478,7 +474,7 @@ public final class Settings implements ToXContent { } Map retVal = new LinkedHashMap<>(); for (Map.Entry> entry : map.entrySet()) { - retVal.put(entry.getKey(), new Settings(Collections.unmodifiableMap(entry.getValue()), keystore)); + retVal.put(entry.getKey(), new Settings(Collections.unmodifiableMap(entry.getValue()), secureSettings)); } return Collections.unmodifiableMap(retVal); } @@ -613,7 +609,7 @@ public final class Settings implements ToXContent { // we use a sorted map for consistent serialization when using getAsMap() private final Map map = new TreeMap<>(); - private SetOnce keystore = new SetOnce<>(); + private SetOnce secureSettings = new SetOnce<>(); private Builder() { @@ -637,12 +633,12 @@ public final class Settings implements ToXContent { return map.get(key); } - /** Sets the secret store for these settings. */ - public void setKeyStore(KeyStoreWrapper keystore) { - if (keystore.isLoaded()) { - throw new IllegalStateException("The keystore wrapper must already be loaded"); + public Builder setSecureSettings(SecureSettings secureSettings) { + if (secureSettings.isLoaded() == false) { + throw new IllegalStateException("Secure settings must already be loaded"); } - this.keystore.set(keystore); + this.secureSettings.set(secureSettings); + return this; } /** @@ -1051,7 +1047,7 @@ public final class Settings implements ToXContent { * set on this builder. */ public Settings build() { - return new Settings(map, keystore.get()); + return new Settings(map, secureSettings.get()); } } @@ -1171,4 +1167,34 @@ public final class Settings implements ToXContent { return size; } } + + private static class PrefixedSecureSettings implements SecureSettings { + private SecureSettings delegate; + private String prefix; + + PrefixedSecureSettings(SecureSettings delegate, String prefix) { + this.delegate = delegate; + this.prefix = prefix; + } + + @Override + public boolean isLoaded() { + return delegate.isLoaded(); + } + + @Override + public boolean hasSetting(String setting) { + return delegate.hasSetting(prefix + setting); + } + + @Override + public SecureString getString(String setting) throws GeneralSecurityException{ + return delegate.getString(prefix + setting); + } + + @Override + public void close() throws IOException { + delegate.close(); + } + } } diff --git a/core/src/test/java/org/elasticsearch/common/settings/KeyStoreCommandTestCase.java b/core/src/test/java/org/elasticsearch/common/settings/KeyStoreCommandTestCase.java index 1e02ae6a202..1e4d24a344e 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/KeyStoreCommandTestCase.java +++ b/core/src/test/java/org/elasticsearch/common/settings/KeyStoreCommandTestCase.java @@ -75,7 +75,7 @@ public abstract class KeyStoreCommandTestCase extends CommandTestCase { KeyStoreWrapper keystore = KeyStoreWrapper.create(password.toCharArray()); assertEquals(0, settings.length % 2); for (int i = 0; i < settings.length; i += 2) { - keystore.setStringSetting(settings[i], settings[i + 1].toCharArray()); + keystore.setString(settings[i], settings[i + 1].toCharArray()); } keystore.save(env.configFile()); return keystore; @@ -92,6 +92,6 @@ public abstract class KeyStoreCommandTestCase extends CommandTestCase { } void assertSecureString(KeyStoreWrapper keystore, String setting, String value) throws Exception { - assertEquals(value, keystore.getStringSetting(setting).toString()); + assertEquals(value, keystore.getString(setting).toString()); } } diff --git a/plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/AwsS3Service.java b/plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/AwsS3Service.java index ddaa45d15fd..e02556ccc2b 100644 --- a/plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/AwsS3Service.java +++ b/plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/AwsS3Service.java @@ -23,6 +23,8 @@ import com.amazonaws.ClientConfiguration; import com.amazonaws.Protocol; import com.amazonaws.services.s3.AmazonS3; import org.elasticsearch.common.component.LifecycleComponent; +import org.elasticsearch.common.settings.SecureSetting; +import org.elasticsearch.common.settings.SecureString; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Setting.Property; import org.elasticsearch.common.settings.Settings; @@ -39,13 +41,12 @@ public interface AwsS3Service extends LifecycleComponent { /** * cloud.aws.access_key: AWS Access key. Shared with discovery-ec2 plugin */ - Setting KEY_SETTING = - Setting.simpleString("cloud.aws.access_key", Property.NodeScope, Property.Filtered, Property.Shared); + SecureSetting KEY_SETTING = SecureSetting.secureString("cloud.aws.access_key", null, true, Property.Shared); + /** * cloud.aws.secret_key: AWS Secret key. Shared with discovery-ec2 plugin */ - Setting SECRET_SETTING = - Setting.simpleString("cloud.aws.secret_key", Property.NodeScope, Property.Filtered, Property.Shared); + SecureSetting SECRET_SETTING = SecureSetting.secureString("cloud.aws.secret_key", null, true, Property.Shared); /** * cloud.aws.protocol: Protocol for AWS API: http or https. Defaults to https. Shared with discovery-ec2 plugin */ @@ -63,12 +64,14 @@ public interface AwsS3Service extends LifecycleComponent { /** * cloud.aws.proxy.username: In case of proxy with auth, define the username. Shared with discovery-ec2 plugin */ - Setting PROXY_USERNAME_SETTING = Setting.simpleString("cloud.aws.proxy.username", Property.NodeScope, Property.Shared); + SecureSetting PROXY_USERNAME_SETTING = + SecureSetting.secureString("cloud.aws.proxy.username", null, true, Property.Shared); + /** * cloud.aws.proxy.password: In case of proxy with auth, define the password. Shared with discovery-ec2 plugin */ - Setting PROXY_PASSWORD_SETTING = - Setting.simpleString("cloud.aws.proxy.password", Property.NodeScope, Property.Filtered, Property.Shared); + SecureSetting PROXY_PASSWORD_SETTING = + SecureSetting.secureString("cloud.aws.proxy.password", null, true, Property.Shared); /** * cloud.aws.signer: If you are using an old AWS API version, you can define a Signer. Shared with discovery-ec2 plugin */ @@ -92,16 +95,13 @@ public interface AwsS3Service extends LifecycleComponent { * cloud.aws.s3.access_key: AWS Access key specific for S3 API calls. Defaults to cloud.aws.access_key. * @see AwsS3Service#KEY_SETTING */ - Setting KEY_SETTING = - new Setting<>("cloud.aws.s3.access_key", AwsS3Service.KEY_SETTING, Function.identity(), - Property.NodeScope, Property.Filtered); + SecureSetting KEY_SETTING = SecureSetting.secureString("cloud.aws.s3.access_key", AwsS3Service.KEY_SETTING, true); /** * cloud.aws.s3.secret_key: AWS Secret key specific for S3 API calls. Defaults to cloud.aws.secret_key. * @see AwsS3Service#SECRET_SETTING */ - Setting SECRET_SETTING = - new Setting<>("cloud.aws.s3.secret_key", AwsS3Service.SECRET_SETTING, Function.identity(), - Property.NodeScope, Property.Filtered); + SecureSetting SECRET_SETTING = SecureSetting.secureString("cloud.aws.s3.secret_key", + AwsS3Service.SECRET_SETTING, true); /** * cloud.aws.s3.protocol: Protocol for AWS API specific for S3 API calls: http or https. Defaults to cloud.aws.protocol. * @see AwsS3Service#PROTOCOL_SETTING @@ -128,17 +128,16 @@ public interface AwsS3Service extends LifecycleComponent { * Defaults to cloud.aws.proxy.username. * @see AwsS3Service#PROXY_USERNAME_SETTING */ - Setting PROXY_USERNAME_SETTING = - new Setting<>("cloud.aws.s3.proxy.username", AwsS3Service.PROXY_USERNAME_SETTING, Function.identity(), - Property.NodeScope); + SecureSetting PROXY_USERNAME_SETTING = + SecureSetting.secureString("cloud.aws.s3.proxy.username", AwsS3Service.PROXY_USERNAME_SETTING, true); /** * cloud.aws.s3.proxy.password: In case of proxy with auth, define the password specific for S3 API calls. * Defaults to cloud.aws.proxy.password. * @see AwsS3Service#PROXY_PASSWORD_SETTING */ - Setting PROXY_PASSWORD_SETTING = - new Setting<>("cloud.aws.s3.proxy.password", AwsS3Service.PROXY_PASSWORD_SETTING, Function.identity(), - Property.NodeScope, Property.Filtered); + SecureSetting PROXY_PASSWORD_SETTING = + SecureSetting.secureString("cloud.aws.s3.proxy.password", AwsS3Service.PROXY_PASSWORD_SETTING, true); + /** * cloud.aws.s3.signer: If you are using an old AWS API version, you can define a Signer. Specific for S3 API calls. * Defaults to cloud.aws.signer. diff --git a/plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/InternalAwsS3Service.java b/plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/InternalAwsS3Service.java index 068083577cc..bd2d99cdc50 100644 --- a/plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/InternalAwsS3Service.java +++ b/plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/InternalAwsS3Service.java @@ -30,13 +30,17 @@ import com.amazonaws.services.s3.AmazonS3; import com.amazonaws.services.s3.AmazonS3Client; import com.amazonaws.services.s3.S3ClientOptions; import org.apache.logging.log4j.Logger; +import org.apache.lucene.util.IOUtils; import org.elasticsearch.ElasticsearchException; import org.elasticsearch.common.Strings; import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.component.AbstractLifecycleComponent; +import org.elasticsearch.common.settings.SecureString; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.repositories.s3.S3Repository; +import java.io.IOException; +import java.io.UncheckedIOException; import java.util.HashMap; import java.util.Map; @@ -93,14 +97,15 @@ public class InternalAwsS3Service extends AbstractLifecycleComponent implements String proxyHost = CLOUD_S3.PROXY_HOST_SETTING.get(settings); if (Strings.hasText(proxyHost)) { Integer proxyPort = CLOUD_S3.PROXY_PORT_SETTING.get(settings); - String proxyUsername = CLOUD_S3.PROXY_USERNAME_SETTING.get(settings); - String proxyPassword = CLOUD_S3.PROXY_PASSWORD_SETTING.get(settings); + try (SecureString proxyUsername = CLOUD_S3.PROXY_USERNAME_SETTING.get(settings); + SecureString proxyPassword = CLOUD_S3.PROXY_PASSWORD_SETTING.get(settings)) { - clientConfiguration - .withProxyHost(proxyHost) - .withProxyPort(proxyPort) - .withProxyUsername(proxyUsername) - .withProxyPassword(proxyPassword); + clientConfiguration + .withProxyHost(proxyHost) + .withProxyPort(proxyPort) + .withProxyUsername(proxyUsername.toString()) + .withProxyPassword(proxyPassword.toString()); + } } if (maxRetries != null) { @@ -123,17 +128,19 @@ public class InternalAwsS3Service extends AbstractLifecycleComponent implements public static AWSCredentialsProvider buildCredentials(Logger logger, Settings settings, Settings repositorySettings) { AWSCredentialsProvider credentials; - String key = getValue(repositorySettings, settings, - S3Repository.Repository.KEY_SETTING, S3Repository.Repositories.KEY_SETTING); - String secret = getValue(repositorySettings, settings, - S3Repository.Repository.SECRET_SETTING, S3Repository.Repositories.SECRET_SETTING); + try (SecureString key = getValue(repositorySettings, settings, + S3Repository.Repository.KEY_SETTING, S3Repository.Repositories.KEY_SETTING); + SecureString secret = getValue(repositorySettings, settings, + S3Repository.Repository.SECRET_SETTING, S3Repository.Repositories.SECRET_SETTING)) { - if (key.isEmpty() && secret.isEmpty()) { - logger.debug("Using either environment variables, system properties or instance profile credentials"); - credentials = new DefaultAWSCredentialsProviderChain(); - } else { - logger.debug("Using basic key/secret credentials"); - credentials = new StaticCredentialsProvider(new BasicAWSCredentials(key, secret)); + if (key.length() == 0 && secret.length() == 0) { + // TODO: add deprecation, except for using instance profile + logger.debug("Using either environment variables, system properties or instance profile credentials"); + credentials = new DefaultAWSCredentialsProviderChain(); + } else { + logger.debug("Using basic key/secret credentials"); + credentials = new StaticCredentialsProvider(new BasicAWSCredentials(key.toString(), secret.toString())); + } } return credentials; diff --git a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java index 1330fa17f80..63973f106d4 100644 --- a/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java +++ b/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java @@ -28,6 +28,8 @@ import org.elasticsearch.cluster.metadata.RepositoryMetaData; import org.elasticsearch.common.Strings; import org.elasticsearch.common.blobstore.BlobPath; import org.elasticsearch.common.blobstore.BlobStore; +import org.elasticsearch.common.settings.SecureSetting; +import org.elasticsearch.common.settings.SecureString; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Setting.Property; import org.elasticsearch.common.settings.Settings; @@ -67,14 +69,14 @@ public class S3Repository extends BlobStoreRepository { * repositories.s3.access_key: AWS Access key specific for all S3 Repositories API calls. Defaults to cloud.aws.s3.access_key. * @see CLOUD_S3#KEY_SETTING */ - Setting KEY_SETTING = - new Setting<>("repositories.s3.access_key", CLOUD_S3.KEY_SETTING, Function.identity(), Property.NodeScope, Property.Filtered); + SecureSetting KEY_SETTING = SecureSetting.secureString("repositories.s3.access_key", CLOUD_S3.KEY_SETTING, true); + /** * repositories.s3.secret_key: AWS Secret key specific for all S3 Repositories API calls. Defaults to cloud.aws.s3.secret_key. * @see CLOUD_S3#SECRET_SETTING */ - Setting SECRET_SETTING = - new Setting<>("repositories.s3.secret_key", CLOUD_S3.SECRET_SETTING, Function.identity(), Property.NodeScope, Property.Filtered); + SecureSetting SECRET_SETTING = SecureSetting.secureString("repositories.s3.secret_key", CLOUD_S3.SECRET_SETTING, true); + /** * repositories.s3.region: Region specific for all S3 Repositories API calls. Defaults to cloud.aws.s3.region. * @see CLOUD_S3#REGION_SETTING @@ -179,12 +181,13 @@ public class S3Repository extends BlobStoreRepository { * access_key * @see Repositories#KEY_SETTING */ - Setting KEY_SETTING = Setting.simpleString("access_key"); + SecureSetting KEY_SETTING = SecureSetting.secureString("access_key", null, true); + /** * secret_key * @see Repositories#SECRET_SETTING */ - Setting SECRET_SETTING = Setting.simpleString("secret_key"); + SecureSetting SECRET_SETTING = SecureSetting.secureString("secret_key", null, true); /** * bucket * @see Repositories#BUCKET_SETTING diff --git a/plugins/repository-s3/src/test/java/org/elasticsearch/cloud/aws/AwsS3ServiceImplTests.java b/plugins/repository-s3/src/test/java/org/elasticsearch/cloud/aws/AwsS3ServiceImplTests.java index 2bd14847f7e..9553e83dd92 100644 --- a/plugins/repository-s3/src/test/java/org/elasticsearch/cloud/aws/AwsS3ServiceImplTests.java +++ b/plugins/repository-s3/src/test/java/org/elasticsearch/cloud/aws/AwsS3ServiceImplTests.java @@ -24,6 +24,7 @@ import com.amazonaws.Protocol; import com.amazonaws.auth.AWSCredentials; import com.amazonaws.auth.AWSCredentialsProvider; import com.amazonaws.auth.DefaultAWSCredentialsProviderChain; +import org.elasticsearch.common.settings.MockSecureSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.repositories.s3.S3Repository; import org.elasticsearch.test.ESTestCase; @@ -41,23 +42,123 @@ public class AwsS3ServiceImplTests extends ESTestCase { public void testAWSCredentialsWithElasticsearchAwsSettings() { Settings repositorySettings = generateRepositorySettings(null, null, "eu-central", null, null); - Settings settings = Settings.builder() - .put(AwsS3Service.KEY_SETTING.getKey(), "aws_key") - .put(AwsS3Service.SECRET_SETTING.getKey(), "aws_secret") - .build(); + MockSecureSettings secureSettings = new MockSecureSettings(); + secureSettings.setString(AwsS3Service.KEY_SETTING.getKey(), "aws_key"); + secureSettings.setString(AwsS3Service.SECRET_SETTING.getKey(), "aws_secret"); + Settings settings = Settings.builder().setSecureSettings(secureSettings).build(); launchAWSCredentialsWithElasticsearchSettingsTest(repositorySettings, settings, "aws_key", "aws_secret"); } public void testAWSCredentialsWithElasticsearchS3Settings() { Settings repositorySettings = generateRepositorySettings(null, null, "eu-central", null, null); - Settings settings = Settings.builder() - .put(AwsS3Service.CLOUD_S3.KEY_SETTING.getKey(), "s3_key") - .put(AwsS3Service.CLOUD_S3.SECRET_SETTING.getKey(), "s3_secret") - .build(); + MockSecureSettings secureSettings = new MockSecureSettings(); + secureSettings.setString(AwsS3Service.CLOUD_S3.KEY_SETTING.getKey(), "s3_key"); + secureSettings.setString(AwsS3Service.CLOUD_S3.SECRET_SETTING.getKey(), "s3_secret"); + Settings settings = Settings.builder().setSecureSettings(secureSettings).build(); launchAWSCredentialsWithElasticsearchSettingsTest(repositorySettings, settings, "s3_key", "s3_secret"); } public void testAWSCredentialsWithElasticsearchAwsAndS3Settings() { + Settings repositorySettings = generateRepositorySettings(null, null, "eu-central", null, null); + MockSecureSettings secureSettings = new MockSecureSettings(); + secureSettings.setString(AwsS3Service.KEY_SETTING.getKey(), "aws_key"); + secureSettings.setString(AwsS3Service.SECRET_SETTING.getKey(), "aws_secret"); + secureSettings.setString(AwsS3Service.CLOUD_S3.KEY_SETTING.getKey(), "s3_key"); + secureSettings.setString(AwsS3Service.CLOUD_S3.SECRET_SETTING.getKey(), "s3_secret"); + Settings settings = Settings.builder().setSecureSettings(secureSettings).build(); + launchAWSCredentialsWithElasticsearchSettingsTest(repositorySettings, settings, "s3_key", "s3_secret"); + } + + public void testAWSCredentialsWithElasticsearchRepositoriesSettings() { + Settings repositorySettings = generateRepositorySettings(null, null, "eu-central", null, null); + MockSecureSettings secureSettings = new MockSecureSettings(); + secureSettings.setString(S3Repository.Repositories.KEY_SETTING.getKey(), "repositories_key"); + secureSettings.setString(S3Repository.Repositories.SECRET_SETTING.getKey(), "repositories_secret"); + Settings settings = Settings.builder().setSecureSettings(secureSettings).build(); + launchAWSCredentialsWithElasticsearchSettingsTest(repositorySettings, settings, "repositories_key", "repositories_secret"); + } + + + public void testAWSCredentialsWithElasticsearchAwsAndRepositoriesSettings() { + Settings repositorySettings = generateRepositorySettings(null, null, "eu-central", null, null); + MockSecureSettings secureSettings = new MockSecureSettings(); + secureSettings.setString(AwsS3Service.KEY_SETTING.getKey(), "aws_key"); + secureSettings.setString(AwsS3Service.SECRET_SETTING.getKey(), "aws_secret"); + secureSettings.setString(S3Repository.Repositories.KEY_SETTING.getKey(), "repositories_key"); + secureSettings.setString(S3Repository.Repositories.SECRET_SETTING.getKey(), "repositories_secret"); + Settings settings = Settings.builder().setSecureSettings(secureSettings).build(); + launchAWSCredentialsWithElasticsearchSettingsTest(repositorySettings, settings, "repositories_key", "repositories_secret"); + } + + public void testAWSCredentialsWithElasticsearchAwsAndS3AndRepositoriesSettings() { + Settings repositorySettings = generateRepositorySettings(null, null, "eu-central", null, null); + MockSecureSettings secureSettings = new MockSecureSettings(); + secureSettings.setString(AwsS3Service.KEY_SETTING.getKey(), "aws_key"); + secureSettings.setString(AwsS3Service.SECRET_SETTING.getKey(), "aws_secret"); + secureSettings.setString(AwsS3Service.CLOUD_S3.KEY_SETTING.getKey(), "s3_key"); + secureSettings.setString(AwsS3Service.CLOUD_S3.SECRET_SETTING.getKey(), "s3_secret"); + secureSettings.setString(S3Repository.Repositories.KEY_SETTING.getKey(), "repositories_key"); + secureSettings.setString(S3Repository.Repositories.SECRET_SETTING.getKey(), "repositories_secret"); + Settings settings = Settings.builder().setSecureSettings(secureSettings).build(); + launchAWSCredentialsWithElasticsearchSettingsTest(repositorySettings, settings, "repositories_key", "repositories_secret"); + } + + public void testAWSCredentialsWithElasticsearchRepositoriesSettingsAndRepositorySettings() { + Settings repositorySettings = generateSecureRepositorySettings("repository_key", "repository_secret", "eu-central", null, null); + MockSecureSettings secureSettings = new MockSecureSettings(); + secureSettings.setString(S3Repository.Repositories.KEY_SETTING.getKey(), "repositories_key"); + secureSettings.setString(S3Repository.Repositories.SECRET_SETTING.getKey(), "repositories_secret"); + Settings settings = Settings.builder().setSecureSettings(secureSettings).build(); + launchAWSCredentialsWithElasticsearchSettingsTest(repositorySettings, settings, "repository_key", "repository_secret"); + } + + public void testAWSCredentialsWithElasticsearchAwsAndRepositoriesSettingsAndRepositorySettings() { + Settings repositorySettings = generateSecureRepositorySettings("repository_key", "repository_secret", "eu-central", null, null); + MockSecureSettings secureSettings = new MockSecureSettings(); + secureSettings.setString(AwsS3Service.KEY_SETTING.getKey(), "aws_key"); + secureSettings.setString(AwsS3Service.SECRET_SETTING.getKey(), "aws_secret"); + secureSettings.setString(S3Repository.Repositories.KEY_SETTING.getKey(), "repositories_key"); + secureSettings.setString(S3Repository.Repositories.SECRET_SETTING.getKey(), "repositories_secret"); + Settings settings = Settings.builder().setSecureSettings(secureSettings).build(); + launchAWSCredentialsWithElasticsearchSettingsTest(repositorySettings, settings, "repository_key", "repository_secret"); + } + + public void testAWSCredentialsWithElasticsearchAwsAndS3AndRepositoriesSettingsAndRepositorySettings() { + Settings repositorySettings = generateSecureRepositorySettings("repository_key", "repository_secret", "eu-central", null, null); + MockSecureSettings secureSettings = new MockSecureSettings(); + secureSettings.setString(AwsS3Service.KEY_SETTING.getKey(), "aws_key"); + secureSettings.setString(AwsS3Service.SECRET_SETTING.getKey(), "aws_secret"); + secureSettings.setString(AwsS3Service.CLOUD_S3.KEY_SETTING.getKey(), "s3_key"); + secureSettings.setString(AwsS3Service.CLOUD_S3.SECRET_SETTING.getKey(), "s3_secret"); + secureSettings.setString(S3Repository.Repositories.KEY_SETTING.getKey(), "repositories_key"); + secureSettings.setString(S3Repository.Repositories.SECRET_SETTING.getKey(), "repositories_secret"); + Settings settings = Settings.builder().setSecureSettings(secureSettings).build(); + launchAWSCredentialsWithElasticsearchSettingsTest(repositorySettings, settings, "repository_key", "repository_secret"); + } + + public void testAWSCredentialsWithElasticsearchAwsSettingsBackcompat() { + Settings repositorySettings = generateRepositorySettings(null, null, "eu-central", null, null); + Settings settings = Settings.builder() + .put(AwsS3Service.KEY_SETTING.getKey(), "aws_key") + .put(AwsS3Service.SECRET_SETTING.getKey(), "aws_secret") + .build(); + launchAWSCredentialsWithElasticsearchSettingsTest(repositorySettings, settings, "aws_key", "aws_secret"); + assertWarnings("[" + AwsS3Service.KEY_SETTING.getKey() + "] setting was deprecated", + "[" + AwsS3Service.SECRET_SETTING.getKey() + "] setting was deprecated"); + } + + public void testAWSCredentialsWithElasticsearchS3SettingsBackcompat() { + Settings repositorySettings = generateRepositorySettings(null, null, "eu-central", null, null); + Settings settings = Settings.builder() + .put(AwsS3Service.CLOUD_S3.KEY_SETTING.getKey(), "s3_key") + .put(AwsS3Service.CLOUD_S3.SECRET_SETTING.getKey(), "s3_secret") + .build(); + launchAWSCredentialsWithElasticsearchSettingsTest(repositorySettings, settings, "s3_key", "s3_secret"); + assertWarnings("[" + AwsS3Service.CLOUD_S3.KEY_SETTING.getKey() + "] setting was deprecated", + "[" + AwsS3Service.CLOUD_S3.SECRET_SETTING.getKey() + "] setting was deprecated"); + } + + public void testAWSCredentialsWithElasticsearchAwsAndS3SettingsBackcompat() { Settings repositorySettings = generateRepositorySettings(null, null, "eu-central", null, null); Settings settings = Settings.builder() .put(AwsS3Service.KEY_SETTING.getKey(), "aws_key") @@ -66,18 +167,22 @@ public class AwsS3ServiceImplTests extends ESTestCase { .put(AwsS3Service.CLOUD_S3.SECRET_SETTING.getKey(), "s3_secret") .build(); launchAWSCredentialsWithElasticsearchSettingsTest(repositorySettings, settings, "s3_key", "s3_secret"); + assertWarnings("[" + AwsS3Service.CLOUD_S3.KEY_SETTING.getKey() + "] setting was deprecated", + "[" + AwsS3Service.CLOUD_S3.SECRET_SETTING.getKey() + "] setting was deprecated"); } - public void testAWSCredentialsWithElasticsearchRepositoriesSettings() { + public void testAWSCredentialsWithElasticsearchRepositoriesSettingsBackcompat() { Settings repositorySettings = generateRepositorySettings(null, null, "eu-central", null, null); Settings settings = Settings.builder() .put(S3Repository.Repositories.KEY_SETTING.getKey(), "repositories_key") .put(S3Repository.Repositories.SECRET_SETTING.getKey(), "repositories_secret") .build(); launchAWSCredentialsWithElasticsearchSettingsTest(repositorySettings, settings, "repositories_key", "repositories_secret"); + assertWarnings("[" + S3Repository.Repositories.KEY_SETTING.getKey() + "] setting was deprecated", + "[" + S3Repository.Repositories.SECRET_SETTING.getKey() + "] setting was deprecated"); } - public void testAWSCredentialsWithElasticsearchAwsAndRepositoriesSettings() { + public void testAWSCredentialsWithElasticsearchAwsAndRepositoriesSettingsBackcompat() { Settings repositorySettings = generateRepositorySettings(null, null, "eu-central", null, null); Settings settings = Settings.builder() .put(AwsS3Service.KEY_SETTING.getKey(), "aws_key") @@ -86,9 +191,11 @@ public class AwsS3ServiceImplTests extends ESTestCase { .put(S3Repository.Repositories.SECRET_SETTING.getKey(), "repositories_secret") .build(); launchAWSCredentialsWithElasticsearchSettingsTest(repositorySettings, settings, "repositories_key", "repositories_secret"); + assertWarnings("[" + S3Repository.Repositories.KEY_SETTING.getKey() + "] setting was deprecated", + "[" + S3Repository.Repositories.SECRET_SETTING.getKey() + "] setting was deprecated"); } - public void testAWSCredentialsWithElasticsearchAwsAndS3AndRepositoriesSettings() { + public void testAWSCredentialsWithElasticsearchAwsAndS3AndRepositoriesSettingsBackcompat() { Settings repositorySettings = generateRepositorySettings(null, null, "eu-central", null, null); Settings settings = Settings.builder() .put(AwsS3Service.KEY_SETTING.getKey(), "aws_key") @@ -99,18 +206,22 @@ public class AwsS3ServiceImplTests extends ESTestCase { .put(S3Repository.Repositories.SECRET_SETTING.getKey(), "repositories_secret") .build(); launchAWSCredentialsWithElasticsearchSettingsTest(repositorySettings, settings, "repositories_key", "repositories_secret"); + assertWarnings("[" + S3Repository.Repositories.KEY_SETTING.getKey() + "] setting was deprecated", + "[" + S3Repository.Repositories.SECRET_SETTING.getKey() + "] setting was deprecated"); } - public void testAWSCredentialsWithElasticsearchRepositoriesSettingsAndRepositorySettings() { + public void testAWSCredentialsWithElasticsearchRepositoriesSettingsAndRepositorySettingsBackcompat() { Settings repositorySettings = generateRepositorySettings("repository_key", "repository_secret", "eu-central", null, null); Settings settings = Settings.builder() .put(S3Repository.Repositories.KEY_SETTING.getKey(), "repositories_key") .put(S3Repository.Repositories.SECRET_SETTING.getKey(), "repositories_secret") .build(); launchAWSCredentialsWithElasticsearchSettingsTest(repositorySettings, settings, "repository_key", "repository_secret"); + assertWarnings("[" + S3Repository.Repository.KEY_SETTING.getKey() + "] setting was deprecated", + "[" + S3Repository.Repository.SECRET_SETTING.getKey() + "] setting was deprecated"); } - public void testAWSCredentialsWithElasticsearchAwsAndRepositoriesSettingsAndRepositorySettings() { + public void testAWSCredentialsWithElasticsearchAwsAndRepositoriesSettingsAndRepositorySettingsBackcompat() { Settings repositorySettings = generateRepositorySettings("repository_key", "repository_secret", "eu-central", null, null); Settings settings = Settings.builder() .put(AwsS3Service.KEY_SETTING.getKey(), "aws_key") @@ -119,9 +230,11 @@ public class AwsS3ServiceImplTests extends ESTestCase { .put(S3Repository.Repositories.SECRET_SETTING.getKey(), "repositories_secret") .build(); launchAWSCredentialsWithElasticsearchSettingsTest(repositorySettings, settings, "repository_key", "repository_secret"); + assertWarnings("[" + S3Repository.Repository.KEY_SETTING.getKey() + "] setting was deprecated", + "[" + S3Repository.Repository.SECRET_SETTING.getKey() + "] setting was deprecated"); } - public void testAWSCredentialsWithElasticsearchAwsAndS3AndRepositoriesSettingsAndRepositorySettings() { + public void testAWSCredentialsWithElasticsearchAwsAndS3AndRepositoriesSettingsAndRepositorySettingsBackcompat() { Settings repositorySettings = generateRepositorySettings("repository_key", "repository_secret", "eu-central", null, null); Settings settings = Settings.builder() .put(AwsS3Service.KEY_SETTING.getKey(), "aws_key") @@ -132,6 +245,8 @@ public class AwsS3ServiceImplTests extends ESTestCase { .put(S3Repository.Repositories.SECRET_SETTING.getKey(), "repositories_secret") .build(); launchAWSCredentialsWithElasticsearchSettingsTest(repositorySettings, settings, "repository_key", "repository_secret"); + assertWarnings("[" + S3Repository.Repository.KEY_SETTING.getKey() + "] setting was deprecated", + "[" + S3Repository.Repository.SECRET_SETTING.getKey() + "] setting was deprecated"); } protected void launchAWSCredentialsWithElasticsearchSettingsTest(Settings singleRepositorySettings, Settings settings, @@ -148,6 +263,46 @@ public class AwsS3ServiceImplTests extends ESTestCase { } public void testAWSConfigurationWithAwsSettings() { + Settings repositorySettings = generateRepositorySettings(null, null, "eu-central", null, null); + MockSecureSettings secureSettings = new MockSecureSettings(); + secureSettings.setString(AwsS3Service.PROXY_USERNAME_SETTING.getKey(), "aws_proxy_username"); + secureSettings.setString(AwsS3Service.PROXY_PASSWORD_SETTING.getKey(), "aws_proxy_password"); + Settings settings = Settings.builder() + .setSecureSettings(secureSettings) + .put(AwsS3Service.PROTOCOL_SETTING.getKey(), "http") + .put(AwsS3Service.PROXY_HOST_SETTING.getKey(), "aws_proxy_host") + .put(AwsS3Service.PROXY_PORT_SETTING.getKey(), 8080) + .put(AwsS3Service.SIGNER_SETTING.getKey(), "AWS3SignerType") + .put(AwsS3Service.READ_TIMEOUT.getKey(), "10s") + .build(); + launchAWSConfigurationTest(settings, repositorySettings, Protocol.HTTP, "aws_proxy_host", 8080, "aws_proxy_username", + "aws_proxy_password", "AWS3SignerType", 3, false, 10000); + } + + public void testAWSConfigurationWithAwsAndS3Settings() { + Settings repositorySettings = generateRepositorySettings(null, null, "eu-central", null, null); + MockSecureSettings secureSettings = new MockSecureSettings(); + secureSettings.setString(AwsS3Service.PROXY_USERNAME_SETTING.getKey(), "aws_proxy_username"); + secureSettings.setString(AwsS3Service.PROXY_PASSWORD_SETTING.getKey(), "aws_proxy_password"); + secureSettings.setString(AwsS3Service.CLOUD_S3.PROXY_USERNAME_SETTING.getKey(), "s3_proxy_username"); + secureSettings.setString(AwsS3Service.CLOUD_S3.PROXY_PASSWORD_SETTING.getKey(), "s3_proxy_password"); + Settings settings = Settings.builder() + .setSecureSettings(secureSettings) + .put(AwsS3Service.PROTOCOL_SETTING.getKey(), "http") + .put(AwsS3Service.PROXY_HOST_SETTING.getKey(), "aws_proxy_host") + .put(AwsS3Service.PROXY_PORT_SETTING.getKey(), 8080) + .put(AwsS3Service.SIGNER_SETTING.getKey(), "AWS3SignerType") + .put(AwsS3Service.CLOUD_S3.PROTOCOL_SETTING.getKey(), "https") + .put(AwsS3Service.CLOUD_S3.PROXY_HOST_SETTING.getKey(), "s3_proxy_host") + .put(AwsS3Service.CLOUD_S3.PROXY_PORT_SETTING.getKey(), 8081) + .put(AwsS3Service.CLOUD_S3.SIGNER_SETTING.getKey(), "NoOpSignerType") + .put(AwsS3Service.CLOUD_S3.READ_TIMEOUT.getKey(), "10s") + .build(); + launchAWSConfigurationTest(settings, repositorySettings, Protocol.HTTPS, "s3_proxy_host", 8081, "s3_proxy_username", + "s3_proxy_password", "NoOpSignerType", 3, false, 10000); + } + + public void testAWSConfigurationWithAwsSettingsBackcompat() { Settings repositorySettings = generateRepositorySettings(null, null, "eu-central", null, null); Settings settings = Settings.builder() .put(AwsS3Service.PROTOCOL_SETTING.getKey(), "http") @@ -160,9 +315,11 @@ public class AwsS3ServiceImplTests extends ESTestCase { .build(); launchAWSConfigurationTest(settings, repositorySettings, Protocol.HTTP, "aws_proxy_host", 8080, "aws_proxy_username", "aws_proxy_password", "AWS3SignerType", 3, false, 10000); + assertWarnings("[" + AwsS3Service.PROXY_USERNAME_SETTING.getKey() + "] setting was deprecated", + "[" + AwsS3Service.PROXY_PASSWORD_SETTING.getKey() + "] setting was deprecated"); } - public void testAWSConfigurationWithAwsAndS3Settings() { + public void testAWSConfigurationWithAwsAndS3SettingsBackcompat() { Settings repositorySettings = generateRepositorySettings(null, null, "eu-central", null, null); Settings settings = Settings.builder() .put(AwsS3Service.PROTOCOL_SETTING.getKey(), "http") @@ -181,6 +338,8 @@ public class AwsS3ServiceImplTests extends ESTestCase { .build(); launchAWSConfigurationTest(settings, repositorySettings, Protocol.HTTPS, "s3_proxy_host", 8081, "s3_proxy_username", "s3_proxy_password", "NoOpSignerType", 3, false, 10000); + assertWarnings("[" + AwsS3Service.CLOUD_S3.PROXY_USERNAME_SETTING.getKey() + "] setting was deprecated", + "[" + AwsS3Service.CLOUD_S3.PROXY_PASSWORD_SETTING.getKey() + "] setting was deprecated"); } protected void launchAWSConfigurationTest(Settings settings, @@ -236,6 +395,15 @@ public class AwsS3ServiceImplTests extends ESTestCase { return builder.build(); } + private static Settings generateSecureRepositorySettings(String key, String secret, String region, String endpoint, + Integer maxRetries) { + Settings settings = generateRepositorySettings(null, null, region, endpoint, maxRetries); + MockSecureSettings secureSettings = new MockSecureSettings(); + secureSettings.setString(S3Repository.Repository.KEY_SETTING.getKey(), key); + secureSettings.setString(S3Repository.Repository.SECRET_SETTING.getKey(), secret); + return Settings.builder().put(settings).setSecureSettings(secureSettings).build(); + } + public void testDefaultEndpoint() { launchAWSEndpointTest(generateRepositorySettings("repository_key", "repository_secret", null, null, null), Settings.EMPTY, ""); launchAWSEndpointTest(generateRepositorySettings("repository_key", "repository_secret", "eu-central", null, null), Settings.EMPTY, diff --git a/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryTests.java b/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryTests.java index 86c27b2e668..0446425229e 100644 --- a/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryTests.java +++ b/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryTests.java @@ -25,6 +25,8 @@ import com.amazonaws.services.s3.AmazonS3; import org.elasticsearch.cloud.aws.AwsS3Service; import org.elasticsearch.cluster.metadata.RepositoryMetaData; import org.elasticsearch.common.component.AbstractLifecycleComponent; +import org.elasticsearch.common.settings.MockSecureSettings; +import org.elasticsearch.common.settings.SecureString; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.ByteSizeUnit; import org.elasticsearch.common.unit.ByteSizeValue; @@ -67,13 +69,21 @@ public class S3RepositoryTests extends ESTestCase { } public void testSettingsResolution() throws Exception { - Settings localSettings = Settings.builder().put(Repository.KEY_SETTING.getKey(), "key1").build(); - Settings globalSettings = Settings.builder().put(Repositories.KEY_SETTING.getKey(), "key2").build(); + MockSecureSettings secureSettings1 = new MockSecureSettings(); + secureSettings1.setString(Repository.KEY_SETTING.getKey(), "key1"); + Settings localSettings = Settings.builder().setSecureSettings(secureSettings1).build(); + MockSecureSettings secureSettings2 = new MockSecureSettings(); + secureSettings2.setString(Repositories.KEY_SETTING.getKey(), "key2"); + Settings globalSettings = Settings.builder().setSecureSettings(secureSettings2).build(); - assertEquals("key1", getValue(localSettings, globalSettings, Repository.KEY_SETTING, Repositories.KEY_SETTING)); - assertEquals("key1", getValue(localSettings, Settings.EMPTY, Repository.KEY_SETTING, Repositories.KEY_SETTING)); - assertEquals("key2", getValue(Settings.EMPTY, globalSettings, Repository.KEY_SETTING, Repositories.KEY_SETTING)); - assertEquals("", getValue(Settings.EMPTY, Settings.EMPTY, Repository.KEY_SETTING, Repositories.KEY_SETTING)); + assertEquals(new SecureString("key1".toCharArray()), + getValue(localSettings, globalSettings, Repository.KEY_SETTING, Repositories.KEY_SETTING)); + assertEquals(new SecureString("key1".toCharArray()), + getValue(localSettings, Settings.EMPTY, Repository.KEY_SETTING, Repositories.KEY_SETTING)); + assertEquals(new SecureString("key2".toCharArray()), + getValue(Settings.EMPTY, globalSettings, Repository.KEY_SETTING, Repositories.KEY_SETTING)); + assertEquals(new SecureString("".toCharArray()), + getValue(Settings.EMPTY, Settings.EMPTY, Repository.KEY_SETTING, Repositories.KEY_SETTING)); } public void testInvalidChunkBufferSizeSettings() throws IOException { diff --git a/plugins/repository-s3/src/test/resources/rest-api-spec/test/repository_s3/20_repository.yaml b/plugins/repository-s3/src/test/resources/rest-api-spec/test/repository_s3/20_repository.yaml index 17294986d2d..fe60a16b4b5 100644 --- a/plugins/repository-s3/src/test/resources/rest-api-spec/test/repository_s3/20_repository.yaml +++ b/plugins/repository-s3/src/test/resources/rest-api-spec/test/repository_s3/20_repository.yaml @@ -1,7 +1,12 @@ # Integration tests for Repository S3 component # "S3 repository can be registered": + - skip: + features: warnings - do: + warnings: + - "[access_key] setting was deprecated in Elasticsearch and it will be removed in a future release! See the breaking changes lists in the documentation for details" + - "[secret_key] setting was deprecated in Elasticsearch and it will be removed in a future release! See the breaking changes lists in the documentation for details" snapshot.create_repository: repository: test_repo_s3_1 verify: false diff --git a/test/framework/src/main/java/org/elasticsearch/common/settings/MockSecureSettings.java b/test/framework/src/main/java/org/elasticsearch/common/settings/MockSecureSettings.java new file mode 100644 index 00000000000..c9821ca700d --- /dev/null +++ b/test/framework/src/main/java/org/elasticsearch/common/settings/MockSecureSettings.java @@ -0,0 +1,54 @@ +/* + * 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.common.settings; + +import java.io.IOException; +import java.util.HashMap; +import java.util.Map; + +/** + * A mock implementation of secure settings for tests to use. + */ +public class MockSecureSettings implements SecureSettings { + + private Map secureStrings = new HashMap<>(); + + @Override + public boolean isLoaded() { + return true; + } + + @Override + public boolean hasSetting(String setting) { + return secureStrings.containsKey(setting); + } + + @Override + public SecureString getString(String setting) { + return secureStrings.get(setting); + } + + public void setString(String setting, String value) { + secureStrings.put(setting, new SecureString(value.toCharArray())); + } + + @Override + public void close() throws IOException {} +} diff --git a/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java index 0c5a654846d..4c9bf444de1 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java @@ -130,6 +130,7 @@ import java.util.stream.Collectors; import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; import static org.elasticsearch.common.util.CollectionUtils.arrayAsArrayList; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasItem; @@ -308,7 +309,7 @@ public abstract class ESTestCase extends LuceneTestCase { + Arrays.asList(expectedWarnings) + "\nActual: " + actualWarnings, expectedWarnings.length, actualWarnings.size()); for (String msg : expectedWarnings) { - assertThat(actualWarnings, hasItem(equalTo(msg))); + assertThat(actualWarnings, hasItem(containsString(msg))); } } finally { resetDeprecationLogger(); From ec73cfe93721627c9692f9b34d1d8d7e5946e220 Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Wed, 11 Jan 2017 20:51:23 +0100 Subject: [PATCH 08/10] Remove unused *XContentGenerator constructors (#22558) --- .../common/xcontent/cbor/CborXContentGenerator.java | 6 ------ .../common/xcontent/json/JsonXContentGenerator.java | 5 ----- .../common/xcontent/smile/SmileXContentGenerator.java | 6 ------ .../common/xcontent/yaml/YamlXContentGenerator.java | 6 ------ 4 files changed, 23 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/xcontent/cbor/CborXContentGenerator.java b/core/src/main/java/org/elasticsearch/common/xcontent/cbor/CborXContentGenerator.java index e63a928109d..119cb5c98c6 100644 --- a/core/src/main/java/org/elasticsearch/common/xcontent/cbor/CborXContentGenerator.java +++ b/core/src/main/java/org/elasticsearch/common/xcontent/cbor/CborXContentGenerator.java @@ -20,20 +20,14 @@ package org.elasticsearch.common.xcontent.cbor; import com.fasterxml.jackson.core.JsonGenerator; -import org.elasticsearch.common.Strings; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.common.xcontent.json.JsonXContentGenerator; import java.io.OutputStream; -import java.util.Collections; import java.util.Set; public class CborXContentGenerator extends JsonXContentGenerator { - public CborXContentGenerator(JsonGenerator jsonGenerator, OutputStream os) { - this(jsonGenerator, os, Collections.emptySet(), Collections.emptySet()); - } - public CborXContentGenerator(JsonGenerator jsonGenerator, OutputStream os, Set includes, Set excludes) { super(jsonGenerator, os, includes, excludes); } diff --git a/core/src/main/java/org/elasticsearch/common/xcontent/json/JsonXContentGenerator.java b/core/src/main/java/org/elasticsearch/common/xcontent/json/JsonXContentGenerator.java index 0742e4a716a..07ae16b96c6 100644 --- a/core/src/main/java/org/elasticsearch/common/xcontent/json/JsonXContentGenerator.java +++ b/core/src/main/java/org/elasticsearch/common/xcontent/json/JsonXContentGenerator.java @@ -44,7 +44,6 @@ import java.io.BufferedInputStream; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; -import java.util.Collections; import java.util.Objects; import java.util.Set; @@ -73,10 +72,6 @@ public class JsonXContentGenerator implements XContentGenerator { private static final DefaultPrettyPrinter.Indenter INDENTER = new DefaultIndenter(" ", LF.getValue()); private boolean prettyPrint = false; - public JsonXContentGenerator(JsonGenerator jsonGenerator, OutputStream os) { - this(jsonGenerator, os, Collections.emptySet(), Collections.emptySet()); - } - public JsonXContentGenerator(JsonGenerator jsonGenerator, OutputStream os, Set includes, Set excludes) { Objects.requireNonNull(includes, "Including filters must not be null"); Objects.requireNonNull(excludes, "Excluding filters must not be null"); diff --git a/core/src/main/java/org/elasticsearch/common/xcontent/smile/SmileXContentGenerator.java b/core/src/main/java/org/elasticsearch/common/xcontent/smile/SmileXContentGenerator.java index afa420805f7..f368c0e383f 100644 --- a/core/src/main/java/org/elasticsearch/common/xcontent/smile/SmileXContentGenerator.java +++ b/core/src/main/java/org/elasticsearch/common/xcontent/smile/SmileXContentGenerator.java @@ -20,20 +20,14 @@ package org.elasticsearch.common.xcontent.smile; import com.fasterxml.jackson.core.JsonGenerator; -import org.elasticsearch.common.Strings; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.common.xcontent.json.JsonXContentGenerator; import java.io.OutputStream; -import java.util.Collections; import java.util.Set; public class SmileXContentGenerator extends JsonXContentGenerator { - public SmileXContentGenerator(JsonGenerator jsonGenerator, OutputStream os) { - this(jsonGenerator, os, Collections.emptySet(), Collections.emptySet()); - } - public SmileXContentGenerator(JsonGenerator jsonGenerator, OutputStream os, Set includes, Set excludes) { super(jsonGenerator, os, includes, excludes); } diff --git a/core/src/main/java/org/elasticsearch/common/xcontent/yaml/YamlXContentGenerator.java b/core/src/main/java/org/elasticsearch/common/xcontent/yaml/YamlXContentGenerator.java index d2c53c8a020..0d969c21a0f 100644 --- a/core/src/main/java/org/elasticsearch/common/xcontent/yaml/YamlXContentGenerator.java +++ b/core/src/main/java/org/elasticsearch/common/xcontent/yaml/YamlXContentGenerator.java @@ -20,20 +20,14 @@ package org.elasticsearch.common.xcontent.yaml; import com.fasterxml.jackson.core.JsonGenerator; -import org.elasticsearch.common.Strings; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.common.xcontent.json.JsonXContentGenerator; import java.io.OutputStream; -import java.util.Collections; import java.util.Set; public class YamlXContentGenerator extends JsonXContentGenerator { - public YamlXContentGenerator(JsonGenerator jsonGenerator, OutputStream os) { - this(jsonGenerator, os, Collections.emptySet(), Collections.emptySet()); - } - public YamlXContentGenerator(JsonGenerator jsonGenerator, OutputStream os, Set includes, Set excludes) { super(jsonGenerator, os, includes, excludes); } From b7995fbc0d3d53fb66be0ba1aefc001afcaee479 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Wed, 11 Jan 2017 17:10:56 -0500 Subject: [PATCH 09/10] Fix default port for unicast zen ping hosts Today when you do not specify a port for an entry in discovery.zen.ping.unicast.hosts, the default port is the value of the setting transport.profiles.default.port and falls back to the value of transport.tcp.port if this is not set. For a node that is explicitly bound to a different port than the default port, this means that the default port will be equal to this explicitly bound port. Yet, the docs say that we fall back to 9300 here. This commit corrects the docs. Relates #22568 --- docs/reference/modules/discovery/zen.asciidoc | 4 ++-- docs/reference/setup/important-settings.asciidoc | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/reference/modules/discovery/zen.asciidoc b/docs/reference/modules/discovery/zen.asciidoc index 1caace2216e..ea50d11e39e 100644 --- a/docs/reference/modules/discovery/zen.asciidoc +++ b/docs/reference/modules/discovery/zen.asciidoc @@ -39,8 +39,8 @@ Unicast discovery provides the following settings with the `discovery.zen.ping.u |======================================================================= |Setting |Description |`hosts` |Either an array setting or a comma delimited setting. Each -value should be in the form of `host:port` or `host` (where `port` defaults to `9300`). Note that IPv6 hosts must be -bracketed. Defaults to `127.0.0.1, [::1]` + value should be in the form of `host:port` or `host` (where `port` defaults to the setting `transport.profiles.default.port` + falling back to `transport.tcp.port` if not set). Note that IPv6 hosts must be bracketed. Defaults to `127.0.0.1, [::1]` |`hosts.resolve_timeout` |The amount of time to wait for DNS lookups on each round of pinging. Specified as <>. Defaults to 5s. |======================================================================= diff --git a/docs/reference/setup/important-settings.asciidoc b/docs/reference/setup/important-settings.asciidoc index bdf7c314476..fc43f5869a9 100644 --- a/docs/reference/setup/important-settings.asciidoc +++ b/docs/reference/setup/important-settings.asciidoc @@ -150,7 +150,7 @@ discovery.zen.ping.unicast.hosts: - 192.168.1.11 <1> - seeds.mydomain.com <2> -------------------------------------------------- -<1> The port will default to 9300 if not specified. +<1> The port will default to `transport.profiles.default.port` and fallback to `transport.tcp.port` if not specified. <2> A hostname that resolves to multiple IP addresses will try all resolved addresses. [float] From 8a0393f718345b41b3415ca18a89ed412c59ddaa Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Wed, 11 Jan 2017 23:37:12 +0100 Subject: [PATCH 10/10] Move assertion for open channels under TcpTransport lock TcpTransport has an actual mechanism to stop resources in subclasses. Instead of overriding `doStop` subclasses should override `stopInternal` that is executed under the connection lock guaranteeing that there is no concurrency etc. Relates to #22554 --- .../org/elasticsearch/transport/TcpTransport.java | 2 +- .../elasticsearch/transport/MockTcpTransport.java | 15 +++------------ 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/transport/TcpTransport.java b/core/src/main/java/org/elasticsearch/transport/TcpTransport.java index f2b29706caf..8be107b1919 100644 --- a/core/src/main/java/org/elasticsearch/transport/TcpTransport.java +++ b/core/src/main/java/org/elasticsearch/transport/TcpTransport.java @@ -840,7 +840,7 @@ public abstract class TcpTransport extends AbstractLifecycleComponent i } @Override - protected void doClose() { + protected final void doClose() { } @Override diff --git a/test/framework/src/main/java/org/elasticsearch/transport/MockTcpTransport.java b/test/framework/src/main/java/org/elasticsearch/transport/MockTcpTransport.java index a08660bb388..51774edcba5 100644 --- a/test/framework/src/main/java/org/elasticsearch/transport/MockTcpTransport.java +++ b/test/framework/src/main/java/org/elasticsearch/transport/MockTcpTransport.java @@ -400,23 +400,14 @@ public class MockTcpTransport extends TcpTransport @Override protected void stopInternal() { ThreadPool.terminate(executor, 10, TimeUnit.SECONDS); + synchronized (openChannels) { + assert openChannels.isEmpty() : "there are still open channels: " + openChannels; + } } @Override protected Version getCurrentVersion() { return mockVersion; } - - @Override - protected void doClose() { - if (Thread.currentThread().isInterrupted() == false) { - // TCPTransport might be interrupted due to a timeout waiting for connections to be closed. - // in this case the thread is interrupted and we can't tell if we really missed something or if we are - // still closing connections. in such a case we don't assert the open channels - synchronized (openChannels) { - assert openChannels.isEmpty() : "there are still open channels: " + openChannels; - } - } - } }