AWS estimation #9

patrislav · 2020-11-09T07:42:15Z

This PR adds the functionality to estimate Terraform plan costs using the pricing data stored in the DB.

It adds the following packages:

terraform - contains the logic of reading a Terraform plan and generating a list of query.Components found in there. This is done by passing Terraform resource values to registered providers (e.g. aws/terraform package) that fetch the pricing using a passed Backend
aws/terraform - contains rules for transforming terraform.Resources into query.Components (only aws_instance and aws_ebs_volume in this PR)
cost - calculate costs of a State (collection of resources) and a Plan (combination of a "prior" and "planned" State)

Usage to calculate prior and planned costs of every resource in a plan:

db, err := sql.Open("mysql", "...")
backend := mysql.NewBackend(db)
file, err := os.Open("plan.json")
plan, err := costestimation.EstimateTerraformPlan(ctx, backend, file)

for _, res := range plan.ResourceDifferences() {
	fmt.Println("Resource `%s`: prior ( %s ), planned ( %s )", res.Address, res.PriorCost(), res.PlannedCost())
}

xescugc

So a few things:

Does TF have a way to read Plans? I say this just for the sake of maintainability, they change the API quite often and if you have to check it one by one it'll be a nightmare, so if they have already an implementation maybe would be good to use it (IDK if they do though, or if it's usable).
I see that on the Provider.New* you make a manual assignation, tenancyVal, _ := values["tenancy"].(string) and then assign and then validate, could be better to just json.Unmarshal it directly, so you don't have to care about empty/casting errors.
Also on the same line of TF, I would try to get a more up to date State as we are already on TF 0.13.5 and things may have changed on the way there.

xescugc · 2020-11-10T09:23:40Z

terraform/plan.go

+// NewPlan returns an empty Plan.
+func NewPlan(opts ...Option) *Plan {
+	plan := &Plan{providerInitializers: make(map[string]ProviderInitializer)}
+	for _, opt := range opts {
+		opt(plan)
+	}
+	return plan
+}


A Plan without providerInitializers is useless no? I'm saying this because you should validate it, and also if the only configuration is the providerInitializers then maybe it's better to have it as a (pi ...ProviderInitializer) and have the ProviderInitializer have a pi.Key/Name/Type which is basically the name.

I would not let the user do a WithProvider("aw", aws.PI) as the aws.PI will ALWAYS need to have the "aws" no?

The reason was that there might be multiple names for the same provider implementation, e.g. google and google-beta and also that the ProviderInitializer was just a type alias for a func, so it cannot provide its name. However, I see now that it would be indeed better to have it as (pi ...ProviderInitializer) with ProviderInitializer being a struct with a slice of provider names that will match it.

xescugc · 2020-11-10T09:28:15Z

terraform/plan.go

+func (p *Plan) Read(r io.Reader) error {
+	if err := json.NewDecoder(r).Decode(p); err != nil {
+		return err
+	}
+	return nil
+}


Does TF have a way to load Plans?

I'm saying this because if they change formats on the future you'll not know and will have to manually maintain it, but if they have it they maintain it.

As mentioned in the other comment, they have but it's unexported 😅

xescugc · 2020-11-10T09:30:58Z

terraform/plan.go

+	return p.extractQueries(p.PriorState.Values, providers)
+}
+
+func (p *Plan) extractProviders() map[string]Provider {


xescugc · 2020-11-10T09:31:05Z

terraform/plan.go

+	return providers
+}
+
+func (p *Plan) extractQueries(modules map[string]Module, providers map[string]Provider) []query.Resource {


xescugc · 2020-11-10T09:31:46Z

terraform/provider.go

+	// ProviderName returns the name of the Provider.
+	ProviderName() string
+


I think that Provierd.ProviderName is a bit redundant, should just be Name.

xescugc · 2020-11-10T09:40:44Z

aws/terraform/instance.go

+// NewInstance creates a new Instance from Terraform values.
+func (p *Provider) NewInstance(values map[string]interface{}) *Instance {
+	instType, _ := values["instance_type"].(string)
+	tenancyVal, _ := values["tenancy"].(string)


This is "optional" on the tfstate, If it's not defined it may cause a panic which it's not dealt with.

AFAIK if the double-value-return form of the type assertion is used (foo, ok := bar.(string)) then it won't cause a panic but instead the ok will be false. In this case, we don't care about the ok, but rely on the variable to hold the zero-value in case of an assertion failure.

xescugc · 2020-11-10T09:40:50Z

aws/terraform/instance.go

+func (p *Provider) NewInstance(values map[string]interface{}) *Instance {
+	instType, _ := values["instance_type"].(string)
+	tenancyVal, _ := values["tenancy"].(string)
+	zone, _ := values["availability_zone"].(string)


This is "optional" on the tfstate, If it's not defined it may cause a panic which it's not dealt with.

xescugc · 2020-11-10T09:43:29Z

aws/terraform/provider.go

+// Provider is an implementation of the terraform.Provider, used to extract component queries from
+// terraform resources.
+type Provider struct {
+	name   string
+	region string
+}
+
+// NewProvider returns a new Provider.
+func NewProvider(name string, region string) *Provider {
+	return &Provider{name: name, region: region}
+}


What is the name? This is inside aws/ already, what do you expect as name?

I think those 2 have to be validated, as they are used afterwards as if they are always set, for example the region is always set based on this Provider.region.

Yep, the name will not be needed anymore (as it will be provided by the ProviderInitializer) and I will validate the region to match the list of regions that we already have.

xescugc · 2020-11-10T09:45:53Z

aws/terraform/volume.go

+	volType, _ := values["type"].(string)
+	volSize, _ := values["size"].(float64)
+	volIops, _ := values["iops"].(float64)


All this 3 are optional.

The volume type too?

Yes, it's "gp2" by default

xescugc · 2020-11-10T09:56:02Z

e2e/testdata/terraform-plan.json

+{
+  "format_version": "0.1",


Is this the same plan you added before? If so just use the same one :)

I wanted to keep them separate as they belong to two different test suites (e2e/testdata and terraform/testdata). But I suppose this can be moved to a testdata dir at the root and be used by both? 🤔

The question is more: are those two likely to evolve differently, if the answer is no then let's merge them, if so then let's keep them separated.

I don't think they'll evolve differently, so putting them in a single place might make more sense

patrislav · 2020-11-10T11:49:39Z

Does TF have a way to read Plans? I say this just for the sake of maintainability, they change the API quite often and if you have to check it one by one it'll be a nightmare, so if they have already an implementation maybe would be good to use it (IDK if they do though, or if it's usable).

Unfortunately not. They define a plan JSON format here but the plan struct and all related functions are unexported (at least not internal 🙄 )

I see that on the Provider.New* you make a manual assignation, tenancyVal, _ := values["tenancy"].(string) and then assign and then validate, could be better to just json.Unmarshal it directly, so you don't have to care about empty/casting errors.

Do you mean having a struct into which the JSON would be unmarshalled? I don't think his would work as at the time of the unmarshal we don't know what the resource is or even to which provider it belongs. So we need to unmarshal each resource into a map[string]interface{} and retrieve the properties dynamically.

Also on the same line of TF, I would try to get a more up to date State as we are already on TF 0.13.5 and things may have changed on the way there.

It seems that the JSON representation of the plan is still on version 0.1 and haven't changed yet.

xlr-8

Various comments done, I think an important part will be the documentation associated with the implementation to make sure it's understandable and thorough regarding what it can/cannot do, whys, etc.

aws/region/code.go

xlr-8 · 2020-11-11T07:37:51Z

aws/terraform/instance.go

+
+	operatingSystem string
+	capacityStatus  string
+	preInstalledSW  string


While understanding the other, I'm having trouble knowing what this is for - perhaps some comments would help on those fields for people less familiar with AWS/Terraform?

xlr-8 · 2020-11-11T07:46:09Z

aws/region/code.go

+}
+
+// Valid returns true if the region exists and is supported, false otherwise.
+func (c Code) Valid() bool {


As Code can also be "" it might be worth checking that ahead of iterating?

xlr-8 · 2020-11-11T07:49:28Z

aws/terraform/instance.go

+	}
+	if volParams == nil {
+		// Default EBS Volume values
+		volParams = map[string]interface{}{"availability_zone": zone}


This can be the initialization then, removing the need to check for volParams == nil as it would be override afterwards if need be

aws/terraform/instance.go

xlr-8 · 2020-11-11T08:08:41Z

aws/terraform_provider.go

+	"github.com/cycloidio/cost-estimation/terraform"
+)
+
+var TerraformQueryKey = "aws"


Not sure about the compiler behaviour, but can be directly a const

Yeah, it's var so that I can override it for the tests, it seemed to be the simplest way

Why do we need this?

IDK why it should be overrided from the tests, this is aws/ pkg already this should not change no?

Yes, but I override it in e2e so that it doesn't conflict with any entries that had been already ingested before. E.g. if real AWS pricing data was ingested, the test might fetch it, breaking the result.

I don't quite like this, doing something due to tests, maybe the code is not right then, should this be a paramter then?

And for what I see, the arguemtn is that the e2e test may break actual exported data? That is like saying that you have a constant on the code for the DB and you have to change it if not you remove production data, then that constant it's a variable I think.

If it's a parameter, it would never be used by real consumers, since they'd always use only the aws value so it'd clutter the public API, I believe. Yes, it could break real data since the test could possibly add an entry with the same properties as a real entry, as well as real data could possibly break a test (if only 1 entry is expected, but there is another, "real" one.) The estimation might give wrong results if two SKU's are found with the same properties.

So I'm not sure what'd be the best way to do it. I could add a QueryKey field to the ProviderInitializer, for example, and then have the TerraformProviderInitializer.QueryKey to be changed from the test (like the MatchNames field already is) but that just moves the problem into another global variable (the global singleton TerraformProviderInitializer struct.)

Couldn't we make it another singleton or something? Initialise it with a certain value if provided else fallback to this one?

Do you mean a separate struct that holds the query key as a field? 🤔 I believe that would just move the problem into a struct, instead of dealing with it, usage would stay the same as with a global variable. Or did I misunderstand what you mean?

xlr-8 · 2020-11-11T08:50:16Z

cost/state.go

+}
+
+// State represents a collection of all the Resource costs (either prior or planned.)
+type State struct {


I assume this is supposed to be a tfstate? No more tf way to get it / exported from TF?

This is not a TF state, but rather an intermediate representation of a state of resources. This struct could possibly be generated by using tools different than TF (if we get to that in the future.) A State here is like a snapshot (could this be a better name?) of resources at a given time, and two states (prior and planned) form a Plan that can be estimated.

Yeah, I'm not sure whats the best wording for that. Perhaps improving slightly the comments would do?

xlr-8 · 2020-11-11T08:50:52Z

cost/state.go

+			rate := prices[0].Value
+			if quantity.Equals(decimal.Zero) {
+				quantity = comp.HourlyQuantity
+				rate = rate.Mul(decimal.NewFromInt(730))


Number of hours in a month, to convert an hourly rate to a monthly one, will add a doc

e2e/aws_estimation_test.go

xlr-8 · 2020-11-11T09:05:22Z

e2e/testdata/terraform-plan.json

+{
+  "format_version": "0.1",


The question is more: are those two likely to evolve differently, if the answer is no then let's merge them, if so then let's keep them separated.

patrislav · 2020-11-11T14:46:26Z

Pushed fixups for most of the comments (except the ones awaiting feedback)

xescugc · 2020-11-16T10:17:01Z

Do you mean having a struct into which the JSON would be unmarshalled? I don't think his would work as at the time of the unmarshal we don't know what the resource is or even to which provider it belongs. So we need to unmarshal each resource into a map[string]interface{} and retrieve the properties dynamically.

Ok I meant this https://github.com/mitchellh/mapstructure

It seems that the JSON representation of the plan is still on version 0.1 and haven't changed yet.

I can tell you that they have changed it some things on the State, as I had to make changes for version

I'll take a look to the new commits.

xescugc

Some general things but I would also try to use golint to solve some of the missing comments. Also there are some missing tests :)

aws/region/code.go

xescugc · 2020-11-16T11:10:40Z

aws/terraform_provider.go

+	"github.com/cycloidio/cost-estimation/terraform"
+)
+
+var TerraformQueryKey = "aws"


Why do we need this?

IDK why it should be overrided from the tests, this is aws/ pkg already this should not change no?

xescugc · 2020-11-16T11:12:26Z

terraform/plan.go

 	"io"

 	"github.com/cycloidio/cost-estimation/query"
 )

 // Plan is a representation of a Terraform plan file.
 type Plan struct {
-	providerInitializers map[string]ProviderInitializer
+	providerInitializers []ProviderInitializer


Nah, IMO the ProviderInitializer should have a whay for you to know the Provider.

So then you can have a map[string]ProviderInitializer where the key is the Provider.Type/Name

It makes things much more easy on the rest of the code.

The reason was that a single provider might match multiple names (such as google and google-beta, for example), so every ProviderInitializer has a MatchNames slice instead. Could it be better to have the same initializer under multiple keys in the map[string]ProviderInitializer?

Yes, so then it's an easy acces and you can also validate that 2 names are not colliding.

terraform/provider.go

patrislav · 2020-11-17T15:57:33Z

Pushed fixups addressing all the comments.

Ok I meant this https://github.com/mitchellh/mapstructure

Updated to use this library, it's much better now, thanks.

I can tell you that they have changed it some things on the State, as I had to make changes for version

Do you mean the JSON output format? 🤔 I will check if there are any differences.

Some general things but I would also try to use golint to solve some of the missing comments. Also there are some missing tests :)

I've run golint after the fixups and all of the files added by this PR pass now. There are still issues with files that already existed but these can be dealt with in a dedicated PR.

xescugc · 2020-11-19T09:24:23Z

Do you mean the JSON output format? thinking I will check if there are any differences.

they changed some things on the State, I found it on my tests for TC update to TF 0.13.5. IDK if the plan has changed though. But not much we can do if they do not export it.

Commented so some of the things but now looks better

patrislav · 2020-11-26T13:55:33Z

Is this OK to merge now or does it require some more work?

they changed some things on the State, I found it on my tests for TC update to TF 0.13.5. IDK if the plan has changed though. But not much we can do if they do not export it.

I checked and apparently the JSON structure hasn't changed for 2 years, so it should be safe 😄

xlr-8 · 2020-11-27T09:20:33Z

Taking a look now

xlr-8

Only minor things remaining, but the documentation to come will have to be really thorough to document everything commented in here, the current scope and limitation of our tool (services, type within the service, etc).

aws/terraform/instance.go

xlr-8 · 2020-11-27T09:26:46Z

aws/terraform_provider.go

+	"github.com/cycloidio/cost-estimation/terraform"
+)
+
+var TerraformQueryKey = "aws"


Couldn't we make it another singleton or something? Initialise it with a certain value if provided else fallback to this one?

