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.

Sign up for GitHub

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

Jump to bottom

Add automated security policy test scripts #1194

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add automated security policy test scripts #1194

wants to merge 2 commits into from

Conversation

MichaelEizaguirre
Copy link
Contributor

These new files added allow for users to run some pre-defined security policy tests. The READ.md also reflects the changes

@MichaelEizaguirre MichaelEizaguirre requested a review from a team September 3, 2020 09:02
@MichaelEizaguirre MichaelEizaguirre requested a review from a team as a code owner September 3, 2020 09:02
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Sep 3, 2020
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 3, 2020
@istio-testing
Copy link
Contributor

Hi @MichaelEizaguirre. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@MichaelEizaguirre
Copy link
Contributor Author

@yangminzhu @carolynhu

@ericvn
Copy link
Contributor

ericvn commented Sep 3, 2020

/ok-to-test

@istio-testing istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Sep 3, 2020
nmittler
nmittler reviewed Sep 3, 2020
View reviewed changes
Copy link
Contributor

@nmittler nmittler left a comment

Choose a reason for hiding this comment

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

Why are these not standard go-based tests?

@MichaelEizaguirre
Copy link
Contributor Author

The name of these scripts may be slightly misleading as they are rather scripts that run the performance tests on certain security policies.

yangminzhu
yangminzhu reviewed Sep 3, 2020
View reviewed changes
perf/benchmark/security/generate_policies/testCases/authZPaths.sh
}
}' > authZPath1000.json
go run ../generate_policies.go ../generate.go ../jwt.go -configFile="authZPath1000.json" > authZPath1000.yaml
echo "Generated a single authZ policy with 1000 paths"
Copy link
Contributor

Choose a reason for hiding this comment

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

there are a lot of duplicate here, can we put the common part into a function?

carolynhu
carolynhu reviewed Sep 4, 2020
View reviewed changes
perf/benchmark/security/generate_policies/testCases/authZPaths.sh
kubectl apply -f authZPath10.yaml
echo "Running variable number of path rules"
echo "Running perf test with conn=8 and qps=100"
pipenv run python3 ../../../runner/runner.py --conn 8 --qps 100 --baseline --duration 240 --load_gen_type=nighthawk --telemetry_mode=none
Copy link
Contributor

Choose a reason for hiding this comment

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

please make this load_gen_type configurable, not hard coded here.

carolynhu
carolynhu reviewed Sep 4, 2020
View reviewed changes
perf/benchmark/security/generate_policies/testCases/Pipfile
bokeh = "*"
pandas = "==0.24.2"
numpy = "*"
pyyaml = "*"
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need put a Pipfile in this repo? since all test was trigger under runner/ folder. Just add the dependencies on the existing Pipfile there?

@nmittler nmittler added the do-not-merge Block automatic merging of a PR. label Sep 8, 2020
@nmittler
Copy link
Contributor

nmittler commented Sep 8, 2020

Hey guys, I've put a temporary hold on this PR. It's not clear to me why we're not creating standard Golang tests for security. Can someone explain the use of test scripts instead? Are these load/stability tests?

FYI @howardjohn

@richardwxn
Copy link
Contributor

@nmittler yep this is to add tests to run with the performance benchmark pipeline

@carolynhu
Copy link
Contributor

Hey guys, I've put a temporary hold on this PR. It's not clear to me why we're not creating standard Golang tests for security. Can someone explain the use of test scripts instead? Are these load/stability tests?

FYI @howardjohn

Hi @nmittler

  • those standard Golang tests should be in a separate PR for his generating security policy features.
  • this PR is to automate the process of running a variety of performance tests with several general use cases of security policy applied.

@yangminzhu
Copy link
Contributor

Hey guys, I've put a temporary hold on this PR. It's not clear to me why we're not creating standard Golang tests for security. Can someone explain the use of test scripts instead? Are these load/stability tests?

FYI @howardjohn

This PR is really just adding some shell scripts to make it easier to run the existing perf tests, not adding any new tests for security.

@istio-testing
Copy link
Contributor

@MichaelEizaguirre: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Oct 3, 2020
@istio-testing
Copy link
Contributor

@MichaelEizaguirre: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
lint_tools 99a2f6e link /test lint_tools
benchmark_check_tools 99a2f6e link /test benchmark_check_tools
containers-test-arm64_tools 99a2f6e link true /test containers-test-arm64

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carolynhu carolynhu carolynhu left review comments

@nmittler nmittler nmittler left review comments

@yangminzhu yangminzhu yangminzhu left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. do-not-merge Block automatic merging of a PR. needs-rebase Indicates a PR needs to be rebased before being merged ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@MichaelEizaguirre @istio-testing @ericvn @nmittler @richardwxn @carolynhu @yangminzhu @googlebot