Skip to content

Commit

Permalink
resources: Fail on non-string template labels.
Browse files Browse the repository at this point in the history
Although ObjectMeta defines labels as a map[string]string, Unstructured
types allow for map[string]interface{}, which means that users can set
non-string values like ints in their configurations, and we will parse it
with no issues.

We discovered that the behavior of
[unstructured.GetLabels](https://github.com/kubernetes/apimachinery/blob/dd12c7a65e7f49c4e00b5bd5eb93de2fcbf7831d/pkg/apis/meta/v1/unstructured/unstructured.go#L395)
will silently ignore errors, returning an empty label set if a
non-string value is included. When used in triggers, this looks like we
are completely ignoring the user provided labels.

This change replaces GetLabels with it's underlying implementation, and bubbles
back the non-string error if it is encountered.
  • Loading branch information
wlynch authored and tekton-robot committed Apr 2, 2021
1 parent 59bd1b7 commit a545f76
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 5 deletions.
15 changes: 11 additions & 4 deletions pkg/resources/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,14 @@ func Create(logger *zap.SugaredLogger, rt json.RawMessage, triggerName, eventID,
return fmt.Errorf("couldn't unmarshal json from the TriggerTemplate: %v", err)
}

data = addLabels(data, map[string]string{
data, err := addLabels(data, map[string]string{
triggersv1.EventListenerLabelKey: elName,
triggersv1.EventIDLabelKey: eventID,
triggersv1.TriggerLabelKey: triggerName,
})
if err != nil {
return err
}

namespace := data.GetNamespace()
// Default the resource creation to the EventListenerNamespace if not found in the resource template
Expand Down Expand Up @@ -112,8 +115,12 @@ func Create(logger *zap.SugaredLogger, rt json.RawMessage, triggerName, eventID,
}

// addLabels adds autogenerated Tekton labels to created resources.
func addLabels(us *unstructured.Unstructured, labelsToAdd map[string]string) *unstructured.Unstructured {
labels := us.GetLabels()
func addLabels(us *unstructured.Unstructured, labelsToAdd map[string]string) (*unstructured.Unstructured, error) {
labels, _, err := unstructured.NestedStringMap(us.Object, "metadata", "labels")
if err != nil {
return nil, err
}

if labels == nil {
labels = make(map[string]string)
}
Expand All @@ -123,5 +130,5 @@ func addLabels(us *unstructured.Unstructured, labelsToAdd map[string]string) *un
}

us.SetLabels(labels)
return us
return us, nil
}
20 changes: 19 additions & 1 deletion pkg/resources/create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -299,10 +299,28 @@ func Test_AddLabels(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := addLabels(tt.us, tt.labelsToAdd)
got, err := addLabels(tt.us, tt.labelsToAdd)
if err != nil {
t.Fatal(err)
}
if diff := cmp.Diff(tt.want, got); diff != "" {
t.Errorf("addLabels(): -want +got: %s", diff)
}
})
}

t.Run("non-string label", func(t *testing.T) {
in := &unstructured.Unstructured{
Object: map[string]interface{}{
"metadata": map[string]interface{}{
"labels": map[string]interface{}{
"foo": 0,
},
},
},
}
if got, err := addLabels(in, map[string]string{"a": "b"}); err == nil {
t.Errorf("expected error, got: %v", got)
}
})
}

0 comments on commit a545f76

Please sign in to comment.