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

feat(services/sftp): setup integration tests #2192

Merged
merged 67 commits into from
May 14, 2023
Merged

feat(services/sftp): setup integration tests #2192

merged 67 commits into from
May 14, 2023

Conversation

silver-ymz
Copy link
Member

@silver-ymz silver-ymz commented May 2, 2023

Impl #2187

Update

  • add integration tests
  • set control directory to avoid temp files in root directory when panic
  • fix root directory not exists bug. Previously, if root directory doesn't exist in the server, it will panic. Now, it will create it firstly.
  • add known_hosts_strategy support

@suyanhanx
Copy link
Member

Hi, I think it's better to use docker to do this.🤔

@silver-ymz
Copy link
Member Author

If we use docker, we should generate key files and put them into the container. But it seems that we cannot run commands before building a container.🤔

@suyanhanx
Copy link
Member

suyanhanx commented May 2, 2023

You could just mount them as a volume. Then we can generate keys first.

@Xuanwo
Copy link
Member

Xuanwo commented May 2, 2023

Invalid workflow file: .github/workflows/service_test_sftp.yml#L51
The workflow is not valid. .github/workflows/service_test_sftp.yml (Line: 51, Col: 9): Unexpected value 'command'

@Xuanwo
Copy link
Member

Xuanwo commented May 2, 2023

Let's take a look over the container setup.

Xuanwo added 2 commits May 2, 2023 21:08
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo
Copy link
Member

Xuanwo commented May 2, 2023

Seems the writer will hang forever on writing.

@NobodyXu
Copy link
Contributor

NobodyXu commented May 9, 2023

It seems that read_task exit unexceptly in https://github.com/openssh-rust/openssh-sftp-client/blob/main/src/tasks.rs#L204-L231. I use lldb to capture that it runs into defer, but it doesn't exit by break Ok(());. Also, the value of shutdown_stage is still 0. Can I ask is there any other way to get out of this loop? @NobodyXu

Yes, the function uses ? to propagate errors, so it can only an error when reading from sftp-server.

@silver-ymz Can you turn on tracing for logging?
I have annotated the read task to print the error on return, but I'm not sure whether feature tracing/tracing-log would actually print these instruments.

@silver-ymz
Copy link
Member Author

silver-ymz commented May 9, 2023

😂The git operation in github is down. It need some time.

In my local machine, the error log is 2023-05-09T10:52:26.683528Z ERROR read_task{read_end_buffer_size=1024}: openssh_sftp_client::tasks: error=The response id 11159 is invalid.

@NobodyXu
Copy link
Contributor

NobodyXu commented May 9, 2023

In my local machine, the error log is 2023-05-09T10:52:26.683528Z ERROR read_task{read_end_buffer_size=1024}: openssh_sftp_client::tasks: error=The response id 11159 is invalid.

Thank you, it seems like either the sftp-server has sent an invalid response id or openssh-sftp-client has some bugs in dealing with response-id.

Can you try if this can be reproduced by latest ssh and sftp (to make sure this isn't a bug that is already fixed)?

@silver-ymz
Copy link
Member Author

silver-ymz commented May 9, 2023

This can be reproduced with OpenSSH_9.3p1, OpenSSL 3.0.2 15 Mar 2022
BTW, openssh version in github action is OpenSSH_8.4p1 Debian-5+deb11u1, OpenSSL 1.1.1n 15 Mar 2022

And it seems that if opening with_test_writer in tracing log, it will miss lots of log from sftp-client.

@silver-ymz
Copy link
Member Author

@NobodyXu
Copy link
Contributor

NobodyXu commented May 9, 2023

@silver-ymz Thanks I will have a look at this tomorrow.

@NobodyXu
Copy link
Contributor

In https://github.com/apache/incubator-opendal/actions/runs/4926523811/jobs/8802203741?pr=2192#step:5:45738, it reproduces this bug.

I think this could be a bug in https://github.com/NobodyXu/concurrent_arena , a project I implemented long ago.

@silver-ymz Can you please audit/review the crate if you have time?
I am a bit busy recently and since I wrote the code, I might not spot the bug in it.

I really want somebody else to also review it since it uses quite a few unsafe and if you'd like, I can also add you as a maintainer to concurrent_arena and openssh

@silver-ymz
Copy link
Member Author

Ok, I'd like to review it.

Signed-off-by: silver-ymz <yinmingzhuo@gmail.com>
Signed-off-by: silver-ymz <yinmingzhuo@gmail.com>
Signed-off-by: silver-ymz <yinmingzhuo@gmail.com>
@silver-ymz
Copy link
Member Author

With openssh-rust/openssh-sftp-client#77, it can pass the tests.

Signed-off-by: silver-ymz <yinmingzhuo@gmail.com>
@silver-ymz silver-ymz requested a review from Xuanwo May 13, 2023 16:23
@silver-ymz
Copy link
Member Author

@Xuanwo I think all things have completed. Could you make the final review?

Copy link
Member

@suyanhanx suyanhanx left a comment

Choose a reason for hiding this comment

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

Thanks for your patience and hard work!

Copy link
Member

@PsiACE PsiACE left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@Xuanwo Xuanwo merged commit a15b9df into apache:main May 14, 2023
41 checks passed
@silver-ymz silver-ymz deleted the feat/add-sftp-ci branch May 14, 2023 07:05
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.

None yet

6 participants