Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ exitCode: 1
image: gcr.io/kpt-fn/dne # non-existing image
args:
namespace: staging
stdErr: "error: function image \"gcr.io/kpt-fn/dne\" doesn't exist"
stdErr: 'Function image "gcr.io/kpt-fn/dne" doesn''t exist'
stdOut: |
[RUNNING] "gcr.io/kpt-fn/dne"
[FAIL] "gcr.io/kpt-fn/dne"
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,4 @@
# limitations under the License.

exitCode: 1
stdErr: 'Error: invalid pipeline: function "gcr.io/kpt-fn/set-labels:unstable": functionConfig must be a valid KRM resource with `apiVersion` and `kind` fields'
stdErr: "functionConfig must be a valid KRM resource with `apiVersion` and `kind` fields"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ideally, kpt should be able to refer to the functionConfig file that has this error, but that will require associating error with the location in Kptfile etc and is a bit non-trivial to do. But the previous error contained function name, that was helpful in narrow down which functionConfig the error is referring to. so may be just prepend Function image "gcr.io/....": to the new error message ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The message is in Reason of the error output. There is a Field value in the output which is a path to the field which has issue.

Field: `pipeline.mutators[1].config`

IMO this is more accurate.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, my bad, I forgot about the complete CLI out. I thought this is what the user will see. Thanks

Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
# limitations under the License.

exitCode: 1
stdErr: "Error: function image \"gcr.io/kpt-fn/set-labels1\" doesn't exist"
stdErr: 'Error: Function image "gcr.io/kpt-fn/dne" doesn''t exist'
stdOut: |
[RUNNING] "gcr.io/kpt-fn/set-namespace:unstable"
[PASS] "gcr.io/kpt-fn/set-namespace:unstable"
4 changes: 2 additions & 2 deletions e2e/testdata/fn-render/missing-fn-image/Kptfile
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@ pipeline:
- image: gcr.io/kpt-fn/set-namespace:unstable
configMap:
namespace: staging
- image: gcr.io/kpt-fn/set-labels1 # non-existing image
- image: gcr.io/kpt-fn/dne # non-existing image
configMap:
tier: backend
tier: backend
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,4 @@
# limitations under the License.

exitCode: 1
stdErr: "following fields are mutually exclusive: 'config', 'configMap', 'configPath'. Got \"configPath, configMap\""
stdErr: 'only one of ''config'', ''configMap'', ''configPath'' can be specified. Got "configPath, configMap"'
41 changes: 41 additions & 0 deletions internal/errors/resolver/container.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// Copyright 2021 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package resolver

import (
"errors"

"github.com/GoogleContainerTools/kpt/internal/fnruntime"
)

//nolint:gochecknoinits
func init() {
AddErrorResolver(&containerImageErrorResolver{})
}

// containerImageErrorResolver is an implementation of the ErrorResolver interface
// to resolve container image errors.
type containerImageErrorResolver struct{}

func (*containerImageErrorResolver) Resolve(err error) (ResolvedResult, bool) {
var containerImageError *fnruntime.ContainerImageError
if !errors.As(err, &containerImageError) {
return ResolvedResult{}, false
}
return ResolvedResult{
Message: containerImageError.Error(),
ExitCode: 1,
}, true
}
8 changes: 8 additions & 0 deletions internal/errors/resolver/pkg.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (

"github.com/GoogleContainerTools/kpt/internal/errors"
"github.com/GoogleContainerTools/kpt/internal/pkg"
kptfile "github.com/GoogleContainerTools/kpt/pkg/api/kptfile/v1alpha2"
)

//nolint:gochecknoinits
Expand All @@ -44,6 +45,7 @@ type pkgErrorResolver struct{}

