Skip to content

Commit 39103b3

Browse files
yangcao77yangcao77
authored
validator checks import attributes (#420)
* checks for import reference attributes in validtor Signed-off-by: Stephanie <[email protected]> * update go format Signed-off-by: Stephanie <[email protected]> * address some review comments Signed-off-by: Stephanie <[email protected]> * check for attribute and print out all default command and it's import reference if any Signed-off-by: Stephanie <[email protected]> * update the attribute keys to use prefix: api.devfile.io Signed-off-by: Stephanie <[email protected]> * use string join to join & split errors Signed-off-by: Stephanie <[email protected]>
1 parent 37a9d1e commit 39103b3

File tree

10 files changed

+224
-73
lines changed

10 files changed

+224
-73
lines changed

‎pkg/validation/commands.go

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ func ValidateCommands(commands []v1alpha2.Command, components []v1alpha2.Compone
2525
parentCommands := make(map[string]string)
2626
err = validateCommand(command, parentCommands, commandMap, components)
2727
if err != nil {
28-
return err
28+
return resolveErrorMessageWithImportAttributes(err, command.Attributes)
2929
}
3030

3131
commandGroup := getGroup(command)
@@ -34,46 +34,47 @@ func ValidateCommands(commands []v1alpha2.Command, components []v1alpha2.Compone
3434
}
3535
}
3636

37-
groupErrors := ""
37+
var groupErrorsList []string
3838
for groupKind, commands := range groupKindCommandMap {
3939
if err = validateGroup(commands); err != nil {
40-
groupErrors += fmt.Sprintf("\ncommand group %s error - %s", groupKind, err.Error())
40+
groupErrorsList = append(groupErrorsList, fmt.Sprintf("command group %s error - %s", groupKind, err.Error()))
4141
}
4242
}
4343

44-
if len(groupErrors) > 0 {
45-
err = fmt.Errorf("%s", groupErrors)
44+
if len(groupErrorsList) > 0 {
45+
groupErrors := strings.Join(groupErrorsList, "\n")
46+
err = fmt.Errorf("\n%s", groupErrors)
4647
}
4748

4849
return err
4950
}
5051

5152
// validateCommand validates a given devfile command where parentCommands is a map to track all the parent commands when validating
5253
// the composite command's subcommands recursively and devfileCommands is a map of command id to the devfile command
53-
func validateCommand(command v1alpha2.Command, parentCommands map[string]string, devfileCommands map[string]v1alpha2.Command, components []v1alpha2.Component) (err error) {
54+
func validateCommand(command v1alpha2.Command, parentCommands map[string]string, devfileCommands map[string]v1alpha2.Command, components []v1alpha2.Component) error {
5455

5556
switch {
5657
case command.Composite != nil:
5758
return validateCompositeCommand(&command, parentCommands, devfileCommands, components)
5859
case command.Exec != nil || command.Apply != nil:
5960
return validateCommandComponent(command, components)
6061
default:
61-
err = fmt.Errorf("command %s type is invalid", command.Id)
62+
return &InvalidCommandTypeError{commandId: command.Id}
6263
}
6364

64-
return err
6565
}
6666

6767
// validateGroup validates commands belonging to a specific group kind. If there are multiple commands belonging to the same group:
6868
// 1. without any default, err out
6969
// 2. with more than one default, err out
7070
func validateGroup(commands []v1alpha2.Command) error {
7171
defaultCommandCount := 0
72-
72+
var defaultCommands []v1alpha2.Command
7373
if len(commands) > 1 {
7474
for _, command := range commands {
7575
if getGroup(command).IsDefault {
7676
defaultCommandCount++
77+
defaultCommands = append(defaultCommands, command)
7778
}
7879
}
7980
} else {
@@ -83,7 +84,15 @@ func validateGroup(commands []v1alpha2.Command) error {
8384
if defaultCommandCount == 0 {
8485
return fmt.Errorf("there should be exactly one default command, currently there is no default command")
8586
} else if defaultCommandCount > 1 {
86-
return fmt.Errorf("there should be exactly one default command, currently there is more than one default command")
87+
var commandsReferenceList []string
88+
for _, command := range defaultCommands {
89+
commandsReferenceList = append(commandsReferenceList,
90+
resolveErrorMessageWithImportAttributes(fmt.Errorf("command: %s", command.Id), command.Attributes).Error())
91+
}
92+
commandsReference := strings.Join(commandsReferenceList, "; ")
93+
// example: there should be exactly one default command, currently there is more than one default command;
94+
// command: <id1>; command: <id2>, imported from uri: http://127.0.0.1:8080, in parent overrides from main devfile"
95+
return fmt.Errorf("there should be exactly one default command, currently there is more than one default command; %s", commandsReference)
8796
}
8897

8998
return nil

‎pkg/validation/commands_test.go

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package validation
22

33
import (
4+
"github.com/devfile/api/v2/pkg/attributes"
45
"testing"
56

67
"github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
@@ -30,9 +31,10 @@ func generateDummyExecCommand(name, component string, group *v1alpha2.CommandGro
3031
}
3132

3233
// generateDummyExecCommand returns a dummy apply command for testing
33-
func generateDummyApplyCommand(name, component string, group *v1alpha2.CommandGroup) v1alpha2.Command {
34+
func generateDummyApplyCommand(name, component string, group *v1alpha2.CommandGroup, cmdAttributes attributes.Attributes) v1alpha2.Command {
3435
return v1alpha2.Command{
35-
Id: name,
36+
Attributes: cmdAttributes,
37+
Id: name,
3638
CommandUnion: v1alpha2.CommandUnion{
3739
Apply: &v1alpha2.ApplyCommand{
3840
LabeledCommand: v1alpha2.LabeledCommand{
@@ -76,6 +78,10 @@ func TestValidateCommands(t *testing.T) {
7678
multipleDefaultCmdErr := ".*there should be exactly one default command, currently there is more than one default command"
7779
invalidCmdErr := ".*command does not map to a container component"
7880

81+
parentOverridesFromMainDevfile := attributes.Attributes{}.PutString(ImportSourceAttribute,
82+
"uri: http://127.0.0.1:8080").PutString(ParentOverrideAttribute, "main devfile")
83+
invalidCmdErrWithImportAttributes := ".*command does not map to a container component, imported from uri: http://127.0.0.1:8080, in parent overrides from main devfile"
84+
7985
tests := []struct {
8086
name string
8187
commands []v1alpha2.Command
@@ -131,7 +137,7 @@ func TestValidateCommands(t *testing.T) {
131137
{
132138
name: "Invalid Apply command with wrong component",
133139
commands: []v1alpha2.Command{
134-
generateDummyApplyCommand("command", "invalidComponent", nil),
140+
generateDummyApplyCommand("command", "invalidComponent", nil, attributes.Attributes{}),
135141
},
136142
wantErr: &invalidCmdErr,
137143
},
@@ -144,6 +150,13 @@ func TestValidateCommands(t *testing.T) {
144150
generateDummyCompositeCommand("composite-a", []string{"basic-exec"}, nil),
145151
},
146152
},
153+
{
154+
name: "Invalid command with import source attribute",
155+
commands: []v1alpha2.Command{
156+
generateDummyApplyCommand("command", "invalidComponent", nil, parentOverridesFromMainDevfile),
157+
},
158+
wantErr: &invalidCmdErrWithImportAttributes,
159+
},
147160
}
148161
for _, tt := range tests {
149162
t.Run(tt.name, func(t *testing.T) {
@@ -193,11 +206,11 @@ func TestValidateCommandComponent(t *testing.T) {
193206
},
194207
{
195208
name: "Valid Apply Command",
196-
command: generateDummyApplyCommand("command", component, nil),
209+
command: generateDummyApplyCommand("command", component, nil, attributes.Attributes{}),
197210
},
198211
{
199212
name: "Invalid Apply Command with wrong component",
200-
command: generateDummyApplyCommand("command", invalidComponent, &v1alpha2.CommandGroup{Kind: runGroup}),
213+
command: generateDummyApplyCommand("command", invalidComponent, &v1alpha2.CommandGroup{Kind: runGroup}, attributes.Attributes{}),
201214
wantErr: &invalidCmdErr,
202215
},
203216
}
@@ -307,7 +320,13 @@ func TestValidateGroup(t *testing.T) {
307320
component := "alias1"
308321

309322
noDefaultCmdErr := ".*there should be exactly one default command, currently there is no default command"
310-
multipleDefaultCmdErr := ".*there should be exactly one default command, currently there is more than one default command"
323+
multipleDefaultError := ".*there should be exactly one default command, currently there is more than one default command"
324+
multipleDefaultCmdErr := multipleDefaultError + "; command: run command; command: customcommand"
325+
326+
parentOverridesFromMainDevfile := attributes.Attributes{}.PutString(ImportSourceAttribute,
327+
"uri: http://127.0.0.1:8080").PutString(ParentOverrideAttribute, "main devfile")
328+
multipleDefaultCmdErrWithImportAttributes := multipleDefaultError +
329+
"; command: run command; command: customcommand, imported from uri: http://127.0.0.1:8080, in parent overrides from main devfile"
311330

312331
tests := []struct {
313332
name string
@@ -322,6 +341,14 @@ func TestValidateGroup(t *testing.T) {
322341
},
323342
wantErr: &multipleDefaultCmdErr,
324343
},
344+
{
345+
name: "Two default run commands with import source attribute",
346+
commands: []v1alpha2.Command{
347+
generateDummyExecCommand("run command", component, &v1alpha2.CommandGroup{Kind: runGroup, IsDefault: true}),
348+
generateDummyApplyCommand("customcommand", component, &v1alpha2.CommandGroup{Kind: runGroup, IsDefault: true}, parentOverridesFromMainDevfile),
349+
},
350+
wantErr: &multipleDefaultCmdErrWithImportAttributes,
351+
},
325352
{
326353
name: "No default for more than one build commands",
327354
commands: []v1alpha2.Command{

‎pkg/validation/components.go

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package validation
22

33
import (
44
"fmt"
5+
"strings"
56

67
"github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
78
"k8s.io/apimachinery/pkg/api/resource"
@@ -26,6 +27,7 @@ func ValidateComponents(components []v1alpha2.Component) error {
2627
processedVolumeMounts := make(map[string][]string)
2728
processedEndPointName := make(map[string]bool)
2829
processedEndPointPort := make(map[int]bool)
30+
processedComponentWithVolumeMounts := make(map[string]v1alpha2.Component)
2931

3032
err := v1alpha2.CheckDuplicateKeys(components)
3133
if err != nil {
@@ -38,6 +40,7 @@ func ValidateComponents(components []v1alpha2.Component) error {
3840
// Process all the volume mounts in container components to validate them later
3941
for _, volumeMount := range component.Container.VolumeMounts {
4042
processedVolumeMounts[component.Name] = append(processedVolumeMounts[component.Name], volumeMount.Name)
43+
processedComponentWithVolumeMounts[component.Name] = component
4144

4245
}
4346

@@ -52,7 +55,7 @@ func ValidateComponents(components []v1alpha2.Component) error {
5255

5356
err := validateEndpoints(component.Container.Endpoints, processedEndPointPort, processedEndPointName)
5457
if err != nil {
55-
return err
58+
return resolveErrorMessageWithImportAttributes(err, component.Attributes)
5659
}
5760
case component.Volume != nil:
5861
processedVolumes[component.Name] = true
@@ -61,54 +64,58 @@ func ValidateComponents(components []v1alpha2.Component) error {
6164
// express storage in Kubernetes. For reference, you may check doc
6265
// https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/
6366
if _, err := resource.ParseQuantity(component.Volume.Size); err != nil {
64-
return &InvalidVolumeError{name: component.Name, reason: fmt.Sprintf("size %s for volume component is invalid, %v. Example - 2Gi, 1024Mi", component.Volume.Size, err)}
67+
invalidVolErr := &InvalidVolumeError{name: component.Name, reason: fmt.Sprintf("size %s for volume component is invalid, %v. Example - 2Gi, 1024Mi", component.Volume.Size, err)}
68+
return resolveErrorMessageWithImportAttributes(invalidVolErr, component.Attributes)
6569
}
6670
}
6771
case component.Openshift != nil:
6872
if component.Openshift.Uri != "" {
6973
err := ValidateURI(component.Openshift.Uri)
7074
if err != nil {
71-
return err
75+
return resolveErrorMessageWithImportAttributes(err, component.Attributes)
7276
}
7377
}
7478

7579
err := validateEndpoints(component.Openshift.Endpoints, processedEndPointPort, processedEndPointName)
7680
if err != nil {
77-
return err
81+
return resolveErrorMessageWithImportAttributes(err, component.Attributes)
7882
}
7983
case component.Kubernetes != nil:
8084
if component.Kubernetes.Uri != "" {
8185
err := ValidateURI(component.Kubernetes.Uri)
8286
if err != nil {
83-
return err
87+
return resolveErrorMessageWithImportAttributes(err, component.Attributes)
8488
}
8589
}
8690
err := validateEndpoints(component.Kubernetes.Endpoints, processedEndPointPort, processedEndPointName)
8791
if err != nil {
88-
return err
92+
return resolveErrorMessageWithImportAttributes(err, component.Attributes)
8993
}
9094
case component.Plugin != nil:
9195
if component.Plugin.RegistryUrl != "" {
9296
err := ValidateURI(component.Plugin.RegistryUrl)
9397
if err != nil {
94-
return err
98+
return resolveErrorMessageWithImportAttributes(err, component.Attributes)
9599
}
96100
}
97101
}
98102

99103
}
100104

101105
// Check if the volume mounts mentioned in the containers are referenced by a volume component
102-
var invalidVolumeMountsErr string
106+
var invalidVolumeMountsErrList []string
103107
for componentName, volumeMountNames := range processedVolumeMounts {
104108
for _, volumeMountName := range volumeMountNames {
105109
if !processedVolumes[volumeMountName] {
106-
invalidVolumeMountsErr += fmt.Sprintf("\nvolume mount %s belonging to the container component %s", volumeMountName, componentName)
110+
missingVolumeMountErr := fmt.Errorf("volume mount %s belonging to the container component %s", volumeMountName, componentName)
111+
newErr := resolveErrorMessageWithImportAttributes(missingVolumeMountErr, processedComponentWithVolumeMounts[componentName].Attributes)
112+
invalidVolumeMountsErrList = append(invalidVolumeMountsErrList, newErr.Error())
107113
}
108114
}
109115
}
110116

111-
if len(invalidVolumeMountsErr) > 0 {
117+
if len(invalidVolumeMountsErrList) > 0 {
118+
invalidVolumeMountsErr := fmt.Sprintf("\n%s", strings.Join(invalidVolumeMountsErrList, "\n"))
112119
return &MissingVolumeMountError{errMsg: invalidVolumeMountsErr}
113120
}
114121

‎pkg/validation/components_test.go

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package validation
22

33
import (
4+
"github.com/devfile/api/v2/pkg/attributes"
45
"testing"
56

67
"github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
@@ -79,10 +80,11 @@ func generateDummyKubernetesComponent(name string, endpoints []v1alpha2.Endpoint
7980
}
8081

8182
// generateDummyPluginComponent returns a dummy Plugin component for testing
82-
func generateDummyPluginComponent(name, url string) v1alpha2.Component {
83+
func generateDummyPluginComponent(name, url string, compAttribute attributes.Attributes) v1alpha2.Component {
8384

8485
return v1alpha2.Component{
85-
Name: name,
86+
Attributes: compAttribute,
87+
Name: name,
8688
ComponentUnion: v1alpha2.ComponentUnion{
8789
Plugin: &v1alpha2.PluginComponent{
8890
ImportReference: v1alpha2.ImportReference{
@@ -137,6 +139,10 @@ func TestValidateComponents(t *testing.T) {
137139
sameTargetPortErr := "devfile contains multiple containers with same TargetPort.*"
138140
invalidURIErr := ".*invalid URI for request"
139141

142+
pluginOverridesFromMainDevfile := attributes.Attributes{}.PutString(ImportSourceAttribute,
143+
"uri: http://127.0.0.1:8080").PutString(PluginOverrideAttribute, "main devfile")
144+
invalidURIErrWithImportAttributes := ".*invalid URI for request, imported from uri: http://127.0.0.1:8080, in plugin overrides from main devfile"
145+
140146
tests := []struct {
141147
name string
142148
components []v1alpha2.Component
@@ -233,10 +239,17 @@ func TestValidateComponents(t *testing.T) {
233239
{
234240
name: "Invalid plugin registry url",
235241
components: []v1alpha2.Component{
236-
generateDummyPluginComponent("abc", "http//invalidregistryurl"),
242+
generateDummyPluginComponent("abc", "http//invalidregistryurl", attributes.Attributes{}),
237243
},
238244
wantErr: &invalidURIErr,
239245
},
246+
{
247+
name: "Invalid component due to bad URI with import source attributes",
248+
components: []v1alpha2.Component{
249+
generateDummyPluginComponent("abc", "http//invalidregistryurl", pluginOverridesFromMainDevfile),
250+
},
251+
wantErr: &invalidURIErrWithImportAttributes,
252+
},
240253
}
241254
for _, tt := range tests {
242255
t.Run(tt.name, func(t *testing.T) {

‎pkg/validation/errors.go

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
package validation
22

3-
import "fmt"
3+
import (
4+
"fmt"
5+
attributesAPI "github.com/devfile/api/v2/pkg/attributes"
6+
)
47

58
// InvalidEventError returns an error if the devfile event type has invalid events
69
type InvalidEventError struct {
@@ -22,6 +25,15 @@ func (e *InvalidCommandError) Error() string {
2225
return fmt.Sprintf("the command %q is invalid - %s", e.commandId, e.reason)
2326
}
2427

28+
// InvalidCommandError returns an error if the command is invalid
29+
type InvalidCommandTypeError struct {
30+
commandId string
31+
}
32+
33+
func (e *InvalidCommandTypeError) Error() string {
34+
return fmt.Sprintf("command %s has invalid type", e.commandId)
35+
}
36+
2537
// ReservedEnvError returns an error if the user attempts to customize a reserved ENV in a container
2638
type ReservedEnvError struct {
2739
componentName string
@@ -77,3 +89,35 @@ type InvalidComponentError struct {
7789
func (e *InvalidComponentError) Error() string {
7890
return fmt.Sprintf("the component %q is invalid - %s", e.componentName, e.reason)
7991
}
92+
93+
// resolveErrorMessageWithImportAttributes returns an updated error message
94+
// with detailed information on the imported and overriden resource.
95+
// example:
96+
// "the component <compName> is invalid - <reason>, imported from Uri: http://example.com/devfile.yaml, in parent overrides from main devfile"
97+
func resolveErrorMessageWithImportAttributes(validationErr error, attributes attributesAPI.Attributes) error {
98+
var findKeyErr error
99+
importReference := attributes.Get(ImportSourceAttribute, &findKeyErr)
100+
101+
// overridden element must contain import resource information
102+
// an overridden element can be either parentOverride or pluginOverride
103+
// example:
104+
// if an element is imported from another devfile, but contains no overrides - ImportSourceAttribute
105+
// if an element is from parentOverride - ImportSourceAttribute + ParentOverrideAttribute
106+
// if an element is from pluginOverride - ImportSourceAttribute + PluginOverrideAttribute
107+
if findKeyErr == nil {
108+
validationErr = fmt.Errorf("%s, imported from %s", validationErr.Error(), importReference)
109+
parentOverrideReference := attributes.Get(ParentOverrideAttribute, &findKeyErr)
110+
if findKeyErr == nil {
111+
validationErr = fmt.Errorf("%s, in parent overrides from %s", validationErr.Error(), parentOverrideReference)
112+
} else {
113+
// reset findKeyErr to nil
114+
findKeyErr = nil
115+
pluginOverrideReference := attributes.Get(PluginOverrideAttribute, &findKeyErr)
116+
if findKeyErr == nil {
117+
validationErr = fmt.Errorf("%s, in plugin overrides from %s", validationErr.Error(), pluginOverrideReference)
118+
}
119+
}
120+
}
121+
122+
return validationErr
123+
}

0 commit comments

Comments
 (0)