Introduce `upload` source and `rukpakctl run` #417

joelanford · 2022-06-03T14:57:15Z

This PR introduces an upload source and a rukpakctl run command that makes use of the new upload source.

Closes #472

The local source is a great way to help us get up and running quickly with a mechanism that enables direct usage bundle contents to a kubernetes cluster without requiring those bundles to be pushed to a git remote or an image registry. However, the local source's use of configmaps could only get us so far. Configmap size is restricted by etcd configuration (typically 2MB), so the local source could only support bundles up to 2MB in size. Some improvements for the local source are discussed in #464.

The upload source does not place this restriction on bundle size because it does not rely on etcd for storage of local bundle content.

An upload source enables bundle developers or other clients to directly upload bundle tar.gz files to inject them into bundles.

This introduces a upload manager service that:

accepts uploads for bundles with the upload source type
serves those bundles to provisioners that request them
garbage collects its local storage when bundle uploads are no longer needed

This also introduces client tooling necessary to actually exercise this functionality in a user-friendly way. It:

Adds rukpakctl run to create a BD with an upload source bundle, get the name of the generated bundle, and then upload the bundle contents to the upload manager, which then makes that bundle content available to the bundle provisioners.
Adds a simple library for running a port-forward to a specific service in the cluster (in this case, we use it to connect to the upload manager from outside the cluster)
A BundleUploader type that is essentially a small API for actually doing the upload. This is used in rukpakctl run and also in the e2e tests.

joelanford · 2022-06-03T21:26:26Z

Hmm.... I wonder if the inclusion of the binary: {} struct in the Bundle API is causing go-spew (and thus the hashing for name generation) to change the output. I ran the e2e twice and I'm pretty sure this PR's code produced the same (incorrect, according to the tests) hash suffix for the generated bundle.

EDIT: Yeah it sounds like upstream has a similar problem: https://github.com/kubernetes/kubernetes/blob/b7337cb171ab126cb892aacdab2816017a290841/pkg/controller/deployment/util/deployment_util.go#L598-L599

akihikokuroda · 2022-06-06T17:53:03Z

About the client tool, can it create the bundle instead of the bundle instance embedding a bundle? So it doesn't need to wait for the bundle name is determined. we can have another command to create the bundle instance that specify the bundle.

joelanford · 2022-06-17T14:22:48Z

About the client tool, can it create the bundle instead of the bundle instance embedding a bundle? So it doesn't need to wait for the bundle name is determined. we can have another command to create the bundle instance that specify the bundle.

To actually run the bundle, we have to create a bundle instance that embeds the binary-type bundle. So at least in that case, I don't think we have any choice but to:

Create the BI
Wait for the bundle to exist
Upload the tar.gz for that bundle name

akihikokuroda · 2022-06-17T14:30:23Z

@joelanford Thanks! I didn't realize that the bundleinstance has to embed the bundle. I thought it could still reference a bundle.

openshift-ci · 2022-07-14T22:21:16Z

@joelanford: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

api/v1alpha1/bundle_types.go

cmd/rukpakctl/cmd/content.go

joelanford · 2022-07-21T15:29:50Z

Do we need to update the core webhook to also accommodate this new source type?

Is there anything I'm missing about what we'd need to check? The bundle spec can't change (and there wouldn't be anything really worth changing since binary isn't configurable)

And the upload manager http server forbids re-uploads of already unpacked binary bundles, so I think that covers the immutability from that perspective.

Adding godocs for some of the exported utility packages that were added.

Will do. They're all under the internal tree so I somewhat glossed over that, but I agree nonetheless. docs > no docs regardless of what/where.

timflannagan · 2022-07-21T16:37:09Z

Is there anything I'm missing about what we'd need to check? The bundle spec can't change (and there wouldn't be anything really worth changing since binary isn't configurable)

This is my bad: I forgot the core webhook just ensures Bundle spec immutability, and we moved the union type validation logic as oneOf CRD schema validations. I was talking about the latter when making that comment.

exdx

LGTM. Nice job!

exdx · 2022-08-02T17:34:39Z

cmd/rukpakctl/cmd/run.go

+	)
+
+	cmd := &cobra.Command{
+		Use:   "run <bundleDeploymentName> <bundleDir>",


WDYT about making the bundleDeploymentName optional, and just using the name of the bundle directory if a BD name is not provided? It makes the UX even more easy

The main reason I chose that is to ensure that the pivot workflow works regardless of whether you use the same directory name. If you had ./my-bundle-v0.1.0 and ./my-bundle-v0.2.0, it would be really easy to create 2 different BDs when what you really want is to re-use the same BD, and pivot it from v0.1.0 to v0.2.0.

cmd/rukpakctl/cmd/run.go

exdx · 2022-08-02T17:47:13Z

internal/util/tar.go

+// This function unsets user and group information in the tar archive so that readers
+// of archives produced by this function do not need to account for differences in


This should help alleviate some of the issues we see when attempting to prune red hat catalog images that include root-permission directories. Maybe this is worth highlighting in the user-facing documentation? If it's not overly technical

Yep, +1 on that recommendation.

It might be a little bit technical given the existing way most users would interact with this (rukpakctl run). That subcommand does all the necessary tar.gz stuff under the hood and it is not exposed at all to users.

Regardless, I could definitely see value in documenting the upload manager service as a separate thing because there may be in-cluster processes that can directly interact with the upload manager without requiring the extra port-forward stuff that rukpakctl run provides.

timflannagan

This is really nice work. Lots of complexity here, and I didn't play around with it locally so YMMV, but the packages that were introduced were pretty clean. Overall, I didn't have any blocking feedback, and I'm excited to see this finally get close to the finish line. I tried to avoid making a mass amount of comments, so I stopped midway through in hopes we can merge this before closing out this current release milestone.

Do we need any follow-up tickets around benchmarking this service? Does it make sense to call out in the user-facing documentation (if we haven't already) and/or the CRD schema documentation that this service can be used to handle arbitrarily sized bundles to avoid the etcd size limitation?

timflannagan · 2022-08-02T19:36:52Z

cmd/rukpakctl/cmd/run.go

+	dynCl, err := dynamic.NewForConfig(cfg)
+	if err != nil {
+		return "", fmt.Errorf("build dynamic client: %v", err)
+	}


Not blocking: create the dynamic client at the driver level, and pass it as a paramter vs. the rest config?

cmd/rukpakctl/cmd/run.go

timflannagan · 2022-08-02T19:39:09Z

cmd/uploadmgr/main.go

+	// NOTE: AddMetricsExtraHandler isn't actually metrics-specific. We can run
+	// whatever handlers we want on the existing webserver that
+	// controller-runtime runs when MetricsBindAddress is configured on the
+	// manager.


timflannagan · 2022-08-02T19:42:13Z

internal/rukpakctl/ca.go

+
+// GetClusterCA returns an x509.CertPool by reading the contents of a Kubernetes Secret. It uses the provided
+// client to get the requested secret and then loads the contents of the secret's "ca.crt" key into the cert pool.
+func GetClusterCA(ctx context.Context, cl client.Reader, ns, secretName string) (*x509.CertPool, error) {


Would it be easier to pass a NamespacedName instead of individual ns/secretName string fields?

timflannagan · 2022-08-02T19:43:54Z

internal/rukpakctl/portforward.go

+	podName := endpoints.Subsets[0].Addresses[0].TargetRef.Name
+	port := pf.port.IntVal
+	if port == 0 {
+		for _, p := range endpoints.Subsets[0].Ports {
+			if p.Name == pf.port.StrVal {
+				port = p.Port
+				break
+			}
+		}
+	}
+	if port == 0 {
+		return fmt.Errorf("could not find port %q for service %q", pf.port.String(), pf.serviceName)
+	}
+
+	path := fmt.Sprintf("/api/v1/namespaces/%s/pods/%s/portforward", pf.serviceNamespace, podName)
+	host := strings.TrimLeft(pf.cfg.Host, "htps:/")
+	serverURL := url.URL{Scheme: "https", Path: path, Host: host}


I think we could modularize this logic, but this is fine for now.

internal/rukpakctl/upload.go

timflannagan · 2022-08-02T19:50:33Z

internal/source/unpacker.go

@@ -90,12 +94,19 @@ func (s *unpacker) Unpack(ctx context.Context, bundle *rukpakv1alpha1.Bundle) (*
 // NewDefaultUnpacker returns a new composite Source that unpacks bundles using
 // a default source mapping with built-in implementations of all of the supported
 // source types.
-func NewDefaultUnpacker(mgr ctrl.Manager, namespace, provisionerName, unpackImage string) (Unpacker, error) {
+//
+// TODO: refactor NewDefaultUnpacker due to growing parameter list


timflannagan · 2022-08-02T19:51:18Z

internal/source/unpacker.go

+		rukpakv1alpha1.SourceTypeUpload: &Upload{
+			baseDownloadURL: baseUploadManagerURL,
+			bearerToken:     mgr.GetConfig().BearerToken,
+			client:          http.Client{Timeout: 10 * time.Second, Transport: httpTransport},


Any value in a constant variable for the timeout period?

Sure, why not!

timflannagan · 2022-08-02T19:52:03Z

internal/util/tar.go

+// This function unsets user and group information in the tar archive so that readers
+// of archives produced by this function do not need to account for differences in


Yep, +1 on that recommendation.

timflannagan · 2022-08-02T19:54:13Z

test/e2e/plain_provisioner_test.go

+				if bundle.Status.Phase != rukpakv1alpha1.PhaseUnpacked {
+					return errors.New("bundle is not unpacked")
+				}
+				return nil


Handle this through gomega matchers?

exdx · 2022-08-02T19:59:43Z

One additional thought I had -- do we ensure that the kube client proxy to the service tears down gracefully after ending the upload? I have seen issues with kubectl port-forward where it would close the stream but the localhost port would still be in use. This would prevent the command from being idempotent.

joelanford · 2022-08-02T21:08:52Z

do we ensure that the kube client proxy to the service tears down gracefully after ending the upload?

If I understand your question, I think the answer is yes. We start a goroutine in the error group whose sole purpose is to wait for the context to be canceled and then close the stop channel.

https://github.com/joelanford/rukpak/blob/01f7e6f85dec68610bf9d0aa390fb991d9c28418/internal/rukpakctl/portforward.go#L104-L108

Then at the very end of that function we call eg.Wait() to block returning from Start() until all error group routines have completed.

joelanford · 2022-08-03T20:20:27Z

CRD schema documentation that this service can be used to handle arbitrarily sized bundles to avoid the etcd size limitation?

I didn't call this out specifically in the "upload" source struct GoDoc because the "image" and "git" sources also support arbitrarily sized bundles. Maybe we should instead call out the local source's limitations?

timflannagan · 2022-08-03T20:22:27Z

Yeah, that sounds reasonable to me. That can be done as a follow-up anyways from my perspective.

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>

joelanford requested a review from a team as a code owner June 3, 2022 14:57

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 3, 2022

joelanford force-pushed the binary-source branch from db23071 to 6b77e0f Compare June 3, 2022 15:04

joelanford force-pushed the binary-source branch 2 times, most recently from 411d98e to 25c1069 Compare June 4, 2022 01:05

joelanford force-pushed the binary-source branch 2 times, most recently from 216edec to b79f77a Compare June 17, 2022 14:10

joelanford force-pushed the binary-source branch from b79f77a to 478e1d8 Compare June 17, 2022 20:42

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2022

joelanford force-pushed the binary-source branch from 478e1d8 to 26d77f9 Compare July 7, 2022 20:57

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 7, 2022

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2022

joelanford force-pushed the binary-source branch from 26d77f9 to 2ab3a02 Compare July 15, 2022 19:08

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2022

joelanford changed the title ~~WIP: Introduce binary source~~ Introduce binary source and rukpakctl run Jul 15, 2022

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 15, 2022

joelanford force-pushed the binary-source branch 4 times, most recently from c2a9fbd to fcc02a5 Compare July 18, 2022 17:26

joelanford mentioned this pull request Jul 18, 2022

rukpakctl test #461

Merged

exdx approved these changes Jul 19, 2022

View reviewed changes

api/v1alpha1/bundle_types.go Outdated Show resolved Hide resolved

cmd/rukpakctl/cmd/content.go Outdated Show resolved Hide resolved

akihikokuroda approved these changes Jul 19, 2022

View reviewed changes

joelanford force-pushed the binary-source branch 2 times, most recently from 204883b to 9c9d013 Compare July 19, 2022 20:03

timflannagan mentioned this pull request Jul 20, 2022

wrap this update call in a retry that ignores conflict errors #452

Closed

joelanford force-pushed the binary-source branch 6 times, most recently from 11554ed to 69c0ae9 Compare July 22, 2022 16:58

joelanford changed the title ~~Introduce binary source and rukpakctl run~~ Introduce upload source and rukpakctl run Jul 25, 2022

joelanford force-pushed the binary-source branch 2 times, most recently from dd26000 to 1c113c0 Compare July 26, 2022 18:24

exdx approved these changes Aug 2, 2022

View reviewed changes

timflannagan approved these changes Aug 2, 2022

View reviewed changes

joelanford force-pushed the binary-source branch from 01f7e6f to 8f0ada7 Compare August 3, 2022 20:14

joelanford added 7 commits August 3, 2022 16:48

introduce 'binary' source and 'rukpakctl run' command

b83a377

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>

don't fail on re-upload of identical data; add godoc, CLI docs

604f71e

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>

rename 'binary' source to 'upload'

8fc144e

rukpakctl: add to container image

8943422

uploadmgr: wait for cache sync

4755f29

explain use of unstructured in rukpakctl run BD patch

c7193f3

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>

upload source: comment complex code paths, other minor improvements

060d165

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>

joelanford force-pushed the binary-source branch from 8f0ada7 to 060d165 Compare August 3, 2022 20:50

joelanford merged commit 3b832db into operator-framework:main Aug 3, 2022

joelanford deleted the binary-source branch August 3, 2022 21:05

exdx mentioned this pull request Aug 8, 2022

e2e add tests for rukpakctl subcommands #459

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `upload` source and `rukpakctl run` #417

Introduce `upload` source and `rukpakctl run` #417

joelanford commented Jun 3, 2022 •

edited

Loading

joelanford commented Jun 3, 2022 •

edited

Loading

akihikokuroda commented Jun 6, 2022

joelanford commented Jun 17, 2022

akihikokuroda commented Jun 17, 2022

openshift-ci bot commented Jul 14, 2022

joelanford commented Jul 21, 2022 •

edited

Loading

timflannagan commented Jul 21, 2022

exdx left a comment

exdx Aug 2, 2022 •

edited

Loading

joelanford Aug 2, 2022

exdx Aug 2, 2022

timflannagan Aug 2, 2022

joelanford Aug 3, 2022

timflannagan left a comment

timflannagan Aug 2, 2022

timflannagan Aug 2, 2022

timflannagan Aug 2, 2022

timflannagan Aug 2, 2022

timflannagan Aug 2, 2022

timflannagan Aug 2, 2022

joelanford Aug 3, 2022

timflannagan Aug 2, 2022

timflannagan Aug 2, 2022

exdx commented Aug 2, 2022

joelanford commented Aug 2, 2022

joelanford commented Aug 3, 2022

timflannagan commented Aug 3, 2022

		// This function unsets user and group information in the tar archive so that readers
		// of archives produced by this function do not need to account for differences in

Introduce upload source and rukpakctl run #417

Introduce upload source and rukpakctl run #417

Conversation

joelanford commented Jun 3, 2022 • edited Loading

joelanford commented Jun 3, 2022 • edited Loading

akihikokuroda commented Jun 6, 2022

joelanford commented Jun 17, 2022

akihikokuroda commented Jun 17, 2022

openshift-ci bot commented Jul 14, 2022

joelanford commented Jul 21, 2022 • edited Loading

timflannagan commented Jul 21, 2022

exdx left a comment

Choose a reason for hiding this comment

exdx Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timflannagan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exdx commented Aug 2, 2022

joelanford commented Aug 2, 2022

joelanford commented Aug 3, 2022

timflannagan commented Aug 3, 2022

Introduce `upload` source and `rukpakctl run` #417

Introduce `upload` source and `rukpakctl run` #417

joelanford commented Jun 3, 2022 •

edited

Loading

joelanford commented Jun 3, 2022 •

edited

Loading

joelanford commented Jul 21, 2022 •

edited

Loading

exdx Aug 2, 2022 •

edited

Loading