Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

support latest in distribution build urls #1569

Merged
merged 19 commits into from
Mar 1, 2022

Conversation

tianleh
Copy link
Member

@tianleh tianleh commented Jan 27, 2022

Signed-off-by: Tianle Huang [email protected]

Description

Add the logic to scan S3 bucket to get the max build number. The version and bucket name are hardcoded for demo purpose. Also move the Lambda function logic to a local js file instead of inline code.

Issues Resolved

#1492

Test

able to get the max build number in my test account.
INFO maxBuildNumber 21

The S3 structure is similar to distribution bucket.
Screen Shot 2022-01-27 at 2 27 37 PM

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Thanks for sharing, good progress on this space.

Do you know why the package-lock.json change is so big? I wouldn't expect much of any change to it.

deployment/lib/cf-url-rewriter.js Outdated Show resolved Hide resolved
deployment/lib/cf-url-rewriter.js Outdated Show resolved Hide resolved
deployment/lib/cf-url-rewriter.js Outdated Show resolved Hide resolved
deployment/lib/cf-url-rewriter.js Outdated Show resolved Hide resolved
deployment/lib/cf-url-rewriter.js Outdated Show resolved Hide resolved
deployment/lib/cf-url-rewriter.js Outdated Show resolved Hide resolved
@kavilla
Copy link
Member

kavilla commented Jan 29, 2022

Will this enable us to hit latest plugins as well?

@tianleh
Copy link
Member Author

tianleh commented Jan 31, 2022

Will this enable us to hit latest plugins as well?

This feature will only replace the latest with the max build number. Thus whatever it has with the build number shall still work with the latest keyword.

@codecov-commenter
Copy link

codecov-commenter commented Feb 3, 2022

Codecov Report

Merging #1569 (9fea470) into main (986736d) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1569      +/-   ##
============================================
- Coverage     94.62%   94.61%   -0.02%     
  Complexity       14       14              
============================================
  Files           164      165       +1     
  Lines          3459     3489      +30     
  Branches         21       25       +4     
============================================
+ Hits           3273     3301      +28     
- Misses          183      185       +2     
  Partials          3        3              
