[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #3197: RFC-3197: Config

["Xuanwo (via GitHub)" <gi...@apache.org>]

Xuanwo commented on code in PR #3197:
URL: https://github.com/apache/incubator-opendal/pull/3197#discussion_r1338647671


##########
core/src/docs/rfcs/3197_config.md:
##########
@@ -0,0 +1,208 @@
+- Proposal Name: `config`
+- Start Date: 2023-09-27
+- RFC PR: [apache/incubator-opendal#3197](https://github.com/apache/incubator-opendal/pull/3197)
+- Tracking Issue: [apache/incubator-opendal#0000](https://github.com/apache/incubator-opendal/issues/0000)
+
+# Summary
+
+Expose services config to the user. 
+
+# Motivation
+
+OpenDAL provides two ways to configure services: through a builder pattern and via a map.
+
+The `Builder` allows users to configure services using the builder pattern:
+
+```rust
+// Create fs backend builder.
+let mut builder = Fs::default();
+// Set the root for fs, all operations will happen under this root.
+builder.root("/tmp");
+
+// Build an `Operator` to start operating the storage.
+let op: Operator = Operator::new(builder)?.finish();
+```
+
+The benefit of builder is that it is type safe and easy to use. However, it is not flexible enough to configure services. Users must create a new builder for each service they wish to configure, translating user input into the API calls for each respective builder.
+
+Consider the following real-world example from one of our users:
+
+```rust
+let mut builder = services::S3::default();
+
+// Credential.
+builder.access_key_id(&cfg.access_key_id);
+builder.secret_access_key(&cfg.secret_access_key);
+builder.security_token(&cfg.security_token);
+builder.role_arn(&cfg.role_arn);
+builder.external_id(&cfg.external_id);
+
+// Root.
+builder.root(&cfg.root);
+
+// Disable credential loader
+if cfg.disable_credential_loader {
+    builder.disable_config_load();
+    builder.disable_ec2_metadata();
+}
+
+// Enable virtual host style
+if cfg.enable_virtual_host_style {
+    builder.enable_virtual_host_style();
+}
+```
+
+The `Map` approach allows users to configure services using a string-based HashMap:
+
+```rust
+let map = HashMap::from([
+  // Set the root for fs, all operations will happen under this root.
+  ("root".to_string(), "/tmp".to_string()),
+]);
+
+// Build an `Operator` to start operating the storage.
+let op: Operator = Operator::via_map(Scheme::Fs, map)?;
+```
+
+This approach is simpler since it allows users to configure all services within a single map. However, it is not type safe and not easy to use. Users will need to convert their input to string and make sure the key is correct. And breaking changes could happen silently.
+
+This is one of our limitations: We need a way to configure services that is type safe, easy to use and flexible. The other one is that there is no way for users to fetch the config of a service after it's built. This limitation complicates the dynamic modification of a service's root path for the user.
+
+Our users have to wrap all our configs into an enum and store it in their own struct:
+
+```rust
+pub enum StorageParams {
+    Azblob(StorageAzblobConfig),
+    Fs(StorageFsConfig),
+    Ftp(StorageFtpConfig),
+    Gcs(StorageGcsConfig),
+    Hdfs(StorageHdfsConfig),
+    Http(StorageHttpConfig),
+    Ipfs(StorageIpfsConfig),
+    Memory,
+    Moka(StorageMokaConfig),
+    Obs(StorageObsConfig),
+    Oss(StorageOssConfig),
+    S3(StorageS3Config),
+    Redis(StorageRedisConfig),
+    Webhdfs(StorageWebhdfsConfig),
+    Cos(StorageCosConfig),
+}
+```
+
+So I propose to expose services config to the users, allowing them to work on config structs directly and fetch the config at runtime.
+
+# Guide-level explanation
+
+First of all, we will add config struct for each service. For example, `Fs` will have a `FsConfig` struct and `S3` will have a `S3Config`. The fields within the config struct are public and marked as non-exhaustive.
+
+```rust
+#[non_exhaustive]
+pub struct S3Config {
+  pub root: Option<String>,
+  pub bucket: String,
+  pub endpoint: Option<String>,
+  pub region: Option<String>,
+  ...
+}
+```
+
+Then, we will add a `Config` enum that contains all the config structs. The enum is public and non-exhaustive too.
+
+```rust
+#[non_exhaustive]
+pub enum Config {
+  Fs(FsConfig)
+  S3(S3Config),
+  Custom(&'static str, HashMap<String, String>),
+}
+```
+
+Notably, a `Custom` variant will be added to the enum. This variant aligns with `Scheme::Custom(name)` and allows users to configure custom services.
+
+At `Operator` level, we will add `from_config` and `via_config` methods.
+
+```rust
+impl Operator {
+  pub fn via_config(cfg: impl Into<Config>) -> Result<Operator> {}
+}
+```
+
+Additionally, `OperatorInfo` will introduce a new API method, `config()`:
+
+```rust
+impl OperatorInfo {
+  pub fn config(&self) -> Config {}
+}
+```
+
+Users can use `config()` to fetch the config of a service at runtime and construct a new operator based on needs.

Review Comment:
   Yes. Calling `Serialize` on `Config` will allow users to fetch the secrets during runtime. We should make it clear in the docs of `Config` and encourage uses of services like AWS IAM instead of static secrets.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org