You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tamas Mate (Code Review)" <ge...@cloudera.org> on 2022/08/05 13:21:28 UTC
[Impala-ASF-CR] IMPALA-11436: Change search bind authentication parameters
Tamas Mate has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18819
Change subject: IMPALA-11436: Change search bind authentication parameters
......................................................................
IMPALA-11436: Change search bind authentication parameters
Impala's search bind authentication intends to mimic Spring's behaviour.
However, the login username and user dn paremeters were swapped for
group searches compared to Spring. This change intends to allign these
parameters.
For user search, Spring uses {0} to replace the login username.
Meanwhile, during group search {0} is used to replace the login user dn
and {1} is used to replace the login username.
Testing:
- Ran LdapSearchBindImpalaShellTest frontend tests
Change-Id: I9808566a348f7c6200b0571fbc05e67f720f2075
---
M be/src/util/ldap-search-bind.cc
M docs/topics/impala_ldap.xml
M fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java
3 files changed, 21 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/18819/1
--
To view, visit http://gerrit.cloudera.org:8080/18819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9808566a348f7c6200b0571fbc05e67f720f2075
Gerrit-Change-Number: 18819
Gerrit-PatchSet: 1
Gerrit-Owner: Tamas Mate <tm...@apache.org>
[Impala-ASF-CR] IMPALA-11436: Change search bind authentication parameters
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/18819 )
Change subject: IMPALA-11436: Change search bind authentication parameters
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18819/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18819/1//COMMIT_MSG@14
PS1, Line 14: For user search, Spring uses {0} to replace the login username.
: Meanwhile, during group search {0} is used to replace the login user dn
: and {1} is used to replace the login username.
> Yes, unfortunately this can break running systems, but the identification o
I am ok with not adding more complexity to the code and warn about this in the release notes.
Can you also update the documentation?
https://github.com/apache/impala/blob/master/docs/topics/impala_ldap.xml#L478
I am not sure whether we usually do it in the same commit or a different one.
--
To view, visit http://gerrit.cloudera.org:8080/18819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9808566a348f7c6200b0571fbc05e67f720f2075
Gerrit-Change-Number: 18819
Gerrit-PatchSet: 2
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Comment-Date: Thu, 11 Aug 2022 12:33:24 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11436: Change search bind authentication parameters
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/18819 )
Change subject: IMPALA-11436: Change search bind authentication parameters
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18819/3/docs/topics/impala_ldap.xml
File docs/topics/impala_ldap.xml:
http://gerrit.cloudera.org:8080/#/c/18819/3/docs/topics/impala_ldap.xml@484
PS3, Line 484: The <codeph>{0}</codeph> and <codeph>{1}</codeph> patterns have been swapped
: between 4.1 and 4.2 releases.
Thank you for the review! No worries, I am happy to make it as clear as possible.
> The description above should contain the new behavior.
> The note should contain that it was swapped, and what was the old behavior.
Done, explained it more clearly with previous and current behaviours.
> The Jira id could be mentioned for people who want more info on topic.
I wanted to avoid adding Jira IDs to the docs and the Jira description is short, so added the key point to this note.
--
To view, visit http://gerrit.cloudera.org:8080/18819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9808566a348f7c6200b0571fbc05e67f720f2075
Gerrit-Change-Number: 18819
Gerrit-PatchSet: 3
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Comment-Date: Fri, 19 Aug 2022 08:47:14 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11436: Change search bind authentication parameters
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/18819 )
Change subject: IMPALA-11436: Change search bind authentication parameters
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18819/3/docs/topics/impala_ldap.xml
File docs/topics/impala_ldap.xml:
http://gerrit.cloudera.org:8080/#/c/18819/3/docs/topics/impala_ldap.xml@484
PS3, Line 484: The <codeph>{0}</codeph> and <codeph>{1}</codeph> patterns have been swapped
: between 4.1 and 4.2 releases.
Sorry for being picky, but can you make it even more clear?
- The description above should contain the new behavior.
- The note should contain that it was swapped, and what was the old behavior.
- The Jira id could be mentioned for people who want more info on topic.
--
To view, visit http://gerrit.cloudera.org:8080/18819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9808566a348f7c6200b0571fbc05e67f720f2075
Gerrit-Change-Number: 18819
Gerrit-PatchSet: 3
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Comment-Date: Thu, 18 Aug 2022 09:31:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11436: Change search bind authentication parameters
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/18819 )
Change subject: IMPALA-11436: Change search bind authentication parameters
......................................................................
Patch Set 1:
(1 comment)
Thank you for the review Csaba, updated the change.
http://gerrit.cloudera.org:8080/#/c/18819/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18819/1//COMMIT_MSG@14
PS1, Line 14: For user search, Spring uses {0} to replace the login username.
: Meanwhile, during group search {0} is used to replace the login user dn
: and {1} is used to replace the login username.
> I am ok with not adding more complexity to the code and warn about this in
Done
--
To view, visit http://gerrit.cloudera.org:8080/18819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9808566a348f7c6200b0571fbc05e67f720f2075
Gerrit-Change-Number: 18819
Gerrit-PatchSet: 1
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Comment-Date: Thu, 18 Aug 2022 08:27:17 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11436: Change search bind authentication parameters
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/18819 )
Change subject: IMPALA-11436: Change search bind authentication parameters
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18819/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18819/1//COMMIT_MSG@11
PS1, Line 11: group searches compared to Spring. This change intends to allign these
> typo
Done
http://gerrit.cloudera.org:8080/#/c/18819/1//COMMIT_MSG@14
PS1, Line 14: For user search, Spring uses {0} to replace the login username.
: Meanwhile, during group search {0} is used to replace the login user dn
: and {1} is used to replace the login username.
> Is this a potentially breaking change? I mean that there can be users who u
Yes, unfortunately this can break running systems, but the identification of the problem and fix is more or less straightforward, authentication will fail immediately with the new version and config change is enough.
The configurations around ldap are already a bit convoluted, maybe we could emphasise this change in the 4.2 release notes?
--
To view, visit http://gerrit.cloudera.org:8080/18819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9808566a348f7c6200b0571fbc05e67f720f2075
Gerrit-Change-Number: 18819
Gerrit-PatchSet: 1
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Comment-Date: Thu, 11 Aug 2022 11:52:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11436: Change search bind authentication parameters
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Hello Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18819
to look at the new patch set (#2).
Change subject: IMPALA-11436: Change search bind authentication parameters
......................................................................
IMPALA-11436: Change search bind authentication parameters
Impala's search bind authentication intends to mimic Spring's behaviour.
However, the login username and user dn paremeters were swapped for
group searches compared to Spring. This change intends to align these
parameters.
For user search, Spring uses {0} to replace the login username.
Meanwhile, during group search {0} is used to replace the login user dn
and {1} is used to replace the login username.
Testing:
- Ran LdapSearchBindImpalaShellTest frontend tests
Change-Id: I9808566a348f7c6200b0571fbc05e67f720f2075
---
M be/src/util/ldap-search-bind.cc
M docs/topics/impala_ldap.xml
M fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java
3 files changed, 21 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/18819/2
--
To view, visit http://gerrit.cloudera.org:8080/18819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9808566a348f7c6200b0571fbc05e67f720f2075
Gerrit-Change-Number: 18819
Gerrit-PatchSet: 2
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-11436: Change search bind authentication parameters
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/18819 )
Change subject: IMPALA-11436: Change search bind authentication parameters
......................................................................
IMPALA-11436: Change search bind authentication parameters
Impala's search bind authentication intends to mimic Spring's behaviour.
However, the login username and user dn paremeters were swapped for
group searches compared to Spring. This change intends to align these
parameters.
For user search, Spring uses {0} to replace the login username.
Meanwhile, during group search {0} is used to replace the login user dn
and {1} is used to replace the login username.
Testing:
- Ran LdapSearchBindImpalaShellTest frontend tests
Change-Id: I9808566a348f7c6200b0571fbc05e67f720f2075
---
M be/src/util/ldap-search-bind.cc
M docs/topics/impala_ldap.xml
M fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java
3 files changed, 30 insertions(+), 23 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/18819/5
--
To view, visit http://gerrit.cloudera.org:8080/18819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9808566a348f7c6200b0571fbc05e67f720f2075
Gerrit-Change-Number: 18819
Gerrit-PatchSet: 5
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
[Impala-ASF-CR] IMPALA-11436: Change search bind authentication parameters
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/18819 )
Change subject: IMPALA-11436: Change search bind authentication parameters
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18819/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18819/1//COMMIT_MSG@11
PS1, Line 11: group searches compared to Spring. This change intends to allign these
typo
http://gerrit.cloudera.org:8080/#/c/18819/1//COMMIT_MSG@14
PS1, Line 14: For user search, Spring uses {0} to replace the login username.
: Meanwhile, during group search {0} is used to replace the login user dn
: and {1} is used to replace the login username.
Is this a potentially breaking change? I mean that there can be users who use the existing pattern. If this is a possibility, then I would prefer to add a flag to switch between the 2 modes of operation.
--
To view, visit http://gerrit.cloudera.org:8080/18819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9808566a348f7c6200b0571fbc05e67f720f2075
Gerrit-Change-Number: 18819
Gerrit-PatchSet: 1
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 Aug 2022 17:22:22 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11436: Change search bind authentication parameters
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18819 )
Change subject: IMPALA-11436: Change search bind authentication parameters
......................................................................
IMPALA-11436: Change search bind authentication parameters
Impala's search bind authentication intends to mimic Spring's behaviour.
However, the login username and user dn paremeters were swapped for
group searches compared to Spring. This change intends to align these
parameters.
For user search, Spring uses {0} to replace the login username.
Meanwhile, during group search {0} is used to replace the login user dn
and {1} is used to replace the login username.
Testing:
- Ran LdapSearchBindImpalaShellTest frontend tests
Change-Id: I9808566a348f7c6200b0571fbc05e67f720f2075
Reviewed-on: http://gerrit.cloudera.org:8080/18819
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/util/ldap-search-bind.cc
M docs/topics/impala_ldap.xml
M fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java
3 files changed, 30 insertions(+), 23 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/18819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I9808566a348f7c6200b0571fbc05e67f720f2075
Gerrit-Change-Number: 18819
Gerrit-PatchSet: 6
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
[Impala-ASF-CR] IMPALA-11436: Change search bind authentication parameters
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/18819 )
Change subject: IMPALA-11436: Change search bind authentication parameters
......................................................................
IMPALA-11436: Change search bind authentication parameters
Impala's search bind authentication intends to mimic Spring's behaviour.
However, the login username and user dn paremeters were swapped for
group searches compared to Spring. This change intends to align these
parameters.
For user search, Spring uses {0} to replace the login username.
Meanwhile, during group search {0} is used to replace the login user dn
and {1} is used to replace the login username.
Testing:
- Ran LdapSearchBindImpalaShellTest frontend tests
Change-Id: I9808566a348f7c6200b0571fbc05e67f720f2075
---
M be/src/util/ldap-search-bind.cc
M docs/topics/impala_ldap.xml
M fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java
3 files changed, 25 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/18819/3
--
To view, visit http://gerrit.cloudera.org:8080/18819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9808566a348f7c6200b0571fbc05e67f720f2075
Gerrit-Change-Number: 18819
Gerrit-PatchSet: 3
Gerrit-Owner: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>