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

adding support for AWS ELB. Accept IPs separated by comma. #117

Merged
merged 4 commits into from
Jan 10, 2019
Merged

adding support for AWS ELB. Accept IPs separated by comma. #117

merged 4 commits into from
Jan 10, 2019

Conversation

rrpadilla
Copy link
Contributor

  1. Accept IPs separated by comma.
    • Probably In a project like Laravel you need to add this config to the .env file and array is not supported in .env
TRUSTEDPROXY_PROXIES='192.168.1.1, 192.168.1.2'
'proxies' => env('TRUSTEDPROXY_PROXIES', null),
  1. Accept headers as a string.
    • Probably In a project like Laravel you need to add this config to the .env file and you can pass the header you want to use as a string:
TRUSTEDPROXY_HEADERS='HEADER_X_FORWARDED_AWS_ELB'
'headers' => env('TRUSTEDPROXY_HEADERS', 'HEADER_X_FORWARDED_ALL'),

@rrpadilla
Copy link
Contributor Author

Hi,
once approved I can submit a new PR to Laravel referencing these changes and including the config file. For more info see this PR laravel/laravel#4799

@rodruiz
Copy link
Contributor

rodruiz commented Dec 10, 2018

Hi @fideloper,
could you review this PR?

Thanks.

@rrpadilla
Copy link
Contributor Author

Hi @fideloper,
do you have time to review this PR?

Thanks.

@fideloper
Copy link
Owner

I will eventually look into this, yes! My first pass at it looks good, I like supporting it as something coming from the .env file as well.

If you have time, could you add a test or two to ensure it's reading text-based configurations and converting it correctly?

@rrpadilla
Copy link
Contributor Author

Hi @fideloper,
yes, I can add the test to ensure text-based configurations are working fine.

Thanks.

@rrpadilla
Copy link
Contributor Author

Hi @fideloper,
test has been added and everything is GREEN!! :)

Could you confirm?

Thanks.

@fideloper
Copy link
Owner

Thanks! I'm getting a 2nd pair of eyes on it to make sure I didn't miss anything since it will end up in Laravel installations by default.

@rodruiz
Copy link
Contributor

rodruiz commented Jan 4, 2019

Hi @fideloper,
did you have time to review this?

Thanks.

@fideloper fideloper merged commit f8b1a68 into fideloper:master Jan 10, 2019
@fideloper
Copy link
Owner

Pulled it in, thanks!

@rrpadilla it sounded like you wanted to see if you could get a PR into Laravel, correct?

Do you want to do that before I make a release out of this? I'll likely make it 4.1 (current is 4.0.0) since I don't believe it's a breaking change.

@rrpadilla
Copy link
Contributor Author

@fideloper I like that idea.
I can submit a PR to Laravel.
Let you know when done.
Thanks.

@rrpadilla
Copy link
Contributor Author

@fideloper You can check Laravel PR here

Thanks.

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.

None yet

4 participants