From dcc019da4c272722ad43876086bbc99aa9298477 Mon Sep 17 00:00:00 2001 From: Clem Blanco Date: Mon, 3 Oct 2022 17:46:32 +0200 Subject: [PATCH 1/5] Add support for null credentials --- composer.json | 7 +- phpunit.xml => phpunit.xml.dist | 18 ++---- src/Contracts/Client.php | 2 + src/Handler.php | 10 +++ src/Kinesis.php | 46 ++++++++----- tests/InteractsWithKinesis.php | 33 +++++++++- tests/MonologKinesisTest.php | 110 ++++++++++++++++++++++++-------- tests/TestCase.php | 10 +-- 8 files changed, 169 insertions(+), 67 deletions(-) rename phpunit.xml => phpunit.xml.dist (55%) diff --git a/composer.json b/composer.json index f0d24c8..91a1e58 100644 --- a/composer.json +++ b/composer.json @@ -14,12 +14,13 @@ "php": ">=7.2.5", "aws/aws-sdk-php": "^3.155", "illuminate/support": "^6.0|^7.0|^8.0|^9.0", - "monolog/monolog": "^2.0" + "laravel/framework": "8.*", + "monolog/monolog": "^2.0", + "orchestra/testbench": "6.*" }, "require-dev": { "mockery/mockery": "^1.2.0", - "phpunit/phpunit": "^8.0|^9.0", - "orchestra/testbench": "^4.0|^5.0|^6.0|^7.0" + "phpunit/phpunit": "^8.0|^9.0" }, "autoload": { "psr-4": { diff --git a/phpunit.xml b/phpunit.xml.dist similarity index 55% rename from phpunit.xml rename to phpunit.xml.dist index e0cc138..ded477a 100644 --- a/phpunit.xml +++ b/phpunit.xml.dist @@ -1,5 +1,7 @@ - + stopOnFailure="false"> - + tests - - - src - - vendor - tests - - - diff --git a/src/Contracts/Client.php b/src/Contracts/Client.php index 7249b49..3f70b77 100644 --- a/src/Contracts/Client.php +++ b/src/Contracts/Client.php @@ -9,6 +9,8 @@ interface Client { public function configure(array $channelConfig): Client; + public function getAwsConfig(): array; + public function putRecord(array $args = []): \Aws\Result; public function putRecords(array $args = []): \Aws\Result; diff --git a/src/Handler.php b/src/Handler.php index 38792da..5e0282e 100644 --- a/src/Handler.php +++ b/src/Handler.php @@ -66,4 +66,14 @@ public function handleBatch(array $records): void // Fire and forget } } + + /** + * Return the configured Client used to write records. + * + * @return Kinesis + */ + public function getClient(): Kinesis + { + return $this->client; + } } diff --git a/src/Kinesis.php b/src/Kinesis.php index 02f5aca..0503061 100644 --- a/src/Kinesis.php +++ b/src/Kinesis.php @@ -15,6 +15,9 @@ class Kinesis implements Client /** @var Repository */ protected $config; + /** @var array */ + protected $awsConfig = []; + public function __construct(Repository $config) { $this->config = $config; @@ -27,19 +30,6 @@ public function __construct(Repository $config) * @return $this */ public function configure(array $channelConfig): Client - { - $this->kinesis = $this->configureKinesisClient($channelConfig); - - return $this; - } - - /** - * Configure the Kinesis client for logging records. - * - * @param array $channelConfig - * @return KinesisClient - */ - private function configureKinesisClient(array $channelConfig): KinesisClient { $defaultConfig = $this->config->get('services.kinesis'); @@ -49,13 +39,15 @@ private function configureKinesisClient(array $channelConfig): KinesisClient 'http' => Arr::get($channelConfig, 'http', Arr::get($defaultConfig, 'http', [])), ]; - if (Arr::has($channelConfig, ['key', 'secret'])) { + if ($this->configHasCredentials($channelConfig)) { $config['credentials'] = Arr::only($channelConfig, ['key', 'secret', 'token']); - } elseif (Arr::has($defaultConfig, ['key', 'secret'])) { + } elseif ($this->configHasCredentials($defaultConfig)) { $config['credentials'] = Arr::only($defaultConfig, ['key', 'secret', 'token']); } - return new KinesisClient($config); + $this->kinesis = new KinesisClient($this->awsConfig = $config); + + return $this; } /** @@ -79,4 +71,26 @@ public function putRecords(array $args = []): \Aws\Result { return $this->kinesis->putRecords($args); } + + /** + * Return the actual array used to configure the AWS Client. + * + * @return array + */ + public function getAwsConfig(): array + { + return $this->awsConfig; + } + + /** + * Make sure some AWS credentials were provided to the configuration array. + * + * @return bool + */ + private function configHasCredentials(array $config): bool + { + return Arr::has($config, ['key', 'secret']) + && is_string(Arr::get($config, 'key')) + && is_string(Arr::get($config, 'secret')); + } } diff --git a/tests/InteractsWithKinesis.php b/tests/InteractsWithKinesis.php index d4cd096..8fa485e 100644 --- a/tests/InteractsWithKinesis.php +++ b/tests/InteractsWithKinesis.php @@ -3,20 +3,49 @@ namespace PodPoint\MonologKinesis\Tests; use Closure; +use Illuminate\Foundation\Application; use PodPoint\MonologKinesis\Contracts\Client; trait InteractsWithKinesis { - public function mockKinesis(): \Mockery\MockInterface + protected function mockKinesis(): \Mockery\MockInterface { $mock = $this->mock(Client::class); + $mock->shouldReceive('configure')->andReturn($mock); return $mock; } - public function mockKinesisWith(Closure $mock): \Mockery\MockInterface + protected function mockKinesisWith(Closure $mock): \Mockery\MockInterface { return $this->mock(Client::class, $mock); } + + protected function withDefaultCredentials(Application $app): void + { + $app->config->set('services.kinesis.key', 'dummy-key'); + $app->config->set('services.kinesis.secret', 'dummy-secret'); + } + + protected function withNullDefaultCredentials(Application $app): void + { + $app->config->set('services.kinesis.key', null); + $app->config->set('services.kinesis.secret', null); + } + + protected function withoutDefaultCredentials(Application $app): void + { + // ... + } + + protected function assertLoggerChannelAwsConfigEquals(string $channel, array $expected): void + { + $kinesisHandler = collect(logger()->channel($channel)->getHandlers())->first(); + + $this->assertInstanceOf(Handler::class, $kinesisHandler); + + /** @var \PodPoint\MonologKinesis\Handler $kinesisHandler */ + $this->assertEquals($expected, $kinesisHandler->getClient()->getAwsConfig()); + } } diff --git a/tests/MonologKinesisTest.php b/tests/MonologKinesisTest.php index f98da23..268200c 100644 --- a/tests/MonologKinesisTest.php +++ b/tests/MonologKinesisTest.php @@ -9,6 +9,81 @@ class MonologKinesisTest extends TestCase { use InteractsWithKinesis; + /** @define-env withDefaultCredentials */ + public function test_aws_credentials_can_be_configured_at_the_service_level() + { + $kinesisHandler = collect(logger()->channel('some_channel')->getHandlers())->first(); + + $this->assertInstanceOf(\PodPoint\MonologKinesis\Handler::class, $kinesisHandler); + + /** @var \PodPoint\MonologKinesis\Handler $kinesisHandler */ + $this->assertEquals([ + 'region' => 'eu-west-1', + 'version' => 'latest', + 'http' => [], + 'credentials' => [ + 'key' => 'dummy-key', + 'secret' => 'dummy-secret', + ], + ], $kinesisHandler->getClient()->getAwsConfig()); + } + + /** @define-env withoutDefaultCredentials */ + public function test_aws_credentials_can_be_completely_optional() + { + $kinesisHandler = collect(logger()->channel('some_channel')->getHandlers())->first(); + + $this->assertInstanceOf(\PodPoint\MonologKinesis\Handler::class, $kinesisHandler); + + /** @var \PodPoint\MonologKinesis\Handler $kinesisHandler */ + $this->assertEquals([ + 'region' => 'eu-west-1', + 'version' => 'latest', + 'http' => [], + ], $kinesisHandler->getClient()->getAwsConfig()); + } + + /** @define-env withNullDefaultCredentials */ + public function test_aws_credentials_can_be_null() + { + $kinesisHandler = collect(logger()->channel('some_channel')->getHandlers())->first(); + + $this->assertInstanceOf(\PodPoint\MonologKinesis\Handler::class, $kinesisHandler); + + /** @var \PodPoint\MonologKinesis\Handler $kinesisHandler */ + $this->assertEquals([ + 'region' => 'eu-west-1', + 'version' => 'latest', + 'http' => [], + ], $kinesisHandler->getClient()->getAwsConfig()); + } + + /** @define-env withDefaultCredentials */ + public function test_aws_credentials_can_be_configured_at_the_channel_level_overriding_default_credentials() + { + $this->mockKinesisWith(function ($mock) { + $mock->shouldReceive('configure')->once()->with(m::on(function ($argument) { + return $argument['key'] === 'some_other_key' + && $argument['secret'] === 'some_other_secret' + && $argument['region'] === 'another_region'; + }))->andReturn($mock); + + $mock->shouldReceive('putRecord')->once(); + }); + + config()->set('logging.default', 'another_channel'); + config()->set('logging.channels.another_channel', [ + 'driver' => 'kinesis', + 'key' => 'some_other_key', + 'secret' => 'some_other_secret', + 'region' => 'another_region', + 'stream' => 'logging', + 'level' => 'debug', + ]); + + logger()->warning('Something went wrong.'); + } + /** * @return array */ @@ -26,7 +101,10 @@ public function loggerLevelTestProvider(): array ]; } - /** @dataProvider loggerLevelTestProvider $logLevel */ + /** + * @dataProvider loggerLevelTestProvider + * @define-env withoutDefaultCredentials + */ public function test_standard_log_levels_are_supported($logLevel) { $this->mockKinesis() @@ -41,6 +119,7 @@ public function test_standard_log_levels_are_supported($logLevel) logger()->$logLevel("Test {$logLevel} message"); } + /** @define-env withoutDefaultCredentials */ public function test_driver_does_not_log_below_default_log_level() { config()->set('logging.channels.some_channel.level', 'warning'); @@ -51,6 +130,7 @@ public function test_driver_does_not_log_below_default_log_level() logger()->debug('Test debug message'); } + /** @define-env withoutDefaultCredentials */ public function test_driver_does_log_greater_than_or_equal_to_default_log_level() { config()->set('logging.channels.some_channel.level', 'warning'); @@ -61,6 +141,7 @@ public function test_driver_does_log_greater_than_or_equal_to_default_log_level( logger()->error('Test error message'); } + /** @define-env withoutDefaultCredentials */ public function test_data_pushed_to_kinesis_is_properly_formatted() { $this->mockKinesis()->shouldReceive('putRecord')->once()->with(m::on(function ($argument) { @@ -77,6 +158,7 @@ public function test_data_pushed_to_kinesis_is_properly_formatted() logger()->info('Test info message'); } + /** @define-env withoutDefaultCredentials */ public function test_data_pushed_to_kinesis_can_also_forward_some_context() { $this->mockKinesis()->shouldReceive('putRecord')->once()->with(m::on(function ($argument) { @@ -88,31 +170,7 @@ public function test_data_pushed_to_kinesis_can_also_forward_some_context() logger()->info('Test info message', ['some_context' => ['key' => 'value']]); } - public function test_channel_specific_aws_credentials_can_be_given() - { - $this->mockKinesisWith(function ($mock) { - $mock->shouldReceive('configure')->once()->with(m::on(function ($argument) { - return $argument['key'] === 'some_other_key' - && $argument['secret'] === 'some_other_secret' - && $argument['region'] === 'another_region'; - }))->andReturn($mock); - - $mock->shouldReceive('putRecord')->once(); - }); - - config()->set('logging.default', 'another_channel'); - config()->set('logging.channels.another_channel', [ - 'driver' => 'kinesis', - 'key' => 'some_other_key', - 'secret' => 'some_other_secret', - 'region' => 'another_region', - 'stream' => 'logging', - 'level' => 'debug', - ]); - - logger()->warning('Something went wrong.'); - } - + /** @define-env withoutDefaultCredentials */ public function test_channel_specific_http_client_options_can_be_given() { $this->mockKinesisWith(function ($mock) { diff --git a/tests/TestCase.php b/tests/TestCase.php index 002015e..7b6150f 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -29,15 +29,11 @@ protected function getPackageProviders($app): array */ protected function defineEnvironment($app): void { - $app['config']->set('services.kinesis', [ - 'key' => 'dummy-key', - 'secret' => 'dummy-secret', - 'region' => 'eu-west-1', - ]); + $app->config->set('services.kinesis.region', 'eu-west-1'); - $app['config']->set('logging.default', 'some_channel'); + $app->config->set('logging.default', 'some_channel'); - $app['config']->set('logging.channels.some_channel', [ + $app->config->set('logging.channels.some_channel', [ 'driver' => 'kinesis', 'stream' => 'logging', 'level' => 'debug', From 8f165f3e35d7b493d9d984ab23d6985132be1966 Mon Sep 17 00:00:00 2001 From: Clem Blanco Date: Mon, 3 Oct 2022 17:47:54 +0200 Subject: [PATCH 2/5] Revert changes on package.json --- composer.json | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/composer.json b/composer.json index 91a1e58..f0d24c8 100644 --- a/composer.json +++ b/composer.json @@ -14,13 +14,12 @@ "php": ">=7.2.5", "aws/aws-sdk-php": "^3.155", "illuminate/support": "^6.0|^7.0|^8.0|^9.0", - "laravel/framework": "8.*", - "monolog/monolog": "^2.0", - "orchestra/testbench": "6.*" + "monolog/monolog": "^2.0" }, "require-dev": { "mockery/mockery": "^1.2.0", - "phpunit/phpunit": "^8.0|^9.0" + "phpunit/phpunit": "^8.0|^9.0", + "orchestra/testbench": "^4.0|^5.0|^6.0|^7.0" }, "autoload": { "psr-4": { From 9266935ce65915e2e9dc755b89fab277821d1c40 Mon Sep 17 00:00:00 2001 From: Clem Blanco Date: Wed, 5 Oct 2022 09:42:22 +0200 Subject: [PATCH 3/5] WIP --- src/LoggerFactory.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/LoggerFactory.php b/src/LoggerFactory.php index 33b79e3..9e91180 100644 --- a/src/LoggerFactory.php +++ b/src/LoggerFactory.php @@ -49,6 +49,6 @@ public function create(array $config): Logger */ private function createKinesisFormatter(): Formatter { - return new Formatter($this->app['config']->get('app.name'), $this->app->environment()); + return new Formatter($this->app->config->get('app.name'), $this->app->environment()); } } From 3fc6109e7c63134ccb551a9e687bd9b5094ce71b Mon Sep 17 00:00:00 2001 From: Clem Blanco Date: Wed, 5 Oct 2022 14:23:15 +0200 Subject: [PATCH 4/5] WIP --- tests/InteractsWithKinesis.php | 10 ---------- tests/MonologKinesisTest.php | 6 +++--- 2 files changed, 3 insertions(+), 13 deletions(-) diff --git a/tests/InteractsWithKinesis.php b/tests/InteractsWithKinesis.php index 8fa485e..f4ae4db 100644 --- a/tests/InteractsWithKinesis.php +++ b/tests/InteractsWithKinesis.php @@ -38,14 +38,4 @@ protected function withoutDefaultCredentials(Application $app): void { // ... } - - protected function assertLoggerChannelAwsConfigEquals(string $channel, array $expected): void - { - $kinesisHandler = collect(logger()->channel($channel)->getHandlers())->first(); - - $this->assertInstanceOf(Handler::class, $kinesisHandler); - - /** @var \PodPoint\MonologKinesis\Handler $kinesisHandler */ - $this->assertEquals($expected, $kinesisHandler->getClient()->getAwsConfig()); - } } diff --git a/tests/MonologKinesisTest.php b/tests/MonologKinesisTest.php index 04a8cc7..14c6696 100644 --- a/tests/MonologKinesisTest.php +++ b/tests/MonologKinesisTest.php @@ -64,8 +64,8 @@ public function test_aws_credentials_can_be_configured_at_the_channel_level_over $this->mockKinesisWith(function ($mock) { $mock->shouldReceive('configure')->once()->with(m::on(function ($argument) { return $argument['key'] === 'some_other_key' - && $argument['secret'] === 'some_other_secret' - && $argument['region'] === 'another_region'; + && $argument['secret'] === 'some_other_secret' + && $argument['region'] === 'another_region'; }))->andReturn($mock); $mock->shouldReceive('putRecord')->once(); @@ -179,7 +179,7 @@ public function test_data_pushed_to_kinesis_can_be_formatted_using_a_custom_form { $this->mockKinesis()->shouldReceive('putRecord')->once()->with(m::on(function ($argument) { return Arr::has($argument, 'Data.custom_message') - && Arr::get($argument, 'Data.custom_message') === 'Test info message'; + && Arr::get($argument, 'Data.custom_message') === 'Test info message'; })); config()->set('logging.channels.some_channel.formatter', DummyCustomFormatter::class); From f3e8399c83be503a91783ce9c98535ba01553562 Mon Sep 17 00:00:00 2001 From: Clem Blanco Date: Wed, 5 Oct 2022 14:24:57 +0200 Subject: [PATCH 5/5] WIP --- tests/MonologKinesisTest.php | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/MonologKinesisTest.php b/tests/MonologKinesisTest.php index 14c6696..de1f0c2 100644 --- a/tests/MonologKinesisTest.php +++ b/tests/MonologKinesisTest.php @@ -141,18 +141,6 @@ public function test_driver_does_log_greater_than_or_equal_to_default_log_level( logger()->error('Test error message'); } - /** @define-env withoutDefaultCredentials */ - public function test_data_pushed_to_kinesis_can_also_forward_some_context() - { - $this->mockKinesis()->shouldReceive('putRecord')->once()->with(m::on(function ($argument) { - $data = json_decode($argument['Data'], true); - - return $data['context'] === ['some_context' => ['key' => 'value']]; - })); - - logger()->info('Test info message', ['some_context' => ['key' => 'value']]); - } - /** @define-env withoutDefaultCredentials */ public function test_data_pushed_to_kinesis_is_properly_formatted_using_the_default_formatter() { @@ -200,6 +188,18 @@ public function test_a_custom_formatter_has_to_implement_the_required_interface_ $this->assertStringContainsString('Unable to create configured logger', $logfile); } + /** @define-env withoutDefaultCredentials */ + public function test_data_pushed_to_kinesis_can_also_forward_some_context() + { + $this->mockKinesis()->shouldReceive('putRecord')->once()->with(m::on(function ($argument) { + $data = json_decode($argument['Data'], true); + + return $data['context'] === ['some_context' => ['key' => 'value']]; + })); + + logger()->info('Test info message', ['some_context' => ['key' => 'value']]); + } + /** @define-env withoutDefaultCredentials */ public function test_channel_specific_http_client_options_can_be_given() {