You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Attila Bukor (Code Review)" <ge...@cloudera.org> on 2022/11/23 11:41:39 UTC

[kudu-CR] KUDU-3423 Add support for Ranger KMS HA

Hello Zoltan Chovan, Alexey Serbin,

I'd like you to do a code review. Please visit

    http://gerrit.cloudera.org:8080/19271

to review the following change.


Change subject: KUDU-3423 Add support for Ranger KMS HA
......................................................................

KUDU-3423 Add support for Ranger KMS HA

Ranger KMS supports high availability by listing multiple Ranger KMS
serves. Up until now, Kudu allowed configuring only one Ranger KMS
server.

This patch adds support for listing multiple KMS servers. This is done
by extending EasyCurl to support multiple URLs. Kudu always attempts to
connect to the first one in the list, and if the connection fails due to
a network error or a timeout, it fails over to the next one. If the
server responds with an error, that is returned immediately and not
tried on the remaining URLs.

Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8
---
M src/kudu/fs/fs_manager.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/ranger-kms/ranger_kms_client.cc
M src/kudu/ranger-kms/ranger_kms_client.h
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
6 files changed, 87 insertions(+), 11 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/19271/1
-- 
To view, visit http://gerrit.cloudera.org:8080/19271
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8
Gerrit-Change-Number: 19271
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>

[kudu-CR] KUDU-3423 Add support for Ranger KMS HA

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Chovan, Alexey Serbin, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/19271

to look at the new patch set (#5).

Change subject: KUDU-3423 Add support for Ranger KMS HA
......................................................................

KUDU-3423 Add support for Ranger KMS HA

Ranger KMS supports high availability by listing multiple Ranger KMS
servers (comma-separated, no spaces). Up until now, Kudu allowed
configuring only one Ranger KMS server.

This patch adds support for listing multiple KMS servers. This is done
by extending EasyCurl to support multiple URLs. Kudu always attempts to
connect to the first one in the list, and if the connection fails due to
a network error or a timeout, it fails over to the next one. If a server
responds with an error, that is considered a valid response and then it
is returned immediately to the caller without trying the remaining URLs.

Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8
---
M src/kudu/fs/fs_manager.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/ranger-kms/ranger_kms_client.cc
M src/kudu/ranger-kms/ranger_kms_client.h
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
6 files changed, 101 insertions(+), 12 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/19271/5
-- 
To view, visit http://gerrit.cloudera.org:8080/19271
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8
Gerrit-Change-Number: 19271
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>

[kudu-CR] KUDU-3423 Add support for Ranger KMS HA

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19271 )

Change subject: KUDU-3423 Add support for Ranger KMS HA
......................................................................


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/19271/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19271/4//COMMIT_MSG@16
PS4, Line 16: If a server
            : responds with an error
> Not sure if it's able to, I thought this is the least opinionated approach.
It's up to you.  Yes, I agree it can be added later, if needed.


http://gerrit.cloudera.org:8080/#/c/19271/5/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

http://gerrit.cloudera.org:8080/#/c/19271/5/src/kudu/fs/fs_manager.cc@149
PS5, Line 149:     if (FLAGS_ranger_kms_url.empty() || FLAGS_encryption_cluster_key_name.empty()) {
This line seems to be too long


http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/util/curl_util.h
File src/kudu/util/curl_util.h:

PS4: 
> Thought about that, but we don't have positive tests for curl-util at all r
Yup, we do have some scenarios using the embedded webserver in webserver-test.cc.  Adding those in a separate changelist looks good to me as well.  It's up to you, I just thought that logically separated pieces with test coverage might be good to have.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8
Gerrit-Change-Number: 19271
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Nov 2022 19:30:15 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3423 Add support for Ranger KMS HA

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/19271 )

Change subject: KUDU-3423 Add support for Ranger KMS HA
......................................................................


Patch Set 6:

(2 comments)

Sorry for the delay Alexey, replied to your comments.

http://gerrit.cloudera.org:8080/#/c/19271/5/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

http://gerrit.cloudera.org:8080/#/c/19271/5/src/kudu/fs/fs_manager.cc@149
PS5, Line 149:     if (FLAGS_ranger_kms_url.empty() ||
> This line seems to be too long
Done


http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/util/curl_util.h
File src/kudu/util/curl_util.h:

PS4: 
> Yup, we do have some scenarios using the embedded webserver in webserver-te
Yea that makes sense. OTOH it feels weird to add a test case to webserver-test that is testing curl itself instead of the webserver.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8
Gerrit-Change-Number: 19271
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Jan 2023 15:36:15 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3423 Add support for Ranger KMS HA

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Chovan, Alexey Serbin, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/19271

to look at the new patch set (#3).

Change subject: KUDU-3423 Add support for Ranger KMS HA
......................................................................

KUDU-3423 Add support for Ranger KMS HA

Ranger KMS supports high availability by listing multiple Ranger KMS
servers (comma-separated, no spaces). Up until now, Kudu allowed
configuring only one Ranger KMS server.

This patch adds support for listing multiple KMS servers. This is done
by extending EasyCurl to support multiple URLs. Kudu always attempts to
connect to the first one in the list, and if the connection fails due to
a network error or a timeout, it fails over to the next one. If a server
responds with an error, that is considered a valid response and it is
returned immediately to the caller without trying the remaining URLs.

Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8
---
M src/kudu/fs/fs_manager.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/ranger-kms/ranger_kms_client.cc
M src/kudu/ranger-kms/ranger_kms_client.h
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
6 files changed, 97 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/19271/3
-- 
To view, visit http://gerrit.cloudera.org:8080/19271
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8
Gerrit-Change-Number: 19271
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>

[kudu-CR] KUDU-3423 Add support for Ranger KMS HA

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Chovan, Alexey Serbin, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/19271

to look at the new patch set (#6).

Change subject: KUDU-3423 Add support for Ranger KMS HA
......................................................................

KUDU-3423 Add support for Ranger KMS HA

Ranger KMS supports high availability by listing multiple Ranger KMS
servers (comma-separated, no spaces). Up until now, Kudu allowed
configuring only one Ranger KMS server.

This patch adds support for listing multiple KMS servers. This is done
by extending EasyCurl to support multiple URLs. Kudu always attempts to
connect to the first one in the list, and if the connection fails due to
a network error or a timeout, it fails over to the next one. If a server
responds with an error, that is considered a valid response and then it
is returned immediately to the caller without trying the remaining URLs.

Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8
---
M src/kudu/fs/fs_manager.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/ranger-kms/ranger_kms_client.cc
M src/kudu/ranger-kms/ranger_kms_client.h
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
6 files changed, 104 insertions(+), 12 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/19271/6
-- 
To view, visit http://gerrit.cloudera.org:8080/19271
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8
Gerrit-Change-Number: 19271
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>

[kudu-CR] KUDU-3423 Add support for Ranger KMS HA

Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/19271 )

Change subject: KUDU-3423 Add support for Ranger KMS HA
......................................................................


Patch Set 4: Code-Review+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8
Gerrit-Change-Number: 19271
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Nov 2022 16:53:35 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3423 Add support for Ranger KMS HA

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Chovan, Alexey Serbin, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/19271

to look at the new patch set (#2).

Change subject: KUDU-3423 Add support for Ranger KMS HA
......................................................................

KUDU-3423 Add support for Ranger KMS HA

Ranger KMS supports high availability by listing multiple Ranger KMS
serves. Up until now, Kudu allowed configuring only one Ranger KMS
server.

This patch adds support for listing multiple KMS servers. This is done
by extending EasyCurl to support multiple URLs. Kudu always attempts to
connect to the first one in the list, and if the connection fails due to
a network error or a timeout, it fails over to the next one. If the
server responds with an error, that is returned immediately and not
tried on the remaining URLs.

Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8
---
M src/kudu/fs/fs_manager.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/ranger-kms/ranger_kms_client.cc
M src/kudu/ranger-kms/ranger_kms_client.h
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
6 files changed, 88 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/19271/2
-- 
To view, visit http://gerrit.cloudera.org:8080/19271
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8
Gerrit-Change-Number: 19271
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>

[kudu-CR] KUDU-3423 Add support for Ranger KMS HA

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/19271 )

Change subject: KUDU-3423 Add support for Ranger KMS HA
......................................................................


Patch Set 4:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/19271/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19271/4//COMMIT_MSG@16
PS4, Line 16: If a server
            : responds with an error
> Is Ranges KMS able to respond with "503 Service Unavailable" or something l
Not sure if it's able to, I thought this is the least opinionated approach. If it's not enough, we can always change it later.


http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/fs/fs_manager.cc@141
PS4, Line 141: " "
> nit: could drop the space and join two parts of the string?
Done


http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/fs/fs_manager.cc@149
PS4, Line 149: FLAGS_ranger_kms_url.empty()
> Since now a list of comma-separated strings can be provided as value for --
Done


http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/util/curl_util.h
File src/kudu/util/curl_util.h:

PS4: 
> Consider separating this newly added {fetch,post}-with-failover functionali
Thought about that, but we don't have positive tests for curl-util at all right now. Do you think it's worth adding tests using our webserver somehow?


http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/util/curl_util.h@61
PS4, Line 61:   // one in the list.
> Maybe, also mention that it stops at the first successful fetch attempt and
Done


http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/util/curl_util.h@78
PS4, Line 78:   Status PostToURL(const std::vector<std::string>& urls,
> ditto: maybe mention what happens on the fist successful post
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8
Gerrit-Change-Number: 19271
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Nov 2022 17:03:58 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3423 Add support for Ranger KMS HA

Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/19271 )

Change subject: KUDU-3423 Add support for Ranger KMS HA
......................................................................


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/19271/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19271/2//COMMIT_MSG@10
PS2, Line 10: serves
nit: servers
Also, maybe add a note about the format of the list (comma separated, no spaces) here?


http://gerrit.cloudera.org:8080/#/c/19271/2//COMMIT_MSG@16
PS2, Line 16: If the
            : server responds with an error
I think this needs to be cleared up a bit. Are you referring to the first server that responds with an error? Are we giving up if the error is not network related/timeout?


http://gerrit.cloudera.org:8080/#/c/19271/2/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

http://gerrit.cloudera.org:8080/#/c/19271/2/src/kudu/fs/fs_manager.cc@140
PS2, Line 140: URLSs
nit: URLs


http://gerrit.cloudera.org:8080/#/c/19271/2/src/kudu/util/curl_util.h
File src/kudu/util/curl_util.h:

http://gerrit.cloudera.org:8080/#/c/19271/2/src/kudu/util/curl_util.h@59
PS2, Line 59:   Status FetchURL(const std::vector<std::string>& urls,
maybe add a note to the comments for the multiple url variants, here and below



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8
Gerrit-Change-Number: 19271
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Nov 2022 14:45:19 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3423 Add support for Ranger KMS HA

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/19271 )

Change subject: KUDU-3423 Add support for Ranger KMS HA
......................................................................


Patch Set 4: Verified+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8
Gerrit-Change-Number: 19271
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Nov 2022 17:41:50 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3423 Add support for Ranger KMS HA

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/19271 )

Change subject: KUDU-3423 Add support for Ranger KMS HA
......................................................................


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/19271/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19271/2//COMMIT_MSG@10
PS2, Line 10: server
> nit: servers
Done


http://gerrit.cloudera.org:8080/#/c/19271/2//COMMIT_MSG@16
PS2, Line 16: If a server
            : responds with an error, that 
> I think this needs to be cleared up a bit. Are you referring to the first s
Added a note about it.


http://gerrit.cloudera.org:8080/#/c/19271/2/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

http://gerrit.cloudera.org:8080/#/c/19271/2/src/kudu/fs/fs_manager.cc@140
PS2, Line 140: URLs.
> nit: URLs
Done


http://gerrit.cloudera.org:8080/#/c/19271/2/src/kudu/util/curl_util.h
File src/kudu/util/curl_util.h:

http://gerrit.cloudera.org:8080/#/c/19271/2/src/kudu/util/curl_util.h@59
PS2, Line 59:   // Same as above, but with failover support. If the connection to the first
> maybe add a note to the comments for the multiple url variants, here and be
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8
Gerrit-Change-Number: 19271
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Nov 2022 16:16:22 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3423 Add support for Ranger KMS HA

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19271 )

Change subject: KUDU-3423 Add support for Ranger KMS HA
......................................................................


Patch Set 6: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8
Gerrit-Change-Number: 19271
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Jan 2023 02:00:01 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3423 Add support for Ranger KMS HA

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/19271 )

Change subject: KUDU-3423 Add support for Ranger KMS HA
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/19271/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19271/3//COMMIT_MSG@17
PS3, Line 17: and
> nit: then or just a comma
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8
Gerrit-Change-Number: 19271
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Nov 2022 16:27:26 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3423 Add support for Ranger KMS HA

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19271 )

Change subject: KUDU-3423 Add support for Ranger KMS HA
......................................................................


Patch Set 4:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/19271/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19271/4//COMMIT_MSG@16
PS4, Line 16: If a server
            : responds with an error
Is Ranges KMS able to respond with "503 Service Unavailable" or something like that?  If so, maybe at least treat 5xx server errors like a non-availability event and switch to the next in the list?


http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/fs/fs_manager.cc@141
PS4, Line 141: " "
nit: could drop the space and join two parts of the string?


http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/fs/fs_manager.cc@149
PS4, Line 149: FLAGS_ranger_kms_url.empty()
Since now a list of comma-separated strings can be provided as value for --ranger_kms_url, maybe check that the result of parsing the contents gives non-empty set of strings?  My concern is that the newly introduced libcurl functions return Status::OK() for empty set of URLs, and a misconfiguration for --ranger_kms_url might be detected only at later stage


