added keystore support for custom functions auth by AndrewChubatiuk · Pull Request #3869 · crossplane/crossplane

AndrewChubatiuk · 2023-03-15T08:46:44Z

Description of your changes

crossplane-xfn doesn't support pulling images from:

registries such as ECR, ACR, which are using temporary credentials
service account credentials
kube secrets

Fixes #3868
Fixes #3717

I have:

Read and followed Crossplane's [contribution process].
Run make reviewable to ensure this PR is ready for review.
Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Build container locally with precompiled crun v1.8.1 and tested in AWS EKS with and without an access to ECR

negz

This looks good to me at first glance. Relates loosely to #3717

Could you please fill out the "how has this been tested" section of the PR template, and sign the DCO. You'll need to do both for us to accept this PR.

AndrewChubatiuk · 2023-03-16T10:35:33Z

This looks good to me at first glance. Relates loosely to #3717

Could you please fill out the "how has this been tested" section of the PR template, and sign the DCO. You'll need to do both for us to accept this PR.

It's not hard to add a support for #3717 as well
Added both test section and DCO

jbw976 · 2023-03-17T15:39:29Z

Thanks a lot @AndrewChubatiuk for taking a stab at fixing these issues that have been blocking some early adopters progress on testing composition functions! That's very helpful 🤓

made it compatible with crun 1.8.1.

Do the changes to make it compatible with crun 1.8.1 have any impact to compatibility with older versions of crun? Should we be considering pinning the version of crun we use, as suggested by @negz here?

bobh66 · 2023-03-17T16:30:04Z

Great work @AndrewChubatiuk ! Thanks for digging into this!
Probably a good idea to pin crun to 1.8.1 as part of these changes, if that interface is going to be fragile.

AndrewChubatiuk · 2023-03-17T20:15:09Z

it should work with older versions, but anyway only 1.8.1 is available now for debian. I was using build container to test with older versions of crun

negz

Thanks @AndrewChubatiuk. A few comments, but this looks good overall.

Please squash/cleanup your git commits before we merge this (but please do keep the sysfs fix in a separate commit).

negz · 2023-03-22T03:48:25Z

internal/oci/pull.go

 	}
-	if opts.pull == ImagePullPolicyNever {
-		return nil, errors.New(errPullNever)
+	keychain, err := k8schain.NewInCluster(ctx, k8schain.Options{})


Will NewInCluster always fail if in-cluster auth isn't supported?

Or more generally, what causes NewInCluster to fail and what causes remote.WithAuthFromKeychain to fail? It may be worth adding comments to clarify.

internal/oci/pull.go

AndrewChubatiuk · 2023-03-22T16:35:46Z

@negz
Moved crun changes to #3893
In this PR additionally:

squashed all commits
added support for imagePullSecrets and service account credentials
removed unused ImagePullAuth

apis/apiextensions/fn/proto/v1alpha1/run_function.proto

apis/apiextensions/v1/composition_types.go

internal/xpkg/fetch.go

turkenh

Thanks @AndrewChubatiuk, left a couple of comments!

I don't think this PR will be fixing #3718 though which is more about passing credentials for the function itself rather than pulling it image.

turkenh · 2023-04-12T10:07:36Z

apis/apiextensions/v1/composition_types.go


+	// Secrets for pulling function images.
+	// +optional
+	ImagePullSecrets []ContainerFunctionImagePullSecret `json:"imagePullSecrets,omitempty"`


Suggested change

ImagePullSecrets []ContainerFunctionImagePullSecret `json:"imagePullSecrets,omitempty"`

ImagePullSecrets []corev1.LocalObjectReference `json:"imagePullSecrets,omitempty"`

Any reason not to use the same type as packagePullSecrets?

Thanks @AndrewChubatiuk, left a couple of comments!

I don't think this PR will be fixing #3718 though which is more about passing credentials for the function itself rather than pulling it image.

it's true, removed this issue from PR

internal/controller/apiextensions/composite/composition_ptf.go

AndrewChubatiuk · 2023-04-14T12:01:35Z

@turkenh made changes, not sure how it's better to pass service account and namespace to RunFunction

AndrewChubatiuk · 2023-04-27T16:16:09Z

@turkenh @negz are there any additional changes needed?

negz

Sorry this review took so long! I had to switch focus for a while but I expect to be working on Composition Functions again for at least a few weeks. I do think this needs a few more changes - please ping me on Crossplane Slack when you're ready for me to take another look.

cmd/crossplane/core/core.go

cmd/xfn/run/run.go

internal/controller/apiextensions/controller/options.go

cmd/xfn/run/run.go

apis/apiextensions/fn/io/v1alpha1/functionio_types.go

internal/controller/apiextensions/composite/composition_ptf.go

internal/controller/apiextensions/composite/composition_ptf_test.go

internal/controller/apiextensions/composite/composition_ptf.go

Signed-off-by: Andrew Chubatiuk <andrew.chubatiuk@motional.com>

negz

Thanks for persisting with this @AndrewChubatiuk! It looks great. I have some ideas for small improvements, but I can open a follow-up PR for those. I'll copy you on that.

This reduces the complexity of RunFunction a little. This commit also has a few small cleanups following on to crossplane#3869 Signed-off-by: Nic Cope <nicc@rk0n.org>

AndrewChubatiuk requested review from a team as code owners March 15, 2023 08:46

AndrewChubatiuk requested review from negz and nullable-eth March 15, 2023 08:46

AndrewChubatiuk force-pushed the xfn-auth-keystore branch 2 times, most recently from 80170da to d628e7a Compare March 15, 2023 09:58

negz reviewed Mar 16, 2023

View reviewed changes

AndrewChubatiuk force-pushed the xfn-auth-keystore branch from 8e45200 to 2f6762f Compare March 16, 2023 09:07

AndrewChubatiuk force-pushed the xfn-auth-keystore branch from 05b1321 to 63eb1eb Compare March 17, 2023 11:08

AndrewChubatiuk mentioned this pull request Mar 17, 2023

cannot run composition function with crossplane/xfn:v1.11.1 #3807

Closed

negz added the backport release-1.11 label Mar 22, 2023

negz reviewed Mar 22, 2023

View reviewed changes

AndrewChubatiuk requested review from hasheddan and muvaf as code owners March 22, 2023 16:08

AndrewChubatiuk force-pushed the xfn-auth-keystore branch 3 times, most recently from 89df254 to 5dfec14 Compare March 22, 2023 16:32

negz removed the backport release-1.11 label Mar 22, 2023

negz added this to the v1.12 milestone Mar 22, 2023

negz reviewed Mar 23, 2023

View reviewed changes

apis/apiextensions/fn/proto/v1alpha1/run_function.proto Show resolved Hide resolved

negz requested changes Mar 23, 2023

View reviewed changes

apis/apiextensions/v1/composition_types.go Outdated Show resolved Hide resolved

internal/xpkg/fetch.go Show resolved Hide resolved

AndrewChubatiuk force-pushed the xfn-auth-keystore branch 2 times, most recently from 3c7e814 to 5f660df Compare March 26, 2023 06:41

AndrewChubatiuk requested a review from a team as a code owner March 26, 2023 06:41

AndrewChubatiuk requested a review from bassam March 26, 2023 06:41

AndrewChubatiuk force-pushed the xfn-auth-keystore branch 2 times, most recently from a3dc9b5 to 800db13 Compare March 26, 2023 06:52

turkenh reviewed Apr 12, 2023

View reviewed changes

AndrewChubatiuk force-pushed the xfn-auth-keystore branch 5 times, most recently from a5186f3 to 41b0fb3 Compare April 18, 2023 19:03

jbw976 removed this from the v1.12 milestone Apr 24, 2023

negz requested changes May 4, 2023

View reviewed changes

AndrewChubatiuk force-pushed the xfn-auth-keystore branch 6 times, most recently from 606d6e5 to 9d55288 Compare May 5, 2023 04:34

added composite function private images auth

2d9f645

Signed-off-by: Andrew Chubatiuk <andrew.chubatiuk@motional.com>

AndrewChubatiuk force-pushed the xfn-auth-keystore branch from 9d55288 to 2d9f645 Compare May 5, 2023 04:36

negz approved these changes May 5, 2023

View reviewed changes

negz merged commit 9beac48 into crossplane:master May 5, 2023

AndrewChubatiuk deleted the xfn-auth-keystore branch May 5, 2023 22:02

negz mentioned this pull request May 6, 2023

Refactor in-cluster auth into a ContainerFunctionRunnerOption #4054

Merged

3 tasks

negz mentioned this pull request May 10, 2023

Implement imagePullSecrets for Composition Functions #3717

Closed

	ImagePullSecrets []ContainerFunctionImagePullSecret `json:"imagePullSecrets,omitempty"`
	ImagePullSecrets []corev1.LocalObjectReference `json:"imagePullSecrets,omitempty"`

Conversation

AndrewChubatiuk commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of your changes

How has this code been tested

Uh oh!

negz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewChubatiuk commented Mar 16, 2023

Uh oh!

jbw976 commented Mar 17, 2023

Uh oh!

bobh66 commented Mar 17, 2023

Uh oh!

AndrewChubatiuk commented Mar 17, 2023

Uh oh!

negz left a comment

Choose a reason for hiding this comment

Uh oh!

negz Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AndrewChubatiuk commented Mar 22, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

turkenh left a comment

Choose a reason for hiding this comment

Uh oh!

turkenh Apr 12, 2023

Choose a reason for hiding this comment

Uh oh!

AndrewChubatiuk Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AndrewChubatiuk commented Apr 14, 2023

Uh oh!

AndrewChubatiuk commented Apr 27, 2023

Uh oh!

negz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

negz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

AndrewChubatiuk commented Mar 15, 2023 •

edited

Loading

negz left a comment •

edited

Loading