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

Suppress user and password info output via Config structure. #947

Merged
merged 11 commits into from
Jun 30, 2021
Merged

Suppress user and password info output via Config structure. #947

merged 11 commits into from
Jun 30, 2021

Conversation

wangzhen11aaa
Copy link
Contributor

@wangzhen11aaa wangzhen11aaa commented Jun 29, 2021

I hereby agree to the terms of the CLA available at: https://datafuse.rs/policies/cla/

Summary

Draft pull request. May not the best solution, Please give advice about the improvement.

Changelog

Rewrite the Debug trait for the Config structure. This fix will affect the extension of the Config, and seems not the best answer.

  • Improvement

  • Not for changelog (changelog entry is not required)

Related Issues

fixes #935

Test Plan

Unit Tests
Stateless Tests

@databend-bot
Copy link
Member

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

self.clickhouse_handler_thread_num, self.flight_api_address, self.http_api_address, self.metric_api_address,
self.store_api_address, self.config_file)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

maybe should add new line : )

#[derive(Clone, Debug, serde::Deserialize, PartialEq, StructOpt, StructOptToml)]
impl fmt::Debug for Config{
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "{{ log_level: \"{}\", log_dir: \"{}\", num_cpus: {}, mysql_handler_host: \"{}\",
Copy link
Member

@zhang2014 zhang2014 Jun 29, 2021

Choose a reason for hiding this comment

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

It looks like a JSON. maybe we can consider the following two implementations:

  1. Reference https://github.com/rust-lang/rust/blob/master/library/core/src/fmt/mod.rs#L523 to implemented.

or

  1. Use serde::Serialize and skip field attribute

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will follow up. Thx.

Copy link
Member

@zhang2014 zhang2014 Jun 29, 2021

Choose a reason for hiding this comment

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

Or we can implement the following code and change the type of store_api_password to Password

struct Password {
    plain_text : String
}

impl Debug for Password {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "******");
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.

@wangzhen11aaa
Copy link
Contributor Author

Please review and give more precious advice.

@wangzhen11aaa
Copy link
Contributor Author

wangzhen11aaa commented Jun 30, 2021

We need implement to_string() function to work with env_helper! macro.
May be like this.
https://doc.rust-lang.org/std/string/trait.ToString.html


#[structopt(long, short = "c", env = CONFIG_FILE, default_value = "")]
pub config_file: String,
}
#[derive(Clone, serde::Deserialize, PartialEq, StructOpt, StructOptToml)]
Copy link
Member

Choose a reason for hiding this comment

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

need new line.

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

write!(f, "******")
}
}
#[derive(Clone, serde::Deserialize, PartialEq, StructOpt, StructOptToml)]
Copy link
Member

Choose a reason for hiding this comment

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

need new line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
This Rust Assist extesion looks good. Do we need this format tool?

Copy link
Member

Choose a reason for hiding this comment

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

We use rust-fmt to format the code. You can use make lint to check the format

@wangzhen11aaa wangzhen11aaa marked this pull request as draft June 30, 2021 11:23
@wangzhen11aaa wangzhen11aaa marked this pull request as ready for review June 30, 2021 11:28
@databend-bot
Copy link
Member

Hello @wangzhen11aaa, 🎉 Thank you for opening the pull request! 🎉
Your pull request state is not in Draft, please add Reviewers or Re-request review again!
FuseQuery: @BohuTANG @sundy-li @zhang2014
FuseStore: @drmingdrmer @dantengsky
Or visit datafuse roadmap for some clues.

@wangzhen11aaa wangzhen11aaa marked this pull request as draft June 30, 2021 12:27
@wangzhen11aaa wangzhen11aaa marked this pull request as ready for review June 30, 2021 12:28
@wangzhen11aaa wangzhen11aaa marked this pull request as draft June 30, 2021 12:30
@wangzhen11aaa wangzhen11aaa marked this pull request as ready for review June 30, 2021 12:32
@wangzhen11aaa
Copy link
Contributor Author

We need implement to_string() function to work with env_helper! macro.
May be like this.
https://doc.rust-lang.org/std/string/trait.ToString.html

Need to implement ToString trait for User and Password.

Copy link
Member

@zhang2014 zhang2014 left a comment

Choose a reason for hiding this comment

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

LGTM
Thank you @wangzhen11aaa

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot databend-bot merged commit 402edbf into databendlabs:master Jun 30, 2021
@wangzhen11aaa wangzhen11aaa deleted the fix_confict branch January 24, 2022 14:41
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.

[security] mask/remove the user and password for config debug
3 participants