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

[LLT-5831] QNAP: move cgi logic to teliod #1012

Merged
merged 6 commits into from
Dec 23, 2024

Conversation

lcruz99
Copy link
Contributor

@lcruz99 lcruz99 commented Dec 4, 2024

Problem

CGI consists in a bash script that handles http requests and directly control teliod daemon execution.
As in the near future more features (eg: authentication) are to be added, after internal discussions it was decided that would be more secure and easy to maintain/implement if the handling of the requests is moved to the teliod binary.

Solution

Make the CGI script redirect http requests to teliod.

☑️ Definition of Done checklist

  • Commit history is clean (requirements)
  • README.md is updated
  • Functionality is covered by unit or integration tests

@lcruz99 lcruz99 requested a review from a team as a code owner December 4, 2024 17:56
@lcruz99 lcruz99 force-pushed the LLT-5831_move_cgi_logic_to_teliod branch from 4d75c96 to 03917df Compare December 4, 2024 18:06
@lcruz99 lcruz99 force-pushed the LLT-5831_move_cgi_logic_to_teliod branch from 03917df to 38938fe Compare December 4, 2024 20:49
@tomaszklak
Copy link
Contributor

tomaszklak commented Dec 5, 2024

Can we use instead something like https://docs.rs/cgi2/latest/cgi/ or https://docs.rs/cgi/0.6.0/cgi/ , so that we avoid implementing irrelevant details of cgi/http, and focus on our business logic?

Copy link
Contributor

@stalowyjez stalowyjez left a comment

Choose a reason for hiding this comment

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

imho looks strange, like someone wanted to write a shell script, but had to do it in rust for some reason

clis/teliod/src/qnap.rs Outdated Show resolved Hide resolved
clis/teliod/src/qnap.rs Outdated Show resolved Hide resolved
clis/teliod/src/main.rs Outdated Show resolved Hide resolved
clis/teliod/src/qnap.rs Outdated Show resolved Hide resolved
clis/teliod/src/qnap.rs Outdated Show resolved Hide resolved
clis/teliod/src/qnap.rs Outdated Show resolved Hide resolved
clis/teliod/src/qnap.rs Outdated Show resolved Hide resolved
clis/teliod/src/qnap.rs Outdated Show resolved Hide resolved
clis/teliod/src/qnap.rs Outdated Show resolved Hide resolved
@lcruz99
Copy link
Contributor Author

lcruz99 commented Dec 5, 2024

imho looks strange, like someone wanted to write a shell script, but had to do it in rust for some reason

@stalowyjez that was more or less what happened, but what do you think it looks strange besides other comments?

@lcruz99
Copy link
Contributor Author

lcruz99 commented Dec 5, 2024

Can we use instead something like https://docs.rs/cgi2/latest/cgi/ or https://docs.rs/cgi/0.6.0/cgi/ , so that we avoid implementing irrelevant details of cgi/http, and focus on our business logic?

@tomaszklak thanks for the recommendations, will take a look!

@stalowyjez
Copy link
Contributor

@stalowyjez that was more or less what happened, but what do you think it looks strange besides other comments?

I mean it just doesn't look like a right tool for the job to me

