generate/internal/manifests.go: add all supported kinds to manifests #4137

estroz · 2020-10-29T15:56:00Z

I almost want to say this is a net-new feature because we explicitly do this for RBAC and no other types, but I am ok with "change" since the explicitness isn't user-facing.

estroz · 2020-10-29T15:54:14Z

This isn't quite true. OLM manages webhooks and their services, no explicitly defined webhook Service objects should be passed. You can filter out Services by finding those associated with {Mutating,Validating}WebhookConfigurations included in a bundled CSV (all webhook configs in the collector.Manifests).

Do we just match on namespace and name? What if a service declares multiple ports and one of them is for the webhook?

Do you think we need to filter out webhook service ports and then filter out any services that have no ports?

Can we constrain the problem by filtering any service that matches a webhook config by name/namespace, and document that webhooks need their own separate service? Modifying a resource's functional content doesn't feel right.

We could. I think I have a working solution that does what I suggest above. Let me know what you think.

-Original file line number
+Diff line change
@@ -0,0 +1,3 @@
+    entries:
+      - description: When generating bundles and packagemanifests, add all resources supported by OLM.
+        kind: change

-Original file line number
+Diff line change
@@ Expand Up / @@ -15,6 +15,7 @@ @@
     package genutil
     import (
+    	"github.com/operator-framework/operator-registry/pkg/lib/bundle"
     	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
     	"github.com/operator-framework/operator-sdk/internal/generate/collector"
@@ Expand All @@
     		objs = append(objs, &c.ServiceAccounts[i])
     	}
+    	// All Services passed in should be written.
+    	for i := range c.Services {
+    		objs = append(objs, &c.Services[i])
+    	}
+    	// Add all other supported kinds
+    	for i := range c.Others {
+    		obj := &c.Others[i]
+    		if supported, _ := bundle.IsSupported(obj.GroupVersionKind().Kind); supported {
+    			objs = append(objs, obj)
+    		}
+    	}
     	// RBAC objects that are not a part of the CSV should be written.
     	_, roleObjs := c.SplitCSVPermissionsObjects()
     	objs = append(objs, roleObjs...)
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -22,6 +22,7 @@ import ( @@
     	v1 "github.com/operator-framework/api/pkg/operators/v1"
     	"github.com/operator-framework/api/pkg/operators/v1alpha1"
+    	"github.com/operator-framework/operator-registry/pkg/lib/bundle"
     	apierrors "k8s.io/apimachinery/pkg/api/errors"
     	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
     	"k8s.io/apimachinery/pkg/runtime/schema"
@@ Expand Down Expand Up @@
     			Kind:    step.Resource.Kind,
     		})
-    		// TODO(joelanford): This seems necessary for service accounts tied to
-    		//    cluster roles and cluster role bindings because the SA namespace
-    		//    is not set in the manifest in this case.
+    		// TODO(joelanford): This seems necessary for namespaced resources
     		//    See: https://github.com/operator-framework/operator-lifecycle-manager/blob/c9405d035bc50d9aa290220cb8d75b0402e72707/pkg/controller/registry/resolver/rbac.go#L133
