Skip to content

Commit

Permalink
Changes from code review.
Browse files Browse the repository at this point in the history 
* Refactor the usage of the Kubernetes client/fetcher for better separation of concerns.
* Add new test cases for failure modes and multiple signatures.
* Use the same value as cosign CLI for timeout.
  • Loading branch information
@stormqueen1990
stormqueen1990 committed Jul 12, 2023
1 parent 82a7474 commit 4873c59
Show file tree
Hide file tree
Showing 13 changed files with 616 additions and 57 deletions.
8 changes: 6 additions & 2 deletions cmd/controller/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import (
"os"
"time"

"github.com/pivotal/kpack/pkg/secret"

"github.com/pivotal/kpack/pkg/cosign"
"github.com/sigstore/cosign/v2/cmd/cosign/cli/sign"
ociremote "github.com/sigstore/cosign/v2/pkg/oci/remote"
Expand Down Expand Up @@ -200,12 +202,14 @@ func main() {
ImageSigner: cosign.NewImageSigner(sign.SignCmd, ociremote.SignatureTag),
}

secretFetcher := &secret.Fetcher{Client: k8sClient}

buildController := build.NewController(ctx, options, k8sClient, buildInformer, podInformer, metadataRetriever, buildpodGenerator, keychainFactory, *injectedSidecarSupport)
imageController := image.NewController(ctx, options, k8sClient, imageInformer, buildInformer, duckBuilderInformer, sourceResolverInformer, pvcInformer, *enablePriorityClasses)
sourceResolverController := sourceresolver.NewController(ctx, options, sourceResolverInformer, gitResolver, blobResolver, registryResolver)
builderController, builderResync := builder.NewController(ctx, options, k8sClient, builderInformer, builderCreator, keychainFactory, clusterStoreInformer, buildpackInformer, clusterBuildpackInformer, clusterStackInformer)
builderController, builderResync := builder.NewController(ctx, options, builderInformer, builderCreator, keychainFactory, clusterStoreInformer, buildpackInformer, clusterBuildpackInformer, clusterStackInformer, secretFetcher)
buildpackController := buildpack.NewController(ctx, options, keychainFactory, buildpackInformer, remoteStoreReader)
clusterBuilderController, clusterBuilderResync := clusterbuilder.NewController(ctx, options, k8sClient, clusterBuilderInformer, builderCreator, keychainFactory, clusterStoreInformer, clusterBuildpackInformer, clusterStackInformer)
clusterBuilderController, clusterBuilderResync := clusterbuilder.NewController(ctx, options, clusterBuilderInformer, builderCreator, keychainFactory, clusterStoreInformer, clusterBuildpackInformer, clusterStackInformer, secretFetcher)
clusterBuildpackController := clusterbuildpack.NewController(ctx, options, keychainFactory, clusterBuildpackInformer, remoteStoreReader)
clusterStoreController := clusterstore.NewController(ctx, options, keychainFactory, clusterStoreInformer, remoteStoreReader)
clusterStackController := clusterstack.NewController(ctx, options, keychainFactory, clusterStackInformer, remoteStackReader)
Expand Down
36 changes: 18 additions & 18 deletions pkg/cnb/create_builder_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
KeychainFactory: keychainFactory,
LifecycleProvider: lifecycleProvider,
ImageSigner: &fakeBuilderSigner{
signBuilder: func(ctx context.Context, s string, secrets []corev1.Secret, keychain authn.Keychain) ([]buildapi.CosignSignature, error) {
signBuilder: func(ctx context.Context, s string, secrets []*corev1.Secret, keychain authn.Keychain) ([]buildapi.CosignSignature, error) {
// no-op
return nil, nil
},
Expand Down Expand Up @@ -352,7 +352,7 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
})

it("creates a custom builder", func() {
builderRecord, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []corev1.Secret{})
builderRecord, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []*corev1.Secret{})
require.NoError(t, err)

assert.Len(t, builderRecord.Buildpacks, 4)
Expand Down Expand Up @@ -642,11 +642,11 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
})

