Skip to content

Commit

Permalink
fix: avoid sending template metadata update if max_port_share_level…
Browse files Browse the repository at this point in the history 
… is default (#190)

Closes #188.

For context, the `MaxPortShareLevel` template metadata value was not
present on the `coderd` create template request prior to 2.15. As such,
during template creation we need to send an update request for backwards
compatibility (though we could probably remove this soon), see #110.

Whilst we don't send this update request is the attribute is omitted on
the resource, we were sending a spurious update request if the value was
explicitly configured to the default value ('owner' for enterprise+,
'public' otherwise). This was causing the error in the linked issue. An
example is seen in the newly added test.

The fix is to just not send that update request if the configured value
is:
- Unknown
- Equal to the value on the newly created template.

note: I'd recommend hiding the whitespace when reviewing the diff
  • Loading branch information
@ethanndickson
ethanndickson authored Feb 26, 2025
1 parent aa3dbcd commit 1d19415
Show file tree
Hide file tree
Showing 2 changed files with 144 additions and 111 deletions.
2 changes: 2 additions & 0 deletions internal/provider/template_resource.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,8 @@ func (r *TemplateResource) Create(ctx context.Context, req resource.CreateReques
// deployment running `v2.15.0` or later.
if data.MaxPortShareLevel.IsUnknown() {
data.MaxPortShareLevel = types.StringValue(string(templateResp.MaxPortShareLevel))
} else if data.MaxPortShareLevel.ValueString() == string(templateResp.MaxPortShareLevel) {
tflog.Info(ctx, "max port share level set to default, not updating")
} else {
mpslReq := data.toUpdateRequest(ctx, &resp.Diagnostics)
if resp.Diagnostics.HasError() {
Expand Down
253 changes: 142 additions & 111 deletions internal/provider/template_resource_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -657,129 +657,160 @@ func TestAccTemplateResourceEnterprise(t *testing.T) {
err = cp.Copy("../../integration/template-test/example-template", exTemplateOne)
require.NoError(t, err)

cfg1 := testAccTemplateResourceConfig{
URL: client.URL.String(),
Token: client.SessionToken(),
Name: ptr.Ref("example-template"),
Versions: []testAccTemplateVersionConfig{
{
// Auto-generated version name
Directory: &exTemplateOne,
Active: ptr.Ref(true),
},
},
ACL: testAccTemplateACLConfig{
GroupACL: []testAccTemplateKeyValueConfig{
{
Key: ptr.Ref(firstUser.OrganizationIDs[0].String()),
Value: ptr.Ref("use"),
},
t.Run("BasicUsage", func(t *testing.T) {
cfg1 := testAccTemplateResourceConfig{
URL: client.URL.String(),
Token: client.SessionToken(),
Name: ptr.Ref("example-template"),
Versions: []testAccTemplateVersionConfig{
{
Key: ptr.Ref(group.ID.String()),
Value: ptr.Ref("admin"),
// Auto-generated version name
Directory: &exTemplateOne,
Active: ptr.Ref(true),
},
},
UserACL: []testAccTemplateKeyValueConfig{
{
Key: ptr.Ref(firstUser.ID.String()),
Value: ptr.Ref("admin"),
ACL: testAccTemplateACLConfig{
GroupACL: []testAccTemplateKeyValueConfig{
{
Key: ptr.Ref(firstUser.OrganizationIDs[0].String()),
Value: ptr.Ref("use"),
},
{
Key: ptr.Ref(group.ID.String()),
Value: ptr.Ref("admin"),
},
},
UserACL: []testAccTemplateKeyValueConfig{
{
Key: ptr.Ref(firstUser.ID.String()),
Value: ptr.Ref("admin"),
},
},
},
},
}
}

cfg2 := cfg1
cfg2.ACL.GroupACL = slices.Clone(cfg2.ACL.GroupACL[1:])
cfg2.MaxPortShareLevel = ptr.Ref("owner")
cfg2 := cfg1
cfg2.ACL.GroupACL = slices.Clone(cfg2.ACL.GroupACL[1:])
cfg2.MaxPortShareLevel = ptr.Ref("owner")

cfg3 := cfg2
cfg3.ACL.null = true
cfg3.MaxPortShareLevel = ptr.Ref("public")
cfg3 := cfg2
cfg3.ACL.null = true
cfg3.MaxPortShareLevel = ptr.Ref("public")

cfg4 := cfg3
cfg4.AllowUserAutostart = ptr.Ref(false)
cfg4.AutostopRequirement = testAccAutostopRequirementConfig{
DaysOfWeek: ptr.Ref([]string{"monday", "tuesday"}),
Weeks: ptr.Ref(int64(2)),
}
cfg4 := cfg3
cfg4.AllowUserAutostart = ptr.Ref(false)
cfg4.AutostopRequirement = testAccAutostopRequirementConfig{
DaysOfWeek: ptr.Ref([]string{"monday", "tuesday"}),
Weeks: ptr.Ref(int64(2)),
}

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
IsUnitTest: true,
ProtoV6ProviderFactories: testAccProtoV6ProviderFactories,
Steps: []resource.TestStep{
{
Config: cfg1.String(t),
Check: resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttr("coderd_template.test", "max_port_share_level", "owner"),
resource.TestCheckResourceAttr("coderd_template.test", "acl.groups.#", "2"),
resource.TestMatchTypeSetElemNestedAttrs("coderd_template.test", "acl.groups.*", map[string]*regexp.Regexp{
"id": regexp.MustCompile(firstUser.OrganizationIDs[0].String()),
"role": regexp.MustCompile("^use$"),
}),
resource.TestMatchTypeSetElemNestedAttrs("coderd_template.test", "acl.groups.*", map[string]*regexp.Regexp{
"id": regexp.MustCompile(group.ID.String()),
"role": regexp.MustCompile("^admin$"),
}),
resource.TestCheckResourceAttr("coderd_template.test", "acl.users.#", "1"),
resource.TestMatchTypeSetElemNestedAttrs("coderd_template.test", "acl.users.*", map[string]*regexp.Regexp{
"id": regexp.MustCompile(firstUser.ID.String()),
"role": regexp.MustCompile("^admin$"),
}),
),
},
{
Config: cfg2.String(t),
Check: resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttr("coderd_template.test", "max_port_share_level", "owner"),
resource.TestMatchTypeSetElemNestedAttrs("coderd_template.test", "acl.users.*", map[string]*regexp.Regexp{
"id": regexp.MustCompile(firstUser.ID.String()),
"role": regexp.MustCompile("^admin$"),
}),
),
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
IsUnitTest: true,
ProtoV6ProviderFactories: testAccProtoV6ProviderFactories,
Steps: []resource.TestStep{
{
Config: cfg1.String(t),
Check: resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttr("coderd_template.test", "max_port_share_level", "owner"),
resource.TestCheckResourceAttr("coderd_template.test", "acl.groups.#", "2"),
resource.TestMatchTypeSetElemNestedAttrs("coderd_template.test", "acl.groups.*", map[string]*regexp.Regexp{
"id": regexp.MustCompile(firstUser.OrganizationIDs[0].String()),
"role": regexp.MustCompile("^use$"),
}),
resource.TestMatchTypeSetElemNestedAttrs("coderd_template.test", "acl.groups.*", map[string]*regexp.Regexp{
"id": regexp.MustCompile(group.ID.String()),
"role": regexp.MustCompile("^admin$"),
}),
resource.TestCheckResourceAttr("coderd_template.test", "acl.users.#", "1"),
resource.TestMatchTypeSetElemNestedAttrs("coderd_template.test", "acl.users.*", map[string]*regexp.Regexp{
"id": regexp.MustCompile(firstUser.ID.String()),
"role": regexp.MustCompile("^admin$"),
}),
),
},
{
Config: cfg2.String(t),
Check: resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttr("coderd_template.test", "max_port_share_level", "owner"),
resource.TestMatchTypeSetElemNestedAttrs("coderd_template.test", "acl.users.*", map[string]*regexp.Regexp{
"id": regexp.MustCompile(firstUser.ID.String()),
"role": regexp.MustCompile("^admin$"),
}),
),
},
{
Config: cfg3.String(t),
Check: resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttr("coderd_template.test", "max_port_share_level", "public"),
resource.TestCheckNoResourceAttr("coderd_template.test", "acl"),
func(s *terraform.State) error {
templates, err := client.Templates(ctx, codersdk.TemplateFilter{})
if err != nil {
return err
}
if len(templates) != 1 {
return fmt.Errorf("expected 1 template, got %d", len(templates))
}
acl, err := client.TemplateACL(ctx, templates[0].ID)
if err != nil {
return err
}
if len(acl.Groups) != 1 {
return fmt.Errorf("expected 1 group ACL, got %d", len(acl.Groups))
}
if acl.Groups[0].Role != "admin" && acl.Groups[0].ID != group.ID {
return fmt.Errorf("expected group ACL to be 'use' for %s, got %s", firstUser.OrganizationIDs[0].String(), acl.Groups[0].Role)
}
if len(acl.Users) != 1 {
return fmt.Errorf("expected 1 user ACL, got %d", len(acl.Users))
}
if acl.Users[0].Role != "admin" && acl.Users[0].ID != firstUser.ID {
return fmt.Errorf("expected user ACL to be 'admin' for %s, got %s", firstUser.ID.String(), acl.Users[0].Role)
}
return nil
},
),
},
{
Config: cfg4.String(t),
Check: resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttr("coderd_template.test", "allow_user_auto_start", "false"),
resource.TestCheckResourceAttr("coderd_template.test", "auto_stop_requirement.days_of_week.#", "2"),
resource.TestCheckResourceAttr("coderd_template.test", "auto_stop_requirement.weeks", "2"),
),
},
},
{
Config: cfg3.String(t),
Check: resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttr("coderd_template.test", "max_port_share_level", "public"),
resource.TestCheckNoResourceAttr("coderd_template.test", "acl"),
func(s *terraform.State) error {
templates, err := client.Templates(ctx, codersdk.TemplateFilter{})
if err != nil {
return err
}
if len(templates) != 1 {
return fmt.Errorf("expected 1 template, got %d", len(templates))
}
acl, err := client.TemplateACL(ctx, templates[0].ID)
if err != nil {
return err
}
if len(acl.Groups) != 1 {
return fmt.Errorf("expected 1 group ACL, got %d", len(acl.Groups))
}
if acl.Groups[0].Role != "admin" && acl.Groups[0].ID != group.ID {
return fmt.Errorf("expected group ACL to be 'use' for %s, got %s", firstUser.OrganizationIDs[0].String(), acl.Groups[0].Role)
}
if len(acl.Users) != 1 {
return fmt.Errorf("expected 1 user ACL, got %d", len(acl.Users))
}
if acl.Users[0].Role != "admin" && acl.Users[0].ID != firstUser.ID {
return fmt.Errorf("expected user ACL to be 'admin' for %s, got %s", firstUser.ID.String(), acl.Users[0].Role)
}
return nil
},
),
})
})

// Verifies that when `max_port_share_level` is set to to the default value,
// an update request that would return HTTP Not Modified is not sent.
t.Run("DefaultMaxPortShareLevel", func(t *testing.T) {
cfg1 := testAccTemplateResourceConfig{
URL: client.URL.String(),
Token: client.SessionToken(),
Name: ptr.Ref("example-template"),
Versions: []testAccTemplateVersionConfig{
{
Directory: &exTemplateOne,
Active: ptr.Ref(true),
},
},
{
Config: cfg4.String(t),
Check: resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttr("coderd_template.test", "allow_user_auto_start", "false"),
resource.TestCheckResourceAttr("coderd_template.test", "auto_stop_requirement.days_of_week.#", "2"),
resource.TestCheckResourceAttr("coderd_template.test", "auto_stop_requirement.weeks", "2"),
),
MaxPortShareLevel: ptr.Ref("owner"),
}

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
IsUnitTest: true,
ProtoV6ProviderFactories: testAccProtoV6ProviderFactories,
Steps: []resource.TestStep{
{
Config: cfg1.String(t),
Check: resource.TestCheckResourceAttr("coderd_template.test", "max_port_share_level", "owner"),
},
},
},
})
})
}

Expand Down

0 comments on commit 1d19415

Please sign in to comment.