http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/ranger-kms/ranger_kms_client.cc
File src/kudu/ranger-kms/ranger_kms_client.cc:

http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/ranger-kms/ranger_kms_client.cc@63
PS4, Line 63: for (int i = 0; i < kms_urls_.size(); i++) {
nit for here and below: since having the index isn't crucial, there could be more concise version

  for (const auto& url : kms_urls_) {
     ...
  }


http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/util/curl_util.h
File src/kudu/util/curl_util.h:

PS4: 
Consider separating this newly added {fetch,post}-with-failover functionality into its own patch and adding test coverage as well.


http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/util/curl_util.h@61
PS4, Line 61:   // one in the list.
Maybe, also mention that it stops at the first successful fetch attempt and return the result.


http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/util/curl_util.h@78
PS4, Line 78:   Status PostToURL(const std::vector<std::string>& urls,
ditto: maybe mention what happens on the fist successful post


http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/util/curl_util.cc
File src/kudu/util/curl_util.cc:

http://gerrit.cloudera.org:8080/#/c/19271/4/src/kudu/util/curl_util.cc@135
PS4, Line 135: for (int i = 0; i < urls.size(); ++i)
nit: since having the index isn't crucial here, consider more concise form of this

  for (const auto& url : urls) {
    ...
  }



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8
Gerrit-Change-Number: 19271
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Nov 2022 22:06:25 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3423 Add support for Ranger KMS HA

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has removed a vote on this change.

Change subject: KUDU-3423 Add support for Ranger KMS HA
......................................................................


Removed Verified-1 by Kudu Jenkins (120)
-- 
To view, visit http://gerrit.cloudera.org:8080/19271
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8
Gerrit-Change-Number: 19271
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>

[kudu-CR] KUDU-3423 Add support for Ranger KMS HA

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Chovan, Alexey Serbin, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/19271

to look at the new patch set (#4).

Change subject: KUDU-3423 Add support for Ranger KMS HA
......................................................................

KUDU-3423 Add support for Ranger KMS HA

Ranger KMS supports high availability by listing multiple Ranger KMS
servers (comma-separated, no spaces). Up until now, Kudu allowed
configuring only one Ranger KMS server.

This patch adds support for listing multiple KMS servers. This is done
by extending EasyCurl to support multiple URLs. Kudu always attempts to
connect to the first one in the list, and if the connection fails due to
a network error or a timeout, it fails over to the next one. If a server
responds with an error, that is considered a valid response and then it
is returned immediately to the caller without trying the remaining URLs.

Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8
---
M src/kudu/fs/fs_manager.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/ranger-kms/ranger_kms_client.cc
M src/kudu/ranger-kms/ranger_kms_client.h
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
6 files changed, 97 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/19271/4
-- 
To view, visit http://gerrit.cloudera.org:8080/19271
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8
Gerrit-Change-Number: 19271
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>

[kudu-CR] KUDU-3423 Add support for Ranger KMS HA

Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/19271 )

Change subject: KUDU-3423 Add support for Ranger KMS HA
......................................................................


Patch Set 3: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/19271/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19271/3//COMMIT_MSG@17
PS3, Line 17: and
nit: then or just a comma



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8
Gerrit-Change-Number: 19271
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Nov 2022 16:23:41 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3423 Add support for Ranger KMS HA

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/19271 )

Change subject: KUDU-3423 Add support for Ranger KMS HA
......................................................................

KUDU-3423 Add support for Ranger KMS HA

Ranger KMS supports high availability by listing multiple Ranger KMS
servers (comma-separated, no spaces). Up until now, Kudu allowed
configuring only one Ranger KMS server.

This patch adds support for listing multiple KMS servers. This is done
by extending EasyCurl to support multiple URLs. Kudu always attempts to
connect to the first one in the list, and if the connection fails due to
a network error or a timeout, it fails over to the next one. If a server
responds with an error, that is considered a valid response and then it
is returned immediately to the caller without trying the remaining URLs.

Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8
Reviewed-on: http://gerrit.cloudera.org:8080/19271
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <al...@apache.org>
---
M src/kudu/fs/fs_manager.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/ranger-kms/ranger_kms_client.cc
M src/kudu/ranger-kms/ranger_kms_client.h
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
6 files changed, 104 insertions(+), 12 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Alexey Serbin: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ibef941ed20eda1f4e624c2c7e16ca7955af570d8
Gerrit-Change-Number: 19271
Gerrit-PatchSet: 7
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>