Use keychain from clusterstack to pull build image in CreateBuild

- fallback to builder keychain if there is no service account on the clusterstack to mimic existing behavior

Author: tomkennedy513

pkg/cnb/create_builder.go

@@ -5,6 +5,7 @@ import (
 
 	"github.com/google/go-containerregistry/pkg/authn"
 	ggcrv1 "github.com/google/go-containerregistry/pkg/v1"
+
 	buildapi "github.com/pivotal/kpack/pkg/apis/build/v1alpha2"
 	corev1alpha1 "github.com/pivotal/kpack/pkg/apis/core/v1alpha1"
 	"github.com/pivotal/kpack/pkg/registry"
@@ -26,13 +27,8 @@ type RemoteBuilderCreator struct {
 	KeychainFactory   registry.KeychainFactory
 }
 
-func (r *RemoteBuilderCreator) CreateBuilder(
-	ctx context.Context,
-	builderKeychain authn.Keychain,
-	fetcher RemoteBuildpackFetcher,
-	clusterStack *buildapi.ClusterStack, spec buildapi.BuilderSpec,
-) (buildapi.BuilderRecord, error) {
-	buildImage, _, err := r.RegistryClient.Fetch(builderKeychain, clusterStack.Status.BuildImage.LatestImage)
+func (r *RemoteBuilderCreator) CreateBuilder(ctx context.Context, builderKeychain authn.Keychain, stackKeychain authn.Keychain, fetcher RemoteBuildpackFetcher, clusterStack *buildapi.ClusterStack, spec buildapi.BuilderSpec) (buildapi.BuilderRecord, error) {
+	buildImage, _, err := r.RegistryClient.Fetch(stackKeychain, clusterStack.Status.BuildImage.LatestImage)
 	if err != nil {
 		return buildapi.BuilderRecord{}, err
 	}

pkg/cnb/create_builder_test.go

@@ -57,7 +57,8 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
 		registryClient = registryfakes.NewFakeClient()
 
 		keychainFactory = &registryfakes.FakeKeychainFactory{}
-		keychain        = authn.NewMultiKeychain(authn.DefaultKeychain)
+		builderKeychain = authn.NewMultiKeychain(authn.DefaultKeychain)
+		stackKeychain   = authn.NewMultiKeychain(authn.DefaultKeychain)
 		secretRef       = registry.SecretRef{}
 
 		ctx = context.Background()
@@ -190,7 +191,7 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
 	)
 
 	it.Before(func() {
-		keychainFactory.AddKeychainForSecretRef(t, secretRef, keychain)
+		keychainFactory.AddKeychainForSecretRef(t, secretRef, builderKeychain)
 
 		buildpack1 := buildpackLayer{
 			v1Layer: buildpack1Layer,
@@ -267,7 +268,7 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
 		fetcher.AddBuildpack(t, "io.buildpack.2", "v2", []buildpackLayer{buildpack3, buildpack2})
 	})
 
-	registryClient.AddSaveKeychain("custom/example", keychain)
+	registryClient.AddSaveKeychain("custom/example", builderKeychain)
 
 	when("CreateBuilder", func() {
 		var (
@@ -286,7 +287,7 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
 			config.OS = os
 			buildImg, err = mutate.ConfigFile(buildImg, config)
 
-			registryClient.AddImage(buildImage, buildImg, keychain)
+			registryClient.AddImage(buildImage, buildImg, stackKeychain)
 
 			lifecycleProvider.metadata = LifecycleMetadata{
 				LifecycleInfo: LifecycleInfo{
@@ -314,7 +315,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, keychain, fetcher, stack, clusterBuilderSpec)
+			builderRecord, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec)
 			require.NoError(t, err)
 
 			assert.Len(t, builderRecord.Buildpacks, 3)
@@ -576,11 +577,11 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
 		})
 
 		it("creates images deterministically ", func() {
-			original, err := subject.CreateBuilder(ctx, keychain, fetcher, stack, clusterBuilderSpec)
+			original, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec)
 			require.NoError(t, err)
 
 			for i := 1; i <= 50; i++ {
-				other, err := subject.CreateBuilder(ctx, keychain, fetcher, stack, clusterBuilderSpec)
+				other, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec)
 				require.NoError(t, err)
 
 				require.Equal(t, original.Image, other.Image)
@@ -610,7 +611,7 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
 					},
 				}
 
-				_, err := subject.CreateBuilder(ctx, keychain, fetcher, stack, clusterBuilderSpec)
+				_, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec)
 				require.EqualError(t, err, "validating buildpack io.buildpack.unsupported.stack@v4: stack io.buildpacks.stacks.some-stack is not supported")
 			})
 
