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

Secrets Manager Proxy #121

Merged
merged 28 commits into from
Mar 30, 2023
Merged

Secrets Manager Proxy #121

merged 28 commits into from
Mar 30, 2023

Conversation

yanw-bq
Copy link
Contributor

@yanw-bq yanw-bq commented Mar 23, 2023

Review Status

  • This is ready for review
  • This is complete

Summary

Implement Secrets Manager Proxy

Description

  • Introduce SECRETS_MANAGER_PROXY
  • Unit Tests

Additional Reviewers

@justing-bq @sergiyvamz

@yanw-bq yanw-bq changed the title Secrets manager Secrets Manager Proxy Mar 28, 2023
@yanw-bq yanw-bq marked this pull request as ready for review March 28, 2023 17:24
driver/secrets_manager_proxy.cc Outdated Show resolved Hide resolved
driver/secrets_manager_proxy.cc Outdated Show resolved Hide resolved
}
}

bool SECRETS_MANAGER_PROXY::real_connect(const char* host, const char* user, const char* passwd, const char* db,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a fan of overriding the real_connect and real_connect_dns_srv in this way.
Instead can we do the same approach as my PR where we define

bool CONNECTION_PROXY::connect(const char* host, const char* user, const char* password,
    const char* database, unsigned int port, const char* socket, unsigned long flags) {

    if (ds->enable_dns_srv) {
        return this->real_connect_dns_srv(host, user, password, database, flags);
    }

    return this->real_connect(host, user, password, database, port, socket, flags);
}

And then move all of this secrets manager connect logic to

bool SECRETS_MANAGER_PROXY::connect(const char* host, const char* user, const char* password,
	const char* database, unsigned int port, const char* socket, unsigned long flags)

And in here we can call next_proxy->connect() instead of next_proxy->real_connect().

Look at my PR for reference.

auto username = get_from_secret_json_value(USERNAME_KEY);
auto password = get_from_secret_json_value(PASSWORD_KEY);
fetched = false;
auto ret = next_proxy->real_connect(host, username.c_str(), password.c_str(), db, port, unix_socket, clientflag);
Copy link
Contributor

Choose a reason for hiding this comment

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

In general I would prefer to see the actual type rather than the auto keyword (unless the type is super unwieldy)

return ret;
}

