Skip to content

Commit

Permalink
Give better message to user for MissingValueAtPath and OutputPathNotS…
Browse files Browse the repository at this point in the history
…atisfied (#406)
  • Loading branch information
emmjohnson authored Nov 29, 2021
1 parent a9d8ec0 commit 9288632
Show file tree
Hide file tree
Showing 18 changed files with 260 additions and 64 deletions.
17 changes: 12 additions & 5 deletions pkg/controller/deliverable/conditions.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@ import (
"fmt"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"

"github.com/vmware-tanzu/cartographer/pkg/apis/v1alpha1"
"github.com/vmware-tanzu/cartographer/pkg/realizer/deliverable"
"github.com/vmware-tanzu/cartographer/pkg/utils"
)

// -- Delivery conditions
Expand Down Expand Up @@ -88,12 +90,17 @@ func TemplateObjectRetrievalFailureCondition(err error) metav1.Condition {
}
}

func MissingValueAtPathCondition(resourceName, expression string) metav1.Condition {
func MissingValueAtPathCondition(obj *unstructured.Unstructured, expression string) metav1.Condition {
var namespaceMsg string
if obj.GetNamespace() != "" {
namespaceMsg = fmt.Sprintf(" in namespace [%s]", obj.GetNamespace())
}
return metav1.Condition{
Type: v1alpha1.DeliverableResourcesSubmitted,
Status: metav1.ConditionUnknown,
Reason: v1alpha1.MissingValueAtPathResourcesSubmittedReason,
Message: fmt.Sprintf("Resource '%s' is waiting to read value '%s'", resourceName, expression),
Type: v1alpha1.WorkloadResourceSubmitted,
Status: metav1.ConditionUnknown,
Reason: v1alpha1.MissingValueAtPathResourcesSubmittedReason,
Message: fmt.Sprintf("Waiting to read value [%s] from resource [%s/%s]%s",
expression, utils.GetFullyQualifiedType(obj), obj.GetName(), namespaceMsg),
}
}

Expand Down
56 changes: 56 additions & 0 deletions pkg/controller/deliverable/conditions_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// Copyright 2021 VMware
//
// 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 deliverable_test

import (
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime/schema"

"github.com/vmware-tanzu/cartographer/pkg/controller/deliverable"
)

var _ = Describe("Conditions", func() {

Describe("MissingValueAtPath", func() {
var obj *unstructured.Unstructured
BeforeEach(func() {
obj = &unstructured.Unstructured{}
obj.SetGroupVersionKind(schema.GroupVersionKind{
Group: "thing.io",
Version: "alphabeta1",
Kind: "Widget",
})
obj.SetName("my-widget")
})

Context("stamped object has a namespace", func() {
It("has the correct message", func() {
obj.SetNamespace("my-ns")

condition := deliverable.MissingValueAtPathCondition(obj, "spec.foo")
Expect(condition.Message).To(Equal("Waiting to read value [spec.foo] from resource [widget.thing.io/my-widget] in namespace [my-ns]"))
})
})

Context("stamped object does not have a namespace", func() {
It("has the correct message", func() {
condition := deliverable.MissingValueAtPathCondition(obj, "spec.foo")
Expect(condition.Message).To(Equal("Waiting to read value [spec.foo] from resource [widget.thing.io/my-widget]"))
})
})
})
})
2 changes: 1 addition & 1 deletion pkg/controller/deliverable/reconciler.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
case templates.DeploymentConditionError:
r.conditionManager.AddPositive(DeploymentConditionNotMetCondition(typedErr))
case templates.JsonPathError:
r.conditionManager.AddPositive(MissingValueAtPathCondition(typedErr.ResourceName(), typedErr.JsonPathExpression()))
r.conditionManager.AddPositive(MissingValueAtPathCondition(typedErr.StampedObject, typedErr.JsonPathExpression()))
default:
r.conditionManager.AddPositive(UnknownResourceErrorCondition(typedErr))
}
Expand Down
27 changes: 19 additions & 8 deletions pkg/controller/deliverable/reconciler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -479,11 +479,22 @@ var _ = Describe("Reconciler", func() {
Context("of type RetrieveOutputError", func() {
var retrieveError realizer.RetrieveOutputError
var wrappedError error
var stampedObject *unstructured.Unstructured

JustBeforeEach(func() {
stampedObject = &unstructured.Unstructured{}
stampedObject.SetGroupVersionKind(schema.GroupVersionKind{
Group: "thing.io",
Version: "alphabeta1",
Kind: "MyThing",
})
stampedObject.SetName("my-obj")
stampedObject.SetNamespace("my-ns")

retrieveError = realizer.RetrieveOutputError{
Err: wrappedError,
Resource: &v1alpha1.ClusterDeliveryResource{Name: "some-resource"},
Err: wrappedError,
Resource: &v1alpha1.ClusterDeliveryResource{Name: "some-resource"},
StampedObject: stampedObject,
}

rlzr.RealizeReturns(nil, retrieveError)
Expand Down Expand Up @@ -511,7 +522,7 @@ var _ = Describe("Reconciler", func() {
Expect(out).To(Say(`"msg":"handled error reconciling deliverable"`))
Expect(out).To(Say(`"deliverable":"my-namespace/my-deliverable-name"`))
Expect(out).To(Say(`"delivery":"some-delivery"`))
Expect(out).To(Say(`"handled error":"unable to retrieve outputs from stamped object for resource \[some-resource\]: some error"`))
Expect(out).To(Say(`"handled error":"unable to retrieve outputs from stamped object \[my-ns/my-obj\] of type \[mything.thing.io\] for resource \[some-resource\]: some error"`))
})
})

Expand All @@ -537,7 +548,7 @@ var _ = Describe("Reconciler", func() {
Expect(out).To(Say(`"msg":"handled error reconciling deliverable"`))
Expect(out).To(Say(`"deliverable":"my-namespace/my-deliverable-name"`))
Expect(out).To(Say(`"delivery":"some-delivery"`))
Expect(out).To(Say(`"handled error":"unable to retrieve outputs from stamped object for resource \[some-resource\]: some error"`))
Expect(out).To(Say(`"handled error":"unable to retrieve outputs from stamped object \[my-ns/my-obj\] of type \[mything.thing.io\] for resource \[some-resource\]: some error"`))
})
})

Expand All @@ -563,7 +574,7 @@ var _ = Describe("Reconciler", func() {
Expect(out).To(Say(`"msg":"handled error reconciling deliverable"`))
Expect(out).To(Say(`"deliverable":"my-namespace/my-deliverable-name"`))
Expect(out).To(Say(`"delivery":"some-delivery"`))
Expect(out).To(Say(`"handled error":"unable to retrieve outputs from stamped object for resource \[some-resource\]: some error"`))
Expect(out).To(Say(`"handled error":"unable to retrieve outputs from stamped object \[my-ns/my-obj\] of type \[mything.thing.io\] for resource \[some-resource\]: some error"`))
})
})

