Skip to content

fix: do not skip parameter validation if min or max = 0 #7707

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
May 30, 2023
Merged

fix: do not skip parameter validation if min or max = 0 #7707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2902f8a
fix: do not skip parameter validation if min or max = 0
mtojek May 29, 2023
aaabe5b
fix
mtojek May 29, 2023
d2d8818
Merge branch 'main' into 7582-min-max-fix
mtojek May 30, 2023
d507f9d
fix: site
mtojek May 30, 2023
d70145a
fix
mtojek May 30, 2023
5497215
Update terraform-provider-coder to v0.8.1
mtojek May 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion cli/create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/coder/coder/cli/clitest"
"github.com/coder/coder/coderd/coderdtest"
"github.com/coder/coder/coderd/gitauth"
"github.com/coder/coder/coderd/util/ptr"
"github.com/coder/coder/codersdk"
"github.com/coder/coder/provisioner/echo"
"github.com/coder/coder/provisionersdk/proto"
Expand Down Expand Up @@ -494,7 +495,7 @@ func TestCreateValidateRichParameters(t *testing.T) {
)

numberRichParameters := []*proto.RichParameter{
{Name: numberParameterName, Type: "number", Mutable: true, ValidationMin: 3, ValidationMax: 10},
{Name: numberParameterName, Type: "number", Mutable: true, ValidationMin: ptr.Ref(int32(3)), ValidationMax: ptr.Ref(int32(10))},
}

stringRichParameters := []*proto.RichParameter{
Expand Down
3 changes: 2 additions & 1 deletion cli/update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (

"github.com/coder/coder/cli/clitest"
"github.com/coder/coder/coderd/coderdtest"
"github.com/coder/coder/coderd/util/ptr"
"github.com/coder/coder/codersdk"
"github.com/coder/coder/provisioner/echo"
"github.com/coder/coder/provisionersdk/proto"
Expand Down Expand Up @@ -245,7 +246,7 @@ func TestUpdateValidateRichParameters(t *testing.T) {
)

numberRichParameters := []*proto.RichParameter{
{Name: numberParameterName, Type: "number", Mutable: true, ValidationMin: 3, ValidationMax: 10},
{Name: numberParameterName, Type: "number", Mutable: true, ValidationMin: ptr.Ref(int32(3)), ValidationMax: ptr.Ref(int32(10))},
}

stringRichParameters := []*proto.RichParameter{
Expand Down
15 changes: 13 additions & 2 deletions coderd/database/db2sdk/db2sdk.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,17 @@ func TemplateVersionParameter(param database.TemplateVersionParameter) (codersdk
if err != nil {
return codersdk.TemplateVersionParameter{}, err
}

var validationMin *int32
if param.ValidationMin.Valid {
validationMin = &param.ValidationMin.Int32
}

var validationMax *int32
if param.ValidationMax.Valid {
validationMax = &param.ValidationMax.Int32
}

return codersdk.TemplateVersionParameter{
Name: param.Name,
DisplayName: param.DisplayName,
Expand All @@ -57,8 +68,8 @@ func TemplateVersionParameter(param database.TemplateVersionParameter) (codersdk
Icon: param.Icon,
Options: options,
ValidationRegex: param.ValidationRegex,
ValidationMin: param.ValidationMin,
ValidationMax: param.ValidationMax,
ValidationMin: validationMin,
ValidationMax: validationMax,
ValidationError: param.ValidationError,
ValidationMonotonic: codersdk.ValidationMonotonicOrder(param.ValidationMonotonic),
Required: param.Required,
Expand Down
4 changes: 2 additions & 2 deletions coderd/database/dump.sql
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions coderd/database/migrations/000124_validation_min_max_nullable.down.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
BEGIN;
UPDATE template_version_parameters SET validation_min = 0 WHERE validation_min = NULL;
UPDATE template_version_parameters SET validation_max = 0 WHERE validation_max = NULL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, the lossyness here seems fine.

ALTER TABLE template_version_parameters ALTER COLUMN validation_min SET NOT NULL;
ALTER TABLE template_version_parameters ALTER COLUMN validation_max SET NOT NULL;
COMMIT;
6 changes: 6 additions & 0 deletions coderd/database/migrations/000124_validation_min_max_nullable.up.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
BEGIN;
ALTER TABLE template_version_parameters ALTER COLUMN validation_min DROP NOT NULL;
ALTER TABLE template_version_parameters ALTER COLUMN validation_max DROP NOT NULL;
UPDATE template_version_parameters SET validation_min = NULL WHERE validation_min = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking out loud, what if someone has explicitly specified min to be 0 so that -1 can't be used? This seems like a potentially lossy migration and we should probably wait until templates are updated to receive their new, null value?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not worse than what we have today, because we treat min=0 as disabling validation!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, didn't realize. That's both surprising and worrying, glad it's getting fixed. 😄

UPDATE template_version_parameters SET validation_max = NULL WHERE validation_max = 0;
COMMIT;
4 changes: 2 additions & 2 deletions coderd/database/models.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions coderd/database/queries.sql.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 17 additions & 2 deletions coderd/provisionerdserver/provisionerdserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -919,6 +919,21 @@ func (server *Server) CompleteJob(ctx context.Context, completed *proto.Complete
if err != nil {
return nil, xerrors.Errorf("marshal parameter options: %w", err)
}

var validationMin, validationMax sql.NullInt32
if richParameter.ValidationMin != nil {
validationMin = sql.NullInt32{
Int32: *richParameter.ValidationMin,
Valid: true,
}
}
if richParameter.ValidationMax != nil {
validationMax = sql.NullInt32{
Int32: *richParameter.ValidationMax,
Valid: true,
}
}

_, err = server.Database.InsertTemplateVersionParameter(ctx, database.InsertTemplateVersionParameterParams{
TemplateVersionID: input.TemplateVersionID,
Name: richParameter.Name,
Expand All @@ -931,8 +946,8 @@ func (server *Server) CompleteJob(ctx context.Context, completed *proto.Complete
Options: options,
ValidationRegex: richParameter.ValidationRegex,
ValidationError: richParameter.ValidationError,
ValidationMin: richParameter.ValidationMin,
ValidationMax: richParameter.ValidationMax,
ValidationMin: validationMin,
ValidationMax: validationMax,
ValidationMonotonic: richParameter.ValidationMonotonic,
Required: richParameter.Required,
LegacyVariableName: richParameter.LegacyVariableName,
Expand Down
13 changes: 11 additions & 2 deletions coderd/templateversions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1706,6 +1706,15 @@ func convertTemplateVersionParameter(param database.TemplateVersionParameter) (c
if err != nil {
return codersdk.TemplateVersionParameter{}, err
}

var validationMin, validationMax *int32
if param.ValidationMin.Valid {
validationMin = &param.ValidationMin.Int32
}
if param.ValidationMax.Valid {
validationMax = &param.ValidationMax.Int32
}

return codersdk.TemplateVersionParameter{
Name: param.Name,
DisplayName: param.DisplayName,
Expand All @@ -1717,8 +1726,8 @@ func convertTemplateVersionParameter(param database.TemplateVersionParameter) (c
Icon: param.Icon,
Options: options,
ValidationRegex: param.ValidationRegex,
ValidationMin: param.ValidationMin,
ValidationMax: param.ValidationMax,
ValidationMin: validationMin,
ValidationMax: validationMax,
ValidationError: param.ValidationError,
ValidationMonotonic: codersdk.ValidationMonotonicOrder(param.ValidationMonotonic),
Required: param.Required,
Expand Down
4 changes: 2 additions & 2 deletions coderd/workspaces_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1973,8 +1973,8 @@ func TestWorkspaceWithRichParameters(t *testing.T) {
DisplayName: secondParameterDisplayName,
Type: secondParameterType,
Description: secondParameterDescription,
ValidationMin: 1,
ValidationMax: 3,
ValidationMin: ptr.Ref(int32(1)),
ValidationMax: ptr.Ref(int32(3)),
ValidationMonotonic: string(secondParameterValidationMonotonic),
},
},
Expand Down
15 changes: 12 additions & 3 deletions codersdk/richparameters.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,15 +79,23 @@ func ValidateWorkspaceBuildParameter(richParameter TemplateVersionParameter, bui
}

validation := &provider.Validation{
Min: int(richParameter.ValidationMin),
Max: int(richParameter.ValidationMax),
Min: ptrInt(richParameter.ValidationMin),
Max: ptrInt(richParameter.ValidationMax),
Regex: richParameter.ValidationRegex,
Error: richParameter.ValidationError,
Monotonic: string(richParameter.ValidationMonotonic),
}
return validation.Valid(richParameter.Type, value)
}

func ptrInt(number *int32) *int {
if number == nil {
return nil
}
n := int(*number)
return &n
}

func findBuildParameter(params []WorkspaceBuildParameter, parameterName string) (*WorkspaceBuildParameter, bool) {
if params == nil {
return nil, false
Expand All @@ -111,7 +119,8 @@ func parameterValuesAsArray(options []TemplateVersionParameterOption) []string {

func validationEnabled(param TemplateVersionParameter) bool {
return len(param.ValidationRegex) > 0 ||
(param.ValidationMin != 0 && param.ValidationMax != 0) ||
param.ValidationMin != nil ||
param.ValidationMax != nil ||
len(param.ValidationMonotonic) > 0 ||
param.Type == "bool" || // boolean type doesn't have any custom validation rules, but the value must be checked (true/false).
param.Type == "list(string)" // list(string) type doesn't have special validation, but we need to check if this is a correct list.
Expand Down
15 changes: 8 additions & 7 deletions codersdk/richparameters_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (

"github.com/stretchr/testify/require"

"github.com/coder/coder/coderd/util/ptr"
"github.com/coder/coder/codersdk"
)

Expand Down Expand Up @@ -72,8 +73,8 @@ func TestParameterResolver_ValidateResolve_PrevInvalid(t *testing.T) {
p := codersdk.TemplateVersionParameter{
Name: "n",
Type: "number",
ValidationMax: 10,
ValidationMin: 1,
ValidationMax: ptr.Ref(int32(10)),
ValidationMin: ptr.Ref(int32(1)),
}
v, err := uut.ValidateResolve(p, nil)
require.Error(t, err)
Expand All @@ -89,8 +90,8 @@ func TestParameterResolver_ValidateResolve_DefaultInvalid(t *testing.T) {
p := codersdk.TemplateVersionParameter{
Name: "n",
Type: "number",
ValidationMax: 10,
ValidationMin: 1,
ValidationMax: ptr.Ref(int32(10)),
ValidationMin: ptr.Ref(int32(1)),
DefaultValue: "11",
}
v, err := uut.ValidateResolve(p, nil)
Expand Down Expand Up @@ -221,19 +222,19 @@ func TestRichParameterValidation(t *testing.T) {

numberRichParameters := []codersdk.TemplateVersionParameter{
{Name: stringParameterName, Type: "string", Mutable: true},
{Name: numberParameterName, Type: "number", Mutable: true, ValidationMin: 3, ValidationMax: 10},
{Name: numberParameterName, Type: "number", Mutable: true, ValidationMin: ptr.Ref(int32(3)), ValidationMax: ptr.Ref(int32(10))},
{Name: boolParameterName, Type: "bool", Mutable: true},
}

monotonicIncreasingNumberRichParameters := []codersdk.TemplateVersionParameter{
{Name: stringParameterName, Type: "string", Mutable: true},
{Name: numberParameterName, Type: "number", Mutable: true, ValidationMin: 3, ValidationMax: 10, ValidationMonotonic: "increasing"},
{Name: numberParameterName, Type: "number", Mutable: true, ValidationMin: ptr.Ref(int32(3)), ValidationMax: ptr.Ref(int32(10)), ValidationMonotonic: "increasing"},
{Name: boolParameterName, Type: "bool", Mutable: true},
}

monotonicDecreasingNumberRichParameters := []codersdk.TemplateVersionParameter{
{Name: stringParameterName, Type: "string", Mutable: true},
{Name: numberParameterName, Type: "number", Mutable: true, ValidationMin: 3, ValidationMax: 10, ValidationMonotonic: "decreasing"},
{Name: numberParameterName, Type: "number", Mutable: true, ValidationMin: ptr.Ref(int32(3)), ValidationMax: ptr.Ref(int32(10)), ValidationMonotonic: "decreasing"},
{Name: boolParameterName, Type: "bool", Mutable: true},
}

Expand Down
4 changes: 2 additions & 2 deletions codersdk/templateversions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ type TemplateVersionParameter struct {
Options []TemplateVersionParameterOption `json:"options"`
ValidationError string `json:"validation_error,omitempty"`
ValidationRegex string `json:"validation_regex,omitempty"`
ValidationMin int32 `json:"validation_min,omitempty"`
ValidationMax int32 `json:"validation_max,omitempty"`
ValidationMin *int32 `json:"validation_min,omitempty"`
ValidationMax *int32 `json:"validation_max,omitempty"`
ValidationMonotonic ValidationMonotonicOrder `json:"validation_monotonic,omitempty" enums:"increasing,decreasing"`
Required bool `json:"required"`
LegacyVariableName string `json:"legacy_variable_name,omitempty"`
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ require (
github.com/codeclysm/extract v2.2.0+incompatible
github.com/coder/flog v1.1.0
github.com/coder/retry v1.3.1-0.20230210155434-e90a2e1e091d
github.com/coder/terraform-provider-coder v0.6.23
github.com/coder/terraform-provider-coder v0.8.1
github.com/coder/wgtunnel v0.1.5
github.com/coreos/go-oidc/v3 v3.6.0
github.com/coreos/go-systemd v0.0.0-20191104093116-d3cd4ed1dbcf
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,8 +336,8 @@ github.com/coder/ssh v0.0.0-20230421140225-04bb837133e1 h1:LBw76rEDuhNJyohve11mb
github.com/coder/ssh v0.0.0-20230421140225-04bb837133e1/go.mod h1:ZSS+CUoKHDrqVakTfTWUlKSr9MtMFkC4UvtQKD7O914=
github.com/coder/tailscale v0.0.0-20230522123520-74712221d00f h1:F0Xr1d8h8dAHn7tab1HXuzYFkcjmCydnEfdMbkOhlVk=
github.com/coder/tailscale v0.0.0-20230522123520-74712221d00f/go.mod h1:jpg+77g19FpXL43U1VoIqoSg1K/Vh5CVxycGldQ8KhA=
github.com/coder/terraform-provider-coder v0.6.23 h1:O2Rcj0umez4DfVdGnKZi63z1Xzxd0IQOn9VQDB8YU8g=
github.com/coder/terraform-provider-coder v0.6.23/go.mod h1:UIfU3bYNeSzJJvHyJ30tEKjD6Z9utloI+HUM/7n94CY=
github.com/coder/terraform-provider-coder v0.8.1 h1:i/LhvFi+Ei0gL+h4GItJfwtxjcITTlQhS+R8J+0vRo8=
github.com/coder/terraform-provider-coder v0.8.1/go.mod h1:UIfU3bYNeSzJJvHyJ30tEKjD6Z9utloI+HUM/7n94CY=
github.com/coder/wgtunnel v0.1.5 h1:WP3sCj/3iJ34eKvpMQEp1oJHvm24RYh0NHbj1kfUKfs=
github.com/coder/wgtunnel v0.1.5/go.mod h1:bokoUrHnUFY4lu9KOeSYiIcHTI2MO1KwqumU4DPDyJI=
github.com/containerd/aufs v0.0.0-20200908144142-dab0cbea06f4/go.mod h1:nukgQABAEopAHvB6j7cnP5zJ+/3aVcE7hCYqvIwAHyE=
Expand Down
13 changes: 11 additions & 2 deletions provisioner/terraform/resources.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,8 +483,8 @@ func ConvertState(modules []*tfjson.StateModule, rawGraph string, rawParameterNa
if len(param.Validation) == 1 {
protoParam.ValidationRegex = param.Validation[0].Regex
protoParam.ValidationError = param.Validation[0].Error
protoParam.ValidationMax = int32(param.Validation[0].Max)
protoParam.ValidationMin = int32(param.Validation[0].Min)
protoParam.ValidationMax = ptrInt32(param.Validation[0].Max)
protoParam.ValidationMin = ptrInt32(param.Validation[0].Min)
protoParam.ValidationMonotonic = param.Validation[0].Monotonic
}
if len(param.Option) > 0 {
Expand Down Expand Up @@ -527,6 +527,15 @@ func ConvertState(modules []*tfjson.StateModule, rawGraph string, rawParameterNa
}, nil
}

func ptrInt32(number *int) *int32 {
var n int32
if number == nil {
return &n
}
n = int32(*number)
return &n
}

// convertAddressToLabel returns the Terraform address without the count
// specifier.
// eg. "module.ec2_dev.ec2_instance.dev[0]" becomes "module.ec2_dev.ec2_instance.dev"
Expand Down
Loading