You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/10/01 20:43:57 UTC
[GitHub] [airflow] ivica-k opened a new pull request #11227: includes the STS token if STS credentials are used
ivica-k opened a new pull request #11227:
URL: https://github.com/apache/airflow/pull/11227
Includes the STS token if temporary STS credentials are used, as per AWS docs https://docs.aws.amazon.com/redshift/latest/dg/copy-parameters-authorization.html#copy-credentials
I'm not sure if I updated the tests in the correct way - they are passing but maybe I should have created tests
for the specific functions I added rather than extending the tests for the `execute` functions. I'll split them if requested.
closes: 9970
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ivica-k commented on pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ivica-k commented on pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#issuecomment-703793262
Does CI need to be triggered again because of cancelled checks? If so, how? :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ivica-k commented on pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ivica-k commented on pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#issuecomment-702683429
> looks like this might be the solution to this issue that I raised?
> #11057
Please see my answer in the mentioned issue; hope it clears things up.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ivica-k commented on pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ivica-k commented on pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#issuecomment-721940724
> Can you please rebase your PR on latest Master since we have applied [Black](https://github.com/apache/airflow/commit/4e8f9cc8d02b29c325b8a5a76b4837671bdf5f68) and [PyUpgrade](https://github.com/apache/airflow/commit/8c42cf1b00c90f0d7f11b8a3a455381de8e003c5) on Master.
>
> It will help if your squash your commits into single commit first so that there are less conflicts.
Just pushed with the rebased code; I hope it looks good. If you have time, could you please check whether the tests I changed are good enough?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ivica-k commented on a change in pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ivica-k commented on a change in pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#discussion_r501733411
##########
File path: airflow/providers/amazon/aws/transfers/s3_to_redshift.py
##########
@@ -90,27 +93,39 @@ def __init__(
self._s3_hook = None
self._postgres_hook = None
+ def _build_copy_query(self, credentials, copy_options):
Review comment:
Will do, thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#issuecomment-721948667
[The Workflow run](https://github.com/apache/airflow/actions/runs/346225680) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle merged pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
feluelle merged pull request #11227:
URL: https://github.com/apache/airflow/pull/11227
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ivica-k commented on a change in pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ivica-k commented on a change in pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#discussion_r509515458
##########
File path: airflow/providers/amazon/aws/utils/redshift.py
##########
@@ -0,0 +1,49 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import logging
+
+from botocore.credentials import ReadOnlyCredentials
+
+log = logging.getLogger(__name__)
+
+
+def build_credentials_block(credentials: ReadOnlyCredentials) -> str:
+ """
+ Generate AWS credentials block for Redshift COPY and UNLOAD
+ commands, as noted in AWS docs
+ https://docs.aws.amazon.com/redshift/latest/dg/copy-parameters-authorization.html#copy-credentials
+
+ :param credentials: ReadOnlyCredentials object from `botocore`
+ :return: str
+ """
+ if credentials.token and credentials.access_key.startswith("ASIA"):
Review comment:
When a principal assumes an IAM role, Security Token Service (STS) hands out a set of values: access key, secret key and a token. These are temporary credentials valid for one hour by default. They are easily recognized as the access key is starting with `ASIA` which is different from regular credentials that are starting with `AKIA`
If a principal wishes to use these credentials then the token must be included along with the access key and secret key. The token does not exist when using regular credentials starting with `AKIA`.
Issue that we encountered when using Airflow is exactly that: Airflow runs under an IAM role but assumes another IAM role to get access to S3 in order to perform the `UNLOAD` command. However, since the token is not included the authentication does not work and role assumption fails, and so does the `UNLOAD` command.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#issuecomment-702386743
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, pylint and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/master/docs/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/master/BREEZE.rst) for testing locally, itโs a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better ๐.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ivica-k commented on a change in pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ivica-k commented on a change in pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#discussion_r502949769
##########
File path: airflow/providers/amazon/aws/transfers/s3_to_redshift.py
##########
@@ -90,27 +93,39 @@ def __init__(
self._s3_hook = None
self._postgres_hook = None
+ def _build_copy_query(self, credentials, copy_options):
Review comment:
Added in the last commit.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ivica-k commented on a change in pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ivica-k commented on a change in pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#discussion_r512942731
##########
File path: tests/providers/amazon/aws/transfers/test_redshift_to_s3.py
##########
@@ -66,24 +70,79 @@ def test_execute(
task_id="task_id",
table_as_file_name=table_as_file_name,
dag=None,
- ).execute(None)
+ )
+
+ op.execute(None)
unload_options = '\n\t\t\t'.join(unload_options)
select_query = "SELECT * FROM {schema}.{table}".format(schema=schema, table=table)
- unload_query = """
- UNLOAD ('{select_query}')
- TO 's3://{s3_bucket}/{s3_key}'
- with credentials
- 'aws_access_key_id={access_key};aws_secret_access_key={secret_key}'
- {unload_options};
- """.format(
- select_query=select_query,
+ credentials_block = build_credentials_block(mock_session.return_value)
+
+ unload_query = op._build_unload_query(
+ credentials_block, select_query, expected_s3_key, unload_options
+ )
Review comment:
Yeah, some hints are definitely needed. I tried literally what you said; made the `_build_unload_query` function return `"GOATS"` and all 4 tests failed
```
AssertionError: assert 'aws_access_key_id' in 'GOATS'
AssertionError: assert 'ASIA_aws_access_key_id' in 'GOATS'
# ... etc
```
As I said, I'm not the greatest when it comes to writing tests. I'd appreciate any pointers once you (or anyone else) gets some time. Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on a change in pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#discussion_r507891865
##########
File path: airflow/providers/amazon/aws/transfers/s3_to_redshift.py
##########
@@ -17,8 +17,10 @@
from typing import List, Optional, Union
+
from airflow.models import BaseOperator
from airflow.providers.amazon.aws.hooks.s3 import S3Hook
+from airflow.providers.amazon.aws.utils.redshift import build_credentials_block
Review comment:
We don't have a `RedshiftHook`. We are using a `PostgresHook` since redshift is based on postgres. And we hadn't really the need yet to create a hook only for redshift specific stuff. :)
https://github.com/apache/airflow/pull/11227#discussion_r501630750
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#discussion_r501173521
##########
File path: airflow/providers/amazon/aws/transfers/s3_to_redshift.py
##########
@@ -90,27 +93,39 @@ def __init__(
self._s3_hook = None
self._postgres_hook = None
+ def _build_copy_query(self, credentials, copy_options):
Review comment:
Is there a common logic that can be worth extracting?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ivica-k commented on a change in pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ivica-k commented on a change in pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#discussion_r500370373
##########
File path: airflow/providers/amazon/aws/transfers/s3_to_redshift.py
##########
@@ -90,27 +93,39 @@ def __init__(
self._s3_hook = None
self._postgres_hook = None
+ def _build_copy_query(self, credentials, copy_options):
Review comment:
One is for the `unload` query while the other one is for the `copy` query.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ivica-k commented on pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ivica-k commented on pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#issuecomment-743794111
Can someone who understands what is going on with the failing `CI Build / Quarantined tests (mysql) (pull_request)` look into it? I don't really see an error message, just exit code 137.
In Docker world that's `Out of memory` according to my googling.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ivica-k commented on a change in pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ivica-k commented on a change in pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#discussion_r499802779
##########
File path: airflow/providers/amazon/aws/transfers/redshift_to_s3.py
##########
@@ -104,29 +107,50 @@ def __init__( # pylint: disable=too-many-arguments
'HEADER',
]
- def execute(self, context):
- postgres_hook = PostgresHook(postgres_conn_id=self.redshift_conn_id)
- s3_hook = S3Hook(aws_conn_id=self.aws_conn_id, verify=self.verify)
+ def _build_unload_query(self, credentials, select_query, s3_key, unload_options):
+ if credentials.token and credentials.access_key.startswith("ASIA"):
+ self.log.debug("STS token found in credentials, including it in the UNLOAD command")
+ # these credentials are obtained from AWS STS
+ # so the token must be included in the CREDENTIALS clause
+ # https://docs.aws.amazon.com/redshift/latest/dg/copy-parameters-authorization.html#copy-credentials
+ credentials_line = (
+ "aws_access_key_id={access_key};aws_secret_access_key={secret_key};token={token}".format(
+ access_key=credentials.access_key,
+ secret_key=credentials.secret_key,
+ token=credentials.token,
+ )
+ )
- credentials = s3_hook.get_credentials()
- unload_options = '\n\t\t\t'.join(self.unload_options)
- s3_key = '{}/{}_'.format(self.s3_key, self.table) if self.table_as_file_name else self.s3_key
- select_query = "SELECT * FROM {schema}.{table}".format(schema=self.schema, table=self.table)
- unload_query = """
+ else:
+ credentials_line = "aws_access_key_id={access_key};aws_secret_access_key={secret_key}".format(
+ access_key=credentials.access_key, secret_key=credentials.secret_key
+ )
+
+ return """
UNLOAD ('{select_query}')
TO 's3://{s3_bucket}/{s3_key}'
with credentials
- 'aws_access_key_id={access_key};aws_secret_access_key={secret_key}'
+ '{credentials_line}'
{unload_options};
""".format(
select_query=select_query,
s3_bucket=self.s3_bucket,
s3_key=s3_key,
- access_key=credentials.access_key,
- secret_key=credentials.secret_key,
+ credentials_line=credentials_line,
unload_options=unload_options,
)
+ def execute(self, context):
+ postgres_hook = PostgresHook(postgres_conn_id=self.redshift_conn_id)
+ s3_hook = S3Hook(aws_conn_id=self.aws_conn_id, verify=self.verify)
+
+ credentials = s3_hook.get_credentials()
+ unload_options = '\n\t\t\t'.join(self.unload_options)
+ s3_key = '{}/{}_'.format(self.s3_key, self.table) if self.table_as_file_name else self.s3_key
+ select_query = "SELECT * FROM {schema}.{table}".format(schema=self.schema, table=self.table)
Review comment:
I personally agree with that. The only reason why I did not use f-strings is because I did not want to introduce too many changes in one go.
I'll push the change shortly, thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#discussion_r511135834
##########
File path: airflow/providers/amazon/aws/utils/redshift.py
##########
@@ -0,0 +1,49 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import logging
+
+from botocore.credentials import ReadOnlyCredentials
+
+log = logging.getLogger(__name__)
+
+
+def build_credentials_block(credentials: ReadOnlyCredentials) -> str:
+ """
+ Generate AWS credentials block for Redshift COPY and UNLOAD
+ commands, as noted in AWS docs
+ https://docs.aws.amazon.com/redshift/latest/dg/copy-parameters-authorization.html#copy-credentials
+
+ :param credentials: ReadOnlyCredentials object from `botocore`
+ :return: str
+ """
+ if credentials.token and credentials.access_key.startswith("ASIA"):
Review comment:
Sorry, I should have been more clear:
Why do we care what the prefix is -- why isn't this `if credentials.token:` -- i.e. if there is a token, use it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ivica-k commented on a change in pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ivica-k commented on a change in pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#discussion_r566371373
##########
File path: tests/providers/amazon/aws/transfers/test_redshift_to_s3.py
##########
@@ -66,24 +70,79 @@ def test_execute(
task_id="task_id",
table_as_file_name=table_as_file_name,
dag=None,
- ).execute(None)
+ )
+
+ op.execute(None)
unload_options = '\n\t\t\t'.join(unload_options)
select_query = "SELECT * FROM {schema}.{table}".format(schema=schema, table=table)
- unload_query = """
- UNLOAD ('{select_query}')
- TO 's3://{s3_bucket}/{s3_key}'
- with credentials
- 'aws_access_key_id={access_key};aws_secret_access_key={secret_key}'
- {unload_options};
- """.format(
- select_query=select_query,
+ credentials_block = build_credentials_block(mock_session.return_value)
+
+ unload_query = op._build_unload_query(
+ credentials_block, select_query, expected_s3_key, unload_options
+ )
Review comment:
Hello @ashb, any hints? The PR may go stale, it's pretty old as it is.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ivica-k commented on pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ivica-k commented on pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#issuecomment-742771296
I don't really understand why the CI failed :/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ivica-k commented on a change in pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ivica-k commented on a change in pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#discussion_r502949719
##########
File path: airflow/providers/amazon/aws/transfers/redshift_to_s3.py
##########
@@ -104,28 +107,42 @@ def __init__( # pylint: disable=too-many-arguments
'HEADER',
]
+ def _build_unload_query(self, credentials, select_query, s3_key, unload_options):
Review comment:
Added in the last commit.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ivica-k commented on a change in pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ivica-k commented on a change in pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#discussion_r500394750
##########
File path: airflow/providers/amazon/aws/transfers/redshift_to_s3.py
##########
@@ -104,28 +107,42 @@ def __init__( # pylint: disable=too-many-arguments
'HEADER',
]
+ def _build_unload_query(self, credentials, select_query, s3_key, unload_options):
Review comment:
I'm a bit confused about the type for `credentials`. My debugger shows it's of type `ReadOnlyCredentials` which in turn is a `NamedTuple`. Which type should I put for `credentials`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] RosterIn commented on pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
RosterIn commented on pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#issuecomment-740880279
@ivica-k rebase was not successful this PR is changing over 2000 files.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#discussion_r511136128
##########
File path: tests/providers/amazon/aws/transfers/test_redshift_to_s3.py
##########
@@ -66,24 +70,79 @@ def test_execute(
task_id="task_id",
table_as_file_name=table_as_file_name,
dag=None,
- ).execute(None)
+ )
+
+ op.execute(None)
unload_options = '\n\t\t\t'.join(unload_options)
select_query = "SELECT * FROM {schema}.{table}".format(schema=schema, table=table)
- unload_query = """
- UNLOAD ('{select_query}')
- TO 's3://{s3_bucket}/{s3_key}'
- with credentials
- 'aws_access_key_id={access_key};aws_secret_access_key={secret_key}'
- {unload_options};
- """.format(
- select_query=select_query,
+ credentials_block = build_credentials_block(mock_session.return_value)
+
+ unload_query = op._build_unload_query(
+ credentials_block, select_query, expected_s3_key, unload_options
+ )
Review comment:
Shout if you need any guidance/help and I'll point you in the right direction (though not until Monday)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on a change in pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#discussion_r501630750
##########
File path: airflow/providers/amazon/aws/transfers/s3_to_redshift.py
##########
@@ -90,27 +93,39 @@ def __init__(
self._s3_hook = None
self._postgres_hook = None
+ def _build_copy_query(self, credentials, copy_options):
Review comment:
I think so but where to put it? We don't have a `RedshiftHook` because most of it can be done via `PostgresHook`. In this case we could add `redshift.py` to `providers.amazon.aws.utils` ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#discussion_r499783147
##########
File path: airflow/providers/amazon/aws/transfers/redshift_to_s3.py
##########
@@ -104,29 +107,50 @@ def __init__( # pylint: disable=too-many-arguments
'HEADER',
]
- def execute(self, context):
- postgres_hook = PostgresHook(postgres_conn_id=self.redshift_conn_id)
- s3_hook = S3Hook(aws_conn_id=self.aws_conn_id, verify=self.verify)
+ def _build_unload_query(self, credentials, select_query, s3_key, unload_options):
+ if credentials.token and credentials.access_key.startswith("ASIA"):
+ self.log.debug("STS token found in credentials, including it in the UNLOAD command")
+ # these credentials are obtained from AWS STS
+ # so the token must be included in the CREDENTIALS clause
+ # https://docs.aws.amazon.com/redshift/latest/dg/copy-parameters-authorization.html#copy-credentials
+ credentials_line = (
+ "aws_access_key_id={access_key};aws_secret_access_key={secret_key};token={token}".format(
+ access_key=credentials.access_key,
+ secret_key=credentials.secret_key,
+ token=credentials.token,
+ )
+ )
- credentials = s3_hook.get_credentials()
- unload_options = '\n\t\t\t'.join(self.unload_options)
- s3_key = '{}/{}_'.format(self.s3_key, self.table) if self.table_as_file_name else self.s3_key
- select_query = "SELECT * FROM {schema}.{table}".format(schema=self.schema, table=self.table)
- unload_query = """
+ else:
+ credentials_line = "aws_access_key_id={access_key};aws_secret_access_key={secret_key}".format(
+ access_key=credentials.access_key, secret_key=credentials.secret_key
+ )
+
+ return """
UNLOAD ('{select_query}')
TO 's3://{s3_bucket}/{s3_key}'
with credentials
- 'aws_access_key_id={access_key};aws_secret_access_key={secret_key}'
+ '{credentials_line}'
{unload_options};
""".format(
select_query=select_query,
s3_bucket=self.s3_bucket,
s3_key=s3_key,
- access_key=credentials.access_key,
- secret_key=credentials.secret_key,
+ credentials_line=credentials_line,
unload_options=unload_options,
)
+ def execute(self, context):
+ postgres_hook = PostgresHook(postgres_conn_id=self.redshift_conn_id)
+ s3_hook = S3Hook(aws_conn_id=self.aws_conn_id, verify=self.verify)
+
+ credentials = s3_hook.get_credentials()
+ unload_options = '\n\t\t\t'.join(self.unload_options)
+ s3_key = '{}/{}_'.format(self.s3_key, self.table) if self.table_as_file_name else self.s3_key
+ select_query = "SELECT * FROM {schema}.{table}".format(schema=self.schema, table=self.table)
Review comment:
This looks good to me but how about using f-string in all those changes? In this way the code will be more readable :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#issuecomment-743911230
> Can someone who understands what is going on with the failing `CI Build / Quarantined tests (mysql) (pull_request)` look into it? I don't really see an error message, just exit code 137.
>
> In Docker world that's `Out of memory` according to my googling.
Yes, it often is. If that is all the failure is we don't let it block us merging things.
(This isn't _great_, and we'll look at it after Airflow 2.0)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#discussion_r500363326
##########
File path: airflow/providers/amazon/aws/transfers/s3_to_redshift.py
##########
@@ -90,27 +93,39 @@ def __init__(
self._s3_hook = None
self._postgres_hook = None
+ def _build_copy_query(self, credentials, copy_options):
Review comment:
Please add type hints
##########
File path: airflow/providers/amazon/aws/transfers/redshift_to_s3.py
##########
@@ -104,28 +107,42 @@ def __init__( # pylint: disable=too-many-arguments
'HEADER',
]
+ def _build_unload_query(self, credentials, select_query, s3_key, unload_options):
Review comment:
Please add type hints
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] TKorr commented on pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
TKorr commented on pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#issuecomment-702418670
looks like this might be the solution to this issue that I raised?
#11057
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#issuecomment-742800903
Looks like infrastructure random failure. Can you please rebase to latest master and force push ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#issuecomment-712020030
/cc @jmcarp as you have a similar/slightly overlapping PR open
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on a change in pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#discussion_r507891865
##########
File path: airflow/providers/amazon/aws/transfers/s3_to_redshift.py
##########
@@ -17,8 +17,10 @@
from typing import List, Optional, Union
+
from airflow.models import BaseOperator
from airflow.providers.amazon.aws.hooks.s3 import S3Hook
+from airflow.providers.amazon.aws.utils.redshift import build_credentials_block
Review comment:
We don't have a `RedshiftHook`. We are using a `PostgresHook` since redshift is based on postgres. And we hadn't really the need yet to create a hook only for redshift specific stuff. :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#discussion_r500363987
##########
File path: airflow/providers/amazon/aws/transfers/s3_to_redshift.py
##########
@@ -90,27 +93,39 @@ def __init__(
self._s3_hook = None
self._postgres_hook = None
+ def _build_copy_query(self, credentials, copy_options):
Review comment:
Btw. do we implement the same method two times?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#discussion_r507643421
##########
File path: airflow/providers/amazon/aws/transfers/s3_to_redshift.py
##########
@@ -17,8 +17,10 @@
from typing import List, Optional, Union
+
from airflow.models import BaseOperator
from airflow.providers.amazon.aws.hooks.s3 import S3Hook
+from airflow.providers.amazon.aws.utils.redshift import build_credentials_block
Review comment:
Please don't create a new file just for this -- add this method on to the Hook instead.
##########
File path: tests/providers/amazon/aws/transfers/test_redshift_to_s3.py
##########
@@ -66,24 +70,79 @@ def test_execute(
task_id="task_id",
table_as_file_name=table_as_file_name,
dag=None,
- ).execute(None)
+ )
+
+ op.execute(None)
unload_options = '\n\t\t\t'.join(unload_options)
select_query = "SELECT * FROM {schema}.{table}".format(schema=schema, table=table)
- unload_query = """
- UNLOAD ('{select_query}')
- TO 's3://{s3_bucket}/{s3_key}'
- with credentials
- 'aws_access_key_id={access_key};aws_secret_access_key={secret_key}'
- {unload_options};
- """.format(
- select_query=select_query,
+ credentials_block = build_credentials_block(mock_session.return_value)
+
+ unload_query = op._build_unload_query(
+ credentials_block, select_query, expected_s3_key, unload_options
+ )
+
+ assert mock_run.call_count == 1
+ assert access_key in unload_query
+ assert secret_key in unload_query
+ assert_equal_ignore_multiple_spaces(self, mock_run.call_args[0][0], unload_query)
+
+ @parameterized.expand(
+ [
+ [True, "key/table_"],
+ [False, "key"],
+ ]
+ )
+ @mock.patch("boto3.session.Session")
+ @mock.patch("airflow.providers.postgres.hooks.postgres.PostgresHook.run")
+ def test_execute_sts_token(
+ self,
+ table_as_file_name,
+ expected_s3_key,
+ mock_run,
+ mock_session,
+ ):
+ access_key = "ASIA_aws_access_key_id"
+ secret_key = "aws_secret_access_key"
+ token = "token"
+ mock_session.return_value = Session(access_key, secret_key, token)
+ mock_session.return_value.access_key = access_key
+ mock_session.return_value.secret_key = secret_key
+ mock_session.return_value.token = token
+ schema = "schema"
+ table = "table"
+ s3_bucket = "bucket"
+ s3_key = "key"
+ unload_options = [
+ 'HEADER',
+ ]
+
+ op = RedshiftToS3Operator(
+ schema=schema,
+ table=table,
s3_bucket=s3_bucket,
- s3_key=expected_s3_key,
- access_key=access_key,
- secret_key=secret_key,
+ s3_key=s3_key,
unload_options=unload_options,
+ include_header=True,
+ redshift_conn_id="redshift_conn_id",
+ aws_conn_id="aws_conn_id",
+ task_id="task_id",
+ table_as_file_name=table_as_file_name,
+ dag=None,
+ )
+
+ op.execute(None)
+
+ unload_options = '\n\t\t\t'.join(unload_options)
+ select_query = "SELECT * FROM {schema}.{table}".format(schema=schema, table=table)
+ credentials_block = build_credentials_block(mock_session.return_value)
+
+ unload_query = op._build_unload_query(
Review comment:
Same problem here -- this just runs the code again, it doesn't actually test what we do in that code.
##########
File path: tests/providers/amazon/aws/transfers/test_redshift_to_s3.py
##########
@@ -66,24 +70,79 @@ def test_execute(
task_id="task_id",
table_as_file_name=table_as_file_name,
dag=None,
- ).execute(None)
+ )
+
+ op.execute(None)
unload_options = '\n\t\t\t'.join(unload_options)
select_query = "SELECT * FROM {schema}.{table}".format(schema=schema, table=table)
- unload_query = """
- UNLOAD ('{select_query}')
- TO 's3://{s3_bucket}/{s3_key}'
- with credentials
- 'aws_access_key_id={access_key};aws_secret_access_key={secret_key}'
- {unload_options};
- """.format(
- select_query=select_query,
+ credentials_block = build_credentials_block(mock_session.return_value)
+
+ unload_query = op._build_unload_query(
+ credentials_block, select_query, expected_s3_key, unload_options
+ )
Review comment:
By calling this method in the tests, you don't actually test the query correctly -- you could change the query to return `GOATS` and the tests will still pass. You need to test against a "fixed" string as the tests were doing before hand.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#issuecomment-721446646
Can you please rebase your PR on latest Master since we have applied [Black](https://github.com/apache/airflow/commit/4e8f9cc8d02b29c325b8a5a76b4837671bdf5f68) and [PyUpgrade](https://github.com/apache/airflow/commit/8c42cf1b00c90f0d7f11b8a3a455381de8e003c5) on Master.
It will help if your squash your commits into single commit first so that there are less conflicts.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ivica-k commented on pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ivica-k commented on pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#issuecomment-739549366
These CI checks will be running forever...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#issuecomment-771711162
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on a change in pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#discussion_r501630750
##########
File path: airflow/providers/amazon/aws/transfers/s3_to_redshift.py
##########
@@ -90,27 +93,39 @@ def __init__(
self._s3_hook = None
self._postgres_hook = None
+ def _build_copy_query(self, credentials, copy_options):
Review comment:
I think so but where to put it? We don't have a `RedshiftHook` because most of it can be done via `PostgresHook`. In this case we could add `redshift.py` to `providers.amazon.aws.utils` ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ivica-k edited a comment on pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ivica-k edited a comment on pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#issuecomment-702683429
> looks like this might be the solution to this issue that I raised?
> #11057
Please see my comment in the mentioned issue; hope it clears things up.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#issuecomment-775032566
Awesome work, congrats on your first merged pull request!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ivica-k commented on pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ivica-k commented on pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#issuecomment-741614580
Thanks @feluelle and @RosterIn, I must have made an error. Will check tonight if I can fix it properly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ivica-k commented on a change in pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ivica-k commented on a change in pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#discussion_r501733411
##########
File path: airflow/providers/amazon/aws/transfers/s3_to_redshift.py
##########
@@ -90,27 +93,39 @@ def __init__(
self._s3_hook = None
self._postgres_hook = None
+ def _build_copy_query(self, credentials, copy_options):
Review comment:
Will do, thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ivica-k commented on a change in pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ivica-k commented on a change in pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#discussion_r541135320
##########
File path: tests/providers/amazon/aws/transfers/test_redshift_to_s3.py
##########
@@ -66,24 +70,79 @@ def test_execute(
task_id="task_id",
table_as_file_name=table_as_file_name,
dag=None,
- ).execute(None)
+ )
+
+ op.execute(None)
unload_options = '\n\t\t\t'.join(unload_options)
select_query = "SELECT * FROM {schema}.{table}".format(schema=schema, table=table)
- unload_query = """
- UNLOAD ('{select_query}')
- TO 's3://{s3_bucket}/{s3_key}'
- with credentials
- 'aws_access_key_id={access_key};aws_secret_access_key={secret_key}'
- {unload_options};
- """.format(
- select_query=select_query,
+ credentials_block = build_credentials_block(mock_session.return_value)
+
+ unload_query = op._build_unload_query(
+ credentials_block, select_query, expected_s3_key, unload_options
+ )
Review comment:
Hey @ashb, could you please look at my previous comment?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#discussion_r507936882
##########
File path: airflow/providers/amazon/aws/transfers/s3_to_redshift.py
##########
@@ -17,8 +17,10 @@
from typing import List, Optional, Union
+
from airflow.models import BaseOperator
from airflow.providers.amazon.aws.hooks.s3 import S3Hook
+from airflow.providers.amazon.aws.utils.redshift import build_credentials_block
Review comment:
Oh yes, good point!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
feluelle commented on pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#issuecomment-740075120
@ivica-k I think there went sth wrong. Can you check again and make sure that you only push your commits / changes, please. Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#issuecomment-771711162
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ivica-k commented on a change in pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ivica-k commented on a change in pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#discussion_r510858007
##########
File path: tests/providers/amazon/aws/transfers/test_redshift_to_s3.py
##########
@@ -66,24 +70,79 @@ def test_execute(
task_id="task_id",
table_as_file_name=table_as_file_name,
dag=None,
- ).execute(None)
+ )
+
+ op.execute(None)
unload_options = '\n\t\t\t'.join(unload_options)
select_query = "SELECT * FROM {schema}.{table}".format(schema=schema, table=table)
- unload_query = """
- UNLOAD ('{select_query}')
- TO 's3://{s3_bucket}/{s3_key}'
- with credentials
- 'aws_access_key_id={access_key};aws_secret_access_key={secret_key}'
- {unload_options};
- """.format(
- select_query=select_query,
+ credentials_block = build_credentials_block(mock_session.return_value)
+
+ unload_query = op._build_unload_query(
+ credentials_block, select_query, expected_s3_key, unload_options
+ )
Review comment:
I'll try to fix it but I'm not the greatest when it comes to testing...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#issuecomment-706750739
[The Workflow run](https://github.com/apache/airflow/actions/runs/300854559) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks$,^Build docs$,^Spell check docs$,^Backport packages$,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ivica-k commented on a change in pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
ivica-k commented on a change in pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#discussion_r566371373
##########
File path: tests/providers/amazon/aws/transfers/test_redshift_to_s3.py
##########
@@ -66,24 +70,79 @@ def test_execute(
task_id="task_id",
table_as_file_name=table_as_file_name,
dag=None,
- ).execute(None)
+ )
+
+ op.execute(None)
unload_options = '\n\t\t\t'.join(unload_options)
select_query = "SELECT * FROM {schema}.{table}".format(schema=schema, table=table)
- unload_query = """
- UNLOAD ('{select_query}')
- TO 's3://{s3_bucket}/{s3_key}'
- with credentials
- 'aws_access_key_id={access_key};aws_secret_access_key={secret_key}'
- {unload_options};
- """.format(
- select_query=select_query,
+ credentials_block = build_credentials_block(mock_session.return_value)
+
+ unload_query = op._build_unload_query(
+ credentials_block, select_query, expected_s3_key, unload_options
+ )
Review comment:
Hello @ashb, any hints?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #11227: includes the STS token if STS credentials are used
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #11227:
URL: https://github.com/apache/airflow/pull/11227#discussion_r501173301
##########
File path: airflow/providers/amazon/aws/transfers/redshift_to_s3.py
##########
@@ -104,28 +107,42 @@ def __init__( # pylint: disable=too-many-arguments
'HEADER',
]
+ def _build_unload_query(self, credentials, select_query, s3_key, unload_options):
Review comment:
I would be in favor of `credentials: ReadOnlyCredentials`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org