it("creates images deterministically ", func() {
original, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []corev1.Secret{})
original, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []*corev1.Secret{})
require.NoError(t, err)

for i := 1; i <= 50; i++ {
other, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []corev1.Secret{})
other, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []*corev1.Secret{})

require.NoError(t, err)

Expand Down Expand Up @@ -677,7 +677,7 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
},
}

_, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []corev1.Secret{})
_, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []*corev1.Secret{})
require.EqualError(t, err, "validating buildpack io.buildpack.unsupported.stack@v4: stack io.buildpacks.stacks.some-stack is not supported")
})

Expand All @@ -701,7 +701,7 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
}},
}}

_, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []corev1.Secret{})
_, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []*corev1.Secret{})
require.EqualError(t, err, "validating buildpack io.buildpack.unsupported.mixin@v4: stack missing mixin(s): something-missing-mixin, something-missing-mixin2")
})

Expand Down Expand Up @@ -746,7 +746,7 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
}},
}}

_, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []corev1.Secret{})
_, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []*corev1.Secret{})
require.Nil(t, err)
})

Expand All @@ -771,7 +771,7 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
}},
}}

_, err := subject.CreateBuilder(ctx, builderKeychain, nil, fetcher, stack, clusterBuilderSpec, []corev1.Secret{})
_, err := subject.CreateBuilder(ctx, builderKeychain, nil, fetcher, stack, clusterBuilderSpec, []*corev1.Secret{})
require.Error(t, err, "validating buildpack io.buildpack.relaxed.old.mixin@v4: stack missing mixin(s): build:common-mixin, run:common-mixin, another-common-mixin")
})

Expand All @@ -794,7 +794,7 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
}},
}}

_, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []corev1.Secret{})
_, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []*corev1.Secret{})
require.EqualError(t, err, "validating buildpack io.buildpack.unsupported.buildpack.api@v4: unsupported buildpack api: 0.1, expecting: 0.2, 0.3")
})

Expand Down Expand Up @@ -837,7 +837,7 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
}},
}}

_, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []corev1.Secret{})
_, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []*corev1.Secret{})
require.NoError(t, err)
})
})
Expand All @@ -864,22 +864,22 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
},
}

_, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []corev1.Secret{})
_, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []*corev1.Secret{})
require.EqualError(t, err, "unsupported platform apis in kpack lifecycle: 0.1, 0.2, 0.999, expecting one of: 0.3, 0.4, 0.5, 0.6, 0.7, 0.8")
})
})

when("signing a builder image", func() {
it("does not populate the signature paths when no secrets were present", func() {
builderRecord, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []corev1.Secret{})
builderRecord, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []*corev1.Secret{})
require.NoError(t, err)
require.NotNil(t, builderRecord)
require.Empty(t, builderRecord.SignaturePaths)
})

it("returns an error if signing fails", func() {
subject.ImageSigner = &fakeBuilderSigner{
signBuilder: func(ctx context.Context, s string, secrets []corev1.Secret, keychain authn.Keychain) ([]buildapi.CosignSignature, error) {
signBuilder: func(ctx context.Context, s string, secrets []*corev1.Secret, keychain authn.Keychain) ([]buildapi.CosignSignature, error) {
return nil, fmt.Errorf("failed to sign builder")
},
}
Expand All @@ -891,7 +891,7 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
},
}

_, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []corev1.Secret{fakeSecret})
_, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []*corev1.Secret{&fakeSecret})
require.Error(t, err)
})

Expand All @@ -904,7 +904,7 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
}

subject.ImageSigner = &fakeBuilderSigner{
signBuilder: func(ctx context.Context, s string, secrets []corev1.Secret, keychain authn.Keychain) ([]buildapi.CosignSignature, error) {
signBuilder: func(ctx context.Context, s string, secrets []*corev1.Secret, keychain authn.Keychain) ([]buildapi.CosignSignature, error) {
return []buildapi.CosignSignature{
{
SigningSecret: fmt.Sprintf("k8s://%s/%s", fakeSecret.Namespace, fakeSecret.Name),
Expand All @@ -914,7 +914,7 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
},
}

builderRecord, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []corev1.Secret{fakeSecret})
builderRecord, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []*corev1.Secret{&fakeSecret})
require.NoError(t, err)
require.NotNil(t, builderRecord)
require.NotEmpty(t, builderRecord.SignaturePaths)
Expand All @@ -924,10 +924,10 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
}

type fakeBuilderSigner struct {
signBuilder func(context.Context, string, []corev1.Secret, authn.Keychain) ([]buildapi.CosignSignature, error)
signBuilder func(context.Context, string, []*corev1.Secret, authn.Keychain) ([]buildapi.CosignSignature, error)
}

func (s *fakeBuilderSigner) SignBuilder(ctx context.Context, imageReference string, signingSecrets []corev1.Secret, builderKeychain authn.Keychain) ([]buildapi.CosignSignature, error) {
func (s *fakeBuilderSigner) SignBuilder(ctx context.Context, imageReference string, signingSecrets []*corev1.Secret, builderKeychain authn.Keychain) ([]buildapi.CosignSignature, error) {
return s.signBuilder(ctx, imageReference, signingSecrets, builderKeychain)
}

Expand Down
26 changes: 13 additions & 13 deletions pkg/cosign/image_signer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,12 @@ import (
"github.com/google/go-containerregistry/pkg/v1/remote"
"github.com/pivotal/kpack/pkg/apis/build/v1alpha2"
"github.com/pkg/errors"
"github.com/sigstore/cosign/v2/cmd/cosign/cli/options"
cosignoptions "github.com/sigstore/cosign/v2/cmd/cosign/cli/options"
cosignremote "github.com/sigstore/cosign/v2/pkg/oci/remote"
corev1 "k8s.io/api/core/v1"
)

type SignFunc func(*options.RootOptions, options.KeyOpts, options.SignOptions, []string) error
type SignFunc func(*cosignoptions.RootOptions, cosignoptions.KeyOpts, cosignoptions.SignOptions, []string) error

type FetchSignatureFunc func(name.Reference, ...cosignremote.Option) (name.Tag, error)

Expand All @@ -39,7 +39,7 @@ func NewImageSigner(signFunc SignFunc, fetchSignatureFunc FetchSignatureFunc) *I
}
}