clis/teliod/src/main.rs Outdated Show resolved Hide resolved
clis/teliod/src/qnap.rs Outdated Show resolved Hide resolved
clis/teliod/src/main.rs Outdated Show resolved Hide resolved
clis/teliod/src/qnap.rs Outdated Show resolved Hide resolved
clis/teliod/src/qnap.rs Outdated Show resolved Hide resolved
qnap/shared/teliod.cgi Show resolved Hide resolved
clis/teliod/src/qnap.rs Outdated Show resolved Hide resolved
@lcruz99 lcruz99 force-pushed the LLT-5831_move_cgi_logic_to_teliod branch from 38938fe to 0abc522 Compare December 10, 2024 18:46
@lcruz99 lcruz99 force-pushed the LLT-5831_move_cgi_logic_to_teliod branch from 0abc522 to 687836b Compare December 10, 2024 18:52
@lcruz99 lcruz99 force-pushed the LLT-5831_move_cgi_logic_to_teliod branch from 687836b to a21cb40 Compare December 11, 2024 14:53
@lcruz99 lcruz99 force-pushed the LLT-5831_move_cgi_logic_to_teliod branch from a21cb40 to 4cd24a8 Compare December 11, 2024 15:42
@lcruz99 lcruz99 force-pushed the LLT-5831_move_cgi_logic_to_teliod branch from 4cd24a8 to 2608ac7 Compare December 11, 2024 15:54
@lcruz99 lcruz99 force-pushed the LLT-5831_move_cgi_logic_to_teliod branch from 2608ac7 to 5f8d630 Compare December 11, 2024 16:22
@lcruz99 lcruz99 force-pushed the LLT-5831_move_cgi_logic_to_teliod branch from 5f8d630 to 2657c32 Compare December 11, 2024 16:25
clis/teliod/QNAP.md Show resolved Hide resolved
clis/teliod/QNAP.md Outdated Show resolved Hide resolved
clis/teliod/QNAP.md Show resolved Hide resolved
clis/teliod/QNAP.md Show resolved Hide resolved
clis/teliod/QNAP.md Show resolved Hide resolved
clis/teliod/src/qnap.rs Show resolved Hide resolved
clis/teliod/Cargo.toml Outdated Show resolved Hide resolved
clis/teliod/src/config.rs Show resolved Hide resolved
clis/teliod/src/qnap.rs Show resolved Hide resolved
clis/teliod/src/qnap.rs Outdated Show resolved Hide resolved
clis/teliod/src/qnap.rs Outdated Show resolved Hide resolved
clis/teliod/src/qnap.rs Outdated Show resolved Hide resolved
clis/teliod/src/qnap.rs Outdated Show resolved Hide resolved
clis/teliod/src/qnap.rs Show resolved Hide resolved
clis/teliod/src/qnap.rs Outdated Show resolved Hide resolved
clis/teliod/src/qnap.rs Outdated Show resolved Hide resolved
@lcruz99 lcruz99 changed the title [LLT-5831] Move cgi logic to teliod [LLT-5831] QNAP: move cgi logic to teliod Dec 12, 2024
packgron added a commit that referenced this pull request Dec 18, 2024
Original work done in #1012.
This is restructure and reabase on common base.
packgron added a commit that referenced this pull request Dec 19, 2024
Original work done in #1012.
This is restructure and reabase on common base.
@lcruz99 lcruz99 force-pushed the LLT-5831_move_cgi_logic_to_teliod branch from 97e2acd to a696f1e Compare December 19, 2024 18:32
clis/teliod/src/main.rs Outdated Show resolved Hide resolved
clis/teliod/src/daemon.rs Show resolved Hide resolved
clis/teliod/src/daemon.rs Show resolved Hide resolved
clis/teliod/src/qnap.rs Outdated Show resolved Hide resolved
clis/teliod/src/qnap.rs Show resolved Hide resolved
clis/teliod/src/qnap.rs Show resolved Hide resolved
clis/teliod/src/qnap.rs Show resolved Hide resolved
clis/teliod/src/qnap.rs Show resolved Hide resolved
@lcruz99 lcruz99 force-pushed the LLT-5831_move_cgi_logic_to_teliod branch from a696f1e to e58d335 Compare December 20, 2024 15:51
@lcruz99 lcruz99 force-pushed the LLT-5831_move_cgi_logic_to_teliod branch from e58d335 to bf4f507 Compare December 20, 2024 15:53
@lcruz99 lcruz99 force-pushed the LLT-5831_move_cgi_logic_to_teliod branch from bf4f507 to ab80e4b Compare December 23, 2024 11:36
@lcruz99 lcruz99 force-pushed the LLT-5831_move_cgi_logic_to_teliod branch from ab80e4b to 39fd537 Compare December 23, 2024 11:38
Copy link
Contributor

@tomaszklak tomaszklak left a comment

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

@Jauler Jauler left a comment

Choose a reason for hiding this comment

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

+1.0

@lcruz99 lcruz99 merged commit b4906ff into main Dec 23, 2024
64 checks passed
@lcruz99 lcruz99 deleted the LLT-5831_move_cgi_logic_to_teliod branch December 23, 2024 12:46
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.

6 participants