Enable Auth for the metrics endpoints for the controllers #81

sohankunkerkar · 2019-11-07T20:10:38Z

No description provided.

sohankunkerkar · 2019-11-08T04:13:40Z

/test e2e-aws

stlaz · 2019-11-08T14:36:18Z

/retest

stlaz · 2019-11-11T16:13:17Z

/test e2e-aws-operator

sohankunkerkar · 2019-11-11T16:51:53Z

/test e2e-aws

marun · 2019-11-11T20:24:44Z

bindata/v4.0.0/apiservice-cabundle-controller/defaultconfig.yaml

 apiVersion: servicecertsigner.config.openshift.io/v1alpha1
 kind: APIServiceCABundleInjectorConfig
 caBundleFile: /var/run/configmaps/signing-cabundle/ca-bundle.crt
-authentication:


@stlaz Is this just a configuration change that prompts library code to ensure the metrics endpoint is secured? If so, why would that require operator-specific e2e coverage?

It would require a library extended test, or at least an integration one, but we would still want to check that the operator is actually behaving the intended way and publishes it's metrics as desired, no?

I finally got to browse through the code - it was not my point to try to check that the endpoints are authenticated, but that they are actually showing something

~~Couldn't that be validated in a unit test?~~

Edit: Likely not a unit test, but if the metrics endpoints is exposed by a library function, would it make sense to implement a library test that operators could trivially reuse? i.e.

