Implement Citadel prometheus monitoring feature. #5015

myidpt · 2018-04-17T18:50:55Z

This monitoring feature exposes a service (port 9093) about Citadel status to prometheus.
Yaml files are changed in #5072.
Tests will be added shortly.

codecov · 2018-04-18T03:23:24Z

Codecov Report

Merging #5015 into master will increase coverage by 1%.
The diff coverage is 98%.

@@          Coverage Diff           @@
##           master   #5015   +/-   ##
======================================
+ Coverage      74%     74%   +1%     
======================================
  Files         310     312    +2     
  Lines       25816   25792   -24     
======================================
- Hits        18888   18875   -13     
+ Misses       6167    6154   -13     
- Partials      761     763    +2

Impacted Files	Coverage Δ
security/pkg/pki/ca/ca.go	`78% <ø> (ø)`	⬆️
security/pkg/server/ca/monitoring.go	`100% <100%> (ø)`
security/pkg/pki/ca/controller/secret.go	`74% <100%> (+1%)`	⬆️
security/pkg/pki/ca/controller/monitoring.go	`100% <100%> (ø)`
security/pkg/server/ca/server.go	`84% <86%> (+1%)`	⬆️
...olarwinds/internal/papertrail/papertrail_logger.go	`62% <0%> (-18%)`	⬇️
pilot/pkg/serviceregistry/eureka/controller.go	`82% <0%> (-9%)`	⬇️
mixer/adapter/servicecontrol/utils.go	`84% <0%> (-5%)`	⬇️
pilot/pkg/serviceregistry/kube/controller.go	`63% <0%> (-4%)`	⬇️
pilot/pkg/serviceregistry/kube/queue.go	`86% <0%> (-3%)`	⬇️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a5ef4a...368f9f8. Read the comment docs.

incfly

Not familiar with promethus, take it as a similar thing as google internal monitoring sys.

incfly · 2018-04-19T17:06:17Z

security/cmd/istio_ca/main.go


 	flags.BoolVar(&opts.signCACerts, "sign-ca-certs", false, "Whether the CA signs certificates for other CAs")

+	// Monitoring configuration
+	flags.IntVar(&opts.monitoringPort, "monitoring-port", 9093, "The port number for monitoring Citadel. "+
+		"If unspecified, Istio CA will disable monitoring.")


Should be consistently using "Citadel"

Sure. Good catch.

incfly · 2018-04-19T17:07:53Z

security/pkg/server/monitoring/monitoring.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package monitoring


Can we put this in pkg/citadel?

Sure. I will do the migration in another PR.

incfly · 2018-04-19T17:08:50Z

security/pkg/server/monitoring/monitoring.go

+)
+
+// StartMonitor starts the Citadel monitor.
+func StartMonitor(port int, enableProfiling bool, errCh chan<- error) (*Monitor, error) {


To be consistent, consider to define:

NewMonitor() *Monitor, error

Monitor.Start()

incfly · 2018-04-19T17:29:39Z

security/pkg/monitoring/monitoring.go

+		Help:      "The number of certificates issuances that have succeeded.",
+	}, caLabels)
+
+	cSRErrorCount = prometheus.NewCounterVec(prometheus.CounterOpts{


We should use csrErrorCount, the abbreviation should be all little case or capital case.
https://github.com/golang/go/wiki/CodeReviewComments#initialisms

Same for the rest of variable.

incfly · 2018-04-19T17:40:23Z

security/pkg/pki/ca/ca.go

@@ -82,7 +83,8 @@ type IstioCA struct {

 	keyCertBundle util.KeyCertBundle

-	livenessProbe *probe.Probe
+	monitoringCounters monitoring.CAOperationCounters


We should make pkg/pki not depend on monitoring libraries. Instead can we:

Define the customized error code for signing related logic, say TTL_FAIL, INVALID_PEM, etc.

ca.go returned the customized error to server.go

The metrics defined there can do signingError[errorCode].Inc()

In this way, we also don't have to define one monitoring metric per error type, which makes code simpler, also in monitoring post-processing side, we can use more expressive to query the motoring metrics.

That makes sense. I will update the code.

incfly · 2018-04-19T17:42:59Z

security/pkg/monitoring/monitoring.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package monitoring


I think as a top level package, pkg/monitoring should only provide generic monitoring facilities. We can define the metrics only where it's used. So that, secrets.go is never able to NewCAOperationCounters().

That's a good call. I will put them into each directory.

myidpt · 2018-04-19T21:44:09Z

@incfly I think they are similar. Citadel only needs to use the client lib to expose the HTTP endpoint for Prometheus to call.

incfly

Thanks for doing this. Just couple of minor suggestions.

incfly · 2018-04-19T23:01:31Z

security/pkg/server/ca/server.go

 }

 // HandleCSR handles an incoming certificate signing request (CSR). It does
 // proper validation (e.g. authentication) and upon validated, signs the CSR
 // and returns the resulting certificate. If not approved, reason for refusal
 // to sign is returned as part of the response object.
 func (s *Server) HandleCSR(ctx context.Context, request *pb.CsrRequest) (*pb.CsrResponse, error) {
+	s.monitoring.CSR.Inc()


Is promethus counter thread safe? otherwise, server needs to have mutex to avoid racing.

That's a good point. I looked it up and found:
"All exported functions and methods are safe to be used concurrently unless specified otherwise."
https://godoc.org/github.com/prometheus/client_golang/prometheus
That's good news :)

incfly · 2018-04-19T23:06:15Z

security/pkg/server/ca/server_test.go

@@ -179,6 +179,7 @@ func TestSign(t *testing.T) {
 			port:           8080,
 			authorizer:     c.authorizer,
 			authenticators: c.authenticators,
+			monitoring:     newMonitoringMetrics(),


Should we add some test for monitoring if monitoring dependency can be arranged? Could do it later.

Yes, I will add the unit tests later.

We should probably add a dashboard for Citadel to the existing dashboards and add an e2e test to handle while we are at it.

For the dashboard, what do we need to change except the yaml file?

we would need to build a new dashboard like the mixer and pilot ones in: https://github.com/istio/istio/tree/master/addons/grafana/dashboards

And then add a test that validates the queries in that dashboard work.

I can help with that after this stuff goes in.

incfly · 2018-04-19T23:25:27Z

security/pkg/server/ca/server.go

 	caller := s.authenticate(ctx)
 	if caller == nil {
 		log.Warn("request authentication failure")
+		s.monitoring.AuthenticationError.Inc()


I think we're good to move forward for now. This is for potential future improvements: would it better to have one metric and using its different labels for each type of error? These metrics users can just look at these errors together or break down by error (label value) if they want. Otherwise they have to find all metrics' name and sum them up.

(Above is assuming promethus behaves simliarly to google's internal monitoring sys).

Yes. That's my plan. I've added a TODO in ca file to have Sign() return an error code. I will implement it later.

incfly · 2018-04-20T17:18:49Z

/approve
/lgtm

istio-merge-robot · 2018-04-20T17:20:07Z

/test all [submit-queue is verifying that this PR is safe to merge]

douglas-reid · 2018-04-20T17:22:54Z

security/cmd/istio_ca/main.go

+		}
+		go monitor.Start(monitorErrCh)
+		log.Info("Citadel monitor has started.")
+		defer monitor.Close()


is runCA() expected to not return until server shutdown?

Yes. Any concerns?

douglas-reid · 2018-04-20T17:25:06Z