Skip to content

feat: introduce as file uri when ref secret #22227

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

Closed
wants to merge 3 commits into from
Closed

Conversation

tabVersion
Copy link
Contributor

@tabVersion tabVersion commented Jun 13, 2025

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

in current impl, fill_secret returns the absolute path, but most sdk requires file://<absolute-path>

What's changed and what's your intention?

Checklist

  • I have written necessary rustdoc comments.
  • I have added necessary unit tests and integration tests.
  • I have added test labels as necessary.
  • I have added fuzzing tests or opened an issue to track them.
  • My PR contains breaking changes.
  • My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
  • I have checked the Release Timeline and Currently Supported Versions to determine which release branches I need to cherry-pick this PR into.

Documentation

  • My PR needs documentation updates.
Release note

introduce AS FILE URI when referencing a secret, which gives file://<absolute path> for a file

@github-actions github-actions bot added the type/fix Type: Bug fix. Only for pull requests. label Jun 13, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR ensures that secret file references include a file:// URI scheme as required by most SDKs.

  • Wraps the local secret file path with file:// when RefAsType::File is used.
Comments suppressed due to low confidence (3)

src/common/secret/src/secret_manager.rs:169

  • On Windows, file URIs typically require a triple slash (file:///C:/path) rather than file://C:/path. Consider using the url crate or a platform-aware helper to construct valid file URIs across operating systems.
Ok(format("file://{}", path_str))

src/common/secret/src/secret_manager.rs:169

  • Add unit tests to verify that fill_secret returns a properly prefixed file:// URL for secret file paths, including edge cases such as Windows paths and special characters.
Ok(format("file://{}", path_str))

src/common/secret/src/secret_manager.rs:166

  • Update the function-level documentation for fill_secret (or its public API) to indicate that the returned path now includes a file:// prefix.
RefAsType::File => {

@yuhao-su
Copy link
Contributor

AFAIK there are quite a few properties in rdkafka requires path instead of uri https://docs.confluent.io/platform/current/clients/librdkafka/html/md_CONFIGURATION.html

This can be a breaking change

most sdk requires file://

Can you elaborate?

@tabVersion
Copy link
Contributor Author

AFAIK there are quite a few properties in rdkafka requires path instead of uri docs.confluent.io/platform/current/clients/librdkafka/html/md_CONFIGURATION.html

This can be a breaking change

make sense, kafka requires an absolute path.
But pulsar oauth requires an uri ref, and serves as an exception.

Maybe a proper approach here is introducing as uri for secret.

This reverts commit 32a5272.
@tabVersion tabVersion marked this pull request as draft June 14, 2025 04:35
@tabVersion tabVersion changed the title fix: return a file:// prefix when ref secret file feat: introduce as uri when ref secret Jun 14, 2025
@github-actions github-actions bot added the type/feature Type: New feature. label Jun 14, 2025
@tabVersion tabVersion changed the title feat: introduce as uri when ref secret feat: introduce as file uri when ref secret Jun 15, 2025
@tabVersion tabVersion marked this pull request as ready for review June 15, 2025 07:07
@hzxa21
Copy link
Collaborator

hzxa21 commented Jun 16, 2025

AFAIK there are quite a few properties in rdkafka requires path instead of uri docs.confluent.io/platform/current/clients/librdkafka/html/md_CONFIGURATION.html
This can be a breaking change

make sense, kafka requires an absolute path. But pulsar oauth requires an uri ref, and serves as an exception.

Maybe a proper approach here is introducing as uri for secret.

If we know pulsar is an exception, can we add the file:// prefix to the properties that need file secrets only for pulsar instead of introducing a new syntax. IMO, it may cause confusion for users to choose between AS FILE and AS FILE URI.

@tabVersion
Copy link
Contributor Author

AFAIK there are quite a few properties in rdkafka requires path instead of uri docs.confluent.io/platform/current/clients/librdkafka/html/md_CONFIGURATION.html
This can be a breaking change

make sense, kafka requires an absolute path. But pulsar oauth requires an uri ref, and serves as an exception.
Maybe a proper approach here is introducing as uri for secret.

If we know pulsar is an exception, can we add the file:// prefix to the properties that need file secrets only for pulsar instead of introducing a new syntax. IMO, it may cause confusion for users to choose between AS FILE and AS FILE URI.

after checking the existing codebase, the only one requires uri is MQTT

    /// Path to client's certificate file (PEM). Required for client authentication.
    /// Can be a file path under fs:// or a string with the certificate content.
    #[serde(rename = "tls.client_cert")]
    pub client_cert: Option<String>,

    /// Path to client's private key file (PEM). Required for client authentication.
    /// Can be a file path under fs:// or a string with the private key content.
    #[serde(rename = "tls.client_key")]
    pub client_key: Option<String>,

and that's because of the community contributor wrote the load cert process himself.

Let's see the pulsar issue as an exception and do hack here

@tabVersion tabVersion closed this Jun 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feature Type: New feature. type/fix Type: Bug fix. Only for pull requests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants