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

enhancement: make security test timeout configurable via env variable #512

Closed
wants to merge 1 commit into from
Closed

enhancement: make security test timeout configurable via env variable #512

wants to merge 1 commit into from

Conversation

raydwaipayan
Copy link

Security Test timeouts are currently hardcoded in API's
config.yml.

Provide user options to use a environment variable
HUSKYCI_CLIENT_TESTS_TIMEOUT in the client code. All requests
send to the API henceforth will contain that timeout.

On the API side, if a non zero custom timeout is detected, it
shall override the default timeouts for the security tests.

Signed-off-by: Dwaipayan Ray dwaipayanray1@gmail.com

Description

Adds support for setting Security Test timeout from the client side
using environment variables.

Closes #509

Proposed Changes

Add custom timeout handling logic in both the api and client. Timeout is
completely optional and no function signatures were changed. So, there
are no breaking changes.

Testing

make install
export HUSKYCI_CLIENT_TESTS_TIMEOUT=1
make run-client-linux

Security Test timeouts are currently hardcoded in API's
config.yml.

Provide user options to use a environment variable
HUSKYCI_CLIENT_TESTS_TIMEOUT in the client code. All requests
send to the API henceforth will contain that timeout.

On the API side, if a non zero custom timeout is detected, it
shall override the default timeouts for the security tests.

Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
@raydwaipayan
Copy link
Author

@rafaveira3 Review please! :)

@rafaveira3 rafaveira3 self-requested a review October 5, 2020 11:48
Copy link
Contributor

@rafaveira3 rafaveira3 left a comment

Choose a reason for hiding this comment

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

Hello, @raydwaipayan! Thanks a lot for your time working on this issue. I have just checked out your branch and was not able to limit the time securityTests are running. This is what I did:

export HUSKYCI_CLIENT_TESTS_TIMEOUT="1"
. .env && make run-client

And the securityTests did not exit after 1 second. Am I doing something wrong? 🙃

I updated the issue with some new tips on how you can tackle this using the timeout command similar with what we are doing with Gitleaks:

@rafaveira3 rafaveira3 self-assigned this Oct 5, 2020
@raydwaipayan
Copy link
Author

Hi @rafaveira3,
I think the timeout in huskydocker.go doesn't work.

The waitContainer function in api.go takes the
timeOutInSeconds variable, but does nothing with it.

Is it perhaps an unimplemented feature?

@rafaveira3
Copy link
Contributor

Indeed, @raydwaipayan! You are totally right. You rock! 🚀

Unfortunately, we are not using this feature with this input yet. We need to work on this later on. 😅

However, for this particular issue, we need to provide our clients (not the API) the ability to set the timeout on their side. We kinda did this (hard-coded at this moment) using the timeout command here.

I was wondering if could use the same logic we are using to handle this %GIT_PRIVATE_SSH_KEY% in the API side. What do you think?

@raydwaipayan
Copy link
Author

@rafaveira3 Since the docker api is being called directly by the program, I think we can't use the timeout command after the api server is started. Probably we have to use something which is supplied by docker's api. Or write our own timeout handler that is.

So the best bet is to either harcode everything or just fix the docker's timeout handling. You could open an issue on that. After it's done we could check this again and it should probably run fine :)

@Krlier Krlier self-requested a review October 6, 2020 20:31
@Krlier
Copy link
Contributor

Krlier commented Oct 6, 2020

@raydwaipayan, I believe that what @rafaveira3 is trying to say is that we can substitute the timeout argument in the config.yml file for each new container being started. This could be pretty much the same way as the %GIT_PRIVATE_SSH_KEY% variable.

In this scenario, we should create another method (HandleTimeoutSubstitution()?), such as this one, to handle the timeout substitution, making it configurable through an environment variable to be supplied by the client.

Other than that, we should also do a timeout error handling, such as this one.

After that, another change you may need to do is to add some logic, similar to this one, so that the security test can handle properly the timeout output.

What do you say we start adding these changes by replacing how GitLeaks does it since it's already hard coded? 🙂

@Krlier Krlier self-assigned this Oct 6, 2020
@raydwaipayan
Copy link
Author

@Krlier True I looked into it and it seems doable. I will try to draw something up :)

@Krlier
Copy link
Contributor

Krlier commented Oct 30, 2020

@Krlier True I looked into it and it seems doable. I will try to draw something up :)

Hey, @raydwaipayan!

How's it going? Did you come up with something?

Do you need any help?

@rafaveira3 rafaveira3 removed their assignment Oct 3, 2022
@fguisso fguisso closed this Oct 19, 2023
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.

Make SecurityTests' timeout configurable
4 participants