From ce5acdf4dc899049bbacc6469096400a4e95bb51 Mon Sep 17 00:00:00 2001
From: Warren Hodgkinson <warren.hodgkinson@gmail.com>
Date: Thu, 19 Dec 2024 19:06:26 +0000
Subject: [PATCH] Fix for multiple WithRemote options (#3982)

When specifying multiple sets of remote options using
WithRemoteOptions, only the last set of options are used.
The earlier ones are discarded. This is a problem for example, when
an initial set of WithRemoteOptions are generated by command-line
flags, and then additional ones need to be specified during
application execution. Doing so would discard the options set
by the command-line flags.

To preserve backwards compatibility, where the WithRemoteOptions
function overrides the defaults, an additional function
WithMoreRemoteOptions has been added, which appends the options
to any existing ones.

Resolves: sigstore/cosign/#3981

Signed-off-by: Warren Hodgkinson <warren.hodgkinson@gmail.com>
---
 pkg/oci/remote/options.go      |  8 ++++
 pkg/oci/remote/options_test.go | 68 +++++++++++++++++++++++++++++++++-
 2 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/pkg/oci/remote/options.go b/pkg/oci/remote/options.go
index 0a7f23842b1..6eeaadd0105 100644
--- a/pkg/oci/remote/options.go
+++ b/pkg/oci/remote/options.go
@@ -113,6 +113,14 @@ func WithRemoteOptions(opts ...remote.Option) Option {
 	}
 }
 
+// WithMoreRemoteOptions is a functional option for adding to the default
+// remote options already specified
+func WithMoreRemoteOptions(opts ...remote.Option) Option {
+	return func(o *options) {
+		o.ROpt = append(o.ROpt, opts...)
+	}
+}
+
 // WithTargetRepository is a functional option for overriding the default
 // target repository hosting the signature and attestation tags.
 func WithTargetRepository(repo name.Repository) Option {
diff --git a/pkg/oci/remote/options_test.go b/pkg/oci/remote/options_test.go
index cea1b026dc9..e70070584dc 100644
--- a/pkg/oci/remote/options_test.go
+++ b/pkg/oci/remote/options_test.go
@@ -16,6 +16,7 @@
 package remote
 
 import (
+	"context"
 	"errors"
 	"os"
 	"reflect"
@@ -42,6 +43,10 @@ func TestOptions(t *testing.T) {
 		// TODO(mattmoor): Incorporate user agent.
 	}
 
+	moreROpt := []remote.Option{
+		remote.WithContext(context.Background()),
+	}
+
 	tests := []struct {
 		name string
 		opts []Option
@@ -105,6 +110,16 @@ func TestOptions(t *testing.T) {
 			TargetRepository:  repo,
 			ROpt:              otherROpt,
 		},
+	}, {
+		name: "more remote options option",
+		opts: []Option{WithRemoteOptions(otherROpt...), WithMoreRemoteOptions(moreROpt...)},
+		want: &options{
+			SignatureSuffix:   SignatureTagSuffix,
+			AttestationSuffix: AttestationTagSuffix,
+			SBOMSuffix:        SBOMTagSuffix,
+			TargetRepository:  repo,
+			ROpt:              append(append([]remote.Option{}, otherROpt...), moreROpt...),
+		},
 	}}
 
 	for _, test := range tests {
@@ -112,7 +127,7 @@ func TestOptions(t *testing.T) {
 			got := makeOptions(repo, test.opts...)
 			test.want.OriginalOptions = test.opts
 
-			if !reflect.DeepEqual(got, test.want) {
+			if !optionsEqual(got, test.want) {
 				t.Errorf("makeOptions() = %#v, wanted %#v", got, test.want)
 			}
 		})
@@ -170,3 +185,54 @@ func TestGetEnvTargetRepository(t *testing.T) {
 		})
 	}
 }
+
+// this is required due to the fact that reflect.DeepEquals reports
+// two different slices of function points, with identical length and
+// contents at each position as being different
+func optionsEqual(o1, o2 *options) bool {
+	if (o1 == nil) != (o2 == nil) {
+		return false
+	}
+	if o1 == nil {
+		return true
+	}
+
+	if o1.AttestationSuffix != o2.AttestationSuffix {
+		return false
+	}
+	if o1.SignatureSuffix != o2.SignatureSuffix {
+		return false
+	}
+	if o1.SBOMSuffix != o2.SBOMSuffix {
+		return false
+	}
+	if o1.TagPrefix != o2.TagPrefix {
+		return false
+	}
+	if !slicesEqual(o1.ROpt, o2.ROpt) {
+		return false
+	}
+	if !slicesEqual(o1.NameOpts, o2.NameOpts) {
+		return false
+	}
+	if !slicesEqual(o1.OriginalOptions, o2.OriginalOptions) {
+		return false
+	}
+	return true
+}
+
+func slicesEqual[T any](o1, o2 []T) bool {
+	if len(o1) != len(o2) {
+		return false
+	}
+
+	for i := range o1 {
+		v1 := reflect.ValueOf(o1[i])
+		v2 := reflect.ValueOf(o2[i])
+		if v1 != v2 {
+			return false
+		}
+	}
+
+	return true
+}