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

[DBPW 2/5] Create middleware for Database v5 #9642

Merged
merged 5 commits into from
Sep 1, 2020
Merged

Conversation

pcman312
Copy link
Contributor

@pcman312 pcman312 commented Jul 30, 2020

Overview

This PR is part of a larger feature adding support for password policies into the combined database engine. This feature is being split into multiple PRs to make for smaller reviews & earlier feedback.

Adds middleware for Database v5. This includes the same middleware as Database v4 (tracing, metrics, and error sanitization). The error sanitization has some bug fixes from the v4 version (like not panicking and only accepting a map[string]string instead of a map[string]interface{})

Database v4 version: sdk/database/dbplugin/databasemiddleware.go

Prerequisites

Related PRs

Original password policies PR
1/X - Database interface & gRPC
3/X - Plugin handling
4/X - Database engine

@pcman312 pcman312 changed the base branch from master to new-database-interface July 30, 2020 21:16
@pcman312 pcman312 changed the base branch from new-database-interface to master July 30, 2020 21:17
@pcman312 pcman312 changed the base branch from master to new-database-interface July 30, 2020 21:17
sdk/database/newdbplugin/middleware_test.go Show resolved Hide resolved
sdk/database/newdbplugin/middleware.go Outdated Show resolved Hide resolved

type secretsFn func() map[string]string

func NewDatabaseErrorSanitizerMiddleware(next Database, secrets secretsFn) DatabaseErrorSanitizerMiddleware {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I know why we're returning a concrete object here (not really modifying the object's fields after instantiation) and why we're not dealing with pointer receivers for its methods, but can you elaborate to confirm my assertions?

Since this is a middleware and likely to be passed around and reference as an interface, I am not sure if this can be problematic upstream at some point if the caller expects a pointer value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You nailed it: the middleware isn't changing after it is initially created. Passing it around under an interface type won't change any behaviors here. All interface types eg var db Database are implicitly pointers so you can do things like nil checks, however the underlying object such as DatabaseErrorSanitizerMiddleware can be a concrete, non-pointer type without issue. Since the middleware type is never manipulating its own variables in any of its functions this won't cause any problems.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any cases where you'd need to type assert on the interface to a typed pointer value followed by a nil check?

Copy link
Contributor Author

@pcman312 pcman312 Aug 18, 2020

Choose a reason for hiding this comment

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

None that I've found. All of the usages of this use the V4 version of this use it as the Database interface, not as the underlying middleware type.

@pcman312
Copy link
Contributor Author

pcman312 commented Aug 7, 2020

FYI: the force pushing is due to rebasing against the dependent PR #9641. Once that PR is merged & the base branch is updated to master the force pushing will cease.

Base automatically changed from new-database-interface to master August 28, 2020 17:22
@tvoran tvoran self-requested a review August 28, 2020 22:00
@pcman312 pcman312 requested a review from austingebauer August 31, 2020 22:18
@pcman312 pcman312 merged commit 2aa9292 into master Sep 1, 2020
@pcman312 pcman312 deleted the dbpw-middleware branch September 1, 2020 16:46
@pcman312 pcman312 changed the title [DBPW 2/X] Create middleware for Database v5 [DBPW 2/5] Create middleware for Database v5 Oct 7, 2020
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.

3 participants