Control Tower Landing Zone manifest retentionDays unmarshalled into string instead of number #2659
Comments
markwellis
added
bug
|
The use of the For whatever reason the
If you actually use I can't really speak to these terraform issues. Perhaps they're doing something wrong in regards to this field, but that would surprise me -- the value you're showing through (for this example I just mocked the wire response instead of creating one of these landing zones, which I know nothing about and it seemed involved/impossible for me without additional setup): package main
import (
"context"
"io"
"log"
"net/http"
"strings"
"github.com/aws/aws-sdk-go-v2/aws"
"github.com/aws/aws-sdk-go-v2/config"
"github.com/aws/aws-sdk-go-v2/service/controltower"
)
const lz = `
{
"landingZone": {
"arn": "$landing_zone_arn",
"driftStatus": {
"status": "IN_SYNC"
},
"latestAvailableVersion": "3.3",
"manifest": {
"accessManagement": {
"enabled": false
},
"securityRoles": {
"accountId": "$account_id"
},
"governedRegions": [
"eu-west-1",
"eu-central-1",
"us-east-1",
"us-west-2"
],
"organizationStructure": {
"security": {
"name": "Security"
}
},
"centralizedLogging": {
"accountId": "$account_id",
"configurations": {
"loggingBucket": {
"retentionDays": 90
},
"kmsKeyArn": "$kms_arn",
"accessLoggingBucket": {
"retentionDays": 90
}
},
"enabled": true
}
},
"status": "ACTIVE",
"version": "3.3"
}
}
`
type mockHTTP struct{}
func (mockHTTP) Do(r *http.Request) (*http.Response, error) {
return &http.Response{
StatusCode: http.StatusOK,
Body: io.NopCloser(strings.NewReader(lz)),
}, nil
}
type Manifest struct {
// omitting stuff we don't care about in this example
CentralizedLogging struct {
Configurations struct {
LoggingBucket struct {
RetentionDays int
}
}
}
}
func main() {
cfg, err := config.LoadDefaultConfig(context.Background(), config.WithClientLogMode(aws.LogResponseWithBody))
if err != nil {
log.Fatal(err)
}
client := controltower.NewFromConfig(cfg)
input := &controltower.GetLandingZoneInput{
LandingZoneIdentifier: aws.String("foo"),
}
output, err := client.GetLandingZone(context.Background(), input, func(o *controltower.Options) {
o.HTTPClient = mockHTTP{}
})
if err != nil {
log.Fatal(err)
}
var manifest Manifest
if err := output.LandingZone.Manifest.UnmarshalSmithyDocument(&manifest); err != nil {
panic(err)
}
log.Printf("\n\nsmithy document: %#v", output.LandingZone.Manifest)
println("retention days", manifest.CentralizedLogging.Configurations.LoggingBucket.RetentionDays)
} |
lucix-aws
added
response-requested
|
That makes sense, but but doesn't explain the issue with terraform, which is where this issue is causing me problems! The terraform provider reads the aws api, using the sdk, which returns it as a smithy document, then it re-encodes it back to a json string. I have adapted your example above to do what the terraform provider does, and you can see in the output both Given this is just round tripping from json string -> smithy document -> json string, shouldn't numbers be preserved as numbers and not turned to strings? package main
import (
"context"
"encoding/json"
smithydocument "github.com/aws/smithy-go/document"
"io"
"log"
"net/http"
"strings"
"github.com/aws/aws-sdk-go-v2/aws"
"github.com/aws/aws-sdk-go-v2/config"
"github.com/aws/aws-sdk-go-v2/service/controltower"
)
const lz = `
{
"landingZone": {
"arn": "$landing_zone_arn",
"driftStatus": {
"status": "IN_SYNC"
},
"latestAvailableVersion": "3.3",
"manifest": {
"accessManagement": {
"enabled": false
},
"securityRoles": {
"accountId": "$account_id"
},
"governedRegions": [
"eu-west-1",
"eu-central-1",
"us-east-1",
"us-west-2"
],
"organizationStructure": {
"security": {
"name": "Security"
}
},
"centralizedLogging": {
"accountId": "$account_id",
"configurations": {
"loggingBucket": {
"retentionDays": 90
},
"kmsKeyArn": "$kms_arn",
"accessLoggingBucket": {
"retentionDays": 90
}
},
"enabled": true
}
},
"status": "ACTIVE",
"version": "3.3"
}
}
`
type mockHTTP struct{}
func (mockHTTP) Do(r *http.Request) (*http.Response, error) {
return &http.Response{
StatusCode: http.StatusOK,
Body: io.NopCloser(strings.NewReader(lz)),
}, nil
}
// from https://github.com/hashicorp/terraform-provider-aws/blob/main/internal/json/smithy.go#L25
func SmithyDocumentToString(document smithydocument.Unmarshaler) (string, error) {
var v map[string]interface{}
err := document.UnmarshalSmithyDocument(&v)
if err != nil {
return "", err
}
bytes, err := json.Marshal(v)
if err != nil {
return "", err
}
return string(bytes), nil
}
func main() {
cfg, err := config.LoadDefaultConfig(context.Background(), config.WithClientLogMode(aws.LogResponseWithBody))
if err != nil {
log.Fatal(err)
}
client := controltower.NewFromConfig(cfg)
input := &controltower.GetLandingZoneInput{
LandingZoneIdentifier: aws.String("foo"),
}
output, err := client.GetLandingZone(context.Background(), input, func(o *controltower.Options) {
o.HTTPClient = mockHTTP{}
})
if err != nil {
log.Fatal(err)
}
//from https://github.com/hashicorp/terraform-provider-aws/blob/main/internal/service/controltower/landing_zone.go#L157
v, _ := SmithyDocumentToString(output.LandingZone.Manifest)
println("as json ", v)
}output
Thanks |
|
You've identified exactly where the disconnect occurs there. Numeric values (so, ones stored in the raw document type Manifest struct {
CentralizedLogging struct {
Configurations struct {
LoggingBucket struct {
RetentionDays interface{} // change this from int -> interface{}
}
}
}
}
func main() {
// ...
var manifest Manifest
if err := output.LandingZone.Manifest.UnmarshalSmithyDocument(&manifest); err != nil {
panic(err)
}
// %T shows us the type
fmt.Printf("RetentionDays is %T\n", manifest.CentralizedLogging.Configurations.LoggingBucket.RetentionDays)
}prints json.Marshal doesn't know anything about document.Number - so when it sees that value, it just sees through reflection that it's a string alias and marshals accordingly. Conversely a json.Number would be round-tripped correctly because the implementation is built to recognize that type specifically. So basically, terraform's logic to map a document type over to a json payload is wrong in the sense that it misses this translation. That's something they need to handle. FWIW if I had to release this from scratch again, I don't think I would have chosen to handle document numerics this way. There's a disconnect here in the sense that the stdlib json decoder gives you the choice to decode into json.Number, whereas we do not. Unfortunately that ship has sailed. Going to close this out at this time, since there's definitively no SDK defect either way. |
lucix-aws
removed
bug
|
This issue is now closed. Comments on closed issues are hard for our team to see. |
Acknowledgements
go get -u github.com/aws/aws-sdk-go-v2/...)Describe the bug
The landing zone manifest has two
retentionDaysfields that are numbers.centralizedLogging.configurations.accessLoggingBucket.retentionDayscentralizedLogging.configurations.loggingBucket.retentionDaysThe AWS API returns them as numbers
aws controltower get-landing-zone --landing-zone-identifier '$landing_zone_arn' --output jsonoutput with sensitive data redacted:
when using the go sdk, they are converted to string which is incorrect:
you can see both
retentionDaysare"90"instead of90Expected Behavior
both
centralizedLogging.configurations.accessLoggingBucket.retentionDays¢ralizedLogging.configurations.loggingBucket.retentionDaysin the manifest should remain numbers and not be stringsCurrent Behavior
both
centralizedLogging.configurations.accessLoggingBucket.retentionDays¢ralizedLogging.configurations.loggingBucket.retentionDaysin the manifest are converted to stringsReproduction Steps
./main "$landing_zone_arnoutput shows the LogResponseWithBody debug logging that the api response is correct. both{"retentionDays":90}are numbers and not strings (formatted for readability)but the smithy document is incorrect, both
{"retentionDays":"90"}are strings and not numbersPossible Solution
No response
Additional Information/Context
this is causing half of this terraform issue hashicorp/terraform-provider-aws#35763 - specifically this part of the plan, trying to change the type from a string to a number, because when terraform reads the current manifest using the aws sdk the type is wrong so terraform tries to change it back. It is not wrong at aws, but the go aws sdk is converting them to a string
AWS Go SDK V2 Module Versions Used
go mod graphCompiler and Version used
go version go1.22.3 linux/amd64
Operating System and version
Fedora Linux 40
The text was updated successfully, but these errors were encountered: