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

backend/s3: Anonymous access not supported #54

Closed
Tracked by #65
BohuTANG opened this issue Feb 24, 2022 · 8 comments · Fixed by #97
Closed
Tracked by #65

backend/s3: Anonymous access not supported #54

BohuTANG opened this issue Feb 24, 2022 · 8 comments · Fixed by #97

Comments

@BohuTANG
Copy link

BohuTANG commented Feb 24, 2022

Background

The s3://repo.databend.rs/dataset/stateful/ontime.csv ACL is Everyone (public access) on AWS S3.

If the credentials not set for OpenDAL:
image

will get the error:

displayText = Parse csv error at line 0, cause: unexpected: (op: read, path: /dataset/stateful/ontime.csv, source: failed to construct request: No credentials in the property bag

Enhancement
It would be better to add a test for minio bucket with public ACL.

@Xuanwo Xuanwo changed the title Anonymous access: displayText = Parse csv error at line 0, cause: unexpected: (op: read, path: /dataset/stateful/ontime.csv, source: failed to construct request: No credentials in the property bag Service s3 doesn't support anonymous access Feb 24, 2022
@BohuTANG

This comment was marked as resolved.

@Xuanwo
Copy link
Member

Xuanwo commented Feb 24, 2022

export S3_STORAGE_ENDPOINT_URL=https://s3.amazonaws.com

It's buggy now.

The behavior between keeping S3_STORAGE_ENDPOINT_URL empty and setting S3_STORAGE_ENDPOINT_URL=https://s3.amazonaws.com/ is different.

  • S3_STORAGE_ENDPOINT_URL="": s3 SDK will use format!("s3.{}.amazonaws.com", region) to construct the correct endpoint.
  • S3_STORAGE_ENDPOINT_URL=https://s3.amazonaws.com/: s3 SDK will take this input as static, and can't handle the region correctly.

I'm considering removing the region option so that:

  • For AWS S3, we can always use https://s3.amazonaws.com/ and detect the region for users.
  • For other services, we can let user input the correct endpoint like http://127.0.0.1:9000

@Xuanwo Xuanwo changed the title Service s3 doesn't support anonymous access backend/s3: Anonymous access not supported Feb 24, 2022
@BohuTANG
Copy link
Author

Thank you.

The attempting to access must be addressed using the specified endpoint error issue already addressed in:
#37

@Xuanwo
Copy link
Member

Xuanwo commented Feb 24, 2022

A bit complex but it is possible.

SDK's maintainer gives a workaround: awslabs/aws-sdk-rust#425 (comment)

And I think we can inject the middleware to make it better:

let builder = aws_smithy_client::Builder::dyn_https()
    .middleware(AwsS3::middleware::DefaultMiddleware::new()) <-- inject middleware here.
    .build();

Ok(Arc::new(Backend {
    // Make `/` as the default of root.
    root,
    bucket: self.bucket.clone(),
    client: AwsS3::Client::with_config(builder, cfg.build()),
}))

We can config OperationSigningConfig.signing_requirements to SigningRequirements::Disabled maybe.

But it may take some time to figure out how to make it works.

@BohuTANG
Copy link
Author

Thanks.
Maybe we can prioritize this issue #12 first, so that databend side can continue to list all files and read them for COPY command.

@Xuanwo
Copy link
Member

Xuanwo commented Mar 1, 2022

Maybe we can use:

let client = aws_sdk_s3::Client::with_config(
    aws_sdk_s3::client::Builder::new()
        .connector(DynConnector::new(my_custom_connector)) // Now with `DynConnector`
        .middleware(DynMiddleware::new(my_custom_middleware)) // Now with `DynMiddleware`
        .default_async_sleep()
        .build(),
    config
);

Read https://github.com/awslabs/aws-sdk-rust/releases/tag/v0.7.0 for more details.

@Xuanwo
Copy link
Member

Xuanwo commented Mar 3, 2022

Just to note, here is my today's exploration, I get the props from the request successfully:

use aws_smithy_client::{Builder, Client};

let hyper_connector = aws_smithy_client::hyper_ext::Adapter::builder()
    .build(aws_smithy_client::conns::https());
// let connector = DynConnector::new()

let x = aws_smithy_client::Builder::new()
    .connector(hyper_connector)
    .middleware(aws_smithy_client::erase::DynMiddleware::new(
        tower::util::MapRequestLayer::new(|req: aws_smithy_http::operation::Request| {
            let (inner, mut props) = req.into_parts();

            {
                let mut p = props.acquire_mut();
                let x = p.get_mut::<usize>();
                println!("{:?}", x);
            }

            aws_smithy_http::operation::Request::from_parts(inner, props)
        }),
    ))
    .build();

let _ = aws_sdk_s3::Client::with_config(x.into_dyn(), cfg.build());

@Xuanwo
Copy link
Member

Xuanwo commented Mar 3, 2022

It works now!

image

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 a pull request may close this issue.

2 participants