Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

curvefs/client: make user agent configurable #2501

Merged
merged 1 commit into from
Jul 24, 2023

Conversation

Ziy1-Tan
Copy link
Contributor

What problem does this PR solve?

Issue Number: #2499

Problem Summary:

Let user agent item configurable for users having security reasons

What is changed and how it works?

What's Changed:

How it Works:

Side effects(Breaking backward compatibility? Performance regression?):

Check List

  • Relevant documentation/comments is changed or added
  • I acknowledge that all my contributions will be made under the project's license

@Ziy1-Tan Ziy1-Tan requested a review from Cyber-SiKu May 25, 2023 07:17
@Ziy1-Tan
Copy link
Contributor Author

cicheck

Copy link
Contributor

@Cyber-SiKu Cyber-SiKu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested that it works?

@@ -10,7 +10,7 @@ s3.sk={{ s3_sk }}
# http = 0, https = 1
s3.http_scheme={{ s3_http_scheme }}
s3.verify_SSL={{ s3_verify_ssl }}
s3.user_agent_conf={{ s3_user_agent_conf }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ansible is no longer maintained and can not be modified

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

@@ -265,7 +265,7 @@ nebd_server_response_return_rpc_when_io_error: false
# s3配置默认值
s3_http_scheme: 0
s3_verify_ssl: false
s3_user_agent_conf: S3 Browser
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@Ziy1-Tan
Copy link
Contributor Author

cicheck

@Ziy1-Tan
Copy link
Contributor Author

Have you tested that it works?

Yes. I made sure that

  1. s3.user_agent appears in any other configuration item such as s3.verify_SSL.
  2. By debugging TEST_F(S3CompactTest, test_S3AdapterManager), I observed that the userAgent item has been assigned correctly.

@Ziy1-Tan
Copy link
Contributor Author

cicheck

@Cyber-SiKu
Copy link
Contributor

Have you tested that it works?

Yes. I made sure that

  1. s3.user_agent appears in any other configuration item such as s3.verify_SSL.
  2. By debugging TEST_F(S3CompactTest, test_S3AdapterManager), I observed that the userAgent item has been assigned correctly.

Please add the corresponding unit test

@Ziy1-Tan
Copy link
Contributor Author

cicheck

1 similar comment
@Ziy1-Tan
Copy link
Contributor Author

cicheck

@Ziy1-Tan
Copy link
Contributor Author

cicheck

1 similar comment
@Ziy1-Tan
Copy link
Contributor Author

cicheck

@wuhongsong
Copy link
Contributor

cicheck

@Cyber-SiKu
Copy link
Contributor

curvefs/conf/client.conf
This file should be the configuration file of the client of curvefs and should also need to be modified.

@Ziy1-Tan
Copy link
Contributor Author

cicheck

@Ziy1-Tan
Copy link
Contributor Author

curvefs/conf/client.conf This file should be the configuration file of the client of curvefs and should also need to be modified.

Get it. I'm trying to figure out why the mds election is timeout but I'm failed.

wuhongsong
wuhongsong previously approved these changes Jun 6, 2023
@wuhongsong
Copy link
Contributor

curvefs/conf/client.conf This file should be the configuration file of the client of curvefs and should also need to be modified.

Get it. I'm trying to figure out why the mds election is timeout but I'm failed.

@SeanHai help

@SeanHai
Copy link
Contributor

SeanHai commented Jun 21, 2023

cicheck

@SeanHai
Copy link
Contributor

SeanHai commented Jun 21, 2023

@Ziy1-Tan

2023-06-21 10:06:04.670070 I | [587688dba922b1f6/10.182.2.26:16701]stop watch current leader
F 2023-06-21T10:06:04.680009+0800 6 s3_adapter.cpp:85]
*** Check failure stack trace: ***
*** Aborted at 1687313164 (unix time) try "date -d @1687313164" if you are using GNU date ***
PC: @ 0x0 (unknown)
*** SIGABRT (@0x6) received by PID 6 (TID 0x7fcbd210d4c0) from PID 6; stack trace: ***
@ 0x7fcbd014e0e0 (unknown)
@ 0x7fcbce805fff gsignal
@ 0x7fcbce80742a abort
@ 0x55bec06199ec google::FlushAndAbort()
@ 0x55bec0616c3e google::LogMessage::Fail()
@ 0x55bec0616b87 google::LogMessage::SendToLog()
@ 0x55bec06163b0 google::LogMessage::Flush()
@ 0x55bec0619380 google::LogMessageFatal::~LogMessageFatal()
@ 0x55bebfde1858 curve::common::InitS3AdaptorOptionExceptS3InfoOption()
@ 0x55bebf94fe43 curvefs::mds::MDS::InitFsManagerOptions()
@ 0x55bebf950179 curvefs::mds::MDS::Init()
@ 0x55bebf94c42a main
@ 0x7fcbce7f32e1 __libc_start_main
@ 0x55bebf94bc4a _start
@ 0x0 (unknown)
fileList: []
WARNING: Logging before InitGoogleLogging() is written to STDERR

The curvefs/conf/mds.conf also need add this conf item. If not, it will crash when init s3 adaptor in mds of curvefs.

@Ziy1-Tan
Copy link
Contributor Author

cicheck

1 similar comment
@Ziy1-Tan
Copy link
Contributor Author

cicheck

@Ziy1-Tan
Copy link
Contributor Author

cicheck

2 similar comments
@Ziy1-Tan
Copy link
Contributor Author

cicheck

@Ziy1-Tan
Copy link
Contributor Author

cicheck

@Ziy1-Tan
Copy link
Contributor Author

cicheck

@Ziy1-Tan
Copy link
Contributor Author

cicheck

1 similar comment
@Ziy1-Tan
Copy link
Contributor Author

cicheck

@Cyber-SiKu
Copy link
Contributor

cichek

@caoxianfei1
Copy link
Contributor

image

add more test case @Ziy1-Tan

@wuhongsong
Copy link
Contributor

cicheck

@wuhongsong
Copy link
Contributor

image add more test case @Ziy1-Tan

I have add some test case,lets try it again.

@Ziy1-Tan
Copy link
Contributor Author

Ziy1-Tan commented Jul 6, 2023

cicheck

@SeanHai
Copy link
Contributor

SeanHai commented Jul 9, 2023

cicheck

@wuhongsong
Copy link
Contributor

@Ziy1-Tan will do it later, It's a bit busy these days.

@Ziy1-Tan
Copy link
Contributor Author

cicheck

@Ziy1-Tan
Copy link
Contributor Author

cicheck

Signed-off-by: Ziy1-Tan <ajb459684460@gmail.com>
@Ziy1-Tan
Copy link
Contributor Author

cicheck

@Cyber-SiKu Cyber-SiKu merged commit 0346d82 into opencurve:master Jul 24, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants