[opt](http) enable auth token with BE http request by morningman · Pull Request #41994 · apache/doris

morningman · 2024-10-16T16:33:49Z

What problem does this PR solve?

Related PR: #39577

Problem Summary:
In #39577, we added the auth check for all HTTP API on FE side.
But it introduced an issue that when enable_all_http_auth, the internal http request
will fail due to lack of authentication info.
For example, when cloning replica from one BE to another, it use HTTP API.

This PR mainly changes:

Unify the token generation and checking logic
Move TokenManager from load package to Env, as a global mgr.
It is responsible for generating tokens at fix interval.
And the token will be sent to BE via heartbeat.
BE will save last 2 tokens, and use the latest token in HTTP request.
All HTTP request sent by BE will add a header Auth-Token,
and BE's HTTP server will check if this token in header is same as token
from FE heartbeat.
Add a new class ClusterInfo on BE side to replace TMasterInfo.
TMasterInfo is a thrift object used to save master info and pass them
from FE to BE via heartbeat.
So it should only be a message payload, we should get info from it and
save it in another structure: ClusterInfo.

Check List (For Committer)

Test
- Regression test
- Unit Test
- Manual test
  I created a cluster with 2 BE, set enable_all_http_auth=true. And create a table with 1 replica,
  and then modify the replica num to 2. The clone task run success.
- No need to test or manual test. Explain why:
  - This is a refactor/code format and no logic has been changed.
  - Previous test can cover this change.
  - No colde files have been changed.
  - Other reason
Behavior changed:
- No.
- Yes.
Does this need documentation?
- No.
- Yes.
Release note
None

Check List (For Reviewer who merge this PR)

Confirm the release note
Confirm test cases
Confirm document
Add branch pick label

doris-robot · 2024-10-16T16:33:53Z

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR

Since 2024-03-18, the Document has been moved to doris-website.
See Doris Document.

github-actions

clang-tidy made some suggestions

github-actions · 2024-10-16T16:39:06Z

+ } else if (_master_info->curr_auth_token != master_info.auth_token)
+ _master_info->__set_last_auth_token(_master_info->curr_auth_token);


warning: statement should be inside braces [readability-braces-around-statements]
Suggested change
} elseif (_master_info->curr_auth_token != master_info.auth_token)
_master_info->__set_last_auth_token(_master_info->curr_auth_token);
} elseif (_master_info->curr_auth_token != master_info.auth_token) {
_master_info->__set_last_auth_token(_master_info->curr_auth_token);
}

github-actions · 2024-10-16T16:39:06Z

 const char* HttpHeaders::WWW_AUTHENTICATE = "WWW-Authenticate";

 const std::string HttpHeaders::JsonType = "application/json";
+const std::string HttpHeaders::AUTH_TOKEN = "Auth-Token";


warning: no member named 'AUTH_TOKEN' in 'doris::HttpHeaders' [clang-diagnostic-error]
const std::string HttpHeaders::AUTH_TOKEN = "Auth-Token"; ^

github-actions

clang-tidy made some suggestions

github-actions · 2024-11-06T16:32:49Z

- uint32_t worker_thread_num, TMasterInfo* local_master_info) {
- HeartbeatServer* heartbeat_server = new HeartbeatServer(local_master_info);
+ uint32_t worker_thread_num, ClusterInfo* cluster_info) {
+ HeartbeatServer* heartbeat_server = new HeartbeatServer(cluster_info);


warning: use auto when initializing with new to avoid duplicating the type name [modernize-use-auto]
Suggested change
HeartbeatServer* heartbeat_server = new HeartbeatServer(cluster_info);
auto* heartbeat_server = new HeartbeatServer(cluster_info);

morningman · 2024-11-07T10:47:39Z

run buildall

morningman · 2024-11-07T10:58:47Z

run buildall

github-actions

clang-tidy made some suggestions

morningman · 2024-11-07T16:17:24Z

run buildall

gavinchou · 2024-11-07T17:21:01Z

+// This class is used to save the cluster info
+// like cluster id, epoch, cloud_unique_id, etc.
+// These info are usually in heartbeat from Master FE.
+class ClusterInfo {


class and struct is same?

gavinchou · 2024-11-07T17:23:16Z

+ int64_t backend_id = 0;
+
+ // Auth token for internal authentication
+ std::string curr_auth_token = "";


What is the relationship and connection between these "tokens"?

added comment

gavinchou · 2024-11-07T17:31:05Z

 11: optional string cloud_unique_id;
 // See configuration item Config.java rehash_tablet_after_be_dead_seconds for meaning
 12: optional i64 tablet_report_inactive_duration_ms;
+ 13: optional string last_auth_token;


seems FE does not set it?

gavinchou · 2024-11-07T17:31:40Z

how do we get it tested?

doris-robot · 2024-11-07T18:10:53Z

TeamCity be ut coverage result:
Function Coverage: 37.90% (9865/26028)
Line Coverage: 29.06% (82054/282361)
Region Coverage: 28.29% (42269/149435)
Branch Coverage: 24.86% (21441/86252)
Coverage Report: http://coverage.selectdb-in.cc/coverage/4c3f88246c1136d8e7d8e273f326aaaa0ec208d4_4c3f88246c1136d8e7d8e273f326aaaa0ec208d4/report/index.html

morningman · 2024-11-07T23:56:38Z

run buildall

morningman · 2024-11-07T23:58:34Z

how do we get it tested?

already described in PR description. But we also need test it in a multi node env

github-actions

clang-tidy made some suggestions

github-actions · 2024-11-08T00:05:15Z

+
+#pragma once
+
+#include <gen_cpp/Types_types.h>


warning: 'gen_cpp/Types_types.h' file not found [clang-diagnostic-error]
#include<gen_cpp/Types_types.h> ^

morningman · 2024-11-10T05:32:03Z

run buildall

doris-robot · 2024-11-10T07:26:15Z

TeamCity be ut coverage result:
Function Coverage: 37.91% (9868/26029)
Line Coverage: 29.11% (82315/282803)
Region Coverage: 28.25% (42359/149942)
Branch Coverage: 24.80% (21451/86500)
Coverage Report: http://coverage.selectdb-in.cc/coverage/8fe9ba30c4c51d151a4081261a22d46bc2caac5a_8fe9ba30c4c51d151a4081261a22d46bc2caac5a/report/index.html

github-actions · 2024-11-11T13:19:40Z

PR approved by anyone and no changes requested.

kaka11chen

LGTM

github-actions · 2024-11-12T02:46:42Z

PR approved by at least one committer and no changes requested.

### What problem does this PR solve? Related PR: #39577 Problem Summary: In #39577, we added the auth check for all HTTP API on FE side. But it introduced an issue that when `enable_all_http_auth`, the internal http request will fail due to lack of authentication info. For example, when cloning replica from one BE to another, it use HTTP API. This PR mainly changes: 1. Unify the token generation and checking logic Move `TokenManager` from `load` package to `Env`, as a global mgr. It is responsible for generating tokens at fix interval. And the token will be sent to BE via heartbeat. BE will save last 2 tokens, and use the latest token in HTTP request. All HTTP request sent by BE will add a header `Auth-Token`, and BE's HTTP server will check if this token in header is same as token from FE heartbeat. 2. Add a new class `ClusterInfo` on BE side to replace `TMasterInfo`. `TMasterInfo` is a thrift object used to save master info and pass them from FE to BE via heartbeat. So it should only be a message payload, we should get info from it and save it in another structure: `ClusterInfo`. Co-authored-by: morningman <yunyou@selectdb.com>

### What problem does this PR solve? Related PR: apache#39577 Problem Summary: In apache#39577, we added the auth check for all HTTP API on FE side. But it introduced an issue that when `enable_all_http_auth`, the internal http request will fail due to lack of authentication info. For example, when cloning replica from one BE to another, it use HTTP API. This PR mainly changes: 1. Unify the token generation and checking logic Move `TokenManager` from `load` package to `Env`, as a global mgr. It is responsible for generating tokens at fix interval. And the token will be sent to BE via heartbeat. BE will save last 2 tokens, and use the latest token in HTTP request. All HTTP request sent by BE will add a header `Auth-Token`, and BE's HTTP server will check if this token in header is same as token from FE heartbeat. 2. Add a new class `ClusterInfo` on BE side to replace `TMasterInfo`. `TMasterInfo` is a thrift object used to save master info and pass them from FE to BE via heartbeat. So it should only be a message payload, we should get info from it and save it in another structure: `ClusterInfo`. Co-authored-by: morningman <yunyou@selectdb.com>

Cherry-picked from #41994 Co-authored-by: Mingyu Chen (Rayner) <morningman@163.com> Co-authored-by: morningman <yunyou@selectdb.com>

### What problem does this PR solve? Related PR: apache#39577 Problem Summary: In apache#39577, we added the auth check for all HTTP API on FE side. But it introduced an issue that when `enable_all_http_auth`, the internal http request will fail due to lack of authentication info. For example, when cloning replica from one BE to another, it use HTTP API. This PR mainly changes: 1. Unify the token generation and checking logic Move `TokenManager` from `load` package to `Env`, as a global mgr. It is responsible for generating tokens at fix interval. And the token will be sent to BE via heartbeat. BE will save last 2 tokens, and use the latest token in HTTP request. All HTTP request sent by BE will add a header `Auth-Token`, and BE's HTTP server will check if this token in header is same as token from FE heartbeat. 2. Add a new class `ClusterInfo` on BE side to replace `TMasterInfo`. `TMasterInfo` is a thrift object used to save master info and pass them from FE to BE via heartbeat. So it should only be a message payload, we should get info from it and save it in another structure: `ClusterInfo`. Co-authored-by: morningman <yunyou@selectdb.com>

github-actionsBot reviewed Oct 16, 2024
View reviewed changes

morningman force-pushed the auth_token branch 2 times, most recently from 2f02060 to 00880a0Compare November 6, 2024 09:51

github-actionsBot reviewed Nov 6, 2024
View reviewed changes

morningman added the dev/3.0.x label Nov 7, 2024

github-actionsBot reviewed Nov 7, 2024
View reviewed changes

morningman marked this pull request as ready for review November 7, 2024 16:16

morningman requested review from CalvinKirs and dataroaring as code owners November 7, 2024 16:16

apache deleted a comment from github-actionsBotNov 7, 2024

gavinchou reviewed Nov 7, 2024
View reviewed changes

morningman marked this pull request as draft November 8, 2024 00:01

github-actionsBot reviewed Nov 8, 2024
View reviewed changes

morningmanand others added 9 commits November 10, 2024 13:31

1
bf86be8

1
3616097

2
718dd1c

fix ut
c571222

add auth token
eef2071

3
c1977bb

fix
fc6322f

fix
3bede3e

fix
b8b90e0

morningman added 2 commits November 10, 2024 13:31

enable auth to test
74d7af2

a
8fe9ba3

morningman force-pushed the auth_token branch from 8531881 to 8fe9ba3Compare November 10, 2024 05:31

morningman marked this pull request as ready for review November 11, 2024 08:31

hubgeter approved these changes Nov 11, 2024
View reviewed changes

github-actionsBot added the reviewed label Nov 11, 2024

kaka11chen approved these changes Nov 12, 2024
View reviewed changes

github-actionsBot added the approved Indicates a PR has been approved by one committer. label Nov 12, 2024

CalvinKirs approved these changes Nov 12, 2024
View reviewed changes

morningman merged commit 7d290e1 into apache:masterNov 12, 2024

github-actionsBot mentioned this pull request Nov 12, 2024
branch-3.0: [opt](http) enable auth token with BE http request #43659
Merged

dataroaring added the kind/behavior-changed label Nov 12, 2024

morningman added a commit that referenced this pull request Nov 12, 2024
branch-3.0: [opt](http) enable auth token with BE http request (#43659)
478b36e
Cherry-picked from #41994 Co-authored-by: Mingyu Chen (Rayner) <morningman@163.com> Co-authored-by: morningman <yunyou@selectdb.com>

morningman added dev/3.0.3-merged and removed dev/3.0.x labels Nov 12, 2024

		} else if (_master_info->curr_auth_token != master_info.auth_token)
		_master_info->__set_last_auth_token(_master_info->curr_auth_token);

	HeartbeatServer* heartbeat_server = new HeartbeatServer(cluster_info);
	auto* heartbeat_server = new HeartbeatServer(cluster_info);


		#pragma once

		#include <gen_cpp/Types_types.h>

Uh oh!

Conversation

morningman commented Oct 16, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

Check List (For Committer)

Check List (For Reviewer who merge this PR)

Uh oh!

doris-robot commented Oct 16, 2024

Uh oh!

github-actionsBot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actionsBotOct 16, 2024

Choose a reason for hiding this comment

Uh oh!

github-actionsBotOct 16, 2024

Choose a reason for hiding this comment

Uh oh!

github-actionsBot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actionsBotNov 6, 2024

Choose a reason for hiding this comment

Uh oh!

morningman commented Nov 7, 2024

Uh oh!

morningman commented Nov 7, 2024

Uh oh!

github-actionsBot left a comment

Choose a reason for hiding this comment

Uh oh!

morningman commented Nov 7, 2024

Uh oh!

gavinchouNov 7, 2024

Choose a reason for hiding this comment

Uh oh!

morningmanNov 7, 2024

Choose a reason for hiding this comment

Uh oh!

gavinchouNov 7, 2024

Choose a reason for hiding this comment

Uh oh!

morningmanNov 7, 2024

Choose a reason for hiding this comment

Uh oh!

gavinchouNov 7, 2024

Choose a reason for hiding this comment

Uh oh!

morningmanNov 7, 2024

Choose a reason for hiding this comment

Uh oh!

gavinchou commented Nov 7, 2024

Uh oh!

doris-robot commented Nov 7, 2024

Uh oh!

morningman commented Nov 7, 2024

Uh oh!

morningman commented Nov 7, 2024

Uh oh!

github-actionsBot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actionsBotNov 8, 2024

Choose a reason for hiding this comment

Uh oh!

morningman commented Nov 10, 2024

Uh oh!

doris-robot commented Nov 10, 2024

Uh oh!

github-actionsBot commented Nov 11, 2024

Uh oh!

kaka11chen left a comment

Choose a reason for hiding this comment

Uh oh!

github-actionsBot commented Nov 12, 2024

Uh oh!

Reviewers

Assignees

Labels

morningman commented Oct 16, 2024•
edited
Loading