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

(internal) Delay SA roles and key resource creation #13

Closed
wants to merge 3 commits into from

Conversation

ravinadhruve10
Copy link
Contributor

Change summary:

Delaying the resources for IAM roles assignment and key creation till the GCP Service account is created. This is to ensure the SA is ready for consumption since its creation is eventually consistent.

Tested using TF multiple times to ensure we don't run into 403s from googleapis when GCP takes time to have the SA ready for consumption.

Change summary:
----------------
Delaying the resources for IAM roles assignment and key creation
till the GCP Service account is created. This is to ensure the SA
is ready for consumption since its creation is eventually consistent.

Tested using TF multiple times to ensure we don't run into 403s
from googleapis when GCP takes time to have the SA ready for
consumption.
Copy link
Contributor

@cgeers cgeers left a comment

Choose a reason for hiding this comment

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

As much as everyone hates even the idea of sleeping, c'est la vie, blame google. However, this isn't what I expected to see. Here, we have all google cloud resources depending upon a sleep, which means the sleep will be executed before the creation of these resources, right?

Ideally, it's the sysdig_secure_organization that should be depending on the sleep, right? Since that's the only thing really affected by this. Since we can't express that here, shouldn't this dependency be flipped? Shouldn't we be sleeping as the last thing this module does?

@blacksd
Copy link

blacksd commented Nov 3, 2023

Random and possibly uncalled-for suggestion, but I'd refrain from using a null_resource with a local exec; it's a bad form - it requires to handle explicit dependencies from downstream resources, it's a one-off that requires tainting/destruction to be reapplied, etc.
A much better way would be to use the hashicorp/time provider - you can set triggers tied to an upstream changing resource and read from those in the downstream modules. It supports the mutations happening upstream and it provides a consistent interface for those attributes in any downstream modules, without explicit depends_on notations required.

Copy link
Contributor

@haresh-suresh haresh-suresh left a comment

Choose a reason for hiding this comment

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

@cgeers @ravinadhruve10

I want to address the concern about not adding a sleep at the very end as opposed to at every role assignment. I agree that intuitively that feels heavy handed.

But digging into TF documentation & suggestions, turns out the only way to do that would be some sort of explicit remote-exec query that can validate some of the role assignments itself. But googling & digging around, I can't find any way to do that for ggogle_service_account resource.

TLDR short of introducing a sleep at the onboarding API main.tf, this is the only workable approach, that I can see working.

So approving this from my side 👍
LMK what you guys think.

@cgeers
Copy link
Contributor

cgeers commented Nov 3, 2023

@blacksd thanks for the help. We're going to close this PR and try this again in a different way, as you suggest.

@cgeers cgeers closed this Nov 3, 2023
@cgeers cgeers deleted the gcp-sa-consistency branch November 3, 2023 19:17
@cgeers cgeers restored the gcp-sa-consistency branch November 3, 2023 19:59
@cgeers cgeers deleted the gcp-sa-consistency branch November 3, 2023 21:22
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.

4 participants