Stop Workflows From Using Root Permissions in Cypress Container #1480
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ndouglas
reviewed
Feb 21, 2023
.github/workflows/a11y-testing.yml
Outdated
| # schedule: | ||
| # - cron: '0 12 * * 1-5' | ||
|
|
||
| name: ZZ Accessibilty Tests |
There was a problem hiding this comment.
Suggested change
| name: ZZ Accessibilty Tests | |
| name: Accessibility Tests |
There was a problem hiding this comment.
Using Tim's method of testing content-build related workflows:
https://dsva.slack.com/archives/CT4GZBM8F/p1674156682111419?thread_ts=1674149076.806359&cid=CT4GZBM8F
I know we've discussed creating a content-build-test repository and all the GHA infrastructure that goes along with it, but that comes with a host of its own problems. Namely protecting Test workflows from releasing content to production S3 buckets. Thing is we want to mirror Content-Build, so periodically overwrite Content-Build-Test with the former's current commits. That's all fine and good for CMS and CMS-Test but for CB destination buckets are set as ENV vars. Also Slack Notifications become a huge issue. Until we can find a safe way to resolve that we'll need to rely on these janky ZZ workflows that live in their own feature branches.
pic unrelated
va-vfs-bot
temporarily deployed
to
master/main/VACMS-12435-custom-cypress-container
va-vfs-bot
temporarily deployed
to
master/main/VACMS-12435-custom-cypress-container
va-vfs-bot
temporarily deployed
to
master/main/VACMS-12435-custom-cypress-container
va-vfs-bot
temporarily deployed
to
master/main/VACMS-12435-custom-cypress-container
va-vfs-bot
temporarily deployed
to
master/main/VACMS-12435-custom-cypress-container
olivereri
changed the title
va-vfs-bot
temporarily deployed
to
master/main/VACMS-12435-custom-cypress-container
va-vfs-bot
temporarily deployed
to
master/main/VACMS-12435-custom-cypress-container
va-vfs-bot
temporarily deployed
to
master/main/VACMS-12435-custom-cypress-container
va-vfs-bot
temporarily deployed
to
master/main/VACMS-12435-custom-cypress-container
va-vfs-bot
temporarily deployed
to
master/main/VACMS-12435-custom-cypress-container
olivereri
changed the title
timcosgrove
reviewed
Feb 24, 2023
| CHROMEDRIVER_FILEPATH: /share/chrome_driver/chromedriver | ||
|
|
||
| steps: | ||
| - name: Checkout content-build | ||
| uses: actions/checkout@v3 | ||
|
|
||
| # Required for Docker | ||
| - name: Move VA cert to /etc/ssl/certs | ||
| run: mv certs/VA-Internal-S2-RCA-combined.pem /etc/ssl/certs/ | ||
| #- name: Move VA cert to /etc/ssl/certs |
There was a problem hiding this comment.
If we don't need these, should we remove them?
There was a problem hiding this comment.
You're right, will do.
timcosgrove
approved these changes
Feb 24, 2023
va-vfs-bot
temporarily deployed
to
master/main/VACMS-12435-custom-cypress-container
ndouglas
reviewed
Feb 24, 2023
.github/workflows/a11y.yml
Outdated
| options: -u 1001:1001 -v /usr/local/share:/share --user root | ||
| options: -u 1001:1001 -v /usr/local/share:/share |
There was a problem hiding this comment.
might be nice to change the flags here to --user and --volume here, because we really should've noticed earlier that we were specifying the same argument twice
There was a problem hiding this comment.
diff --git a/.github/workflows/a11y-heading-order.yml b/.github/workflows/a11y-heading-order.yml
index 5c0fda9ac..f058f2699 100644
--- a/.github/workflows/a11y-heading-order.yml
+++ b/.github/workflows/a11y-heading-order.yml
@@ -125,7 +125,7 @@ jobs:
runs-on: [self-hosted, asg]
container:
image: public.ecr.aws/cypress-io/cypress/browsers:node16.13.2-chrome100-ff98
- options: -u 1001:1001 -v /usr/local/share:/share
+ options: --user 1001:1001 --volume /usr/local/share:/share
strategy:
fail-fast: false
max-parallel: 32
(1/1) Stage this hunk [y,n,q,a,d,e,?]? y
diff --git a/.github/workflows/a11y.yml b/.github/workflows/a11y.yml
index 4121b6b15..be9adeb0d 100644
--- a/.github/workflows/a11y.yml
+++ b/.github/workflows/a11y.yml
@@ -159,7 +159,7 @@ jobs:
runs-on: [self-hosted, asg]
container:
image: public.ecr.aws/cypress-io/cypress/browsers:node16.13.2-chrome100-ff98
- options: -u 1001:1001 -v /usr/local/share:/share
+ options: --user 1001:1001 --volume /usr/local/share:/share
strategy:
fail-fast: false
max-parallel: 32
(1/1) Stage this hunk [y,n,q,a,d,e,?]? y
diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml
index 1591db5a6..819494cd6 100644
--- a/.github/workflows/continuous-integration.yml
+++ b/.github/workflows/continuous-integration.yml
@@ -358,7 +358,7 @@ jobs:
timeout-minutes: 30
container:
image: public.ecr.aws/cypress-io/cypress/browsers:node16.13.2-chrome100-ff98
- options: -u 1001:1001
+ options: --user 1001:1001
volumes:
- /usr/local/share:/share
- /etc/ssl/certs:/etc/ssl/certs
(1/1) Stage this hunk [y,n,q,a,d,e,?]? y
ndouglas
approved these changes
Feb 24, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes the need to run Cypress containers as root in the below GHA workflows;
When these containers are run as root it changes the runner user (UID 1001) home directory files to root ownership. Other workflow jobs that use these runners fail to execute because they fail due to permission denied errors. Cypress containers DO NOT need to run as root. Adding the option to run as the runner user (UID 1001)
-u 1001:1001ensures that container mounted volumes retain the host user file permissions.Continuous Integration GHA workflow has been migrated away from using On-demand GHA runners and is directed to use AWS Auto-Scaling Group GHA runners. On-demand runners were configured to run as root and switching to ASG GHA runners obviates that.
Raw Slack discussion here:
https://dsva.slack.com/archives/CT4GZBM8F/p1677089769027479
closes department-of-veterans-affairs/va.gov-team#50148
relates to department-of-veterans-affairs/va.gov-cms#12435
Testing done & Screenshots
QA steps
What needs to be checked to prove this works?
Continuous Integration tests pass
What needs to be checked to prove it didn't break any related things?
Continuous Integration tests pass, Accessibility Tests and Heading Order tests continue to function normally.
What variations of circumstances (users, actions, values) need to be checked?
Acceptance criteria
Definition of done