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

Add environment variable to override endpoint_url (#2099) #2746

Closed
wants to merge 7 commits into from

Conversation

rwillmer
Copy link

1st time contributing to this project, let me know if I need to change anything.

@ozbillwang
Copy link

ozbillwang commented Jan 31, 2021

Thanks to raise this PR. I am waiting for this featuer as well (#1375 #2099)

But I have a question.

In latest localstack, it has an improved feature to combine all endpoint url with one port (it used to be different ports for different services). So with your change, it supposes the port is always same for all services.

But will this be always the case?

If in the future, someone need support with different ports, looks this PR hardcode without different port, and it would be hard to update to support different ports for different services.

@hobthross
Copy link

I agree @ozbillwang that your suggestion would be an improvement.
However since the PR as it stands fixes my use-case, I'm going to leave it as-is, since I don't know my way around the boto3 code well enough to implement it.
(More than happy for someone else to take my PR and build on it)

@luminescent
Copy link

I am also interested in this PR - it would be very useful for localstack.

@NicolasLeCorre
Copy link

We really need this feature, in conjunction with the AWS_CONFIG_FILE and AWS_SHARED_CREDENTIALS_FILE env vars it would allow us to change the cloud backend outside of the code.

but this pr has almost 3 months and no reviewer... is there a problem including it ?

@rwillmer
Copy link
Author

Should I ask someone to review it? Not sure what the process is to get a PR into core

@luminescent
Copy link

luminescent commented Apr 16, 2021 via email

@luminescent
Copy link

If anyone else needs a workaround for localstack while waiting for the PR, I have one using a reverse proxy that intercepts the calls made to the default AWS endpoints and redirects them to localstack on 4566. This means no code changes are required in the lambdas.

https://github.com/luminescent/localstack-reverse-proxy

boto3/session.py Outdated
Comment on lines 259 to 262
try:
endpoint_url = os.environ["AWS_ENDPOINT_URL"]
except KeyError:
pass

Choose a reason for hiding this comment

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

This could do a .get() which returns None if the env var is not set.

Suggested change
try:
endpoint_url = os.environ["AWS_ENDPOINT_URL"]
except KeyError:
pass
endpoint_url = os.environ.get("AWS_ENDPOINT_URL")

Copy link

Choose a reason for hiding this comment

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

Are we able to make this change and get the PR merged?

@CodePint
Copy link

CodePint commented May 5, 2021

+1 this would be incredibly use for me

@rwillmer
Copy link
Author

rwillmer commented Jul 5, 2021

Any chance of getting this merged?

@mojokb
Copy link

mojokb commented Jul 16, 2021

+1 this would be incredibly use for me

@chrisroat
Copy link

This is a very nice, minimally invasive solution to a very common problem. Please do consider it.

@CodePint
Copy link

CodePint commented Aug 25, 2021

@rwillmer are you able to make the changes requested in the code review?
@AidanDelaney I believe this is the reason that this has not been approved?

If you do not have time or I don't hear back, I am happy to branch off and make the requested changes.

@CodePint
Copy link

CodePint commented Aug 25, 2021

For anyone looking for an interim solution to use with PynamoDB
This is the approach I have been using, it is by no means a good one but it allowed me to continue working

import os

from botocore.session import Session
from pynamodb.models import Model


class BaseModel(Model):
    class Meta:
        if os.environ['APP_ENV'] in ['local', 'test']:
            host = os.environ['AWS_ENDPOINT_URL']
            region = os.environ['AWS_DEFAULT_REGION']
        else:
            region = Session().get_config_variable('region')

@eyalengel-pagaya
Copy link

watchig.
can't wait for this to be merged

@rwillmer
Copy link
Author

@AidanDelaney @CodePint I've made the suggested change

@AidanDelaney
Copy link

Looks like we need to get the attention of one of the maintainers to see what they think of this contribution. I beleive this PR fixes issue #2099 that was assigned to @kdaily. Maybe they would like to comment and/or review?

@aroxby-honor
Copy link

The last two people to merge pull requests were @nateprewitt and @joguSD. Maybe they can offer some guidance on this?

@jbvsmo
Copy link

jbvsmo commented Oct 15, 2021

Bumping the issue. It would be greatly appreciated to have this merged.

@rwillmer
Copy link
Author

I'm the original author of the PR and AFAIK I've made the suggested change.
It's a code change of a few lines, with tests; I cannot understand why it is so hard to get this PR merged.

@aroxby-honor
Copy link

@nateprewitt Have you seen this?

@aroxby-honor
Copy link

@kdaily Have you seen this?

@jaypan13
Copy link

+1 for this! Really needed! Today I ran into same issue when I needed to invoke SAM lambda and SAM API simultaneously. As all lambdas are using boto3, it will be beneficial instead of changing the code.

@rwillmer
Copy link
Author

I'm the OP. I'm seeing 2 messages:

  • "2 workflows awaiting approval - First-time contributors need a maintainer to approve running workflows."
  • "This branch has no conflicts with the base branch - Only those with write access can merge pull requests"

I don't have access to request any more reviews - so could someone, who does, ask a maintainer to approve this? assuming that the 2 existing reviewers aren't maintainers

I don't have write access to this repo, so cannot merge this PR. Could someone, who does, merge this?

In the meantime, I'll try to keep my fork up-to-date with the current boto3/develop branch, in the hope that this would be useful to folk.

@pappacena
Copy link

Just to clarify, this doesn't cover cases where we use different local components, right?

If we need to set, for example, an endpoint for a local DynamoDB and another for minio at the same time, we would still need to implement a customized call to boto3.client.

client._endpoint.host == ENDPOINT_URL,
"AWS_ENDPOINT_URL env var not used when set",
)
del os.environ["AWS_ENDPOINT_URL"]
Copy link

@pappacena pappacena Dec 28, 2021

Choose a reason for hiding this comment

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

If AWS_ENDPOINT_URL is actually set on the environment before this test, del os.environ["AWS_ENDPOINT_URL"] could mess up with it for other tests.

Maybe a setUp method like this would be better, to restore the os.environ to its original state after each test run?

    def restore_dict(self, dictionary: dict, original_data: dict):
        dictionary.clear()
        dictionary.update(original_data)

    def setUp(self):
        original_env = os.environ.copy()
        self.addCleanup(lambda: self.restore_dict(os.environ, original_env))

kabirkhan added a commit to kabirkhan/cloudpathlib that referenced this pull request Jan 25, 2022
…ery useful for localstack while waiting for upstream PR from boto3: boto/boto3#2746
pjbull pushed a commit to drivendataorg/cloudpathlib that referenced this pull request Jan 26, 2022
)

* Add ability to set endpoint_url from AWS_ENDPOINT_URL env variable. Very useful for localstack while waiting for upstream PR from boto3: boto/boto3#2746

* add test

* rm print

* lint
pjbull added a commit to drivendataorg/cloudpathlib that referenced this pull request Jan 26, 2022
) (#195)

* Add ability to set endpoint_url from AWS_ENDPOINT_URL env variable. Very useful for localstack while waiting for upstream PR from boto3: boto/boto3#2746

* add test

* rm print

* lint

Co-authored-by: Kabir Khan <kabirkhan1137@gmail.com>
@reddec
Copy link

reddec commented Feb 5, 2022

I am literally confused - so simple and so wanted fix was not yet merged. Do not want to press on maintainers, but mates, if you have no time to merge, maybe give some others active contributors write access? Or just enable something like auto-merge (anyone can merge PR as soon as approved and CI checks passed)

@CodePint
Copy link

And...i'm running into this again with localstack and S3 buckets.
I will be bumping this PR and tagging AWS devs until this either gets merged or rejected.

@nateprewitt @kellertk @shepazon

@CodePint
Copy link

CodePint commented Apr 19, 2022

After looking at the code more, there is a strong argument that this change should be integrated with botocore config for the sake of consistency with other session variables. boto3 session.client is just a wrapper around the much more complex botocore session.create_client.

Would it be worth trying to ping the botocore maintainers and working at this from both sides?
@rwillmer @nateprewitt @kellertk @shepazon

botocore references:
https://github.com/boto/botocore/blob/10ebb46bbd87789ad3fe90d55c30cd8bb095d270/botocore/configprovider.py#L49
https://github.com/boto/botocore/blob/10ebb46bbd87789ad3fe90d55c30cd8bb095d270/botocore/session.py#L277
https://github.com/boto/botocore/blob/10ebb46bbd87789ad3fe90d55c30cd8bb095d270/botocore/session.py#L314
https://github.com/boto/botocore/blob/10ebb46bbd87789ad3fe90d55c30cd8bb095d270/botocore/session.py#L757

With this being said, the PR here is very lightweight and would be of great use as is.
It would still be good to get it merged as a stepping stone whilst the botocore implementation is investigated.

@CodePint
Copy link

Implementation of this PR using a boto3 session subclass, will leave this here in case it's useful to anyone else:

import os

from boto3.session import Session


class BotoSession(Session):
    def client(self, *args, **kwargs):
        if kwargs.get('endpoint_url') is None:
            kwargs['endpoint_url'] = os.environ.get("AWS_ENDPOINT_URL")
        return super().client(*args, **kwargs)


# s3_client = BotoSession().client('s3')

@rwillmer
Copy link
Author

I'm the OP. I offered this 2-line change 15 months ago.

Zero interest from the maintainers in taking a PR, which included tests.

I've given up expecting that this change will be added to boto/boto3.

@kapilt
Copy link

kapilt commented Apr 19, 2022 via email

@rwillmer
Copy link
Author

What account manager is that? I'm a freelancer, offering a wee bit of code that I thought would be useful, with tests.

It's turned out to be impossible to get this tiny PR merged.

@reddec
Copy link

reddec commented Apr 20, 2022

Is it possible that this PR is not merged just because it allows developers to use other S3-like storage?
In that case, there is strong a conflict of interest between AWS services and the community. If I am right, this PR will never be merged in foreseeable future.

Maintainers, please tell me that I am wrong.

@CodePint
Copy link

CodePint commented Apr 20, 2022

Is it possible that this PR is not merged just because it allows developers to use other S3-like storage? In that case, there is strong a conflict of interest between AWS services and the community. If I am right, this PR will never be merged in foreseeable future.

Maintainers, please tell me that I am wrong.

I think this was already addressed in a different issue (can't seem to find it currently).

Basically there are use cases within the AWS official ecosystem where an AWS_ENDPOINT_URL env var is useful/required. The functionality is already exposed via the endpoint_url parameter which can be passed to the boto3 client function, the ability to configure it using an env var is purely an extension of that.

@CodePint
Copy link

CodePint commented Apr 20, 2022

Fwiw, The more effective way to lobby for this change is via your account managers and reference this issue. Likely the change needs to be coordinated across other language sdks for consistent usage.

What account manager is that? I'm a freelancer, offering a wee bit of code that I thought would be useful, with tests.

It's turned out to be impossible to get this tiny PR merged.

This is a good idea, I will see If I can raise this through our AWS support channels.

@CodePint
Copy link

Likely the change needs to be coordinated across other language sdks for consistent usage.

Then this needs to be communicated by the maintainers, so we can stop wasting time here and focus on the other sdks

@kellertk
Copy link
Contributor

Thank you for posting your feedback here, and our apologies that we’ve been thinking this over for a long time without much forward motion. There are similar requests to implement this feature in a few of the AWS SDKs and the AWS CLI, so in order to coordinate those teams - and hopefully make the discussions a little easier to follow - we’ve created a new issue in aws/aws-sdk here: aws/aws-sdk#229

@boto boto locked and limited conversation to collaborators Apr 26, 2022
@tim-finnigan
Copy link
Contributor

Hi all,

We recently added a pull request (aws/aws-sdk#230) that contains a proposal based on community comments and suggestions and our own discussions. This document proposes to extend the options for configuring the endpoint to allow users to provide an endpoint URL independently for each AWS service via an environment variable or a profile subsection in the shared configuration file.

You can read the proposal here.

For more information on how to give feedback, please see this comment on the aws/aws-sdk repository:

aws/aws-sdk#229 (comment)

Thanks!

@kdaily
Copy link
Member

kdaily commented Jul 7, 2023

I'm happy to announce that the ability to configure the endpoint URL via the shared configuration file and environment variables is now available in the AWS Python SDK! You can now specify the endpoint to use for all service requests through the shared configuration file and environment variables, as well as specify the endpoint URL for individual AWS services.

To start using this feature, install boto3>=1.28.0.

To read more about this feature, see the documentation page "Service-specific Endpoints" in the AWS SDKs and Tools Reference Guide:

https://docs.aws.amazon.com/sdkref/latest/guide/feature-ss-endpoints.html

Look forward to a blog post demonstrating the use of this feature on the AWS Developer Tools Blog!

@kdaily kdaily closed this Jul 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet