Skip to content

Add automated security policy test scripts#1194

Open
MichaelEizaguirre wants to merge 2 commits intoistio:masterfrom
MichaelEizaguirre:Add_automated_security_tests
Open

Add automated security policy test scripts#1194
MichaelEizaguirre wants to merge 2 commits intoistio:masterfrom
MichaelEizaguirre:Add_automated_security_tests

Conversation

@MichaelEizaguirre
Copy link
Contributor

@MichaelEizaguirre MichaelEizaguirre commented Sep 3, 2020

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

istio-testing commented Sep 3, 2020

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.

Details

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

MichaelEizaguirre commented Sep 3, 2020

@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

MichaelEizaguirre commented Sep 3, 2020

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

@yangminzhu yangminzhu Sep 3, 2020

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

@carolynhu carolynhu Sep 4, 2020

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

@carolynhu carolynhu Sep 4, 2020

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

richardwxn commented Sep 8, 2020

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

@carolynhu
Copy link
Contributor

carolynhu 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

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

yangminzhu commented Sep 9, 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

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

istio-testing commented Oct 3, 2020

@MichaelEizaguirre: PR needs rebase.

Details

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

istio-testing commented Oct 14, 2022

@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
Details

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

3 more reviewers

@yangminzhu yangminzhu yangminzhu left review comments

@nmittler nmittler nmittler left review comments

@carolynhu carolynhu carolynhu left review comments

Reviewers whose approvals may not affect merge requirements

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