-
Notifications
You must be signed in to change notification settings - Fork 3
feat: ValidatePrebuilds to validate presets for prebuild use #194
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
base: main
Are you sure you want to change the base?
Changes from all commits
bbc1114
1ff03fc
e65bdd7
176b0c5
aafd6e4
c6db0b6
13f6911
3400cff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,34 @@ | ||
| package extract | ||
|
|
||
| import ( | ||
| "fmt" | ||
|
|
||
| "github.com/aquasecurity/trivy/pkg/iac/terraform" | ||
| "github.com/coder/preview/types" | ||
| "github.com/hashicorp/hcl/v2" | ||
|
|
||
| "github.com/coder/preview/types" | ||
| ) | ||
|
|
||
| func PresetFromBlock(block *terraform.Block) types.Preset { | ||
| func PresetFromBlock(block *terraform.Block) (tfPreset types.Preset) { | ||
| defer func() { | ||
| // Extra safety mechanism to ensure that if a panic occurs, we do not break | ||
| // everything else. | ||
| if r := recover(); r != nil { | ||
| tfPreset = types.Preset{ | ||
| PresetData: types.PresetData{ | ||
| Name: block.Label(), | ||
| }, | ||
| Diagnostics: types.Diagnostics{ | ||
| { | ||
| Severity: hcl.DiagError, | ||
| Summary: "Panic occurred in extracting preset. This should not happen, please report this to Coder.", | ||
| Detail: fmt.Sprintf("panic in preset extract: %+v", r), | ||
| }, | ||
| }, | ||
| } | ||
| } | ||
| }() | ||
|
Comment on lines
+13
to
+30
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is just a safety for when we use |
||
|
|
||
| p := types.Preset{ | ||
| PresetData: types.PresetData{ | ||
| Parameters: make(map[string]string), | ||
|
|
@@ -41,5 +63,13 @@ func PresetFromBlock(block *terraform.Block) types.Preset { | |
| p.Default = defaultAttr.Value().True() | ||
| } | ||
|
|
||
| prebuildBlock := block.GetBlock("prebuilds") | ||
| if prebuildBlock != nil { | ||
| p.Prebuilds = &types.PrebuildData{ | ||
| // Invalid values will be set to 0 | ||
| Instances: int(optionalInteger(prebuildBlock, "instances")), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I want to make sure I understand how preview and terraform-provider interact. IIUC, when updating a template via the UI and hitting build, preview runs first (static HCL analysis), then terraform plan/apply runs with terraform-provider validation. We already validate
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes this is correct. The static HCL analysis is also a "best guess" and I try to error on the permissive side when things cannot be determined. The terraform provider should always be a more restrictive validation.
Yes, and that should be the source of truth for validation.
Exactly. If the instance count is So this function is a check that says for all presets that will be used in prebuilds, are their values completely valid for a workspace build? (without any additional user input) |
||
| } | ||
| } | ||
|
|
||
| return p | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,6 +68,57 @@ func (o Output) MarshalJSON() ([]byte, error) { | |
| }) | ||
| } | ||
|
|
||
| // ValidatePrebuilds will iterate over each preset, validate the inputs as a set, | ||
| // and attach any diagnostics to the preset if there are issues. This must be done | ||
| // because prebuilds have to be valid without user input. | ||
| // | ||
| // This will only validate presets that have prebuilds configured and have no | ||
| // existing error diagnostics. This should only be used when doing Template | ||
| // Imports as a protection against invalid presets. A template can still be | ||
| // usable if it's presets are invalid. It just cannot be used for prebuilds. | ||
| func ValidatePrebuilds(ctx context.Context, input Input, preValid []types.Preset, dir fs.FS) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will only be executed on template import, right?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. I will add that to the comment |
||
| for i := range preValid { | ||
| pre := &preValid[i] | ||
| if pre.Prebuilds == nil || pre.Prebuilds.Instances <= 0 { | ||
| // No prebuilds, so presets do not need to be valid without user input | ||
| continue | ||
| } | ||
|
|
||
| if hcl.Diagnostics(pre.Diagnostics).HasErrors() { | ||
| // Piling on diagnostics is not helpful, if an error exists, leave it at that. | ||
| continue | ||
| } | ||
|
|
||
| // Diagnostics are added to the existing preset. | ||
| input.ParameterValues = pre.Parameters | ||
| output, diagnostics := Preview(ctx, input, dir) | ||
| if diagnostics.HasErrors() { | ||
| pre.Diagnostics = append(pre.Diagnostics, diagnostics...) | ||
| // Do not pile on more diagnostics for individual params, it already failed | ||
| continue | ||
| } | ||
|
|
||
| if output == nil { | ||
| continue | ||
| } | ||
|
|
||
| // If any parameter is invalid, then the preset is invalid. | ||
| // A value must be specified for this failing parameter. | ||
| for _, param := range output.Parameters { | ||
| if hcl.Diagnostics(param.Diagnostics).HasErrors() { | ||
| for _, paramDiag := range param.Diagnostics { | ||
| if paramDiag.Severity != hcl.DiagError { | ||
| continue // Only care about errors here | ||
| } | ||
| orig := paramDiag.Summary | ||
| paramDiag.Summary = fmt.Sprintf("Parameter %s: %s", param.Name, orig) | ||
| pre.Diagnostics = append(pre.Diagnostics, paramDiag) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func Preview(ctx context.Context, input Input, dir fs.FS) (output *Output, diagnostics hcl.Diagnostics) { | ||
| // The trivy package works with `github.com/zclconf/go-cty`. This package is | ||
| // similar to `reflect` in its usage. This package can panic if types are | ||
|
|
@@ -180,7 +231,9 @@ func Preview(ctx context.Context, input Input, dir fs.FS) (output *Output, diagn | |
|
|
||
| diags := make(hcl.Diagnostics, 0) | ||
| rp, rpDiags := parameters(modules) | ||
| presets := presets(modules, rp) | ||
| // preValidPresets are extracted as written in terraform. Each individual | ||
| // param value is checked, however, the preset as a whole might not be valid. | ||
| preValidPresets := presets(modules, rp) | ||
| tags, tagDiags := workspaceTags(modules, p.Files()) | ||
| vars := variables(modules) | ||
|
|
||
|
|
@@ -191,7 +244,7 @@ func Preview(ctx context.Context, input Input, dir fs.FS) (output *Output, diagn | |
| ModuleOutput: outputs, | ||
| Parameters: rp, | ||
| WorkspaceTags: tags, | ||
| Presets: presets, | ||
| Presets: preValidPresets, | ||
| Files: p.Files(), | ||
| Variables: vars, | ||
| }, diags.Extend(rpDiags).Extend(tagDiags) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,12 +41,13 @@ func Test_Extract(t *testing.T) { | |
| failPreview bool | ||
| input preview.Input | ||
|
|
||
| expTags map[string]string | ||
| unknownTags []string | ||
| params map[string]assertParam | ||
| variables map[string]assertVariable | ||
| presets func(t *testing.T, presets []types.Preset) | ||
| warnings []*regexp.Regexp | ||
| expTags map[string]string | ||
| unknownTags []string | ||
| params map[string]assertParam | ||
| variables map[string]assertVariable | ||
| presetsFuncs func(t *testing.T, presets []types.Preset) | ||
| presets map[string]assertPreset | ||
| warnings []*regexp.Regexp | ||
| }{ | ||
| { | ||
| name: "bad param values", | ||
|
|
@@ -266,6 +267,27 @@ func Test_Extract(t *testing.T) { | |
| errorDiagnostics("Required"), | ||
| }, | ||
| }, | ||
| { | ||
| name: "valid prebuild", | ||
| dir: "preset", | ||
| expTags: map[string]string{}, | ||
| input: preview.Input{}, | ||
| unknownTags: []string{}, | ||
| params: map[string]assertParam{ | ||
| "number": ap(), | ||
| "has_default": ap(), | ||
| }, | ||
| presets: map[string]assertPreset{ | ||
| "valid_preset": aPre(). | ||
| value("number", "9"). | ||
| value("has_default", "changed"). | ||
| prebuildCount(3), | ||
| "prebuild_instance_zero": aPre(). | ||
| prebuildCount(0), | ||
| "not_prebuild": aPre(). | ||
| prebuildCount(0), | ||
| }, | ||
| }, | ||
| { | ||
| name: "invalid presets", | ||
| dir: "invalidpresets", | ||
|
|
@@ -276,50 +298,14 @@ func Test_Extract(t *testing.T) { | |
| "valid_parameter_name": ap(). | ||
| optVals("valid_option_value"), | ||
| }, | ||
| presets: func(t *testing.T, presets []types.Preset) { | ||
| presetMap := map[string]func(t *testing.T, preset types.Preset){ | ||
| "empty_parameters": func(t *testing.T, preset types.Preset) { | ||
| require.Len(t, preset.Diagnostics, 0) | ||
| }, | ||
| "no_parameters": func(t *testing.T, preset types.Preset) { | ||
| require.Len(t, preset.Diagnostics, 0) | ||
| }, | ||
| "invalid_parameter_name": func(t *testing.T, preset types.Preset) { | ||
| require.Len(t, preset.Diagnostics, 1) | ||
| require.Equal(t, preset.Diagnostics[0].Summary, "Undefined Parameter") | ||
| require.Equal(t, preset.Diagnostics[0].Detail, "Preset parameter \"invalid_parameter_name\" is not defined by the template.") | ||
| }, | ||
| "invalid_parameter_value": func(t *testing.T, preset types.Preset) { | ||
| require.Len(t, preset.Diagnostics, 1) | ||
| require.Equal(t, preset.Diagnostics[0].Summary, "Value must be a valid option") | ||
| require.Equal(t, preset.Diagnostics[0].Detail, "the value \"invalid_value\" must be defined as one of options") | ||
| }, | ||
| "valid_preset": func(t *testing.T, preset types.Preset) { | ||
| require.Len(t, preset.Diagnostics, 0) | ||
| require.Equal(t, preset.Parameters, map[string]string{ | ||
| "valid_parameter_name": "valid_option_value", | ||
| }) | ||
| }, | ||
| } | ||
|
|
||
| for _, preset := range presets { | ||
| if fn, ok := presetMap[preset.Name]; ok { | ||
| fn(t, preset) | ||
| } | ||
| } | ||
|
|
||
| var defaultPresetsWithError int | ||
| for _, preset := range presets { | ||
| if preset.Name == "default_preset" || preset.Name == "another_default_preset" { | ||
| for _, diag := range preset.Diagnostics { | ||
| if diag.Summary == "Multiple default presets" { | ||
| defaultPresetsWithError++ | ||
| break | ||
| } | ||
| } | ||
| } | ||
| } | ||
| require.Equal(t, 1, defaultPresetsWithError, "exactly one default preset should have the multiple defaults error") | ||
| presets: map[string]assertPreset{ | ||
| "empty_parameters": aPre(), | ||
| "no_parameters": aPre(), | ||
| "invalid_parameter_name": aPreWithDiags().errorDiagnostics("Preset parameter \"invalid_parameter_name\" is not defined by the template."), | ||
| "invalid_parameter_value": aPreWithDiags().errorDiagnostics("the value \"invalid_value\" must be defined as one of options"), | ||
| "valid_preset": aPre().value("valid_parameter_name", "valid_option_value"), | ||
| "another_default_preset": aPre().def(true), | ||
| "default_preset": aPreWithDiags().errorDiagnostics("Only one preset can be marked as default. \"another_default_preset\" is already marked as default"), | ||
| }, | ||
| }, | ||
| { | ||
|
|
@@ -649,6 +635,9 @@ func Test_Extract(t *testing.T) { | |
| } | ||
| require.False(t, diags.HasErrors()) | ||
|
|
||
| // Validate prebuilds too | ||
| preview.ValidatePrebuilds(context.Background(), tc.input, output.Presets, dirFs) | ||
|
|
||
| if len(tc.warnings) > 0 { | ||
| for _, w := range tc.warnings { | ||
| idx := slices.IndexFunc(diags, func(diagnostic *hcl.Diagnostic) bool { | ||
|
|
@@ -684,9 +673,10 @@ func Test_Extract(t *testing.T) { | |
| check(t, param) | ||
| } | ||
|
|
||
| // Assert presets | ||
| if tc.presets != nil { | ||
| tc.presets(t, output.Presets) | ||
| for _, preset := range output.Presets { | ||
| check, ok := tc.presets[preset.Name] | ||
| require.True(t, ok, "unknown preset %s", preset.Name) | ||
| check(t, preset) | ||
| } | ||
|
|
||
| // Assert variables | ||
|
|
@@ -700,6 +690,55 @@ func Test_Extract(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| func TestPresetValidation(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| for _, tc := range []struct { | ||
| name string | ||
| dir string | ||
| input preview.Input | ||
| presetAssert map[string]assertPreset | ||
| }{ | ||
| { | ||
| name: "preset failure", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we maybe add some extra test cases here?
|
||
| dir: "presetfail", | ||
| input: preview.Input{}, | ||
| presetAssert: map[string]assertPreset{ | ||
| "invalid_parameters": aPreWithDiags(). | ||
| errorDiagnostics("Parameter no_default: Required parameter not provided"), | ||
| "valid_preset": aPre(). | ||
| value("has_default", "changed"). | ||
| value("no_default", "custom value"). | ||
| noDiagnostics(), | ||
| "prebuild_instance_zero": aPre().noDiagnostics().prebuildCount(0), | ||
| "not_prebuild": aPre().noDiagnostics().prebuildCount(0), | ||
| }, | ||
| }, | ||
| } { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| dirFs := os.DirFS(filepath.Join("testdata", tc.dir)) | ||
| output, diags := preview.Preview(context.Background(), tc.input, dirFs) | ||
| if diags.HasErrors() { | ||
| t.Logf("diags: %s", diags) | ||
| } | ||
| require.False(t, diags.HasErrors()) | ||
| require.Len(t, diags, 0) | ||
|
|
||
| preview.ValidatePrebuilds(context.Background(), tc.input, output.Presets, dirFs) | ||
| for _, preset := range output.Presets { | ||
| check, ok := tc.presetAssert[preset.Name] | ||
| require.True(t, ok, "unknown preset %s", preset.Name) | ||
| check(t, preset) | ||
| delete(tc.presetAssert, preset.Name) | ||
| } | ||
|
|
||
| require.Len(t, tc.presetAssert, 0, "some presets were not found") | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| type assertVariable func(t *testing.T, variable types.Variable) | ||
|
|
||
| func av() assertVariable { | ||
|
|
@@ -890,6 +929,81 @@ func (a assertVariable) extend(f assertVariable) assertVariable { | |
| } | ||
| } | ||
|
|
||
| type assertPreset func(t *testing.T, preset types.Preset) | ||
|
|
||
| func aPre() assertPreset { | ||
| return func(t *testing.T, parameter types.Preset) { | ||
| t.Helper() | ||
| assert.Empty(t, parameter.Diagnostics, "parameter should have no diagnostics") | ||
| } | ||
| } | ||
|
|
||
| func aPreWithDiags() assertPreset { | ||
| return func(t *testing.T, parameter types.Preset) {} | ||
| } | ||
|
|
||
| func (a assertPreset) def(def bool) assertPreset { | ||
| return a.extend(func(t *testing.T, preset types.Preset) { | ||
| require.Equal(t, def, preset.Default) | ||
| }) | ||
| } | ||
|
|
||
| func (a assertPreset) prebuildCount(exp int) assertPreset { | ||
| return a.extend(func(t *testing.T, preset types.Preset) { | ||
| if exp == 0 && preset.Prebuilds == nil { | ||
| return | ||
| } | ||
| require.NotNilf(t, preset.Prebuilds, "prebuild should not be nil, expected %d instances", exp) | ||
| require.Equal(t, exp, preset.Prebuilds.Instances) | ||
| }) | ||
| } | ||
|
|
||
| func (a assertPreset) value(key, value string) assertPreset { | ||
| return a.extend(func(t *testing.T, preset types.Preset) { | ||
| v, ok := preset.Parameters[key] | ||
| require.Truef(t, ok, "preset parameter %q existence check", key) | ||
| assert.Equalf(t, value, v, "preset parameter %q value equality check", key) | ||
| }) | ||
| } | ||
|
|
||
| func (a assertPreset) errorDiagnostics(patterns ...string) assertPreset { | ||
| return a.diagnostics(hcl.DiagError, patterns...) | ||
| } | ||
|
|
||
| func (a assertPreset) warnDiagnostics(patterns ...string) assertPreset { | ||
| return a.diagnostics(hcl.DiagWarning, patterns...) | ||
| } | ||
|
|
||
| func (a assertPreset) diagnostics(sev hcl.DiagnosticSeverity, patterns ...string) assertPreset { | ||
| shadow := patterns | ||
| return a.extend(func(t *testing.T, preset types.Preset) { | ||
| t.Helper() | ||
|
|
||
| assertDiags(t, sev, preset.Diagnostics, shadow...) | ||
| }) | ||
| } | ||
|
|
||
| func (a assertPreset) noDiagnostics() assertPreset { | ||
| return a.extend(func(t *testing.T, preset types.Preset) { | ||
| t.Helper() | ||
|
|
||
| assert.Empty(t, preset.Diagnostics, "parameter should have no diagnostics") | ||
| }) | ||
| } | ||
|
|
||
| //nolint:revive | ||
| func (a assertPreset) extend(f assertPreset) assertPreset { | ||
| if a == nil { | ||
| a = func(t *testing.T, v types.Preset) {} | ||
| } | ||
|
|
||
| return func(t *testing.T, v types.Preset) { | ||
| t.Helper() | ||
| (a)(t, v) | ||
| f(t, v) | ||
| } | ||
| } | ||
|
|
||
| func assertDiags(t *testing.T, sev hcl.DiagnosticSeverity, diags types.Diagnostics, patterns ...string) { | ||
| t.Helper() | ||
| checks := make([]string, len(patterns)) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you run into a case where this can panic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish I had receipts. I did not find a location where presets panic'd, but we've hit panics in the past when converting
cty-> normal types.Since the preset code is not tested that much, I just feel more comfortable with this safeguard in place. A panic never raises well, and we hit it a few times in the early days of parameters.
This is just being defensive.