Merge remote-tracking branch 'upstream/main' into buildkite-add-artif…

…act-url
brianseeders · Oct 13, 2023 · 4cecbc9 · 4cecbc9
2 parents a633d92 + 323d936
commit 4cecbc9
Show file tree

Hide file tree

Showing 11 changed files with 297 additions and 67 deletions.
diff --git a/docs/changelog/100779.yaml b/docs/changelog/100779.yaml
@@ -0,0 +1,6 @@
+pr: 100779
+summary: Fix NullPointerException in RotableSecret
+area: Security
+type: bug
+issues:
+ - 99759
diff --git a/docs/changelog/100828.yaml b/docs/changelog/100828.yaml
@@ -0,0 +1,5 @@
+pr: 100828
+summary: Consider task cancelled exceptions as recoverable
+area: Transform
+type: bug
+issues: []
diff --git a/docs/reference/security/authorization/built-in-roles.asciidoc b/docs/reference/security/authorization/built-in-roles.asciidoc
@@ -141,11 +141,6 @@ read access to the `.ml-notifications` and `.ml-anomalies*` indices
 {ml-cap} users also need index privileges for source and destination
 indices and roles that grant access to {kib}. See {ml-docs-setup-privileges}.
 
-[[built-in-roles-manage-enrich]] `manage_enrich`::
-Grants privileges to access and use all of the {ref}/enrich-apis.html[enrich APIs].
-Users with this role can manage enrich policies that add data from your existing 
-indices to incoming documents during ingest.
-
 [[built-in-roles-monitoring-user]] `monitoring_user`::
 Grants the minimum privileges required for any user of {monitoring} other than those
 required to use {kib}. This role grants access to the monitoring indices and grants
@@ -171,10 +166,10 @@ Reporting users should also be assigned additional roles that grant
 access to the <<roles-indices-priv,indices>> that will be used to generate reports.
 
 [[built-in-roles-rollup-admin]] `rollup_admin`::
-Grants `manage_rollup` cluster privileges, which enable you to manage and execute all rollup actions. 
+Grants `manage_rollup` cluster privileges, which enable you to manage and execute all rollup actions.
 
 [[built-in-roles-rollup-user]] `rollup_user`::
-Grants `monitor_rollup` cluster privileges, which enable you to perform read-only operations related to rollups. 
+Grants `monitor_rollup` cluster privileges, which enable you to perform read-only operations related to rollups.
 
 [[built-in-roles-snapshot-user]] `snapshot_user`::
 Grants the necessary privileges to create snapshots of **all** the indices and

diff --git a/docs/reference/security/authorization/privileges.asciidoc b/docs/reference/security/authorization/privileges.asciidoc
@@ -186,6 +186,9 @@ security roles of the user who created or updated them.
 All cluster read-only operations, like cluster health and state, hot threads,
 node info, node and cluster stats, and pending cluster tasks.
 
+`monitor_enrich`::
+All read-only operations related to managing and executing enrich policies.
+
 `monitor_ml`::
 All read-only {ml} operations, such as getting information about {dfeeds}, jobs,
 model snapshots, or results.

diff --git a/server/src/internalClusterTest/java/org/elasticsearch/indices/recovery/IndexRecoveryIT.java b/server/src/internalClusterTest/java/org/elasticsearch/indices/recovery/IndexRecoveryIT.java
diff --git a/server/src/main/java/org/elasticsearch/common/settings/RotatableSecret.java b/server/src/main/java/org/elasticsearch/common/settings/RotatableSecret.java
@@ -69,10 +69,16 @@ public boolean isSet() {
      */
     public boolean matches(SecureString secret) {
         checkExpired();
-        if ((Strings.hasText(secrets.current) == false && Strings.hasText(secrets.prior) == false) || Strings.hasText(secret) == false) {
+        if (Strings.hasText(secret) == false) {
             return false;
         }
-        return secrets.current.equals(secret) || (secrets.prior != null && secrets.prior.equals(secret));
+        boolean currentSecretValid = Strings.hasText(secrets.current);
+        boolean priorSecretValid = Strings.hasText(secrets.prior);
+        if (currentSecretValid && secrets.current.equals(secret)) {
+            return true;
+        } else {
+            return priorSecretValid && secrets.prior.equals(secret);
+        }
     }
 
     // for testing only
@@ -92,12 +98,12 @@ boolean isWriteLocked() {
     private void checkExpired() {
         boolean needToUnlock = false;
         long stamp = stampedLock.tryOptimisticRead();
-        boolean expired = secrets.prior != null && secrets.priorValidTill.isBefore(Instant.now()); // optimistic read
+        boolean expired = secrets.prior != null && secrets.priorValidTill.compareTo(Instant.now()) <= 0; // optimistic read
         if (stampedLock.validate(stamp) == false) {
             // optimism failed...potentially block to obtain the read lock and try the read again
             stamp = stampedLock.readLock();
             needToUnlock = true;
-            expired = secrets.prior != null && secrets.priorValidTill.isBefore(Instant.now()); // locked read
+            expired = secrets.prior != null && secrets.priorValidTill.compareTo(Instant.now()) <= 0; // locked read
         }
         try {
             if (expired) {

diff --git a/server/src/test/java/org/elasticsearch/common/settings/RotatableSecretTests.java b/server/src/test/java/org/elasticsearch/common/settings/RotatableSecretTests.java
@@ -27,7 +27,6 @@ public class RotatableSecretTests extends ESTestCase {
     private final SecureString secret2 = new SecureString(randomAlphaOfLength(10));
     private final SecureString secret3 = new SecureString(randomAlphaOfLength(10));
 
-    @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/99759")
     public void testBasicRotation() throws Exception {
         // initial state
         RotatableSecret rotatableSecret = new RotatableSecret(secret1);

diff --git a/x-pack/plugin/esql/compute/build.gradle b/x-pack/plugin/esql/compute/build.gradle
@@ -19,7 +19,7 @@ tasks.named('checkstyleMain').configure {
   excludes = [ "**/*.java.st" ]
 }
 
-spotlessJava.dependsOn stringTemplates
+tasks.named("spotlessJava") { dependsOn stringTemplates }
 
 spotless {
   java {

diff --git a/...m/src/main/java/org/elasticsearch/xpack/transform/transforms/TransformFailureHandler.java b/...m/src/main/java/org/elasticsearch/xpack/transform/transforms/TransformFailureHandler.java
@@ -188,7 +188,7 @@ private void handleBulkIndexingException(BulkIndexingException bulkIndexingExcep
      * @param numFailureRetries the number of configured retries
      */
     private void handleElasticsearchException(ElasticsearchException elasticsearchException, boolean unattended, int numFailureRetries) {
-        if (unattended == false && ExceptionRootCauseFinder.IRRECOVERABLE_REST_STATUSES.contains(elasticsearchException.status())) {
+        if (unattended == false && ExceptionRootCauseFinder.isExceptionIrrecoverable(elasticsearchException)) {
             String message = "task encountered irrecoverable failure: " + elasticsearchException.getDetailedMessage();
             fail(message);
         } else {

diff --git a/...sform/src/main/java/org/elasticsearch/xpack/transform/utils/ExceptionRootCauseFinder.java b/...sform/src/main/java/org/elasticsearch/xpack/transform/utils/ExceptionRootCauseFinder.java
@@ -10,10 +10,9 @@
 import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.action.bulk.BulkItemResponse;
 import org.elasticsearch.rest.RestStatus;
+import org.elasticsearch.tasks.TaskCancelledException;
 
-import java.util.Arrays;
 import java.util.Collection;
-import java.util.HashSet;
 import java.util.Set;
 
 /**
@@ -24,17 +23,15 @@ public final class ExceptionRootCauseFinder {
     /**
      * List of rest statuses that we consider irrecoverable
      */
-    public static final Set<RestStatus> IRRECOVERABLE_REST_STATUSES = new HashSet<>(
-        Arrays.asList(
-            RestStatus.GONE,
-            RestStatus.NOT_IMPLEMENTED,
-            RestStatus.NOT_FOUND,
-            RestStatus.BAD_REQUEST,
-            RestStatus.UNAUTHORIZED,
-            RestStatus.FORBIDDEN,
-            RestStatus.METHOD_NOT_ALLOWED,
-            RestStatus.NOT_ACCEPTABLE
-        )
+    static final Set<RestStatus> IRRECOVERABLE_REST_STATUSES = Set.of(
+        RestStatus.GONE,
+        RestStatus.NOT_IMPLEMENTED,
+        RestStatus.NOT_FOUND,
+        RestStatus.BAD_REQUEST,
+        RestStatus.UNAUTHORIZED,
+        RestStatus.FORBIDDEN,
+        RestStatus.METHOD_NOT_ALLOWED,
+        RestStatus.NOT_ACCEPTABLE
     );
 
     /**
@@ -65,7 +62,7 @@ public static Throwable getFirstIrrecoverableExceptionFromBulkResponses(Collecti
             }
 
             if (unwrappedThrowable instanceof ElasticsearchException elasticsearchException) {
-                if (IRRECOVERABLE_REST_STATUSES.contains(elasticsearchException.status())) {
+                if (isExceptionIrrecoverable(elasticsearchException)) {
                     return elasticsearchException;
                 }
             }
@@ -74,6 +71,21 @@ public static Throwable getFirstIrrecoverableExceptionFromBulkResponses(Collecti
         return null;
     }
 
-    private ExceptionRootCauseFinder() {}
+    public static boolean isExceptionIrrecoverable(ElasticsearchException elasticsearchException) {
+        if (IRRECOVERABLE_REST_STATUSES.contains(elasticsearchException.status())) {
+
+            // Even if the status indicates the exception is irrecoverable, some exceptions
+            // with these status are worth retrying on.
 
+            // A TaskCancelledException occurs if a sub-action of a search encounters a circuit
+            // breaker exception. In this case the overall search task is cancelled.
+            if (elasticsearchException instanceof TaskCancelledException) {
+                return false;
+            }
+            return true;
+        }
+        return false;
+    }
+
+    private ExceptionRootCauseFinder() {}
 }
diff --git a/.../src/test/java/org/elasticsearch/xpack/transform/utils/ExceptionRootCauseFinderTests.java b/.../src/test/java/org/elasticsearch/xpack/transform/utils/ExceptionRootCauseFinderTests.java
@@ -11,11 +11,17 @@
 import org.elasticsearch.ResourceNotFoundException;
 import org.elasticsearch.action.DocWriteRequest.OpType;
 import org.elasticsearch.action.bulk.BulkItemResponse;
+import org.elasticsearch.common.breaker.CircuitBreaker;
+import org.elasticsearch.common.breaker.CircuitBreakingException;
 import org.elasticsearch.common.util.concurrent.EsRejectedExecutionException;
+import org.elasticsearch.index.Index;
 import org.elasticsearch.index.mapper.DocumentParsingException;
+import org.elasticsearch.index.mapper.MapperException;
 import org.elasticsearch.index.shard.ShardId;
 import org.elasticsearch.index.translog.TranslogException;
+import org.elasticsearch.indices.IndexClosedException;
 import org.elasticsearch.rest.RestStatus;
+import org.elasticsearch.tasks.TaskCancelledException;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.xcontent.XContentLocation;
 
@@ -149,6 +155,20 @@ public void testGetFirstIrrecoverableExceptionFromBulkResponses() {
         assertNull(ExceptionRootCauseFinder.getFirstIrrecoverableExceptionFromBulkResponses(bulkItemResponses.values()));
     }
 
+    public void testIsIrrecoverable() {
+        assertFalse(ExceptionRootCauseFinder.isExceptionIrrecoverable(new MapperException("mappings problem")));
+        assertFalse(ExceptionRootCauseFinder.isExceptionIrrecoverable(new TaskCancelledException("cancelled task")));
+        assertFalse(
+            ExceptionRootCauseFinder.isExceptionIrrecoverable(
+                new CircuitBreakingException("circuit broken", CircuitBreaker.Durability.TRANSIENT)
+            )
+        );
+        assertTrue(ExceptionRootCauseFinder.isExceptionIrrecoverable(new IndexClosedException(new Index("index", "1234"))));
+        assertTrue(
+            ExceptionRootCauseFinder.isExceptionIrrecoverable(new DocumentParsingException(new XContentLocation(1, 2), "parse error"))
+        );
+    }
+
     private static void assertFirstException(Collection<BulkItemResponse> bulkItemResponses, Class<?> expectedClass, String message) {
         Throwable t = ExceptionRootCauseFinder.getFirstIrrecoverableExceptionFromBulkResponses(bulkItemResponses);
         assertNotNull(t);