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

Use configure-aws-credentials workflow instead of passing secret_access_key #1363

Open
wants to merge 1 commit into
base: unstable
Choose a base branch
from

Conversation

vudiep411
Copy link

Summary

This PR fixes #1346 where we can get rid of the long term credentials by using OpenID Connect. OpenID Connect (OIDC) allows your GitHub Actions workflows to access resources in Amazon Web Services (AWS), without needing to store the AWS credentials as long-lived GitHub secrets.

Changes

We can remove these secrets that were passed in previously:

token: ${{ secrets.GITHUB_TOKEN }}
      bucket: ${{ secrets.AWS_S3_BUCKET }}
      access_key_id: ${{ secrets.AWS_S3_ACCESS_KEY_ID }}
      secret_access_key: ${{ secrets.AWS_S3_ACCESS_KEY }}

Instead we only need the role-to-assume arn. For more information OIDC.

Prerequisites

Before merging this PR, we need to make sure to set up the proper Identity providers on the production AWS account. Follow this guides.
Quick guide:

  1. Create an Identity provider with OpenID Connect.
    Provider url: https://token.actions.githubusercontent.com

    Audience: sts.amazonaws.com
Screenshot 2024-11-26 at 1 02 23 PM
  1. Assign the role into this new provider with this trust policy like below
{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Allow",
            "Principal": {
                "Federated": "arn:aws:iam::<aws_account_id>:oidc-provider/token.actions.githubusercontent.com"
            },
            "Action": "sts:AssumeRoleWithWebIdentity",
            "Condition": {
                "StringLike": {
                    "token.actions.githubusercontent.com:sub": [
                        "repo:valkey-io/valkey:ref:refs/heads/unstable",
                        "repo:valkey-io/valkey:ref:refs/tags/*"
                    ],
                    "token.actions.githubusercontent.com:aud": "sts.amazonaws.com"
                }
            }
        }
    ]
}
  1. In Github secrets, Add these secrets:
Screenshot 2024-11-26 at 2 11 02 PM

Results

Github action run:
Screenshot 2024-11-26 at 2 06 17 PM

Files in S3 Dev env:
Screenshot 2024-11-26 at 2 09 42 PM

Note: the failed workflow can be found in this issue #1343 which will be in a separate PR.

Signed-off-by: vudiep411 <vdiep@amazon.com>
Copy link

codecov bot commented Nov 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.56%. Comparing base (9305b49) to head (cd1fe2d).
Report is 2 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1363      +/-   ##
============================================
- Coverage     70.62%   70.56%   -0.06%     
============================================
  Files           117      117              
  Lines         63315    63315              
============================================
- Hits          44714    44679      -35     
- Misses        18601    18636      +35     

see 11 files with indirect coverage changes

Copy link
Member

@enjoy-binbin enjoy-binbin left a comment

Choose a reason for hiding this comment

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

seems ok, @roshkhatri please take a look

Copy link
Member

@roshkhatri roshkhatri left a comment

Choose a reason for hiding this comment

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

LGTM, just had a few questions.

@@ -11,6 +11,7 @@ on:
required: true

permissions:
id-token: write
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need the write permissions here?

Copy link
Author

Choose a reason for hiding this comment

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

This is to ensure that your workflow has the necessary permission to perform OIDC authentication with AWS.


permissions:
id-token: write
Copy link
Member

Choose a reason for hiding this comment

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

why do we need write permissions here?

Copy link
Author

Choose a reason for hiding this comment

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

Same as the comment above. Each job spawned in the matrix runs in a separate environment and each of this env needs permission to perform OIDC authentication with AWS.

Comment on lines -66 to -69
- name: Install AWS cli.
run: |
sudo apt-get install -y awscli

Copy link
Member

Choose a reason for hiding this comment

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

Is the aws cli installed by aws-actions/configure-aws-credentials@v4 ??

Copy link
Author

Choose a reason for hiding this comment

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

Yes, aws-actions/configure-aws-credentials@v4 will configured the runner environment with the proper AWS credentials and CLI

@roshkhatri
Copy link
Member

We will also need to do the prerequisites steps for the main repo before we merge this.

@roshkhatri
Copy link
Member

@vudiep411, will it be possible to add a test, where we uploads a test build binary to a test s3 bucket when ever changes are made to these workflows.

With this we can be sure that when we release stuff, it doesn't break on the main valkey repository

@vudiep411
Copy link
Author

vudiep411 commented Nov 27, 2024

Yes it is possible, we can use github environment for that. So we can write something like this:

- name: Configure AWS Credentials
        uses: aws-actions/configure-aws-credentials@v2
        with:
          role-to-assume: ${{ env.ROLE_TO_ASSUME }}
          role-session-name: ${{ env.ROLE_SESSION_NAME }}
          aws-region: ${{ env.AWS_REGION }}

We use env to dynamically assigned the role to assume and env. But this would required us to set up multiple OCID on different accounts not just in the workflow itself so it should be in a separate issue. For now we can just set this one up and after this looks good, I will open another issue for it if that's ok.
@roshkhatri

@roshkhatri
Copy link
Member

roshkhatri commented Nov 28, 2024

I think we can also do it based on the github event trigger maybe?

  1. We set two OCID for two buckets, releases and test.
  2. When the changes are made to release workflow files, for a pull_request event we run the same workflow with secrets.AWS_TEST_ROLE_TO_ASSUME and secrets.AWS_TEST_S3_BUCKET and for a workflow_dispatch and release
    event trigger we run with the current setup.
  3. We would do it at the head of unstable.
  4. Once we upload the test binaries, we can confirm the upload, delete the delete the file and pass the test.

Also, I think we should add the test on the same PR so we can run the test on this PR and know that the tests also work. We would also have to do the pre-requisites only once for both the scenarios.

@vudiep411
Copy link
Author

vudiep411 commented Nov 28, 2024

That's a really good suggestion. We can look into that. That way it would automated the testing of a PR with a test bucket. Definitely doable

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

Successfully merging this pull request may close these issues.

Use configure-aws-credentials instead of passing secret_access_key
3 participants