Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

HADOOP-17377: ABFS: MsiTokenProvider doesn't retry HTTP 429/410 from the Instance Metadata Service #5273

Open
wants to merge 25 commits into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
3f6386a
Add retry for HTTP 429 and HTTP 410
anmolasrani123 Jan 4, 2023
56c6f7d
Add retry for HTTP 429 and HTTP 410
anmolasrani123 Jan 4, 2023
2c25b39
Add retry for HTTP 429 and HTTP 410
anmolasrani123 Jan 4, 2023
88e51b6
Added tests to verify retry
anmolasrani123 Jan 5, 2023
3be0b00
Removing unused imports
anmolasrani123 Jan 5, 2023
e04011c
Adding negative tests
anmolasrani123 Jan 5, 2023
6283410
Merge branch 'trunk' into HADOOP-17377
anmolanmol1234 Apr 3, 2023
cf73a70
Merge branch 'apache:trunk' into HADOOP-17377
anmolanmol1234 Aug 22, 2023
e6740a3
Resolving PR comments
anmolanmol1234 Aug 22, 2023
0688809
Mockito fix
anmolanmol1234 Aug 22, 2023
115e88a
Added in other PR
anmolanmol1234 Aug 23, 2023
fe15f44
Removed
anmolanmol1234 Aug 23, 2023
01ec633
Removed new version changes
anmolanmol1234 Aug 23, 2023
4faa92d
Merge branch 'HADOOP-17377' of https://github.com/anmolanmol1234/hado…
anmolanmol1234 Aug 23, 2023
24c0483
Reverted
anmolanmol1234 Aug 23, 2023
99e66fa
Reverted mockito changes
anmolanmol1234 Aug 23, 2023
8b5e883
Merge branch 'apache:trunk' into HADOOP-17377
anmolanmol1234 Aug 23, 2023
1472eb8
POM
anmolanmol1234 Aug 23, 2023
7ba573f
Merge branch 'apache:trunk' into HADOOP-17377
anmolanmol1234 Aug 29, 2023
3f80d00
Merge branch 'HADOOP-17377' of https://github.com/anmolanmol1234/hado…
anmolanmol1234 Aug 30, 2023
462a3b6
Checkstyle fix
anmolanmol1234 Aug 30, 2023
78329de
PR comments
anmolanmol1234 Aug 31, 2023
240965d
Correct default value for backoff interval
anmolanmol1234 Nov 15, 2023
b0563a1
Merge branch 'apache:trunk' into HADOOP-17377
anmolanmol1234 Nov 15, 2023
225541e
Merge branch 'HADOOP-17377' of https://github.com/anmolanmol1234/hado…
anmolanmol1234 Nov 15, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ public Writable call(

// Nothing should be logged for a suppressed exception.
server.logException(logger, new TestException1(), dummyCall);
verifyZeroInteractions(logger);
verifyNoInteractions(logger);

// No stack trace should be logged for a terse exception.
server.logException(logger, new TestException2(), dummyCall);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,7 @@ public void testSameOrigin() throws ServletException, IOException {
CrossOriginFilter filter = new CrossOriginFilter();
filter.init(filterConfig);
filter.doFilter(mockReq, mockRes, mockChain);

Mockito.verifyZeroInteractions(mockRes);
Mockito.verifyNoInteractions(mockRes);
Mockito.verify(mockChain).doFilter(mockReq, mockRes);
}

Expand Down Expand Up @@ -224,7 +223,7 @@ public void testDisallowedOrigin() throws ServletException, IOException {
filter.init(filterConfig);
filter.doFilter(mockReq, mockRes, mockChain);

Mockito.verifyZeroInteractions(mockRes);
Mockito.verifyNoInteractions(mockRes);
Mockito.verify(mockChain).doFilter(mockReq, mockRes);
}

Expand Down Expand Up @@ -252,7 +251,7 @@ public void testDisallowedMethod() throws ServletException, IOException {
filter.init(filterConfig);
filter.doFilter(mockReq, mockRes, mockChain);

Mockito.verifyZeroInteractions(mockRes);
Mockito.verifyNoInteractions(mockRes);
Mockito.verify(mockChain).doFilter(mockReq, mockRes);
}

Expand Down Expand Up @@ -283,7 +282,7 @@ public void testDisallowedHeader() throws ServletException, IOException {
filter.init(filterConfig);
filter.doFilter(mockReq, mockRes, mockChain);

Mockito.verifyZeroInteractions(mockRes);
Mockito.verifyNoInteractions(mockRes);
Mockito.verify(mockChain).doFilter(mockReq, mockRes);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ public void testNoHeaderDefaultConfigBadRequest()

verify(mockRes, atLeastOnce()).sendError(
HttpServletResponse.SC_BAD_REQUEST, EXPECTED_MESSAGE);
Mockito.verifyZeroInteractions(mockChain);
Mockito.verifyNoInteractions(mockChain);
}

@Test
Expand Down Expand Up @@ -110,7 +110,7 @@ public void testNoHeaderCustomAgentConfigBadRequest()

verify(mockRes, atLeastOnce()).sendError(
HttpServletResponse.SC_BAD_REQUEST, EXPECTED_MESSAGE);
Mockito.verifyZeroInteractions(mockChain);
Mockito.verifyNoInteractions(mockChain);
}

@Test
Expand Down Expand Up @@ -228,7 +228,7 @@ public void testMissingHeaderWithCustomHeaderConfigBadRequest()
filter.init(filterConfig);
filter.doFilter(mockReq, mockRes, mockChain);

Mockito.verifyZeroInteractions(mockChain);
Mockito.verifyNoInteractions(mockChain);
}

@Test
Expand Down Expand Up @@ -260,7 +260,7 @@ public void testMissingHeaderNoMethodsToIgnoreConfigBadRequest()
filter.init(filterConfig);
filter.doFilter(mockReq, mockRes, mockChain);

Mockito.verifyZeroInteractions(mockChain);
Mockito.verifyNoInteractions(mockChain);
}

@Test
Expand Down Expand Up @@ -356,6 +356,6 @@ public void testMissingHeaderMultipleIgnoreMethodsConfigBadRequest()
filter.init(filterConfig);
filter.doFilter(mockReq, mockRes, mockChain);

Mockito.verifyZeroInteractions(mockChain);
Mockito.verifyNoInteractions(mockChain);
}
}
5 changes: 5 additions & 0 deletions hadoop-hdfs-project/hadoop-hdfs-rbf/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,11 @@ https://maven.apache.org/xsd/maven-4.0.0.xsd">
<artifactId>junit-jupiter-params</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<scope>test</scope>
</dependency>
</dependencies>

<build>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import static org.junit.Assert.assertTrue;

import java.io.IOException;
import java.lang.reflect.Field;
import java.security.PrivilegedExceptionAction;
import java.util.Collections;
import java.util.HashMap;
Expand Down Expand Up @@ -62,13 +63,13 @@
import org.apache.hadoop.security.UserGroupInformation;
import org.apache.hadoop.test.LambdaTestUtils;
import org.apache.hadoop.util.Lists;
import org.apache.hadoop.util.ReflectionUtils;
import org.apache.hadoop.util.Time;
import org.junit.AfterClass;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Test;
import org.mockito.Mockito;
import org.mockito.internal.util.reflection.FieldSetter;

/**
* The administrator interface of the {@link Router} implemented by
Expand Down Expand Up @@ -118,18 +119,19 @@ public static void globalSetUp() throws Exception {
* @throws IOException
* @throws NoSuchFieldException
*/
private static void setUpMocks() throws IOException, NoSuchFieldException {
private static void setUpMocks() throws IOException, NoSuchFieldException, IllegalAccessException {
RouterRpcServer spyRpcServer =
Mockito.spy(routerContext.getRouter().createRpcServer());
FieldSetter.setField(routerContext.getRouter(),
Router.class.getDeclaredField("rpcServer"), spyRpcServer);
Field rpcServerField = Router.class.getDeclaredField("rpcServer");
rpcServerField.setAccessible(true);
rpcServerField.set(routerContext.getRouter(), spyRpcServer);
Mockito.doReturn(null).when(spyRpcServer).getFileInfo(Mockito.anyString());

// mock rpc client for destination check when editing mount tables.
mockRpcClient = Mockito.spy(spyRpcServer.getRPCClient());
FieldSetter.setField(spyRpcServer,
RouterRpcServer.class.getDeclaredField("rpcClient"),
mockRpcClient);
Field rpcClientField = RouterRpcServer.class.getDeclaredField("rpcClient");
rpcClientField.setAccessible(true);
rpcClientField.set(spyRpcServer, mockRpcClient);
RemoteLocation remoteLocation0 =
new RemoteLocation("ns0", "/testdir", null);
RemoteLocation remoteLocation1 =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import static org.mockito.Matchers.any;
Copy link
Contributor

Choose a reason for hiding this comment

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

reinstate so this file doesn't change

import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import static org.apache.hadoop.test.Whitebox.getInternalState;
Expand Down Expand Up @@ -73,6 +72,7 @@
import org.apache.hadoop.ipc.StandbyException;
import org.apache.hadoop.test.GenericTestUtils;
import org.junit.Test;
import org.mockito.Mockito;
import org.slf4j.event.Level;

/**
Expand Down Expand Up @@ -423,7 +423,7 @@ public void testSubclusterDown() throws Exception {
FSNamesystem ns0 = nn0.getNamesystem();
HAContext nn0haCtx = (HAContext)getInternalState(ns0, "haContext");
HAContext mockCtx = mock(HAContext.class);
doThrow(new StandbyException("Mock")).when(mockCtx).checkOperation(any());
doThrow(new StandbyException("Mock")).when(mockCtx).checkOperation(Mockito.any());
Copy link
Contributor

Choose a reason for hiding this comment

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

return to the existing any() static import

setInternalState(ns0, "haContext", mockCtx);

// router0 should throw an exception
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1575,7 +1575,7 @@ public void testNoLookupsWhenNotUsed() throws Exception {
CacheManager cm = cluster.getNamesystem().getCacheManager();
LocatedBlocks locations = Mockito.mock(LocatedBlocks.class);
cm.setCachedLocations(locations);
Mockito.verifyZeroInteractions(locations);
Mockito.verifyNoInteractions(locations);
}

@Test(timeout=120000)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,6 @@ public void testShortCircuitSnapshotSearch() throws SnapshotException {
INodesInPath iip = Mockito.mock(INodesInPath.class);
List<INodeDirectory> snapDirs = new ArrayList<>();
FSDirSnapshotOp.checkSnapshot(fsn.getFSDirectory(), iip, snapDirs);
Mockito.verifyZeroInteractions(iip);
Mockito.verifyNoInteractions(iip);
}
}
15 changes: 14 additions & 1 deletion hadoop-project/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -1308,7 +1308,20 @@
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<version>2.28.2</version>
<version>4.11.0</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

add a new property mockito.version and reference in both places

<scope>compile</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-inline</artifactId>
<version>4.11.0</version>
<scope>test</scope>
<exclusions>
<exclusion>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
Expand Down
8 changes: 0 additions & 8 deletions hadoop-tools/hadoop-azure/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -321,21 +321,13 @@
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<version>4.11.0</version>
<scope>test</scope>
</dependency>

<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

is this needed? because its not in the base project pom.

I would rather this PR doesn't need that mockito upgrade as mockito upgrades are always a painful piece of work which never gets backported.

<groupId>org.mockito</groupId>
<artifactId>mockito-inline</artifactId>
<version>4.11.0</version>
<scope>test</scope>
<exclusions>
<exclusion>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
</exclusion>
</exclusions>
</dependency>

<dependency>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,12 @@ public final class AbfsHttpConstants {
// The HTTP 100 Continue informational status response code indicates that everything so far
// is OK and that the client should continue with the request or ignore it if it is already finished.
public static final String HUNDRED_CONTINUE = "100-continue";
/**
* HTTP status code indicating that the server has received too many requests and the client should
* qualify for retrying the operation, as described in the Microsoft Azure documentation.
* {@link "https://learn.microsoft.com/en-us/azure/active-directory/managed-identities-azure-resources/how-to-use-vm-token#error-handling"}.
*/
public static final int HTTP_TOO_MANY_REQUESTS = 429;

public static final char CHAR_FORWARD_SLASH = '/';
public static final char CHAR_EXCLAMATION_POINT = '!';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,18 @@ public String getRequestId() {
return this.requestId;
}

/**
Constructs an instance of HttpException with detailed information about an HTTP error response.
This exception is designed to encapsulate details of an HTTP error response, providing context about the error
encountered during an HTTP operation. It includes the HTTP error code, the associated request ID, an error message,
the URL that triggered the error, the content type of the response, and the response body.
@param httpErrorCode The HTTP error code indicating the nature of the encountered error.
@param requestId The unique identifier associated with the corresponding HTTP request.
@param message A descriptive error message providing additional information about the encountered error.
@param url The URL that resulted in the HTTP error response.
@param contentType The content type of the HTTP response.
@param body The body of the HTTP response, containing more details about the error.
*/
public HttpException(
final int httpErrorCode,
final String requestId,
Expand Down Expand Up @@ -341,6 +353,19 @@ private static boolean isRecoverableFailure(IOException e) {
|| e instanceof FileNotFoundException);
}

/**
Retrieves an Azure OAuth token for authentication through a single API call.
This method facilitates the acquisition of an OAuth token from Azure Active Directory
to enable secure authentication for various services. It supports both Managed Service Identity (MSI)
tokens and non-MSI tokens based on the provided parameters.
@param authEndpoint The URL endpoint for OAuth token retrieval.
@param payload The payload to be included in the token request. This typically contains grant type and
any required parameters for token acquisition.
@param headers A Hashtable containing additional HTTP headers to be included in the token request.
@param httpMethod The HTTP method to be used for the token request (e.g., GET, POST).
@param isMsi A boolean flag indicating whether to request a Managed Service Identity (MSI) token or not.
@return An AzureADToken object containing the acquired OAuth token and associated metadata.
*/
public static AzureADToken getTokenSingleCall(String authEndpoint,
anmolanmol1234 marked this conversation as resolved.
Show resolved Hide resolved
String payload, Hashtable<String, String> headers, String httpMethod,
boolean isMsi)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.apache.hadoop.classification.VisibleForTesting;

import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.HTTP_CONTINUE;
import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.HTTP_TOO_MANY_REQUESTS;

/**
* Retry policy used by AbfsClient.
Expand Down Expand Up @@ -58,13 +59,6 @@ public class ExponentialRetryPolicy {
*/
private static final double MAX_RANDOM_RATIO = 1.2;

/**
* Qualifies for retry based on
* https://learn.microsoft.com/en-us/azure/active-directory/
* managed-identities-azure-resources/how-to-use-vm-token#error-handling
*/
private static final int HTTP_TOO_MANY_REQUESTS = 429;

/**
* Holds the random number generator used to calculate randomized backoff intervals
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import java.lang.reflect.Field;
import java.net.HttpURLConnection;
import java.util.Date;

import org.junit.Assume;
import org.junit.Test;
import org.mockito.MockedStatic;
import org.mockito.Mockito;
Expand All @@ -33,6 +35,7 @@
import org.apache.hadoop.fs.azurebfs.oauth2.MsiTokenProvider;
import org.apache.hadoop.fs.azurebfs.services.ExponentialRetryPolicy;

import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.HTTP_TOO_MANY_REQUESTS;
import static org.apache.hadoop.fs.azurebfs.constants.FileSystemConfigurations.DEFAULT_AZURE_OAUTH_TOKEN_FETCH_RETRY_MAX_ATTEMPTS;
import static org.apache.hadoop.test.LambdaTestUtils.intercept;
import static org.junit.Assume.assumeThat;
Expand All @@ -55,8 +58,6 @@
public final class ITestAbfsMsiTokenProvider
extends AbstractAbfsIntegrationTest {

private static final int HTTP_TOO_MANY_REQUESTS = 429;

public ITestAbfsMsiTokenProvider() throws Exception {
super();
}
Expand Down Expand Up @@ -102,7 +103,6 @@ private String getTrimmedPasswordString(AbfsConfiguration conf, String key,

/**
* Test to verify that token fetch is retried for throttling errors (too many requests 429).
* @throws Exception
*/
@Test
public void testRetryForThrottling() throws Exception {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.reset;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyZeroInteractions;
import static org.mockito.Mockito.verifyNoInteractions;
import static org.mockito.Mockito.when;

/**
Expand Down Expand Up @@ -90,7 +90,7 @@ public void testContainerRetries() throws Exception {

providerService.buildContainerRetry(mockLauncher, getConfig(),
componentLaunchContext, componentInstance);
verifyZeroInteractions(mockLauncher);
verifyNoInteractions(mockLauncher);


//OnFailure restart policy
Expand Down
Loading