Expand All @@ -574,7 +585,7 @@ var _ = Describe("Reconciler", func() {

It("calls the condition manager to report", func() {
_, _ = reconciler.Reconcile(ctx, req)
Expect(conditionManager.AddPositiveArgsForCall(1)).To(Equal(deliverable.MissingValueAtPathCondition("some-resource", "this.wont.find.anything")))
Expect(conditionManager.AddPositiveArgsForCall(1)).To(Equal(deliverable.MissingValueAtPathCondition(stampedObject, "this.wont.find.anything")))
})

It("does not return an error", func() {
Expand All @@ -589,7 +600,7 @@ var _ = Describe("Reconciler", func() {
Expect(out).To(Say(`"msg":"handled error reconciling deliverable"`))
Expect(out).To(Say(`"deliverable":"my-namespace/my-deliverable-name"`))
Expect(out).To(Say(`"delivery":"some-delivery"`))
Expect(out).To(Say(`"handled error":"unable to retrieve outputs from stamped object for resource \[some-resource\]: failed to evaluate json path 'this.wont.find.anything': some error"`))
Expect(out).To(Say(`"handled error":"unable to retrieve outputs \[this.wont.find.anything\] from stamped object \[my-ns/my-obj\] of type \[mything.thing.io\] for resource \[some-resource\]: failed to evaluate json path 'this.wont.find.anything': some error"`))
})
})

Expand All @@ -615,7 +626,7 @@ var _ = Describe("Reconciler", func() {
Expect(out).To(Say(`"msg":"handled error reconciling deliverable"`))
Expect(out).To(Say(`"deliverable":"my-namespace/my-deliverable-name"`))
Expect(out).To(Say(`"delivery":"some-delivery"`))
Expect(out).To(Say(`"handled error":"unable to retrieve outputs from stamped object for resource \[some-resource\]: some error"`))
Expect(out).To(Say(`"handled error":"unable to retrieve outputs from stamped object \[my-ns/my-obj\] of type \[mything.thing.io\] for resource \[some-resource\]: some error"`))
})
})
})
Expand Down
25 changes: 20 additions & 5 deletions pkg/controller/runnable/conditions.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,13 @@
package runnable

import (
"fmt"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"

"github.com/vmware-tanzu/cartographer/pkg/apis/v1alpha1"
"github.com/vmware-tanzu/cartographer/pkg/utils"
)

// -- ClusterRunTemplate conditions
Expand Down Expand Up @@ -48,12 +52,23 @@ func StampedObjectRejectedByAPIServerCondition(err error) metav1.Condition {
}
}

func OutputPathNotSatisfiedCondition(err error) metav1.Condition {
func OutputPathNotSatisfiedCondition(obj *unstructured.Unstructured, errMsg string) metav1.Condition {
var namespaceMsg string
if obj.GetNamespace() != "" {
namespaceMsg = fmt.Sprintf(" in namespace [%s]", obj.GetNamespace())
}

name := obj.GetName()
if name == "" {
name = obj.GetGenerateName()
}

return metav1.Condition{
Type: v1alpha1.RunTemplateReady,
Status: metav1.ConditionFalse,
Reason: v1alpha1.OutputPathNotSatisfiedRunTemplateReason,
Message: err.Error(),
Type: v1alpha1.RunTemplateReady,
Status: metav1.ConditionFalse,
Reason: v1alpha1.OutputPathNotSatisfiedRunTemplateReason,
Message: fmt.Sprintf("Waiting to read value from resource [%s/%s]%s: %s",
utils.GetFullyQualifiedType(obj), name, namespaceMsg, errMsg),
}
}

Expand Down
56 changes: 56 additions & 0 deletions pkg/controller/runnable/conditions_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// Copyright 2021 VMware
//
// 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 runnable_test

import (
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime/schema"

"github.com/vmware-tanzu/cartographer/pkg/controller/runnable"
)

var _ = Describe("Conditions", func() {

Describe("MissingValueAtPath", func() {
var obj *unstructured.Unstructured
BeforeEach(func() {
obj = &unstructured.Unstructured{}
obj.SetGroupVersionKind(schema.GroupVersionKind{
Group: "thing.io",
Version: "alphabeta1",
Kind: "Widget",
})
obj.SetName("my-widget")
})

Context("stamped object has a namespace", func() {
It("has the correct message", func() {
obj.SetNamespace("my-ns")

condition := runnable.OutputPathNotSatisfiedCondition(obj, "problem at spec.foo")
Expect(condition.Message).To(Equal("Waiting to read value from resource [widget.thing.io/my-widget] in namespace [my-ns]: problem at spec.foo"))
})
})

Context("stamped object does not have a namespace", func() {
It("has the correct message", func() {
condition := runnable.OutputPathNotSatisfiedCondition(obj, "problem at spec.foo")
Expect(condition.Message).To(Equal("Waiting to read value from resource [widget.thing.io/my-widget]: problem at spec.foo"))
})
})
})
})
2 changes: 1 addition & 1 deletion pkg/controller/runnable/reconciler.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
r.conditionManager.AddPositive(FailedToListCreatedObjectsCondition(typedErr))
err = controller.NewUnhandledError(err)
case realizer.RetrieveOutputError:
r.conditionManager.AddPositive(OutputPathNotSatisfiedCondition(typedErr))
r.conditionManager.AddPositive(OutputPathNotSatisfiedCondition(typedErr.StampedObject, typedErr.Error()))
default:
r.conditionManager.AddPositive(UnknownErrorCondition(typedErr))
err = controller.NewUnhandledError(err)
Expand Down
20 changes: 16 additions & 4 deletions pkg/controller/runnable/reconciler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -505,17 +505,29 @@ var _ = Describe("Reconcile", func() {

Context("of type RetrieveOutputError", func() {
var err error
var stampedObject *unstructured.Unstructured

BeforeEach(func() {
stampedObject = &unstructured.Unstructured{}
stampedObject.SetGroupVersionKind(schema.GroupVersionKind{
Group: "thing.io",
Version: "alphabeta1",
Kind: "MyThing",
})
stampedObject.SetName("my-obj")
stampedObject.SetNamespace("my-ns")

err = realizer.RetrieveOutputError{
Err: errors.New("some error"),
Runnable: &v1alpha1.Runnable{ObjectMeta: metav1.ObjectMeta{Name: "my-runnable", Namespace: "my-ns"}},
Err: errors.New("some error"),
Runnable: &v1alpha1.Runnable{ObjectMeta: metav1.ObjectMeta{Name: "my-runnable", Namespace: "my-ns"}},
StampedObject: stampedObject,
}
rlzr.RealizeReturns(nil, nil, err)
})

It("calls the condition manager to report", func() {
_, _ = reconciler.Reconcile(ctx, request)
Expect(conditionManager.AddPositiveArgsForCall(0)).To(Equal(runnable.OutputPathNotSatisfiedCondition(err)))
Expect(conditionManager.AddPositiveArgsForCall(0)).To(Equal(runnable.OutputPathNotSatisfiedCondition(stampedObject, err.Error())))
})

It("does not return an error", func() {
Expand All @@ -528,7 +540,7 @@ var _ = Describe("Reconcile", func() {

Expect(out).To(Say(`"level":"info"`))
Expect(out).To(Say(`"msg":"handled error reconciling runnable"`))
Expect(out).To(Say(`"handled error":"unable to retrieve outputs from stamped object for runnable \[my-ns/my-runnable\]: some error"`))
Expect(out).To(Say(`"handled error":"unable to retrieve outputs from stamped object \[my-ns/my-obj\] of type \[mything.thing.io\] for runnable \[my-ns/my-runnable\]: some error"`))
})
})

Expand Down
12 changes: 2 additions & 10 deletions pkg/controller/workload/conditions.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@ package workload

import (
"fmt"
"strings"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"

"github.com/vmware-tanzu/cartographer/pkg/apis/v1alpha1"
"github.com/vmware-tanzu/cartographer/pkg/utils"
)

// -- Supply Chain conditions
Expand Down Expand Up @@ -90,14 +90,6 @@ func TemplateObjectRetrievalFailureCondition(err error) metav1.Condition {
}

func MissingValueAtPathCondition(obj *unstructured.Unstructured, expression string) metav1.Condition {
var fullyQualifiedType string
if obj.GetObjectKind().GroupVersionKind().Group == "" {
fullyQualifiedType = strings.ToLower(obj.GetKind())
} else {
fullyQualifiedType = fmt.Sprintf("%s.%s", strings.ToLower(obj.GetKind()),
obj.GetObjectKind().GroupVersionKind().Group)
}

var namespaceMsg string
if obj.GetNamespace() != "" {
namespaceMsg = fmt.Sprintf(" in namespace [%s]", obj.GetNamespace())
Expand All @@ -107,7 +99,7 @@ func MissingValueAtPathCondition(obj *unstructured.Unstructured, expression stri
Status: metav1.ConditionUnknown,
Reason: v1alpha1.MissingValueAtPathResourcesSubmittedReason,
Message: fmt.Sprintf("Waiting to read value [%s] from resource [%s/%s]%s",
expression, fullyQualifiedType, obj.GetName(), namespaceMsg),
expression, utils.GetFullyQualifiedType(obj), obj.GetName(), namespaceMsg),
}
}

Expand Down
5 changes: 3 additions & 2 deletions pkg/realizer/deliverable/component.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,9 @@ func (r *resourceRealizer) Do(ctx context.Context, resource *v1alpha1.ClusterDel
if err != nil {
log.Error(err, "failed to retrieve output from object", "object", stampedObject)
return stampedObject, nil, RetrieveOutputError{
Err: err,
Resource: resource,
Err: err,
Resource: resource,
StampedObject: stampedObject,
}
}

Expand Down
Loading

0 comments on commit 9288632

Please sign in to comment.