feat(aws): Initial sketch of cloudformation support #803
Conversation
|
I was looking into how to support cloudformation. The basic idea is to split resource creation into I think the biggest thing to figure out for multi-provider support is the usage .yml file. The current support is essentially terraform specific because the usage keys map directly to a terraform resource block. One option for multi-provider support would be to keep the current format but create a mapping from the current usage keys to CF resources. This may not be straightforward because there is not a 1-1 correlation between CF and TF resources. E.g. CF has separate Another option that might feel more natural for CF users would be to let them specify usage in a similar structure as the CF template. So given a template resource: The usage parameters could be specified as If we want to go this route, it probably means adding more structure to the usage parameters so the files can be parsed and generated easily in whatever format is needed. |
|
There are additional cloudformation specific issues that need to be figured out:
|
|
Thanks @tim775! Still going through this, but will leave initial thoughts/comments.
This is cool. I can see how in the future
One thing we had in the past was a Terraform provider so users could specify the usage data in HCL code, so it was similar to the TF they were writing. We found that nobody used it, but not sure if that was due to it being HCL or the fact it had to be bundled in their existing TF project. Totally making an assumption here, but I'm wondering if more people write their CF in YAML if they're handwriting it, and maybe JSON is used more if they're generated it. Would this be how you'd see option 1 looking? version: 0.1
resource_usage:
# TF resource
aws_dynamodb_table.my_table:
monthly_write_request_units: 3000000
monthly_read_request_units: 8000000
# CF resource (AWS::DynamoDB::Table)
AlertsTable:
monthly_write_request_units: 3000000
monthly_read_request_units: 8000000
# CF resource (AWS::DynamoDB::GlobalTable)
GlobalAlertsTable:
monthly_write_request_units: 3000000
monthly_read_request_units: 8000000 |
So NO that wasn't what I was thinking, but I like that better 🙂. I guess that really simplifies things because we're still just using the natural 'address' which for TF is . but for CF is just . That gets my vote. |
We might need to do this for TF as well, since sometimes it's impossible to detect from the plan file. We could have a usage file key for it, or maybe an environment variable like the AWS cli has.
I guess we could use func mapTags(d interface{}) map[string]string {
r := reflect.ValueOf(d)
f := reflect.Indirect(r).FieldByName("Tags")
mapped := make(map[string]string)
if f.IsValid() {
tags, ok := f.Interface().([]Tag)
if ok {
for _, tag := range tags {
mapped[tag.Key] = tag.Value
}
}
}
return mapped
} |
|
Not sure if |
internal/providers/detect.go
Outdated
| return false | ||
| } | ||
|
|
||
| return jsonFormat.AWSTemplateFormatVersion != "" && jsonFormat.Resources != nil |
There was a problem hiding this comment.
AWSTemplateFormatVersion is optional. We have some more CloudFormation template recognition logic here if you want to borrow ideas
similar discussions: aws-cloudformation/cfn-lint#1171, aws-cloudformation/cfn-lint-visual-studio-code#99, aws/aws-toolkit-jetbrains#1715 (comment), https://github.com/github/super-linter/pull/231#discussion_r445708695, mwpearce/vscode-cfn-nag#6, joenye/coc-cfn-lint#3 (comment), aws-cloudformation/cfn-lint-atom#29, github-linguist/linguist#2987
| ReplicaRegions []string `json:"replicaRegions,omitempty"` | ||
| } | ||
|
|
||
| func NewDynamoDBTable(args *DynamoDbTableArguments, u *schema.UsageData) *schema.Resource { |
There was a problem hiding this comment.
@tim775 Just leaving my comment here too. I believe the Resource Abstraction should not care about what fields come from what file. So all it takes is a bunch of mandatory and optional fields. The caller of this function then can decide how to construct it.
If I want to make an example is let's say we have the resource X with fields of A, B and C. In terraform, fields A and B are present in the state file and field C comes from usage data. But, there's no guarantee that all providers are like terraform. Another provider like Pulumi might have support for field C in its own config and state and here's where things get super complicated if we take two different parameters.
| MonthlyWriteRequestUnits *int64 `json:"monthlyWriteRequestUnits,omitempty"` | ||
| MonthlyReadRequestUnits *int64 `json:"monthlyReadRequestUnits,omitempty"` | ||
| StorageGB *int64 `json:"storageGB,omitempty"` | ||
| PitrBackupStorageGB *int64 `json:"pitrBackupStorageGB,omitempty"` | ||
| OnDemandBackupStorageGB *int64 `json:"onDemandBackupStorageGB,omitempty"` | ||
| MonthlyDataRestoredGB *int64 `json:"monthlyDataRestoredGB,omitempty"` | ||
| MonthlyStreamsReadRequestUnits *int64 `json:"monthlyStreamsReadRequestUnits,omitempty"` |
There was a problem hiding this comment.
Could we add the fields that @sinabakh mentioned in his usage-file schema comment here so we have 1 source of truth for them? That source could then be used in the future by --sync-usage-file and docs.
There was a problem hiding this comment.
Yes, the usage schema for the resource is defined separately from the args a few lines below:
https://github.com/infracost/infracost/pull/803/files/ded0f66110486cb9aeffb5992fe7223aec699ba5#diff-71ed323ee96294a7b798471f490d0ecde86fa1b63763f1e023d4bf93e40dc21eR38-R46
and it's added to the resource created by NewDynamoDBTable. (This is a little bit different than adding it to the RegistryItem which is what @sinabakh had proposed).
I made changes to sync-usage-file so it will use this resource.UsageSchema if present, or fall back to the "old way" of using the usage-example.
internal/schema/usage_data.go
Outdated
| @@ -30,6 +50,15 @@ func (u *UsageData) Get(key string) gjson.Result { | |||
| return u.Attributes[key] | |||
| } | |||
|
|
|||
| func (u *UsageData) GetInt(key string) *int64 { | |||
There was a problem hiding this comment.
I like this, a lot cleaner than checking for gjson.Null everywhere.
|
I extracted the non-cloudformation stuff to #824 |
|
Going with the philosophy of putting stuff out there to get feedback, I think we should merge this since it doesn't affect anything in the core Terraform functionality. @tim775 can you rebase this to pick up the Float64 changes and I will merge? |
|
@aliscott I rebased and this is ready. I added the goformation package. Is it typical to see all the |
@tim775 not usually this many. Not sure why it's added these new lines but when I run |
No description provided.