Skip to content

[DBPW 1/5] Add Database v5 interface with gRPC client & server#9641

Merged
pcman312 merged 19 commits into
masterfrom
new-database-interface
Aug 28, 2020
Merged

[DBPW 1/5] Add Database v5 interface with gRPC client & server#9641
pcman312 merged 19 commits into
masterfrom
new-database-interface

Conversation

@pcman312
Copy link
Copy Markdown
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.

Creates a new Database interface for use in the combined database engine. This new interface will be v5 of the engine and is designed to support password policies. It also changes the pattern of the previous interface to a more gRPC style for better future compatibility. This is done so changes to the interface in the future are hopefully less painful.

Nothing is referencing this package yet. That will happen in a subsequent PR

Files

sdk/database/newdbplugin/ - New package for keeping all of the v5 database code. This is a temporary location. This package will replace the existing sdk/database/dbplugin package in a subsequent PR. The existing sdk/database/dbplugin package will be renamed to indicate it is deprecated.
database.go - New v5 interface & associated request/response objects
proto/database.proto - New v5 interface as defined in gRPC
grpc_client.go - gRPC client implementation of proto/database.proto. This does the conversion between Go types in database.go to protobuf-generated types.
grpc_server.go - gRPC server implementation of proto/database.proto. This does the conversion between protobuf-generated types and Go types in database.go.

Related PRs

Original password policies PR
2/X - Middleware
3/X - Plugin handling
4/X - Database engine

@pcman312 pcman312 changed the title [DBPW] Add Database v5 interface with gRPC client & server [DBPW 1/X] Add Database v5 interface with gRPC client & server Jul 30, 2020
Comment thread sdk/database/newdbplugin/database.go
Comment thread sdk/database/newdbplugin/database.go
Comment thread sdk/database/newdbplugin/database.go
Comment thread sdk/database/newdbplugin/database.go
Comment thread sdk/database/newdbplugin/grpc_client.go Outdated
Comment thread sdk/database/newdbplugin/grpc_server_test.go Outdated
Copy link
Copy Markdown
Member

@tvoran tvoran left a comment

Choose a reason for hiding this comment

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

Just some questions about statements.

Comment thread sdk/database/newdbplugin/proto/database.proto
Comment thread sdk/database/newdbplugin/grpc_client_test.go
Comment thread sdk/database/newdbplugin/grpc_server_test.go
Comment thread sdk/database/newdbplugin/database.go
@hashicorp hashicorp deleted a comment Aug 24, 2020
Copy link
Copy Markdown
Member

@tvoran tvoran left a comment

Choose a reason for hiding this comment

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

I think there's some missing statements logic.

Comment thread sdk/database/newdbplugin/grpc_server.go
Comment thread sdk/database/newdbplugin/grpc_server.go
Comment thread sdk/database/newdbplugin/database.go Outdated
Comment thread sdk/database/newdbplugin/grpc_client.go
Comment thread sdk/database/newdbplugin/grpc_server.go
Copy link
Copy Markdown
Contributor

@austingebauer austingebauer left a comment

Choose a reason for hiding this comment

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

Looking good overall! Just a few small comments and suggestions.

Comment thread sdk/database/newdbplugin/database.go Outdated
Comment thread sdk/database/newdbplugin/database.go Outdated
Comment thread sdk/database/newdbplugin/database.go Outdated
Comment thread sdk/database/newdbplugin/grpc_server.go
Comment thread sdk/database/newdbplugin/grpc_server.go
Copy link
Copy Markdown
Contributor

@austingebauer austingebauer left a comment

Choose a reason for hiding this comment

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

LGTM

@pcman312 pcman312 merged commit 6478665 into master Aug 28, 2020
@pcman312 pcman312 deleted the new-database-interface branch August 28, 2020 17:22

func getUpdateUserRequest(req *proto.UpdateUserRequest) (UpdateUserRequest, error) {
var password *ChangePassword
if req.GetPassword() != nil && req.GetPassword().GetNewPassword() != "" {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

GetNewPassword() has a built-in nil check on the receiver so you shouldn't need req.GetPassword() != nil to protect against potential panic, but it's good to be explicit.

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.

4 participants