testing: add handling of unknown providers and empty terraform #55

xlr-8 · 2021-08-12T09:18:53Z

This PR adds the handling of unknown provider (eg. vmware) and also cases of empty terraform plan/module. That allows to give better output when the cost estimated is '0.0'. Now instead of having '0.0' an error will be returned.

In order to improve the testing, some pricing dump got also injected as part of it.

Related to: #21

xescugc

I feel like the HCL case is not handled and that this is maybe not the best way to handle the case that we do not have 0.0.

There are 2 ways to do it:

On the functions of the cost.Plan
Initialization of the Plan if the prior and planned are totally empty.

Or following what you did you could also add the if onto the terraform.ExtractQueriesFromHCL

e2e/aws_estimation_test.go

xescugc · 2021-08-12T14:54:53Z

terraform/error.go

+	ErrNoQueries       = fmt.Errorf("no terraform entities found, looks empty")
+	ErrNoKnownProvider = fmt.Errorf("terraform providers are not yet supported")


It's the same but I would use error.New as fmt.Errorf is mainly used to create interpolation messages with %s and so on.

xescugc · 2021-08-12T14:55:40Z

terraform/hcl.go

 }

 // extractHCLModule returns the resources found in the provided module.
 func extractHCLModule(providers map[string]Provider, parser *configs.Parser, modPath string, modName string, mod *configs.Module, evalCtx *hcl.EvalContext) ([]query.Resource, error) {
 	queries := make([]query.Resource, 0, len(mod.ManagedResources))

+	// knownProvider will remain false, until one resource from a
+	// known provider is encountered. This allows to know if through
+	// all the given resources some could	be estimated or not


Maybe it's GH but I see a weird space on could be estimated or not

xescugc · 2021-08-12T15:02:48Z

terraform/error.go

+
+// Errors that might be returned from procesing the HCL
+var (
+	ErrNoQueries       = fmt.Errorf("no terraform entities found, looks empty")


Means empty or that we do not support any of the resources on the Plan/HCL?

Means no queries were found - eg something generates a diff where there is nothing to process or contains no files.

xlr-8 · 2021-08-12T15:54:33Z

I feel like the HCL case is not handled and that this is maybe not the best way to handle the case that we do not have 0.0.

Why? Both cases are tested on the PR though?

There are 2 ways to do it:
* On the functions of the cost.Plan
* Initialization of the Plan if the prior and planned are totally empty.

HCL doesn't contain prior and plan costs. Plus as we are speaking of plan, you could have some prior queries, but no new ones.

Or following what you did you could also add the if onto the terraform.ExtractQueriesFromHCL

I'm not sure to follow your point there.

xlr-8 · 2021-08-12T15:55:04Z

I'm currently having issues with the pipeline to properly test it, so I'll remove the need review label.

xescugc · 2021-08-13T06:43:19Z

Why? Both cases are tested on the PR though?

IDK why I did only see the NoQuereis on the State and not the HCL case

HCL doesn't contain prior and plan costs. Plus as we are speaking of plan, you could have some prior queries, but no new ones.

What I don't like about your solution is that it's the same logic in 2 places, and the Queries itself are of no "use" until you put them into a State. And both of them use the State to calculate so I would move that logic to the State initialization, so then it's not HCL nor Plan but common place, and also it's where the actual calculations are made so it would make sense to be there.

Also as the query returns s []query you could calculate them from different files and then aggregate all the queries into 1 State which would validate if it's empty or not.

xlr-8 · 2021-08-13T09:36:24Z

Done, pipeline has been fixed too to work with the dump.

xescugc

rs

xlr-8 · 2021-08-20T07:26:22Z

Done

xescugc · 2021-08-20T13:46:22Z

estimation.go

@@ -27,7 +27,7 @@ func EstimateTerraformPlan(ctx context.Context, backend Backend, plan io.Reader,
 	}

 	priorQueries, err := tfplan.ExtractPriorQueries()
-	if err != nil {
+	if err != nil && err != terraform.ErrNoQueries {


This is not needed no?

No, AFAIK this is needed in case there are no prior queries.

But the ExtractPriorQueries doeds not return this error anymore, this was useful before but now it's not.

Indeed, done

xlr-8 · 2021-08-20T15:14:05Z

I've updated the Makefile/docker-compose file to ease the testing locally/in the CI

xescugc

RS + merge

The testdata has been re-arranged to better reflect which providers where being tested; this allows to deal more easily with tests and the providers which are not yet supported. Some SQL dump of a previous ingestion has also been added to be able to test things locally more easily, it will be injected into the database whenever this one is started. The database can now be access via `make db-cli`

Instead of returning 0 as a cost estimated, Terracost will now return an error depending on the condition: no known providers were found, so it had no data to estimate it, or when a terraform module/files is empty and nothing could be extracted.

The known providers are now put into a separated files, which helps with adding new supported one to the default list.

xlr-8 self-assigned this Aug 12, 2021

xlr-8 added the need review label Aug 12, 2021

xlr-8 force-pushed the hr-improve-error branch 2 times, most recently from d456e9d to aea48c5 Compare August 12, 2021 12:36

xescugc requested changes Aug 12, 2021

View reviewed changes

xlr-8 removed the need review label Aug 12, 2021

xlr-8 added the need review label Aug 13, 2021

xescugc reviewed Aug 17, 2021

View reviewed changes

xlr-8 force-pushed the hr-improve-error branch from 2775c12 to db5159d Compare August 20, 2021 07:26

xescugc reviewed Aug 20, 2021

View reviewed changes

xescugc previously approved these changes Aug 30, 2021

View reviewed changes

Hugo Rosnet added 3 commits August 30, 2021 16:18

provider: rearrange files to ease reading/addition

dcca7a5

The known providers are now put into a separated files, which helps with adding new supported one to the default list.

xlr-8 dismissed xescugc’s stale review via dcca7a5 August 30, 2021 14:19

xlr-8 force-pushed the hr-improve-error branch from a9b1203 to dcca7a5 Compare August 30, 2021 14:19

xlr-8 merged commit ddbcd17 into master Aug 30, 2021

xlr-8 mentioned this pull request Aug 31, 2021

estimation: unexpected error if 2 providers (supported/unsupported) are mixed together #56

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

testing: add handling of unknown providers and empty terraform #55

testing: add handling of unknown providers and empty terraform #55

xlr-8 commented Aug 12, 2021 •

edited

Loading

xescugc left a comment

xescugc Aug 12, 2021

xescugc Aug 12, 2021

xescugc Aug 12, 2021

xlr-8 Aug 12, 2021

xlr-8 commented Aug 12, 2021

xlr-8 commented Aug 12, 2021

xescugc commented Aug 13, 2021

xlr-8 commented Aug 13, 2021

xescugc left a comment

xlr-8 commented Aug 20, 2021

xescugc Aug 20, 2021

xlr-8 Aug 20, 2021

xescugc Aug 23, 2021

xlr-8 Aug 30, 2021

xlr-8 commented Aug 20, 2021

xescugc left a comment •

edited

Loading

		ErrNoQueries = fmt.Errorf("no terraform entities found, looks empty")
		ErrNoKnownProvider = fmt.Errorf("terraform providers are not yet supported")

testing: add handling of unknown providers and empty terraform #55

testing: add handling of unknown providers and empty terraform #55

Conversation

xlr-8 commented Aug 12, 2021 • edited Loading

xescugc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xlr-8 commented Aug 12, 2021

xlr-8 commented Aug 12, 2021

xescugc commented Aug 13, 2021

xlr-8 commented Aug 13, 2021

xescugc left a comment

Choose a reason for hiding this comment

xlr-8 commented Aug 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xlr-8 commented Aug 20, 2021

xescugc left a comment • edited Loading

Choose a reason for hiding this comment

xlr-8 commented Aug 12, 2021 •

edited

Loading

xescugc left a comment •

edited

Loading