Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cvm): Fix optional parameters never being omitted from requests #2988

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jalle19
Copy link

@Jalle19 Jalle19 commented Dec 2, 2024

Initially noticed this problem when I tried to create a launch template containing the following block:

internet_accessible {
  public_ip_assigned = true
  internet_max_bandwidth_out = 100
}

This resulted in an error during apply like this:

Code=InvalidParameterValue, Message=The value `` specified in the parameter `.InternetAccessible.InternetChargeType` is invalid.

I figured okay, I'll just add what should be the defaults and get on with it. But ultimately I got the same error from the BandwidthPackageId field. There is no good default value for it, so I was unable to make the launch template.

Turns out that throughout the SDK, all scalar fields are always included, at least wherever InterfacesHeadMap is used. The value of the individual fields is never checked, so every field ends up being sent, with the default value being not the default value defined in the schema, but the default value for the data type, which is definitely wrong. Since the default value for strings is an empty string, any field inside a block that takes an ID parameter of some kind will fail with an API validation error.

In this commit I've only fixed the internet_accessible block for launch templates, but ultimately all locations where InterfacesHeadMap (and possibly others, I haven't investigated that much) need to be updated.

FWIW, the AWS Terraform provider uses this very same way of dealing with this issue.

Initially noticed this problem when I tried to create a launch template containing the following block:

```
internet_accessible {
  public_ip_assigned = true
  internet_max_bandwidth_out = 100
}
```

This resulted in an error during apply like this:

```
Code=InvalidParameterValue, Message=The value `` specified in the parameter `.InternetAccessible.InternetChargeType` is invalid.
```

I figured okay, I'll just add what should be the defaults and get on with it. But ultimately I got the same error from the `BandwidthPackageId` field. There is no good default value for it, so I was unable to make the launch template.

Turns out that throughout the SDK, all scalar fields are always included, at least wherever `InterfacesHeadMap` is used. The value of the individual fields is never checked, so every field ends up being sent, with the default value being not the default value defined in the schema, but the default value for the data type, which is definitely wrong. Since the default value for strings is an empty string, any field inside a block that takes an ID parameter of some kind will fail with an API validation error.

In this commit I've only fixed the `internet_accessible` block for launch templates, but ultimately all locations where `InterfacesHeadMap` (and possibly others, I haven't investigated that much) need to be updated.

FWIW, the AWS Terraform provider uses this very same way of dealing with this issue.
@Jalle19 Jalle19 force-pushed the launch-template-internet-accessible branch from 8fabd58 to 642dec8 Compare December 2, 2024 12:54
@Jalle19 Jalle19 changed the title Fix optional parameters never being omitted from requests fix(cvm): Fix optional parameters never being omitted from requests Dec 2, 2024
@SevenEarth
Copy link
Collaborator

It's true, This way of writing code is indeed more robust

if v, ok := dMap["internet_charge_type"].(string); ok && v != "" {
    internetAccessible.InternetChargeType = helper.String(v)
}

@Jalle19
Copy link
Author

Jalle19 commented Dec 5, 2024

Ideally all places where InterfacesHeadMap is used should be checked

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants