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

fix: force vpc flow logs to wait for auth policy sleep #949

Merged
merged 7 commits into from
Feb 11, 2025

Conversation

toddgiguere
Copy link
Contributor

Description

Added a reference to the time_sleep resource of bucket auth polices to the VPC Module input for flow log bucket, to implicitly force the flow logs to wait for all bucket auth policies to complete first.

This fixes a random timing issue in which flow logs are getting set up slightly before the auth policy for buckets are finished.

Release required?

  • No release
  • Patch release (x.x.X)
  • Minor release (x.X.x)
  • Major release (X.x.x)
Release notes content

Run the pipeline

If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.

Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:

/run pipeline

Checklist for reviewers

  • If relevant, a test for the change is included or updated with this PR.
  • If relevant, documentation for the change is included or updated with this PR.

For mergers

  • Use a conventional commit message to set the release level. Follow the guidelines.
  • Include information that users need to know about the PR in the commit message. The commit message becomes part of the GitHub release notes.
  • Use the Squash and merge option.

@toddgiguere toddgiguere requested a review from Aashiq-J as a code owner February 8, 2025 00:08
@toddgiguere
Copy link
Contributor Author

/run pipeline

@toddgiguere
Copy link
Contributor Author

/run pipeline

@toddgiguere
Copy link
Contributor Author

I believe this is working, there was only one error on the last run and it was a failure in schematics service.

Will keep trying, but I think this fix will resolve the timing issue on auth policies.

@toddgiguere
Copy link
Contributor Author

/run pipeline

@toddgiguere
Copy link
Contributor Author

@ocofaigh in the last test run (which is the current one right now) the only test to fail was TestRunUpgradeVpcPattern, which failed on main branch flow logs due to the timing issue I'm trying to fix. The problem with the upgrade tests here is that the main branch will still have the random occurance of the timing issue.

I'm leaving this latest run here, I think we can safely skip the upgrade tests again? In previous runs here on this PR this latest failure did not happen, so I think we will be at the mercy of the random fails (timing issue) on all the upgrade tests.

@toddgiguere
Copy link
Contributor Author

/run pipeline

@toddgiguere
Copy link
Contributor Author

/run pipeline

@ocofaigh
Copy link
Member

Can we merge #948 first so that the validation done uses schematics?

@toddgiguere
Copy link
Contributor Author

Only errors in latest run were upgrade errors, and they were all the same:

Resource(s) identified to be destroyed 
Name: wait_for_authorization_policy_buckets
Address: module.vpc_landing_zone.module.landing_zone.time_sleep.wait_for_authorization_policy_buckets
Actions: [delete]

This was the only resource reported by the upgrade tests, and it is expected, as I've changed the wait_for_authorization_policy_buckets from a single instance into a for_each multiple resource, one for each bucket.

I'm going to skip upgrade tests since these are expected, and to avoid the main branch failing for the timing reason we are fixing here.

@toddgiguere
Copy link
Contributor Author

/run pipeline

@ocofaigh ocofaigh merged commit ebeabf3 into terraform-ibm-modules:main Feb 11, 2025
2 checks passed
@terraform-ibm-modules-ops
Copy link
Contributor

🎉 This PR is included in version 7.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants