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

breaking vpn ip configuration to unique step #11620

Merged
merged 13 commits into from
Mar 11, 2024
Merged

Conversation

dwhitestratiform
Copy link
Contributor

@dwhitestratiform dwhitestratiform commented Mar 7, 2024

Description

When the vpn restriction code was merged into mcr we noticed a few things

  1. the a11y tests and e2e both had a reliance on the registering of github ip's. The registering of IPs happened in e2e, there is a scenario where the ip's don't register in time and a11y tests fail because lack of access
  2. registering ip's to the ip set and delisting them from the ip set for environments that were not restricted by vpn originally was considered "fine to run" in those environments but we've seen issues specifically in val

This PR aims to fix the above issues by doing the following

  1. moving the registering of IPs out to its own unique step so that we can ensure downstream jobs have the requirement.
  2. only running IP registering and ip cleanup steps only on ephemeral branches or branches that have vpn enabled

In addition I've added some debug into the cleanup step for future debugging should issues arise similar to what we saw where IPs could not be cleaned up in val.

When we merge this the expectation is that main, and val will no longer register IP's and run the cleanup step
if/when we want to add ip restrictions we'll need to update the Serverless conditional as well as the if statements in both register and cleanup steps.

Related ticket(s)


How to test

I wanted to ensure that we know what will happen in upper environments. I pushed a branch that created the waf and ipsets. The job ran through successfully including running the cleanup step that removes cidr blocks from the ipset after a successful run.
I modified the code and added this branch (waf-fix-refactor) as part of the conditional to mirror that of an upper environment where we do not have vpn restriction but also want to run tests, and also do not need to run the cleanup step. see screenshots below ... note the register IP step and the cleanup steps are being skipped but tests are still running:

configuration:
Screenshot 2024-03-07 at 6 10 57 PM

results:
Screenshot 2024-03-07 at 6 14 28 PM

I then removed my branch from the branch restrictions to ensure that registering the ip's and cleanup runs successfully. See below.... note register ip and cleanup run when then conditional for this branch is removed:

configuration:
Screenshot 2024-03-07 at 6 19 18 PM

results:

Screenshot 2024-03-07 at 7 02 17 PM

Important updates


Author checklist

  • I have performed a self-review of my code
  • I have added thorough tests, if necessary
  • I have updated relevant documentation, if necessary

convert to a different template: test → val | val → prod

@dwhitestratiform dwhitestratiform marked this pull request as ready for review March 8, 2024 00:04
@dwhitestratiform dwhitestratiform changed the title DRAFT -- waf refactor breaking vpn ip configuration to unique step Mar 8, 2024
.github/workflows/deploy.yml Outdated Show resolved Hide resolved
.github/workflows/deploy.yml Outdated Show resolved Hide resolved
Copy link

codeclimate bot commented Mar 8, 2024

Code Climate has analyzed commit 7199e5d and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (90% is the threshold).

This pull request will bring the total coverage in the repository to 96.8% (0.0% change).

View more on Code Climate.

@dwhitestratiform dwhitestratiform merged commit 5aa5b44 into main Mar 11, 2024
19 checks passed
@dwhitestratiform dwhitestratiform deleted the waf-fix-refactor branch March 11, 2024 12:59
This was referenced Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants