pkg/metrics: Allow multi port metrics Service creation (#1560)

* pkg/metrics: Allow multi port metrics Service creation

This allows users to pass in any metrics port and we expose that port in
one Service. With that we also expose the custom resource metrics port.

* CHANGELOG.md: Mention change to func

* doc/user/metrics: Update README to reflect changes

Author: lilic

CHANGELOG.md

@@ -17,12 +17,14 @@
 - Remove TypeMeta declaration from the implementation of the objects [#1462](https://github.com/operator-framework/operator-sdk/pull/1462/)
 - Relaxed API version format check when parsing `pkg/apis` in code generators. API dir structures can now be of the format `pkg/apis/<group>/<anything>`, where `<anything>` was previously required to be in the Kubernetes version format, ex. `v1alpha1`. ([#1525](https://github.com/operator-framework/operator-sdk/pull/1525))
 - The SDK and operator projects will work outside of `$GOPATH/src` when using [Go modules](https://github.com/golang/go/wiki/Modules). ([#1475](https://github.com/operator-framework/operator-sdk/pull/1475))
+-  `CreateMetricsService()` function from the metrics package accepts an array of ServicePort objects ([]v1.ServicePort) as input to create Service metrics. `CRPortName` constant is added to describe the string of custom resource port name. ([#1560](https://github.com/operator-framework/operator-sdk/pull/1560))
 
 ### Deprecated
 
 ### Removed
 
 - The SDK no longer depends on a `vendor/` directory to manage dependencies *only if* using [Go modules](https://github.com/golang/go/wiki/Modules). The SDK and operator projects will only use vendoring if using `dep`, or modules and a `vendor/` dir is present. ([#1519](https://github.com/operator-framework/operator-sdk/pull/1519))
+- **Breaking change:** `ExposeMetricsPort` is removed and replaced with `CreateMetricsService()` function. `PrometheusPortName` constant is replaced with `OperatorPortName`. ([#1560](https://github.com/operator-framework/operator-sdk/pull/1560))
 
 ### Bug Fixes
 

doc/user/metrics/README.md

@@ -6,16 +6,20 @@
 
 ### General metrics
 
-The `ExposeMetricsPort(ctx context.Context, port int32) (*v1.Service, error)` function exposes general metrics about the running program. These metrics are inherited from controller-runtime. To understand which metrics are exposed, read the metrics package doc of [controller-runtime][controller-metrics]. The `ExposeMetricsPort` function creates a [Service][service] object with the metrics port exposed, which can then be accessed by Prometheus. The Service object is [garbage collected][gc] when the leader pod's root owner is deleted.
+The `"CreateMetricsService(ctx context.Context, servicePorts []v1.ServicePort) (*v1.Service, error)` function exposes general metrics about the running program. These metrics are inherited from controller-runtime. To understand which metrics are exposed, read the metrics package doc of [controller-runtime][controller-metrics]. The `ExposeMetricsPort` function creates a [Service][service] object with the metrics port exposed, which can then be accessed by Prometheus. The Service object is [garbage collected][gc] when the leader pod's root owner is deleted.
 
 By default, the metrics are served on `0.0.0.0:8383/metrics`. To modify the port the metrics are exposed on, change the `var metricsPort int32 = 8383` variable in the `cmd/manager/main.go` file of the generated operator.
 
 #### Usage:
 
 ```go
     import(
+        "context"
+
         "github.com/operator-framework/operator-sdk/pkg/metrics"
         "sigs.k8s.io/controller-runtime/pkg/manager"
+        "k8s.io/api/core/v1"
+        "k8s.io/apimachinery/pkg/util/intstr"
     )
 
     func main() {
@@ -34,14 +38,19 @@ By default, the metrics are served on `0.0.0.0:8383/metrics`. To modify the port
 
         ...
 
+        // Add to the below struct any other metrics ports you want to expose.
+	    servicePorts := []v1.ServicePort{
+		    {Port: metricsPort, Name: metrics.OperatorPortName, Protocol: v1.ProtocolTCP, TargetPort: intstr.IntOrString{Type: intstr.Int, IntVal: metricsPort}},
+	    }
+
         // Create Service object to expose the metrics port.
-        _, err = metrics.ExposeMetricsPort(ctx, metricsPort)
+        _, err = metrics.CreateMetricsService(context.TODO(), servicePorts)
         if err != nil {
             // handle error
-            log.Info(err.Error())
         }
 
         ...
+
     }
 ```
 

hack/tests/e2e-helm.sh

@@ -34,15 +34,17 @@ test_operator() {
     fi
 
     # verify that metrics service was created
-    if ! timeout 20s bash -c -- "until kubectl get service/nginx-operator > /dev/null 2>&1; do sleep 1; done";
+    if ! timeout 20s bash -c -- "until kubectl get service/nginx-operator-metrics > /dev/null 2>&1; do sleep 1; done";
     then
+        echo "Failed to get metrics service"
         kubectl logs deployment/nginx-operator
         exit 1
     fi
 
     # verify that the metrics endpoint exists
-    if ! timeout 1m bash -c -- "until kubectl run -it --rm --restart=Never test-metrics --image=registry.access.redhat.com/ubi7/ubi-minimal:latest -- curl -sfo /dev/null http://nginx-operator:8383/metrics; do sleep 1; done";
+    if ! timeout 1m bash -c -- "until kubectl run -it --rm --restart=Never test-metrics --image=registry.access.redhat.com/ubi7/ubi-minimal:latest -- curl -sfo /dev/null http://nginx-operator-metrics:8383/metrics; do sleep 1; done";
     then
+        echo "Failed to verify that metrics endpoint exists"
         kubectl logs deployment/nginx-operator
         exit 1
     fi

internal/pkg/scaffold/cmd.go

@@ -58,6 +58,8 @@ import (
 	"github.com/operator-framework/operator-sdk/pkg/restmapper"
 	sdkVersion "github.com/operator-framework/operator-sdk/version"
 	"github.com/spf13/pflag"
+	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/util/intstr"
 	"sigs.k8s.io/controller-runtime/pkg/client/config"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
@@ -150,9 +152,14 @@ func main() {
 	if err = serveCRMetrics(cfg); err != nil {
 		log.Info("Could not generate and serve custom resource metrics: ", err.Error())
 	}
-
-	// Create Service object to expose the metrics port.
-	_, err = metrics.ExposeMetricsPort(ctx, metricsPort)
+	
+	// Add to the below struct any other metrics ports you want to expose.
+	servicePorts := []v1.ServicePort{
+		{Port: metricsPort, Name: metrics.OperatorPortName, Protocol: v1.ProtocolTCP, TargetPort: intstr.IntOrString{Type: intstr.Int, IntVal: metricsPort}},
+		{Port: operatorMetricsPort, Name: metrics.CRPortName, Protocol: v1.ProtocolTCP, TargetPort: intstr.IntOrString{Type: intstr.Int, IntVal: operatorMetricsPort}},
+	}
+	// Create Service object to expose the metrics port(s).
+	_, err = metrics.CreateMetricsService(ctx, servicePorts)
 	if err != nil {
 		log.Info(err.Error())
 	}

internal/pkg/scaffold/cmd_test.go

@@ -57,6 +57,8 @@ import (
 	"github.com/operator-framework/operator-sdk/pkg/restmapper"
 	sdkVersion "github.com/operator-framework/operator-sdk/version"
 	"github.com/spf13/pflag"
+	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/util/intstr"
 	"sigs.k8s.io/controller-runtime/pkg/client/config"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
@@ -150,8 +152,13 @@ func main() {
 		log.Info("Could not generate and serve custom resource metrics: ", err.Error())
 	}
 
-	// Create Service object to expose the metrics port.
-	_, err = metrics.ExposeMetricsPort(ctx, metricsPort)
+	// Add to the below struct any other metrics ports you want to expose.
+	servicePorts := []v1.ServicePort{
+		{Port: metricsPort, Name: metrics.OperatorPortName, Protocol: v1.ProtocolTCP, TargetPort: intstr.IntOrString{Type: intstr.Int, IntVal: metricsPort}},
+		{Port: operatorMetricsPort, Name: metrics.CRPortName, Protocol: v1.ProtocolTCP, TargetPort: intstr.IntOrString{Type: intstr.Int, IntVal: operatorMetricsPort}},
+	}
+	// Create Service object to expose the metrics port(s).
+	_, err = metrics.CreateMetricsService(ctx, servicePorts)
 	if err != nil {
 		log.Info(err.Error())
 	}

pkg/ansible/run.go

@@ -28,14 +28,19 @@ import (
 	"github.com/operator-framework/operator-sdk/pkg/leader"
 	"github.com/operator-framework/operator-sdk/pkg/metrics"
 	sdkVersion "github.com/operator-framework/operator-sdk/version"
+	"k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/intstr"
 
 	"sigs.k8s.io/controller-runtime/pkg/client/config"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
 )
 
-var log = logf.Log.WithName("cmd")
+var (
+	log               = logf.Log.WithName("cmd")
+	metricsPort int32 = 8383
+)
 
 func printVersion() {
 	log.Info(fmt.Sprintf("Go Version: %s", runtime.Version()))
@@ -66,7 +71,7 @@ func Run(flags *aoflags.AnsibleOperatorFlags) error {
 	// TODO: probably should expose the host & port as an environment variables
 	mgr, err := manager.New(cfg, manager.Options{
 		Namespace:          namespace,
-		MetricsBindAddress: "0.0.0.0:8383",
+		MetricsBindAddress: fmt.Sprintf("0.0.0.0:%d", metricsPort),
 	})
 	if err != nil {
 		log.Error(err, "Failed to create a new manager.")
@@ -85,8 +90,13 @@ func Run(flags *aoflags.AnsibleOperatorFlags) error {
 		return err
 	}
 
+	// Add to the below struct any other metrics ports you want to expose.
+	servicePorts := []v1.ServicePort{
+		{Port: metricsPort, Name: metrics.OperatorPortName, Protocol: v1.ProtocolTCP, TargetPort: intstr.IntOrString{Type: intstr.Int, IntVal: metricsPort}},
+	}
+	// Create Service object to expose the metrics port(s).
 	// TODO: probably should expose the port as an environment variable
-	_, err = metrics.ExposeMetricsPort(context.TODO(), 8383)
+	_, err = metrics.CreateMetricsService(context.TODO(), servicePorts)
 	if err != nil {
 		log.Error(err, "Exposing metrics port failed.")
 		return err

pkg/helm/run.go

@@ -29,7 +29,9 @@ import (
 	"github.com/operator-framework/operator-sdk/pkg/metrics"
 	sdkVersion "github.com/operator-framework/operator-sdk/version"
 
+	"k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/intstr"
 	"sigs.k8s.io/controller-runtime/pkg/client/config"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
@@ -117,8 +119,11 @@ func Run(flags *hoflags.HelmOperatorFlags) error {
 		return err
 	}
 
-	// Create Service object to expose the metrics port.
-	_, err = metrics.ExposeMetricsPort(ctx, metricsPort)
+	servicePorts := []v1.ServicePort{
+		{Port: metricsPort, Name: metrics.OperatorPortName, Protocol: v1.ProtocolTCP, TargetPort: intstr.IntOrString{Type: intstr.Int, IntVal: metricsPort}},
+	}
+	// Create Service object to expose the metrics port(s).
+	_, err = metrics.CreateMetricsService(ctx, servicePorts)
 	if err != nil {
 		log.Info(err.Error())
 	}

pkg/metrics/metrics.go

@@ -25,7 +25,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/apimachinery/pkg/util/intstr"
 	crclient "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/config"
 	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
@@ -36,19 +35,23 @@ var log = logf.Log.WithName("metrics")
 var trueVar = true
 
 const (
-	// PrometheusPortName defines the port name used in the metrics Service.
-	PrometheusPortName = "metrics"
+	// OperatorPortName defines the default operator metrics port name used in the metrics Service.
+	OperatorPortName = "http-metrics"
+	// CRPortName defines the custom resource specific metrics' port name used in the metrics Service.
+	CRPortName = "cr-metrics"
 )
 
-// ExposeMetricsPort creates a Kubernetes Service to expose the passed metrics port.
-func ExposeMetricsPort(ctx context.Context, port int32) (*v1.Service, error) {
+// CreateMetricsService creates a Kubernetes Service to expose the passed metrics
+// port(s) with the given name(s).
+func CreateMetricsService(ctx context.Context, servicePorts []v1.ServicePort) (*v1.Service, error) {
+	if len(servicePorts) < 1 {
+		return nil, fmt.Errorf("failed to create metrics Serice; service ports were empty")
+	}
 	client, err := createClient()
 	if err != nil {
 		return nil, fmt.Errorf("failed to create new client: %v", err)
 	}
-	// We do not need to check the validity of the port, as controller-runtime
-	// would error out and we would never get to this stage.
-	s, err := initOperatorService(ctx, client, port, PrometheusPortName)
+	s, err := initOperatorService(ctx, client, servicePorts)
 	if err != nil {
 		if err == k8sutil.ErrNoNamespace {
 			log.Info("Skipping metrics Service creation; not running in a cluster.")
@@ -93,8 +96,8 @@ func createOrUpdateService(ctx context.Context, client crclient.Client, s *v1.Se
 	return s, nil
 }
 
-// initOperatorService returns the static service which exposes specified port.
-func initOperatorService(ctx context.Context, client crclient.Client, port int32, portName string) (*v1.Service, error) {
+// initOperatorService returns the static service which exposes specified port(s).
+func initOperatorService(ctx context.Context, client crclient.Client, sp []v1.ServicePort) (*v1.Service, error) {
 	operatorName, err := k8sutil.GetOperatorName()
 	if err != nil {
 		return nil, err
@@ -103,27 +106,16 @@ func initOperatorService(ctx context.Context, client crclient.Client, port int32
 	if err != nil {
 		return nil, err
 	}
-
 	label := map[string]string{"name": operatorName}
 
 	service := &v1.Service{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      operatorName,
+			Name:      fmt.Sprintf("%s-metrics", operatorName),
 			Namespace: namespace,
 			Labels:    label,
 		},
 		Spec: v1.ServiceSpec{
-			Ports: []v1.ServicePort{
-				{
-					Port:     port,
-					Protocol: v1.ProtocolTCP,
-					TargetPort: intstr.IntOrString{
-						Type:   intstr.Int,
-						IntVal: port,
-					},
-					Name: portName,
-				},
-			},
+			Ports:    sp,
 			Selector: label,
 		},
 	}

test/e2e/memcached_test.go

@@ -644,7 +644,7 @@ func memcachedMetricsTest(t *testing.T, f *framework.Framework, ctx *framework.T
 
 	// Make sure metrics Service exists
 	s := v1.Service{}
-	err = f.Client.Get(context.TODO(), types.NamespacedName{Name: operatorName, Namespace: namespace}, &s)
+	err = f.Client.Get(context.TODO(), types.NamespacedName{Name: fmt.Sprintf("%s-metrics", operatorName), Namespace: namespace}, &s)
 	if err != nil {
 		return fmt.Errorf("could not get metrics Service: (%v)", err)
 	}