Impacted Files Coverage Δ
...loyment/lambdas/cf-url-rewriter/cf-url-rewriter.ts 100.00% <100.00%> (+50.00%) ⬆️
deployment/lambdas/cf-url-rewriter/https-get.ts 100.00% <100.00%> (ø)
src/system/temporary_directory.py 83.87% <0.00%> (-12.91%) ⬇️
src/system/os.py 92.30% <0.00%> (-7.70%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 986736d...9fea470. Read the comment docs.

@tianleh
Copy link
Member Author

tianleh commented Feb 3, 2022

Experiment a bit about this comment about pagination support #1569 (comment)

I wrote a script to put >1K objects under a version ("1.7.1") in my personal AWS account to simulate the situation where distribution build generates more than 1K builds for a version.
Screen Shot 2022-02-02 at 10 26 02 PM

Without making the change to support pagination, I run the Lambda function and can indeed see the limitation of ListObjectsV2 API about CommonPrefixes field as documented https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListObjectsV2.html#AmazonS3-ListObjectsV2-response-CommonPrefixes

After my change, I am able to see the full result being retrieved. The CloudWatch log snippet shows:
Screen Shot 2022-02-02 at 10 27 19 PM

@tianleh
Copy link
Member Author

tianleh commented Feb 6, 2022

Added the logic to upload index.yml but it failed with the following S3 exception.

com.amazonaws.services.s3.model.AmazonS3Exception: Access Denied (Service: Amazon S3; Status Code: 403; Error Code: AccessDenied;

@tianleh
Copy link
Member Author

tianleh commented Feb 6, 2022

Added the logic to upload index.yml but it failed with the following S3 exception.

com.amazonaws.services.s3.model.AmazonS3Exception: Access Denied (Service: Amazon S3; Status Code: 403; Error Code: AccessDenied;

Created a Github issue to track the S3 permission which could be worked in parallel. #1601

I temporarily upload to $BUCKET/distribution-build-opensearch/1.2.4/dist instead of $BUCKET/distribution-build-opensearch/1.2.4 to continue working of this PR.

@tianleh
Copy link
Member Author

tianleh commented Feb 7, 2022

Tuning the Lambda function to get latest replaced by a build number in my personal AWS account.

request.uri = request.uri.replace('latest', '147');

The one with real number works
d271n53yru5cr7.cloudfront.net/distribution-build-opensearch/2.0.0/147/test.png

But the latest one is not
d271n53yru5cr7.cloudfront.net/distribution-build-opensearch/2.0.0/latest/test.png

Need to dig more why the Lambda is not working.

@seraphjiang
Copy link
Member

is result cached by cloudfront cdn?

@tianleh
Copy link
Member Author

tianleh commented Feb 7, 2022

is result cached by cloudfront cdn?

Thanks for the reminder. After digging more with @seraphjiang , the new logic needs to go through new version publish and deployment to Lambda@Edge to take effect. Now the latest url is working
https://d271n53yru5cr7.cloudfront.net/distribution-build-opensearch/2.0.0/latest/test.png

Next step: read yml to get the build number instead of hardcoding mapping latest to 147

@dblock
Copy link
Member

dblock commented Feb 7, 2022

I suggest redirecting latest to an actual build number with a 302, which avoids the whole cloudfront caching business.

@peternied
Copy link
Member

Today I had some time so I've created two PRs to help with this one, @tianleh please take a look at them and consider merging them or building off of them.

  • Add permissions to upload index.yml files for the bundle role #3
  • Switch url rewriter from js to typescipt #4

@tianleh
Copy link
Member Author

tianleh commented Feb 9, 2022

I suggest redirecting latest to an actual build number with a 302, which avoids the whole cloudfront caching business.

Sure. I was able to make this happen in my Lambda.

https://d271n53yru5cr7.cloudfront.net/distribution-build-opensearch/2.0.0/latest/test.png

    const redirectResponse = {
          status: '302',
          statusDescription: 'Moved temporarily',
          headers: {
            'location': [{
              key: 'Location',
              value: request.uri.replace('latest', '151'),
            }],
            'cache-control': [{
              key: 'Cache-Control',
              value: "max-age=3600"
            }],
          },
      };

@tianleh
Copy link
Member Author

tianleh commented Feb 9, 2022

experimenting on how to get the index.yml

Option 1: download (e.g use JavaScript library 'https') the yml file from the URL d271n53yru5cr7.cloudfront.net/distribution-build-opensearch/2.0.0/dist/index.yml to a Lambda local file system. Read the yml and get the build number.

Option 2: get the S3 keys from the url https://ci.opensearch.org/ci/dbc/distribution-build-opensearch/1.1.1/latest/linux/x64/dist/opensearch/manifest.yml However, we do not know the S3 bucket name. May need a way to figure this out.

@tianleh
Copy link
Member Author

tianleh commented Feb 10, 2022

experimenting on how to get the index.yml

Option 1: download (e.g use JavaScript library 'https') the yml file from the URL d271n53yru5cr7.cloudfront.net/distribution-build-opensearch/2.0.0/dist/index.yml to a Lambda local file system. Read the yml and get the build number.

Option 2: get the S3 keys from the url https://ci.opensearch.org/ci/dbc/distribution-build-opensearch/1.1.1/latest/linux/x64/dist/opensearch/manifest.yml However, we do not know the S3 bucket name. May need a way to figure this out.

struggled with Option 1 to download from url. Based on my research, the https library cannot be promisified. https://stackoverflow.com/questions/65306617/async-await-for-node-js-https-get

Will try to see if there is other library to handle the file download.

@tianleh
Copy link
Member Author

tianleh commented Feb 10, 2022

experimenting on how to get the index.yml
Option 1: download (e.g use JavaScript library 'https') the yml file from the URL d271n53yru5cr7.cloudfront.net/distribution-build-opensearch/2.0.0/dist/index.yml to a Lambda local file system. Read the yml and get the build number.
Option 2: get the S3 keys from the url https://ci.opensearch.org/ci/dbc/distribution-build-opensearch/1.1.1/latest/linux/x64/dist/opensearch/manifest.yml However, we do not know the S3 bucket name. May need a way to figure this out.

struggled with Option 1 to download from url. Based on my research, the https library cannot be promisified. https://stackoverflow.com/questions/65306617/async-await-for-node-js-https-get

Will try to see if there is other library to handle the file download.

Received the following error when tuning inside Lambda console. I suspect there is some version upgrade needed to support such in Lambda. Researching...

Screen Shot 2022-02-10 at 1 47 43 AM

@tianleh
Copy link
Member Author

tianleh commented Feb 28, 2022

Notes for reviewers:

  1. This current PR contains another open PR fix failed jest by unifying cdk versions #1675 which is for fixing jest CI flow. I would like to have the related PR merged first before merging this PR.

  2. Since our CI runs yarn https://github.com/opensearch-project/opensearch-build/runs/5355023313?check_suite_focus=true , I am also including deployment/yarn.lock in the commits.

tianleh and others added 18 commits February 28, 2022 19:19
Signed-off-by: Tianle Huang <[email protected]>
The bundle role is the one used by the distribution-build jobs when
vars/uploadArtifacts.groovy is processes.  This change allows the
uploading of an index.yml

Signed-off-by: Peter Nied <[email protected]>
I pulled in a new library @aws-cdk/aws-lambda-nodejs that handles the
transformation of javascript to typescript.  Had to patch up a couple of
things which made this change a little more complex that intended.

- All of the CDK components were not version locked, that meant
  depending on the order of npm installs they could drift and get into a
dependancy mismatch spiral.  The version itself isn't important, its
uniformity is all that matters.

- Added permissions to the lambda role for interacting with cloudfronts
  distribution system, these were handled by the lambda function during
deployment, but it seems this new library while handling the js -> ts
brought some limitations and I had to manually implement the permssions see
for details https://tinyurl.com/5xy3n8zt

- Need to fix some typescript compliation issues, please feel free to
  re-work all of this and just keep the method signature

Signed-off-by: Peter Nied <[email protected]>
Signed-off-by: Tianle Huang <[email protected]>

Revert "support js-yaml"

This reverts commit c78e701.

Signed-off-by: Tianle Huang <[email protected]>

add js-yaml

Signed-off-by: Tianle Huang <[email protected]>
Signed-off-by: Tianle Huang <[email protected]>

clean up

Signed-off-by: Tianle Huang <[email protected]>

update lock json

Signed-off-by: Tianle Huang <[email protected]>

update

Signed-off-by: Tianle Huang <[email protected]>

update the s3 location for index.json

Signed-off-by: Tianle Huang <[email protected]>

delete unused from rebase

Signed-off-by: Tianle Huang <[email protected]>
Signed-off-by: Tianle Huang <[email protected]>
Signed-off-by: Tianle Huang <[email protected]>
Signed-off-by: Tianle Huang <[email protected]>
Signed-off-by: Tianle Huang <[email protected]>
Signed-off-by: Tianle Huang <[email protected]>
Signed-off-by: Tianle Huang <[email protected]>
Signed-off-by: Tianle Huang <[email protected]>
Signed-off-by: Tianle Huang <[email protected]>
Signed-off-by: Tianle Huang <[email protected]>
Signed-off-by: Tianle Huang <[email protected]>
Signed-off-by: Tianle Huang <[email protected]>
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

I am good with this if @peternied is good with this.

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Thank you for this change!

@peternied peternied merged commit a0e25d8 into opensearch-project:main Mar 1, 2022
@peternied
Copy link
Member

Can @opensearch-project/engineering-effectiveness update this PR after the change has been deployed?

I know there are a bunch of folks that would love to use 'latest' in their codebases.

@gaiksaya
Copy link
Member

gaiksaya commented Mar 1, 2022

Can @opensearch-project/engineering-effectiveness update this PR after the change has been deployed?

I know there are a bunch of folks that would love to use 'latest' in their codebases.

Looking into it. Will update this PR once the change is deployed. Thanks!

@gaiksaya
Copy link
Member

gaiksaya commented Mar 1, 2022

These changes have been deployed. @tianleh Can you check if it is working as expected?
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Add support for urls to substitute latest for the most recent build number
8 participants