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

Added 'region' and 'insecure' flag #84

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

DustinKLo
Copy link

@DustinKLo DustinKLo commented Oct 30, 2020

related issues:

the region and service values are parsed from the endpoint flag but sometimes the url is inconsistent with how the code is expecting it, so the region value is an empty string which returns a 403

level=error msg="Received 403 from AWSAuth, invalidating credentials for retrial"

now the proxy has a optional region flag (ie. -region us-west-2)

the proxy also assumes that your server has the proper SSL certificates and will return a verification error when you don't
i added a optional insecure flag to fix that issue

level=error msg="Get \"https://elastic.example.comt/\": x509: certificate is valid for *.eu-central-1.es.amazonaws.com, not elastic.example.com

…gion (#1)

hard-coded proxy.service value to "es"
added insecure flag and logic to avoid SSL verification
* added region flag to avoid having to parse the endpoint to get the region

hard-coded proxy.service value to "es"
added insecure flag and logic to avoid SSL verification

* cleaning up code, moving insecure logic to main function (TODO: still need to test)

* removed redundant use of insecure flag
@DustinKLo
Copy link
Author

Also im not sure where to bump the version, can you point me where to do that? thanks 👍

@abutaha
Copy link
Owner

abutaha commented Nov 6, 2020

Thank you @DustinKLo. I like the SSL insecure option, however, I'm a bit concerned about having 'region' as a mandatory option and changing the current behavior of the proxy as this change will break many implementations of the proxy across multiple users who have the proxy installed and configured as part of their CI/CD. This change will break it for them and I want to keep backward compatibility within v1.x.

With that said, I've started the development of v2.x which WILL break the backward compatibility. In v2.x I'm moving command line options and parameters to a config file to make it easier to update the settings. I've included insecure ssl and region in the config in v2.0. Please have a look and let me know if you have any ideas.

Thanks,

@DustinKLo
Copy link
Author

@abutaha thanks for the review 👍

i can make the region flag optional and re-add the endpoint parsing logic if the region flag isn't supplied in the command line
this should ensure backwards compatibility I hope 🤞

Let me know what you think, i can continue development if you want to go this direction

@abutaha
Copy link
Owner

abutaha commented Nov 15, 2020

@abutaha thanks for the review

i can make the region flag optional and re-add the endpoint parsing logic if the region flag isn't supplied in the command line
this should ensure backwards compatibility I hope

Let me know what you think, i can continue development if you want to go this direction

Yes please go ahead. Just ensure to pull the latest master.

Thanks,

DustinKLo and others added 3 commits November 15, 2020 11:47
* Added support for assuming a role that has access to ElasticSearch

* Fixed small conflicts

* Added new contributors

Co-authored-by: Joshua Garnett <[email protected]>
Co-authored-by: Dido (Christoph Poelt) <[email protected]>
Co-authored-by: Muslim AbuTaha <[email protected]>
…a region flag) (#4)

made -region flag optional, if not supplied will go through the endpoint parsing logic instead
edited README.md description for region flag
added "assume" description to --help in README.md
@DustinKLo
Copy link
Author

@abutaha thanks for the response
I re-added the endpoint parsing logic and also allows for a optional region flag
please review when you have the time 👍

@pshomov
Copy link

pshomov commented Apr 19, 2021

This seems like something that is ready to be merged but comms ended abruptly. Sure would appreciate having this fixed 👍🏻.Thank you for the great work on this tool 🙏🏻 .

@neuralsandwich
Copy link

I've been experiencing the 403 as well but testing the changes on hysds:master doesn't seem to have addressed the issue.

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.

4 participants