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

validator checks import attributes #420

Merged
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
18 changes: 12 additions & 6 deletions pkg/validation/commands.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ func ValidateCommands(commands []v1alpha2.Command, components []v1alpha2.Compone
parentCommands := make(map[string]string)
err = validateCommand(command, parentCommands, commandMap, components)
if err != nil {
return err
return resolveErrorMessageWithImportAttributes(err, command.Attributes)
}

commandGroup := getGroup(command)
Expand All @@ -50,30 +50,30 @@ func ValidateCommands(commands []v1alpha2.Command, components []v1alpha2.Compone

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

switch {
case command.Composite != nil:
return validateCompositeCommand(&command, parentCommands, devfileCommands, components)
case command.Exec != nil || command.Apply != nil:
return validateCommandComponent(command, components)
default:
err = fmt.Errorf("command %s type is invalid", command.Id)
return &InvalidCommandTypeError{commandId: command.Id}
}

return err
}

// validateGroup validates commands belonging to a specific group kind. If there are multiple commands belonging to the same group:
maysunfaisal marked this conversation as resolved.
Show resolved Hide resolved
// 1. without any default, err out
// 2. with more than one default, err out
func validateGroup(commands []v1alpha2.Command) error {
defaultCommandCount := 0

var defaultCommands []v1alpha2.Command
if len(commands) > 1 {
for _, command := range commands {
if getGroup(command).IsDefault {
defaultCommandCount++
defaultCommands = append(defaultCommands, command)
}
}
} else {
Expand All @@ -83,7 +83,13 @@ func validateGroup(commands []v1alpha2.Command) error {
if defaultCommandCount == 0 {
return fmt.Errorf("there should be exactly one default command, currently there is no default command")
} else if defaultCommandCount > 1 {
return fmt.Errorf("there should be exactly one default command, currently there is more than one default command")
var commandsReference string
for _, command := range defaultCommands {
commandsReference += resolveErrorMessageWithImportAttributes(fmt.Errorf("; command: %s", command.Id), command.Attributes).Error()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think fmt.Sprintf might be cleaner here for formatting:

Suggested change
commandsReference += resolveErrorMessageWithImportAttributes(fmt.Errorf("; command: %s", command.Id), command.Attributes).Error()
commandsReference = fmt.Sprintf("%s; %s", commandsReference, resolveErrorMessageWithImportAttributes(fmt.Errorf("command: %s", command.Id), command.Attributes).Error()

or we could accumulate to a []string and use Join:

Suggested change
commandsReference += resolveErrorMessageWithImportAttributes(fmt.Errorf("; command: %s", command.Id), command.Attributes).Error()
commandsReferenceList = append(commandsReferenceList, resolveErrorMessageWithImportAttributes(fmt.Errorf("command: %s", command.Id), command.Attributes).Error())
//...
strings.Join(commandsReferenceList, "; ")

This would avoid the need to format the message below with "default command%s"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

}
maysunfaisal marked this conversation as resolved.
Show resolved Hide resolved
// example: there should be exactly one default command, currently there is more than one default command;
// command: <id1>; command: <id2>, imported from uri: http://127.0.0.1:8080, in parent overrides from main devfile"
return fmt.Errorf("there should be exactly one default command, currently there is more than one default command%s", commandsReference)
}

return nil
Expand Down
39 changes: 33 additions & 6 deletions pkg/validation/commands_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package validation

import (
"github.com/devfile/api/v2/pkg/attributes"
"testing"

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

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

parentOverridesFromMainDevfile := attributes.Attributes{}.PutString(ImportSourceAttribute,
"uri: http://127.0.0.1:8080").PutString(ParentOverrideAttribute, "main devfile")
invalidCmdErrWithImportAttributes := ".*command does not map to a container component, imported from uri: http://127.0.0.1:8080, in parent overrides from main devfile"

tests := []struct {
name string
commands []v1alpha2.Command
Expand Down Expand Up @@ -131,7 +137,7 @@ func TestValidateCommands(t *testing.T) {
{
name: "Invalid Apply command with wrong component",
commands: []v1alpha2.Command{
generateDummyApplyCommand("command", "invalidComponent", nil),
generateDummyApplyCommand("command", "invalidComponent", nil, attributes.Attributes{}),
},
wantErr: &invalidCmdErr,
},
Expand All @@ -144,6 +150,13 @@ func TestValidateCommands(t *testing.T) {
generateDummyCompositeCommand("composite-a", []string{"basic-exec"}, nil),
},
},
{
name: "Invalid command with import source attribute",
commands: []v1alpha2.Command{
maysunfaisal marked this conversation as resolved.
Show resolved Hide resolved
generateDummyApplyCommand("command", "invalidComponent", nil, parentOverridesFromMainDevfile),
},
wantErr: &invalidCmdErrWithImportAttributes,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down Expand Up @@ -193,11 +206,11 @@ func TestValidateCommandComponent(t *testing.T) {
},
{
name: "Valid Apply Command",
command: generateDummyApplyCommand("command", component, nil),
command: generateDummyApplyCommand("command", component, nil, attributes.Attributes{}),
},
{
name: "Invalid Apply Command with wrong component",
command: generateDummyApplyCommand("command", invalidComponent, &v1alpha2.CommandGroup{Kind: runGroup}),
command: generateDummyApplyCommand("command", invalidComponent, &v1alpha2.CommandGroup{Kind: runGroup}, attributes.Attributes{}),
wantErr: &invalidCmdErr,
},
}
Expand Down Expand Up @@ -307,7 +320,13 @@ func TestValidateGroup(t *testing.T) {
component := "alias1"

noDefaultCmdErr := ".*there should be exactly one default command, currently there is no default command"
multipleDefaultCmdErr := ".*there should be exactly one default command, currently there is more than one default command"
multipleDefaultError := ".*there should be exactly one default command, currently there is more than one default command"
multipleDefaultCmdErr := multipleDefaultError + "; command: run command; command: customcommand"

parentOverridesFromMainDevfile := attributes.Attributes{}.PutString(ImportSourceAttribute,
"uri: http://127.0.0.1:8080").PutString(ParentOverrideAttribute, "main devfile")
multipleDefaultCmdErrWithImportAttributes := multipleDefaultError +
"; command: run command; command: customcommand, imported from uri: http://127.0.0.1:8080, in parent overrides from main devfile"

tests := []struct {
name string
Expand All @@ -322,6 +341,14 @@ func TestValidateGroup(t *testing.T) {
},
wantErr: &multipleDefaultCmdErr,
},
{
name: "Two default run commands with import source attribute",
commands: []v1alpha2.Command{
generateDummyExecCommand("run command", component, &v1alpha2.CommandGroup{Kind: runGroup, IsDefault: true}),
generateDummyApplyCommand("customcommand", component, &v1alpha2.CommandGroup{Kind: runGroup, IsDefault: true}, parentOverridesFromMainDevfile),
},
wantErr: &multipleDefaultCmdErrWithImportAttributes,
},
{
name: "No default for more than one build commands",
commands: []v1alpha2.Command{
Expand Down
21 changes: 13 additions & 8 deletions pkg/validation/components.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ func ValidateComponents(components []v1alpha2.Component) error {
processedVolumeMounts := make(map[string][]string)
processedEndPointName := make(map[string]bool)
processedEndPointPort := make(map[int]bool)
processedComponentWithVolumeMounts := make(map[string]v1alpha2.Component)

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

}

Expand All @@ -52,7 +54,7 @@ func ValidateComponents(components []v1alpha2.Component) error {

err := validateEndpoints(component.Container.Endpoints, processedEndPointPort, processedEndPointName)
if err != nil {
return err
return resolveErrorMessageWithImportAttributes(err, component.Attributes)
}
case component.Volume != nil:
processedVolumes[component.Name] = true
Expand All @@ -61,37 +63,38 @@ func ValidateComponents(components []v1alpha2.Component) error {
// express storage in Kubernetes. For reference, you may check doc
// https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/
if _, err := resource.ParseQuantity(component.Volume.Size); err != nil {
return &InvalidVolumeError{name: component.Name, reason: fmt.Sprintf("size %s for volume component is invalid, %v. Example - 2Gi, 1024Mi", component.Volume.Size, err)}
invalidVolErr := &InvalidVolumeError{name: component.Name, reason: fmt.Sprintf("size %s for volume component is invalid, %v. Example - 2Gi, 1024Mi", component.Volume.Size, err)}
return resolveErrorMessageWithImportAttributes(invalidVolErr, component.Attributes)
}
}
case component.Openshift != nil:
if component.Openshift.Uri != "" {
err := ValidateURI(component.Openshift.Uri)
if err != nil {
return err
return resolveErrorMessageWithImportAttributes(err, component.Attributes)
}
}

err := validateEndpoints(component.Openshift.Endpoints, processedEndPointPort, processedEndPointName)
if err != nil {
return err
return resolveErrorMessageWithImportAttributes(err, component.Attributes)
}
case component.Kubernetes != nil:
if component.Kubernetes.Uri != "" {
err := ValidateURI(component.Kubernetes.Uri)
if err != nil {
return err
return resolveErrorMessageWithImportAttributes(err, component.Attributes)
}
}
err := validateEndpoints(component.Kubernetes.Endpoints, processedEndPointPort, processedEndPointName)
if err != nil {
return err
return resolveErrorMessageWithImportAttributes(err, component.Attributes)
}
case component.Plugin != nil:
if component.Plugin.RegistryUrl != "" {
err := ValidateURI(component.Plugin.RegistryUrl)
if err != nil {
return err
return resolveErrorMessageWithImportAttributes(err, component.Attributes)
}
}
}
Expand All @@ -103,7 +106,9 @@ func ValidateComponents(components []v1alpha2.Component) error {
for componentName, volumeMountNames := range processedVolumeMounts {
for _, volumeMountName := range volumeMountNames {
if !processedVolumes[volumeMountName] {
invalidVolumeMountsErr += fmt.Sprintf("\nvolume mount %s belonging to the container component %s", volumeMountName, componentName)
missingVolumeMountErr := fmt.Errorf("\nvolume mount %s belonging to the container component %s", volumeMountName, componentName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Outside the scope of this PR, but seeing fmt.Errorf("\nvolume makes me feel like MissingVolumeMountError should store errs: []string rather than errMsg: string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

newErr := resolveErrorMessageWithImportAttributes(missingVolumeMountErr, processedComponentWithVolumeMounts[componentName].Attributes)
invalidVolumeMountsErr += newErr.Error()
}
}
}
Expand Down
19 changes: 16 additions & 3 deletions pkg/validation/components_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package validation

import (
"github.com/devfile/api/v2/pkg/attributes"
"testing"

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

// generateDummyPluginComponent returns a dummy Plugin component for testing
func generateDummyPluginComponent(name, url string) v1alpha2.Component {
func generateDummyPluginComponent(name, url string, compAttribute attributes.Attributes) v1alpha2.Component {

return v1alpha2.Component{
Name: name,
Attributes: compAttribute,
Name: name,
ComponentUnion: v1alpha2.ComponentUnion{
Plugin: &v1alpha2.PluginComponent{
ImportReference: v1alpha2.ImportReference{
Expand Down Expand Up @@ -137,6 +139,10 @@ func TestValidateComponents(t *testing.T) {
sameTargetPortErr := "devfile contains multiple containers with same TargetPort.*"
invalidURIErr := ".*invalid URI for request"

pluginOverridesFromMainDevfile := attributes.Attributes{}.PutString(ImportSourceAttribute,
"uri: http://127.0.0.1:8080").PutString(PluginOverrideAttribute, "main devfile")
invalidURIErrWithImportAttributes := ".*invalid URI for request, imported from uri: http://127.0.0.1:8080, in plugin overrides from main devfile"

tests := []struct {
name string
components []v1alpha2.Component
Expand Down Expand Up @@ -233,10 +239,17 @@ func TestValidateComponents(t *testing.T) {
{
name: "Invalid plugin registry url",
components: []v1alpha2.Component{
generateDummyPluginComponent("abc", "http//invalidregistryurl"),
generateDummyPluginComponent("abc", "http//invalidregistryurl", attributes.Attributes{}),
},
wantErr: &invalidURIErr,
},
{
name: "Invalid component due to bad URI with import source attributes",
components: []v1alpha2.Component{
generateDummyPluginComponent("abc", "http//invalidregistryurl", pluginOverridesFromMainDevfile),
},
wantErr: &invalidURIErrWithImportAttributes,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down
46 changes: 45 additions & 1 deletion pkg/validation/errors.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package validation

import "fmt"
import (
"fmt"
attributesAPI "github.com/devfile/api/v2/pkg/attributes"
)

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

// InvalidCommandError returns an error if the command is invalid
type InvalidCommandTypeError struct {
commandId string
}

func (e *InvalidCommandTypeError) Error() string {
return fmt.Sprintf("command %s type is invalid", e.commandId)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return fmt.Sprintf("command %s type is invalid", e.commandId)
return fmt.Sprintf("command %s has invalid type", e.commandId)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

}

// ReservedEnvError returns an error if the user attempts to customize a reserved ENV in a container
type ReservedEnvError struct {
componentName string
Expand Down Expand Up @@ -77,3 +89,35 @@ type InvalidComponentError struct {
func (e *InvalidComponentError) Error() string {
return fmt.Sprintf("the component %q is invalid - %s", e.componentName, e.reason)
}

// resolveErrorMessageWithImportAttributes returns an updated error message
// with detailed information on the imported and overriden resource.
// example:
// "the component <compName> is invalid - <reason>, imported from Uri: http://example.com/devfile.yaml, in parent overrides from main devfile"
func resolveErrorMessageWithImportAttributes(validationErr error, attributes attributesAPI.Attributes) error {
var findKeyErr error
importReference := attributes.Get(ImportSourceAttribute, &findKeyErr)

maysunfaisal marked this conversation as resolved.
Show resolved Hide resolved
// overridden element must contain import resource information
// an overridden element can be either parentOverride or pluginOverride
// example:
// if an element is imported from another devfile, but contains no overrides - ImportSourceAttribute
// if an element is from parentOverride - ImportSourceAttribute + ParentOverrideAttribute
// if an element is from pluginOverride - ImportSourceAttribute + PluginOverrideAttribute
if findKeyErr == nil {
validationErr = fmt.Errorf("%s, imported from %s", validationErr.Error(), importReference)
parentOverrideReference := attributes.Get(ParentOverrideAttribute, &findKeyErr)
if findKeyErr == nil {
validationErr = fmt.Errorf("%s, in parent overrides from %s", validationErr.Error(), parentOverrideReference)
} else {
// reset findKeyErr to nil
findKeyErr = nil
pluginOverrideReference := attributes.Get(PluginOverrideAttribute, &findKeyErr)
if findKeyErr == nil {
validationErr = fmt.Errorf("%s, in plugin overrides from %s", validationErr.Error(), pluginOverrideReference)
}
}
}

return validationErr
}
9 changes: 5 additions & 4 deletions pkg/validation/events_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"testing"

"github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
"github.com/devfile/api/v2/pkg/attributes"
"github.com/stretchr/testify/assert"
)

Expand All @@ -12,8 +13,8 @@ func TestValidateEvents(t *testing.T) {
containers := []string{"container1", "container2"}

commands := []v1alpha2.Command{
generateDummyApplyCommand("apply1", containers[0], nil),
generateDummyApplyCommand("apply2", containers[0], nil),
generateDummyApplyCommand("apply1", containers[0], nil, attributes.Attributes{}),
generateDummyApplyCommand("apply2", containers[0], nil, attributes.Attributes{}),
generateDummyExecCommand("exec1", containers[1], nil),
generateDummyExecCommand("exec2", containers[1], nil),
generateDummyCompositeCommand("compositeOnlyApply", []string{"apply1", "apply2"}, nil),
Expand Down Expand Up @@ -112,8 +113,8 @@ func TestIsEventValid(t *testing.T) {
containers := []string{"container1", "container2"}

commands := []v1alpha2.Command{
generateDummyApplyCommand("apply1", containers[0], nil),
generateDummyApplyCommand("apply2", containers[0], nil),
generateDummyApplyCommand("apply1", containers[0], nil, attributes.Attributes{}),
generateDummyApplyCommand("apply2", containers[0], nil, attributes.Attributes{}),
generateDummyExecCommand("exec1", containers[1], nil),
generateDummyExecCommand("exec2", containers[1], nil),
generateDummyCompositeCommand("compositeOnlyApply", []string{"apply1", "apply2"}, nil),
Expand Down
Loading