xescugc · 2020-12-03T10:33:05Z

LGTM RS

patrislav · 2020-12-03T10:40:11Z

Rebased

xescugc

Overall I think it's all fine, just this small ones.

I'll check after this is merge more in the global structure/architecture, but I think it's fine.

xescugc · 2020-12-04T15:26:34Z

terraform/plan.go

+func NewPlan(providerInitializers ...ProviderInitializer) *Plan {
+	piMap := make(map[string]ProviderInitializer)
+	for _, pi := range providerInitializers {
+		for _, name := range pi.MatchNames {
+			piMap[name] = pi
+		}
+	}
+	plan := &Plan{providerInitializers: piMap}
+	return plan
+}


Would make sense to validat len(providerInitializers)?

Not exactly sure - I wouldn't say that it's an "error" to have no providers configured. The Plan would always return a zero estimate, but that's expected (no input equals no output.) Also, this is a lower-level struct and func that is not supposed to be used by library consumers - the EstimateTerraformPlan func is and it validates providerInitializers. Does it make sense?

xescugc · 2020-12-04T15:34:54Z

aws/region/code.go

+func (c Code) Valid() bool {
+	if c == "" {
+		return false
+	}
+	for _, code := range regionMap {
+		if c == code {
+			return true
+		}
+	}
+	return false
+}
+


I think that to make things "easier" I would call on func init(){} and generate a reverse map of the regionMap so we can search for both ways easy.

xescugc · 2020-12-04T15:35:34Z

aws/terraform/instance.go

+	// capacityStatus describes the status of capacity reservations.
+	// Valid values include: Used, UnusedCapacityReservation, AllocatedCapacityReservation.
+	// Note: only "Used" is supported at the moment.
+	capacityStatus string // valid values: Used, UnusedCapacityReservation, AllocatedCapacityReservation


I think we can remove this comment as it's already on the docs.

xescugc · 2020-12-04T15:37:57Z

aws/terraform_provider.go

+
+// TerraformProviderInitializer is a terraform.ProviderInitializer that initializes the default AWS provider.
+var TerraformProviderInitializer = terraform.ProviderInitializer{
+	MatchNames: []string{"aws"},


Why not TerraformQueryKey?

The reason was that the names from MatchNames and from TerraformQueryKey are actually two different things technically: TerraformQueryKey is the name under which the pricing data was saved in the DB, while MatchNames includes a list of names that Terraform recognizes. These may be different (e.g. for GCP, there are two providers: google and google-beta, while there'd be only one query key).

However, in the case of AWS they're all the same, including the fact that TerraformQueryKey is a duplicate of the existing ProviderName 😅 - so I will use that in all cases.

Ok, yes Azure also have a few providers.

xescugc · 2020-12-04T15:40:22Z

cost/plan.go

+	return rds
+}
+
+func mergeResourceDiffsFromState(rdmap map[string]ResourceDiff, state *State, planned bool) {


xescugc · 2020-12-04T15:43:30Z

estimation.go

+// EstimateTerraformPlan is a helper function that reads a Terraform plan using the provided io.Reader,
+// generates the prior and planned cost.State, and then creates a cost.Plan from them that is returned.
+// It uses the Backend to retrieve the pricing data.
+func EstimateTerraformPlan(ctx context.Context, backend Backend, r io.Reader, providerInitializers ...terraform.ProviderInitializer) (*cost.Plan, error) {


Maybe for clarity I would s/r io.Reader/ plan io.Reader/ having it just be r it's not clear, it requires you to actually read the docs or it's not possible, just changing that I can "easily" know what it needs.

xlr-8

Mostly minor changes on test cases. The rest looks good to me!

xlr-8 · 2020-12-07T08:20:54Z

terraform/plan_test.go

+	queries, err := plan.ExtractPriorQueries()
+	require.NoError(t, err)
+	require.Len(t, queries, 1)
+}


I'd add cases for failures too, just to ensure an error stops the execution / is returned

xlr-8 · 2020-12-07T08:29:06Z

cost/state.go

+	return total
+}
+
+func (s *State) addComponent(resAddress, compLabel string, component Component) {


xlr-8 · 2020-12-07T08:33:57Z

cost/state_test.go

+
+	state, err := cost.NewState(ctx, backend, queries)
+	require.NoError(t, err)
+	assert.Equal(t, expected, state)


Add cases of failures too

xlr-8 · 2020-12-07T08:37:37Z

e2e/aws_estimation_test.go

+	rootVol := diffs[0].ComponentDiffs["Root volume: Storage"]
+	require.NotNil(t, rootVol)
+	assertDecimalEqual(t, decimal.NewFromFloat(3.6), rootVol.PriorCost())
+	assertDecimalEqual(t, decimal.NewFromFloat(3.6), rootVol.PlannedCost())


Cases of failures too, doesn't have to make the other calls fail voluntarily but just ensuring all errors are handled

patrislav · 2020-12-07T14:58:01Z

Pushed fixups that should resolve all the comments

xescugc · 2020-12-08T11:48:55Z

LGTM RS when @xlr-8 says

xlr-8 · 2020-12-09T11:06:50Z

Seems like the tests of failure cases were added, were they?

The purpose of the terraform package is to read a Terraform plan file and extract query.Resource records. These will be subsequently used to estimate the plan cost.

Define a Provider that implements the terraform.Provider interface and provide a NewTerraformProvider func that matches the signature of the terraform.ProviderInitializer type. This allows it to be used as one of the providers during plan reading process. The Provider returns a query.Component slice for each terraform.Resource that uses the Provider.

The purpose of the cost package is to retrieve pricing data for a particular State (list of resources) and to merge two States to achieve a Plan - containing cost differences for all resources included in both or either of the states.

patrislav · 2020-12-09T11:41:33Z

Rebased

xescugc

LGTM :)

query: Add Resource and Component

1988031

patrislav self-assigned this Nov 9, 2020

patrislav mentioned this pull request Nov 9, 2020

AWS cost estimation #6

Closed

xescugc requested changes Nov 10, 2020

View reviewed changes

xlr-8 suggested changes Nov 11, 2020

View reviewed changes

xescugc requested changes Nov 16, 2020

View reviewed changes

xlr-8 suggested changes Nov 27, 2020

View reviewed changes

patrislav force-pushed the pk-aws-estimation branch from 9b38835 to a5a051e Compare December 3, 2020 10:39

xescugc requested changes Dec 4, 2020

View reviewed changes

xlr-8 suggested changes Dec 7, 2020

View reviewed changes

Patryk Kalinowski added 8 commits December 9, 2020 14:30

terraform: plan schema and resource extraction

a565c76

The purpose of the terraform package is to read a Terraform plan file and extract query.Resource records. These will be subsequently used to estimate the plan cost.

tests(aws): add tests for aws_instance and aws_ebs_volume

381ae49

tests(cost): add tests for creating State and cost calculation

be23111

costestimation: EstimateTerraformPlan function

dd82554

mock: add TerraformProvider mock

95f9e2d

e2e: add AWS estimation E2E tests, using a real plan and database

e19a511

go.mod: add mapstructure dependency

fe8303b

patrislav force-pushed the pk-aws-estimation branch from ea6b5aa to fe8303b Compare December 9, 2020 11:40

xlr-8 approved these changes Dec 9, 2020

View reviewed changes

xescugc approved these changes Dec 9, 2020

View reviewed changes

patrislav merged commit d50eb11 into master Dec 10, 2020

patrislav deleted the pk-aws-estimation branch December 10, 2020 07:59

		// ProviderName returns the name of the Provider.
		ProviderName() string

AWS estimation #9

AWS estimation #9

Conversation

patrislav commented Nov 9, 2020

xescugc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrislav Nov 10, 2020 • edited Loading

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrislav commented Nov 10, 2020

xlr-8 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrislav commented Nov 11, 2020

xescugc commented Nov 16, 2020

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

patrislav commented Nov 17, 2020

xescugc commented Nov 19, 2020

patrislav commented Nov 26, 2020

xlr-8 commented Nov 27, 2020

xlr-8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xescugc commented Dec 3, 2020

patrislav commented Dec 3, 2020

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

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 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

patrislav Nov 10, 2020 •

edited

Loading