func (*pkgErrorResolver) Resolve(err error) (ResolvedResult, bool) {
var kptfileError *pkg.KptfileError
var validateError *kptfile.ValidateError
if errors.As(err, &kptfileError) {
path := kptfileError.Path
tmplArgs := map[string]interface{}{
Expand All @@ -63,6 +65,12 @@ func (*pkgErrorResolver) Resolve(err error) (ResolvedResult, bool) {
ExitCode: 1,
}, true
}
if errors.As(err, &validateError) {
return ResolvedResult{
Message: validateError.Error(),
ExitCode: 1,
}, true
}

return ResolvedResult{}, false
}
23 changes: 20 additions & 3 deletions internal/fnruntime/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,10 @@ func (f *ContainerFn) prepareImage() error {
var output []byte
var err error
if output, err = cmd.CombinedOutput(); err != nil {
return fmt.Errorf("failed to check local function image %q: %w", f.Image, err)
return &ContainerImageError{
Image: f.Image,
Output: string(output),
}
}
if strings.Contains(string(output), strings.Split(f.Image, ":")[0]) {
// image exists locally
Expand All @@ -178,8 +181,22 @@ func (f *ContainerFn) prepareImage() error {
ctx, cancel := context.WithTimeout(context.Background(), timeout)
defer cancel()
cmd = exec.CommandContext(ctx, dockerBin, args...)
if _, err := cmd.CombinedOutput(); err != nil {
return fmt.Errorf("function image %q doesn't exist", f.Image)
if output, err := cmd.CombinedOutput(); err != nil {
return &ContainerImageError{
Image: f.Image,
Output: string(output),
}
}
return nil
}

// ContainerImageError is an error type which will be returned when
// the container run time cannot verify docker image.
type ContainerImageError struct {
Image string
Output string
}

func (e *ContainerImageError) Error() string {
return fmt.Sprintf("Function image %q doesn't exist.", e.Image)
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just a note: Eventually, we should develop our own little abstraction containerClient so that all docker interaction can be in one place.

14 changes: 14 additions & 0 deletions internal/util/addmergecomment/addmergecomment.go
Original file line number Diff line number Diff line change
@@ -1,3 +1,17 @@
// Copyright 2021 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package addmergecomment

import (
Expand Down
14 changes: 14 additions & 0 deletions internal/util/addmergecomment/addmergecomment_test.go
Original file line number Diff line number Diff line change
@@ -1,3 +1,17 @@
// Copyright 2021 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package addmergecomment

import (
Expand Down
62 changes: 50 additions & 12 deletions pkg/api/kptfile/v1alpha2/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,29 +41,41 @@ func (p *Pipeline) validate() error {
if p == nil {
return nil
}
fns := []Function{}
fns = append(fns, p.Mutators...)
fns = append(fns, p.Validators...)
for i := range fns {
f := fns[i]
err := f.validate()
for i := range p.Mutators {
f := p.Mutators[i]
err := f.validate("mutators", i)
if err != nil {
return fmt.Errorf("function %q: %w", f.Image, err)
}
Comment thread
Shell32-Natsu marked this conversation as resolved.
}
for i := range p.Validators {
f := p.Validators[i]
err := f.validate("validators", i)
if err != nil {
return fmt.Errorf("function %q: %w", f.Image, err)
}
}
return nil
}

func (f *Function) validate() error {
func (f *Function) validate(fnType string, idx int) error {
err := validateFunctionName(f.Image)
if err != nil {
return fmt.Errorf("'image' is invalid: %w", err)
return &ValidateError{
Field: fmt.Sprintf("pipeline.%s[%d].image", fnType, idx),
Value: f.Image,
Reason: err.Error(),
}
}

var configFields []string
if f.ConfigPath != "" {
if err := validatePath(f.ConfigPath); err != nil {
return fmt.Errorf("'configPath' %q is invalid: %w", f.ConfigPath, err)
return &ValidateError{
Field: fmt.Sprintf("pipeline.%s[%d].configPath", fnType, idx),
Value: f.ConfigPath,
Reason: err.Error(),
}
}
configFields = append(configFields, "configPath")
}
Expand All @@ -73,13 +85,19 @@ func (f *Function) validate() error {
if !IsNodeZero(&f.Config) {
config := yaml.NewRNode(&f.Config)
if _, err := config.GetMeta(); err != nil {
return fmt.Errorf("functionConfig must be a valid KRM resource with `apiVersion` and `kind` fields")
return &ValidateError{
Field: fmt.Sprintf("pipeline.%s[%d].config", fnType, idx),
Reason: "functionConfig must be a valid KRM resource with `apiVersion` and `kind` fields",
}
}
configFields = append(configFields, "config")
}
if len(configFields) > 1 {
return fmt.Errorf("following fields are mutually exclusive: 'config', 'configMap', 'configPath'. Got %q",
strings.Join(configFields, ", "))
return &ValidateError{
Field: fmt.Sprintf("pipeline.%s[%d]", fnType, idx),
Reason: fmt.Sprintf("only one of 'config', 'configMap', 'configPath' can be specified. Got %q",
strings.Join(configFields, ", ")),
}
}

return nil
Expand Down Expand Up @@ -148,3 +166,23 @@ func validatePath(p string) error {
}
return nil
}

// ValidateError is the error returned when validation fails.
type ValidateError struct {
// Field is the field that causes error
Field string
// Value is the value of invalid field
Value string
// Reason is the reson for the error
Reason string
}

func (e *ValidateError) Error() string {
var sb strings.Builder
sb.WriteString(fmt.Sprintf("Kptfile is invalid:\nField: `%s`\n", e.Field))
if e.Value != "" {
sb.WriteString(fmt.Sprintf("Value: %q\n", e.Value))
}
sb.WriteString(fmt.Sprintf("Reason: %s\n", e.Reason))
return sb.String()
}