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

Add allowed_account_id provider configuration parameter #226

Merged

Conversation

ennyjfrick
Copy link
Contributor

@ennyjfrick ennyjfrick commented Jan 9, 2025

What was changed

This PR adds a new optional provider configuration parameter, allowed_account_id, that, when set, prevents operations against accounts that do not have the same ID as that provided.

Why?

My org has a separate Temporal Cloud account for our development environment that we manage using IaC; currently, it is incredibly easy to accidentally operate on the wrong Temporal Cloud account by having the wrong API key active in our environment and has caused cross-contamination between our dev environment and staging/production environments. The AWS Terraform provider allows you to prevent this by providing an allowed_account_ids configuration parameter (implementation here), which we use for our AWS resources. This provides a similar functionality.

Checklist

1. Closes n/a

2. How was this tested:

This unfortunately needs to be tested manually (since the post-acceptance test delete returns an (expected) error). at a later date we might want to consider generating some mocks to fully test the provider configuration.
Test steps:

  1. build and install the provider as according to the Readme
  2. create a simple main.tf with the following, replacing ACCOUNT_ID with the appropriate account ID:
provider "temporalcloud" {
   allowed_account_id = "ACCOUNT_ID"
}

data "temporalcloud_namespaces" "test" {}
  1. run terraform init && terraform apply
  2. the apply should run successfully
  3. edit main.tf to match the following:
provider "temporalcloud" {
   allowed_account_id = "non-existent-account-id"
}

data "temporalcloud_namespaces" "test" {}
  1. run terraform init && terraform apply again
  2. this time, the apply should fail with an error stating that the current account ID does not match the allowed account ID

3. Any docs updates needed?

Docs generated!

@ennyjfrick ennyjfrick requested review from jlacefie and a team as code owners January 9, 2025 21:05
@ennyjfrick
Copy link
Contributor Author

@swgillespie Hi again! Wanted to ping you on this. We are running our Pulumi provider off of a fork of this repo that contains this change (and have been for some time); once this is in I can move the Pulumi provider off of the fork and make it public!

@briankassouf
Copy link
Contributor

Hi @ennyjfrick thanks for submitting this, i think it's a great idea. I'm wondering if you'd mind adding a test for this behavior in internal/provider/provider_test.go?

@ennyjfrick
Copy link
Contributor Author

Hey @briankassouf, I originally wrote an acceptance test for it but since it throws an error when callingTerraformCloudProvider.Configure, I can't test for an expected error when the account IDs mismatch as there doesn't seem to be a way to indicate an expected error on the destroy step.

The AWS provider handles this by setting up a bunch of mocks for the STS service endpoints; I am not aware of any existing similar mocks for cloudservicev1.CloudOperationsServer that we could inject.

@briankassouf
Copy link
Contributor

briankassouf commented Jan 9, 2025

@ennyjfrick I haven't used it before so I'm not certain, but I wonder if TestCase.ErrorCheck will let you ignore the error on destroy?

	// ErrorCheck allows providers the option to handle errors such as skipping
	// tests based on certain errors.
	//
	// This functionality is only intended for provider-controlled error
	// messaging. While in certain scenarios this can also catch testing logic
	// error messages, those messages are not protected by compatibility
	// promises.
	ErrorCheck ErrorCheckFunc

If we can't get it to work with the acceptance testing framework then that's ok, we can revisit adding a mocked backend at a later date.

@ennyjfrick
Copy link
Contributor Author

ennyjfrick commented Jan 9, 2025

@briankassouf hmm, doesn't look like we can make setting ErrorCheck work, the testing library doesn't call that function on a post-test destroy. I did try it out using the below snippet but no dice:

var destroyErrExp = regexp.MustCompile(`Error\srunning\spost-test\sdestroy.*`)

func TestAccProviderAllowedAccountID(t *testing.T) {
	config := func(accountID string) string {
		return fmt.Sprintf(`
provider "temporalcloud" {
   allowed_account_id = %q
}

data "temporalcloud_namespaces" "test" {}
`, accountID)
	}

	resource.ParallelTest(t, resource.TestCase{
		PreCheck:                 func() { testAccPreCheck(t) },
		ProtoV6ProviderFactories: testAccProtoV6ProviderFactories,
		ErrorCheck: func(err error) error {
			if destroyErrExp.MatchString(err.Error()) {
				return nil
			}
			return err
		},
		Steps: []resource.TestStep{
			{
				Config: config(defaultAccountID(t)),
			},
			{
				Config:      config("non-existent-account-id"),
				ExpectError: regexp.MustCompile(`.*current\saccount\sID\s'\w+'\sdoes\snot\smatch\sallowed\saccount\sID.*`),
			},
		},
	})
}

func defaultAccountID(t *testing.T) string {
	conn := newConnection(t)
	accountResp, err := conn.GetAccount(context.TODO(), &cloudservicev1.GetAccountRequest{})
	if err != nil {
		t.Fatalf("failed to get account information: %v", err)
	}
	return accountResp.GetAccount().GetId()
}

@briankassouf
Copy link
Contributor

Ok, thank you for trying!

@ennyjfrick ennyjfrick force-pushed the add-allowed-account-id-param branch from 6c5153b to 917c958 Compare January 9, 2025 23:11
@briankassouf briankassouf merged commit d6c7a3c into temporalio:main Jan 9, 2025
4 of 5 checks passed
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.

2 participants