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

PostgresHook: deepcopy connection to avoid mutating connection obj #15412

Merged
merged 1 commit into from
May 2, 2021
Merged

PostgresHook: deepcopy connection to avoid mutating connection obj #15412

merged 1 commit into from
May 2, 2021

Conversation

zhzhang
Copy link
Contributor

@zhzhang zhzhang commented Apr 16, 2021

PostgresHook: deepcopy connection object to avoid unexpected mutation

For AWS IAM based connections, the connection object is mutated by setting the login, password and port properties after the call to self.get_iam_token(...). The connection object's login field is set to "IAM:original-login" because the login value returned has IAM: prefixed.

As a result, any subsequent calls to get_conn() will fail with the AWS client.get_cluster_credentials(...) call complaining that : cannot be part of the DbUser argument.

In general, the values within any connection objects used by this hook should not be subject to mutation by get_conn() calls, and the addition of deepcopy is meant to enforce this.

Copy link
Contributor

@kurtqq kurtqq left a comment

Choose a reason for hiding this comment

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

Nice!

@zhzhang
Copy link
Contributor Author

zhzhang commented Apr 27, 2021

Unable to merge still :( what's the best way to get somebody with write access to have a look at this?

@kurtqq
Copy link
Contributor

kurtqq commented May 2, 2021

you need one of the maintainers to approve
@kaxil @dstandish @XD-DENG @xinbinhuang

@xinbinhuang xinbinhuang merged commit fc845ca into apache:master May 2, 2021
@xinbinhuang
Copy link
Contributor

xinbinhuang commented May 2, 2021

@zhzhang @kurtqq Nice catch! thanks for the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants