Add Traffic Filter data source #619

andrew-moldovan · 2023-04-13T17:46:03Z

First contribution to this project so I'm approaching it slowly. The current implementation is super simple and just fetches the name when provided with an ID. (Also I'm sure there's a million things wrong with it still, never written Go before or touched the TF provider).

The reason behind this PR is #405. The ask there is to be able to fetch the id based on the name, so reverse of what I have now. The problem is that I'm not sure we have an API that can do that? Anyone got any ideas?

/cc @dimuon @tobio

Description

Related Issues

Motivation and Context

How Has This Been Tested?

Types of Changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Refactoring (improves code quality but has no user-facing effect)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation

Readiness Checklist

My code follows the code style of this project
My change requires a change to the documentation
I have updated the documentation accordingly
I have added tests to cover my changes
All new and existing tests passed

…ter to fetch the name

tobio · 2023-04-17T03:39:42Z

The problem is that I'm not sure we have an API that can do that? Anyone got any ideas?

List all rulesets then do client side filtering on the name I think is the only solution at the moment.

ec/ecdatasource/trafficfilterdatasource/datasource.go

…e of the filter we're looking for

ec/ecdatasource/trafficfilterdatasource/datasource.go

dimuon · 2023-04-18T09:06:53Z

@andrew-moldovan , I think it makes sense to add unit tests for modelToState, similar to ec/ecresource/trafficfilterresource/flatteners_test.go and an integration test for the new data source.

andrew-moldovan · 2023-04-18T13:55:16Z

Yep, I will add tests still, and expose more of the attributes. Just struggling with Go and learning how this all works, so progress is a bit slow

…ing more of the properties of the ruleset, though not yet complete

ec/version.go

andrew-moldovan · 2023-04-19T01:10:13Z

@dimuon could you please take a look at the test I have tried to write? I am struggling with types, figuring out how they all go and why they're so awful 😂 I feel like what I have should theoretically work, but it's yelling about something

…s a different id

andrew-moldovan · 2023-04-20T16:54:10Z

Ok so I've got the unit tests up and running and working on the acceptance tests. Couple things:

Almost all of them seem to be failing and I have a hard time believing that adding an extra data source has made them all fail. They're all failing with After applying this test step, the plan was not empty and it looks like they might be trying to add apm_secret_token?
For my actual acceptance test, I basically want to make sure it can filter by id/name/region (or even just one of them) and return the number of rulesets I expect and some details of the ruleset. However, I'm not sure what account is being used and if this is checking our QA? or Staging? Can I add a single traffic filter that I can make sure won't be deleted? Right now there seems to be 42 rulesets for us-east-1 so I'm worried that isn't likely to be a stable number. I see that the datasource_stack test essentially just checks to see if version is set but doesn't check to what it's set. I guess I could use something similar, ie just make sure that some rulesets are returned? But even that could be screwed if all the rulesets are deleted by some other process.

Any ideas @tobio @dimuon

dimuon · 2023-04-20T17:35:23Z

@andrew-moldovan, the first issue should be already fixed in master branch.

…

On Thu, 20 Apr 2023 at 18:54, Andrew Moldovan ***@***.***> wrote: Ok so I've got the unit tests up and running and working on the acceptance tests. Couple things: 1. Almost all of them seem to be failing and I have a hard time believing that adding an extra data source has made them all fail. They're all failing with After applying this test step, the plan was not empty and it looks like they might be trying to add apm_secret_token? 2. For my actual acceptance test, I basically want to make sure it can filter by id/name/region (or even just one of them) and return the number of rulesets I expect and some details of the ruleset. However, I'm not sure what account is being used and if this is checking our QA? or Staging? Can I add a single traffic filter that I can make sure won't be deleted? Right now there seems to be 42 rulesets for us-east-1 so I'm worried that isn't likely to be a stable number. I see that the datasource_stack test essentially just checks to see if version is set but doesn't check to *what* it's set. I guess I could use something similar, ie just make sure that *some* rulesets are returned? But even that could be screwed if all the rulesets are deleted by some other process. Any ideas @tobio <https://github.com/tobio> @dimuon <https://github.com/dimuon> — Reply to this email directly, view it on GitHub <#619 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACEMA64XNJNL5JUCTY5U4N3XCFS35ANCNFSM6AAAAAAW5M45KQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

dimuon · 2023-04-20T18:30:23Z

@andrew-moldovan , can you leverage ec_deployment_traffic_filter to create a filter or two and then reuse them in acceptance test? E.g. ec/acc/deployment_traffic_filter_test.go creates such traffic filters.

…n using that to query in the data source

…ithub.com/andrew-moldovan/terraform-provider-ec into elastic#405-add-traffic-filter-datasource

andrew-moldovan · 2023-04-20T22:08:04Z

@dimuon ah yes good call. I think I've got that working 🎉

Marking this as ready to review now

tobio

We should add some docs for this data source.

examples/deployment/deployment.tf

tobio

Small docs change, but lgtm. We should be generating these docs so happy to leave this as is for now and hopefully someo can automate it soon.

tobio · 2023-04-27T05:46:01Z

docs/data-sources/ec_trafficfilter.md

+* `region` (Optional) - Region where the traffic filter is. For Elastic Cloud Enterprise (ECE) installations, use `"ece-region`.
+
+## Attributes Reference
+See https://www.elastic.co/guide/en/cloud/current/definitions.html#TrafficFilterRulesets for the API guide


Suggested change

See https://www.elastic.co/guide/en/cloud/current/definitions.html#TrafficFilterRulesets for the API guide

See the [API guide](https://www.elastic.co/guide/en/cloud/current/definitions.html#TrafficFilterRulesets) for the available fields.

Although it would be better to document the actual structure here instead.

I deliberately didn't document the actual structure here because I felt that wasn't the right choice. Why am I copy/pasting the structure from the link and writing up my own descriptions (or copying them) when that structure could change or new things added or descriptions changed? Then we'd have to remember to update them in these docs as well instead of just having people visit the one source of truth

Just re-read your comment about autogenerating these docs and I totally agree with that!

when that structure could change or new things added or descriptions changed

Because API structure changes aren't automatically reflected here. We'd have to add those fields to the provider as well.

Adding a basic traffic filter data source that uses the ID of the fil…

aa4baba

…ter to fetch the name

tobio reviewed Apr 17, 2023

View reviewed changes

ec/ecdatasource/trafficfilterdatasource/datasource.go Outdated Show resolved Hide resolved

Pivoted to using the list api and filtering on the client for the nam…

dc25ccf

…e of the filter we're looking for

tobio reviewed Apr 18, 2023

View reviewed changes

ec/ecdatasource/trafficfilterdatasource/datasource.go Show resolved Hide resolved

ec/ecdatasource/trafficfilterdatasource/datasource.go Show resolved Hide resolved

ec/ecdatasource/trafficfilterdatasource/datasource.go Outdated Show resolved Hide resolved

andrew-moldovan added 2 commits April 18, 2023 10:57

adding the ability to filter on region and id as well and also return…

2a7a8bb

…ing more of the properties of the ruleset, though not yet complete

got the rules showing up as well 🎉

c2e9db6

andrew-moldovan commented Apr 18, 2023

View reviewed changes

ec/version.go Show resolved Hide resolved

andrew-moldovan added 2 commits April 18, 2023 11:39

fixing some lint

d2d17a9

struggling with tests

2b5ab94

andrew-moldovan added 5 commits April 19, 2023 10:04

wow a test actually works, quick commit before I screw it up more

0569bb8

refactored the tests and added a few more to cover the scenarios

5c9337c

and one more for matching region

fa561c7

adding a basic acceptance test though I expect it to fail because it'…

7e1e552

…s a different id

refactoring the acceptance test to use region instead of id

95ec473

Merge branch 'master' into elastic#405-add-traffic-filter-datasource

29ffabd

andrew-moldovan added 2 commits April 20, 2023 14:40

more acc test refactoring. Creating a traffic filter resource and the…

7e54a92

…n using that to query in the data source

Merge branch 'elastic#405-add-traffic-filter-datasource' of https://g…

43cde9f

…ithub.com/andrew-moldovan/terraform-provider-ec into elastic#405-add-traffic-filter-datasource

andrew-moldovan marked this pull request as ready for review April 20, 2023 22:08

andrew-moldovan requested a review from a team as a code owner April 20, 2023 22:08

tobio reviewed Apr 26, 2023

View reviewed changes

examples/deployment/deployment.tf Outdated Show resolved Hide resolved

removing the testing code from the example deployment and adding docs

943a12a

andrew-moldovan requested a review from alaudazzi as a code owner April 26, 2023 16:08

tobio approved these changes Apr 27, 2023

View reviewed changes

andrew-moldovan added 2 commits April 27, 2023 08:06

update docs link

30efe54

Merge branch 'master' into elastic#405-add-traffic-filter-datasource

bcf4d79

andrew-moldovan merged commit cd25ecf into elastic:master Apr 27, 2023

andrew-moldovan deleted the #405-add-traffic-filter-datasource branch April 27, 2023 17:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Traffic Filter data source #619

Add Traffic Filter data source #619

andrew-moldovan commented Apr 13, 2023

tobio commented Apr 17, 2023

dimuon commented Apr 18, 2023

andrew-moldovan commented Apr 18, 2023

andrew-moldovan commented Apr 19, 2023

andrew-moldovan commented Apr 20, 2023

dimuon commented Apr 20, 2023 via email

dimuon commented Apr 20, 2023

andrew-moldovan commented Apr 20, 2023

tobio left a comment

tobio left a comment

tobio Apr 27, 2023

andrew-moldovan Apr 27, 2023

andrew-moldovan Apr 27, 2023

tobio Apr 27, 2023

	See https://www.elastic.co/guide/en/cloud/current/definitions.html#TrafficFilterRulesets for the API guide
	See the [API guide](https://www.elastic.co/guide/en/cloud/current/definitions.html#TrafficFilterRulesets) for the available fields.

Add Traffic Filter data source #619

Add Traffic Filter data source #619

Conversation

andrew-moldovan commented Apr 13, 2023

Description

Related Issues

Motivation and Context

How Has This Been Tested?

Types of Changes

Readiness Checklist

tobio commented Apr 17, 2023

dimuon commented Apr 18, 2023

andrew-moldovan commented Apr 18, 2023

andrew-moldovan commented Apr 19, 2023

andrew-moldovan commented Apr 20, 2023

dimuon commented Apr 20, 2023 via email

dimuon commented Apr 20, 2023

andrew-moldovan commented Apr 20, 2023

tobio left a comment

Choose a reason for hiding this comment

tobio left a comment

Choose a reason for hiding this comment

tobio Apr 27, 2023

Choose a reason for hiding this comment

andrew-moldovan Apr 27, 2023

Choose a reason for hiding this comment

andrew-moldovan Apr 27, 2023

Choose a reason for hiding this comment

tobio Apr 27, 2023

Choose a reason for hiding this comment