[WIP] Add metrics for OIDC

[Leo6Leo]

Fixes #7877

Proposed Changes

• Adding the Unauthenticate request count to broker filter ingress, introduce the new report methods in its the stats_reporter

Pre-review Checklist

• At least 80% unit test coverage
• E2E tests for any new behavior
• Docs PR for any user-facing impact
• Spec PR for any new API feature
• Conformance test for any change to the spec

Release Note

Docs

[knative-prow]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[knative-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Leo6Leo
Once this PR has been reviewed and has the lgtm label, please assign creydr for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[Leo6Leo]

How do you think the way here to distinguish these 2 different scenario? Are there any better approach you would recommend? @creydr

[creydr]

I think this can work. You could also think about passing the reporter to the tokenVerifier, so this reports the metrics on each request and not only do this in the caller 🤔

[Leo6Leo]

/cc @creydr Am I on the right track? I am only doing broker filter for now, just in case my approach is wrong.

[Leo6Leo]

Does this looks good? @creydr By passing the statsReporter to the verifier, and pass the reportArgs when verifying happens.

[creydr]

Can we make this an optional argument (e.g. by using a configuration-option function as we do for example in

eventing/pkg/reconciler/testing/v1/broker.go

Lines 35 to 44 in 825202f

	func NewBroker(name, namespace string, o ...BrokerOption) *v1.Broker {
	b := &v1.Broker{
	ObjectMeta: metav1.ObjectMeta{
	Namespace: namespace,
	Name: name,
	},
	}
	for _, opt := range o {
	opt(b)
	}

with type BrokerOption func(*v1.Broker))? This allows us to add options, without the need to update the packages immediately which use the NewOIDCTokenVerifier.

[codecov]

Codecov Report

Attention: Patch coverage is 15.18987% with 67 lines in your changes missing coverage. Please review.

Project coverage is 67.50%. Comparing base (825202f) to head (82f8327).
Report is 110 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/broker/filter/filter_handler.go	0.00%	30 Missing ⚠️
pkg/broker/filter/stats_reporter.go	50.00%	12 Missing ⚠️
pkg/auth/token_verifier.go	0.00%	11 Missing ⚠️
pkg/broker/ingress/ingress_handler.go	0.00%	5 Missing ⚠️
pkg/channel/event_receiver.go	0.00%	5 Missing ⚠️
cmd/broker/filter/main.go	0.00%	2 Missing ⚠️
cmd/jobsink/main.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8015      +/-   ##
==========================================
- Coverage   67.79%   67.50%   -0.29%     
==========================================
  Files         363      364       +1     
  Lines       16899    17113     +214     
==========================================
+ Hits        11456    11552      +96     
- Misses       4735     4841     +106     
- Partials      708      720      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Leo6Leo]

Creating an interface to avoid import cycle.

Before this, it is stuck with a import cycle between auth and filter packages. The auth code needs filter's stats reporting, but filter is already using auth stuff. To fix this I created a simple interface in auth that just covers the stats we need. Then making an adapter in filter to bridge the gap. Is this a good approach to solve this problem?

[creydr]

As this is not only relevant to broker-filters anymore, should we move this StatsReporter to a dedicated package and make it more generic? E.g. pkg/metrics?
(This could also help to remove the import cycle.)

Then you could also change the ReportArgs to an interface (which has the method generateTag()) to have component specific args. E.g. from a Trigger (which would to the same as the current reporter.generateTag(), or then for a Channel, etc.

[creydr]

So for example:

type ReportArgs interface { //TODO: find a better name
    generateTag(tags ...tag.Mutator) (context.Context, error)
}

and then for example for the broker/trigger:

type BrokerArgs struct {
	ns            string
	trigger       string
	broker        string
	filterType    string
	requestType   string
	requestScheme string
}

func (args *BrokerArgs) generateTag(tags ...tag.Mutator) (context.Context, error) {
	ctx := metricskey.WithResource(emptyContext, resource.Resource{
		Type: eventingmetrics.ResourceTypeKnativeTrigger,
		Labels: map[string]string{
			eventingmetrics.LabelNamespaceName: args.ns,
			eventingmetrics.LabelBrokerName:    args.broker,
			eventingmetrics.LabelTriggerName:   args.trigger,
		},
	})
	// Note that filterType and filterSource can be empty strings, so they need a special treatment.
	ctx, err := tag.New(
		ctx,
		append(tags,
			tag.Insert(triggerFilterTypeKey, valueOrAny(args.filterType)),
			tag.Insert(triggerFilterRequestTypeKey, args.requestType),
			tag.Insert(triggerFilterRequestSchemeKey, args.requestScheme),
		)...)
	return ctx, err
}

And then we call this e.g. in ReportEventCount():

func (r *reporter) ReportEventCount(args ReportArgs, responseCode int) error {
	ctx, err := args.generateTag(
		tag.Insert(responseCodeKey, strconv.Itoa(responseCode)),
		tag.Insert(responseCodeClassKey, metrics.ResponseCodeClass(responseCode)))
	if err != nil {
		return err
	}
	metrics.Record(ctx, eventCountM.M(1))
	return nil
}

and use it

reportArgs := &BrokerArgs{
	Ns:          broker.Namespace,
	Broker:      broker.Name,
	RequestType: "broker_ingress",
}

err = h.tokenVerifier.VerifyJWTFromRequest(ctx, request, broker.Status.Address.Audience, writer, reportArgs)

[knative-prow-robot]

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[github-actions]

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Reopen with /reopen. Mark as fresh by adding the
comment /remove-lifecycle stale.