[BugFix] Support authentication for StarRocks external table #11871
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
padmejin
reviewed
Sep 30, 2022
nshangyiming
previously approved these changes
Sep 30, 2022
fe/fe-core/src/main/java/com/starrocks/service/FrontendServiceImpl.java
Outdated
Show resolved
Hide resolved
|
@padmejin @nshangyiming Thanks for your review. I have resolved the comments, and please help to take a look if your are available. |
padmejin
previously approved these changes
Sep 30, 2022
murphyatwork
previously approved these changes
Sep 30, 2022
|
run starrocks_admit_test |
|
run starrocks_admit_test |
nshangyiming
previously approved these changes
Oct 1, 2022
auto-merge was automatically disabled
October 2, 2022 16:20
Head branch was pushed to by a user without write access
banmoy
dismissed stale reviews from nshangyiming, murphyatwork, and padmejin
via
bcd6905
|
run starrocks_admit_test |
2 similar comments
|
run starrocks_admit_test |
|
run starrocks_admit_test |
nshangyiming
previously approved these changes
Oct 3, 2022
|
run starrocks_fe_unittest |
|
run starrocks_admit_test |
banmoy
force-pushed
the
external_table_main
branch
from
46b0c86 to
7eef603
Compare
|
run starrocks_admit_test |
banmoy
force-pushed
the
external_table_main
branch
from
7eef603 to
dc4f9c5
Compare
|
run starrocks_admit_test |
|
Kudos, SonarCloud Quality Gate passed! |
[FE PR Coverage Check]😞 fail : 47 / 63 (74.60%) file detail
|
|
run starrocks_tscancode |
padmejin
reviewed
Oct 8, 2022
| LOG.debug("Receive TAuthenticateParams [user: {}, host: {}, db: {}, tables: {}]", | ||
| authParams.user, authParams.getHost(), authParams.getDb_name(), authParams.getTable_names()); | ||
| if (!Config.enable_starrocks_external_table_auth_check) { | ||
| LOG.debug("enable_starrocks_external_table_auth_check is disabled, " + |
There was a problem hiding this comment.
I suggest you change this log level to INFO. I think it's an important log
There was a problem hiding this comment.
@padmejin It may cause too many logs? every transaction will print this line.
There was a problem hiding this comment.
@padmejin This log is helpful in the case the check is enabled, but a wrong user/password/privilege is passed. I think it may be reproduced easily if happened, and we can enable DEBUG level logging dynamically to simplify the log. What do you think?
nshangyiming
approved these changes
Oct 8, 2022
|
run starrocks_be_unittest |
|
run starrocks_clang-tidy |
padmejin
approved these changes
Oct 9, 2022
Astralidea
approved these changes
Oct 9, 2022
gengjun-git
approved these changes
Oct 9, 2022
This was referenced Oct 9, 2022
[cherry-pick][branch-2.2][BugFix]Support authentication for StarRocks external table (#11871)
#11979
Merged
southernriver
pushed a commit
to southernriver/starrocks
that referenced
this pull request
Nov 11, 2022
…nto branch-2.4-tq * 'branch-2.4' of https://github.com/StarRocks/starrocks: (84 commits) Upgrade opencsv due to CVE-2022-42889 (StarRocks#12526) [Cherry-pick][BugFix] Fix complex window + agg prune bug [BugFix] Fix transaction stream load empty label return message (StarRocks#12372) [Enhancement] Fix log printing in be (StarRocks#12497) [BugFix] Fix use-after-free in when TabletScanner call destructor (StarRocks#12453) [Doc]routine bugfix (StarRocks#12464) Update SHOW ALTER.md (StarRocks#12423) Update the link for Coloate join (StarRocks#12431) [Bugfix] Publish timeout in commitPreparedTransaction should raise error (StarRocks#12217) (StarRocks#12300) [Docs] Remove /bin/sh when starting binary (StarRocks#12434) [BugFix] fix alter table in new publish; add detail error message for load/insert publish timeout (StarRocks#12237) [BugFix] fix incorrect memory metrics of jemalloc (StarRocks#12365) Update Data_model.md (StarRocks#12420) [Doc] Update StreamLoad.md in Branch 2.4 (StarRocks#12410) [Doc]Update filemanager.md (StarRocks#12401) [Doc] correctify the parameter name of sink.properties.columns (StarRocks#12389) Cherry-pick MAXVALUE bugfixs to branch-2.4 [BugFix] Fix fencing failed when a new leader is elected (StarRocks#12138) [cherry-pick][branch-2.4][BugFix] Support authentication for StarRocks external table (StarRocks#11871) (StarRocks#12011) [Doc] Update Stream_Load_transaction_interface.md in Branch 2.4 (StarRocks#12371) ...
16 tasks
Astralidea
pushed a commit
that referenced
this pull request
Jan 4, 2023
…16130) In #11871, to support authentication for StarRocks external table, we add three fields in thrift struct TAuthenticateParams: host, db_name and table_names, and they are marked required which leads to a compatible problem. Denote the starrocks before #11871 as v1, and starrocks after #11871 as v2, and we can ingesting data to v1 via the starrocks external table on v2 , but can't ingest data to v2 via external table on v1. The exception is as follows #11871 adds host, db_name and table_names to TAuthenticateParams as required struct TAuthenticateParams { 1: required string user 2: required string passwd 3: required string host 4: required string db_name 5: required list<string> table_names; } The reason is that external table on v1 will issue a RPC to v2 to request the meta of destination table The RPC carries a parameter TAuthenticateParams created on v1 which not contains fields host, db_name and table_names thrift requires these fields on v2, so the RPC failed when ingesting data from v2 to v1, the RPC created on v2 does not carry TAuthenticateParams, so there is no problem
mergify bot
pushed a commit
that referenced
this pull request
Jan 4, 2023
…16130) In #11871, to support authentication for StarRocks external table, we add three fields in thrift struct TAuthenticateParams: host, db_name and table_names, and they are marked required which leads to a compatible problem. Denote the starrocks before #11871 as v1, and starrocks after #11871 as v2, and we can ingesting data to v1 via the starrocks external table on v2 , but can't ingest data to v2 via external table on v1. The exception is as follows #11871 adds host, db_name and table_names to TAuthenticateParams as required struct TAuthenticateParams { 1: required string user 2: required string passwd 3: required string host 4: required string db_name 5: required list<string> table_names; } The reason is that external table on v1 will issue a RPC to v2 to request the meta of destination table The RPC carries a parameter TAuthenticateParams created on v1 which not contains fields host, db_name and table_names thrift requires these fields on v2, so the RPC failed when ingesting data from v2 to v1, the RPC created on v2 does not carry TAuthenticateParams, so there is no problem (cherry picked from commit 99b2b77)
mergify bot
pushed a commit
that referenced
this pull request
Jan 4, 2023
…16130) In #11871, to support authentication for StarRocks external table, we add three fields in thrift struct TAuthenticateParams: host, db_name and table_names, and they are marked required which leads to a compatible problem. Denote the starrocks before #11871 as v1, and starrocks after #11871 as v2, and we can ingesting data to v1 via the starrocks external table on v2 , but can't ingest data to v2 via external table on v1. The exception is as follows #11871 adds host, db_name and table_names to TAuthenticateParams as required struct TAuthenticateParams { 1: required string user 2: required string passwd 3: required string host 4: required string db_name 5: required list<string> table_names; } The reason is that external table on v1 will issue a RPC to v2 to request the meta of destination table The RPC carries a parameter TAuthenticateParams created on v1 which not contains fields host, db_name and table_names thrift requires these fields on v2, so the RPC failed when ingesting data from v2 to v1, the RPC created on v2 does not carry TAuthenticateParams, so there is no problem (cherry picked from commit 99b2b77)
mergify bot
pushed a commit
that referenced
this pull request
Jan 4, 2023
…16130) In #11871, to support authentication for StarRocks external table, we add three fields in thrift struct TAuthenticateParams: host, db_name and table_names, and they are marked required which leads to a compatible problem. Denote the starrocks before #11871 as v1, and starrocks after #11871 as v2, and we can ingesting data to v1 via the starrocks external table on v2 , but can't ingest data to v2 via external table on v1. The exception is as follows #11871 adds host, db_name and table_names to TAuthenticateParams as required struct TAuthenticateParams { 1: required string user 2: required string passwd 3: required string host 4: required string db_name 5: required list<string> table_names; } The reason is that external table on v1 will issue a RPC to v2 to request the meta of destination table The RPC carries a parameter TAuthenticateParams created on v1 which not contains fields host, db_name and table_names thrift requires these fields on v2, so the RPC failed when ingesting data from v2 to v1, the RPC created on v2 does not carry TAuthenticateParams, so there is no problem (cherry picked from commit 99b2b77)
mergify bot
pushed a commit
that referenced
this pull request
Jan 4, 2023
…16130) In #11871, to support authentication for StarRocks external table, we add three fields in thrift struct TAuthenticateParams: host, db_name and table_names, and they are marked required which leads to a compatible problem. Denote the starrocks before #11871 as v1, and starrocks after #11871 as v2, and we can ingesting data to v1 via the starrocks external table on v2 , but can't ingest data to v2 via external table on v1. The exception is as follows #11871 adds host, db_name and table_names to TAuthenticateParams as required struct TAuthenticateParams { 1: required string user 2: required string passwd 3: required string host 4: required string db_name 5: required list<string> table_names; } The reason is that external table on v1 will issue a RPC to v2 to request the meta of destination table The RPC carries a parameter TAuthenticateParams created on v1 which not contains fields host, db_name and table_names thrift requires these fields on v2, so the RPC failed when ingesting data from v2 to v1, the RPC created on v2 does not carry TAuthenticateParams, so there is no problem (cherry picked from commit 99b2b77)
wanpengfei-git
pushed a commit
that referenced
this pull request
Jan 4, 2023
…16130) In #11871, to support authentication for StarRocks external table, we add three fields in thrift struct TAuthenticateParams: host, db_name and table_names, and they are marked required which leads to a compatible problem. Denote the starrocks before #11871 as v1, and starrocks after #11871 as v2, and we can ingesting data to v1 via the starrocks external table on v2 , but can't ingest data to v2 via external table on v1. The exception is as follows #11871 adds host, db_name and table_names to TAuthenticateParams as required struct TAuthenticateParams { 1: required string user 2: required string passwd 3: required string host 4: required string db_name 5: required list<string> table_names; } The reason is that external table on v1 will issue a RPC to v2 to request the meta of destination table The RPC carries a parameter TAuthenticateParams created on v1 which not contains fields host, db_name and table_names thrift requires these fields on v2, so the RPC failed when ingesting data from v2 to v1, the RPC created on v2 does not carry TAuthenticateParams, so there is no problem (cherry picked from commit 99b2b77)
wanpengfei-git
pushed a commit
that referenced
this pull request
Jan 4, 2023
…16130) In #11871, to support authentication for StarRocks external table, we add three fields in thrift struct TAuthenticateParams: host, db_name and table_names, and they are marked required which leads to a compatible problem. Denote the starrocks before #11871 as v1, and starrocks after #11871 as v2, and we can ingesting data to v1 via the starrocks external table on v2 , but can't ingest data to v2 via external table on v1. The exception is as follows #11871 adds host, db_name and table_names to TAuthenticateParams as required struct TAuthenticateParams { 1: required string user 2: required string passwd 3: required string host 4: required string db_name 5: required list<string> table_names; } The reason is that external table on v1 will issue a RPC to v2 to request the meta of destination table The RPC carries a parameter TAuthenticateParams created on v1 which not contains fields host, db_name and table_names thrift requires these fields on v2, so the RPC failed when ingesting data from v2 to v1, the RPC created on v2 does not carry TAuthenticateParams, so there is no problem (cherry picked from commit 99b2b77)
wanpengfei-git
pushed a commit
that referenced
this pull request
Jan 4, 2023
…16130) In #11871, to support authentication for StarRocks external table, we add three fields in thrift struct TAuthenticateParams: host, db_name and table_names, and they are marked required which leads to a compatible problem. Denote the starrocks before #11871 as v1, and starrocks after #11871 as v2, and we can ingesting data to v1 via the starrocks external table on v2 , but can't ingest data to v2 via external table on v1. The exception is as follows #11871 adds host, db_name and table_names to TAuthenticateParams as required struct TAuthenticateParams { 1: required string user 2: required string passwd 3: required string host 4: required string db_name 5: required list<string> table_names; } The reason is that external table on v1 will issue a RPC to v2 to request the meta of destination table The RPC carries a parameter TAuthenticateParams created on v1 which not contains fields host, db_name and table_names thrift requires these fields on v2, so the RPC failed when ingesting data from v2 to v1, the RPC created on v2 does not carry TAuthenticateParams, so there is no problem (cherry picked from commit 99b2b77)
wanpengfei-git
pushed a commit
that referenced
this pull request
Jan 5, 2023
…16130) In #11871, to support authentication for StarRocks external table, we add three fields in thrift struct TAuthenticateParams: host, db_name and table_names, and they are marked required which leads to a compatible problem. Denote the starrocks before #11871 as v1, and starrocks after #11871 as v2, and we can ingesting data to v1 via the starrocks external table on v2 , but can't ingest data to v2 via external table on v1. The exception is as follows #11871 adds host, db_name and table_names to TAuthenticateParams as required struct TAuthenticateParams { 1: required string user 2: required string passwd 3: required string host 4: required string db_name 5: required list<string> table_names; } The reason is that external table on v1 will issue a RPC to v2 to request the meta of destination table The RPC carries a parameter TAuthenticateParams created on v1 which not contains fields host, db_name and table_names thrift requires these fields on v2, so the RPC failed when ingesting data from v2 to v1, the RPC created on v2 does not carry TAuthenticateParams, so there is no problem (cherry picked from commit 99b2b77)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this：
Which issues of this PR fixes ：
Fixes #
Problem Summary(Required) ：
When creating StarRocks external table, properties of
userandpasswordare required. But currently the target cluster does't check the password andLOAD_PRIV, so theINSERT INTOwill be successful even if the user does not exist, or the user has noLOAD_PRIVon the target cluster. We should fix this problem.How we fix it
When inserting data into the table on the target cluster, the source cluster will issue multiple RPCs to the target cluster in order, including
beginRemoteTxn,commitRemoteTxn, andabortRemoteTxninFrontendServiceImpl. We only need todo the check in the first RPC
beginRemoteTxn. So authentication information will be carried by the RPC, and checked in the targetbeginRemoteTxn.Checklist: