New resources: Traffic mirroring #9372

johnthedev97 · 2019-07-16T21:58:20Z

Adding support for AWS Traffic mirroring resources

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Release note for CHANGELOG:

Added the following resources to support AWS Traffic mirroring
* aws_vpc_traffic_mirror_filter
* aws_vpc_traffic_mirror_filter_rule
* aws_vpc_traffic_mirror_session
* aws_vpc_traffic_mirror_target

Output from acceptance testing:

make testacc TESTARGS="-run=TestAccAWSTrafficMirror"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSTrafficMirror -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSTrafficMirrorFilterRule_basic
=== PAUSE TestAccAWSTrafficMirrorFilterRule_basic
=== RUN   TestAccAWSTrafficMirrorFilter_basic
=== PAUSE TestAccAWSTrafficMirrorFilter_basic
=== RUN   TestAccAWSTrafficMirrorSession_basic
=== PAUSE TestAccAWSTrafficMirrorSession_basic
=== RUN   TestAccAWSTrafficMirrorTarget_nlb
=== PAUSE TestAccAWSTrafficMirrorTarget_nlb
=== RUN   TestAccAWSTrafficMirrorTarget_eni
=== PAUSE TestAccAWSTrafficMirrorTarget_eni
=== CONT  TestAccAWSTrafficMirrorFilterRule_basic
=== CONT  TestAccAWSTrafficMirrorTarget_eni
=== CONT  TestAccAWSTrafficMirrorSession_basic
=== CONT  TestAccAWSTrafficMirrorTarget_nlb
=== CONT  TestAccAWSTrafficMirrorFilter_basic
--- PASS: TestAccAWSTrafficMirrorFilter_basic (47.58s)
--- PASS: TestAccAWSTrafficMirrorFilterRule_basic (50.55s)
--- PASS: TestAccAWSTrafficMirrorTarget_eni (111.66s)
--- PASS: TestAccAWSTrafficMirrorTarget_nlb (290.85s)
--- PASS: TestAccAWSTrafficMirrorSession_basic (295.84s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	295.927s

...

Just started, saving progress, no testing done yet

… through all the fitlers in case of create and import

johnthedev97 · 2019-07-21T18:17:57Z

This became a little larger and longer than I thought. I have taken the [WIP] as I assume this will get reviewed only after that.

One thing I myself was not certain is regarding virtual_network_id in aws_traffic_mirror_session. The AWS API has set this field as optional, but if not passed AWS selects a random value for this. If I read this value and save in state it causes a change in every plan as the value from config comes by default as "0". So as a workaround I am reading this value into state only if it is set in config as well. If there is a better way to handle this let me know. I also had to set this field to ignore in import verify test.

Another doubt I had was regarding the state management. While writing tests I noticed that for many fields which AWS allows removal terraform doesn't remove those attributes from state, rather keep the attribute with Zero value in the state. This made me write many tests to use TestCheckResourceAttr with zero value though logically it made sense to use TestCheckNoResourceAttr. If terraform is by design storing removed attributes in state with Zero vale, I was wondering it would be a good idea to re-write TestCheckNoResourceAttr such that it passes even if the attribute is present but with Zero value.

johnthedev97 · 2019-08-20T20:28:44Z

@bflad Sorry to ping you directly, but since you are the one who merged my previous PR, I thought of asking you what does it take to get this merged?

pbartruff · 2019-09-04T19:34:11Z

Folks,

Is there reason this is lingering? It doesn't seem to cause and conflicts or is there a different effort underway to address the issue?

R/
Paul

aeschright · 2019-09-04T19:50:44Z

Hi everyone, per our guidelines it will be much easier for us to review and test these changes if each resource is in its own PR. Long PRs tend to linger because we have so many things in the queue, so I recommend you split it and resubmit. Thanks so much for getting this started!

pbartruff · 2019-09-05T01:20:05Z

@aeschright Thanks!
I'm not the requester; however, I understand the need for "small bites" at a time. Sadly all of these resources would be required to mirror packets. Individually they don't provide much. Maybe @johnthedev97 will be able to break it up a bit.

johnthedev97 · 2019-09-07T06:35:50Z

@aeschright , like @pbartruff has mentioned the resources are interdependent. traffic mirror filter rule tests will fail without traffic mirror filter. And the tests for session will fail without any of the other three resources. So I feel all these belong together, and should be reviewed together as well. Still if you think this needs to be broke into multiple PRs let me know, I will try my best to do it.

…form-provider-aws into feature/traffic-mirroring

chrislujan · 2019-11-04T20:59:09Z

Just checking in, hoping we haven't forgot about this.

stevesmoot · 2019-11-14T23:01:09Z

It really doesnt seem like splitting and resubmitting will help here, even if better per guidelines.

…erraform-provider-aws into feature/traffic-mirroring

johnthedev97 · 2020-02-17T17:51:39Z

Thanks @bflad I will look into your comments.

remove sidebar-current formatter move the resources inside EC2 subcategory

Removed partial() Remove variables in favour of inline declarations Favour "equals" instead of "SetXXX()" functions Added error handling while setting complex values Unset the Id if a filter is not found during read, causing terraform to remove it from state and suggesting adding it again remove explicit SetXX() is delete operation

bflad

Hi again @johnthedev97 👋 Thank you for the updates so far! Sorry for resubmitting the review items if you were still working through them, but I'm guessing GitHub marks them as outdated after files are moved 😖. I've resubmitted remaining items just so they still show up in the PR Files changed view (and not outdated). Please reach out with any questions or if you do not have time to finish any of them.

aws/resource_aws_ec2_traffic_mirror_filter.go

aws/resource_aws_ec2_traffic_mirror_filter_rule.go

aws/resource_aws_ec2_traffic_mirror_target.go

aws/resource_aws_ec2_traffic_mirror_target_test.go

website/docs/r/ec2_traffic_mirror_filter.html.markdown

Co-Authored-By: Brian Flad <bflad417@gmail.com>

johnthedev97 · 2020-02-21T06:27:11Z

@bflad I believe I have fixed all your comments.

bflad

Thank you for those updates, @johnthedev97 🚀 -- this should be the last batch. Since they're all one-liners, I'll try to auto-merge them via GitHub suggestions, but otherwise will handle these on merge and ensure acceptance testing passes. 👍

Output from acceptance testing:

--- PASS: TestAccAWSEc2TrafficMirrorFilter_basic (19.83s)
--- PASS: TestAccAWSEc2TrafficMirrorFilterRule_basic (20.80s)
--- PASS: TestAccAWSEc2TrafficMirrorTarget_eni (73.99s)
--- PASS: TestAccAWSEc2TrafficMirrorTarget_nlb (249.99s)
=== CONT  TestAccAWSEc2TrafficMirrorSession_basic

------- Stderr: -------
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x3e81ec7]

goroutine 1057 [running]:
github.com/terraform-providers/terraform-provider-aws/aws.resourceAwsEc2TrafficMirrorTargetRead(0xc000215490, 0x51bbae0, 0xc000975400, 0x0, 0x0)
	/opt/teamcity-agent/work/2e10e023da0c7520/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_ec2_traffic_mirror_target.go:99 +0x3e7
github.com/terraform-providers/terraform-provider-aws/aws.resourceAwsEc2TrafficMirrorTargetCreate(0xc000215490, 0x51bbae0, 0xc000975400, 0x2, 0xae9ae60)
	/opt/teamcity-agent/work/2e10e023da0c7520/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_ec2_traffic_mirror_target.go:71 +0x22a

aws/resource_aws_ec2_traffic_mirror_target.go

aws/resource_aws_ec2_traffic_mirror_session.go

aws/resource_aws_ec2_traffic_mirror_target.go

aws/resource_aws_ec2_traffic_mirror_session.go

aws/resource_aws_ec2_traffic_mirror_filter_rule.go

aws/resource_aws_ec2_traffic_mirror_filter.go

bflad

Thank you so much for all your work on this, @johnthedev97. This will make so many people happy! 😄 LGTM with fixes for the one test, which will be handled on merge. 🚀

Output from acceptance testing:

--- PASS: TestAccAWSEc2TrafficMirrorFilter_basic (19.81s)
--- PASS: TestAccAWSEc2TrafficMirrorFilterRule_basic (20.82s)
--- PASS: TestAccAWSEc2TrafficMirrorTarget_eni (63.62s)
--- PASS: TestAccAWSEc2TrafficMirrorTarget_nlb (240.87s)
--- PASS: TestAccAWSEc2TrafficMirrorSession_basic (282.32s)

bflad · 2020-02-21T16:50:53Z

aws/resource_aws_ec2_traffic_mirror_session_test.go

+			//create
+			{
+				Config: testAccTrafficMirrorSessionConfig(lbName, session),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSEc2TrafficMirrorSessionExists(resourceName),
+					resource.TestCheckResourceAttr(resourceName, "description", ""),
+					resource.TestCheckResourceAttr(resourceName, "packet_length", "0"),
+					resource.TestCheckResourceAttr(resourceName, "session_number", strconv.Itoa(session)),
+					resource.TestCheckNoResourceAttr(resourceName, "virtual_network_id"),
+				),
+			},
+			// update of description, packet length and VNI
+			{
+				Config: testAccTrafficMirrorSessionConfigWithOptionals(description, lbName, session, pLen, vni),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSEc2TrafficMirrorSessionExists(resourceName),
+					resource.TestCheckResourceAttr(resourceName, "description", description),
+					resource.TestCheckResourceAttr(resourceName, "packet_length", strconv.Itoa(pLen)),
+					resource.TestCheckResourceAttr(resourceName, "session_number", strconv.Itoa(session)),
+					resource.TestCheckResourceAttr(resourceName, "virtual_network_id", strconv.Itoa(vni)),
+				),
+			},
+			// removal of description, packet length and VNI
+			{
+				Config: testAccTrafficMirrorSessionConfig(lbName, session),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSEc2TrafficMirrorSessionExists(resourceName),
+					resource.TestCheckResourceAttr(resourceName, "description", ""),
+					resource.TestCheckResourceAttr(resourceName, "packet_length", "0"),
+					resource.TestCheckResourceAttr(resourceName, "session_number", strconv.Itoa(session)),
+					resource.TestCheckResourceAttr(resourceName, "virtual_network_id", "0"),
+				),
+			},
+			// import test without VNI
+			{
+				ResourceName:            resourceName,
+				ImportState:             true,
+				ImportStateVerify:       true,
+				ImportStateVerifyIgnore: []string{"virtual_network_id"},
+			},


Now that virtual_network_id is always set in the Terraform state, the following allows this test to pass:

//create { Config: testAccTrafficMirrorSessionConfig(lbName, session), Check: resource.ComposeTestCheckFunc( testAccCheckAWSEc2TrafficMirrorSessionExists(resourceName), resource.TestCheckResourceAttr(resourceName, "description", ""), resource.TestCheckResourceAttr(resourceName, "packet_length", "0"), resource.TestCheckResourceAttr(resourceName, "session_number", strconv.Itoa(session)), resource.TestMatchResourceAttr(resourceName, "virtual_network_id", regexp.MustCompile(`\d+`)), ), }, // update of description, packet length and VNI { Config: testAccTrafficMirrorSessionConfigWithOptionals(description, lbName, session, pLen, vni), Check: resource.ComposeTestCheckFunc( testAccCheckAWSEc2TrafficMirrorSessionExists(resourceName), resource.TestCheckResourceAttr(resourceName, "description", description), resource.TestCheckResourceAttr(resourceName, "packet_length", strconv.Itoa(pLen)), resource.TestCheckResourceAttr(resourceName, "session_number", strconv.Itoa(session)), resource.TestCheckResourceAttr(resourceName, "virtual_network_id", strconv.Itoa(vni)), ), }, // removal of description, packet length and VNI { Config: testAccTrafficMirrorSessionConfig(lbName, session), Check: resource.ComposeTestCheckFunc( testAccCheckAWSEc2TrafficMirrorSessionExists(resourceName), resource.TestCheckResourceAttr(resourceName, "description", ""), resource.TestCheckResourceAttr(resourceName, "packet_length", "0"), resource.TestCheckResourceAttr(resourceName, "session_number", strconv.Itoa(session)), resource.TestMatchResourceAttr(resourceName, "virtual_network_id", regexp.MustCompile(`\d+`)), ), }, // import test without VNI { ResourceName: resourceName, ImportState: true, ImportStateVerify: true, },

Will handle on merge. 👍

--- PASS: TestAccAWSEc2TrafficMirrorSession_basic (282.32s)

johnthedev97 · 2020-02-21T17:54:38Z

Thanks @bflad for all the valuable review comments, pointers and the last set of fixes :-)

…icMirrorSession_basic Reference: #9372 (comment) Output from acceptance testing: ``` --- PASS: TestAccAWSEc2TrafficMirrorSession_basic (339.57s) ```

ghost · 2020-02-28T14:39:05Z

This has been released in version 2.51.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 for triage. Thanks!

ghost · 2020-03-23T14:50:39Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

johnthedev97 added 3 commits July 15, 2019 06:55

initial commit

d71e53f

Just started, saving progress, no testing done yet

added network service option and update capability to filter and tested

294f89a

filter and filter rule working

aacd9f3

johnthedev97 requested a review from a team July 16, 2019 21:58

ghost added size/XL Managed by automation to categorize the size of a PR. provider Pertains to the provider itself, rather than any interaction with AWS. labels Jul 16, 2019

johnthedev97 and others added 3 commits July 17, 2019 01:05

Added mirror target, fixed a typo in filter rule

5efbfb3

Add mirror session

3b6b498

First round of completion - functionality, tests and docs

fcf9f8b

johnthedev97 added 2 commits July 21, 2019 19:11

fix a lint error

f7c1fdb

refactor the filter rule to avoid composite ID at the cost of looping…

89987c4

… through all the fitlers in case of create and import

johnthedev97 changed the title ~~[WIP] New resources: Traffic mirroring~~ New resources: Traffic mirroring Jul 21, 2019

johnthedev97 added 2 commits July 28, 2019 11:01

Merge branch 'master' into feature/traffic-mirroring

306f82b

restructure the documentation according to new structure

ad58bef

aeschright added the new-resource Introduces a new resource. label Sep 11, 2019

Merge branch 'master' of https://github.com/terraform-providers/terra…

b54f538

…form-provider-aws into feature/traffic-mirroring

crjohn added 2 commits November 19, 2019 11:43

Merge branch 'feature/traffic-mirroring' of github.com:johnthedev97/t…

3b01491

…erraform-provider-aws into feature/traffic-mirroring

refactored for terraform-plugin-sdk

6cd9db0

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Feb 11, 2020

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Feb 17, 2020

johnthedev97 added 3 commits February 17, 2020 22:14

renaming files and resources

fdd7f92

fix subcategory

96879ec

Documentation fixes

2fab962

remove sidebar-current formatter move the resources inside EC2 subcategory

ghost added the service/ec2 Issues and PRs that pertain to the ec2 service. label Feb 17, 2020

johnthedev97 added 3 commits February 17, 2020 22:31

Merge branch 'master' into feature/traffic-mirroring

8e5ba61

fix website lint error

5a702cf

bflad suggested changes Feb 18, 2020

View reviewed changes

johnthedev97 and others added 5 commits February 20, 2020 19:23

Update aws/resource_aws_ec2_traffic_mirror_target.go

1069f81

Co-Authored-By: Brian Flad <bflad417@gmail.com>

Update aws/resource_aws_ec2_traffic_mirror_filter_rule.go

37fd41d

Co-Authored-By: Brian Flad <bflad417@gmail.com>

Update aws/resource_aws_ec2_traffic_mirror_session.go

f1211eb

Co-Authored-By: Brian Flad <bflad417@gmail.com>

review fixes

445055e

fix spelling mistake

861b78f

bflad self-requested a review February 21, 2020 14:33

bflad added this to the v2.51.0 milestone Feb 21, 2020

bflad suggested changes Feb 21, 2020

View reviewed changes

service/ec2: Apply suggestions from hashicorp#9372 code review

9d59e2a

bflad approved these changes Feb 21, 2020

View reviewed changes

bflad merged commit 473cfe9 into hashicorp:master Feb 21, 2020

bflad added a commit that referenced this pull request Feb 21, 2020

Update CHANGELOG for #9372

01f8a69

bflad added a commit that referenced this pull request Feb 21, 2020

Fix CHANGELOG for #9372

ae75981

ghost locked and limited conversation to collaborators Mar 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New resources: Traffic mirroring #9372

New resources: Traffic mirroring #9372

johnthedev97 commented Jul 16, 2019 •

edited

Loading

johnthedev97 commented Jul 21, 2019 •

edited

Loading

johnthedev97 commented Aug 20, 2019

pbartruff commented Sep 4, 2019

aeschright commented Sep 4, 2019

pbartruff commented Sep 5, 2019

johnthedev97 commented Sep 7, 2019 •

edited

Loading

chrislujan commented Nov 4, 2019

stevesmoot commented Nov 14, 2019

johnthedev97 commented Feb 17, 2020

bflad left a comment

johnthedev97 commented Feb 21, 2020

bflad left a comment

bflad left a comment

bflad Feb 21, 2020

johnthedev97 commented Feb 21, 2020

ghost commented Feb 28, 2020

ghost commented Mar 23, 2020

New resources: Traffic mirroring #9372

New resources: Traffic mirroring #9372

Conversation

johnthedev97 commented Jul 16, 2019 • edited Loading

Community Note

johnthedev97 commented Jul 21, 2019 • edited Loading

johnthedev97 commented Aug 20, 2019

pbartruff commented Sep 4, 2019

aeschright commented Sep 4, 2019

pbartruff commented Sep 5, 2019

johnthedev97 commented Sep 7, 2019 • edited Loading

chrislujan commented Nov 4, 2019

stevesmoot commented Nov 14, 2019

johnthedev97 commented Feb 17, 2020

bflad left a comment

Choose a reason for hiding this comment

johnthedev97 commented Feb 21, 2020

bflad left a comment

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

bflad Feb 21, 2020

Choose a reason for hiding this comment

johnthedev97 commented Feb 21, 2020

ghost commented Feb 28, 2020

ghost commented Mar 23, 2020

johnthedev97 commented Jul 16, 2019 •

edited

Loading

johnthedev97 commented Jul 21, 2019 •

edited

Loading

johnthedev97 commented Sep 7, 2019 •

edited

Loading