From 14d4a989d994ae4ff30fba758b76c13d9f23a130 Mon Sep 17 00:00:00 2001 From: Lee Bernick Date: Fri, 7 Apr 2023 16:29:55 -0400 Subject: [PATCH] Fix resolutionrequest conversion This commit addresses two issues with ResolutionRequest conversion. First, it updates the "hubVersion" to v1alpha1, since v1alpha1 is the version that supports conversion between versions, and addresses an incorrect comment about what "hubVersion" represents. Second, it adds conversion for ResolutionRequest statuses. It updates tests to test the round trip conversion instead of one-way conversion, as this helps avoid bugs with copy-paste. --- cmd/webhook/main.go | 6 +- .../v1alpha1/resolution_request_conversion.go | 45 +++ .../resolution_request_conversion_test.go | 263 ++++++++---------- 3 files changed, 166 insertions(+), 148 deletions(-) diff --git a/cmd/webhook/main.go b/cmd/webhook/main.go index 3107ec666e9..56ee9b35784 100644 --- a/cmd/webhook/main.go +++ b/cmd/webhook/main.go @@ -166,7 +166,9 @@ func newConversionController(ctx context.Context, cmw configmap.Watcher) *contro "/resource-conversion", // Specify the types of custom resource definitions that should be converted - // "HubVersion" is the stored version, and "Zygotes" are the supported versions + // "HubVersion" specifies which version of the CustomResource supports + // conversions to and from all types. + // "Zygotes" are the supported versions. map[schema.GroupKind]conversion.GroupKindConversion{ v1.Kind("Task"): { DefinitionName: pipeline.TaskResource.String(), @@ -202,7 +204,7 @@ func newConversionController(ctx context.Context, cmw configmap.Watcher) *contro }, resolutionv1beta1.Kind("ResolutionRequest"): { DefinitionName: resolution.ResolutionRequestResource.String(), - HubVersion: resolutionv1beta1GroupVersion, + HubVersion: resolutionv1alpha1GroupVersion, Zygotes: map[string]conversion.ConvertibleObject{ resolutionv1alpha1GroupVersion: &resolutionv1alpha1.ResolutionRequest{}, resolutionv1beta1GroupVersion: &resolutionv1beta1.ResolutionRequest{}, diff --git a/pkg/apis/resolution/v1alpha1/resolution_request_conversion.go b/pkg/apis/resolution/v1alpha1/resolution_request_conversion.go index 25a5b98ec60..7ed4befd078 100644 --- a/pkg/apis/resolution/v1alpha1/resolution_request_conversion.go +++ b/pkg/apis/resolution/v1alpha1/resolution_request_conversion.go @@ -37,6 +37,7 @@ func (rr *ResolutionRequest) ConvertTo(ctx context.Context, sink apis.Convertibl switch sink := sink.(type) { case *v1beta1.ResolutionRequest: sink.ObjectMeta = rr.ObjectMeta + rr.Status.convertTo(ctx, &sink.Status) return rr.Spec.ConvertTo(ctx, &sink.Spec) default: return fmt.Errorf("unknown version, got: %T", sink) @@ -58,6 +59,22 @@ func (rrs *ResolutionRequestSpec) ConvertTo(ctx context.Context, sink *v1beta1.R return nil } +// convertTo converts a v1alpha1.ResolutionRequestStatus to a v1beta1.ResolutionRequestStatus +func (rrs *ResolutionRequestStatus) convertTo(ctx context.Context, sink *v1beta1.ResolutionRequestStatus) { + sink.Data = rrs.Data + if rrs.RefSource != nil { + refSource := pipelinev1beta1.RefSource{} + refSource.URI = rrs.RefSource.URI + refSource.EntryPoint = rrs.RefSource.EntryPoint + digest := make(map[string]string) + for k, v := range rrs.RefSource.Digest { + digest[k] = v + } + refSource.Digest = digest + sink.RefSource = &refSource + } +} + // ConvertFrom implements apis.Convertible func (rr *ResolutionRequest) ConvertFrom(ctx context.Context, from apis.Convertible) error { if apis.IsInDelete(ctx) { @@ -66,6 +83,7 @@ func (rr *ResolutionRequest) ConvertFrom(ctx context.Context, from apis.Converti switch from := from.(type) { case *v1beta1.ResolutionRequest: rr.ObjectMeta = from.ObjectMeta + rr.Status.convertFrom(ctx, &from.Status) return rr.Spec.ConvertFrom(ctx, &from.Spec) default: return fmt.Errorf("unknown version, got: %T", from) @@ -93,3 +111,30 @@ func (rrs *ResolutionRequestSpec) ConvertFrom(ctx context.Context, from *v1beta1 return nil } + +// convertTo converts a v1alpha1.ResolutionRequestStatus to a v1beta1.ResolutionRequestStatus +func (rrs *ResolutionRequestStatus) convertFrom(ctx context.Context, from *v1beta1.ResolutionRequestStatus) { + rrs.Data = from.Data + + if from.RefSource != nil { + refSource := pipelinev1beta1.RefSource{} + refSource.URI = from.RefSource.URI + refSource.EntryPoint = from.RefSource.EntryPoint + digest := make(map[string]string) + for k, v := range from.RefSource.Digest { + digest[k] = v + } + refSource.Digest = digest + rrs.RefSource = &refSource + } else if from.Source != nil { + refSource := pipelinev1beta1.RefSource{} + refSource.URI = from.Source.URI + refSource.EntryPoint = from.Source.EntryPoint + digest := make(map[string]string) + for k, v := range from.Source.Digest { + digest[k] = v + } + refSource.Digest = digest + rrs.RefSource = &refSource + } +} diff --git a/pkg/apis/resolution/v1alpha1/resolution_request_conversion_test.go b/pkg/apis/resolution/v1alpha1/resolution_request_conversion_test.go index 773073464c3..7c2bfc8d365 100644 --- a/pkg/apis/resolution/v1alpha1/resolution_request_conversion_test.go +++ b/pkg/apis/resolution/v1alpha1/resolution_request_conversion_test.go @@ -19,7 +19,6 @@ package v1alpha1 import ( "context" - "errors" "testing" "github.com/google/go-cmp/cmp" @@ -42,182 +41,154 @@ func TestResolutionRequestConversionBadType(t *testing.T) { } } -func TestResolutionRequestConvertTo(t *testing.T) { - versions := []apis.Convertible{&v1beta1.ResolutionRequest{}} - +func TestResolutionRequestConvertRoundTrip(t *testing.T) { testCases := []struct { name string in *ResolutionRequest out apis.Convertible - }{ - { - name: "no params", - in: &ResolutionRequest{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "bar", - }, - Spec: ResolutionRequestSpec{ - Parameters: nil, - }, + }{{ + name: "no params", + in: &ResolutionRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", }, - out: &v1beta1.ResolutionRequest{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "bar", - }, - Spec: v1beta1.ResolutionRequestSpec{ - Params: nil, + Spec: ResolutionRequestSpec{ + Parameters: nil, + }, + }, + }, { + name: "with params", + in: &ResolutionRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + }, + Spec: ResolutionRequestSpec{ + Parameters: map[string]string{ + "some-param": "some-value", }, }, - }, { - name: "with params", - in: &ResolutionRequest{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "bar", + }, + }, { + name: "with status refsource", + in: &ResolutionRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + }, + Spec: ResolutionRequestSpec{ + Parameters: map[string]string{ + "some-param": "some-value", }, - Spec: ResolutionRequestSpec{ - Parameters: map[string]string{ - "some-param": "some-value", + }, + Status: ResolutionRequestStatus{ + ResolutionRequestStatusFields: ResolutionRequestStatusFields{ + Data: "foobar", + RefSource: &pipelinev1beta1.RefSource{ + URI: "abcd", + Digest: map[string]string{"123": "456"}, + EntryPoint: "baz", }, }, }, - out: &v1beta1.ResolutionRequest{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "bar", + }, + }, { + name: "with status, no refsource", + in: &ResolutionRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + }, + Spec: ResolutionRequestSpec{ + Parameters: map[string]string{ + "some-param": "some-value", }, - Spec: v1beta1.ResolutionRequestSpec{ - Params: []pipelinev1beta1.Param{{ - Name: "some-param", - Value: *pipelinev1beta1.NewStructuredValues("some-value"), - }}, + }, + Status: ResolutionRequestStatus{ + ResolutionRequestStatusFields: ResolutionRequestStatusFields{ + Data: "foobar", }, }, }, - } + }} for _, tc := range testCases { - for _, version := range versions { - t.Run(tc.name, func(t *testing.T) { - got := version - if err := tc.in.ConvertTo(context.Background(), got); err != nil { - t.Fatalf("ConvertTo() = %v", err) - } - t.Logf("ConvertTo() = %#v", got) - if d := cmp.Diff(tc.out, got); d != "" { - t.Errorf("converted ResolutionRequest did not match expected: %s", diff.PrintWantGot(d)) - } - }) - } + t.Run(tc.name, func(t *testing.T) { + got := &v1beta1.ResolutionRequest{} + if err := tc.in.ConvertTo(context.Background(), got); err != nil { + t.Fatalf("ConvertTo() = %v", err) + } + + t.Logf("ConvertTo() = %#v", got) + + roundTrip := &ResolutionRequest{} + if err := roundTrip.ConvertFrom(context.Background(), got); err != nil { + t.Errorf("ConvertFrom() = %v", err) + } + + if d := cmp.Diff(tc.in, roundTrip); d != "" { + t.Errorf("converted ResolutionRequest did not match expected: %s", diff.PrintWantGot(d)) + } + }) } } -func TestResolutionRequestConvertFrom(t *testing.T) { - versions := []apis.Convertible{&ResolutionRequest{}} - +func TestResolutionRequestConvertFromDeprecated(t *testing.T) { testCases := []struct { - name string - in apis.Convertible - out *ResolutionRequest - expectedErr error - }{ - { - name: "no params", - in: &v1beta1.ResolutionRequest{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "bar", - }, - Spec: v1beta1.ResolutionRequestSpec{ - Params: nil, - }, - }, - out: &ResolutionRequest{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "bar", - }, - Spec: ResolutionRequestSpec{ - Parameters: nil, - }, + name string + in *v1beta1.ResolutionRequest + want apis.Convertible + }{{ + name: "with status.source", + in: &v1beta1.ResolutionRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", }, - }, { - name: "with only string params", - in: &v1beta1.ResolutionRequest{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "bar", - }, - Spec: v1beta1.ResolutionRequestSpec{ - Params: []pipelinev1beta1.Param{{ - Name: "some-param", - Value: *pipelinev1beta1.NewStructuredValues("some-value"), - }}, - }, + Spec: v1beta1.ResolutionRequestSpec{ + Params: nil, }, - out: &ResolutionRequest{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "bar", - }, - Spec: ResolutionRequestSpec{ - Parameters: map[string]string{ - "some-param": "some-value", + Status: v1beta1.ResolutionRequestStatus{ + ResolutionRequestStatusFields: v1beta1.ResolutionRequestStatusFields{ + Source: &pipelinev1beta1.ConfigSource{ + URI: "abcd", + Digest: map[string]string{"123": "456"}, + EntryPoint: "baz", }, }, }, - }, { - name: "with non-string params", - in: &v1beta1.ResolutionRequest{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "bar", - }, - Spec: v1beta1.ResolutionRequestSpec{ - Params: []pipelinev1beta1.Param{ - { - Name: "array-val", - Value: *pipelinev1beta1.NewStructuredValues("one", "two"), - }, { - Name: "string-val", - Value: *pipelinev1beta1.NewStructuredValues("a-string"), - }, { - Name: "object-val", - Value: *pipelinev1beta1.NewObject(map[string]string{ - "key-one": "value-one", - "key-two": "value-two", - }), - }, + }, + want: &ResolutionRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + }, + Spec: ResolutionRequestSpec{ + Parameters: nil, + }, + Status: ResolutionRequestStatus{ + ResolutionRequestStatusFields: ResolutionRequestStatusFields{ + RefSource: &pipelinev1beta1.RefSource{ + URI: "abcd", + Digest: map[string]string{"123": "456"}, + EntryPoint: "baz", }, }, }, - out: nil, - expectedErr: errors.New("cannot convert v1beta1 to v1alpha, non-string type parameter(s) found: array-val, object-val"), }, - } + }} for _, tc := range testCases { - for _, version := range versions { - t.Run(tc.name, func(t *testing.T) { - got := version - err := got.ConvertFrom(context.Background(), tc.in) - if tc.expectedErr != nil { - if err == nil { - t.Fatalf("expected error '%s', but did not get an error", tc.expectedErr.Error()) - } else if d := cmp.Diff(tc.expectedErr.Error(), err.Error()); d != "" { - t.Fatalf("error did not meet expected: %s", diff.PrintWantGot(d)) - } - return - } else if err != nil { - t.Fatalf("ConvertFrom() = %v", err) - } - t.Logf("ConvertFrom() = %#v", got) - if d := cmp.Diff(tc.out, got); d != "" { - t.Errorf("converted ResolutionRequest did not match expected: %s", diff.PrintWantGot(d)) - } - }) - } + t.Run(tc.name, func(t *testing.T) { + got := &ResolutionRequest{} + if err := got.ConvertFrom(context.Background(), tc.in); err != nil { + t.Errorf("ConvertFrom() = %v", err) + } + + if d := cmp.Diff(tc.want, got); d != "" { + t.Errorf("converted ResolutionRequest did not match expected: %s", diff.PrintWantGot(d)) + } + }) } }