8000 NoPrivateNetworkHttpClient decorator by hallboav · Pull Request #35 · nicolas-grekas/symfony · GitHub
[go: up one dir, main page]

Skip to content

NoPrivateNetworkHttpClient decorator #35

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

Conversation

hallboav
Copy link
Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR [WIP]

Bringing symfony/symfony's branch master and my commit...

@hallboav hallboav requested a review from sroze as a code owner January 12, 2020 06:28
@hallboav hallboav force-pushed the no-private-network-http-client-decorator branch from 233e90d to b9159af Compare January 13, 2020 03:23
@nicolas-grekas
Copy link
Owner

Can you please rebase on the latest version of master? There is some git history issue here apparently :)

@hallboav hallboav force-pushed the no-private-network-http-client-decorator branch from b9159af to e727470 Compare January 14, 2020 17:24
@hallboav
Copy link
Author

Can you please rebase on the latest version of master? There is some git history issue here apparently :)

Done.

@hallboav hallboav force-pushed the no-private-network-http-client-decorator branch from e727470 to aa3f856 Compare January 17, 2020 03:50
duncan3dc and others added 22 commits January 17, 2020 17:27
… a double variable...) when accessing the cache panel (@see symfony#35419)
Make sure the `$cache->release()` method exists before executing it.
This PR was merged into the 4.4 branch.

Discussion
----------

[HttpKernel] Check if lock can be released

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix symfony#35421
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

[HttpKernel] Make sure the `$cache->release()` method exists before executing it

Commits
-------

2001e54 [HttpKernel] Check if lock can be released
nicolas-grekas and others added 21 commits January 31, 2020 10:56
* 4.4:
  Bump phpunit-bridge cache
* 5.0:
  Bump phpunit-bridge cache
…rverBundle (jrushlow)

This PR was squashed before being merged into the 5.1-dev branch (closes symfony#35538).

Discussion
----------

[FrameworkBundle] fixed suggesting deprecated WebServerBundle

| Q             | A
| ------------- | ---
| Branch?       | 5.0
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix symfony#35495
| License       | MIT
| Doc PR        |

Removed suggestion to install `symfony/web-server-bundle` in console missing packages suggestions. The web server bundle was deprecated and no longer works with Symfony 5.x, .

Commits
-------

134129b [FrameworkBundle] fixed suggesting deprecated WebServerBundle
…or interface and implementation on reflection (joelwurtz, Korbeil)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[PropertyInfo] Add accessor and mutator extractor interface and implementation on reflection

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony#30248, partially: symfony#22190, symfony#18016, symfony#5013, symfony#9336, symfony#5219,
| License       | MIT
| Doc PR        | TODO

This PR brings accessor / mutator extraction on the PropertyInfo component,

There is no link to existing code, as IMO it should be in another PR as this will add a dependency on property access to the property info component and not sure this is something wanted (although, it will reduce a lot of code base on the property access component as a lot of code seems to be duplicated)

Code is extracted from symfony#30248 also there is some new features (that can be removed if not wanted)

 * Allow extracting private accessor / mutator (will do a new PR that improve private extraction on reflection latter)
 * Allow extracting static accessor / mutators
 * Allow extracting constructor mutators

Current implementation try to be as close as the PropertyAccess implementation and i did not reuse some methods already available in the class as there is some differences in implementation, but maybe it will be a good time to make this consistent (Looking forward to your input) ?

Things that should be done in a new PR:

 * Linking property info to property access to remove a lot of duplicate code
 * Add a new system that allow adding Virtual Property based on this extractor

Commits
-------

0a92dab Rebase, fix tests, review & update CHANGELOG
fc25086 [PropertyInfo] Add accessor and mutator extractor interface and implementation on reflection
* 3.4:
  Update PR template
* 4.4:
  Update PR template
  bumped Symfony version to 4.4.5
  updated VERSION for 4.4.4
  updated CHANGELOG for 4.4.4
* 5.0:
  Update PR template
  bumped Symfony version to 5.0.5
  updated VERSION for 5.0.4
  updated CHANGELOG for 5.0.4
  bumped Symfony version to 4.4.5
  updated VERSION for 4.4.4
  updated CHANGELOG for 4.4.4
* 5.0:
  [Bridge/PhpUnit] dont conflict with phpunit 4.8
…e mocking http client (Koc)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[HttpClient] Allow pass array of callable to the mocking http client

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | not yet

For the now MockHttpClient allows pass closure as response factory. It useful for tests to perform assertions that expected request was sent. But If we are sending multiple sequental requests then it became a little bit tricky to perform assertions:

```php
<?php

$requestIndex = 0;
$expectedRequest = function ($method, $url, $options) use (&$requestIndex) {
    switch (++$requestIndex) {
        case 1:
            $this->assertSame('GET', $method);
            $this->assertSame('https://example.com/api/v1/customer', $url);

            return new MockResponse(CustomerFixture::CUSTOMER_RESPONSE);

        case 2:
            $this->assertSame('POST', $method);
            $this->assertSame('https://example.com/api/v1/customer/1/products', $url);
            $this->assertJsonStringEqualsJsonFile(CustomerFixture::CUSTOMER_PRODUCT_PAYLOAD, $options['json']);

            return new MockResponse();

        default:
            throw new \InvalidArgumentException('Too much requests');
    }
};

$client = new MockHttpClient($expectedRequest);
static::$container->set('http_client.example', $client);

$commandTester->execute(['--since' => '2019-01-01 00:05:00', '--until' => '2019-01-01 00:35:00']);

$this->assertSame(2, $requestIndex, 'All expected requests was sent.');
```

This PR introduces possibility to define multiple callable response factories and `getSentRequestsCount` method to make sure that each factory was called:

```php
<?php

$expectedRequests = [
    function ($method, $url, $options) {
        $this->assertSame('GET', $method);
        $this->assertSame('https://example.com/api/v1/customer', $url);

        return new MockResponse(CustomerFixture::CUSTOMER_RESPONSE);
    },
    function ($method, $url, $options) {
        $this->assertSame('POST', $method);
        $this->assertSame('https://example.com/api/v1/customer/1/products', $url);
        $this->assertJsonStringEqualsJsonFile(CustomerFixture::CUSTOMER_PRODUCT_PAYLOAD, $options['json']);

        return new MockResponse();
    },
];

$client = new MockHttpClient($expectedRequest);
static::$container->set('http_client.example', $client);

$commandTester->execute(['--since' => '2019-01-01 00:05:00', '--until' => '2019-01-01 00:35:00']);

$this->assertSame(2, $client->getSentRequestsCount(), 'All expected requests was sent.');
```

Also it adds a lot of tests.

Commits
-------

a36797d Allow pass array of callable to the mocking http client
Copy link
Owner
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

LGTM, here are some remaining comments. I invite you to open the PR on the official repo now. Thanks!

use Psr\Log\LoggerInterface;
use Symfony\Component\HttpClient\Exception\InvalidArgumentException;
use Symfony\Component\HttpClient\Exception\TransportException;
use Symfony\Component\HttpFoundation\IpUtils;
Copy link
Owner

Choose a reason for hiding this comment

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

we should throw a LogicException when the class is missing, to invite to install http-foundation (see similar exceptions in the source code - look for "composer require")

Copy link
Author

Choose a reason for hiding this comment

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

I did what CachingHttpClient does, except that I removed version information from exception string, because IpUtils was introduced on version 2.2. Is that OK or should I use ^4.4?

Copy link
Owner

Choose a reason for hiding this comment

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

CachingHttpClient takes a class from HttpKernel as argument so it cannot be used without the component, that's why there is no check there.

Copy link
Author

Choose a reason for hiding this comment

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

<?php
public function __construct(HttpClientInterface $client, $subnets = null)
{
    if (!class_exists(IpUtils::class)) {
        throw new \LogicException(sprintf('Using "%s" requires that the HttpFoundation component is installed, try running "composer require symfony/http-foundation".', __CLASS__));
    }

    // ...
}```

Copy link
Owner

Choose a reason for hiding this comment

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

maybe this would be more consistent with existing errors:
'You can not use "%s" if the HttpFoundation component is not installed. Try running "composer require symfony/http-foundation".'

use Symfony\Contracts\HttpClient\ResponseStreamInterface;

/**
* Decorator that block requests to private network by default.
Copy link
Owner

Choose a reason for hiding this comment

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

Decorator that blocks requests to private networks by default.

];

/**
* @var HttpClientInterface
Copy link
Owner

Choose a reason for hiding this comment

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

should be removed - already explicit in the signature of the constructor

/**
* Subnets in CIDR notation that IpUtils will make use of.
*
* @var string|array|null
Copy link
Owner

Choose a reason for hiding this comment

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

docblock can be removed also

private $subnets;

/**
* Constructor.
Copy link
Owner

Choose a reason for hiding this comment

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

should be removed

/**
* Constructor.
*
* @param HttpClientInterface $client A HttpClientInterface instance.
Copy link
Owner

Choose a reason for hiding this comment

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

should be removed as everything is already in the signature

(null !== $subnets && IpUtils::checkIp($info['primary_ip'], $subnets)) ||
(null === $subnets && (
(0 < substr_count($info['primary_ip'], '.') && IpUtils::checkIp($info['primary_ip'], self::IPV4_PRIVATE_SUBNETS)) ||
(0 < substr_count($info['primary_ip'], ':') && IpUtils::checkIp($info['primary_ip'], self::IPV6_PRIVATE_SUBNETS))
Copy link
Owner

Choose a reason for hiding this comment

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

shouldn't we merge both IPV4/V6 constants in one since IpUtils::checkIp() is able to deal with both?
this would reduce the logic to
IpUtils::checkIp($info['primary_ip'], $subnets ?? self::PRIVATE_SUBNETS)

Copy link
Author

Choose a reason for hiding this comment

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

Implementation will be more simpler, but we'll lose performance by checking IPv4 on IPv6 subnets (that will return false, which is expected), but still are unnecessary checks, is that OK?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, I think that's OK, the check is fast compared to the network anyway.

(0 < substr_count($info['primary_ip'], ':') && IpUtils::checkIp($info['primary_ip'], self::IPV6_PRIVATE_SUBNETS))
))
) {
throw new TransportException(sprintf('IP "%s" is blacklisted.', $info['primary_ip']));
Copy link
Owner

Choose a reason for hiding this comment

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

we could give the url also:
sprintf('IP "%s" is blacklisted for "%s".', $info['primary_ip'], $info['url'])

$subnets = $this->subnets;

$lastPrimaryIp = '';
$onProgress = $options['on_progress'] ?? null;
Copy link
Owner

Choose a reason for hiding this comment

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

doing this should allow more accurate error reporting when the progress callback is broken:

if (!\is_callable($options['on_progress'] ?? 'var_dump')) {
    return $this->client->request($method, $url, $options);
}

Copy link
Author
@hallboav hallboav Feb 2, 2020

Choose a reason for hiding this comment

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

Sorry, Nicolas, I didn't get this part. If we return when user pass non-callable on on_progress, then we'll never parse his IP. You mean use var_dump by default if on_progress is non-callable or is missing? Then we can securelly call $onProgress because it will always is a callable?

Copy link
Owner

Choose a reason for hiding this comment

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

If the option is non-null but is not a callable, and error will be raised so there is nothing to do.

$lastPrimaryIp = $info['primary_ip'];
}

\is_callable($onProgress) && $onProgress($dlNow, $dlSize, $info);
Copy link
Owner

Choose a reason for hiding this comment

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

with the previoulsy suggested check, this should be reduced to:
null !== $onProgress && $onProgress($dlNow, $dlSize, $info);

@hallboav hallboav force-pushed the no-private-network-http-client-decorator branch from aa3f856 to c5147dd Compare February 3, 2020 01:08
…as been collected (nicolas-grekas)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[HttpClient] dont display any content when none has been collected

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Commits
-------

36536c9 [HttpClient] dont display any content when none has been collected
@hallboav hallboav force-pushed the no-private-network-http-client-decorator branch from c5147dd to e48b6d5 Compare February 3, 2020 13:33
@nicolas-grekas nicolas-grekas force-pushed the no-private-network-http-client-decorator branch from e48b6d5 to 63fec80 Compare February 3, 2020 16:39
@nicolas-grekas nicolas-grekas merged commit 63fec80 into nicolas-grekas:master Feb 5, 2020
nicolas-grekas pushed a commit that referenced this pull request Mar 7, 2024
…hen publishing a message. (jwage)

This PR was squashed before being merged into the 6.4 branch.

Discussion
----------

[Messenger] [Amqp] Handle AMQPConnectionException when publishing a message.

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix symfony#36538 Fix symfony#48241
| License       | MIT

If you have a message handler that dispatches messages to another queue, you can encounter `AMQPConnectionException` with the message "Library error: a SSL error occurred" or "a socket error occurred"  depending on if you are using tls or not or if you are running behind a load balancer or not.

You can manually reproduce this issue by dispatching a message where the handler then dispatches another message to a different queue, then go to rabbitmq admin and close the connection manually, then dispatch another message and when the message handler goes to dispatch the other message, you will get this exception:

```
a socket error occurred
#0 /vagrant/vendor/symfony/amqp-messenger/Transport/AmqpTransport.php(60): Symfony\Component\Messenger\Bridge\Amqp\Transport\AmqpSender->send()
#1 /vagrant/vendor/symfony/messenger/Middleware/SendMessageMiddleware.php(62): Symfony\Component\Messenger\Bridge\Amqp\Transport\AmqpTransport->send()
#2 /vagrant/vendor/symfony/messenger/Middleware/FailedMessageProcessingMiddleware.php(34): Symfony\Component\Messenger\Middleware\SendMessageMiddleware->handle()
#3 /vagrant/vendor/symfony/messenger/Middleware/DispatchAfterCurrentBusMiddleware.php(61): Symfony\Component\Messenger\Middleware\FailedMessageProcessingMiddleware->handle()
#4 /vagrant/vendor/symfony/messenger/Middleware/RejectRedeliveredMessageMiddleware.php(41): Symfony\Component\Messenger\Middleware\DispatchAfterCurrentBusMiddleware->handle()
#5 /vagrant/vendor/symfony/messenger/Middleware/AddBusNameStampMiddleware.php(37): Symfony\Component\Messenger\Middleware\RejectRedeliveredMessageMiddleware->handle()
#6 /vagrant/vendor/symfony/messenger/Middleware/TraceableMiddleware.php(40): Symfony\Component\Messenger\Middleware\AddBusNameStampMiddleware->handle()
#7 /vagrant/vendor/symfony/messenger/MessageBus.php(70): Symfony\Component\Messenger\Middleware\TraceableMiddleware->handle()
#8 /vagrant/vendor/symfony/messenger/TraceableMessageBus.php(38): Symfony\Component\Messenger\MessageBus->dispatch()
#9 /vagrant/src/Messenger/MessageBus.php(37): Symfony\Component\Messenger\TraceableMessageBus->dispatch()
#10 /vagrant/vendor/symfony/mailer/Mailer.php(66): App\Messenger\MessageBus->dispatch()
#11 /vagrant/src/Mailer/Mailer.php(83): Symfony\Component\Mailer\Mailer->send()
#12 /vagrant/src/Mailer/Mailer.php(96): App\Mailer\Mailer->send()
#13 /vagrant/src/MessageHandler/Trading/StrategySubscriptionMessageHandler.php(118): App\Mailer\Mailer->sendEmail()
#14 /vagrant/src/MessageHandler/Trading/StrategySubscriptionMessageHandler.php(72): App\MessageHandler\Trading\StrategySubscriptionMessageHandler->handle()
#15 /vagrant/vendor/symfony/messenger/Middleware/HandleMessageMiddleware.php(152): App\MessageHandler\Trading\StrategySubscriptionMessageHandler->__invoke()
#16 /vagrant/vendor/symfony/messenger/Middleware/HandleMessageMiddleware.php(91): Symfony\Component\Messenger\Middleware\HandleMessageMiddleware->callHandler()
#17 /vagrant/vendor/symfony/messenger/Middleware/SendMessageMiddleware.php(71): Symfony\Component\Messenger\Middleware\HandleMessageMiddleware->handle()
#18 /vagrant/vendor/symfony/messenger/Middleware/FailedMessageProcessingMiddleware.php(34): Symfony\Component\Messenger\Middleware\SendMessageMiddleware->handle()
#19 /vagrant/vendor/symfony/messenger/Middleware/DispatchAfterCurrentBusMiddleware.php(68): Symfony\Component\Messenger\Middleware\FailedMessageProcessingMiddleware->handle()
#20 /vagrant/vendor/symfony/messenger/Middleware/RejectRedeliveredMessageMiddleware.php(41): Symfony\Component\Messenger\Middleware\DispatchAfterCurrentBusMiddleware->handle()
#21 /vagrant/vendor/symfony/messenger/Middleware/AddBusNameStampMiddleware.php(37): Symfony\Component\Messenger\Middleware\RejectRedeliveredMessageMiddleware->handle()
#22 /vagrant/vendor/symfony/messenger/Middleware/TraceableMiddleware.php(40): Symfony\Component\Messenger\Middleware\AddBusNameStampMiddleware->handle()
#23 /vagrant/vendor/symfony/messenger/MessageBus.php(70): Symfony\Component\Messenger\Middleware\TraceableMiddleware->handle()
#24 /vagrant/vendor/symfony/messenger/TraceableMessageBus.php(38): Symfony\Component\Messenger\MessageBus->dispatch()
#25 /vagrant/vendor/symfony/messenger/RoutableMessageBus.php(54): Symfony\Component\Messenger\TraceableMessageBus->dispatch()
#26 /vagrant/vendor/symfony/messenger/Worker.php(162): Symfony\Component\Messenger\RoutableMessageBus->dispatch()
#27 /vagrant/vendor/symfony/messenger/Worker.php(109): Symfony\Component\Messenger\Worker->handleMessage()
#28 /vagrant/vendor/symfony/messenger/Command/ConsumeMessagesCommand.php(238): Symfony\Component\Messenger\Worker->run()
#29 /vagrant/vendor/symfony/console/Command/Command.php(326): Symfony\Component\Messenger\Command\ConsumeMessagesCommand->execute()
#30 /vagrant/vendor/symfony/console/Application.php(1096): Symfony\Component\Console\Command\Command->run()
#31 /vagrant/vendor/symfony/framework-bundle/Console/Application.php(126): Symfony\Component\Console\Application->doRunCommand()
#32 /vagrant/vendor/symfony/console/Application.php(324): Symfony\Bundle\FrameworkBundle\Console\Application->doRunCommand()
#33 /vagrant/vendor/symfony/framework-bundle/Console/Application.php(80): Symfony\Component\Console\Application->doRun()
#34 /vagrant/vendor/symfony/console/Application.php(175): Symfony\Bundle\FrameworkBundle\Console\Application->doRun()
#35 /vagrant/vendor/symfony/runtime/Runner/Symfony/ConsoleApplicationRunner.php(49): Symfony\Component\Console\Application->run()
#36 /vagrant/vendor/autoload_runtime.php(29): Symfony\Component\Runtime\Runner\Symfony\ConsoleApplicationRunner->run()
#37 /vagrant/bin/console(11): require_once('...')
#38 {main}
```

TODO:

- [x] Add test for retry logic when publishing messages

Commits
-------

f123370 [Messenger] [Amqp] Handle AMQPConnectionException when publishing a message.
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.

0