aws / aws-sdk

Landing page for the AWS SDKs on GitHub
https://aws.amazon.com/tools/
Other
68 stars 12 forks source link

imagebuilder fails to serialize `types.LaunchTemplateConfiguration.SetDefaultVersion` when set to `false` #736

Open DanielRieske opened 1 month ago

DanielRieske commented 1 month ago

Acknowledgements

Describe the bug

The types.LaunchTemplateConfiguration.SetDefaultVersion fails to serialize when set to false.

Logging:

  http.request.body=
  | {"clientToken":"terraform-20240505213014851300000002","distributions":[{"amiDistributionConfiguration":{"name":"{{ imagebuilder:buildDate }}"},"containerDistributionConfiguration":{"targetRepository":{"repositoryName":"repository-name","service":"ECR"}},"fastLaunchConfigurations":[{"accountId":"170689858638","enabled":true,"launchTemplate":{"launchTemplateId":"lt-07dac61f7e03c2b40","launchTemplateVersion":"1"},"maxParallelLaunches":6,"snapshotConfiguration":{"targetResourceCount":1}}],"launchTemplateConfigurations":[{"accountId":"170689858638","launchTemplateId":"lt-07dac61f7e03c2b40"}],"region":"us-west-2"}],"name":"tf-acc-test-6166724565934347995"}
  |   # aws_imagebuilder_distribution_configuration.test will be created
  |   + resource "aws_imagebuilder_distribution_configuration" "test" {
  |       + arn          = (known after apply)
  |       + date_created = (known after apply)
  |       + date_updated = (known after apply)
  |       + id           = (known after apply)
  |       + name         = "tf-acc-test-6166724565934347995"
  |       + tags_all     = (known after apply)
  | 
  |       + distribution {
  |           + license_configuration_arns = []
  |           + region                     = "us-west-2"
  | 
  |           + ami_distribution_configuration {
  |               + name               = "{{ imagebuilder:buildDate }}"
  |               + target_account_ids = []
  |             }
  | 
  |           + container_distribution_configuration {
  |               + container_tags = []
  | 
  |               + target_repository {
  |                   + repository_name = "repository-name"
  |                   + service         = "ECR"
  |                 }
  |             }
  | 
  |           + fast_launch_configuration {
  |               + account_id            = "170689858638"
  |               + enabled               = true
  |               + max_parallel_launches = 6
  | 
  |               + launch_template {
  |                   + launch_template_id      = (known after apply)
  |                   + launch_template_version = "1"
  |                 }
  | 
  |               + snapshot_configuration {
  |                   + target_resource_count = 1
  |                 }
  |             }
  | 
  |           + launch_template_configuration {
  |               + account_id         = "170689858638"
  |               + default            = false <--- This argument
  |               + launch_template_id = (known after apply)
  |             }
  |         }
  |     }

Expected Behavior

That the SetDefaultVersion was serialized in it's creation call.

Current Behavior

SetDefaultVersion isn't serialized.

Reproduction Steps

See above but running: https://github.com/bschaatsbergen/terraform-provider-aws/blob/c8a4204284bc59aae965acddba1257239062009a/internal/service/imagebuilder/distribution_configuration_data_source_test.go#L16

Possible Solution

No response

Additional Information/Context

Relates: https://github.com/aws/aws-sdk-go-v2/issues/2162

AWS Go SDK V2 Module Versions Used

github.com/aws/aws-sdk-go-v2/service/imagebuilder v1.33.3

Compiler and Version used

go version go1.22.2 darwin/arm64

Operating System and version

macOS Sonoma 14.4.1

RanVaknin commented 1 month ago

Hi @DanielRieske ,

Thanks for reporting this issue. I'm able to reproduce this behavior. What we suspect the issue here is that the ImageBuilder service defines a default value of false to this member, but when the value is not explicitly sent, the service assigns a true default value for it.

We are working on upstreaming this issue to the service and will let you know once we have more details.

Thanks, Ran~

DanielRieske commented 1 month ago

HI @RanVaknin

Thanks for taking a look at this!

From your explanation I understand that this would happen if we don't explicitly set the SetDefaultVersion bool to false and sent it to the service. However during testing I set it explicitly to false but it wasn't able to serialize in the request body to the API.

And looking at the current implementation of how this argument serializes that makes sense.

Wouldn't this be an issue in the logic for serialization rather than an issue for the imagebuilder service team?

Again thanks for your help so far!

RanVaknin commented 1 month ago

Hi @DanielRieske ,

Thanks for your patience. The problem here is two fold:

  1. The Go SDK will not serialize the default value if it matches the zero value of that data type. In this case the zero value for a boolean is false, and that is also the default, therefore the serializer will omit the value in order to be more performant. This should be circumvented by a default value set on the service side, or with the service annotating their model with the required trait.

  2. The service side default is set to true instead of false. For example, if I send the request without that value you can see that the server side defaults to true, contrary to what the model specifies.

$ aws imagebuilder get-distribution-configuration --distribution-configuration-arn arn:aws:imagebuilder:us-east-1:REDACTED:distribution-configuration/example-distribution-config
{
    "requestId": "REDACTED",
    "distributionConfiguration": {
        "arn": "arn:aws:imagebuilder:us-east-1:REDACTED:distribution-configuration/example-distribution-config",
        "name": "example-distribution-config",
        "distributions": [
            {
                "region": "us-east-1",
                "amiDistributionConfiguration": {
                    "name": "MyAMI-{{ imagebuilder:buildDate }}"
                },
                "launchTemplateConfigurations": [
                    {
                        "launchTemplateId": "lt-REDACTED",
                        "accountId": "REDACTED",
                        "setDefaultVersion": true  <-------
                    }
                ]
            }
        ],
        "dateCreated": "2024-05-07T18:28:35.486Z",
        "tags": {}
    }
}

We are discussing what is the best way to fix this up and will update you as soon as we can. Thanks, Ran~

bschaatsbergen commented 2 weeks ago

Hey @RanVaknin, just checking in - is there an update on this already?