bool SECRETS_MANAGER_PROXY::real_connect_dns_srv(const char* dns_srv_name, const char* user, const char* passwd,
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise. Another benefit of my earlier suggestion is that we won't need to repeat all this logic again.

{
std::unique_lock<std::mutex> lock(secrets_cache_mutex);

if (const auto search = secrets_cache.find(this->secret_key); search != secrets_cache.end() && !force_re_fetch) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not really familiar with this if (A; B) notation. Similar to a for loop, I'm guessing the first part is just initialization while the second part is the actual if condition? If so, can we move the const auto search = secrets_cache.find(this->secret_key); to above the if statement? I think it's more clear that way.

auto res_json = Aws::Utils::Json::JsonValue(json_string);
if (!res_json.WasParseSuccessful()) {
MYLOG_DBC_TRACE(dbc, res_json.GetErrorMessage().c_str());
throw std::runtime_error("Error parsing secrets manager response body. " + res_json.GetErrorMessage());
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does this exception get caught and handled?

else {
const auto error = "Unable to extract the " + key + " from secrets manager response.";
MYLOG_DBC_TRACE(dbc, error.c_str());
throw std::runtime_error(error);
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise. Where does this exception get caught?

unit_testing/mock_objects.h Show resolved Hide resolved
/**
* A helper class to initialize/shutdown AWS API once per DLL load/unload.
*/
class AWS_SDK_HELPER {
Copy link
Contributor

Choose a reason for hiding this comment

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

With a class this small I'm fine with just defining the entire class in a header file rather than splitting into a .h and .cc.

if (!secret_ID) {
const auto error = "Missing required config parameter for Secrets Manager: Secret ID";
MYLOG_DBC_TRACE(dbc, error);
this->set_custom_error_message(error);
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tested this error?

Doing this works in the connect() logic but I'm less sure of the constructor logic.

@yanw-bq yanw-bq merged commit 165e902 into dev Mar 30, 2023
@yanw-bq yanw-bq deleted the secrets-manager branch March 30, 2023 06:08
justing-bq pushed a commit that referenced this pull request Apr 28, 2023
* new secrets_manager files

* working secrets manager client

* add cache for secrets manager proxy

* first unit test for secrets manager proxy

* added more unit tests

* fix unique lock missing template arguments

* fix multiple calls to aws api in secrets manager unit tests

* fix multiple aws init/shutdown api calls in integration tests

* fix calling init/shutdownApi in windows

* verbose log for windows community tests

* wrap initapi and shutdownapi inside secrets manager proxy contructor/destructor

* move init/shutdown aws api to alloc/free env handle

* move init/shutdown aws api to alloc/free env handle

* test

* test

* fix static variables

* fix atomic int init

* fix unit tests

* renaming

* cleanup

* introduce aws sdk helpoer

* address comments

* nit

* fix rebase

* fix rebase

* set error

* fix build

* better error message
yanw-bq added a commit that referenced this pull request May 2, 2023
* new secrets_manager files

* working secrets manager client

* add cache for secrets manager proxy

* first unit test for secrets manager proxy

* added more unit tests

* fix unique lock missing template arguments

* fix multiple calls to aws api in secrets manager unit tests

* fix multiple aws init/shutdown api calls in integration tests

* fix calling init/shutdownApi in windows

* verbose log for windows community tests

* wrap initapi and shutdownapi inside secrets manager proxy contructor/destructor

* move init/shutdown aws api to alloc/free env handle

* move init/shutdown aws api to alloc/free env handle

* test

* test

* fix static variables

* fix atomic int init

* fix unit tests

* renaming

* cleanup

* introduce aws sdk helpoer

* address comments

* nit

* fix rebase

* fix rebase

* set error

* fix build

* better error message
yanw-bq added a commit that referenced this pull request May 2, 2023
* new secrets_manager files

* working secrets manager client

* add cache for secrets manager proxy

* first unit test for secrets manager proxy

* added more unit tests

* fix unique lock missing template arguments

* fix multiple calls to aws api in secrets manager unit tests

* fix multiple aws init/shutdown api calls in integration tests

* fix calling init/shutdownApi in windows

* verbose log for windows community tests

* wrap initapi and shutdownapi inside secrets manager proxy contructor/destructor

* move init/shutdown aws api to alloc/free env handle

* move init/shutdown aws api to alloc/free env handle

* test

* test

* fix static variables

* fix atomic int init

* fix unit tests

* renaming

* cleanup

* introduce aws sdk helpoer

* address comments

* nit

* fix rebase

* fix rebase

* set error

* fix build

* better error message
justing-bq pushed a commit that referenced this pull request May 4, 2023
* new secrets_manager files

* working secrets manager client

* add cache for secrets manager proxy

* first unit test for secrets manager proxy

* added more unit tests

* fix unique lock missing template arguments

* fix multiple calls to aws api in secrets manager unit tests

* fix multiple aws init/shutdown api calls in integration tests

* fix calling init/shutdownApi in windows

* verbose log for windows community tests

* wrap initapi and shutdownapi inside secrets manager proxy contructor/destructor

* move init/shutdown aws api to alloc/free env handle

* move init/shutdown aws api to alloc/free env handle

* test

* test

* fix static variables

* fix atomic int init

* fix unit tests

* renaming

* cleanup

* introduce aws sdk helpoer

* address comments

* nit

* fix rebase

* fix rebase

* set error

* fix build

* better error message
justing-bq pushed a commit that referenced this pull request May 4, 2023
* new secrets_manager files

* working secrets manager client

* add cache for secrets manager proxy

* first unit test for secrets manager proxy

* added more unit tests

* fix unique lock missing template arguments

* fix multiple calls to aws api in secrets manager unit tests

* fix multiple aws init/shutdown api calls in integration tests

* fix calling init/shutdownApi in windows

* verbose log for windows community tests

* wrap initapi and shutdownapi inside secrets manager proxy contructor/destructor

* move init/shutdown aws api to alloc/free env handle

* move init/shutdown aws api to alloc/free env handle

* test

* test

* fix static variables

* fix atomic int init

* fix unit tests

* renaming

* cleanup

* introduce aws sdk helpoer

* address comments

* nit

* fix rebase

* fix rebase

* set error

* fix build

* better error message
yanw-bq added a commit that referenced this pull request May 17, 2023
* new secrets_manager files

* working secrets manager client

* add cache for secrets manager proxy

* first unit test for secrets manager proxy

* added more unit tests

* fix unique lock missing template arguments

* fix multiple calls to aws api in secrets manager unit tests

* fix multiple aws init/shutdown api calls in integration tests

* fix calling init/shutdownApi in windows

* verbose log for windows community tests

* wrap initapi and shutdownapi inside secrets manager proxy contructor/destructor

* move init/shutdown aws api to alloc/free env handle

* move init/shutdown aws api to alloc/free env handle

* test

* test

* fix static variables

* fix atomic int init

* fix unit tests

* renaming

* cleanup

* introduce aws sdk helpoer

* address comments

* nit

* fix rebase

* fix rebase

* set error

* fix build

* better error message
alexr-bq added a commit that referenced this pull request May 18, 2023
* Add AWS Authentication parameters to DSN UI (#115)

* Refactor MYSQL_PROXY (#118)

* EFM PROXY

* complete EFM PROXY

* fix unit tests

* fix include

* generate node keys after setting next proxy for EFM_PROXY

* fix unit tests

* stop monitoring init() and options()

* replace MYSQL_MONITOR_PROXY with MYSQL_PROXY

* fix unit tests

* stop monitoring ping()

* stop monitoring ssl_set and option4

* stop monitoring autocommit

* comment out aws code

* stop monitoring client_find_plugin

* newline at eof

* free monitor connection before getting a new one

* fix unit tests

* test

* disable failover and monitoring for monitor thread

* fix memory leak in unit test

* needs connection handler for monitor

* add null check in monitor service start monitoring

* fix env handle still used after deleting

* comment out mysql_library_init

* move connection handler to its own header file

* Further refactor MYSQL_PROXY (#119)

* create DUMMY_PROXY to handle mysql library calls, refactor MYSQL_PROXY to only forward proxy call

* remove commented include

* include thread header in dummy proxy

* rename mysql_proxy to connection_proxy

* rename dummy proxy to mysql proxy

* remove secrets manager proxy as it should be a different PR

* include in alphabetical order

* AWS SDK helper class (#123)

* Implement IAM Authentication (#120)

* Add Authentication Parameters to ConnectionStringBuilder (#124)

* Secrets Manager Proxy (#121)

* new secrets_manager files

* working secrets manager client

* add cache for secrets manager proxy

* first unit test for secrets manager proxy

* added more unit tests

* fix unique lock missing template arguments

* fix multiple calls to aws api in secrets manager unit tests

* fix multiple aws init/shutdown api calls in integration tests

* fix calling init/shutdownApi in windows

* verbose log for windows community tests

* wrap initapi and shutdownapi inside secrets manager proxy contructor/destructor

* move init/shutdown aws api to alloc/free env handle

* move init/shutdown aws api to alloc/free env handle

* test

* test

* fix static variables

* fix atomic int init

* fix unit tests

* renaming

* cleanup

* introduce aws sdk helpoer

* address comments

* nit

* fix rebase

* fix rebase

* set error

* fix build

* better error message

* Secrets manager integration test (#127)

* create secrets in java util

* simple sm test

* simplify setup

* fix integration

* comment out toxiporxy

* comment out toxiporxy

* sm test not inherit from base anymore

* fix build

* fix set error message, fix dsn not picked up

* add {} in connection string

* fix curly braces

* fix log

* fix build

* more logging

* more logging

* fix test

* better error handling

* uncomment

* address comments

* revert

* IAM Authentication Integration Tests (#126)

* Refactor Integration Tests (#128)

* Fix installer (#129)

* fix minor version exceeding 255, remove lib folder for installer

* remove setup lib from installer

* add aws sdk component in wix

* build aws sdk on release action

* workflow dispatch

* bundle aws sdk when doing cpack on mac and linux

* fix mac installer aws dependencies

* Adjust DSN config UI for AWS Authentication (#130)

* Documentation for AWS Authentication (#131)

* rename windows installer (#132)

* Update license (#133)

* update license

* ignore file change in license

* update license in build script

* Parse Region from Secret if User does not provide Region (#134)

* Fix SQLCancel not going through proxy (#135)

* fix SQLCancel not going through proxy

* cleanup ds during dbc clone

* spacing

* Fix typo in doc

* Implement ENABLE_STRICT_READER_FAILOVER user parameter (#136)

* Override change_user() in IAM/Secrets Manager proxy (#137)

* override change_user() in IAM/Secrets Manager proxy

* better naming

* Fix failover timeout not obeyed (#138)

* replace async future with packed_task and thread to avoid blocking future destructor

* fix unit tests

* add template arguments for std::packaged_task

* missing template arguments

* use thread pool to run failover and wait for all when driver unloads, fix logger

* increase thread pool size

* fix reader failover where failed result overwrite successful result

* not stop thread pool on free env handle

* redirect maven central and stop thread pool in teardown

* maven central

* add thread id to logs

* move future valid check before wait for

* move future valid check before wait_for() for writer failover

* mark logger as extern

* test diable logs

* test failover_integration_test first

* test

* debug gh action

* make env var available for debug session

* debug in docker

* refactor integration tests to use shared pointer for rds client

* make rds client back to non static

* fix build

* reset rds client in teardown

* Revert "refactor integration tests to use shared pointer for rds client"

This reverts commit 2ec1ae5.

* disable remote debug

* fix build

* debug

* specifically call thread pool stop in myodbc_end

* move failover thread pool inside env

* fix unit test

* fix build

* cleanup

* China endpoint support (#140)

* china endpoint support

* nit

* Verify Writer Cluster Connections (#139)

* Fix shadowing member variables causing null pointer exception (#148)

* fix some shadowing member variables casuing null pointer exception

* extend perf tests aws credentials duration from 3 to 4 hours

* workaround mysqlclient

* fix zlib not found

* fix mac build

* bump mysql version

* fix mac build

* bump github action runner macos versiooon

* update download link

* fix build

* install openssl1.1 for macos13

* debug

* debug

* fall back to macos 11

* change macos version to 12

* fix build

* fix build

* Fix monitor timeout (#149)

* fix monitor timeout

* fix login timeout and unit tests

* pass monitor flag

* rename a component that may have name collision issue on mac (#151)

* Remove is_multi_writer flag and check for last updated writer

* Fix tests

* Remove uneeded line

---------

Co-authored-by: justing-bq <62349012+justing-bq@users.noreply.github.com>
Co-authored-by: Yan Wang <68562925+yanw-bq@users.noreply.github.com>
Co-authored-by: justing-bq <justing@bitquilltech.com>
Co-authored-by: Alex Rehnby-Martin <alexr@AlexRsMacbookPro.local>
yanw-bq added a commit that referenced this pull request May 18, 2023
* new secrets_manager files

* working secrets manager client

* add cache for secrets manager proxy

* first unit test for secrets manager proxy

* added more unit tests

* fix unique lock missing template arguments

* fix multiple calls to aws api in secrets manager unit tests

* fix multiple aws init/shutdown api calls in integration tests

* fix calling init/shutdownApi in windows

* verbose log for windows community tests

* wrap initapi and shutdownapi inside secrets manager proxy contructor/destructor

* move init/shutdown aws api to alloc/free env handle

* move init/shutdown aws api to alloc/free env handle

* test

* test

* fix static variables

* fix atomic int init

* fix unit tests

* renaming

* cleanup

* introduce aws sdk helpoer

* address comments

* nit

* fix rebase

* fix rebase

* set error

* fix build

* better error message
yanw-bq added a commit that referenced this pull request May 18, 2023
* Add AWS Authentication parameters to DSN UI (#115)

* Refactor MYSQL_PROXY (#118)

* EFM PROXY

* complete EFM PROXY

* fix unit tests

* fix include

* generate node keys after setting next proxy for EFM_PROXY

* fix unit tests

* stop monitoring init() and options()

* replace MYSQL_MONITOR_PROXY with MYSQL_PROXY

* fix unit tests

* stop monitoring ping()

* stop monitoring ssl_set and option4

* stop monitoring autocommit

* comment out aws code

* stop monitoring client_find_plugin

* newline at eof

* free monitor connection before getting a new one

* fix unit tests

* test

* disable failover and monitoring for monitor thread

* fix memory leak in unit test

* needs connection handler for monitor

* add null check in monitor service start monitoring

* fix env handle still used after deleting

* comment out mysql_library_init

* move connection handler to its own header file

* Further refactor MYSQL_PROXY (#119)

* create DUMMY_PROXY to handle mysql library calls, refactor MYSQL_PROXY to only forward proxy call

* remove commented include

* include thread header in dummy proxy

* rename mysql_proxy to connection_proxy

* rename dummy proxy to mysql proxy

* remove secrets manager proxy as it should be a different PR

* include in alphabetical order

* AWS SDK helper class (#123)

* Implement IAM Authentication (#120)

* Add Authentication Parameters to ConnectionStringBuilder (#124)

* Secrets Manager Proxy (#121)

* new secrets_manager files

* working secrets manager client

* add cache for secrets manager proxy

* first unit test for secrets manager proxy

* added more unit tests

* fix unique lock missing template arguments

* fix multiple calls to aws api in secrets manager unit tests

* fix multiple aws init/shutdown api calls in integration tests

* fix calling init/shutdownApi in windows

* verbose log for windows community tests

* wrap initapi and shutdownapi inside secrets manager proxy contructor/destructor

* move init/shutdown aws api to alloc/free env handle

* move init/shutdown aws api to alloc/free env handle

* test

* test

* fix static variables

* fix atomic int init

* fix unit tests

* renaming

* cleanup

* introduce aws sdk helpoer

* address comments

* nit

* fix rebase

* fix rebase

* set error

* fix build

* better error message

* Secrets manager integration test (#127)

* create secrets in java util

* simple sm test

* simplify setup

* fix integration

* comment out toxiporxy

* comment out toxiporxy

* sm test not inherit from base anymore

* fix build

* fix set error message, fix dsn not picked up

* add {} in connection string

* fix curly braces

* fix log

* fix build

* more logging

* more logging

* fix test

* better error handling

* uncomment

* address comments

* revert

* IAM Authentication Integration Tests (#126)

* Refactor Integration Tests (#128)

* Fix installer (#129)

* fix minor version exceeding 255, remove lib folder for installer

* remove setup lib from installer

* add aws sdk component in wix

* build aws sdk on release action

* workflow dispatch

* bundle aws sdk when doing cpack on mac and linux

* fix mac installer aws dependencies

* Adjust DSN config UI for AWS Authentication (#130)

* Documentation for AWS Authentication (#131)

* rename windows installer (#132)

* Update license (#133)

* update license

* ignore file change in license

* update license in build script

* Parse Region from Secret if User does not provide Region (#134)

* Fix SQLCancel not going through proxy (#135)

* fix SQLCancel not going through proxy

* cleanup ds during dbc clone

* spacing

* Fix typo in doc

* Implement ENABLE_STRICT_READER_FAILOVER user parameter (#136)

* Override change_user() in IAM/Secrets Manager proxy (#137)

* override change_user() in IAM/Secrets Manager proxy

* better naming

* Fix failover timeout not obeyed (#138)

* replace async future with packed_task and thread to avoid blocking future destructor

* fix unit tests

* add template arguments for std::packaged_task

* missing template arguments

* use thread pool to run failover and wait for all when driver unloads, fix logger

* increase thread pool size

* fix reader failover where failed result overwrite successful result

* not stop thread pool on free env handle

* redirect maven central and stop thread pool in teardown

* maven central

* add thread id to logs

* move future valid check before wait for

* move future valid check before wait_for() for writer failover

* mark logger as extern

* test diable logs

* test failover_integration_test first

* test

* debug gh action

* make env var available for debug session

* debug in docker

* refactor integration tests to use shared pointer for rds client

* make rds client back to non static

* fix build

* reset rds client in teardown

* Revert "refactor integration tests to use shared pointer for rds client"

This reverts commit 2ec1ae5.

* disable remote debug

* fix build

* debug

* specifically call thread pool stop in myodbc_end

* move failover thread pool inside env

* fix unit test

* fix build

* cleanup

* China endpoint support (#140)

* china endpoint support

* nit

* Verify Writer Cluster Connections (#139)

* Fix shadowing member variables causing null pointer exception (#148)

* fix some shadowing member variables casuing null pointer exception

* extend perf tests aws credentials duration from 3 to 4 hours

* workaround mysqlclient

* fix zlib not found

* fix mac build

* bump mysql version

* fix mac build

* bump github action runner macos versiooon

* update download link

* fix build

* install openssl1.1 for macos13

* debug

* debug

* fall back to macos 11

* change macos version to 12

* fix build

* fix build

* Fix monitor timeout (#149)

* fix monitor timeout

* fix login timeout and unit tests

* pass monitor flag

* rename a component that may have name collision issue on mac (#151)

* Remove is_multi_writer flag and check for last updated writer

* Fix tests

* Remove uneeded line

---------

Co-authored-by: justing-bq <62349012+justing-bq@users.noreply.github.com>
Co-authored-by: Yan Wang <68562925+yanw-bq@users.noreply.github.com>
Co-authored-by: justing-bq <justing@bitquilltech.com>
Co-authored-by: Alex Rehnby-Martin <alexr@AlexRsMacbookPro.local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants