impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-6067: Enable s3 access via IAM roles for EC2 VMs
Date Thu, 09 Nov 2017 02:21:00 GMT
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8294 )

Change subject: IMPALA-6067: Enable s3 access via IAM roles for EC2 VMs
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8294/2/bin/impala-config.sh
File bin/impala-config.sh:

http://gerrit.cloudera.org:8080/#/c/8294/2/bin/impala-config.sh@294
PS2, Line 294:   if (set +x; [[ -z ${AWS_ACCESS_KEY_ID-} && -z ${AWS_SECRET_ACCESS_KEY-}
]]); then
> I'm not sure that putting the checks into a separate script would change an
Lines 252-264 seem ok - I don't have a problem with setting environment variables in this
script.


http://gerrit.cloudera.org:8080/#/c/8294/2/bin/impala-config.sh@294
PS2, Line 294:   if (set +x; [[ -z ${AWS_ACCESS_KEY_ID-} && -z ${AWS_SECRET_ACCESS_KEY-}
]]); then
> In most of the use cases the network access is avoided, it happens only if 
Yeah, I'd question whether "aws ls" belongs in this script either.

The performance is one thing but I just generally think we should restrict this script to
doing the minimum possible to set up environment variables. I don't see why it should be extended
to do heavyweight things to validate the configuration - we don't do anything to validate
the vast majority of other variables that are set in this script.

It does appear that people have added validations in an ad-hoc way before so if a majority
of people think that that's a good idea, I can yield to that.

We should also keep in mind that this script is a crappy programming environment, because
it's running in the context of the user's shell and we can't use things like "set -x" and
have to be careful setting variables that we don't intend to leak into the user's shell.

So I think regardless this logic would be more maintainable in a separate script to validate
the AWS config. My preference is that we also run that script from elsewhere to keep impala-config.sh
lightweight but if other people feel strongly that impala-config.sh should be doing more validation
of configs, etc then that's not the worst thing in the world.


http://gerrit.cloudera.org:8080/#/c/8294/2/bin/impala-config.sh@303
PS2, Line 303: CURL_ARGS
This variable will leak out into the user's shell.



-- 
To view, visit http://gerrit.cloudera.org:8080/8294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14cd9d4453a91baad3c379aa7e4944993fca95ae
Gerrit-Change-Number: 8294
Gerrit-PatchSet: 2
Gerrit-Owner: Laszlo Gaal <laszlo.gaal@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple-impala@apache.org>
Gerrit-Reviewer: Joe McDonnell <joemcdonnell@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <laszlo.gaal@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Nov 2017 02:21:00 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message