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: invalid aws region #1129

Merged
merged 2 commits into from
Oct 23, 2023

Conversation

jbleduigou
Copy link
Contributor

@jbleduigou jbleduigou commented Oct 23, 2023

Problem

Provideds a fix for #1122

Solution

I believe that the bug lies in the fact that each goroutine is going to modify the region configured with the AWS client.
By making a shallow copy of the client we can modify the region without affecting the original client.

Changes Made

  • Update providers/aws/aws.go to make a shallow copy of the aws client

How to Test

Start komiser on an AWS account with several resources in different regions.

Screenshots

n/a

Notes

n/a

Checklist

  • Code follows the contributing guidelines
  • Changes have been thoroughly tested
  • Documentation has been updated, if necessary
  • Any dependencies have been added to the project, if necessary

Reviewers

@mlabouardy

@jbleduigou jbleduigou force-pushed the fix/1122-invalid-aws-region branch from f9e6563 to 92e046e Compare October 23, 2023 09:47
@AvineshTripathi
Copy link
Collaborator

AvineshTripathi commented Oct 23, 2023

Hey @jbleduigou can you try something like

for _, region := range listOfSupportedRegions {
    for _, fetchResources := range listOfSupportedServices() {
        regionCopy := region
        wp.SubmitTask(func() {
            client.AWSClient.Region = regionCopy 

        })
    }
}

instead of making whole client copy above example is a random thought and can be worked upon?

EDITED:

I gave a thought and I think we have to go your way itself

@mlabouardy mlabouardy added this to the v3.1.3 milestone Oct 23, 2023
@mlabouardy mlabouardy added the aws label Oct 23, 2023
@mlabouardy mlabouardy merged commit b51cdaf into tailwarden:develop Oct 23, 2023
3 checks passed
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