-    		if step.Resource.Kind == "ServiceAccount" && obj.GetNamespace() == "" {
+    		if supported, namespaced := bundle.IsSupported(step.Resource.Kind); supported && bool(namespaced) {
     			obj.SetNamespace(installPlanKey.Namespace)
     		}
     		switch step.Resource.Kind {
     		case "CustomResourceDefinition":
     			crds = append(crds, obj)
@@ Expand Down @@

-Original file line number
+Diff line change
@@ -0,0 +1,13 @@
+    apiVersion: monitoring.coreos.com/v1
+    kind: ServiceMonitor
+    metadata:
+      labels:
+        control-plane: controller-manager
+      name: memcached-operator-controller-manager-metrics-monitor
+    spec:
+      endpoints:
+      - path: /metrics
+        port: https
+      selector:
+        matchLabels:
+          control-plane: controller-manager

-Original file line number
+Diff line change
@@ -0,0 +1,16 @@
+    apiVersion: v1
+    kind: Service
+    metadata:
+      creationTimestamp: null
+      labels:
+        control-plane: controller-manager
+      name: memcached-operator-controller-manager-metrics-service
+    spec:
+      ports:
+      - name: https
+        port: 8443
+        targetPort: https
+      selector:
+        control-plane: controller-manager
+    status:
+      loadBalancer: {}

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

generate/internal/manifests.go: add all supported kinds to manifests #4137

Uh oh!

Diff view

Diff view

There are no files selected for viewing

estroz Oct 29, 2020

Uh oh!

estroz Oct 29, 2020

Uh oh!

joelanford Oct 29, 2020

Uh oh!

estroz Oct 29, 2020

Uh oh!

joelanford Oct 29, 2020

Uh oh!

-Original file line number
+Diff line change
@@ Expand Up @@
     ### Bundle format
-    A [bundle][bundle] consists of manifests (CSV and CRDs) and metadata that define an Operator
-    at a particular version. You may have also heard of a bundle image. From the bundle docs:
+    A [bundle][bundle] consists of manifests (CSV, CRDs, and other supported kinds) and metadata that define an Operator
+    at a particular version, and an optional [scorecard][scorecard] configuration file. You may have also heard of a
+    bundle image. From the bundle docs:
     > An Operator Bundle is built as a scratch (non-runnable) container image that
     > contains operator manifests and specific metadata in designated directories
@@ Expand All @@
     SDK projects are scaffolded with a `Makefile` containing the `bundle` recipe by default,
     which wraps `generate kustomize manifests`, `generate bundle`, and other related commands.
-    By default `make bundle` will generate a CSV, copy CRDs, and generate metadata in the bundle format:
+    By default `make bundle` will generate a CSV, copy CRDs and other supported kinds, generate metadata,
+    and add your scorecard configuration in the bundle format:
     ```console
     $ make bundle
     $ tree ./bundle
     ./bundle
     ├── manifests
-    │   ├── cache.my.domain_memcacheds.yaml
-    │   └── memcached-operator.clusterserviceversion.yaml
-    └── metadata
-        └── annotations.yaml
+    │   ├── cache.example.com_memcacheds.yaml
+    │   ├── memcached-operator.clusterserviceversion.yaml
+    │   ├── memcached-operator-controller-manager-metrics-monitor_monitoring.coreos.com_v1_servicemonitor.yaml
+    │   ├── memcached-operator-controller-manager-metrics-service_v1_service.yaml
+    │   ├── memcached-operator-metrics-reader_rbac.authorization.k8s.io_v1beta1_clusterrole.yaml
+    │   └── memcached-operator-webhook-service_v1_service.yaml
+    ├── metadata
+    │   └── annotations.yaml
+    └── tests
+        └── scorecard
+            └── config.yaml
     ```
     Bundle metadata in `bundle/metadata/annotations.yaml` contains information about a particular Operator version
@@ Expand Down Expand Up / @@ -146,9 +155,9 @@ bundle: ... @@
     ### Package manifests format
-    A [package manifests][package-manifests] format consists of on-disk manifests (CSV and CRDs) and metadata that
-    define an Operator at all versions of that Operator. Each version is contained in its own directory, with a parent
-    package manifest YAML file containing channel-to-version mappings, much like a bundle's metadata.
+    A [package manifests][package-manifests] format consists of on-disk manifests (CSV, CRDs and other supported kinds)
+    and metadata that define an Operator at all versions of that Operator. Each version is contained in its own directory,
+    with a parent package manifest YAML file containing channel-to-version mappings, much like a bundle's metadata.
     If your Operator is already formatted as a package manifests and you do not wish to migrate to the bundle format yet,
     you should add the following to your `Makefile` to make development easier:
@@ Expand Down Expand Up / @@ -310,3 +319,4 @@ being managed, each with a `name` and `url`. @@
     [olm-capabilities]:/docs/advanced-topics/operator-capabilities/operator-capabilities
     [csv-markers]:/docs/building-operators/golang/references/markers
     [operatorhub]:https://operatorhub.io/
+    [scorecard]:/docs/advanced-topics/scorecard

generate/internal/manifests.go: add all supported kinds to manifests #4137

Uh oh!

generate/internal/manifests.go: add all supported kinds to manifests #4137

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

estroz Oct 29, 2020

Choose a reason for hiding this comment

Uh oh!

estroz Oct 29, 2020

Choose a reason for hiding this comment

Uh oh!

joelanford Oct 29, 2020

Choose a reason for hiding this comment

Uh oh!

estroz Oct 29, 2020

Choose a reason for hiding this comment

Uh oh!

joelanford Oct 29, 2020

Choose a reason for hiding this comment

Uh oh!