func (s *ImageSigner) Sign(ro *options.RootOptions, report platform.ExportReport, secretLocation string, annotations, cosignRepositories, cosignDockerMediaTypes map[string]interface{}) error {
func (s *ImageSigner) Sign(ro *cosignoptions.RootOptions, report platform.ExportReport, secretLocation string, annotations, cosignRepositories, cosignDockerMediaTypes map[string]interface{}) error {
cosignSecrets, err := findCosignSecrets(secretLocation)
if err != nil {
return errors.Errorf("no keys found for cosign signing: %v\n", err)
Expand All @@ -64,11 +64,11 @@ func (s *ImageSigner) Sign(ro *options.RootOptions, report platform.ExportReport
return nil
}

func (s *ImageSigner) sign(ro *options.RootOptions, refImage, secretLocation, cosignSecret string, annotations, cosignRepositories, cosignDockerMediaTypes map[string]interface{}) error {
func (s *ImageSigner) sign(ro *cosignoptions.RootOptions, refImage, secretLocation, cosignSecret string, annotations, cosignRepositories, cosignDockerMediaTypes map[string]interface{}) error {
cosignKeyFile := fmt.Sprintf("%s/%s/cosign.key", secretLocation, cosignSecret)
cosignPasswordFile := fmt.Sprintf("%s/%s/cosign.password", secretLocation, cosignSecret)

ko := options.KeyOpts{KeyRef: cosignKeyFile, PassFunc: func(bool) ([]byte, error) {
ko := cosignoptions.KeyOpts{KeyRef: cosignKeyFile, PassFunc: func(bool) ([]byte, error) {
content, err := ioutil.ReadFile(cosignPasswordFile)
// When password file is not available, default empty password is used
if err != nil {
Expand Down Expand Up @@ -97,9 +97,9 @@ func (s *ImageSigner) sign(ro *options.RootOptions, refImage, secretLocation, co
cosignAnnotations = append(cosignAnnotations, fmt.Sprintf("%s=%s", key, value))
}

signOptions := options.SignOptions{
Registry: options.RegistryOptions{KubernetesKeychain: true},
AnnotationOptions: options.AnnotationOptions{
signOptions := cosignoptions.SignOptions{
Registry: cosignoptions.RegistryOptions{KubernetesKeychain: true},
AnnotationOptions: cosignoptions.AnnotationOptions{
Annotations: cosignAnnotations,
},
Upload: true,
Expand Down Expand Up @@ -129,7 +129,7 @@ func (s *ImageSigner) SignBuilder(

for _, cosignSecret := range cosignSecrets {
keyRef := fmt.Sprintf("k8s://%s/%s", cosignSecret.Namespace, cosignSecret.Name)
keyOpts := options.KeyOpts{
keyOpts := cosignoptions.KeyOpts{
KeyRef: keyRef,
PassFunc: func(bool) ([]byte, error) {
if password, ok := cosignSecret.Data[cosignutil.SecretDataCosignPassword]; ok {
Expand All @@ -152,17 +152,17 @@ func (s *ImageSigner) SignBuilder(
}
}

registryOptions := options.RegistryOptions{KubernetesKeychain: true, Keychain: builderKeychain}
registryOptions := cosignoptions.RegistryOptions{KubernetesKeychain: true, Keychain: builderKeychain}

signOptions := options.SignOptions{
signOptions := cosignoptions.SignOptions{
Registry: registryOptions,
AnnotationOptions: options.AnnotationOptions{},
AnnotationOptions: cosignoptions.AnnotationOptions{},
Upload: true,
Recursive: false,
TlogUpload: false,
}

rootOptions := options.RootOptions{}
rootOptions := cosignoptions.RootOptions{Timeout: cosignoptions.DefaultTimeout}

if err := s.signFunc(
&rootOptions,
Expand Down
4 changes: 2 additions & 2 deletions pkg/cosign/image_signer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -527,7 +527,7 @@ func testImageSigner(t *testing.T, when spec.G, it spec.S) {
}

fakeSecret := cosigntesting.GenerateFakeKeyPair(t, cosignSecretName, testNamespaceName, "", nil)
cosignCreds := []corev1.Secret{fakeSecret}
cosignCreds := []*corev1.Secret{&fakeSecret}
cosignSA := corev1.ServiceAccount{
ObjectMeta: metav1.ObjectMeta{
Name: cosignServiceAccountName,
Expand Down Expand Up @@ -605,7 +605,7 @@ func testImageSigner(t *testing.T, when spec.G, it spec.S) {
}

fakeSecret := cosigntesting.GenerateFakeKeyPair(t, cosignSecretName, testNamespaceName, "", annotations)
cosignCreds := []corev1.Secret{fakeSecret}
cosignCreds := []*corev1.Secret{&fakeSecret}
cosignSA := corev1.ServiceAccount{
ObjectMeta: metav1.ObjectMeta{
Name: cosignServiceAccountName,
Expand Down
4 changes: 2 additions & 2 deletions pkg/reconciler/builder/builder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func NewController(
buildpackInformer buildinformers.BuildpackInformer,
clusterBuildpackInformer buildinformers.ClusterBuildpackInformer,
clusterStackInformer buildinformers.ClusterStackInformer,
secretFetcher *secret.Fetcher,
secretFetcher secret.IFetcher,
) (*controller.Impl, func()) {
c := &Reconciler{
Client: opt.Client,
Expand Down Expand Up @@ -114,7 +114,7 @@ type Reconciler struct {
BuildpackLister buildlisters.BuildpackLister
ClusterBuildpackLister buildlisters.ClusterBuildpackLister
ClusterStackLister buildlisters.ClusterStackLister
SecretFetcher *secret.Fetcher
SecretFetcher secret.IFetcher
}

func (c *Reconciler) Reconcile(ctx context.Context, key string) error {
Expand Down
15 changes: 10 additions & 5 deletions pkg/reconciler/builder/builder_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import (
"errors"
"testing"

"github.com/pivotal/kpack/pkg/secret/secretfakes"

k8sfake "k8s.io/client-go/kubernetes/fake"

"github.com/sclevine/spec"
Expand Down Expand Up @@ -44,9 +46,12 @@ func testBuilderReconciler(t *testing.T, when spec.G, it spec.S) {
)

var (
builderCreator = &testhelpers.FakeBuilderCreator{}
keychainFactory = &registryfakes.FakeKeychainFactory{}
fakeTracker = &testhelpers.FakeTracker{}
builderCreator = &testhelpers.FakeBuilderCreator{}
keychainFactory = &registryfakes.FakeKeychainFactory{}
fakeTracker = &testhelpers.FakeTracker{}
fakeSecretFetcher = &secretfakes.FakeFetchSecret{
FakeSecrets: []*corev1.Secret{},
}
)

rt := testhelpers.ReconcilerTester(t,
Expand All @@ -64,7 +69,7 @@ func testBuilderReconciler(t *testing.T, when spec.G, it spec.S) {
BuildpackLister: listers.GetBuildpackLister(),
ClusterBuildpackLister: listers.GetClusterBuildpackLister(),
ClusterStackLister: listers.GetClusterStackLister(),
K8sClient: k8sfakeClient,
SecretFetcher: fakeSecretFetcher,
}
return &kreconciler.NetworkErrorReconciler{Reconciler: r}, rtesting.ActionRecorderList{fakeClient, k8sfakeClient}, rtesting.EventList{Recorder: record.NewFakeRecorder(10)}
})
Expand Down Expand Up @@ -290,7 +295,7 @@ func testBuilderReconciler(t *testing.T, when spec.G, it spec.S) {
Fetcher: expectedFetcher,
ClusterStack: clusterStack,
BuilderSpec: builder.Spec.BuilderSpec,
SigningSecrets: []corev1.Secret{},
SigningSecrets: []*corev1.Secret{},
}}, builderCreator.CreateBuilderCalls)
})

Expand Down
4 changes: 2 additions & 2 deletions pkg/reconciler/clusterbuilder/clusterbuilder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func NewController(
clusterStoreInformer buildinformers.ClusterStoreInformer,
clusterBuildpackInformer buildinformers.ClusterBuildpackInformer,
clusterStackInformer buildinformers.ClusterStackInformer,
secretFetcher *secret.Fetcher,
secretFetcher secret.IFetcher,
) (*controller.Impl, func()) {
c := &Reconciler{
Client: opt.Client,
Expand Down Expand Up @@ -106,7 +106,7 @@ type Reconciler struct {
ClusterStoreLister buildlisters.ClusterStoreLister
ClusterBuildpackLister buildlisters.ClusterBuildpackLister
ClusterStackLister buildlisters.ClusterStackLister
SecretFetcher *secret.Fetcher
SecretFetcher secret.IFetcher
}

func (c *Reconciler) Reconcile(ctx context.Context, key string) error {
Expand Down
Loading

0 comments on commit 4873c59

Please sign in to comment.