func TestMetricsEndpoint(t *testing.T) { libraryTestFunc(t, myEndpointCfg, func(result []byte) { # validate the output in an operator-specific way }) }

I think that that would be a fine general approach to test an endpoint, yes. I wonder how many endpoints we'd like to test this way though.

I've identified 10 operators in the service ca compatibility audit that should be tested this way, and there are likely more.

I think what @marun suggested makes sense to me.

stlaz · 2019-11-12T14:31:07Z

test/e2e/e2e_test.go

+		TypeMeta: metav1.TypeMeta{
+			Kind:       "Route",
+			APIVersion: "route.openshift.io/v1",
+		},


No need to specify TypeMeta, I think the client takes care of it

stlaz · 2019-11-12T14:32:14Z

test/e2e/e2e_test.go

+		TypeMeta: metav1.TypeMeta{
+			Kind:       "Service",
+			APIVersion: "v1",
+		},


stlaz · 2019-11-12T14:32:52Z

test/e2e/e2e_test.go

+		ObjectMeta: metav1.ObjectMeta{
+			Name:      s.ObjectMeta.Name,
+			Namespace: s.ObjectMeta.Namespace,
+			Labels:    labels,
+		},
+		Spec: routev1.RouteSpec{
+			To: routev1.RouteTargetReference{
+				Kind: "Service",
+				Name: s.ObjectMeta.Name,
+			},
+			Port: &routev1.RoutePort{
+				TargetPort: s.Spec.Ports[0].TargetPort,
+			},


Just use constant names

stlaz · 2019-11-12T14:54:42Z

bindata/v4.0.0/apiservice-cabundle-controller/defaultconfig.yaml

 apiVersion: servicecertsigner.config.openshift.io/v1alpha1
 kind: APIServiceCABundleInjectorConfig
 caBundleFile: /var/run/configmaps/signing-cabundle/ca-bundle.crt
-authentication:


I finally got to browse through the code - it was not my point to try to check that the endpoints are authenticated, but that they are actually showing something

stlaz · 2019-11-12T14:58:55Z

test/e2e/e2e_test.go

+	}
+	defer res.Body.Close()
+	if res.StatusCode != 403 {
+		t.Fatalf("The metrics endpoint is not secured: %v", err)


You may want to see what was actually returned in order to realize what went wrong in the test + start with lowercase letter 🙂

stlaz · 2019-11-12T14:59:50Z

test/e2e/e2e_test.go

+
+	url := "http://" + route.Spec.Host + "/metrics"
+
+	res, err := http.Get(url)


This check is fine, but I would like you to also see that the metrics endpoint actually returns some metrics

sohankunkerkar · 2019-11-13T18:00:10Z

/test e2e-aws

test/e2e/e2e_test.go

sohankunkerkar · 2019-11-19T14:49:40Z

/test e2e-aws-operator

test/e2e/e2e_test.go

sohankunkerkar · 2019-11-19T18:06:21Z

/test e2e-aws

stlaz · 2019-11-20T10:50:07Z

test/e2e/e2e_test.go

 }

+// generateRoute creates a OpenShift routes
+func generateRoute(routeClient *routeclient.RouteV1Client, s *v1.Service, namespace string) (*routev1.Route, error) {


there's still the naming discrepancy with createService, call this createRouteForService

test/e2e/e2e_test.go

stlaz · 2019-11-20T10:52:35Z

test/e2e/e2e_test.go

+
+	var header string
+	for k, v := range res.Header {
+		header = fmt.Sprintf("Header field %q, Value %q, Statuscode %q\n", k, v, strconv.Itoa(res.StatusCode))


are you just rewriting the header variable for every key in the res.Header map?

stlaz · 2019-11-20T10:56:40Z

test/e2e/e2e_test.go

+	for k, v := range res.Header {
+		header = fmt.Sprintf("Header field %q, Value %q, Statuscode %q\n", k, v, strconv.Itoa(res.StatusCode))
+	}
+	fmt.Println(header)


use t.Logf(), and only in the error case here

stlaz · 2019-11-20T11:01:51Z

test/e2e/e2e_test.go

+		t.Fatalf("error getting route client: %v", err)
+	}
+
+	// Test case to validate if metrics endpoint for the apiservice-cabundle-injector controller is secured.


for _, controller := []string{"apiservice-cabundle-injector", "configmap-cabundle-injector", "service-serving-cert-signer"}, derive your service name for testMetricsEndpoint() from the controller name, last argument of that function is constant.

Improve the test case name, at leat to <controller-name> /metrics

sohankunkerkar · 2019-11-20T15:28:29Z

/test e2e-aws-operator

marun · 2019-11-20T15:16:05Z

test/e2e/e2e_test.go

 }

+// createRouteForService creates a OpenShift routes
+func createRouteForService(routeClient *routeclient.RouteV1Client, s *v1.Service, namespace string) (*routev1.Route, error) {


Exposing the metrics endpoint with a route should not be necessary here - way too complicated. The endpoint should be checked from inside the cluster

I think that sounds reasonable to me. I have designed this logic after having some discussion with @stlaz
@stlaz wdyt?

either is fine I think, I noticed people from console doing this and it looked like a good idea to me, for testing

A common pattern in upstream kube when needing to validate a cluster-internal endpoint is to launch a busybox pod that connects to the endpoint. This pattern limits the scope of test dependency. Why is it desirable for validation of internal connectivity to depend on external connectivity?

marun · 2019-11-20T15:27:57Z

bindata/v4.0.0/apiservice-cabundle-controller/clusterrole.yaml

+- apiGroups:
+  - ""
+  resources:
+  - configmaps


Is it necessary for the controller to access configmaps across the entire cluster, or only in the service-ca namespace?

I have added that change after seeing the following error:

configmaps "extension-apiserver-authentication" is forbidden: User "system:serviceaccount:openshift-service-ca:apiservice-cabundle-injector-sa" cannot get resource "configmaps" in API group "" in the namespace "kube-system"

That suggests limiting the scope of authorization to that resource in that namespace rather than granting global access to configmaps.

marun · 2019-11-20T15:28:24Z

test/e2e/e2e_test.go

+	}
+
+	// Test case to validate if metrics endpoint for the controllers are secured.
+	for _, controller := range []string{"apiservice-cabundle-injector", "configmap-cabundle-injector", "service-serving-cert-signer"} {


What about the operator?

The test cases are added in conjunction with the changes done in this PR. I can add another test case if that serves the purpose.

A metrics endpoint is a metrics endpoint - it is exposed by the controller framework that both the operator and its controllers use. It should be possible to test any metrics endpoint with the same code.

marun · 2019-11-20T15:39:57Z

test/e2e/e2e_test.go

+	httpclient := &http.Client{
+		Transport: &http.Transport{
+			TLSClientConfig: &tls.Config{
+				InsecureSkipVerify: true,


This needs to be false.

A metrics endpoint needs to be secured with a service ca-provided serving cert.

The metrics endpoint is automatically secured by a serving cert provided the pod template has the serving cert secret mounted to /var/run/secrets/serving-cert (this is already true for the operator).

The controller framework will generate an insecure self-signed certificate on startup and will exit the pod when the cert files are updated, so even the service ca controllers and operators can be secured with serving certs.

Configuring each controller with a service ca-provided ca bundle will require creating an annotated service ( service.beta.openshift.io/serving-cert-secret-name) that exposes the metrics endpoint.

The client to access the metrics endpoint needs to be configured with a service ca-provided ca bundle (see the rotation tests for how to accomplish this).

@stlaz Who is going to watch these new endpoints, and how do they discover them?

Those endpoints are not new, they are being scraped by monitoring operator, they most probably use their service account for that.

The InsecureSkipVerify is here as you're not trying to see whether the endpoint is secured by a valid certificate, that it should always be, otherwise scraping the data would not work, it merely tested that you can't scrape it unauthenticated.

The use of InsecureSkipVerify is masking a bug. Metrics endpoints are not secured automatically by a valid certificate, as per my previous comment, and anyone trying to connect to the endpoint enabled by the current state of this PR is going to be unable to validate the cert.

Can you please point me to how the monitoring operator knows how to read from these endpoints? Given that the endpoint cert is not secure in the current iteration of this PR, I am interested to know how to verify on the monitoring operator side whether the endpoints it is charged with collecting from are configured properly. I found the monitoring doc instructions and posted them in channel.

sohankunkerkar · 2019-11-20T20:14:59Z

/test e2e-aws-operator

marun · 2019-11-20T21:31:24Z

Given that these metrics endpoints are not currently being used by the cluster, I think there are 2 potential paths forward:

Disable the endpoints
- Who is intended to use them in their current form?
  - They are using insecure self-signed certificates.
Configure prometheus monitoring
- According to the monitoring doc this is the responsibility of all operators anyway.
  - The doc suggests that instrumenting an operator is likely to take a few hours.

I do not believe that enabling authn/authz on the metrics endpoints and writing e2e tests to verify that configuration, as currently proposed, is worth pursuing. Testing what is essentially a function of library go (configuration of metrics endpoints) should be out-of-scope for this operator. If the endpoints are intended to be used, testing that prometheus monitoring is properly configured to use them would ensure proper configuration without requiring bespoke testing. There is already at least one example of testing that prometheus is receiving alerts and this pattern could probably be generalized for inclusion in library-go (cc: @sallyom).

marun · 2019-11-22T04:08:30Z

LGTM

@stlaz I asked for the e2e test be removed. Ensuring that the generic operator config doesn't disable the auth is sufficient to ensure that the endpoint isn't accessible to unauthorized access. A follow-on PR can configure prometheus to read from the endpoint and leverage a generic test for prometheus metric configuration that @sallyom and @sanchezl are working on adding to library-go.

stlaz · 2019-11-22T14:31:48Z

/lgtm
@marun sounds good to me

openshift-ci-robot · 2019-11-22T14:32:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sohankunkerkar, stlaz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [stlaz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 7, 2019

openshift-ci-robot requested review from deads2k and stlaz November 7, 2019 20:11

Enable Auth for metrics endpoint

7a4c32a

sohankunkerkar force-pushed the fix_metrics_endpoint branch from 6173cc9 to 7a4c32a Compare November 7, 2019 21:54

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 10, 2019

sohankunkerkar force-pushed the fix_metrics_endpoint branch 4 times, most recently from ae7f6e3 to df673d6 Compare November 11, 2019 15:24

sohankunkerkar force-pushed the fix_metrics_endpoint branch from df673d6 to 7124468 Compare November 11, 2019 18:04

marun reviewed Nov 11, 2019

View reviewed changes

sohankunkerkar force-pushed the fix_metrics_endpoint branch 3 times, most recently from 70a153d to b43a5b5 Compare November 12, 2019 04:50

stlaz reviewed Nov 12, 2019

View reviewed changes

sohankunkerkar force-pushed the fix_metrics_endpoint branch 4 times, most recently from 15456f5 to 6e971b8 Compare November 13, 2019 17:36

sohankunkerkar force-pushed the fix_metrics_endpoint branch from 6e971b8 to 4e8662b Compare November 13, 2019 18:54

stlaz reviewed Nov 14, 2019

View reviewed changes

test/e2e/e2e_test.go Outdated Show resolved Hide resolved

test/e2e/e2e_test.go Outdated Show resolved Hide resolved

stlaz suggested changes Nov 19, 2019

View reviewed changes

test/e2e/e2e_test.go Outdated Show resolved Hide resolved

sohankunkerkar force-pushed the fix_metrics_endpoint branch 2 times, most recently from b95b27f to 1ccfb97 Compare November 19, 2019 16:26

stlaz suggested changes Nov 20, 2019

View reviewed changes

sohankunkerkar force-pushed the fix_metrics_endpoint branch 2 times, most recently from cb78062 to 9229697 Compare November 20, 2019 14:34

marun suggested changes Nov 20, 2019

View reviewed changes

sohankunkerkar force-pushed the fix_metrics_endpoint branch 2 times, most recently from b7196b2 to 879aa7b Compare November 20, 2019 18:44

sohankunkerkar force-pushed the fix_metrics_endpoint branch from 879aa7b to 7a4c32a Compare November 21, 2019 18:40

sohankunkerkar force-pushed the fix_metrics_endpoint branch from 08b23df to 032ddaa Compare November 21, 2019 20:13

Generated code

6f2a866

sohankunkerkar force-pushed the fix_metrics_endpoint branch from 032ddaa to 6f2a866 Compare November 21, 2019 20:29

sohankunkerkar changed the title ~~Enable Auth for metrics endpoint for controllers~~ Enable Auth for the metrics endpoints for the controllers Nov 21, 2019

openshift-ci-robot assigned stlaz Nov 22, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 22, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 22, 2019

openshift-merge-robot merged commit 1e69be2 into openshift:master Nov 22, 2019


		url := "http://" + route.Spec.Host + "/metrics"

		res, err := http.Get(url)

Enable Auth for the metrics endpoints for the controllers #81

Enable Auth for the metrics endpoints for the controllers #81

Uh oh!

Conversation

sohankunkerkar commented Nov 7, 2019

Uh oh!

sohankunkerkar commented Nov 8, 2019

Uh oh!

stlaz commented Nov 8, 2019

Uh oh!

stlaz commented Nov 11, 2019

Uh oh!

sohankunkerkar commented Nov 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marun Nov 12, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sohankunkerkar commented Nov 13, 2019

Uh oh!

Uh oh!

Uh oh!

sohankunkerkar commented Nov 19, 2019

Uh oh!

Uh oh!

sohankunkerkar commented Nov 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sohankunkerkar commented Nov 20, 2019

Uh oh!

marun Nov 20, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marun Nov 12, 2019 •

edited

Loading

marun Nov 20, 2019 •

edited

Loading

marun Nov 20, 2019 •

edited

Loading

marun Nov 20, 2019 •

edited

Loading

marun commented Nov 20, 2019 •

edited

Loading