-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
adds an event bus policy resource #16874
Conversation
|
Managed to get the diff suppression working! |
Thank you for your contribution! 🚀 Please note that typically Go dependency changes are handled in this repository by dependabot or the maintainers. This is to prevent pull request merge conflicts and further delay reviews of contributions. Remove any changes to the Additional details:
|
Thank you for your contribution! 🚀 Please note that the Remove any changes to the |
Thanks a lot for the contribution @cohen990 - This is the last feature to complete EventBridge feature parity with CloudFormation. I'll look into it early next week. At a first glance, it looks straightforward enough, so might be able to finish triaging next week the latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good - missing tests on update/delete, and check on policy properties for safety update.
We also updated validateFunc since this PR was created so you can simply accept the suggestion as tests are broken now.
After these happy to send to prioritization so we can complete EventBridge feature coverage on TF
Thanks a lot for the hard work on this!
Check: resource.ComposeTestCheckFunc( | ||
testAccCheckAWSCloudwatchEventBusPolicyExists(resourceName), | ||
), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add an extra step to test for updates?
e.g.
Config: testAccIAMGroupPolicyConfigUpdate(rInt), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testAccAWSCloudwatchEventBusPolicyConfigUpdate
added.
Steps: []resource.TestStep{ | ||
{ | ||
Config: testAccAWSCloudwatchEventBusPolicyConfig(rstring), | ||
Check: resource.ComposeTestCheckFunc( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you check for properties in the policy to confirm they match key details like principal and statement sid?
e.g. resource.TestCheckResourceAttr(resourceName1, "principal", principal),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return fmt.Errorf("Creating CloudWatch Events policy failed: %w", err) | ||
} | ||
|
||
d.SetId(eventBusName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be wrong, but I'm on the fence about this not being a combination of eventBus + policy statement ID (unique), like in the event_permission
resource.
I'd defer to core maintainers on this one
return output.Policy, nil | ||
} | ||
|
||
func resourceAwsCloudWatchEventBusPolicyUpdate(d *schema.ResourceData, meta interface{}) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you create a test updating a policy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testAccAWSCloudwatchEventBusPolicyConfigUpdate
added.
return resourceAwsCloudWatchEventBusPolicyRead(d, meta) | ||
} | ||
|
||
func resourceAwsCloudWatchEventBusPolicyDelete(d *schema.ResourceData, meta interface{}) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you create a test for deletion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestAccAWSCloudwatchEventBusPolicy_disappears
added.
return fmt.Errorf("Creating CloudWatch Events policy failed: %w", err) | ||
} | ||
|
||
d.SetId(eventBusName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on the fence of this not being a combination of bus name + statement Id - I suspect it's okay given you can have a single resource policy per bus, so I'll defer to core maintainers on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering the "aws_cloudwatch_event_bus_policy" resource supports multiple statements we think that making it unique for each event bus won't introduce limitations. Also this implementation appears to be consistent with the SDK/CLI, which don't model event bus permissions as independent resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't disagree. I don't just understand what the rationale was behind aws_cloudwatch_event_permission
to create an unique ID back then: https://github.com/hashicorp/terraform-provider-aws/blob/fafcf78238182983aa8a5238c326f9049448aa70/aws/resource_aws_cloudwatch_event_permission.go#L103-L102
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "aws_cloudwatch_event_permission" resource only defines one statement in the policy, and it's therefore identified by it. The resource "aws_cloudwatch_event_policy" defines the complete policy for the event bus (possibly with multiple statements) and it's intended to be unique for each event bus.
Config: testAccAWSCloudwatchEventBusPolicyConfig(rstring), | ||
Check: resource.ComposeTestCheckFunc( | ||
testAccCheckAWSCloudwatchEventBusPolicyExists(resourceName), | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you check for policy properties like statement_id and principal? e.g. check whether policy matches what's being created
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This now does a deep diff from TF state and what's available in the bus. It does the job and run through the entire logic IMHO and state.
Deferring to Hashicorp core team if they want to be nitpick and check whether X key has changed.
|
||
```hcl | ||
resource "aws_cloudwatch_event_bus_policy" "test" { | ||
policy = data.aws_iam_policy_document.access.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add this sample policy as part of the example to improve docs experience?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs added with multiple examples. LGTM
update according to suggestion Co-authored-by: Heitor Lessa <[email protected]>
Yes, that’s correct.
…On Tue, 15 Jun 2021 at 18:11, Francesco Fucile ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In aws/resource_aws_cloudwatch_event_bus_policy.go
<#16874 (comment)>
:
> +
+ log.Printf("[DEBUG] Update CloudWatch EventBus policy: %s", input)
+ _, err := conn.PutPermission(&input)
+ if isAWSErr(err, events.ErrCodeResourceNotFoundException, "") {
+ log.Printf("[WARN] CloudWatch EventBus %q not found, removing from state", d.Id())
+ d.SetId("")
+ return nil
+ }
+ if err != nil {
+ return fmt.Errorf("error updating policy for CloudWatch EventBus (%s): %w", d.Id(), err)
+ }
+
+ return resourceAwsCloudWatchEventBusPolicyRead(d, meta)
+}
+
+func resourceAwsCloudWatchEventBusPolicyDelete(d *schema.ResourceData, meta interface{}) error {
Are you referring to a test for
"resourceAwsCloudWatchEventBusPolicyDelete"? Something like deleting the
policy and verifying it doesn't exist anymore using the SDK?
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#16874 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZPQBANDMT6LJKXSNUDXSDTS53THANCNFSM4VFSETCA>
.
|
Thanks for the comments, Francesco! I'll have a second look tomorrow,
especially the deletion test.
…On Thu, 17 Jun 2021 at 15:16, Francesco Fucile ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In aws/resource_aws_cloudwatch_event_bus_policy.go
<#16874 (comment)>
:
> +
+ eventBusName := d.Get("event_bus_name").(string)
+ policy := d.Get("policy").(string)
+
+ input := events.PutPermissionInput{
+ EventBusName: aws.String(eventBusName),
+ Policy: aws.String(policy),
+ }
+
+ log.Printf("[DEBUG] Creating CloudWatch Events policy: %s", input)
+ _, err := conn.PutPermission(&input)
+ if err != nil {
+ return fmt.Errorf("Creating CloudWatch Events policy failed: %w", err)
+ }
+
+ d.SetId(eventBusName)
Considering the "aws_cloudwatch_event_bus_policy" resource supports
multiple statements we think that making it unique for each event bus won't
introduce limitations. Also this implementation appears to be consistent
with the SDK/CLI, which don't model event bus permissions as independent
resources.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#16874 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZPQBCKJ2WBZJUFLKXWXR3TTHYT5ANCNFSM4VFSETCA>
.
|
…s. Fix linting for TF configuration defined in tests. Add "attributes reference" paragraph in resource docs.
Not that I’m aware no. I’ve tried looking for references to the
Organization’s resource test where we use an extra data caller identity
which can be tested using TestResourceAttrPair, but it wouldn’t in this
case.
My best guess would be to create a function to JSON decode and assert
whether a given key has the expected value - this would be useful to test
the Update logic that isn’t tested atm, for example.
…On Wed, 16 Jun 2021 at 12:34, Francesco Fucile ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In aws/resource_aws_cloudwatch_event_bus_policy_test.go
<#16874 (comment)>
:
> + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
+ "github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
+)
+
+func TestAccAWSCloudwatchEventBusPolicy_basic(t *testing.T) {
+ resourceName := "aws_cloudwatch_event_bus_policy.test"
+ rstring := acctest.RandString(5)
+
+ resource.ParallelTest(t, resource.TestCase{
+ PreCheck: func() { testAccPreCheck(t) },
+ Providers: testAccProviders,
+ CheckDestroy: testAccCheckAWSCloudWatchEventBusDestroy,
+ Steps: []resource.TestStep{
+ {
+ Config: testAccAWSCloudwatchEventBusPolicyConfig(rstring),
+ Check: resource.ComposeTestCheckFunc(
The problem with this check is that we are setting the whole policy JSON
text as a field in the "event_bus_policy" resource configuration. We are
therefore not able to check fields individually, we can just compare the
JSON document in the state with a reference. Since this JSON is generated
at runtime we are not able to set such reference as a parameter for the
test function.
We found a similar scenario for the "aws_iam_role
<https://github.com/Cazoo-uk/terraform-provider-aws/blob/42c30754145d2a5a1bfb2a3eb5e765a5e9caa782/aws/data_source_aws_iam_role_test.go#L70>".
In this case the tests make use of a data source for re-importing the
configuration of the generated resource into the state and compare its
fields with the ones of the previously generated resource. Implementing a
solution like this would require us to implement the "event_bus_policy"
data source.
Is there a different way to handle this case?
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#16874 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZPQBBBLIPGUTMOLQ5NA6DTTB45FANCNFSM4VFSETCA>
.
|
Extend tests for the "aws_cloudwatch_event_bus_policy" resource as suggested from the Terraform maintainers.
@heitorlessa this should be ready for another review :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - Hashicorp core team might want to check whether it needs a attribute check on update test or whether the current deep value check of what's in TF state vs what's available in the Cloud suffice.
Sending to Hashicorp folks to prioritize it ;) Thanks a lot for those changes @cohen990 !
Latest test results
make testacc TESTARGS='-run=TestAccAWSCloudwatchEventBusPolicy'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSCloudwatchEventBusPolicy -timeout 180m
=== RUN TestAccAWSCloudwatchEventBusPolicy_basic
=== PAUSE TestAccAWSCloudwatchEventBusPolicy_basic
=== RUN TestAccAWSCloudwatchEventBusPolicy_disappears
=== PAUSE TestAccAWSCloudwatchEventBusPolicy_disappears
=== CONT TestAccAWSCloudwatchEventBusPolicy_basic
=== CONT TestAccAWSCloudwatchEventBusPolicy_disappears
--- PASS: TestAccAWSCloudwatchEventBusPolicy_basic (51.69s)
--- PASS: TestAccAWSCloudwatchEventBusPolicy_disappears (146.29s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 149.018s
return resourceAwsCloudWatchEventBusPolicyRead(d, meta) | ||
} | ||
|
||
func resourceAwsCloudWatchEventBusPolicyDelete(d *schema.ResourceData, meta interface{}) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestAccAWSCloudwatchEventBusPolicy_disappears
added.
Config: testAccAWSCloudwatchEventBusPolicyConfig(rstring), | ||
Check: resource.ComposeTestCheckFunc( | ||
testAccCheckAWSCloudwatchEventBusPolicyExists(resourceName), | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This now does a deep diff from TF state and what's available in the bus. It does the job and run through the entire logic IMHO and state.
Deferring to Hashicorp core team if they want to be nitpick and check whether X key has changed.
Check: resource.ComposeTestCheckFunc( | ||
testAccCheckAWSCloudwatchEventBusPolicyExists(resourceName), | ||
), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testAccAWSCloudwatchEventBusPolicyConfigUpdate
added.
Steps: []resource.TestStep{ | ||
{ | ||
Config: testAccAWSCloudwatchEventBusPolicyConfig(rstring), | ||
Check: resource.ComposeTestCheckFunc( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return output.Policy, nil | ||
} | ||
|
||
func resourceAwsCloudWatchEventBusPolicyUpdate(d *schema.ResourceData, meta interface{}) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testAccAWSCloudwatchEventBusPolicyConfigUpdate
added.
Actually, @cohen990, it seems that the changelog vanished in the last merge - Could you re-add it? If it wasn't there in the first place and I dreamed it :D, here's what the file should look like so CI is happy about it. File: Content: triple backsticks with
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀.
Commercial
% make testacc TEST=./aws TESTARGS='-run=TestAccAWSCloudwatchEventBusPolicy_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSCloudwatchEventBusPolicy_ -timeout 180m
=== RUN TestAccAWSCloudwatchEventBusPolicy_basic
=== PAUSE TestAccAWSCloudwatchEventBusPolicy_basic
=== RUN TestAccAWSCloudwatchEventBusPolicy_disappears
=== PAUSE TestAccAWSCloudwatchEventBusPolicy_disappears
=== CONT TestAccAWSCloudwatchEventBusPolicy_basic
=== CONT TestAccAWSCloudwatchEventBusPolicy_disappears
--- PASS: TestAccAWSCloudwatchEventBusPolicy_basic (25.64s)
--- PASS: TestAccAWSCloudwatchEventBusPolicy_disappears (133.16s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 136.188s
GovCloud
% make testacc TEST=./aws TESTARGS='-run=TestAccAWSCloudwatchEventBusPolicy_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSCloudwatchEventBusPolicy_ -timeout 180m
=== RUN TestAccAWSCloudwatchEventBusPolicy_basic
=== PAUSE TestAccAWSCloudwatchEventBusPolicy_basic
=== RUN TestAccAWSCloudwatchEventBusPolicy_disappears
=== PAUSE TestAccAWSCloudwatchEventBusPolicy_disappears
=== CONT TestAccAWSCloudwatchEventBusPolicy_basic
=== CONT TestAccAWSCloudwatchEventBusPolicy_disappears
--- PASS: TestAccAWSCloudwatchEventBusPolicy_basic (35.24s)
--- PASS: TestAccAWSCloudwatchEventBusPolicy_disappears (138.06s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 141.106s
Add CHANGELOG entry for #16874
This functionality has been released in v3.47.0 of the Terraform AWS Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
Community Note
Closes #16838
Release note for CHANGELOG:
Output from acceptance testing: