From 0349e3b0f0d68f4c0cf1582fa98267d4dc2b87c9 Mon Sep 17 00:00:00 2001 From: Ian Roquebert <72234714+gzpcho@users.noreply.github.com> Date: Thu, 1 Feb 2024 00:28:13 -0600 Subject: [PATCH 1/4] [deploy-agent] Return type hints in download folder (#1403) Add type hints --- deploy-agent/deployd/download/download_helper.py | 6 +++--- deploy-agent/deployd/download/download_helper_factory.py | 3 ++- deploy-agent/deployd/download/downloader.py | 8 ++++---- deploy-agent/deployd/download/gpg_helper.py | 2 +- deploy-agent/deployd/download/http_download_helper.py | 8 ++++---- deploy-agent/deployd/download/local_download_helper.py | 8 ++++---- deploy-agent/deployd/download/s3_download_helper.py | 8 ++++---- 7 files changed, 22 insertions(+), 21 deletions(-) diff --git a/deploy-agent/deployd/download/download_helper.py b/deploy-agent/deployd/download/download_helper.py index 5d760ab0ed..823286ac28 100644 --- a/deploy-agent/deployd/download/download_helper.py +++ b/deploy-agent/deployd/download/download_helper.py @@ -22,11 +22,11 @@ class DownloadHelper(metaclass=abc.ABCMeta): - def __init__(self, url): + def __init__(self, url) -> None: self._url = url @staticmethod - def hash_file(file_path): + def hash_file(file_path) -> str: sha = hashlib.sha1() with open(file_path, "rb") as f: for chunk in iter(lambda: f.read(4096), b""): @@ -34,7 +34,7 @@ def hash_file(file_path): return sha.hexdigest() @staticmethod - def md5_file(file_path): + def md5_file(file_path) -> str: md5 = hashlib.md5() with open(file_path, "rb") as f: for chunk in iter(lambda: f.read(4096), b""): diff --git a/deploy-agent/deployd/download/download_helper_factory.py b/deploy-agent/deployd/download/download_helper_factory.py index d9b2c4ffc8..d483e76b7e 100644 --- a/deploy-agent/deployd/download/download_helper_factory.py +++ b/deploy-agent/deployd/download/download_helper_factory.py @@ -13,6 +13,7 @@ # limitations under the License. import logging +from typing import Optional from future.moves.urllib.parse import urlparse from boto.s3.connection import S3Connection @@ -28,7 +29,7 @@ class DownloadHelperFactory(object): @staticmethod - def gen_downloader(url, config): + def gen_downloader(url, config) -> Optional[S3DownloadHelper|LocalDownloadHelper|HTTPDownloadHelper]: url_parse = urlparse(url) if url_parse.scheme == 's3': aws_access_key_id = config.get_aws_access_key() diff --git a/deploy-agent/deployd/download/downloader.py b/deploy-agent/deployd/download/downloader.py index 8e2b51514c..baf0dfa7da 100644 --- a/deploy-agent/deployd/download/downloader.py +++ b/deploy-agent/deployd/download/downloader.py @@ -31,7 +31,7 @@ class Downloader(object): - def __init__(self, config, build, url, env_name): + def __init__(self, config, build, url, env_name) -> None: self._matcher = re.compile(r'^.*?[.](?Ptar\.gz|tar\.bz2|\w+)$') self._base_dir = config.get_builds_directory() self._build_name = env_name @@ -39,15 +39,15 @@ def __init__(self, config, build, url, env_name): self._url = url self._config = config - def _get_inner_extension(self, url): + def _get_inner_extension(self, url) -> str: outerExtension = self._get_extension(url) inverseExtLen = (len(outerExtension) * -1) - 1 return self._get_extension(url[:inverseExtLen]) - def _get_extension(self, url): + def _get_extension(self, url) -> str: return self._matcher.match(url).group('ext') - def download(self): + def download(self) -> int: extension = self._get_extension(self._url.lower()) local_fn = u'{}-{}.{}'.format(self._build_name, self._build, extension) local_full_fn = os.path.join(self._base_dir, local_fn) diff --git a/deploy-agent/deployd/download/gpg_helper.py b/deploy-agent/deployd/download/gpg_helper.py index d680352908..1a2d02faa3 100644 --- a/deploy-agent/deployd/download/gpg_helper.py +++ b/deploy-agent/deployd/download/gpg_helper.py @@ -22,7 +22,7 @@ class gpgHelper(object): @staticmethod - def decryptFile(source, destination): + def decryptFile(source, destination) -> int: download_cmd = ['gpg', '--batch', '--yes', '--output', destination, '--decrypt', source] log.info('Running command: {}'.format(' '.join(download_cmd))) error_code = Status.SUCCEEDED diff --git a/deploy-agent/deployd/download/http_download_helper.py b/deploy-agent/deployd/download/http_download_helper.py index 43148099ed..1bdfa3198d 100644 --- a/deploy-agent/deployd/download/http_download_helper.py +++ b/deploy-agent/deployd/download/http_download_helper.py @@ -29,11 +29,11 @@ class HTTPDownloadHelper(DownloadHelper): - def __init__(self, url=None, config=None): + def __init__(self, url=None, config=None) -> None: super().__init__(url) self._config = config if config else Config() - def _download_files(self, local_full_fn): + def _download_files(self, local_full_fn) -> int: download_cmd = ['curl', '-o', local_full_fn, '-fksS', self._url] log.info('Running command: {}'.format(' '.join(download_cmd))) output, error, process_return_code = Caller.call_and_log(download_cmd, cwd=os.getcwd()) @@ -45,7 +45,7 @@ def _download_files(self, local_full_fn): log.info('Finish downloading: {} to {}'.format(self._url, local_full_fn)) return status_code - def download(self, local_full_fn): + def download(self, local_full_fn) -> int: log.info("Start to download from url {} to {}".format( self._url, local_full_fn)) if not self.validate_source(): @@ -76,7 +76,7 @@ def download(self, local_full_fn): log.error('Could not connect to: {}'.format(self._url)) return Status.FAILED - def validate_source(self): + def validate_source(self) -> bool: tags = {'type': 'http', 'url': self._url} create_sc_increment(DOWNLOAD_VALIDATE_METRICS, tags=tags) diff --git a/deploy-agent/deployd/download/local_download_helper.py b/deploy-agent/deployd/download/local_download_helper.py index 47d1188d42..1ca6cbac12 100644 --- a/deploy-agent/deployd/download/local_download_helper.py +++ b/deploy-agent/deployd/download/local_download_helper.py @@ -23,7 +23,7 @@ class LocalDownloadHelper(DownloadHelper): - def _download_files(self, local_full_fn): + def _download_files(self, local_full_fn) -> int: download_cmd = ['curl', '-o', local_full_fn, '-ks', self._url] log.info('Running command: {}'.format(' '.join(download_cmd))) error_code = Status.SUCCEEDED @@ -37,7 +37,7 @@ def _download_files(self, local_full_fn): log.info('Finished downloading: {} to {}'.format(self._url, local_full_fn)) return error_code - def download(self, local_full_fn): + def download(self, local_full_fn) -> int: log.info("Start to download from local path {} to {}".format( self._url, local_full_fn)) @@ -46,5 +46,5 @@ def download(self, local_full_fn): log.error('Failed to download the local tar ball for {}'.format(local_full_fn)) return error - def validate_source(self): - return True \ No newline at end of file + def validate_source(self) -> bool: + return True diff --git a/deploy-agent/deployd/download/s3_download_helper.py b/deploy-agent/deployd/download/s3_download_helper.py index d1ff0d8458..479c6c09e6 100644 --- a/deploy-agent/deployd/download/s3_download_helper.py +++ b/deploy-agent/deployd/download/s3_download_helper.py @@ -27,7 +27,7 @@ class S3DownloadHelper(DownloadHelper): - def __init__(self, local_full_fn, aws_connection=None, url=None): + def __init__(self, local_full_fn, aws_connection=None, url=None) -> None: super(S3DownloadHelper, self).__init__(local_full_fn) self._s3_matcher = "^s3://(?P[a-zA-Z0-9\-_]+)/(?P[a-zA-Z0-9\-_/\.]+)/?" self._config = Config() @@ -45,7 +45,7 @@ def __init__(self, local_full_fn, aws_connection=None, url=None): self._key = s3url_parse.group("KEY") - def download(self, local_full_fn): + def download(self, local_full_fn) -> int: log.info(f"Start to download file {self._key} from s3 bucket {self._bucket_name} to {local_full_fn}") if not self.validate_source(): log.error(f'Invalid url: {self._url}. Skip downloading.') @@ -76,9 +76,9 @@ def download(self, local_full_fn): log.error("Failed to get package from s3: {}".format(traceback.format_exc())) return Status.FAILED - def validate_source(self): + def validate_source(self) -> bool: allow_list = self._config.get_s3_download_allow_list() tags = {'type': 's3', 'url': self._url, 'bucket' : self._bucket_name} create_sc_increment(DOWNLOAD_VALIDATE_METRICS, tags=tags) - return self._bucket_name in allow_list if allow_list else True \ No newline at end of file + return self._bucket_name in allow_list if allow_list else True From 443a6275f8200fa9043bbe9946a4504123040a41 Mon Sep 17 00:00:00 2001 From: Ian Roquebert <72234714+gzpcho@users.noreply.github.com> Date: Thu, 1 Feb 2024 17:26:43 -0600 Subject: [PATCH 2/4] Add GitHub action for auto-labeling PRs (#1438) At deployment time, we don't have a way within Teletraan to differentiate which commits are relevant to that service. With this GHA, we will be able to look at the diff in GitHub and filter by labels depending on which service we are deploying. https://github.com/actions/labeler --- .github/labeler.yml | 12 ++++++++++++ .github/workflows/labeler.yml | 16 ++++++++++++++++ 2 files changed, 28 insertions(+) create mode 100644 .github/labeler.yml create mode 100644 .github/workflows/labeler.yml diff --git a/.github/labeler.yml b/.github/labeler.yml new file mode 100644 index 0000000000..05a534244b --- /dev/null +++ b/.github/labeler.yml @@ -0,0 +1,12 @@ +deploy-agent: + - changed-files: + - any-glob-to-any-file: deploy-agent/** +deploy-board: + - changed-files: + - any-glob-to-any-file: deploy-board/** +deploy-sentinel: + - changed-files: + - any-glob-to-any-file: deploy-sentinel/** +deploy-service: + - changed-files: + - any-glob-to-any-file: deploy-service/** diff --git a/.github/workflows/labeler.yml b/.github/workflows/labeler.yml new file mode 100644 index 0000000000..7134e419e2 --- /dev/null +++ b/.github/workflows/labeler.yml @@ -0,0 +1,16 @@ +name: "Pull Request Labeler" + +on: + - pull_request_target + +jobs: + labeler: + permissions: + contents: read + pull-requests: write + runs-on: ubuntu-latest + steps: + - uses: actions/labeler@v5 + with: + repo-token: "${{ secrets.GITHUB_TOKEN }}" + sync-labels: true From 3a59a93f6169aa63c4c8304b9c2c78b8eb85707d Mon Sep 17 00:00:00 2001 From: Tyler Ouyang Date: Fri, 2 Feb 2024 15:39:58 -0800 Subject: [PATCH 3/4] Trim knox key (#1433) Teletraan API and Teletraan Worker read Rodimus API keys from Knox. A newline can be inserted into a Knox key and result in a malformed authentication header. Defensive programming should avoid this mistake. --- .../deployservice/common/KnoxKeyReader.java | 6 +++- .../common/KnoxKeyReaderTest.java | 28 +++++++++++++++---- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/deploy-service/common/src/main/java/com/pinterest/deployservice/common/KnoxKeyReader.java b/deploy-service/common/src/main/java/com/pinterest/deployservice/common/KnoxKeyReader.java index dcf7826f29..d8c2a70318 100644 --- a/deploy-service/common/src/main/java/com/pinterest/deployservice/common/KnoxKeyReader.java +++ b/deploy-service/common/src/main/java/com/pinterest/deployservice/common/KnoxKeyReader.java @@ -73,6 +73,10 @@ private String getKeyInternal() { LOG.warn("Using default key since knoxManager is null"); return defaultKeyContent; } - return knoxManager.getKey(); + String key = knoxManager.getKey(); + if (key != null) { + return key.trim(); + } + return key; } } diff --git a/deploy-service/common/src/test/java/com/pinterest/deployservice/common/KnoxKeyReaderTest.java b/deploy-service/common/src/test/java/com/pinterest/deployservice/common/KnoxKeyReaderTest.java index bda5f0fb7b..0cc3be6af7 100644 --- a/deploy-service/common/src/test/java/com/pinterest/deployservice/common/KnoxKeyReaderTest.java +++ b/deploy-service/common/src/test/java/com/pinterest/deployservice/common/KnoxKeyReaderTest.java @@ -5,28 +5,34 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.only; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import org.junit.Before; import org.junit.Test; public class KnoxKeyReaderTest { + private KnoxKeyReader sut; + private KnoxKeyManager mockKnoxKeyManager; + + @Before + public void setUp() { + sut = new KnoxKeyReader(); + mockKnoxKeyManager = mock(KnoxKeyManager.class); + } + @Test public void testGetKey_noInit() { - KnoxKeyReader sut = new KnoxKeyReader(); assertEquals("defaultKeyContent", sut.getKey()); } @Test public void testGetKey_initialized() { - KnoxKeyReader sut = new KnoxKeyReader(); sut.init("someRandomKey"); assertNull(sut.getKey()); } @Test public void testGetKey_cacheIsWorking() { - KnoxKeyReader sut = new KnoxKeyReader(); - KnoxKeyManager mockKnoxKeyManager = mock(KnoxKeyManager.class); - sut.init("someRandomKey"); sut.setKnoxManager(mockKnoxKeyManager); sut.getKey(); @@ -34,4 +40,16 @@ public void testGetKey_cacheIsWorking() { verify(mockKnoxKeyManager, only()).getKey(); } + + @Test + public void testGetKey_trimmed() { + String expected = "keyWith234!~@"; + when(mockKnoxKeyManager.getKey()).thenReturn(String.format(" \n\r\t %s \n\r\t", expected)); + + sut.init("someRandomKey"); + sut.setKnoxManager(mockKnoxKeyManager); + String trimmed = sut.getKey(); + + assertEquals(expected, trimmed); + } } From 61b54dc3361caa97bd37ed62710571fcc6a622a3 Mon Sep 17 00:00:00 2001 From: Tyler Ouyang Date: Fri, 2 Feb 2024 15:41:23 -0800 Subject: [PATCH 4/4] Add JsonProcessingExceptionMapper (#1440) * Add JsonProcessingExceptionMapper * updated comment * show details --- .../main/java/com/pinterest/teletraan/TeletraanService.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/deploy-service/teletraanservice/src/main/java/com/pinterest/teletraan/TeletraanService.java b/deploy-service/teletraanservice/src/main/java/com/pinterest/teletraan/TeletraanService.java index 0fda13f885..532780ed58 100644 --- a/deploy-service/teletraanservice/src/main/java/com/pinterest/teletraan/TeletraanService.java +++ b/deploy-service/teletraanservice/src/main/java/com/pinterest/teletraan/TeletraanService.java @@ -24,6 +24,7 @@ import io.dropwizard.configuration.SubstitutingSourceProvider; import io.dropwizard.health.conf.HealthConfiguration; import io.dropwizard.health.core.HealthCheckBundle; +import io.dropwizard.jersey.jackson.JsonProcessingExceptionMapper; import io.dropwizard.setup.Bootstrap; import io.dropwizard.setup.Environment; import io.swagger.jaxrs.config.BeanConfig; @@ -172,6 +173,8 @@ public void run(TeletraanServiceConfiguration configuration, Environment environ environment.healthChecks().register("generic", new GenericHealthCheck(context)); // Exception handler + // Jackson Json parsing exceptions + environment.jersey().register(new JsonProcessingExceptionMapper(true)); environment.jersey().register(new GenericExceptionMapper(configuration.getSystemFactory().getClientError())); // Swagger API docs generation related