[WIP] Package Extension API Server #433

njhale · 2018-08-28T14:13:04Z

Description

Implements a package manifest extension-apiserver that uses the Kubernetes Aggregation Layer to provide parsed package manifests as top-level Kubernetes resources. Manifests are parsed from config maps pointed at by sourceType: internal catalog sources and are stored in memory instead of etcd.

Result of ALM-648

ecordell

awesome work! it's pretty close to being something we could deploy right away.

I'll also leave this here for us to consider:
https://www.thesaurus.com/browse/package

ecordell · 2018-08-30T13:08:51Z

Gopkg.toml

@@ -55,3 +55,14 @@
 [[constraint]]
  name = "github.com/writeas/go-strip-markdown"
  version = "2.0.0"
+
+# resolved to 1.1.3 which doesn't support flags used by apimachinary release-1.11


these were set a while ago, did you try removing them and resolving to see if it built?

ecordell · 2018-08-30T13:09:41Z

deploy/chart/templates/20-packageserver.yaml

+  name: v1alpha1.apps.coreos.com
+spec:
+  insecureSkipTLSVerify: true
+  group: apps.coreos.com


for actually deploying we probably want to think about this group name

I was thinking something like packages.apps.redhat.com

ecordell · 2018-08-30T13:11:26Z

deploy/chart/templates/20-packageserver.yaml

@@ -0,0 +1,146 @@
+apiVersion: apiregistration.k8s.io/v1beta1


In order for our packaging scripts to work we need each of these yamls to have its own file (threw them all in one for testing)

Alternately you could fix our packaging scripts to not require that each be in a separate file, whichever

ecordell · 2018-08-30T13:12:54Z

pkg/package-server/apis/packagemanifest/install/install.go

+// Install registers the API group and adds types to a scheme
+func Install(scheme *runtime.Scheme) {
+	v1alpha1.AddToScheme(scheme)
+	//utilruntime.Must(v1alpha1.AddToScheme(scheme))


do we need these?

ecordell · 2018-08-30T13:13:32Z

pkg/package-server/apis/packagemanifest/v1alpha1/types.go

+	metav1.ObjectMeta `json:"metadata"`
+
+	Spec PackageManifestSpec `json:"spec"`
+	//TODO: status


As a super simple status, we could write what catalog a package came from?

ecordell · 2018-08-30T13:24:48Z

pkg/package-server/provider/inmem.go

+					Name:      spec.PackageName,
+					Namespace: cm.GetNamespace(),
+				},
+				Spec: spec,


we should think about what should actually be here with @alecmerdler

the current "package spec" from the catalog configmap isn't enough to display packages in the ui. the ui is using the package spec to find the CSV in the configmap and then using that for display.

it could probably be as simple as:

spec: package: package spec csv: - csv spec for csv in default channel

which would match what we currently do. or it could have it's own set of metadata instead.

ecordell · 2018-08-30T13:25:27Z

pkg/package-server/provider/inmem.go

+			// TODO: add check for invalid package definitions
+			manifests = append(manifests, manifest)
+		}
+		log.Debugf("Load ConfigMap      -- Found packages: %v", manifests)


since we're copying this over anyway, could we make this structured with log.WithField etc like we do for other parts of olm?

Yup, sounds good.

ecordell · 2018-08-30T13:25:58Z

pkg/package-server/provider/inmem.go

+
+	// check if the sourcetype is internal
+	switch catsrc.Spec.SourceType {
+	case "internal":


👍 for future proofing!

ecordell · 2018-08-30T13:27:07Z

pkg/package-server/storage/packagemanifest/reststorage.go

+
+	return &manifest, nil
+}
+


So what happens if I try and create a Package in the API? do I get a reasonable error back?

Error from server (MethodNotAllowed): error when creating "pkg-manifest.json": the server does not allow this method on the requested resource which I think is reasonable; though it could be more descriptive.

ecordell · 2018-08-30T13:27:30Z

scripts/build_local.sh

@@ -5,11 +5,12 @@

 set -e

-ps x | grep -q [m]inikube || minikube start --kubernetes-version="v1.11.1" --extra-config=apiserver.v=4 || { echo 'Cannot start minikube.'; exit 1; }
+ps x | grep -q [m]inikube || minikube start --extra-config=apiserver.v=4 || { echo 'Cannot start minikube.'; exit 1; }


is minikube 1.11 by default now?

jpeeler · 2018-09-07T21:05:41Z

deploy/chart/templates/22-packageserver.yaml

 apiVersion: apiregistration.k8s.io/v1beta1
 kind: APIService
 metadata:
  name: v1alpha1.packages.apps.redhat.com
 spec:
