Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use keychain from clusterstack to pull build image in CreateBuild #1223

Merged
merged 1 commit into from
May 24, 2023
Merged

Use keychain from clusterstack to pull build image in CreateBuild #1223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 3 additions & 7 deletions pkg/cnb/create_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
}
Expand Down
29 changes: 15 additions & 14 deletions pkg/cnb/create_builder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 (
Expand All @@ -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{
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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")
})

Expand All @@ -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")
})

Expand Down Expand Up @@ -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)
})

Expand All @@ -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")
})

Expand All @@ -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")
})

Expand Down Expand Up @@ -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)
})
})
Expand All @@ -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")
})
})
Expand Down
17 changes: 14 additions & 3 deletions pkg/reconciler/builder/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little hesitant on falling back to the stack keychain, this feels like compromising design for backwards compatibility. That being said, I'm not too married to this idea and certainly not enough to block this PR

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
}
Expand Down
17 changes: 12 additions & 5 deletions pkg/reconciler/builder/builder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)
})

Expand Down
17 changes: 14 additions & 3 deletions pkg/reconciler/clusterbuilder/clusterbuilder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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
}
Expand Down
17 changes: 12 additions & 5 deletions pkg/reconciler/clusterbuilder/clusterbuilder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)
})

Expand Down
24 changes: 13 additions & 11 deletions pkg/reconciler/testhelpers/fake_builder_creator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down