Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

metrics: Add RPC rate metrics to endpoints that validate TLS names #15900

Merged
merged 1 commit into from
Jan 26, 2023

Conversation

tgross
Copy link
Member

@tgross tgross commented Jan 26, 2023

Continues the work done in #15876 by extending rate metrics measurement over the various RPCs that are authenticated via TLS name validation. This changeset also canonicalizes the order-of-operations between Authenticate and that validation. We call Authenticate first so that we get identity info like the remote IP address. This changeset doesn't change the existing behavior that drops the RPC before forwarding; I think that's worth having a discussion about but I didn't want to bloat this PR with behavior changes.

Copy link
Member

@shoenig shoenig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; just one idea: what if the MeasureRPCRate signature accepted a Name() interface as the first parameter instead of a string, then instead of hardcoding endpoint names you'd just pass in the endpoint object, e.g.

k.srv.MeasureRPFRate(k, structs.RateMetricRead, args)

Or maybe that's less clear, I dunno

@tgross
Copy link
Member Author

tgross commented Jan 26, 2023

LGTM; just one idea: what if the MeasureRPCRate signature accepted a Name() interface as the first parameter instead of a string, then instead of hardcoding endpoint names you'd just pass in the endpoint object,

I like that. We could probably use it in other places like the logger too. That'll touch a lot of handlers though (including the others I have in review for this feature), so I'll break that out into its own PR.

@tgross tgross merged commit 4ba836b into main Jan 26, 2023
@tgross tgross deleted the rpc-rate-metrics-tls-name-validated branch January 26, 2023 20:04
@tgross
Copy link
Member Author

tgross commented Jan 27, 2023

LGTM; just one idea: what if the MeasureRPCRate signature accepted a Name() interface as the first parameter instead of a string, then instead of hardcoding endpoint names you'd just pass in the endpoint object, e.g.

So I started implementing this and I think it ends up being kind of boilerplate-y without a lot of readability improvement. I checked the disassembly and the strings all get interned anyways. What do you think about:

func (j *Job) measureRPCRate(op string, args structs.RequestWithIdentity) {
	j.srv.MeasureRPCRate("job", op, args)
}

which results in callers like:

// Register is used to upsert a job for scheduling
func (j *Job) Register(args *structs.JobRegisterRequest, reply *structs.JobRegisterResponse) error {
	authErr := j.srv.Authenticate(j.ctx, args)
	if done, err := j.srv.forward("Job.Register", args, args, reply); done {
		return err
	}
	j.measureRPCRate(structs.RateMetricWrite, args) // <-- HERE
	if authErr != nil {
		return structs.ErrPermissionDenied
	}
	defer metrics.MeasureSince([]string{"nomad", "job", "register"}, time.Now())

That's still pretty boilerplate-y and doesn't win us that much though. This is all the sort of thing I'd like to have rolled up in the generic RPC middleware we've discussed too, so maybe not worth the effort to do right now until we have that figured out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants