feat(new sink): Add sinks sftp support #18076

Xuanwo · 2023-07-25T07:19:16Z

This PR is part of #3382.

Signed-off-by: Xuanwo <github@xuanwo.io>

netlify · 2023-07-25T07:19:21Z

✅ Deploy Preview for vector-project canceled.

Name	Link
🔨 Latest commit	`e3298a4`
🔍 Latest deploy log	https://app.netlify.com/sites/vector-project/deploys/64c47a99dbacde0008003b39

netlify · 2023-07-25T07:19:26Z

✅ Deploy Preview for vrl-playground canceled.

Name	Link
🔨 Latest commit	`e3298a4`
🔍 Latest deploy log	https://app.netlify.com/sites/vrl-playground/deploys/64c47a99d5723800081c3c69

gaby · 2023-07-28T12:26:24Z

@Xuanwo The tokio-utils update has been merged:

Rel #18078

jszwedko · 2023-07-29T00:32:53Z

Are you still seeing the compilation errors with io-lifetimes? I'm actually seeing different compilation errors related to the code in this branch.

Thanks for opening this to get the conversation started! We'll get a more thorough review soon.

Signed-off-by: Xuanwo <github@xuanwo.io>

Xuanwo · 2023-07-29T02:35:31Z

I'm actually seeing different compilation errors related to the code in this branch.

Thanks for the remind from @gaby, all build issues have been fixed.

Thanks for opening this to get the conversation started! We'll get a more thorough review soon.

Let's confirm the correct direction before pushing any works to docs and configs.

StephenWakely · 2023-08-08T12:23:05Z

@Xuanwo, just to confirm, if we merge this will you be willing to continue supporting this sink in future?

Xuanwo · 2023-08-08T12:39:20Z

@Xuanwo, just to confirm, if we merge this will you be willing to continue supporting this sink in future?

This PR is far away from been merged. So I'm guessing we are talking the future: when this PR is ready and sftp is able to use, will I keep supporting sftp sink?

My answer is YES. As you may know, both webhdfs and sftp are OpenDAL based sinks. Most logic are vector internal and not related to sinks.

And sink related things breaking means OpenDAL is broken. In this case, I will make sure it fixed so that all opendal users will benefit from the fix too.

Xuanwo · 2023-08-08T12:43:40Z

As you may know, both webhdfs and sftp are OpenDAL based sinks.

There is an ongoing discussion on adding a doc about how to implement and maintain sinks that based opendal.

#15715 (comment)

StephenWakely · 2023-08-08T12:47:30Z

when this PR is ready and sftp is able to use, will I keep supporting sftp sink?

Yes. Thank you.

dsmith3197

Overall, this looks good to me once the comments are addressed.

I would like to see us add an integration test for this sink, similar to the webhdfs sink.

Also, can you run make generate-component-docs to update the generated cue docs to add the sftp to the user-facing documentation? We'll also want to add a note that this only works on unix systems, as mentioned in the crate's docs.

dsmith3197 · 2023-08-21T16:32:08Z

src/sinks/sftp/mod.rs

+//! `sftp` sink.
+//!
+//! `sftp` SFTP, or Secure File Transfer Protocol, is a network protocol used for
+//! securely transferring files over the internet. It operates over the Secure
+//! Shell (SSH) data stream, providing secure file transfer by both encrypting
+//! the data and maintaining the integrity of the transfer. SFTP also supports
+//! file management operations like moving and deleting files on the server, unlike FTP.
+//!
+//! For more information, please refer to:
+//!
+//! - [sftp(1) — Linux manual page](https://man7.org/linux/man-pages/man1/sftp.1.html)
+//!
+//! `sftp` is an OpenDal based services. This mod itself only provide config to build an
+//! [`crate::sinks::opendal_common::OpenDalSink`]. All real implement are powered by
+//! [`crate::sinks::opendal_common::OpenDalSink`].


Thanks for adding a module doc! I have a few small suggestions.

Suggested change

//! `sftp` sink.

//!

//! `sftp` SFTP, or Secure File Transfer Protocol, is a network protocol used for

//! securely transferring files over the internet. It operates over the Secure

//! Shell (SSH) data stream, providing secure file transfer by both encrypting

//! the data and maintaining the integrity of the transfer. SFTP also supports

//! file management operations like moving and deleting files on the server, unlike FTP.

//!

//! For more information, please refer to:

//!

//! - [sftp(1) — Linux manual page](https://man7.org/linux/man-pages/man1/sftp.1.html)

//!

//! `sftp` is an OpenDal based services. This mod itself only provide config to build an

//! [`crate::sinks::opendal_common::OpenDalSink`]. All real implement are powered by

//! [`crate::sinks::opendal_common::OpenDalSink`].

//! `sftp` sink.

//!

//! SFTP, or Secure File Transfer Protocol, is a network protocol used for

//! securely transferring files over the internet. It operates over the Secure

//! Shell (SSH) data stream, providing secure file transfer by both encrypting

//! the data and maintaining the integrity of the transfer. SFTP also supports

//! file management operations like moving and deleting files on the server, unlike FTP.

//!

//! For more information, please refer to:

//!

//! - [sftp(1) — Linux manual page](https://man7.org/linux/man-pages/man1/sftp.1.html)

//!

//! `sftp` is an OpenDal-based sink. This module itself only provide config to build an

//! [`crate::sinks::opendal_common::OpenDalSink`].

dsmith3197 · 2023-08-21T16:39:58Z

src/sinks/sftp/config.rs

+use vector_config::configurable_component;
+use vector_core::{
+    config::{AcknowledgementsConfig, DataType, Input},
+    sink::VectorSink,
+};
+
+use crate::{
+    codecs::{Encoder, EncodingConfigWithFraming, SinkType},
+    config::{GenerateConfig, SinkConfig, SinkContext},
+    sinks::{
+        opendal_common::*,
+        util::{
+            partitioner::KeyPartitioner, BatchConfig, BulkSizeBasedDefaultBatchSettings,
+            Compression,
+        },
+        Healthcheck,
+    },
+};


Small nit - we can remove some of the common imports by including crate::sinks::prelude::*.

Suggested change

use vector_config::configurable_component;

use vector_core::{

config::{AcknowledgementsConfig, DataType, Input},

sink::VectorSink,

};

use crate::{

codecs::{Encoder, EncodingConfigWithFraming, SinkType},

config::{GenerateConfig, SinkConfig, SinkContext},

sinks::{

opendal_common::*,

util::{

partitioner::KeyPartitioner, BatchConfig, BulkSizeBasedDefaultBatchSettings,

Compression,

},

Healthcheck,

},

};

use vector_core::config::DataType;

use crate::{

codecs::{EncodingConfigWithFraming, SinkType},

sinks::{

prelude::*,

opendal_common::*,

util::{

partitioner::KeyPartitioner, BulkSizeBasedDefaultBatchSettings,

},

},

};

dsmith3197 · 2023-08-21T16:45:52Z

src/sinks/sftp/config.rs

+};
+
+/// Configuration for the `sftp` sink.
+#[configurable_component(sink("sftp", "Sftp."))]


Can we add a more detailed description to this component (the second string)? This description ends up getting populated in the docs at vector.dev.

Suggested change

#[configurable_component(sink("sftp", "Sftp."))]

#[configurable_component(sink("sftp", "Secure File Transfer Protocol (SFTP)."))]

dsmith3197 · 2023-08-21T16:53:35Z

src/sinks/sftp/config.rs

