From 0d56d0af51e985fcb059a3905e89adc54d84546b Mon Sep 17 00:00:00 2001 From: Tyler Ouyang Date: Fri, 1 Nov 2024 10:42:18 -0700 Subject: [PATCH 01/13] wip OKhttp --- deploy-service/common/pom.xml | 6 + .../deployservice/common/ChangeFeedJob.java | 10 +- .../deployservice/handler/CommonHandler.java | 3 +- .../deployservice/handler/DeployHandler.java | 10 +- .../deployservice/handler/WebhookJob.java | 20 +-- .../rodimus/RodimusManagerImpl.java | 73 +++----- .../deployservice/scm/GithubManager.java | 9 +- .../deployservice/common/KnoxKeyTest.java | 132 +------------- .../rodimus/RodimusManagerImplTest.java | 92 ++++++---- deploy-service/pom.xml | 10 +- deploy-service/teletraanservice/pom.xml | 4 +- deploy-service/universal/pom.xml | 12 +- .../teletraan/universal/http/HttpClient.java | 161 ++++++++++++++++++ 13 files changed, 293 insertions(+), 249 deletions(-) create mode 100644 deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java diff --git a/deploy-service/common/pom.xml b/deploy-service/common/pom.xml index c7656f7784..8b6ca5446d 100644 --- a/deploy-service/common/pom.xml +++ b/deploy-service/common/pom.xml @@ -207,5 +207,11 @@ org.quartz-scheduler quartz + + com.squareup.okhttp3 + mockwebserver + ${okhttp.version} + test + diff --git a/deploy-service/common/src/main/java/com/pinterest/deployservice/common/ChangeFeedJob.java b/deploy-service/common/src/main/java/com/pinterest/deployservice/common/ChangeFeedJob.java index 6a268c1c3f..a7c8de2603 100644 --- a/deploy-service/common/src/main/java/com/pinterest/deployservice/common/ChangeFeedJob.java +++ b/deploy-service/common/src/main/java/com/pinterest/deployservice/common/ChangeFeedJob.java @@ -15,9 +15,9 @@ */ package com.pinterest.deployservice.common; +import com.pinterest.teletraan.universal.http.HttpClient; import java.lang.reflect.Field; import java.util.ArrayList; -import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -30,19 +30,17 @@ public final class ChangeFeedJob implements Callable { private static final Logger LOG = LoggerFactory.getLogger(ChangeFeedJob.class); - private final int RETRIES = 3; + private static final HttpClient httpClient = new HttpClient(); private String payload; private String changeFeedUrl; private Object oriObj; private Object curObj; - private HTTPClient httpClient; public ChangeFeedJob(String payload, String changeFeedUrl, Object oriObj, Object curObj) { this.payload = payload; this.changeFeedUrl = changeFeedUrl; this.oriObj = oriObj; this.curObj = curObj; - this.httpClient = new HTTPClient(); } private static String toStringRepresentation(Object object) throws IllegalAccessException { @@ -184,9 +182,7 @@ public Void call() { if (!StringUtils.isEmpty(changeFeedUrl)) { LOG.info(String.format("Send change feed %s to %s", payload, changeFeedUrl)); - Map headers = new HashMap<>(); - headers.put("Content-Type", "application/json"); - httpClient.post(changeFeedUrl, payload, headers, RETRIES); + httpClient.post(changeFeedUrl, payload, null); } } catch (Throwable t) { LOG.error(String.format("Failed to send change feed: %s", payload), t); diff --git a/deploy-service/common/src/main/java/com/pinterest/deployservice/handler/CommonHandler.java b/deploy-service/common/src/main/java/com/pinterest/deployservice/handler/CommonHandler.java index 671498f91d..cdcd24bd86 100644 --- a/deploy-service/common/src/main/java/com/pinterest/deployservice/handler/CommonHandler.java +++ b/deploy-service/common/src/main/java/com/pinterest/deployservice/handler/CommonHandler.java @@ -363,8 +363,7 @@ void transition(DeployBean deployBean, DeployBean newDeployBean, EnvironBean env dataHandler.getDataById( envBean.getWebhooks_config_id(), WebhookDataFactory.class); if (webhooks != null && !CollectionUtils.isEmpty(webhooks.getPostDeployHooks())) { - jobPool.submit( - new WebhookJob(webhooks.getPostDeployHooks(), deployBean, envBean)); + jobPool.submit(new WebhookJob(webhooks.getPostDeployHooks(), deployBean)); LOG.info("Submitted post deploy hook job for deploy {}.", deployId); } diff --git a/deploy-service/common/src/main/java/com/pinterest/deployservice/handler/DeployHandler.java b/deploy-service/common/src/main/java/com/pinterest/deployservice/handler/DeployHandler.java index 9e939d8fd7..d8fe9d6499 100644 --- a/deploy-service/common/src/main/java/com/pinterest/deployservice/handler/DeployHandler.java +++ b/deploy-service/common/src/main/java/com/pinterest/deployservice/handler/DeployHandler.java @@ -43,7 +43,6 @@ import com.pinterest.deployservice.common.CommonUtils; import com.pinterest.deployservice.common.Constants; import com.pinterest.deployservice.common.DeployInternalException; -import com.pinterest.deployservice.common.HTTPClient; import com.pinterest.deployservice.common.StateMachines; import com.pinterest.deployservice.common.WebhookDataFactory; import com.pinterest.deployservice.dao.AgentDAO; @@ -56,6 +55,7 @@ import com.pinterest.deployservice.db.DatabaseUtil; import com.pinterest.deployservice.db.DeployQueryFilter; import com.pinterest.deployservice.scm.SourceControlManagerProxy; +import com.pinterest.teletraan.universal.http.HttpClient; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; @@ -102,6 +102,7 @@ public class DeployHandler implements DeployHandlerInterface { "This stage requires SOX builds. A private build cannot be used in a sox-compliant stage."; static final String ERROR_STAGE_REQUIRES_SOX_BUILD_COMPLIANT_SOURCE = "This stage requires SOX builds. The build must be from a sox-compliant source. Contact your sox administrators."; + private static final HttpClient httpClient = new HttpClient(); private final DeployDAO deployDAO; private final EnvironDAO environDAO; @@ -126,11 +127,9 @@ private final class NotifyJob implements Callable { private final EnvironBean envBean; private final DeployBean newDeployBean; private final DeployBean oldDeployBean; - private final HTTPClient httpClient; private final CommonHandler commonHandler; private final String deployBoardUrlPrefix; private final String changeFeedUrl; - private static final int RETRIES = 3; public NotifyJob( EnvironBean envBean, @@ -142,7 +141,6 @@ public NotifyJob( this.envBean = envBean; this.newDeployBean = newDeployBean; this.oldDeployBean = oldDeployBean; - this.httpClient = new HTTPClient(); this.commonHandler = commonHandler; this.deployBoardUrlPrefix = deployBoardUrlPrefix; this.changeFeedUrl = changeFeedUrl; @@ -165,7 +163,7 @@ private void updateChangeFeed() { envBean.getExternal_id()); Map headers = new HashMap<>(); headers.put("Content-Type", "application/json"); - httpClient.post(changeFeedUrl, feedPayload, headers, RETRIES); + httpClient.post(changeFeedUrl, feedPayload, headers); LOG.info("Send change feed {} to {} successfully", feedPayload, changeFeedUrl); } catch (Exception e) { LOG.error( @@ -397,7 +395,7 @@ String internalDeploy(EnvironBean envBean, DeployBean deployBean) throws Excepti EnvWebHookBean webhook = dataHandler.getDataById(envBean.getWebhooks_config_id(), WebhookDataFactory.class); if (webhook != null && !CollectionUtils.isEmpty(webhook.getPreDeployHooks())) { - jobPool.submit(new WebhookJob(webhook.getPreDeployHooks(), deployBean, envBean)); + jobPool.submit(new WebhookJob(webhook.getPreDeployHooks(), deployBean)); } LOG.info("Submitted notify job for deploy {}", deployId); diff --git a/deploy-service/common/src/main/java/com/pinterest/deployservice/handler/WebhookJob.java b/deploy-service/common/src/main/java/com/pinterest/deployservice/handler/WebhookJob.java index 583d1b5395..91925d3ae6 100644 --- a/deploy-service/common/src/main/java/com/pinterest/deployservice/handler/WebhookJob.java +++ b/deploy-service/common/src/main/java/com/pinterest/deployservice/handler/WebhookJob.java @@ -17,9 +17,8 @@ import com.google.common.base.Splitter; import com.pinterest.deployservice.bean.DeployBean; -import com.pinterest.deployservice.bean.EnvironBean; import com.pinterest.deployservice.bean.WebHookBean; -import com.pinterest.deployservice.common.HTTPClient; +import com.pinterest.teletraan.universal.http.HttpClient; import java.util.List; import java.util.Map; import java.util.concurrent.Callable; @@ -29,22 +28,17 @@ public class WebhookJob implements Callable { private static final Logger LOG = LoggerFactory.getLogger(WebhookJob.class); + private static final HttpClient httpClient = new HttpClient(); private List webhooks; private DeployBean deployBean; - private EnvironBean envBean; - private HTTPClient httpClient; - private final int RETRIES = 3; - public WebhookJob(List webhooks, DeployBean deployBean, EnvironBean envBean) { + public WebhookJob(List webhooks, DeployBean deployBean) { this.webhooks = webhooks; this.deployBean = deployBean; - this.envBean = envBean; - httpClient = new HTTPClient(); } public Void call() { for (WebHookBean webhook : webhooks) { - // TODO use method, version and headers, use jersey or apache http client // TODO we transform $TELETRAAN_NAME into the actual values, currently we support // $TELETRAAN_DEPLOY_ID, $TELETRAAN_DEPLOY_START, $TELETRAAN_NUMERIC_DEPLOY_STATE // We should support more such as $TELETRAAN_COMMIT, $TELETRAAN_ENV_NAME etc. @@ -97,13 +91,13 @@ public Void call() { try { // Supports http GET, PUT, POST, DELETE if (method.equalsIgnoreCase("GET")) { - httpClient.get(url, null, null, headers, RETRIES); + httpClient.get(url, null, headers); } else if (method.equalsIgnoreCase("POST")) { - httpClient.post(url, bodyString, headers, RETRIES); + httpClient.post(url, bodyString, headers); } else if (method.equalsIgnoreCase("PUT")) { - httpClient.put(url, bodyString, headers, RETRIES); + httpClient.put(url, bodyString, headers); } else if (method.equalsIgnoreCase("DELETE")) { - httpClient.delete(url, bodyString, headers, RETRIES); + httpClient.delete(url, bodyString, headers); } else { LOG.error("Current http method " + method + " is not supported!"); } diff --git a/deploy-service/common/src/main/java/com/pinterest/deployservice/rodimus/RodimusManagerImpl.java b/deploy-service/common/src/main/java/com/pinterest/deployservice/rodimus/RodimusManagerImpl.java index 5e62bd8d6b..59afbe3c10 100644 --- a/deploy-service/common/src/main/java/com/pinterest/deployservice/rodimus/RodimusManagerImpl.java +++ b/deploy-service/common/src/main/java/com/pinterest/deployservice/rodimus/RodimusManagerImpl.java @@ -22,9 +22,9 @@ import com.google.gson.JsonObject; import com.google.gson.JsonPrimitive; import com.google.gson.reflect.TypeToken; -import com.pinterest.deployservice.common.HTTPClient; import com.pinterest.deployservice.common.KeyReader; import com.pinterest.deployservice.common.KnoxKeyReader; +import com.pinterest.teletraan.universal.http.HttpClient; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -36,16 +36,9 @@ public class RodimusManagerImpl implements RodimusManager { private static final Logger LOG = LoggerFactory.getLogger(RodimusManagerImpl.class); - private static final int RETRIES = 3; - - protected static enum Verb { - GET, - POST, - DELETE - }; + private final HttpClient httpClient; private String rodimusUrl; - private HTTPClient httpClient; private Map headers; private Gson gson; private KeyReader knoxKeyReader = new KnoxKeyReader(); @@ -58,22 +51,25 @@ public RodimusManagerImpl( String httpProxyPort) throws Exception { this.rodimusUrl = rodimusUrl; - this.httpClient = new HTTPClient(); this.headers = new HashMap<>(); - this.headers.put("Content-Type", "application/json"); this.headers.put("Accept", "*/*"); int httpProxyPortInt; - if (Boolean.TRUE.equals(useProxy)) { + if (useProxy) { try { httpProxyPortInt = Integer.parseInt(httpProxyPort); } catch (NumberFormatException exception) { LOG.error(httpProxyPort, exception); throw exception; } - this.httpClient = new HTTPClient(useProxy, httpProxyAddr, httpProxyPortInt); + this.httpClient = + new HttpClient( + useProxy, + httpProxyAddr, + httpProxyPortInt, + this::authorizationHeaderSupplier); } else { - this.httpClient = new HTTPClient(); + this.httpClient = new HttpClient(this::authorizationHeaderSupplier); } if (StringUtils.isNotBlank(knoxKey)) { @@ -99,35 +95,12 @@ public boolean shouldSkipClass(Class c) { } } - private void setAuthorization() throws Exception { + private String authorizationHeaderSupplier() throws IllegalStateException { String knoxKey = knoxKeyReader.getKey(); - if (knoxKey == null) { - throw new IllegalStateException("Rodimus knoxKey is null"); - } - this.headers.put("Authorization", String.format("token %s", knoxKey)); - } - - private String switchHttpClient(Verb verb, String url, String payload) throws Exception { - String res = null; - - switch (verb) { - case GET: - res = httpClient.get(url, payload, null, this.headers, RETRIES); - break; - case POST: - res = httpClient.post(url, payload, this.headers, RETRIES); - break; - case DELETE: - res = httpClient.delete(url, payload, this.headers, RETRIES); - break; + if (StringUtils.isBlank(knoxKey)) { + throw new IllegalStateException("Rodimus knoxKey is blank"); } - - return res; - } - - protected String callHttpClient(Verb verb, String url, String payload) throws Exception { - setAuthorization(); - return switchHttpClient(verb, url, payload); + return String.format("token %s", knoxKey); } @Override @@ -147,8 +120,8 @@ public void terminateHostsByClusterName( String.format( "%s/v1/clusters/%s/hosts?replaceHost=%s", this.rodimusUrl, clusterName, replaceHost); - callHttpClient(Verb.DELETE, url, gson.toJson(hostIds)); - } // terminateHostsByClusterName + httpClient.delete(url, gson.toJson(hostIds), headers); + } @Override public Collection getTerminatedHosts(Collection hostIds) throws Exception { @@ -158,14 +131,14 @@ public Collection getTerminatedHosts(Collection hostIds) throws // NOTE: it's better to call this function with single host id String url = String.format("%s/v1/hosts/state?actionType=%s", rodimusUrl, "TERMINATED"); - String res = callHttpClient(Verb.POST, url, gson.toJson(hostIds)); + String res = httpClient.post(url, gson.toJson(hostIds), headers); return gson.fromJson(res, new TypeToken>() {}.getType()); - } // getTerminatedHosts + } @Override public Long getClusterInstanceLaunchGracePeriod(String clusterName) throws Exception { String url = String.format("%s/v1/groups/%s/config", rodimusUrl, clusterName); - String res = callHttpClient(Verb.GET, url, null); + String res = httpClient.get(url, null, null); JsonObject jsonObject = gson.fromJson(res, JsonObject.class); if (jsonObject == null || jsonObject.isJsonNull()) { @@ -178,14 +151,14 @@ public Long getClusterInstanceLaunchGracePeriod(String clusterName) throws Excep } return launchGracePeriod.getAsLong(); - } // getClusterInstanceLaunchGracePeriod + } @Override public Map> getEc2Tags(Collection hostIds) throws Exception { String url = String.format("%s/v1/host_ec2tags", rodimusUrl); - String res = callHttpClient(Verb.POST, url, gson.toJson(hostIds)); + String res = httpClient.post(url, gson.toJson(hostIds), headers); return gson.fromJson(res, new TypeToken>>() {}.getType()); - } // getEc2Tags -} // class RodimusManagerImpl + } +} diff --git a/deploy-service/common/src/main/java/com/pinterest/deployservice/scm/GithubManager.java b/deploy-service/common/src/main/java/com/pinterest/deployservice/scm/GithubManager.java index 5638ed4b64..cda1bb8494 100644 --- a/deploy-service/common/src/main/java/com/pinterest/deployservice/scm/GithubManager.java +++ b/deploy-service/common/src/main/java/com/pinterest/deployservice/scm/GithubManager.java @@ -19,8 +19,8 @@ import com.google.gson.reflect.TypeToken; import com.pinterest.deployservice.bean.CommitBean; import com.pinterest.deployservice.common.EncryptionUtils; -import com.pinterest.deployservice.common.HTTPClient; import com.pinterest.deployservice.common.KnoxKeyReader; +import com.pinterest.teletraan.universal.http.HttpClient; import java.io.IOException; import java.util.HashMap; import java.util.LinkedList; @@ -42,6 +42,7 @@ public class GithubManager extends BaseManager { private static final Logger LOG = LoggerFactory.getLogger(GithubManager.class); private static final String UNKNOWN_LOGIN = "UNKNOWN"; + private static final HttpClient httpClient = new HttpClient(); private static final long TOKEN_TTL_MILLIS = 600000; // token expires after 10 minutes private final String apiPrefix; private final String urlPrefix; @@ -179,14 +180,13 @@ public String getUrlPrefix() { @Override public CommitBean getCommit(String repo, String sha) throws Exception { - HTTPClient httpClient = new HTTPClient(); String url = String.format("%s/repos/%s/commits/%s", apiPrefix, repo, sha); // TODO: Do not RETRY since it will timeout the thrift caller, need to revisit setHeaders(); String jsonPayload; try { - jsonPayload = httpClient.get(url, null, null, headers, 1); + jsonPayload = httpClient.get(url, null, headers); } catch (IOException e) { // an IOException (and its subclasses) in this case indicates that the commit hash is // not found in the repo. @@ -208,7 +208,6 @@ public CommitBean getCommit(String repo, String sha) throws Exception { @Override public Queue getCommits(String repo, String startSha, boolean keepHead, String path) throws Exception { - HTTPClient httpClient = new HTTPClient(); String url = String.format("%s/repos/%s/commits", apiPrefix, repo); // TODO: Do not RETRY since it will timeout the thrift caller, need to revisit @@ -217,7 +216,7 @@ public Queue getCommits(String repo, String startSha, boolean keepHe params.put("path", path); setHeaders(); - String jsonPayload = httpClient.get(url, null, params, headers, 1); + String jsonPayload = httpClient.get(url, params, headers); Queue CommitBeans = new LinkedList<>(); GsonBuilder builder = new GsonBuilder(); Map[] jsonMaps = diff --git a/deploy-service/common/src/test/java/com/pinterest/deployservice/common/KnoxKeyTest.java b/deploy-service/common/src/test/java/com/pinterest/deployservice/common/KnoxKeyTest.java index a6ca8c30e2..a865b6d6dd 100644 --- a/deploy-service/common/src/test/java/com/pinterest/deployservice/common/KnoxKeyTest.java +++ b/deploy-service/common/src/test/java/com/pinterest/deployservice/common/KnoxKeyTest.java @@ -51,14 +51,11 @@ private static enum Answer { private static final String postAnswerTag = "{\"i-001\":{\"Name\": \"devapp-example1\"},\"i-002\":{\"Name\": \"devrestricted-example2\"}}"; private static final String postAnswerArray = "[\"i-001\",\"i-002\"]"; - private static final String getAnswerValue = "{\"launchLatencyTh\": 10}"; private RodimusManager rodimusManager = null; private KnoxKeyReader mockKnoxKeyReader; - private HTTPClient mockHttpClient; private List answerList; private String[] testKey = new String[2]; - private String postAnswerReturn = null; @BeforeEach void setUp() throws Exception { @@ -70,80 +67,17 @@ void setUp() throws Exception { mockKnoxKeyReader = Mockito.mock(KnoxKeyReader.class); // Create mock for httpClient - mockHttpClient = Mockito.mock(HTTPClient.class); rodimusManager = new RodimusManagerImpl("http://localhost", "teletraan:test", false, "", ""); // Allocate answerList answerList = new ArrayList(); - mockClasses(rodimusManager, mockKnoxKeyReader, mockHttpClient); - - when(mockHttpClient.get( - Mockito.anyString(), - Mockito.any(), - Mockito.any(), - Mockito.anyMap(), - Mockito.anyInt())) - .thenAnswer(invocation -> this.getAnswer(invocation)); - - when(mockHttpClient.post( - Mockito.anyString(), - Mockito.anyString(), - Mockito.anyMap(), - Mockito.anyInt())) - .thenAnswer(invocation -> this.postAnswer(invocation)); - - when(mockHttpClient.delete( - Mockito.anyString(), - Mockito.anyString(), - Mockito.anyMap(), - Mockito.anyInt())) - .thenAnswer(invocation -> this.deleteAnswer(invocation)); + mockClasses(rodimusManager, mockKnoxKeyReader); } - // ### terminateHostsByClusterName tests ### - @Test - void terminateHostsByClusterName_Ok() throws Exception { - // All working as expected - when(this.mockKnoxKeyReader.getKey()).thenReturn(this.testKey[1]); - - try { - this.rodimusManager.terminateHostsByClusterName( - "cluster", Collections.singletonList("i-001")); - } catch (Exception e) { - fail("Unexpected exception: " + e); - } - - final Answer[] expected = {Answer.NULL}; - assertArrayEquals(expected, answerList.toArray()); - } - - @Test - void terminateHostsByClusterName_ErrorOk() throws Exception { - // Token does not work, refresh and retry, second try works - when(this.mockKnoxKeyReader.getKey()).thenReturn(this.testKey[0], this.testKey[1]); - - Exception exception = - assertThrows( - DeployInternalException.class, - () -> { - this.rodimusManager.terminateHostsByClusterName( - "cluster", Collections.singletonList("i-001")); - }); - assertTrue(exception.getMessage().contains(msgUnauthException)); - - try { - this.rodimusManager.terminateHostsByClusterName( - "cluster", Collections.singletonList("i-001")); - } catch (Exception e) { - fail("Unexpected exception: " + e); - } - final Answer[] expected = {Answer.EXCEPTION, Answer.NULL}; - assertArrayEquals(expected, answerList.toArray()); - } @Test void terminateHostsByClusterName_MultipleError() { @@ -359,74 +293,12 @@ void getEC2Tags_MultipleError() { // ### HELPER METHODS ### private void mockClasses( - RodimusManager rodimusMngr, KnoxKeyReader mokKnox, HTTPClient mokHttpClient) + RodimusManager rodimusMngr, KnoxKeyReader mokKnox) throws Exception { // Modify fsKnox to use our mock Field classKnox = rodimusMngr.getClass().getDeclaredField("knoxKeyReader"); classKnox.setAccessible(true); classKnox.set(rodimusMngr, mokKnox); classKnox.setAccessible(false); - - // Modify httpClient to use our mock - Field classHttpClient = rodimusMngr.getClass().getDeclaredField("httpClient"); - classHttpClient.setAccessible(true); - classHttpClient.set(rodimusMngr, mokHttpClient); - classHttpClient.setAccessible(false); - } - - private String getToken(Map headers) { - // Get token out of Map of headers - for (Map.Entry entry : headers.entrySet()) { - if (entry.getKey() == "Authorization") return entry.getValue(); - } - return null; - } - - private Object deleteAnswer(InvocationOnMock invocation) throws Exception { - // HTTPClient "DELETE" answer method - Object[] args = invocation.getArguments(); - - Map headers = (Map) args[2]; - String token = getToken(headers); - - if (Objects.equals(token, "token bbb")) { - this.answerList.add(Answer.NULL); - return null; - } else { - this.answerList.add(Answer.EXCEPTION); - throw new DeployInternalException(msgUnauthException); - } - } - - private Object postAnswer(InvocationOnMock invocation) throws Exception { - // HTTPClient "POST" answer method - Object[] args = invocation.getArguments(); - - Map headers = (Map) args[2]; - String token = getToken(headers); - - if (Objects.equals(token, "token bbb")) { - this.answerList.add(Answer.ARRAY); - return this.postAnswerReturn; - } else { - this.answerList.add(Answer.EXCEPTION); - throw new DeployInternalException(msgUnauthException); - } - } - - private Object getAnswer(InvocationOnMock invocation) throws Exception { - // HTTPClient "GET" answer method - Object[] args = invocation.getArguments(); - - Map headers = (Map) args[3]; - String token = getToken(headers); - - if (Objects.equals(token, "token bbb")) { - this.answerList.add(Answer.LATENCY); - return getAnswerValue; - } else { - this.answerList.add(Answer.EXCEPTION); - throw new DeployInternalException(msgUnauthException); - } } } diff --git a/deploy-service/common/src/test/java/com/pinterest/deployservice/rodimus/RodimusManagerImplTest.java b/deploy-service/common/src/test/java/com/pinterest/deployservice/rodimus/RodimusManagerImplTest.java index 6513e00aa4..9dcc8a2ffb 100644 --- a/deploy-service/common/src/test/java/com/pinterest/deployservice/rodimus/RodimusManagerImplTest.java +++ b/deploy-service/common/src/test/java/com/pinterest/deployservice/rodimus/RodimusManagerImplTest.java @@ -15,55 +15,87 @@ */ package com.pinterest.deployservice.rodimus; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.verify; +import static org.junit.jupiter.api.Assertions.fail; +import static org.mockito.Mockito.when; -import com.pinterest.deployservice.common.HTTPClient; -import com.pinterest.deployservice.rodimus.RodimusManagerImpl.Verb; -import java.lang.reflect.Field; -import java.util.Map; +import java.io.IOException; +import java.util.Collections; +import okhttp3.mockwebserver.MockResponse; +import okhttp3.mockwebserver.MockWebServer; +import okhttp3.mockwebserver.RecordedRequest; + +import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import org.mockito.ArgumentCaptor; -import org.mockito.Mockito; +import org.mockito.stubbing.Answer; + +import com.pinterest.deployservice.common.DeployInternalException; -public class RodimusManagerImplTest { +class RodimusManagerImplTest { private RodimusManagerImpl sut; - private HTTPClient mockHttpClient; + private static MockWebServer mockWebServer; private String TEST_URL = "testUrl"; - @BeforeEach - public void setUp() throws Exception { - sut = new RodimusManagerImpl("http://localhost", null, false, "", ""); + @BeforeAll + public static void setUp() throws Exception { + mockWebServer = new MockWebServer(); + mockWebServer.start(); + } - mockHttpClient = Mockito.mock(HTTPClient.class); - Field classHttpClient = sut.getClass().getDeclaredField("httpClient"); - classHttpClient.setAccessible(true); - classHttpClient.set(sut, mockHttpClient); - classHttpClient.setAccessible(false); + @BeforeEach + public void setUpEach() throws Exception { + sut = new RodimusManagerImpl(mockWebServer.url(TEST_URL).toString(), null, false, "", ""); } @Test - public void nullKnoxKey_defaultKeyIsUsed() throws Exception { - sut.callHttpClient(Verb.DELETE, TEST_URL, null); - - ArgumentCaptor> argument = - ArgumentCaptor.forClass((Class>) (Class) Map.class); - verify(mockHttpClient).delete(eq(TEST_URL), eq(null), argument.capture(), eq(3)); + void nullKnoxKey_defaultKeyIsUsed() throws Exception { + mockWebServer.enqueue(new MockResponse().setBody("[]")); + sut.getTerminatedHosts(Collections.singletonList("testHost")); - Map headers = argument.getValue(); - assertTrue(headers.containsKey("Authorization")); - assertEquals("token defaultKeyContent", headers.get("Authorization")); + RecordedRequest request = mockWebServer.takeRequest(); + assertEquals("token defaultKeyContent", request.getHeader("Authorization")); } @Test - public void invalidKnoxKey_exceptionThrown() throws Exception { + void invalidKnoxKey_exceptionThrown() throws Exception { RodimusManagerImpl sut = - new RodimusManagerImpl("http://localhost", "invalidRodimusKnoxKey", false, "", ""); + new RodimusManagerImpl( + mockWebServer.url(TEST_URL).toString(), + "invalidRodimusKnoxKey", + false, + "", + ""); assertThrows( - IllegalStateException.class, () -> sut.callHttpClient(Verb.DELETE, TEST_URL, null)); + IllegalStateException.class, + () -> sut.getTerminatedHosts(Collections.singletonList(""))); + } + + @Test + void terminateHostsByClusterName_Ok() throws Exception { + mockWebServer.enqueue(new MockResponse().setResponseCode(200)); + try { + sut.terminateHostsByClusterName( + "cluster", Collections.singletonList("i-001")); + } catch (Exception e) { + fail("Unexpected exception: " + e); + } + } + + @Test + void terminateHostsByClusterName_ErrorOk() throws Exception { + mockWebServer.enqueue(new MockResponse().setResponseCode(401)); + + Exception exception = + assertThrows( + IOException.class, + () -> { + sut.terminateHostsByClusterName( + "cluster", Collections.singletonList("i-001")); + }); + assertTrue(exception.getMessage().contains("unauthenticated")); } } diff --git a/deploy-service/pom.xml b/deploy-service/pom.xml index 92e5441b28..09a3ccd20d 100644 --- a/deploy-service/pom.xml +++ b/deploy-service/pom.xml @@ -20,6 +20,7 @@ 5.10.1 1.7 2.30.0 + 4.12.0 ${maven.repo.local}/com/pinterest/teletraan/${project.artifactId}/${project.version} @@ -33,8 +34,10 @@ io.micrometer - micrometer-core + micrometer-bom ${micrometer.version} + pom + import io.projectreactor @@ -65,6 +68,11 @@ common 0.1-SNAPSHOT + + com.squareup.okhttp3 + okhttp + 4.12.0 + diff --git a/deploy-service/teletraanservice/pom.xml b/deploy-service/teletraanservice/pom.xml index 3e722d9990..cc81a59055 100644 --- a/deploy-service/teletraanservice/pom.xml +++ b/deploy-service/teletraanservice/pom.xml @@ -95,7 +95,7 @@ com.squareup.okhttp3 mockwebserver - 4.12.0 + ${okhttp.version} test @@ -189,4 +189,4 @@ - \ No newline at end of file + diff --git a/deploy-service/universal/pom.xml b/deploy-service/universal/pom.xml index 5c98ac54c5..7689581ec4 100644 --- a/deploy-service/universal/pom.xml +++ b/deploy-service/universal/pom.xml @@ -29,12 +29,14 @@ io.micrometer micrometer-commons - ${micrometer.version} io.micrometer micrometer-registry-opentsdb - ${micrometer.version} + + + io.micrometer + micrometer-observation io.projectreactor @@ -87,6 +89,10 @@ dropwizard-auth ${dropwizard.version} + + com.squareup.okhttp3 + okhttp + @@ -103,7 +109,7 @@ com.squareup.okhttp3 mockwebserver - 4.12.0 + ${okhttp.version} test diff --git a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java new file mode 100644 index 0000000000..7dcd1b626d --- /dev/null +++ b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java @@ -0,0 +1,161 @@ +/** + * Copyright (c) 2024 Pinterest, Inc. + * + * Licensed 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 com.pinterest.teletraan.universal.http; + +import io.micrometer.core.instrument.Metrics; +import io.micrometer.core.instrument.binder.okhttp3.OkHttpObservationInterceptor; +import io.micrometer.core.instrument.observation.DefaultMeterObservationHandler; +import io.micrometer.observation.ObservationRegistry; +import java.io.IOException; +import java.net.InetSocketAddress; +import java.net.Proxy; +import java.time.Duration; +import java.util.Map; +import java.util.function.Supplier; +import lombok.Getter; +import okhttp3.Headers; +import okhttp3.HttpUrl; +import okhttp3.MediaType; +import okhttp3.OkHttpClient; +import okhttp3.Request; +import okhttp3.RequestBody; +import okhttp3.Response; + +public class HttpClient { + private static final MediaType MEDIA_TYPE_JSON = MediaType.parse("application/json"); + private static final ObservationRegistry observationRegistry = ObservationRegistry.create(); + private static final OkHttpClient sharedOkHttpClient = new OkHttpClient(); + /** + * The HTTP client used for making network requests. This client is an instance of {@link + * OkHttpClient}. + */ + @Getter private final OkHttpClient httpClient; + + public HttpClient() { + this(false, null, 0, null); + } + + public HttpClient(Supplier authorizationSupplier) { + this(false, null, 0, authorizationSupplier); + } + + public HttpClient(boolean useProxy, String httpProxyAddr, int httpProxyPort) { + this(useProxy, httpProxyAddr, httpProxyPort, null); + } + + public HttpClient( + boolean useProxy, + String httpProxyAddr, + int httpProxyPort, + Supplier authorizationSupplier) { + observationRegistry + .observationConfig() + .observationHandler(new DefaultMeterObservationHandler(Metrics.globalRegistry)); + + OkHttpClient.Builder clientBuilder = + sharedOkHttpClient + .newBuilder() + .connectTimeout(Duration.ofSeconds(15)) + .readTimeout(Duration.ofSeconds(15)) + .addInterceptor(observationInterceptorBuilder().build()); + if (useProxy) { + String proxyAddr = httpProxyAddr != null ? httpProxyAddr : "localhost"; + int proxyPort = httpProxyPort > 0 ? httpProxyPort : 19193; + clientBuilder.proxy( + new Proxy(Proxy.Type.HTTP, new InetSocketAddress(proxyAddr, proxyPort))); + } + if (authorizationSupplier != null) { + clientBuilder.addInterceptor( + (chain) -> { + Request request = chain.request(); + return chain.proceed( + request.newBuilder() + .header("Authorization", authorizationSupplier.get()) + .build()); + }); + } + httpClient = clientBuilder.build(); + } + + private static OkHttpObservationInterceptor.Builder observationInterceptorBuilder() { + return OkHttpObservationInterceptor.builder(observationRegistry, "okhttp.requests"); + } + + public String get(String url, Map params, Map headers) + throws IOException { + Request request = + new Request.Builder() + .url(buildUrl(url, params)) + .headers(buildHeaders(headers)) + .build(); + return makeCall(request); + } + + public String post(String url, String body, Map headers) throws IOException { + Request request = + new Request.Builder() + .url(buildUrl(url, null)) + .headers(buildHeaders(headers)) + .post(RequestBody.create(body, MEDIA_TYPE_JSON)) + .build(); + return makeCall(request); + } + + public String put(String url, String body, Map headers) throws IOException { + Request request = + new Request.Builder() + .url(buildUrl(url, null)) + .headers(buildHeaders(headers)) + .put(RequestBody.create(body, MEDIA_TYPE_JSON)) + .build(); + return makeCall(request); + } + + public String delete(String url, String body, Map headers) throws IOException { + Request request = + new Request.Builder() + .url(buildUrl(url, null)) + .headers(buildHeaders(headers)) + .delete(RequestBody.create(body, MEDIA_TYPE_JSON)) + .build(); + return makeCall(request); + } + + public Headers buildHeaders(Map rawHeaders) { + Headers.Builder headersBuilder = new Headers.Builder(); + if (rawHeaders != null) { + rawHeaders.forEach(headersBuilder::set); + } + return headersBuilder.build(); + } + + public HttpUrl buildUrl(String url, Map params) { + HttpUrl.Builder urlBuilder = HttpUrl.parse(url).newBuilder(); + if (params != null) { + params.forEach(urlBuilder::addQueryParameter); + } + return urlBuilder.build(); + } + + String makeCall(Request request) throws IOException { + try (Response response = httpClient.newCall(request).execute()) { + if (!response.isSuccessful()) { + throw new IOException("Unexpected code " + response); + } + return response.body().string(); + } + } +} From c48434579e841ec24704517d074dc124c47ad987 Mon Sep 17 00:00:00 2001 From: Tyler Ouyang Date: Fri, 1 Nov 2024 16:27:56 -0700 Subject: [PATCH 02/13] wip --- .../deployservice/common/KnoxKeyTest.java | 231 +----------------- .../rodimus/RodimusManagerImplTest.java | 50 ++-- .../teletraan/universal/http/HttpClient.java | 47 +++- .../universal/http/RetryInterceptor.java | 68 ++++++ 4 files changed, 140 insertions(+), 256 deletions(-) create mode 100644 deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/RetryInterceptor.java diff --git a/deploy-service/common/src/test/java/com/pinterest/deployservice/common/KnoxKeyTest.java b/deploy-service/common/src/test/java/com/pinterest/deployservice/common/KnoxKeyTest.java index a865b6d6dd..55198ad621 100644 --- a/deploy-service/common/src/test/java/com/pinterest/deployservice/common/KnoxKeyTest.java +++ b/deploy-service/common/src/test/java/com/pinterest/deployservice/common/KnoxKeyTest.java @@ -15,26 +15,13 @@ */ package com.pinterest.deployservice.common; -import static org.junit.jupiter.api.Assertions.assertArrayEquals; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.junit.jupiter.api.Assertions.fail; -import static org.mockito.Mockito.when; - import com.pinterest.deployservice.rodimus.RodimusManager; import com.pinterest.deployservice.rodimus.RodimusManagerImpl; import java.lang.reflect.Field; import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collections; import java.util.List; -import java.util.Map; -import java.util.Objects; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; import org.mockito.Mockito; -import org.mockito.invocation.InvocationOnMock; @SuppressWarnings("unchecked") class KnoxKeyTest { @@ -76,225 +63,9 @@ void setUp() throws Exception { mockClasses(rodimusManager, mockKnoxKeyReader); } - - - - @Test - void terminateHostsByClusterName_MultipleError() { - // Token does not work - when(this.mockKnoxKeyReader.getKey()).thenReturn(this.testKey[0], this.testKey[0]); - - for (int i = 1; i <= 2; i++) { - Exception exception = - assertThrows( - DeployInternalException.class, - () -> { - this.rodimusManager.terminateHostsByClusterName( - "cluster", Collections.singletonList("i-001")); - }); - - assertTrue(exception.getMessage().contains(msgUnauthException)); - } - - final Answer[] expected = {Answer.EXCEPTION, Answer.EXCEPTION}; - assertArrayEquals(expected, answerList.toArray()); - } - - // ### getTerminatedHosts tests ### - - @Test - void getTerminatedHosts_Ok() throws Exception { - // All working as expected - when(this.mockKnoxKeyReader.getKey()).thenReturn(this.testKey[1]); - this.postAnswerReturn = postAnswerArray; - - try { - this.rodimusManager.getTerminatedHosts(Arrays.asList("i-001", "i-002")); - } catch (Exception e) { - fail("Unexpected exception: " + e); - } - - final Answer[] expected = {Answer.ARRAY}; - assertArrayEquals(expected, answerList.toArray()); - } - - @Test - void getTerminatedHosts_ErrorOk() throws Exception { - // Token does not work, refresh and retry, second try works - when(this.mockKnoxKeyReader.getKey()).thenReturn(this.testKey[0], this.testKey[1]); - this.postAnswerReturn = postAnswerArray; - - Exception exception = - assertThrows( - DeployInternalException.class, - () -> { - this.rodimusManager.getTerminatedHosts(Arrays.asList("i-001", "i-002")); - }); - assertTrue(exception.getMessage().contains(msgUnauthException)); - - try { - this.rodimusManager.getTerminatedHosts(Arrays.asList("i-001", "i-002")); - } catch (Exception e) { - fail("Unexpected exception: " + e); - } - - final Answer[] expected = {Answer.EXCEPTION, Answer.ARRAY}; - assertArrayEquals(expected, answerList.toArray()); - } - - @Test - void getTerminatedHosts_MultipleError() { - // Token does not work, refresh does not offer new token - when(this.mockKnoxKeyReader.getKey()).thenReturn(this.testKey[0], this.testKey[0]); - this.postAnswerReturn = postAnswerArray; - - for (int i = 1; i <= 2; i++) { - Exception exception = - assertThrows( - DeployInternalException.class, - () -> { - this.rodimusManager.getTerminatedHosts( - Arrays.asList("i-001", "i-002")); - }); - - assertTrue(exception.getMessage().contains(msgUnauthException)); - } - - final Answer[] expected = {Answer.EXCEPTION, Answer.EXCEPTION}; - assertArrayEquals(expected, answerList.toArray()); - } - - // ### getClusterInstanceLaunchGracePeriod tests - - @Test - void getClusterInstanceLaunchGracePeriod_Ok() throws Exception { - // All working as expected - when(this.mockKnoxKeyReader.getKey()).thenReturn(this.testKey[1]); - long res = 0; - try { - res = this.rodimusManager.getClusterInstanceLaunchGracePeriod("cluster"); - assertEquals(res, (long) 10); - } catch (Exception e) { - fail("Unexpected exception: " + e); - } - - final Answer[] expected = {Answer.LATENCY}; - assertArrayEquals(expected, answerList.toArray()); - } - - @Test - void getClusterInstanceLaunchGracePeriod_test() throws Exception { - // Token does not work, refresh and retry, second try works - when(this.mockKnoxKeyReader.getKey()).thenReturn(this.testKey[0], this.testKey[1]); - this.postAnswerReturn = postAnswerArray; - - Exception exception = - assertThrows( - DeployInternalException.class, - () -> { - this.rodimusManager.getClusterInstanceLaunchGracePeriod("cluster"); - }); - assertTrue(exception.getMessage().contains("HTTP request failed, status")); - - try { - this.rodimusManager.getClusterInstanceLaunchGracePeriod("cluster"); - } catch (Exception e) { - fail("Unexpected exception: " + e); - } - - final Answer[] expected = {Answer.EXCEPTION, Answer.LATENCY}; - assertArrayEquals(expected, answerList.toArray()); - } - - @Test - void getClusterInstanceLaunchGracePeriod_MultipleError() { - // Token does not work, refresh does not offer new token - when(this.mockKnoxKeyReader.getKey()).thenReturn(this.testKey[0], this.testKey[0]); - - for (int i = 1; i <= 2; i++) { - Exception exception = - assertThrows( - DeployInternalException.class, - () -> { - this.rodimusManager.getClusterInstanceLaunchGracePeriod("cluster"); - }); - - assertTrue(exception.getMessage().contains("HTTP request failed, status")); - } - - final Answer[] expected = {Answer.EXCEPTION, Answer.EXCEPTION}; - assertArrayEquals(expected, answerList.toArray()); - } - - // ### getEC2Tags tests ### - - @Test - void getEC2Tags_Ok() throws Exception { - // All working as expected - - when(this.mockKnoxKeyReader.getKey()).thenReturn(this.testKey[1]); - this.postAnswerReturn = postAnswerTag; - - try { - rodimusManager.getEc2Tags(Arrays.asList("i-001", "i-002")); - } catch (Exception e) { - fail("Unexpected exception: " + e); - } - - final Answer[] expected = {Answer.ARRAY}; - assertArrayEquals(expected, answerList.toArray()); - } - - @Test - void getEC2Tags_ErrorOk() throws Exception { - // Token does not work, refresh and retry, second try works - when(this.mockKnoxKeyReader.getKey()).thenReturn(this.testKey[0], this.testKey[1]); - this.postAnswerReturn = postAnswerTag; - - Exception exception = - assertThrows( - DeployInternalException.class, - () -> { - this.rodimusManager.getEc2Tags(Arrays.asList("i-001", "i-002")); - }); - assertTrue(exception.getMessage().contains("HTTP request failed, status")); - - try { - this.rodimusManager.getEc2Tags(Arrays.asList("i-001", "i-002")); - } catch (Exception e) { - fail("Unexpected exception: " + e); - } - - final Answer[] expected = {Answer.EXCEPTION, Answer.ARRAY}; - assertArrayEquals(expected, answerList.toArray()); - } - - @Test - void getEC2Tags_MultipleError() { - // Token does not work, refresh does not offer new token - when(this.mockKnoxKeyReader.getKey()).thenReturn(this.testKey[0], this.testKey[0]); - this.postAnswerReturn = postAnswerTag; - - for (int i = 1; i <= 2; i++) { - Exception exception = - assertThrows( - DeployInternalException.class, - () -> { - this.rodimusManager.getEc2Tags(Arrays.asList("i-001", "i-002")); - }); - - assertTrue(exception.getMessage().contains("HTTP request failed, status")); - } - - final Answer[] expected = {Answer.EXCEPTION, Answer.EXCEPTION}; - assertArrayEquals(expected, answerList.toArray()); - } - // ### HELPER METHODS ### - private void mockClasses( - RodimusManager rodimusMngr, KnoxKeyReader mokKnox) - throws Exception { + private void mockClasses(RodimusManager rodimusMngr, KnoxKeyReader mokKnox) throws Exception { // Modify fsKnox to use our mock Field classKnox = rodimusMngr.getClass().getDeclaredField("knoxKeyReader"); classKnox.setAccessible(true); diff --git a/deploy-service/common/src/test/java/com/pinterest/deployservice/rodimus/RodimusManagerImplTest.java b/deploy-service/common/src/test/java/com/pinterest/deployservice/rodimus/RodimusManagerImplTest.java index 9dcc8a2ffb..0802241cd7 100644 --- a/deploy-service/common/src/test/java/com/pinterest/deployservice/rodimus/RodimusManagerImplTest.java +++ b/deploy-service/common/src/test/java/com/pinterest/deployservice/rodimus/RodimusManagerImplTest.java @@ -15,30 +15,26 @@ */ package com.pinterest.deployservice.rodimus; -import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; -import static org.mockito.Mockito.when; -import java.io.IOException; +import com.pinterest.teletraan.universal.http.HttpClient.ClientErrorException; +import com.pinterest.teletraan.universal.http.HttpClient.ServerErrorException; +import java.util.Arrays; import java.util.Collections; import okhttp3.mockwebserver.MockResponse; import okhttp3.mockwebserver.MockWebServer; import okhttp3.mockwebserver.RecordedRequest; - import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import org.mockito.stubbing.Answer; - -import com.pinterest.deployservice.common.DeployInternalException; class RodimusManagerImplTest { private RodimusManagerImpl sut; private static MockWebServer mockWebServer; - private String TEST_URL = "testUrl"; + private String TEST_URL = "testUrl/"; @BeforeAll public static void setUp() throws Exception { @@ -76,26 +72,50 @@ void invalidKnoxKey_exceptionThrown() throws Exception { @Test void terminateHostsByClusterName_Ok() throws Exception { - mockWebServer.enqueue(new MockResponse().setResponseCode(200)); + mockWebServer.enqueue(new MockResponse()); try { - sut.terminateHostsByClusterName( - "cluster", Collections.singletonList("i-001")); + sut.terminateHostsByClusterName("cluster", Collections.singletonList("i-001")); } catch (Exception e) { fail("Unexpected exception: " + e); } } - @Test - void terminateHostsByClusterName_ErrorOk() throws Exception { + @Test + void terminateHostsByClusterName_Error() throws Exception { mockWebServer.enqueue(new MockResponse().setResponseCode(401)); Exception exception = assertThrows( - IOException.class, + ClientErrorException.class, + () -> { + sut.terminateHostsByClusterName( + "cluster", Collections.singletonList("i-001")); + }); + assertTrue(exception.getMessage().contains("401")); + } + + @Test + void terminateHostsByClusterName_ServerError() throws Exception { + mockWebServer.enqueue(new MockResponse().setResponseCode(500)); + + Exception exception = + assertThrows( + ServerErrorException.class, () -> { sut.terminateHostsByClusterName( "cluster", Collections.singletonList("i-001")); }); - assertTrue(exception.getMessage().contains("unauthenticated")); + assertTrue(exception.getMessage().contains("500")); + } + + @Test + void getTerminatedHosts_Ok() throws Exception { + mockWebServer.enqueue(new MockResponse().setBody("[]")); + + try { + sut.getTerminatedHosts(Arrays.asList("i-001", "i-002")); + } catch (Exception e) { + fail("Unexpected exception: " + e); + } } } diff --git a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java index 7dcd1b626d..5b61b95080 100644 --- a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java +++ b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java @@ -26,6 +26,7 @@ import java.util.Map; import java.util.function.Supplier; import lombok.Getter; +import lombok.extern.slf4j.Slf4j; import okhttp3.Headers; import okhttp3.HttpUrl; import okhttp3.MediaType; @@ -34,15 +35,12 @@ import okhttp3.RequestBody; import okhttp3.Response; +@Slf4j public class HttpClient { private static final MediaType MEDIA_TYPE_JSON = MediaType.parse("application/json"); private static final ObservationRegistry observationRegistry = ObservationRegistry.create(); private static final OkHttpClient sharedOkHttpClient = new OkHttpClient(); - /** - * The HTTP client used for making network requests. This client is an instance of {@link - * OkHttpClient}. - */ - @Getter private final OkHttpClient httpClient; + @Getter private final OkHttpClient okHttpClient; public HttpClient() { this(false, null, 0, null); @@ -78,7 +76,8 @@ public HttpClient( new Proxy(Proxy.Type.HTTP, new InetSocketAddress(proxyAddr, proxyPort))); } if (authorizationSupplier != null) { - clientBuilder.addInterceptor( + clientBuilder + .addInterceptor( (chain) -> { Request request = chain.request(); return chain.proceed( @@ -87,7 +86,7 @@ public HttpClient( .build()); }); } - httpClient = clientBuilder.build(); + okHttpClient = clientBuilder.build(); } private static OkHttpObservationInterceptor.Builder observationInterceptorBuilder() { @@ -151,11 +150,37 @@ public HttpUrl buildUrl(String url, Map params) { } String makeCall(Request request) throws IOException { - try (Response response = httpClient.newCall(request).execute()) { - if (!response.isSuccessful()) { - throw new IOException("Unexpected code " + response); + try (Response response = okHttpClient.newCall(request).execute()) { + int responseCode = response.code(); + String responseBody = response.body().string(); + + if (responseCode >= 200 && responseCode < 300) { + return responseBody; + } else if (responseCode >= 400 && responseCode < 500) { + log.error("Client error: {} - {}", responseCode, responseBody); + throw new ClientErrorException( + "Client error: " + responseCode + " - " + responseBody); + } else if (responseCode >= 500) { + log.error("Server error: {} - {}", responseCode, responseBody); + throw new ServerErrorException( + "Server error: " + responseCode + " - " + responseBody); + } else { + log.error("Unexpected response code: {} - {}", responseCode, responseBody); + throw new IOException( + "Unexpected response code: " + responseCode + " - " + responseBody); } - return response.body().string(); + } + } + + public static class ClientErrorException extends IOException { + public ClientErrorException(String message) { + super(message); + } + } + + public static class ServerErrorException extends IOException { + public ServerErrorException(String message) { + super(message); } } } diff --git a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/RetryInterceptor.java b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/RetryInterceptor.java new file mode 100644 index 0000000000..4adb4c8ca4 --- /dev/null +++ b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/RetryInterceptor.java @@ -0,0 +1,68 @@ +/** + * Copyright (c) 2024 Pinterest, Inc. + * + * Licensed 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 com.pinterest.teletraan.universal.http; + +import java.io.IOException; +import java.util.concurrent.TimeUnit; +import okhttp3.Interceptor; +import okhttp3.Request; +import okhttp3.Response; + +public class RetryInterceptor implements Interceptor { + private final int maxRetries; + private final long retryInterval; + + public RetryInterceptor(int maxRetries, long retryInterval) { + this.maxRetries = maxRetries; + this.retryInterval = retryInterval; + } + + @Override + public Response intercept(Chain chain) throws IOException { + Request request = chain.request(); + Response response = null; + IOException lastException = null; + + for (int i = 0; i < maxRetries; i++) { + try { + response = chain.proceed(request); + if (response.isSuccessful() || !shouldRetry(response)) { + return response; + } + } catch (IOException e) { + lastException = e; + } + + try { + TimeUnit.MILLISECONDS.sleep(retryInterval); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IOException("Retry interrupted", e); + } + } + + if (response != null) { + return response; + } else { + throw lastException != null ? lastException : new IOException("Unknown error"); + } + } + + private boolean shouldRetry(Response response) { + int code = response.code(); + return code == 500 || code == 502 || code == 503 || code == 504; + } +} From 5b1fc0ccdebf7bc3a8211acafef38298d4d1e2c7 Mon Sep 17 00:00:00 2001 From: Tyler Ouyang Date: Tue, 5 Nov 2024 16:04:44 -0800 Subject: [PATCH 03/13] Add tests --- .../rodimus/RodimusManagerImpl.java | 30 +- .../deployservice/scm/GithubManager.java | 2 +- .../rodimus/RodimusManagerImplTest.java | 166 ++++++++--- .../teletraan/universal/http/HttpClient.java | 114 ++++---- .../universal/http/RetryInterceptor.java | 5 +- .../universal/http/HttpClientTest.java | 270 ++++++++++++++++++ 6 files changed, 471 insertions(+), 116 deletions(-) create mode 100644 deploy-service/universal/src/test/java/com/pinterest/teletraan/universal/http/HttpClientTest.java diff --git a/deploy-service/common/src/main/java/com/pinterest/deployservice/rodimus/RodimusManagerImpl.java b/deploy-service/common/src/main/java/com/pinterest/deployservice/rodimus/RodimusManagerImpl.java index 59afbe3c10..c129635aa9 100644 --- a/deploy-service/common/src/main/java/com/pinterest/deployservice/rodimus/RodimusManagerImpl.java +++ b/deploy-service/common/src/main/java/com/pinterest/deployservice/rodimus/RodimusManagerImpl.java @@ -28,7 +28,6 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; -import java.util.HashMap; import java.util.Map; import org.apache.commons.lang3.StringUtils; import org.slf4j.Logger; @@ -38,10 +37,9 @@ public class RodimusManagerImpl implements RodimusManager { private static final Logger LOG = LoggerFactory.getLogger(RodimusManagerImpl.class); private final HttpClient httpClient; - private String rodimusUrl; - private Map headers; - private Gson gson; - private KeyReader knoxKeyReader = new KnoxKeyReader(); + private final String rodimusUrl; + private final Gson gson; + private final KeyReader knoxKeyReader = new KnoxKeyReader(); public RodimusManagerImpl( String rodimusUrl, @@ -51,10 +49,9 @@ public RodimusManagerImpl( String httpProxyPort) throws Exception { this.rodimusUrl = rodimusUrl; - this.headers = new HashMap<>(); - this.headers.put("Accept", "*/*"); int httpProxyPortInt; + HttpClient.HttpClientBuilder clientBuilder = HttpClient.builder(); if (useProxy) { try { httpProxyPortInt = Integer.parseInt(httpProxyPort); @@ -62,15 +59,10 @@ public RodimusManagerImpl( LOG.error(httpProxyPort, exception); throw exception; } - this.httpClient = - new HttpClient( - useProxy, - httpProxyAddr, - httpProxyPortInt, - this::authorizationHeaderSupplier); - } else { - this.httpClient = new HttpClient(this::authorizationHeaderSupplier); + clientBuilder.httpProxyAddr(httpProxyAddr).httpProxyPort(httpProxyPortInt); } + this.httpClient = + clientBuilder.authorizationSupplier(this::authorizationHeaderSupplier).build(); if (StringUtils.isNotBlank(knoxKey)) { knoxKeyReader.init(knoxKey); @@ -83,7 +75,7 @@ public RodimusManagerImpl( .create(); } - private class CustomExclusionStrategy implements ExclusionStrategy { + private static class CustomExclusionStrategy implements ExclusionStrategy { @Override public boolean shouldSkipField(FieldAttributes f) { return f.getName().equals("__isset_bit_vector"); @@ -120,7 +112,7 @@ public void terminateHostsByClusterName( String.format( "%s/v1/clusters/%s/hosts?replaceHost=%s", this.rodimusUrl, clusterName, replaceHost); - httpClient.delete(url, gson.toJson(hostIds), headers); + httpClient.delete(url, gson.toJson(hostIds), null); } @Override @@ -131,7 +123,7 @@ public Collection getTerminatedHosts(Collection hostIds) throws // NOTE: it's better to call this function with single host id String url = String.format("%s/v1/hosts/state?actionType=%s", rodimusUrl, "TERMINATED"); - String res = httpClient.post(url, gson.toJson(hostIds), headers); + String res = httpClient.post(url, gson.toJson(hostIds), null); return gson.fromJson(res, new TypeToken>() {}.getType()); } @@ -157,7 +149,7 @@ public Long getClusterInstanceLaunchGracePeriod(String clusterName) throws Excep public Map> getEc2Tags(Collection hostIds) throws Exception { String url = String.format("%s/v1/host_ec2tags", rodimusUrl); - String res = httpClient.post(url, gson.toJson(hostIds), headers); + String res = httpClient.post(url, gson.toJson(hostIds), null); return gson.fromJson(res, new TypeToken>>() {}.getType()); } diff --git a/deploy-service/common/src/main/java/com/pinterest/deployservice/scm/GithubManager.java b/deploy-service/common/src/main/java/com/pinterest/deployservice/scm/GithubManager.java index cda1bb8494..016bf7a8e8 100644 --- a/deploy-service/common/src/main/java/com/pinterest/deployservice/scm/GithubManager.java +++ b/deploy-service/common/src/main/java/com/pinterest/deployservice/scm/GithubManager.java @@ -42,7 +42,7 @@ public class GithubManager extends BaseManager { private static final Logger LOG = LoggerFactory.getLogger(GithubManager.class); private static final String UNKNOWN_LOGIN = "UNKNOWN"; - private static final HttpClient httpClient = new HttpClient(); + private static final HttpClient httpClient = HttpClient.builder().maxRetries(1).build(); private static final long TOKEN_TTL_MILLIS = 600000; // token expires after 10 minutes private final String apiPrefix; private final String urlPrefix; diff --git a/deploy-service/common/src/test/java/com/pinterest/deployservice/rodimus/RodimusManagerImplTest.java b/deploy-service/common/src/test/java/com/pinterest/deployservice/rodimus/RodimusManagerImplTest.java index 0802241cd7..2077c83a8f 100644 --- a/deploy-service/common/src/test/java/com/pinterest/deployservice/rodimus/RodimusManagerImplTest.java +++ b/deploy-service/common/src/test/java/com/pinterest/deployservice/rodimus/RodimusManagerImplTest.java @@ -15,107 +15,187 @@ */ package com.pinterest.deployservice.rodimus; +import static org.junit.Assert.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.junit.jupiter.api.Assertions.fail; -import com.pinterest.teletraan.universal.http.HttpClient.ClientErrorException; -import com.pinterest.teletraan.universal.http.HttpClient.ServerErrorException; -import java.util.Arrays; +import java.util.Collection; import java.util.Collections; +import java.util.List; +import java.util.Map; +import javax.ws.rs.ClientErrorException; +import javax.ws.rs.ServerErrorException; +import okhttp3.mockwebserver.Dispatcher; import okhttp3.mockwebserver.MockResponse; import okhttp3.mockwebserver.MockWebServer; import okhttp3.mockwebserver.RecordedRequest; -import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; class RodimusManagerImplTest { - private RodimusManagerImpl sut; + private static final String TEST_CLUSTER = "cluster1"; + private static final List HOST_IDS = Collections.singletonList("i-001"); + private static final String TEST_PATH = "/testUrl"; + private static MockWebServer mockWebServer; - private String TEST_URL = "testUrl/"; + private RodimusManagerImpl sut; - @BeforeAll - public static void setUp() throws Exception { + @BeforeEach + public void setUpEach() throws Exception { mockWebServer = new MockWebServer(); mockWebServer.start(); + sut = new RodimusManagerImpl(mockWebServer.url(TEST_PATH).toString(), null, false, "", ""); } - @BeforeEach - public void setUpEach() throws Exception { - sut = new RodimusManagerImpl(mockWebServer.url(TEST_URL).toString(), null, false, "", ""); + @AfterEach + public void tearDown() throws Exception { + mockWebServer.shutdown(); + } + + @Test + void testConstructorInValidProxyConfig() { + assertThrows( + NumberFormatException.class, + () -> { + new RodimusManagerImpl(TEST_PATH, null, true, "localhost", "invalidPort"); + }); } @Test - void nullKnoxKey_defaultKeyIsUsed() throws Exception { + void testNullKnoxKeyUsesDefaultKey() throws Exception { + // return 401 to trigger authentication flow + mockWebServer.enqueue(new MockResponse().setResponseCode(401)); mockWebServer.enqueue(new MockResponse().setBody("[]")); sut.getTerminatedHosts(Collections.singletonList("testHost")); + // discard first request RecordedRequest request = mockWebServer.takeRequest(); + request = mockWebServer.takeRequest(); assertEquals("token defaultKeyContent", request.getHeader("Authorization")); } @Test - void invalidKnoxKey_exceptionThrown() throws Exception { + void testInvalidKnoxKeyThrowsException() throws Exception { + // return 401 to trigger authentication flow + mockWebServer.enqueue(new MockResponse().setResponseCode(401)); RodimusManagerImpl sut = new RodimusManagerImpl( - mockWebServer.url(TEST_URL).toString(), + mockWebServer.url(TEST_PATH).toString(), "invalidRodimusKnoxKey", false, "", ""); - assertThrows( - IllegalStateException.class, - () -> sut.getTerminatedHosts(Collections.singletonList(""))); + assertThrows(IllegalStateException.class, () -> sut.getTerminatedHosts(HOST_IDS)); + assertEquals(1, mockWebServer.getRequestCount()); } @Test - void terminateHostsByClusterName_Ok() throws Exception { + void testTerminateHostsByClusterNameOk() { mockWebServer.enqueue(new MockResponse()); - try { - sut.terminateHostsByClusterName("cluster", Collections.singletonList("i-001")); - } catch (Exception e) { - fail("Unexpected exception: " + e); - } + + assertDoesNotThrow( + () -> { + sut.terminateHostsByClusterName(TEST_CLUSTER, HOST_IDS); + }); } @Test - void terminateHostsByClusterName_Error() throws Exception { - mockWebServer.enqueue(new MockResponse().setResponseCode(401)); + void testTerminateHostsByClusterNameEmptyHosts() { + assertDoesNotThrow( + () -> { + sut.terminateHostsByClusterName(TEST_CLUSTER, Collections.emptyList()); + }); + } - Exception exception = + @Test + void testTerminateHostsByClusterNameClientError() { + mockWebServer.enqueue(new MockResponse().setResponseCode(404)); + + ClientErrorException exception = assertThrows( ClientErrorException.class, () -> { - sut.terminateHostsByClusterName( - "cluster", Collections.singletonList("i-001")); + sut.terminateHostsByClusterName(TEST_CLUSTER, HOST_IDS); }); - assertTrue(exception.getMessage().contains("401")); + assertEquals(404, exception.getResponse().getStatus()); + assertEquals(1, mockWebServer.getRequestCount()); } @Test - void terminateHostsByClusterName_ServerError() throws Exception { - mockWebServer.enqueue(new MockResponse().setResponseCode(500)); - - Exception exception = + void testTerminateHostsByClusterNameServerError() { + mockWebServer.setDispatcher(new ServerErrorDispatcher()); + ServerErrorException exception = assertThrows( ServerErrorException.class, () -> { - sut.terminateHostsByClusterName( - "cluster", Collections.singletonList("i-001")); + sut.terminateHostsByClusterName(TEST_CLUSTER, HOST_IDS); }); - assertTrue(exception.getMessage().contains("500")); + assertEquals(500, exception.getResponse().getStatus()); + assertEquals(3, mockWebServer.getRequestCount()); } @Test - void getTerminatedHosts_Ok() throws Exception { - mockWebServer.enqueue(new MockResponse().setBody("[]")); + void testGetTerminatedHostsOk() throws Exception { + mockWebServer.enqueue(new MockResponse().setBody(HOST_IDS.toString())); + + Collection terminatedHosts = sut.getTerminatedHosts(HOST_IDS); + assertArrayEquals(HOST_IDS.toArray(), terminatedHosts.toArray()); + } + + @Test + void testGetTerminatedHostEmptyHostIds() throws Exception { + Collection terminatedHosts = sut.getTerminatedHosts(Collections.emptyList()); + assertArrayEquals(new String[] {}, terminatedHosts.toArray()); + } + + @Test + void testGetClusterInstanceLaunchGracePeriodOk() throws Exception { + mockWebServer.enqueue(new MockResponse().setBody("{\"launchLatencyTh\": 300}")); + + Long gracePeriod = sut.getClusterInstanceLaunchGracePeriod(TEST_CLUSTER); + assertEquals(300L, gracePeriod); + } + + @Test + void testGetClusterInstanceLaunchGracePeriodNullResponse() throws Exception { + mockWebServer.enqueue(new MockResponse()); + + Long gracePeriod = sut.getClusterInstanceLaunchGracePeriod(TEST_CLUSTER); + assertEquals(null, gracePeriod); + } + + @Test + void testGetClusterInstanceLaunchGracePeriodNoLaunchLatencyTh() throws Exception { + mockWebServer.enqueue(new MockResponse().setBody("{}")); + + Long gracePeriod = sut.getClusterInstanceLaunchGracePeriod(TEST_CLUSTER); + assertEquals(null, gracePeriod); + } + + @Test + void testGetEc2TagsOk() throws Exception { + String responseBody = "{\"i-001\": {\"Name\": \"test-instance\"}}"; + mockWebServer.enqueue(new MockResponse().setBody(responseBody)); + + Map> ec2Tags = sut.getEc2Tags(HOST_IDS); + assertEquals("test-instance", ec2Tags.get("i-001").get("Name")); + } + + @Test + void testGetEc2TagsEmptyResponse() throws Exception { + mockWebServer.enqueue(new MockResponse().setBody("{}")); + + Map> ec2Tags = sut.getEc2Tags(HOST_IDS); + assertTrue(ec2Tags.isEmpty()); + } - try { - sut.getTerminatedHosts(Arrays.asList("i-001", "i-002")); - } catch (Exception e) { - fail("Unexpected exception: " + e); + static class ServerErrorDispatcher extends Dispatcher { + @Override + public MockResponse dispatch(RecordedRequest request) { + return new MockResponse().setResponseCode(500); } } } diff --git a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java index 5b61b95080..61cd61d17e 100644 --- a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java +++ b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java @@ -25,6 +25,10 @@ import java.time.Duration; import java.util.Map; import java.util.function.Supplier; +import javax.ws.rs.ClientErrorException; +import javax.ws.rs.ServerErrorException; +import lombok.AccessLevel; +import lombok.Builder; import lombok.Getter; import lombok.extern.slf4j.Slf4j; import okhttp3.Headers; @@ -40,25 +44,43 @@ public class HttpClient { private static final MediaType MEDIA_TYPE_JSON = MediaType.parse("application/json"); private static final ObservationRegistry observationRegistry = ObservationRegistry.create(); private static final OkHttpClient sharedOkHttpClient = new OkHttpClient(); - @Getter private final OkHttpClient okHttpClient; - public HttpClient() { - this(false, null, 0, null); - } + private static final int DEFAULT_MAX_RETRIES = 3; + private static final boolean DEFAULT_USE_PROXY = false; + private static final String DEFAULT_HTTP_PROXY_ADDR = "localhost"; + private static final int DEFAULT_PROXY_PORT = 19193; + private static final long RETRY_INTERVAL = 500; - public HttpClient(Supplier authorizationSupplier) { - this(false, null, 0, authorizationSupplier); - } + @Getter private final OkHttpClient okHttpClient; - public HttpClient(boolean useProxy, String httpProxyAddr, int httpProxyPort) { - this(useProxy, httpProxyAddr, httpProxyPort, null); + public HttpClient() { + this(null, null, null, null, null, null); } + @Builder(access = AccessLevel.PUBLIC) public HttpClient( - boolean useProxy, + Integer maxRetries, + Long retryInterval, + Boolean useProxy, String httpProxyAddr, - int httpProxyPort, + Integer httpProxyPort, Supplier authorizationSupplier) { + if (maxRetries == null) { + maxRetries = DEFAULT_MAX_RETRIES; + } + if (useProxy == null) { + useProxy = DEFAULT_USE_PROXY; + } + if (httpProxyAddr == null) { + httpProxyAddr = DEFAULT_HTTP_PROXY_ADDR; + } + if (httpProxyPort == null) { + httpProxyPort = DEFAULT_PROXY_PORT; + } + if (retryInterval == null) { + retryInterval = RETRY_INTERVAL; + } + observationRegistry .observationConfig() .observationHandler(new DefaultMeterObservationHandler(Metrics.globalRegistry)); @@ -68,29 +90,32 @@ public HttpClient( .newBuilder() .connectTimeout(Duration.ofSeconds(15)) .readTimeout(Duration.ofSeconds(15)) - .addInterceptor(observationInterceptorBuilder().build()); + .addInterceptor(observationInterceptorBuilder().build()) + .addInterceptor(new RetryInterceptor(maxRetries, retryInterval)); if (useProxy) { - String proxyAddr = httpProxyAddr != null ? httpProxyAddr : "localhost"; - int proxyPort = httpProxyPort > 0 ? httpProxyPort : 19193; clientBuilder.proxy( - new Proxy(Proxy.Type.HTTP, new InetSocketAddress(proxyAddr, proxyPort))); + new Proxy( + Proxy.Type.HTTP, new InetSocketAddress(httpProxyAddr, httpProxyPort))); } if (authorizationSupplier != null) { - clientBuilder - .addInterceptor( - (chain) -> { - Request request = chain.request(); - return chain.proceed( - request.newBuilder() - .header("Authorization", authorizationSupplier.get()) - .build()); + clientBuilder.authenticator( + (route, response) -> { + if (response.request().header("Authorization") != null) { + return null; // Give up, we've already failed to authenticate. + } + + String credential = authorizationSupplier.get(); + return response.request() + .newBuilder() + .header("Authorization", credential) + .build(); }); } okHttpClient = clientBuilder.build(); } private static OkHttpObservationInterceptor.Builder observationInterceptorBuilder() { - return OkHttpObservationInterceptor.builder(observationRegistry, "okhttp.requests"); + return OkHttpObservationInterceptor.builder(observationRegistry, "okhttp"); } public String get(String url, Map params, Map headers) @@ -108,7 +133,7 @@ public String post(String url, String body, Map headers) throws new Request.Builder() .url(buildUrl(url, null)) .headers(buildHeaders(headers)) - .post(RequestBody.create(body, MEDIA_TYPE_JSON)) + .post(buildJsonBody(body)) .build(); return makeCall(request); } @@ -118,7 +143,7 @@ public String put(String url, String body, Map headers) throws I new Request.Builder() .url(buildUrl(url, null)) .headers(buildHeaders(headers)) - .put(RequestBody.create(body, MEDIA_TYPE_JSON)) + .put(buildJsonBody(body)) .build(); return makeCall(request); } @@ -128,7 +153,7 @@ public String delete(String url, String body, Map headers) throw new Request.Builder() .url(buildUrl(url, null)) .headers(buildHeaders(headers)) - .delete(RequestBody.create(body, MEDIA_TYPE_JSON)) + .delete(buildJsonBody(body)) .build(); return makeCall(request); } @@ -149,38 +174,25 @@ public HttpUrl buildUrl(String url, Map params) { return urlBuilder.build(); } + public RequestBody buildJsonBody(String json) { + if (json == null) { + return null; + } + return RequestBody.create(json, MEDIA_TYPE_JSON); + } + String makeCall(Request request) throws IOException { try (Response response = okHttpClient.newCall(request).execute()) { int responseCode = response.code(); - String responseBody = response.body().string(); + String responseBody = response.body() != null ? response.body().string() : ""; - if (responseCode >= 200 && responseCode < 300) { + if (response.isSuccessful()) { return responseBody; } else if (responseCode >= 400 && responseCode < 500) { - log.error("Client error: {} - {}", responseCode, responseBody); - throw new ClientErrorException( - "Client error: " + responseCode + " - " + responseBody); - } else if (responseCode >= 500) { - log.error("Server error: {} - {}", responseCode, responseBody); - throw new ServerErrorException( - "Server error: " + responseCode + " - " + responseBody); + throw new ClientErrorException(responseBody, responseCode); } else { - log.error("Unexpected response code: {} - {}", responseCode, responseBody); - throw new IOException( - "Unexpected response code: " + responseCode + " - " + responseBody); + throw new ServerErrorException(responseBody, responseCode); } } } - - public static class ClientErrorException extends IOException { - public ClientErrorException(String message) { - super(message); - } - } - - public static class ServerErrorException extends IOException { - public ServerErrorException(String message) { - super(message); - } - } } diff --git a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/RetryInterceptor.java b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/RetryInterceptor.java index 4adb4c8ca4..51f424a86c 100644 --- a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/RetryInterceptor.java +++ b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/RetryInterceptor.java @@ -47,7 +47,8 @@ public Response intercept(Chain chain) throws IOException { } try { - TimeUnit.MILLISECONDS.sleep(retryInterval); + long backoff = (long) Math.pow(2, i) * retryInterval; + TimeUnit.MILLISECONDS.sleep(backoff); } catch (InterruptedException e) { Thread.currentThread().interrupt(); throw new IOException("Retry interrupted", e); @@ -63,6 +64,6 @@ public Response intercept(Chain chain) throws IOException { private boolean shouldRetry(Response response) { int code = response.code(); - return code == 500 || code == 502 || code == 503 || code == 504; + return code == 429 || code == 500 || code == 502 || code == 503 || code == 504; } } diff --git a/deploy-service/universal/src/test/java/com/pinterest/teletraan/universal/http/HttpClientTest.java b/deploy-service/universal/src/test/java/com/pinterest/teletraan/universal/http/HttpClientTest.java new file mode 100644 index 0000000000..bcfc0480c0 --- /dev/null +++ b/deploy-service/universal/src/test/java/com/pinterest/teletraan/universal/http/HttpClientTest.java @@ -0,0 +1,270 @@ +/** + * Copyright (c) 2024 Pinterest, Inc. + * + * Licensed 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 com.pinterest.teletraan.universal.http; + +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import com.google.common.collect.ImmutableMap; +import io.micrometer.core.instrument.Metrics; +import io.micrometer.core.instrument.simple.SimpleMeterRegistry; +import java.io.IOException; +import java.util.Map; +import javax.ws.rs.ClientErrorException; +import javax.ws.rs.HttpMethod; +import javax.ws.rs.ServerErrorException; +import javax.ws.rs.WebApplicationException; +import okhttp3.mockwebserver.Dispatcher; +import okhttp3.mockwebserver.MockResponse; +import okhttp3.mockwebserver.MockWebServer; +import okhttp3.mockwebserver.RecordedRequest; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +class HttpClientTest { + private static final String TEST_PATH = "/testUrl"; + private static final String TEST_BODY = "{\"key\":\"value\"}"; + private static final Map TEST_HEADERS = ImmutableMap.of("h1", "v1", "h2", "v2"); + private static final Map TEST_PARAMS = ImmutableMap.of("p1", "v1", "p2", "v2"); + private static MockWebServer mockWebServer; + + private HttpClient sut; + + @BeforeAll + public static void setUpClass() { + Metrics.addRegistry(new SimpleMeterRegistry()); + } + + @BeforeEach + void setUp() throws IOException { + mockWebServer = new MockWebServer(); + mockWebServer.start(); + sut = new HttpClient.HttpClientBuilder().maxRetries(3).retryInterval(10L).build(); + } + + @AfterEach + public void tearDown() throws Exception { + mockWebServer.shutdown(); + } + + @ParameterizedTest + @ValueSource(ints = {429, 500, 502, 503, 504}) + void testRetry(int responseStatus) throws IOException { + mockWebServer.setDispatcher(new ServerErrorDispatcher(responseStatus)); + WebApplicationException exception = + assertThrows( + WebApplicationException.class, + () -> { + sut.get( + mockWebServer.url(TEST_PATH).toString(), + TEST_PARAMS, + TEST_HEADERS); + }); + assertEquals(responseStatus, exception.getResponse().getStatus()); + assertEquals(3, mockWebServer.getRequestCount()); + } + + @Test + void testObservability() throws IOException { + mockWebServer.enqueue(new MockResponse()); + sut.get(mockWebServer.url(TEST_PATH).toString(), null, TEST_HEADERS); + + assertNotNull( + Metrics.globalRegistry + .get("okhttp") + .tag("method", "GET") + .tag("status", "200") + .tag("host", mockWebServer.getHostName()) + .timer()); + } + + @Test + void testProxy() throws IOException, InterruptedException { + String host = "example.com:123"; + HttpClient sut = + new HttpClient.HttpClientBuilder() + .useProxy(true) + .httpProxyAddr(mockWebServer.getHostName()) + .httpProxyPort(mockWebServer.getPort()) + .build(); + + mockWebServer.enqueue(new MockResponse().setBody(TEST_BODY)); + + sut.get("http://" + host + TEST_PATH, TEST_PARAMS, TEST_HEADERS); + + RecordedRequest request = mockWebServer.takeRequest(); + assertEquals("/", request.getPath()); + assertEquals("GET", request.getMethod()); + assertEquals(TEST_HEADERS.get("h1"), request.getHeader("h1")); + assertEquals(TEST_HEADERS.get("h2"), request.getHeader("h2")); + assertEquals(host, request.getHeader("host")); + } + + @Test + void testBuildHeaders() { + okhttp3.Headers result = sut.buildHeaders(TEST_HEADERS); + assertEquals(2, result.size()); + assertEquals(TEST_HEADERS.get("h1"), result.get("h1")); + assertEquals(TEST_HEADERS.get("h2"), result.get("h2")); + } + + @Test + void testBuildHeadersNull() { + okhttp3.Headers headers = sut.buildHeaders(null); + assertEquals(0, headers.size()); + } + + @Test + void testBuildHeadersEmpty() { + okhttp3.Headers result = sut.buildHeaders(ImmutableMap.of()); + assertEquals(0, result.size()); + } + + @ParameterizedTest + @ValueSource(strings = {"", "/", TEST_PATH}) + void testBuildUrl(String path) { + String url = mockWebServer.url(path).toString(); + okhttp3.HttpUrl httpUrl = sut.buildUrl(url, TEST_PARAMS); + assertEquals(url + "?p1=v1&p2=v2", httpUrl.toString()); + } + + @Test + void testBuildJsonBody() throws IOException { + okhttp3.RequestBody jsonBody = sut.buildJsonBody(TEST_BODY); + assertEquals("application/json; charset=utf-8", jsonBody.contentType().toString()); + assertEquals(TEST_BODY.length(), jsonBody.contentLength()); + } + + @Test + void testBuildJsonBodyNull() throws IOException { + okhttp3.RequestBody result = sut.buildJsonBody(null); + assertNull(result); + } + + @Test + void testGetSuccess() throws IOException, InterruptedException { + mockWebServer.enqueue(new MockResponse().setBody(TEST_BODY)); + + String result = sut.get(mockWebServer.url(TEST_PATH).toString(), null, TEST_HEADERS); + assertEquals(TEST_BODY, result); + + RecordedRequest request = mockWebServer.takeRequest(); + assertEquals(TEST_PATH, request.getPath()); + assertEquals("GET", request.getMethod()); + assertEquals(TEST_HEADERS.get("h1"), request.getHeader("h1")); + assertEquals(TEST_HEADERS.get("h2"), request.getHeader("h2")); + } + + @Test + void testGetClientError() { + mockWebServer.enqueue(new MockResponse().setResponseCode(404)); + + ClientErrorException exception = + assertThrows( + ClientErrorException.class, + () -> { + sut.get(mockWebServer.url(TEST_PATH).toString(), null, TEST_HEADERS); + }); + assertEquals(404, exception.getResponse().getStatus()); + } + + @Test + void testGetServerError() { + mockWebServer.setDispatcher(new ServerErrorDispatcher(500)); + + ServerErrorException exception = + assertThrows( + ServerErrorException.class, + () -> { + sut.get(mockWebServer.url(TEST_PATH).toString(), null, TEST_HEADERS); + }); + assertEquals(500, exception.getResponse().getStatus()); + } + + @ParameterizedTest + @ValueSource(strings = {HttpMethod.PUT, HttpMethod.DELETE, HttpMethod.POST}) + void testRequestWithBodySuccess(String method) throws IOException, InterruptedException { + mockWebServer.enqueue(new MockResponse().setBody(TEST_BODY)); + + String result = requestWithBody(method, TEST_BODY, TEST_HEADERS); + assertEquals(TEST_BODY, result); + + RecordedRequest request = mockWebServer.takeRequest(); + assertEquals(TEST_PATH, request.getPath()); + assertEquals(method, request.getMethod()); + assertEquals(TEST_BODY, request.getBody().readUtf8()); + assertEquals(TEST_HEADERS.get("h1"), request.getHeader("h1")); + assertEquals(TEST_HEADERS.get("h2"), request.getHeader("h2")); + } + + @ParameterizedTest + @ValueSource(strings = {HttpMethod.PUT, HttpMethod.DELETE, HttpMethod.POST}) + void testRequestWithBodyClientError(String method) { + mockWebServer.enqueue(new MockResponse().setResponseCode(404)); + + ClientErrorException exception = + assertThrows( + ClientErrorException.class, + () -> requestWithBody(method, "", TEST_HEADERS)); + assertEquals(404, exception.getResponse().getStatus()); + } + + @ParameterizedTest + @ValueSource(strings = {HttpMethod.PUT, HttpMethod.DELETE, HttpMethod.POST}) + void testRequestWithBodyServerError(String method) { + mockWebServer.setDispatcher(new ServerErrorDispatcher(500)); + + ServerErrorException exception = + assertThrows( + ServerErrorException.class, + () -> requestWithBody(method, "", TEST_HEADERS)); + assertEquals(500, exception.getResponse().getStatus()); + } + + String requestWithBody(String method, String body, Map headers) + throws IOException { + switch (method) { + case HttpMethod.PUT: + return sut.put(mockWebServer.url(TEST_PATH).toString(), body, headers); + case HttpMethod.DELETE: + return sut.delete(mockWebServer.url(TEST_PATH).toString(), body, headers); + case HttpMethod.POST: + return sut.post(mockWebServer.url(TEST_PATH).toString(), body, headers); + default: + break; + } + return null; + } + + static class ServerErrorDispatcher extends Dispatcher { + private int responseStatus; + + ServerErrorDispatcher(int responseStatus) { + this.responseStatus = responseStatus; + } + + @Override + public MockResponse dispatch(RecordedRequest request) { + return new MockResponse().setResponseCode(responseStatus); + } + } +} From a2bafda2f75816a3d0523722235f18a545e3ed37 Mon Sep 17 00:00:00 2001 From: Tyler Ouyang Date: Tue, 5 Nov 2024 16:14:44 -0800 Subject: [PATCH 04/13] Update tests --- .../universal/http/HttpClientTest.java | 33 +++++++++++++++---- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/deploy-service/universal/src/test/java/com/pinterest/teletraan/universal/http/HttpClientTest.java b/deploy-service/universal/src/test/java/com/pinterest/teletraan/universal/http/HttpClientTest.java index bcfc0480c0..c8d2d5ea9b 100644 --- a/deploy-service/universal/src/test/java/com/pinterest/teletraan/universal/http/HttpClientTest.java +++ b/deploy-service/universal/src/test/java/com/pinterest/teletraan/universal/http/HttpClientTest.java @@ -68,8 +68,8 @@ public void tearDown() throws Exception { @ParameterizedTest @ValueSource(ints = {429, 500, 502, 503, 504}) - void testRetry(int responseStatus) throws IOException { - mockWebServer.setDispatcher(new ServerErrorDispatcher(responseStatus)); + void testRetry(int responseCode) throws IOException { + mockWebServer.setDispatcher(new ServerErrorDispatcher(responseCode)); WebApplicationException exception = assertThrows( WebApplicationException.class, @@ -79,7 +79,7 @@ void testRetry(int responseStatus) throws IOException { TEST_PARAMS, TEST_HEADERS); }); - assertEquals(responseStatus, exception.getResponse().getStatus()); + assertEquals(responseCode, exception.getResponse().getStatus()); assertEquals(3, mockWebServer.getRequestCount()); } @@ -119,6 +119,25 @@ void testProxy() throws IOException, InterruptedException { assertEquals(host, request.getHeader("host")); } + @Test + void testAuthentication() throws IOException, InterruptedException { + String authHeader = "Bearer token"; + HttpClient sut = + new HttpClient.HttpClientBuilder().authorizationSupplier(() -> authHeader).build(); + + mockWebServer.enqueue(new MockResponse().setResponseCode(401)); + mockWebServer.enqueue(new MockResponse().setResponseCode(200)); + + sut.get(mockWebServer.url(TEST_PATH).toString(), TEST_PARAMS, TEST_HEADERS); + + // First request triggers authentication flow + RecordedRequest request1 = mockWebServer.takeRequest(); + assertNull(request1.getHeader("Authorization")); + + RecordedRequest request2 = mockWebServer.takeRequest(); + assertEquals(authHeader, request2.getHeader("Authorization")); + } + @Test void testBuildHeaders() { okhttp3.Headers result = sut.buildHeaders(TEST_HEADERS); @@ -256,15 +275,15 @@ String requestWithBody(String method, String body, Map headers) } static class ServerErrorDispatcher extends Dispatcher { - private int responseStatus; + private int responseCode; - ServerErrorDispatcher(int responseStatus) { - this.responseStatus = responseStatus; + ServerErrorDispatcher(int responseCode) { + this.responseCode = responseCode; } @Override public MockResponse dispatch(RecordedRequest request) { - return new MockResponse().setResponseCode(responseStatus); + return new MockResponse().setResponseCode(responseCode); } } } From e9cfb1f29db28bd624d2615e4a62af45a80d6480 Mon Sep 17 00:00:00 2001 From: Tyler Ouyang Date: Tue, 5 Nov 2024 16:34:32 -0800 Subject: [PATCH 05/13] Remove legacy code --- .../deployservice/common/HTTPClient.java | 206 ------------------ .../deployservice/common/HTTPClientTest.java | 81 ------- .../deployservice/common/KnoxKeyTest.java | 75 ------- deploy-service/pom.xml | 2 +- .../teletraan/universal/http/HttpClient.java | 35 +-- 5 files changed, 9 insertions(+), 390 deletions(-) delete mode 100644 deploy-service/common/src/main/java/com/pinterest/deployservice/common/HTTPClient.java delete mode 100644 deploy-service/common/src/test/java/com/pinterest/deployservice/common/HTTPClientTest.java delete mode 100644 deploy-service/common/src/test/java/com/pinterest/deployservice/common/KnoxKeyTest.java diff --git a/deploy-service/common/src/main/java/com/pinterest/deployservice/common/HTTPClient.java b/deploy-service/common/src/main/java/com/pinterest/deployservice/common/HTTPClient.java deleted file mode 100644 index 1957cf2d3f..0000000000 --- a/deploy-service/common/src/main/java/com/pinterest/deployservice/common/HTTPClient.java +++ /dev/null @@ -1,206 +0,0 @@ -/** - * Copyright (c) 2016-2024 Pinterest, Inc. - * - * Licensed 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 com.pinterest.deployservice.common; - -import java.io.OutputStreamWriter; -import java.net.HttpURLConnection; -import java.net.InetSocketAddress; -import java.net.Proxy; -import java.net.URL; -import java.net.URLEncoder; -import java.util.Map; -import org.apache.commons.io.IOUtils; -import org.apache.commons.lang3.StringUtils; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -// A simple HttpURLConnection wrapper -public class HTTPClient { - private static final int TIMEOUT = 15 * 1000; // http timeout in 15 seconds - private static final Logger LOG = LoggerFactory.getLogger(HTTPClient.class); - public static String secretMask = "xxxxxxxxx"; - private boolean useProxy = false; - private String httpProxyAddr; - private int httpProxyPort; - - public HTTPClient() { - // HTTPClient useProxy default is false - } - - public HTTPClient(boolean useProxy, String httpProxyAddr, int httpProxyPort) { - if (Boolean.TRUE.equals(useProxy)) { - if (httpProxyAddr == null) { - throw new IllegalArgumentException( - "useProxy was configured but missing required httpProxyAddr"); - } - this.useProxy = useProxy; - this.httpProxyAddr = httpProxyAddr; - this.httpProxyPort = httpProxyPort; - } - } - - public boolean getUseProxy() { - // HTTPClient useProxy default is false - return useProxy; - } - - public String getHttpProxyAddr() { - return httpProxyAddr; - } - - public int getHttpProxyPort() { - return httpProxyPort; - } - - private String generateUrlAndQuery(String url, Map params, boolean scrubUrl) - throws Exception { - if (params == null || params.isEmpty()) { - return url; - } - StringBuilder sb = new StringBuilder(); - sb.append(url); - String prefix = "?"; - for (Map.Entry entry : params.entrySet()) { - sb.append(prefix); - prefix = "&"; - // note: scrubUrlQueryValue could be expensive with many filtered values - // consider using it only in only a DEBUG logging context in the future - String reportedValue = - scrubUrl - ? scrubUrlQueryValue(entry.getKey(), entry.getValue()) - : entry.getValue(); - sb.append( - String.format( - "%s=%s", entry.getKey(), URLEncoder.encode(reportedValue, "UTF-8"))); - } - return sb.toString(); - } - - private String scrubUrlQueryValue(String queryParamKey, String queryParamValue) { - String[] filteredQueryKeySubstrings = {"token"}; - - for (String filteredQueryKeySubstring : filteredQueryKeySubstrings) { - if (StringUtils.containsIgnoreCase(queryParamKey, filteredQueryKeySubstring)) { - return secretMask; - } - } - return queryParamValue; - } - - public String get( - String url, - String payload, - Map params, - Map headers, - int retries) - throws Exception { - return internalCall(url, params, "GET", payload, headers, retries); - } - - public String post(String url, String payload, Map headers, int retries) - throws Exception { - return internalCall(url, null, "POST", payload, headers, retries); - } - - public String put(String url, String payload, Map headers, int retries) - throws Exception { - return internalCall(url, null, "PUT", payload, headers, retries); - } - - public String delete(String url, String payload, Map headers, int retries) - throws Exception { - return internalCall(url, null, "DELETE", payload, headers, retries); - } - - private String internalCall( - String base_url, - Map params, - String method, - String payload, - Map headers, - int retries) - throws Exception { - HttpURLConnection conn = null; - Exception lastException = null; - - String scrubbedUrl = generateUrlAndQuery(base_url, params, true); - String url = generateUrlAndQuery(base_url, params, false); - - for (int i = 0; i < retries; i++) { - try { - URL urlObj = new URL(url); - if (useProxy) { - Proxy httpProxy = - new Proxy( - Proxy.Type.HTTP, - new InetSocketAddress(httpProxyAddr, httpProxyPort)); - conn = (HttpURLConnection) urlObj.openConnection(httpProxy); - } else { - conn = (HttpURLConnection) urlObj.openConnection(); - } - conn.setRequestMethod(method); - conn.setRequestProperty("Accept-Charset", "UTF-8"); - conn.setConnectTimeout(TIMEOUT); - conn.setReadTimeout(TIMEOUT); - - if (headers != null) { - for (Map.Entry entry : headers.entrySet()) { - conn.setRequestProperty(entry.getKey(), entry.getValue()); - } - } - - if (StringUtils.isNotEmpty(payload)) { - conn.setDoOutput(true); - OutputStreamWriter writer = new OutputStreamWriter(conn.getOutputStream()); - writer.write(payload); - writer.flush(); - writer.close(); - } - - String ret = IOUtils.toString(conn.getInputStream(), "UTF-8"); - int responseCode = conn.getResponseCode(); - if (responseCode >= 400) { - throw new DeployInternalException( - "HTTP request failed, status = {}, content = {}", responseCode, ret); - } - LOG.info( - "HTTP Request returned with response code {} for URL {}", - responseCode, - scrubbedUrl); - return ret; - } catch (Exception e) { - lastException = e; - String proxyMsg = ""; - if (useProxy) { - proxyMsg = String.format(" via proxy %s:%s,", httpProxyAddr, httpProxyPort); - } - LOG.error( - "Failed to send HTTP Request to {},{} with method {} with payload {}, with headers {}", - scrubbedUrl, - proxyMsg, - method, - payload, - headers, - e); - } finally { - if (conn != null) { - conn.disconnect(); - } - } - } - throw lastException; - } -} diff --git a/deploy-service/common/src/test/java/com/pinterest/deployservice/common/HTTPClientTest.java b/deploy-service/common/src/test/java/com/pinterest/deployservice/common/HTTPClientTest.java deleted file mode 100644 index d8f50ff714..0000000000 --- a/deploy-service/common/src/test/java/com/pinterest/deployservice/common/HTTPClientTest.java +++ /dev/null @@ -1,81 +0,0 @@ -/** - * Copyright (c) 2021 Pinterest, Inc. - * - * Licensed 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 com.pinterest.deployservice.common; - -import static org.junit.jupiter.api.Assertions.assertEquals; - -import java.lang.reflect.Method; -import java.util.HashMap; -import java.util.Map; -import org.junit.jupiter.api.Test; - -public class HTTPClientTest { - @Test - public void testScrubUrlQueryValue() throws Exception { - HTTPClient httpClient = new HTTPClient(); - // Future: there should be some better way to test HTTPClient private methods rather than - // use reflection on private methods - Method scrubUrlQueryValue = - HTTPClient.class.getDeclaredMethod( - "scrubUrlQueryValue", String.class, String.class); - scrubUrlQueryValue.setAccessible(true); - - assertEquals( - HTTPClient.secretMask, - scrubUrlQueryValue.invoke(httpClient, "access_token", "dangerous_stuff")); - assertEquals( - HTTPClient.secretMask, - scrubUrlQueryValue.invoke(httpClient, "token", "dangerous_stuff")); - assertEquals("some_stuff", scrubUrlQueryValue.invoke(httpClient, "s", "some_stuff")); - } - - @Test - public void testGenerateUrlAndQuery() throws Exception { - HTTPClient httpClient = new HTTPClient(); - // Future: there should be some better way to test HTTPClient private methods rather than - // use reflection on private methods - Method generateUrlAndQuery = - HTTPClient.class.getDeclaredMethod( - "generateUrlAndQuery", String.class, Map.class, boolean.class); - generateUrlAndQuery.setAccessible(true); - - String baseUrl = "example.com/example/tests/"; - - Map filteredParams1 = new HashMap<>(); - filteredParams1.put("access_token", "dangerous_stuff"); - - Map filteredParams2 = new HashMap<>(); - filteredParams2.put("token", "dangerous_stuff"); - - Map unfilteredParams1 = new HashMap<>(); - unfilteredParams1.put("s", "some_stuff"); - - // only pass 1 param to avoid flaky tests due to unordered Map - // Future: Consider how handle multiple query parameter cases when testing HTTPClient - assertEquals( - "example.com/example/tests/?access_token=" + HTTPClient.secretMask, - generateUrlAndQuery.invoke(httpClient, baseUrl, filteredParams1, true)); - assertEquals( - "example.com/example/tests/?token=" + HTTPClient.secretMask, - generateUrlAndQuery.invoke(httpClient, baseUrl, filteredParams2, true)); - assertEquals( - "example.com/example/tests/?access_token=dangerous_stuff", - generateUrlAndQuery.invoke(httpClient, baseUrl, filteredParams1, false)); - assertEquals( - "example.com/example/tests/?s=some_stuff", - generateUrlAndQuery.invoke(httpClient, baseUrl, unfilteredParams1, true)); - } -} diff --git a/deploy-service/common/src/test/java/com/pinterest/deployservice/common/KnoxKeyTest.java b/deploy-service/common/src/test/java/com/pinterest/deployservice/common/KnoxKeyTest.java deleted file mode 100644 index 55198ad621..0000000000 --- a/deploy-service/common/src/test/java/com/pinterest/deployservice/common/KnoxKeyTest.java +++ /dev/null @@ -1,75 +0,0 @@ -/** - * Copyright (c) 2022-2023 Pinterest, Inc. - * - * Licensed 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 com.pinterest.deployservice.common; - -import com.pinterest.deployservice.rodimus.RodimusManager; -import com.pinterest.deployservice.rodimus.RodimusManagerImpl; -import java.lang.reflect.Field; -import java.util.ArrayList; -import java.util.List; -import org.junit.jupiter.api.BeforeEach; -import org.mockito.Mockito; - -@SuppressWarnings("unchecked") -class KnoxKeyTest { - - private static enum Answer { - NULL, - EXCEPTION, - ARRAY, - LATENCY - } - - private static final String msgUnauthException = - "HTTP request failed, status = 401, content = Unauthorized"; - private static final String postAnswerTag = - "{\"i-001\":{\"Name\": \"devapp-example1\"},\"i-002\":{\"Name\": \"devrestricted-example2\"}}"; - private static final String postAnswerArray = "[\"i-001\",\"i-002\"]"; - - private RodimusManager rodimusManager = null; - private KnoxKeyReader mockKnoxKeyReader; - private List answerList; - private String[] testKey = new String[2]; - - @BeforeEach - void setUp() throws Exception { - // Load testKeys - testKey[0] = "aaa"; // auth error - testKey[1] = "bbb"; // auth ok - - // Create mock for Knox - mockKnoxKeyReader = Mockito.mock(KnoxKeyReader.class); - - // Create mock for httpClient - - rodimusManager = - new RodimusManagerImpl("http://localhost", "teletraan:test", false, "", ""); - - // Allocate answerList - answerList = new ArrayList(); - mockClasses(rodimusManager, mockKnoxKeyReader); - } - - // ### HELPER METHODS ### - - private void mockClasses(RodimusManager rodimusMngr, KnoxKeyReader mokKnox) throws Exception { - // Modify fsKnox to use our mock - Field classKnox = rodimusMngr.getClass().getDeclaredField("knoxKeyReader"); - classKnox.setAccessible(true); - classKnox.set(rodimusMngr, mokKnox); - classKnox.setAccessible(false); - } -} diff --git a/deploy-service/pom.xml b/deploy-service/pom.xml index 09a3ccd20d..fec2cd2c7e 100644 --- a/deploy-service/pom.xml +++ b/deploy-service/pom.xml @@ -71,7 +71,7 @@ com.squareup.okhttp3 okhttp - 4.12.0 + ${okhttp.version} diff --git a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java index 61cd61d17e..2fc62e5e1d 100644 --- a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java +++ b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java @@ -120,42 +120,23 @@ private static OkHttpObservationInterceptor.Builder observationInterceptorBuilde public String get(String url, Map params, Map headers) throws IOException { - Request request = - new Request.Builder() - .url(buildUrl(url, params)) - .headers(buildHeaders(headers)) - .build(); - return makeCall(request); + return makeCall(createRequestBuilder(url, headers).build()); } public String post(String url, String body, Map headers) throws IOException { - Request request = - new Request.Builder() - .url(buildUrl(url, null)) - .headers(buildHeaders(headers)) - .post(buildJsonBody(body)) - .build(); - return makeCall(request); + return makeCall(createRequestBuilder(url, headers).post(buildJsonBody(body)).build()); } public String put(String url, String body, Map headers) throws IOException { - Request request = - new Request.Builder() - .url(buildUrl(url, null)) - .headers(buildHeaders(headers)) - .put(buildJsonBody(body)) - .build(); - return makeCall(request); + return makeCall(createRequestBuilder(url, headers).put(buildJsonBody(body)).build()); } public String delete(String url, String body, Map headers) throws IOException { - Request request = - new Request.Builder() - .url(buildUrl(url, null)) - .headers(buildHeaders(headers)) - .delete(buildJsonBody(body)) - .build(); - return makeCall(request); + return makeCall(createRequestBuilder(url, headers).delete(buildJsonBody(body)).build()); + } + + private Request.Builder createRequestBuilder(String url, Map headers) { + return new Request.Builder().url(buildUrl(url, null)).headers(buildHeaders(headers)); } public Headers buildHeaders(Map rawHeaders) { From f450f2ad5abc5b1b753bd6b6beff12d60574bc75 Mon Sep 17 00:00:00 2001 From: Tyler Ouyang Date: Wed, 6 Nov 2024 14:56:47 -0800 Subject: [PATCH 06/13] Use Proxy for Rodimus --- .../pinterest/deployservice/rodimus/RodimusManagerImpl.java | 5 ++++- .../com/pinterest/teletraan/universal/http/HttpClient.java | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/deploy-service/common/src/main/java/com/pinterest/deployservice/rodimus/RodimusManagerImpl.java b/deploy-service/common/src/main/java/com/pinterest/deployservice/rodimus/RodimusManagerImpl.java index c129635aa9..e7702b4192 100644 --- a/deploy-service/common/src/main/java/com/pinterest/deployservice/rodimus/RodimusManagerImpl.java +++ b/deploy-service/common/src/main/java/com/pinterest/deployservice/rodimus/RodimusManagerImpl.java @@ -59,7 +59,10 @@ public RodimusManagerImpl( LOG.error(httpProxyPort, exception); throw exception; } - clientBuilder.httpProxyAddr(httpProxyAddr).httpProxyPort(httpProxyPortInt); + clientBuilder + .useProxy(true) + .httpProxyAddr(httpProxyAddr) + .httpProxyPort(httpProxyPortInt); } this.httpClient = clientBuilder.authorizationSupplier(this::authorizationHeaderSupplier).build(); diff --git a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java index 2fc62e5e1d..374ded9418 100644 --- a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java +++ b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java @@ -48,7 +48,7 @@ public class HttpClient { private static final int DEFAULT_MAX_RETRIES = 3; private static final boolean DEFAULT_USE_PROXY = false; private static final String DEFAULT_HTTP_PROXY_ADDR = "localhost"; - private static final int DEFAULT_PROXY_PORT = 19193; + private static final int DEFAULT_PROXY_PORT = 19194; private static final long RETRY_INTERVAL = 500; @Getter private final OkHttpClient okHttpClient; From 6adc938c71eb6771d6a241b55a99e03e3abd4600 Mon Sep 17 00:00:00 2001 From: Tyler Ouyang Date: Wed, 6 Nov 2024 15:09:39 -0800 Subject: [PATCH 07/13] Fix jenkins --- .../deployservice/common/Jenkins.java | 19 ++++++++----------- .../teletraan/universal/http/HttpClient.java | 2 +- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/deploy-service/common/src/main/java/com/pinterest/deployservice/common/Jenkins.java b/deploy-service/common/src/main/java/com/pinterest/deployservice/common/Jenkins.java index 592893b77d..6c9e2e70d2 100644 --- a/deploy-service/common/src/main/java/com/pinterest/deployservice/common/Jenkins.java +++ b/deploy-service/common/src/main/java/com/pinterest/deployservice/common/Jenkins.java @@ -21,13 +21,13 @@ import java.util.Map; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import com.pinterest.teletraan.universal.http.HttpClient; // TODO: make it generic /** Wrapper for Jenkins API calls */ public class Jenkins { private static final Logger LOG = LoggerFactory.getLogger(Jenkins.class); - private static final int RETRIES = 3; - private HTTPClient httpClient = new HTTPClient(); + private static final HttpClient httpClient = HttpClient.builder().maxRetries(3).build(); private String jenkinsUrl; private String jenkinsRemoteToken; @@ -87,9 +87,8 @@ public boolean isPinterestJenkinsUrl(String url) { String getJenkinsToken() throws Exception { String url = String.format("%s/%s", this.jenkinsUrl, "crumbIssuer/api/json"); - String ret = httpClient.get(url, null, null, null, RETRIES); - JsonParser parser = new JsonParser(); - JsonObject json = (JsonObject) parser.parse(ret); + String ret = httpClient.get(url, null, null); + JsonObject json = (JsonObject) JsonParser.parseString(ret); return json.get("crumb").getAsString(); } @@ -98,7 +97,7 @@ public void startBuild(String url) throws Exception { Map headers = new HashMap<>(1); headers.put(".crumb", token); LOG.debug("Calling jenkins with url " + url + " and token " + token); - httpClient.post(url, null, headers, RETRIES); + httpClient.post(url, null, headers); } public void startBuild(String jobName, String buildParams) throws Exception { @@ -110,18 +109,16 @@ public void startBuild(String jobName, String buildParams) throws Exception { String.format( "%s/job/%s/buildWithParameters?%s%s", this.jenkinsUrl, jobName, tokenString, buildParams); - // startBuild(url); // Use GET instead, which is the same as POST but no need for token - httpClient.get(url, null, null, null, RETRIES); + httpClient.get(url, null, null); LOG.info("Successfully post to jenkins for job " + jobName); } public Build getBuild(String jobName, String jobNum) throws Exception { String url = String.format("%s/job/%s/%s/api/json", this.jenkinsUrl, jobName, jobNum); LOG.debug("Calling jenkins with url " + url); - String ret = httpClient.get(url, null, null, null, RETRIES); - JsonParser parser = new JsonParser(); - JsonObject json = (JsonObject) parser.parse(ret); + String ret = httpClient.get(url, null, null); + JsonObject json = (JsonObject) JsonParser.parseString(ret); return new Build( json.get("number").toString(), json.get("result").toString(), diff --git a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java index 374ded9418..2fc62e5e1d 100644 --- a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java +++ b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java @@ -48,7 +48,7 @@ public class HttpClient { private static final int DEFAULT_MAX_RETRIES = 3; private static final boolean DEFAULT_USE_PROXY = false; private static final String DEFAULT_HTTP_PROXY_ADDR = "localhost"; - private static final int DEFAULT_PROXY_PORT = 19194; + private static final int DEFAULT_PROXY_PORT = 19193; private static final long RETRY_INTERVAL = 500; @Getter private final OkHttpClient okHttpClient; From af3e3b513d7773d68805acb796935f657a0dd676 Mon Sep 17 00:00:00 2001 From: Tyler Ouyang Date: Wed, 6 Nov 2024 17:09:34 -0800 Subject: [PATCH 08/13] fix auth header --- .../deployservice/common/Jenkins.java | 2 +- .../rodimus/RodimusManagerImplTest.java | 8 +------- .../teletraan/universal/http/HttpClient.java | 18 +++++++----------- .../universal/http/HttpClientTest.java | 9 ++------- 4 files changed, 11 insertions(+), 26 deletions(-) diff --git a/deploy-service/common/src/main/java/com/pinterest/deployservice/common/Jenkins.java b/deploy-service/common/src/main/java/com/pinterest/deployservice/common/Jenkins.java index 6c9e2e70d2..a083ab3c3d 100644 --- a/deploy-service/common/src/main/java/com/pinterest/deployservice/common/Jenkins.java +++ b/deploy-service/common/src/main/java/com/pinterest/deployservice/common/Jenkins.java @@ -17,11 +17,11 @@ import com.google.gson.JsonObject; import com.google.gson.JsonParser; +import com.pinterest.teletraan.universal.http.HttpClient; import java.util.HashMap; import java.util.Map; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import com.pinterest.teletraan.universal.http.HttpClient; // TODO: make it generic /** Wrapper for Jenkins API calls */ diff --git a/deploy-service/common/src/test/java/com/pinterest/deployservice/rodimus/RodimusManagerImplTest.java b/deploy-service/common/src/test/java/com/pinterest/deployservice/rodimus/RodimusManagerImplTest.java index 2077c83a8f..1c3568b0af 100644 --- a/deploy-service/common/src/test/java/com/pinterest/deployservice/rodimus/RodimusManagerImplTest.java +++ b/deploy-service/common/src/test/java/com/pinterest/deployservice/rodimus/RodimusManagerImplTest.java @@ -66,21 +66,15 @@ void testConstructorInValidProxyConfig() { @Test void testNullKnoxKeyUsesDefaultKey() throws Exception { - // return 401 to trigger authentication flow - mockWebServer.enqueue(new MockResponse().setResponseCode(401)); mockWebServer.enqueue(new MockResponse().setBody("[]")); sut.getTerminatedHosts(Collections.singletonList("testHost")); - // discard first request RecordedRequest request = mockWebServer.takeRequest(); - request = mockWebServer.takeRequest(); assertEquals("token defaultKeyContent", request.getHeader("Authorization")); } @Test void testInvalidKnoxKeyThrowsException() throws Exception { - // return 401 to trigger authentication flow - mockWebServer.enqueue(new MockResponse().setResponseCode(401)); RodimusManagerImpl sut = new RodimusManagerImpl( mockWebServer.url(TEST_PATH).toString(), @@ -89,7 +83,7 @@ void testInvalidKnoxKeyThrowsException() throws Exception { "", ""); assertThrows(IllegalStateException.class, () -> sut.getTerminatedHosts(HOST_IDS)); - assertEquals(1, mockWebServer.getRequestCount()); + assertEquals(0, mockWebServer.getRequestCount()); } @Test diff --git a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java index 2fc62e5e1d..64e1f7350f 100644 --- a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java +++ b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java @@ -98,17 +98,13 @@ public HttpClient( Proxy.Type.HTTP, new InetSocketAddress(httpProxyAddr, httpProxyPort))); } if (authorizationSupplier != null) { - clientBuilder.authenticator( - (route, response) -> { - if (response.request().header("Authorization") != null) { - return null; // Give up, we've already failed to authenticate. - } - - String credential = authorizationSupplier.get(); - return response.request() - .newBuilder() - .header("Authorization", credential) - .build(); + clientBuilder.addInterceptor( + (chain) -> { + Request request = chain.request(); + return chain.proceed( + request.newBuilder() + .header("Authorization", authorizationSupplier.get()) + .build()); }); } okHttpClient = clientBuilder.build(); diff --git a/deploy-service/universal/src/test/java/com/pinterest/teletraan/universal/http/HttpClientTest.java b/deploy-service/universal/src/test/java/com/pinterest/teletraan/universal/http/HttpClientTest.java index c8d2d5ea9b..599e3b3c6f 100644 --- a/deploy-service/universal/src/test/java/com/pinterest/teletraan/universal/http/HttpClientTest.java +++ b/deploy-service/universal/src/test/java/com/pinterest/teletraan/universal/http/HttpClientTest.java @@ -125,17 +125,12 @@ void testAuthentication() throws IOException, InterruptedException { HttpClient sut = new HttpClient.HttpClientBuilder().authorizationSupplier(() -> authHeader).build(); - mockWebServer.enqueue(new MockResponse().setResponseCode(401)); mockWebServer.enqueue(new MockResponse().setResponseCode(200)); sut.get(mockWebServer.url(TEST_PATH).toString(), TEST_PARAMS, TEST_HEADERS); - // First request triggers authentication flow - RecordedRequest request1 = mockWebServer.takeRequest(); - assertNull(request1.getHeader("Authorization")); - - RecordedRequest request2 = mockWebServer.takeRequest(); - assertEquals(authHeader, request2.getHeader("Authorization")); + RecordedRequest request = mockWebServer.takeRequest(); + assertEquals(authHeader, request.getHeader("Authorization")); } @Test From 669ce7c96d62c93dc02d4272cb221930a515be9c Mon Sep 17 00:00:00 2001 From: Tyler Ouyang Date: Wed, 6 Nov 2024 18:27:22 -0800 Subject: [PATCH 09/13] add logging --- deploy-service/universal/pom.xml | 5 ++++ .../teletraan/universal/http/HttpClient.java | 29 +++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/deploy-service/universal/pom.xml b/deploy-service/universal/pom.xml index 7689581ec4..b6cb0f8477 100644 --- a/deploy-service/universal/pom.xml +++ b/deploy-service/universal/pom.xml @@ -93,6 +93,11 @@ com.squareup.okhttp3 okhttp + + com.squareup.okhttp3 + logging-interceptor + ${okhttp.version} + diff --git a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java index 64e1f7350f..fcb3ba4bc1 100644 --- a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java +++ b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java @@ -38,6 +38,9 @@ import okhttp3.Request; import okhttp3.RequestBody; import okhttp3.Response; +import okhttp3.logging.HttpLoggingInterceptor; +import okhttp3.logging.HttpLoggingInterceptor.Level; +import okhttp3.logging.HttpLoggingInterceptor.Logger; @Slf4j public class HttpClient { @@ -90,6 +93,7 @@ public HttpClient( .newBuilder() .connectTimeout(Duration.ofSeconds(15)) .readTimeout(Duration.ofSeconds(15)) + .addInterceptor(createHttpLoggingInterceptor()) .addInterceptor(observationInterceptorBuilder().build()) .addInterceptor(new RetryInterceptor(maxRetries, retryInterval)); if (useProxy) { @@ -110,6 +114,31 @@ public HttpClient( okHttpClient = clientBuilder.build(); } + private static HttpLoggingInterceptor createHttpLoggingInterceptor() { + HttpLoggingInterceptor logging = + new HttpLoggingInterceptor( + new Logger() { + @Override + public void log(String message) { + if (log.isTraceEnabled()) { + log.trace(message); + } else { + log.debug(message); + } + } + }); + if (log.isTraceEnabled()) { + logging.setLevel(Level.BODY); + } else if (log.isDebugEnabled()) { + logging.setLevel(Level.BASIC); + } else { + logging.setLevel(Level.NONE); + } + logging.redactHeader("Authorization"); + logging.redactHeader("Cookie"); + return logging; + } + private static OkHttpObservationInterceptor.Builder observationInterceptorBuilder() { return OkHttpObservationInterceptor.builder(observationRegistry, "okhttp"); } From 8a917e6fa73e779014b15f55c0f4289953fd7d17 Mon Sep 17 00:00:00 2001 From: Tyler Ouyang Date: Thu, 7 Nov 2024 16:58:13 -0800 Subject: [PATCH 10/13] simplify github auth --- .../rodimus/RodimusManagerImpl.java | 4 +- .../deployservice/scm/GithubManager.java | 45 +++++++++---------- 2 files changed, 24 insertions(+), 25 deletions(-) diff --git a/deploy-service/common/src/main/java/com/pinterest/deployservice/rodimus/RodimusManagerImpl.java b/deploy-service/common/src/main/java/com/pinterest/deployservice/rodimus/RodimusManagerImpl.java index e7702b4192..8c4ab76a78 100644 --- a/deploy-service/common/src/main/java/com/pinterest/deployservice/rodimus/RodimusManagerImpl.java +++ b/deploy-service/common/src/main/java/com/pinterest/deployservice/rodimus/RodimusManagerImpl.java @@ -65,7 +65,7 @@ public RodimusManagerImpl( .httpProxyPort(httpProxyPortInt); } this.httpClient = - clientBuilder.authorizationSupplier(this::authorizationHeaderSupplier).build(); + clientBuilder.authorizationSupplier(this::fetchAuthorizationHeader).build(); if (StringUtils.isNotBlank(knoxKey)) { knoxKeyReader.init(knoxKey); @@ -90,7 +90,7 @@ public boolean shouldSkipClass(Class c) { } } - private String authorizationHeaderSupplier() throws IllegalStateException { + private String fetchAuthorizationHeader() throws IllegalStateException { String knoxKey = knoxKeyReader.getKey(); if (StringUtils.isBlank(knoxKey)) { throw new IllegalStateException("Rodimus knoxKey is blank"); diff --git a/deploy-service/common/src/main/java/com/pinterest/deployservice/scm/GithubManager.java b/deploy-service/common/src/main/java/com/pinterest/deployservice/scm/GithubManager.java index 016bf7a8e8..d44dd4dc8e 100644 --- a/deploy-service/common/src/main/java/com/pinterest/deployservice/scm/GithubManager.java +++ b/deploy-service/common/src/main/java/com/pinterest/deployservice/scm/GithubManager.java @@ -19,6 +19,7 @@ import com.google.gson.reflect.TypeToken; import com.pinterest.deployservice.bean.CommitBean; import com.pinterest.deployservice.common.EncryptionUtils; +import com.pinterest.deployservice.common.KeyReader; import com.pinterest.deployservice.common.KnoxKeyReader; import com.pinterest.teletraan.universal.http.HttpClient; import java.io.IOException; @@ -42,7 +43,6 @@ public class GithubManager extends BaseManager { private static final Logger LOG = LoggerFactory.getLogger(GithubManager.class); private static final String UNKNOWN_LOGIN = "UNKNOWN"; - private static final HttpClient httpClient = HttpClient.builder().maxRetries(1).build(); private static final long TOKEN_TTL_MILLIS = 600000; // token expires after 10 minutes private final String apiPrefix; private final String urlPrefix; @@ -50,8 +50,8 @@ public class GithubManager extends BaseManager { private final String githubAppPrivateKeyKnox; private final String githubAppOrganization; private final String token; - - public Map headers = new HashMap<>(); + private final HttpClient httpClient; + private final KeyReader knoxKeyReader = new KnoxKeyReader(); public GithubManager( String token, @@ -68,19 +68,24 @@ public GithubManager( this.githubAppPrivateKeyKnox = appPrivateKeyKnox; this.githubAppOrganization = appOrganization; this.token = token; + if (StringUtils.isNotBlank(githubAppPrivateKeyKnox)) { + // get private key PEM from knox + knoxKeyReader.init(githubAppPrivateKeyKnox); + } + httpClient = + HttpClient.builder() + .authorizationSupplier(this::fetchAuthorizationHeader) + .maxRetries(1) + .build(); } - private void setHeaders() throws Exception { + private String fetchAuthorizationHeader() { // if token is specified, use token auth, otherwise, use GitHub app auth - if (StringUtils.isEmpty(this.token)) { + if (StringUtils.isBlank(this.token)) { try { - // get private key PEM from knox - KnoxKeyReader knoxKey = new KnoxKeyReader(); - knoxKey.init(this.githubAppPrivateKeyKnox); - String githubAppPrivateKey = knoxKey.getKey(); - if (StringUtils.isEmpty(githubAppPrivateKey)) { - LOG.error("Failed to get Github Knox key"); - throw new IllegalArgumentException("Failed to get Github Knox key"); + String githubAppPrivateKey = knoxKeyReader.getKey(); + if (StringUtils.isBlank(githubAppPrivateKey)) { + throw new IllegalStateException("Github Knox key is blank"); } // generate jwt token by signing with GitHub app id and private key @@ -98,17 +103,13 @@ private void setHeaders() throws Exception { appInstallation.createToken().create(); // always use the newly created GitHub app token as the token will expire - this.headers.put( - "Authorization", - String.format("Token %s", appInstallationToken.getToken())); + return String.format("Token %s", appInstallationToken.getToken()); } catch (Exception e) { - // e.printStackTrace(); - LOG.error("Exception when getting Github token: ", e); - throw e; + throw new IllegalStateException("Failed to get Github token", e); } } else { // this is the token that does not expire - this.headers.put("Authorization", String.format("Token %s", this.token)); + return String.format("Token %s", this.token); } } @@ -183,10 +184,9 @@ public CommitBean getCommit(String repo, String sha) throws Exception { String url = String.format("%s/repos/%s/commits/%s", apiPrefix, repo, sha); // TODO: Do not RETRY since it will timeout the thrift caller, need to revisit - setHeaders(); String jsonPayload; try { - jsonPayload = httpClient.get(url, null, headers); + jsonPayload = httpClient.get(url, null, null); } catch (IOException e) { // an IOException (and its subclasses) in this case indicates that the commit hash is // not found in the repo. @@ -215,8 +215,7 @@ public Queue getCommits(String repo, String startSha, boolean keepHe params.put("sha", startSha); params.put("path", path); - setHeaders(); - String jsonPayload = httpClient.get(url, params, headers); + String jsonPayload = httpClient.get(url, params, null); Queue CommitBeans = new LinkedList<>(); GsonBuilder builder = new GsonBuilder(); Map[] jsonMaps = From ce3eeb476d7e81388a3ad3743539eb301d50b2da Mon Sep 17 00:00:00 2001 From: Tyler Ouyang Date: Fri, 8 Nov 2024 13:21:46 -0800 Subject: [PATCH 11/13] Log configs --- .../deployservice/common/ChangeFeedJob.java | 2 +- .../deployservice/handler/DeployHandler.java | 2 +- .../deployservice/handler/WebhookJob.java | 2 +- .../teletraan/universal/http/HttpClient.java | 52 +++++++------------ 4 files changed, 22 insertions(+), 36 deletions(-) diff --git a/deploy-service/common/src/main/java/com/pinterest/deployservice/common/ChangeFeedJob.java b/deploy-service/common/src/main/java/com/pinterest/deployservice/common/ChangeFeedJob.java index a7c8de2603..8bc38f11c9 100644 --- a/deploy-service/common/src/main/java/com/pinterest/deployservice/common/ChangeFeedJob.java +++ b/deploy-service/common/src/main/java/com/pinterest/deployservice/common/ChangeFeedJob.java @@ -30,7 +30,7 @@ public final class ChangeFeedJob implements Callable { private static final Logger LOG = LoggerFactory.getLogger(ChangeFeedJob.class); - private static final HttpClient httpClient = new HttpClient(); + private static final HttpClient httpClient = HttpClient.builder().build(); private String payload; private String changeFeedUrl; private Object oriObj; diff --git a/deploy-service/common/src/main/java/com/pinterest/deployservice/handler/DeployHandler.java b/deploy-service/common/src/main/java/com/pinterest/deployservice/handler/DeployHandler.java index d8fe9d6499..41b3451248 100644 --- a/deploy-service/common/src/main/java/com/pinterest/deployservice/handler/DeployHandler.java +++ b/deploy-service/common/src/main/java/com/pinterest/deployservice/handler/DeployHandler.java @@ -102,7 +102,7 @@ public class DeployHandler implements DeployHandlerInterface { "This stage requires SOX builds. A private build cannot be used in a sox-compliant stage."; static final String ERROR_STAGE_REQUIRES_SOX_BUILD_COMPLIANT_SOURCE = "This stage requires SOX builds. The build must be from a sox-compliant source. Contact your sox administrators."; - private static final HttpClient httpClient = new HttpClient(); + private static final HttpClient httpClient = HttpClient.builder().build(); private final DeployDAO deployDAO; private final EnvironDAO environDAO; diff --git a/deploy-service/common/src/main/java/com/pinterest/deployservice/handler/WebhookJob.java b/deploy-service/common/src/main/java/com/pinterest/deployservice/handler/WebhookJob.java index 91925d3ae6..10a51f3d4d 100644 --- a/deploy-service/common/src/main/java/com/pinterest/deployservice/handler/WebhookJob.java +++ b/deploy-service/common/src/main/java/com/pinterest/deployservice/handler/WebhookJob.java @@ -28,7 +28,7 @@ public class WebhookJob implements Callable { private static final Logger LOG = LoggerFactory.getLogger(WebhookJob.class); - private static final HttpClient httpClient = new HttpClient(); + private static final HttpClient httpClient = HttpClient.builder().build(); private List webhooks; private DeployBean deployBean; diff --git a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java index fcb3ba4bc1..6b1d2907da 100644 --- a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java +++ b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java @@ -27,7 +27,6 @@ import java.util.function.Supplier; import javax.ws.rs.ClientErrorException; import javax.ws.rs.ServerErrorException; -import lombok.AccessLevel; import lombok.Builder; import lombok.Getter; import lombok.extern.slf4j.Slf4j; @@ -48,42 +47,16 @@ public class HttpClient { private static final ObservationRegistry observationRegistry = ObservationRegistry.create(); private static final OkHttpClient sharedOkHttpClient = new OkHttpClient(); - private static final int DEFAULT_MAX_RETRIES = 3; - private static final boolean DEFAULT_USE_PROXY = false; - private static final String DEFAULT_HTTP_PROXY_ADDR = "localhost"; - private static final int DEFAULT_PROXY_PORT = 19193; - private static final long RETRY_INTERVAL = 500; - @Getter private final OkHttpClient okHttpClient; - public HttpClient() { - this(null, null, null, null, null, null); - } - - @Builder(access = AccessLevel.PUBLIC) - public HttpClient( - Integer maxRetries, - Long retryInterval, - Boolean useProxy, + @Builder(buildMethodName = "buildInternal") + private HttpClient( + int maxRetries, + long retryInterval, + boolean useProxy, String httpProxyAddr, - Integer httpProxyPort, + int httpProxyPort, Supplier authorizationSupplier) { - if (maxRetries == null) { - maxRetries = DEFAULT_MAX_RETRIES; - } - if (useProxy == null) { - useProxy = DEFAULT_USE_PROXY; - } - if (httpProxyAddr == null) { - httpProxyAddr = DEFAULT_HTTP_PROXY_ADDR; - } - if (httpProxyPort == null) { - httpProxyPort = DEFAULT_PROXY_PORT; - } - if (retryInterval == null) { - retryInterval = RETRY_INTERVAL; - } - observationRegistry .observationConfig() .observationHandler(new DefaultMeterObservationHandler(Metrics.globalRegistry)); @@ -114,6 +87,19 @@ public HttpClient( okHttpClient = clientBuilder.build(); } + public static class HttpClientBuilder { + private int maxRetries = 3; + private long retryInterval = 500; + private boolean useProxy = false; + private String httpProxyAddr = "localhost"; + private int httpProxyPort = 19193; + + public HttpClient build() { + log.info("building HttpClient with configs: {}", this.toString()); + return buildInternal(); + } + } + private static HttpLoggingInterceptor createHttpLoggingInterceptor() { HttpLoggingInterceptor logging = new HttpLoggingInterceptor( From e3b3f2826ad3eaab5cde872bfe16ac60597950ec Mon Sep 17 00:00:00 2001 From: Tyler Ouyang Date: Fri, 8 Nov 2024 13:25:01 -0800 Subject: [PATCH 12/13] comment to explaine --- .../java/com/pinterest/teletraan/universal/http/HttpClient.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java index 6b1d2907da..c9e4a493f7 100644 --- a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java +++ b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/HttpClient.java @@ -87,6 +87,8 @@ private HttpClient( okHttpClient = clientBuilder.build(); } + // Partial builder implementation to allow for default values and custom build method + // lombok will generate the rest of the builder public static class HttpClientBuilder { private int maxRetries = 3; private long retryInterval = 500; From fd5a76bea7f110b6dc92a377b2c9a346e8caf62e Mon Sep 17 00:00:00 2001 From: Tyler Ouyang Date: Tue, 12 Nov 2024 16:00:40 -0800 Subject: [PATCH 13/13] Addressed comments --- .../com/pinterest/deployservice/common/Jenkins.java | 2 +- .../teletraan/universal/http/RetryInterceptor.java | 12 +++++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/deploy-service/common/src/main/java/com/pinterest/deployservice/common/Jenkins.java b/deploy-service/common/src/main/java/com/pinterest/deployservice/common/Jenkins.java index a083ab3c3d..08341607cb 100644 --- a/deploy-service/common/src/main/java/com/pinterest/deployservice/common/Jenkins.java +++ b/deploy-service/common/src/main/java/com/pinterest/deployservice/common/Jenkins.java @@ -27,7 +27,7 @@ /** Wrapper for Jenkins API calls */ public class Jenkins { private static final Logger LOG = LoggerFactory.getLogger(Jenkins.class); - private static final HttpClient httpClient = HttpClient.builder().maxRetries(3).build(); + private static final HttpClient httpClient = HttpClient.builder().build(); private String jenkinsUrl; private String jenkinsRemoteToken; diff --git a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/RetryInterceptor.java b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/RetryInterceptor.java index 51f424a86c..4d66e3f8d8 100644 --- a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/RetryInterceptor.java +++ b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/RetryInterceptor.java @@ -15,6 +15,7 @@ */ package com.pinterest.teletraan.universal.http; +import com.google.common.collect.ImmutableList; import java.io.IOException; import java.util.concurrent.TimeUnit; import okhttp3.Interceptor; @@ -22,6 +23,8 @@ import okhttp3.Response; public class RetryInterceptor implements Interceptor { + private static final ImmutableList RETRY_CODES = + ImmutableList.of(429, 500, 502, 503, 504); private final int maxRetries; private final long retryInterval; @@ -39,13 +42,17 @@ public Response intercept(Chain chain) throws IOException { for (int i = 0; i < maxRetries; i++) { try { response = chain.proceed(request); - if (response.isSuccessful() || !shouldRetry(response)) { + if (response.isSuccessful()) { return response; } } catch (IOException e) { lastException = e; } + if (!shouldRetry(response) || i == maxRetries - 1) { + break; + } + try { long backoff = (long) Math.pow(2, i) * retryInterval; TimeUnit.MILLISECONDS.sleep(backoff); @@ -63,7 +70,6 @@ public Response intercept(Chain chain) throws IOException { } private boolean shouldRetry(Response response) { - int code = response.code(); - return code == 429 || code == 500 || code == 502 || code == 503 || code == 504; + return RETRY_CODES.contains(response.code()); } }