@@ -634,7 +635,7 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
 					}},
 				}}
 
-				_, err := subject.CreateBuilder(ctx, keychain, fetcher, stack, clusterBuilderSpec)
+				_, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec)
 				require.EqualError(t, err, "validating buildpack io.buildpack.unsupported.mixin@v4: stack missing mixin(s): something-missing-mixin, something-missing-mixin2")
 			})
 
@@ -679,7 +680,7 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
 					}},
 				}}
 
-				_, err := subject.CreateBuilder(ctx, keychain, fetcher, stack, clusterBuilderSpec)
+				_, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec)
 				require.Nil(t, err)
 			})
 
@@ -704,7 +705,7 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
 					}},
 				}}
 
-				_, err := subject.CreateBuilder(ctx, keychain, fetcher, stack, clusterBuilderSpec)
+				_, err := subject.CreateBuilder(ctx, builderKeychain, nil, fetcher, stack, clusterBuilderSpec)
 				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")
 			})
 
@@ -727,7 +728,7 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
 					}},
 				}}
 
-				_, err := subject.CreateBuilder(ctx, keychain, fetcher, stack, clusterBuilderSpec)
+				_, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec)
 				require.EqualError(t, err, "validating buildpack io.buildpack.unsupported.buildpack.api@v4: unsupported buildpack api: 0.1, expecting: 0.2, 0.3")
 			})
 
@@ -770,7 +771,7 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
 					}},
 				}}
 
-				_, err := subject.CreateBuilder(ctx, keychain, fetcher, stack, clusterBuilderSpec)
+				_, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec)
 				require.NoError(t, err)
 			})
 		})
@@ -797,7 +798,7 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
 					},
 				}
 
-				_, err := subject.CreateBuilder(ctx, keychain, fetcher, stack, clusterBuilderSpec)
+				_, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec)
 				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")
 			})
 		})

pkg/reconciler/builder/builder.go

@@ -33,7 +33,7 @@ const (
 )
 
 type BuilderCreator interface {
-	CreateBuilder(ctx context.Context, keychain authn.Keychain, fetcher cnb.RemoteBuildpackFetcher, clusterStack *buildapi.ClusterStack, spec buildapi.BuilderSpec) (buildapi.BuilderRecord, error)
+	CreateBuilder(ctx context.Context, builderKeychain authn.Keychain, keychain authn.Keychain, fetcher cnb.RemoteBuildpackFetcher, clusterStack *buildapi.ClusterStack, spec buildapi.BuilderSpec) (buildapi.BuilderRecord, error)
 }
 
 func NewController(
@@ -203,17 +203,28 @@ func (c *Reconciler) reconcileBuilder(ctx context.Context, builder *buildapi.Bui
 		return buildapi.BuilderRecord{}, errors.Errorf("stack %s is not ready", clusterStack.Name)
 	}
 
-	keychain, err := c.KeychainFactory.KeychainForSecretRef(ctx, registry.SecretRef{
+	builderKeychain, err := c.KeychainFactory.KeychainForSecretRef(ctx, registry.SecretRef{
 		ServiceAccount: builder.Spec.ServiceAccount(),
 		Namespace:      builder.Namespace,
 	})
 	if err != nil {
 		return buildapi.BuilderRecord{}, err
 	}
 
+	stackKeychain := builderKeychain
+	if clusterStack.Spec.ServiceAccountRef != nil {
+		stackKeychain, err = c.KeychainFactory.KeychainForSecretRef(ctx, registry.SecretRef{
+			ServiceAccount: clusterStack.Spec.ServiceAccountRef.Name,
+			Namespace:      clusterStack.Spec.ServiceAccountRef.Namespace,
+		})
+		if err != nil {
+			return buildapi.BuilderRecord{}, err
+		}
+	}
+
 	fetcher := cnb.NewRemoteBuildpackFetcher(c.KeychainFactory, clusterStore, buildpacks, clusterBuildpacks)
 
-	buildRecord, err := c.BuilderCreator.CreateBuilder(ctx, keychain, fetcher, clusterStack, builder.Spec.BuilderSpec)
+	buildRecord, err := c.BuilderCreator.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, clusterStack, builder.Spec.BuilderSpec)
 	if err != nil {
 		return buildapi.BuilderRecord{}, err
 	}

pkg/reconciler/builder/builder_test.go

@@ -85,6 +85,12 @@ func testBuilderReconciler(t *testing.T, when spec.G, it spec.S) {
 			Kind:       "ClusterStack",
 			APIVersion: "kpack.io/v1alpha2",
 		},
+		Spec: buildapi.ClusterStackSpec{
+			ServiceAccountRef: &corev1.ObjectReference{
+				Name:      "some-service-account",
+				Namespace: testNamespace,
+			},
+		},
 		Status: buildapi.ClusterStackStatus{
 			Status: corev1alpha1.Status{
 				ObservedGeneration: 0,
@@ -247,11 +253,12 @@ func testBuilderReconciler(t *testing.T, when spec.G, it spec.S) {
 			})
 
 			assert.Equal(t, []testhelpers.CreateBuilderArgs{{
-				Context:      context.Background(),
-				Keychain:     &registryfakes.FakeKeychain{},
-				Fetcher:      expectedFetcher,
-				ClusterStack: clusterStack,
-				BuilderSpec:  builder.Spec.BuilderSpec,
+				Context:         context.Background(),
+				BuilderKeychain: &registryfakes.FakeKeychain{},
+				StackKeychain:   &registryfakes.FakeKeychain{},
+				Fetcher:         expectedFetcher,
+				ClusterStack:    clusterStack,
+				BuilderSpec:     builder.Spec.BuilderSpec,
 			}}, builderCreator.CreateBuilderCalls)
 		})
 

pkg/reconciler/clusterbuilder/clusterbuilder.go

@@ -34,7 +34,7 @@ const (
 )
 
 type BuilderCreator interface {
-	CreateBuilder(ctx context.Context, keychain authn.Keychain, fetcher cnb.RemoteBuildpackFetcher, clusterStack *buildapi.ClusterStack, spec buildapi.BuilderSpec) (buildapi.BuilderRecord, error)
+	CreateBuilder(ctx context.Context, builderKeychain authn.Keychain, stackKeychain authn.Keychain, fetcher cnb.RemoteBuildpackFetcher, clusterStack *buildapi.ClusterStack, spec buildapi.BuilderSpec) (buildapi.BuilderRecord, error)
 }
 
 func NewController(
@@ -188,17 +188,28 @@ func (c *Reconciler) reconcileBuilder(ctx context.Context, builder *buildapi.Clu
 		return buildapi.BuilderRecord{}, errors.Errorf("stack %s is not ready", clusterStack.Name)
 	}
 
-	keychain, err := c.KeychainFactory.KeychainForSecretRef(ctx, registry.SecretRef{
+	builderKeychain, err := c.KeychainFactory.KeychainForSecretRef(ctx, registry.SecretRef{
 		ServiceAccount: builder.Spec.ServiceAccountRef.Name,
 		Namespace:      builder.Spec.ServiceAccountRef.Namespace,
 	})
 	if err != nil {
 		return buildapi.BuilderRecord{}, err
 	}
 
+	stackKeychain := builderKeychain
+	if clusterStack.Spec.ServiceAccountRef != nil {
+		stackKeychain, err = c.KeychainFactory.KeychainForSecretRef(ctx, registry.SecretRef{
+			ServiceAccount: clusterStack.Spec.ServiceAccountRef.Name,
+			Namespace:      clusterStack.Spec.ServiceAccountRef.Namespace,
+		})
+		if err != nil {
+			return buildapi.BuilderRecord{}, err
+		}
+	}
+
 	fetcher := cnb.NewRemoteBuildpackFetcher(c.KeychainFactory, clusterStore, nil, clusterBuildpacks)
 
-	buildRecord, err := c.BuilderCreator.CreateBuilder(ctx, keychain, fetcher, clusterStack, builder.Spec.BuilderSpec)
+	buildRecord, err := c.BuilderCreator.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, clusterStack, builder.Spec.BuilderSpec)
 	if err != nil {
 		return buildapi.BuilderRecord{}, err
 	}

pkg/reconciler/clusterbuilder/clusterbuilder_test.go

@@ -83,6 +83,12 @@ func testClusterBuilderReconciler(t *testing.T, when spec.G, it spec.S) {
 			Kind:       "ClusterStack",
 			APIVersion: "kpack.io/v1alpha2",
 		},
+		Spec: buildapi.ClusterStackSpec{
+			ServiceAccountRef: &corev1.ObjectReference{
+				Namespace: "some-sa-namespace",
+				Name:      "some-sa-name",
+			},
+		},
 		Status: buildapi.ClusterStackStatus{
 			Status: corev1alpha1.Status{
 				ObservedGeneration: 0,
@@ -243,11 +249,12 @@ func testClusterBuilderReconciler(t *testing.T, when spec.G, it spec.S) {
 			})
 
 			assert.Equal(t, []testhelpers.CreateBuilderArgs{{
-				Context:      context.Background(),
-				Keychain:     &registryfakes.FakeKeychain{},
-				Fetcher:      expectedFetcher,
-				ClusterStack: clusterStack,
-				BuilderSpec:  builder.Spec.BuilderSpec,
+				Context:         context.Background(),
+				BuilderKeychain: &registryfakes.FakeKeychain{},
+				StackKeychain:   &registryfakes.FakeKeychain{},
+				Fetcher:         expectedFetcher,
+				ClusterStack:    clusterStack,
+				BuilderSpec:     builder.Spec.BuilderSpec,
 			}}, builderCreator.CreateBuilderCalls)
 		})
 

pkg/reconciler/testhelpers/fake_builder_creator.go

@@ -19,20 +19,22 @@ type FakeBuilderCreator struct {
 }
 
 type CreateBuilderArgs struct {
-	Context      context.Context
-	Keychain     authn.Keychain
-	Fetcher      cnb.RemoteBuildpackFetcher
-	ClusterStack *buildapi.ClusterStack
-	BuilderSpec  buildapi.BuilderSpec
+	Context         context.Context
+	BuilderKeychain authn.Keychain
+	StackKeychain   authn.Keychain
+	Fetcher         cnb.RemoteBuildpackFetcher
+	ClusterStack    *buildapi.ClusterStack
+	BuilderSpec     buildapi.BuilderSpec
 }
 
-func (f *FakeBuilderCreator) CreateBuilder(ctx context.Context, keychain authn.Keychain, fetcher cnb.RemoteBuildpackFetcher, clusterStack *buildapi.ClusterStack, builder buildapi.BuilderSpec) (buildapi.BuilderRecord, error) {
+func (f *FakeBuilderCreator) CreateBuilder(ctx context.Context, builderKeychain authn.Keychain, stackKeychain authn.Keychain, fetcher cnb.RemoteBuildpackFetcher, clusterStack *buildapi.ClusterStack, spec buildapi.BuilderSpec) (buildapi.BuilderRecord, error) {
 	f.CreateBuilderCalls = append(f.CreateBuilderCalls, CreateBuilderArgs{
-		Context:      ctx,
-		Keychain:     keychain,
-		Fetcher:      fetcher,
-		ClusterStack: clusterStack,
-		BuilderSpec:  builder,
+		Context:         ctx,
+		BuilderKeychain: builderKeychain,
+		StackKeychain:   stackKeychain,
+		Fetcher:         fetcher,
+		ClusterStack:    clusterStack,
+		BuilderSpec:     spec,
 	})
 
 	return f.Record, f.CreateErr