+#[derive(Clone, Debug)]
+#[serde(deny_unknown_fields)]
+pub struct SftpConfig {
+    /// The root path for Sftp.


I would suggest adopting the language from the opendal sftp docs here to make these easier for Vector users to configure.

Suggested change

/// The root path for Sftp.

/// The working directory for the backend. Defaults to the default directory

/// set by the remote SFTP server.

dsmith3197 · 2023-08-21T17:02:49Z

src/sinks/sftp/config.rs

+    #[configurable(metadata(docs::templateable))]
+    pub prefix: String,
+
+    /// The endpoint to connect to sftp.


Suggested change

/// The endpoint to connect to sftp.

/// The endpoint of the SFTP backend.

dsmith3197 · 2023-08-21T17:17:50Z

src/sinks/sftp/test.rs

+}
+
+#[test]
+fn Sftp_build_request() {


Suggested change

fn Sftp_build_request() {

fn sftp_build_request() {

dsmith3197 · 2023-08-21T17:20:04Z

src/sinks/sftp/test.rs

+fn request_builder(sink_config: &SftpConfig) -> OpenDalRequestBuilder {
+    let transformer = sink_config.encoding.transformer();
+    let (framer, serializer) = sink_config
+        .encoding
+        .build(SinkType::MessageBased)
+        .expect("encoding must build with success");
+    let encoder = Encoder::<Framer>::new(framer, serializer);
+
+    OpenDalRequestBuilder {
+        encoder: (transformer, encoder),
+        compression: sink_config.compression,
+    }
+}


Rather than re-implement this logic here, we should extract the corresponding logic out of SftpConfig::build_processor into a separate method so that we can test it in isolation.

dsmith3197 · 2023-08-21T17:20:16Z

src/sinks/sftp/test.rs

+}
+
+#[test]
+fn Sftp_generate_config() {


Suggested change

fn Sftp_generate_config() {

fn sftp_generate_config() {

dsmith3197 · 2023-08-21T17:22:31Z

src/sinks/sftp/test.rs

+    let req = build_request(Compression::None);
+    assert!(req.metadata.partition_key.ends_with(".log"));
+
+    let req = build_request(Compression::None);
+    assert!(req.metadata.partition_key.ends_with(".log"));


These lines seem to be redundant.

Suggested change

let req = build_request(Compression::None);

assert!(req.metadata.partition_key.ends_with(".log"));

let req = build_request(Compression::None);

assert!(req.metadata.partition_key.ends_with(".log"));

let req = build_request(Compression::None);

assert!(req.metadata.partition_key.ends_with(".log"));

dsmith3197 · 2023-08-21T17:27:52Z

src/sinks/mod.rs

@@ -87,6 +87,8 @@ pub mod redis;
 pub mod s3_common;
 #[cfg(feature = "sinks-sematext")]
 pub mod sematext;
+#[cfg(feature = "sinks-sftp")]


We should also gate this behind unix as the sftp opendal service states that it only works for unix.

Suggested change

#[cfg(feature = "sinks-sftp")]

#[cfg(all(unix, feature = "sinks-sftp"))]

Xuanwo · 2023-08-22T05:08:03Z

Thanks for the review! I will come back next week.

gaby · 2023-11-22T05:41:08Z

Thanks for the review! I will come back next week.

@Xuanwo Any updates on this?

gaby · 2024-02-27T05:37:42Z

@Xuanwo Do you have plans for updating this PR?

gaby · 2024-07-03T13:50:34Z

@dsmith3197 This can probably be closed. The author hasnt responded in months.

jszwedko · 2024-07-03T13:53:57Z

I think we can leave it open until someone else picks up the torch so that they have a base to start from. Otherwise someone might start from scratch, not realizing some efforts have been made already. I'll mark it as draft though.

feat: Add sinks sftp support

bd86fbf

Signed-off-by: Xuanwo <github@xuanwo.io>

Xuanwo requested a review from a team July 25, 2023 07:19

github-actions bot added the domain: sinks Anything related to the Vector's sinks label Jul 25, 2023

Xuanwo mentioned this pull request Jul 25, 2023

add support for sftp source and sink #3382

Open

neuronull assigned StephenWakely Jul 25, 2023

Xuanwo added 2 commits July 29, 2023 10:22

Merge branch 'master' into sinks-sftp

23426d0

Fix build

e3298a4

Signed-off-by: Xuanwo <github@xuanwo.io>

dsmith3197 self-assigned this Aug 21, 2023

dsmith3197 suggested changes Aug 21, 2023

View reviewed changes

dsmith3197 added the meta: awaiting author Pull requests that are awaiting their author. label Sep 1, 2023

gaby mentioned this pull request Mar 26, 2024

New syslog sink #6863

Open

jszwedko marked this pull request as draft July 3, 2024 13:54

jszwedko mentioned this pull request Sep 3, 2024

New sink: postgres #15765

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(new sink): Add sinks sftp support #18076

feat(new sink): Add sinks sftp support #18076

Xuanwo commented Jul 25, 2023 •

edited

Loading

netlify bot commented Jul 25, 2023 •

edited

Loading

netlify bot commented Jul 25, 2023 •

edited

Loading

gaby commented Jul 28, 2023

jszwedko commented Jul 29, 2023

Xuanwo commented Jul 29, 2023

StephenWakely commented Aug 8, 2023

Xuanwo commented Aug 8, 2023 •

edited

Loading

Xuanwo commented Aug 8, 2023 •

edited

Loading

StephenWakely commented Aug 8, 2023

dsmith3197 left a comment

dsmith3197 Aug 21, 2023

dsmith3197 Aug 21, 2023

dsmith3197 Aug 21, 2023 •

edited

Loading

dsmith3197 Aug 21, 2023 •

edited

Loading

dsmith3197 Aug 21, 2023

dsmith3197 Aug 21, 2023

dsmith3197 Aug 21, 2023

dsmith3197 Aug 21, 2023

dsmith3197 Aug 21, 2023

dsmith3197 Aug 21, 2023 •

edited

Loading

Xuanwo commented Aug 22, 2023 •

edited

Loading

gaby commented Nov 22, 2023

gaby commented Feb 27, 2024

gaby commented Jul 3, 2024

jszwedko commented Jul 3, 2024

	#[configurable_component(sink("sftp", "Sftp."))]
	#[configurable_component(sink("sftp", "Secure File Transfer Protocol (SFTP)."))]

	/// The root path for Sftp.
	/// The working directory for the backend. Defaults to the default directory
	/// set by the remote SFTP server.

	/// The endpoint to connect to sftp.
	/// The endpoint of the SFTP backend.

	#[cfg(feature = "sinks-sftp")]
	#[cfg(all(unix, feature = "sinks-sftp"))]

feat(new sink): Add sinks sftp support #18076

Are you sure you want to change the base?

feat(new sink): Add sinks sftp support #18076

Conversation

Xuanwo commented Jul 25, 2023 • edited Loading

netlify bot commented Jul 25, 2023 • edited Loading

✅ Deploy Preview for vector-project canceled.

netlify bot commented Jul 25, 2023 • edited Loading

✅ Deploy Preview for vrl-playground canceled.

gaby commented Jul 28, 2023

jszwedko commented Jul 29, 2023

Xuanwo commented Jul 29, 2023

StephenWakely commented Aug 8, 2023

Xuanwo commented Aug 8, 2023 • edited Loading

Xuanwo commented Aug 8, 2023 • edited Loading

StephenWakely commented Aug 8, 2023

dsmith3197 left a comment

Choose a reason for hiding this comment

dsmith3197 Aug 21, 2023

Choose a reason for hiding this comment

dsmith3197 Aug 21, 2023

Choose a reason for hiding this comment

dsmith3197 Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

dsmith3197 Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

dsmith3197 Aug 21, 2023

Choose a reason for hiding this comment

dsmith3197 Aug 21, 2023

Choose a reason for hiding this comment

dsmith3197 Aug 21, 2023

Choose a reason for hiding this comment

dsmith3197 Aug 21, 2023

Choose a reason for hiding this comment

dsmith3197 Aug 21, 2023

Choose a reason for hiding this comment

dsmith3197 Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

Xuanwo commented Aug 22, 2023 • edited Loading

gaby commented Nov 22, 2023

gaby commented Feb 27, 2024

gaby commented Jul 3, 2024

jszwedko commented Jul 3, 2024

Xuanwo commented Jul 25, 2023 •

edited

Loading

netlify bot commented Jul 25, 2023 •

edited

Loading

netlify bot commented Jul 25, 2023 •

edited

Loading

Xuanwo commented Aug 8, 2023 •

edited

Loading

Xuanwo commented Aug 8, 2023 •

edited

Loading

dsmith3197 Aug 21, 2023 •

edited

Loading

dsmith3197 Aug 21, 2023 •

edited

Loading

dsmith3197 Aug 21, 2023 •

edited

Loading

Xuanwo commented Aug 22, 2023 •

edited

Loading