-  insecureSkipTLSVerify: true
+  caBundle: {{ $ca.Cert | b64enc | quote }}


You should be able to write this as caBundle: {{ b64enc $ca.Cert }}.

ecordell · 2018-09-10T12:56:45Z

ci.kubeconfig

@@ -0,0 +1,29 @@
+apiVersion: v1


🚫 don't commit secrets...

ecordell · 2018-09-10T13:01:06Z

pkg/package-server/apis/packagemanifest/v1alpha1/types.go

+type PackageManifestSpec struct{}
+
+// PackageManifestStatus represents the current status of the PackageManifest
+type PackageManifestStatus struct {


We should pull info from the CSV into this. And actually, I think some of it should go into labels if it's not too weird (so that when you do a label query, the object you get back has labels to match)

ecordell · 2018-09-10T13:03:12Z

pkg/package-server/client/clientset/versioned/typed/packagemanifest/v1alpha1/packagemanifest.go

+}
+
+// Get takes name of the packageManifest, and returns the corresponding packageManifest object, and an error if there is any.
+func (c *packageManifests) Get(name string, options v1.GetOptions) (result *v1alpha1.PackageManifest, err error) {


Is there a way to tell the Client codegen to only generate get/list/watch?

Yes - I'm testing that out now.

ecordell · 2018-09-10T13:04:37Z

pkg/package-server/server/server.go

+	"k8s.io/client-go/tools/cache"
+	"k8s.io/client-go/tools/clientcmd"
+
+	//"k8s.io/sample-apiserver/pkg/admission/plugin/banflunder"


these can go

ecordell · 2018-09-10T13:05:24Z

pkg/package-server/server/server.go

+	go sourceProvider.Run(stopCh)
+
+	// add health checks
+	// server.AddHealthzChecks(healthz.NamedCheck("healthz", mgr.CheckHealth))


health checks would be nice if this helper does what it says

updates dependencies to kube 1.11 don't require etcd to start apiserver fix(apiserver): make apiserver start feat(package-api): make minimally functional in-mem store fix(package-api): use correct group refactor(package-server): change rest.Storage implementation fix(package-server): fix package-server feat(package-server): add better filtering fix(types): add struct tags for metadata chore(certs): remove local client certs chore(bin): remove package-server binary chore(scripts): remove second run_console script feat(types): add package mainfest status resource refactor(types): register different api group Registers package manifests under the `packages.apps.redhat.com` api group. feat(make): add openapi gen rules feat(install_local): add wait for package-server rollout refactor(charts): change package apiserver deployment name Changes package-manifest-apiserver to package-server in deployment and dependent resources

- Enables openapi for the package-server extension-apiserver - Removes leftover sample-apiserver fuzzer comments fix(make): update openapi gen rule to include dependencies fix(openapi): add openapi comment

chore(codgen): update codegen chore(package-server): regen clients fix(build_local): specify kube version chore(vendor): re-vendor chore(package-server): clean up leftover comments

Fixes unmarshalling of catalog source package manifest yaml to PackageManifestStatus

- Changes package-server serviceaccount name - Changes package-server service name - Adds debug flag to package-server container commands

- Adds basic e2e test - Adds package-server e2e helm values

Declares cobra command in main package init to make visible to test binary

- Adds debug flag - Change default watch interval to 5 minutes

- Removes commented packages - Removes commented health checks

ecordell · 2018-09-11T11:32:13Z

deploy/chart/templates/22-packageserver.yaml

+  name: v1alpha1.packages.apps.redhat.com
+spec:
+  caBundle: {{ $ca.Cert | b64enc | quote }}
+  group: packages.apps.redhat.com


This seems weird since the other group is operators.coreos.com

I was wondering whether we want to continue using *.coreos.com. Also, I added the package prefix because the package-server is only going to be responsible for package related resources. If I had just used apps.redhat.com or apps.coreos.com, the group would be unusable by any other CRDs or extension-apiservers.

ecordell

LGTM

I think this is good to merge, but there are some things I think we'll want to circle back around on soon:

If we're keeping the configmap catalog source parsing, maybe it makes more sense to just run the packages server in the same binary as the catalog operator, so that we don't have to deploy two separate control loops watching catalogsources and parsing confmaps into memory
Not sure we want the entire contents of every channel's CSV in the package manifest.
We know we'll need to change a chunk of these control loops when we have the catalog api up

[WIP] Package Extension API Server

njhale force-pushed the pkg-api branch from adb8b09 to bcd59b0 Compare August 29, 2018 17:16

ecordell requested changes Aug 30, 2018

View reviewed changes

njhale force-pushed the pkg-api branch 2 times, most recently from 993c8d7 to 1d588a4 Compare September 6, 2018 22:44

jpeeler reviewed Sep 7, 2018

View reviewed changes

ecordell requested changes Sep 10, 2018

View reviewed changes

njhale force-pushed the pkg-api branch from 263e12e to 69ba446 Compare September 10, 2018 14:12

ecordell and others added 23 commits September 10, 2018 10:17

feat(package-server): enable openapi

8f25e2a

- Enables openapi for the package-server extension-apiserver - Removes leftover sample-apiserver fuzzer comments fix(make): update openapi gen rule to include dependencies fix(openapi): add openapi comment

chore(package-server): remove and ignore local config

d38ada2

refactor(packagemanifest): move all package data to status

6701217

chore(codgen): update codegen chore(package-server): regen clients fix(build_local): specify kube version chore(vendor): re-vendor chore(package-server): clean up leftover comments

fix(inmem): change struct tags

93a1a1b

Fixes unmarshalling of catalog source package manifest yaml to PackageManifestStatus

feat(deploy): update package-server templates

a04c043

- Changes package-server serviceaccount name - Changes package-server service name - Adds debug flag to package-server container commands

test(package-server): add basic e2e test

390b808

- Adds basic e2e test - Adds package-server e2e helm values

feat(package-server): add run-local package-server helm values

e336291

fix(package-server): surface cobra flags to package level

1c3d16e

Declares cobra command in main package init to make visible to test binary

feat(package-server): add debug flag

a255c61

- Adds debug flag - Change default watch interval to 5 minutes

fix(provider): don't use pointer in manifest selection loop

4b30735

feat(reststorage): return NotFoundError if manifest not found

51fa03a

feat(docker): add package-server to Dockerfiles

cffc32c

feat(scripts): add package-server to build scripts

5432d76

feat(ci): add package-server to ci

83caf8e

fix(scripts): uncomment cleanup

69f2f56

fix(charts): add missing coreos pull secret

85c193a

fix(charts): correct indentation

c69c577

refactor(charts): use olm-operator-serviceaccount for package-server

1ed835f

fix(charts): update expected package-server chart value name

b9c213d

fix(charts): change default package-server internal port value to 443

cc2b867

fix(charts): change cert secret mount path

54e2491

test(e2e): add logging to TestPackageManifestLoading

16d2467

test(packagemanifest): add fetch function for packagemanifests

7f25bfa

ecordell force-pushed the pkg-api branch from 69ba446 to 7f25bfa Compare September 10, 2018 14:20

njhale added 7 commits September 10, 2018 19:46

fix(build): add OLM_VERSION file and .git directory to e2e.Dockerfile

93565b3

feat(packagemanifest): add CSVDescription to PackageManifest

120422f

chore(openapi): regenerate openapi spec

ba6a70d

test(e2e): use CSVDescription in expected PackageManifestStatus

56ed3f4

chore(server): remove commented packages

1fe9a86

- Removes commented packages - Removes commented health checks

feat(inmem): collect CSV data in PackageManifestStatus

a555240

fix(reststorage): start with empty slice when filtering

5e861c6

ecordell reviewed Sep 11, 2018

View reviewed changes

ecordell approved these changes Sep 11, 2018

View reviewed changes

njhale merged commit 724b209 into operator-framework:master Sep 11, 2018

ecordell pushed a commit to ecordell/operator-lifecycle-manager that referenced this pull request Mar 8, 2019

Merge pull request operator-framework#433 from njhale/pkg-api

8372a6d

[WIP] Package Extension API Server

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Package Extension API Server #433

[WIP] Package Extension API Server #433

njhale commented Aug 28, 2018

ecordell left a comment

ecordell Aug 30, 2018

ecordell Aug 30, 2018

njhale Aug 30, 2018

ecordell Aug 30, 2018

ecordell Aug 30, 2018

ecordell Aug 30, 2018

ecordell Aug 30, 2018

ecordell Aug 30, 2018

njhale Aug 30, 2018

ecordell Aug 30, 2018

ecordell Aug 30, 2018

njhale Aug 31, 2018

ecordell Aug 30, 2018

jpeeler Sep 7, 2018

ecordell Sep 10, 2018

ecordell Sep 10, 2018

ecordell Sep 10, 2018

njhale Sep 11, 2018

ecordell Sep 10, 2018

ecordell Sep 10, 2018

ecordell Sep 11, 2018

njhale Sep 11, 2018

ecordell left a comment

[WIP] Package Extension API Server #433

[WIP] Package Extension API Server #433

Conversation

njhale commented Aug 28, 2018

Description

ecordell 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecordell left a comment

Choose a reason for hiding this comment