Add istio.io docs test for security/authn-policy #16140
Conversation
rlenglet
added
area/test and release
do-not-merge/work-in-progress
rlenglet
requested review from
costinm,
geeknoid,
howardjohn,
linsun,
nmittler,
ozevren and
rshriram
as code owners
googlebot
added
the
cla: yes
istio-testing
added
the
size/L
pkg/test/istio.io/tasks/security/authn-policy/authn-policy_test.go
Outdated
Show resolved
Hide resolved
| # Wait for all containers to be ready, for up to 2 minutes = 12 * 10 seconds. | ||
| retries=12 | ||
| while [ "$retries" -gt "0" ]; do | ||
| if kubectl get pods --all-namespaces -o jsonpath='{.items[*].status.containerStatuses[*].ready}' | grep -q -v false; then |
There was a problem hiding this comment.
(doesn't need to be in this PR) the test framework is going to need some retrying strategy, otherwise every script is going to need this kind of logic
There was a problem hiding this comment.
Yes, agreed. There are implicit delays and retries that are not explicit in the docs. There will be tons of those.
pkg/test/istio.io/tasks/security/authn-policy/part1-verify-reachability-from-non-istio.sh
Show resolved
Hide resolved
istio-testing
added
size/XL
rlenglet
changed the title
istio-testing
removed
the
do-not-merge/work-in-progress
|
This PR is reviewable. It's not yet very useful because of all the remaining And the docs are missing instructions on how to wait for configuration to be realized. |
|
|
||
| // https://preliminary.istio.io/docs/tasks/security/authn-policy/ | ||
| // https://github.com/istio/istio.io/blob/master/content/docs/tasks/security/authn-policy/index.md | ||
| func TestAuthnPolicy(t *testing.T) { |
There was a problem hiding this comment.
Perhaps it would be better to create separate test functions for each example?
| @@ -0,0 +1,14 @@ | |||
| #!/bin/bash | |||
| set -e | |||
There was a problem hiding this comment.
I think we need some mechanism to not have these first two lines show up in the output that's generated for use on istio.io. One option is to not include the lines in these files, and instead have the test framework insert them autoamtically before executing the script. The alternative is to automatically remove these lines when producing the final output.
There was a problem hiding this comment.
In this particular case, I can replace this with bash's -e command-line parameter which has the same effect:
#!/bin/bash -e
We could then filter out all comments (any line matching ^#).
We also need a way to capture the output of commands, both for asserting on it in tests, and for documentation. Many examples in the docs show the output of curl, for instance. I think the script's expected output should be contained in the script itself, in comments, similarly to Python doctest. Maybe something like:
#!/bin/bash -e
# expect exit status 22
# expect output begin
# 401
# expect output end
kubectl exec $(kubectl get pod -l app=sleep -n legacy -o jsonpath={.items..metadata.name}) -c sleep -n legacy -- curl http://httpbin.foo:8000/ip -s -o /dev/null -w "%{http_code}\n"
This doesn't allow capturing the output per command, but it's close to what we need.
There was a problem hiding this comment.
Having the test automatically insert them sounds good to me.
|
I don't like the fact the framework is so close to the test cases. I wonder if pkg/test/istio.io/examples should be moved to pkg/test/framework/examples, leaving only the test cases in pkg/test/istio.io |
|
it seems like these tests should actually be in the istio.io repo. Then
they can easily directly reference the scripts run in the docs instead of
testing something that may or may not be what is actually run in the docs.
This would probably make the presubmit way too long but we could move to
postsubmit
…On Fri, Aug 9, 2019, 4:38 AM Martin Taillefer ***@***.***> wrote:
I don't like the fact the framework is so close to the test cases. I
wonder if pkg/test/istio.io/examples should be moved to
pkg/test/framework/examples, leaving only the test cases in pkg/test/
istio.io
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#16140?email_source=notifications&email_token=AAEYGXOU27D3YNRG5V7LIT3QDVJLDA5CNFSM4IKNOUT2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD36NOSA#issuecomment-519886664>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEYGXJQJUODL4AA6QCAHHDQDVJLDANCNFSM4IKNOUTQ>
.
|
istio/istio.io docs already reference istio/istio for the sample files. And the tests will heavily depend on the sample files too. I would prefer to keep the scripts and sample files in the same repo. Keeping tests in istio/istio doesn't add any extra dependencies. Or, if we moved the tests to istio/istio.io, we should move the samples along with the tests. |
|
Good point. I don't really care too much where it ends up as long as we
have a single point of truth. If we have docs using some script and the
tests testing s copy of that they will fall out if sync very quickly
…On Fri, Aug 9, 2019, 6:57 AM Romain Lenglet ***@***.***> wrote:
it seems like these tests should actually be in the istio.io repo. Then
they can easily directly reference the scripts run in the docs instead of
testing something that may or may not be what is actually run in the docs.
istio/istio.io docs already reference istio/istio for the sample files.
And the tests will heavily depend on the sample files too. I would prefer
to keep the scripts and sample files in the same repo. Keeping tests in
istio/istio doesn't add any extra dependencies.
Or, if we moved the tests to istio/istio.io, we should move the samples
along with the tests.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#16140?email_source=notifications&email_token=AAEYGXMIL3KFXOD4H2RKNCLQDVZUDA5CNFSM4IKNOUT2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD36XZMY#issuecomment-519929011>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEYGXOL3I5VRF6BSRW2MALQDVZUDANCNFSM4IKNOUTQ>
.
|
| @@ -56,12 +56,26 @@ func New(t *testing.T, name string) Example { | |||
|
|
|||
| // AddScript adds a directive to run a script | |||
| func (example *Example) AddScript(namespace string, script string, output outputType) { | |||
| example.t.Helper() | |||
|
|
|||
| //fullPath := getFullPath(istioPath + script) | |||
| //fullPath := getFullPath(istioPath + script) | ||
| example.steps = append(example.steps, newStepScript("./"+script, output)) | ||
| fullPath := "./"+script | ||
| stats, err := os.Stat(fullPath) |
There was a problem hiding this comment.
Should we move this validation to a common function and use it for files as well?
| cfg.Values["global.mtls.enabled"] = "false" | ||
| } | ||
|
|
||
| // https://preliminary.istio.io/docs/tasks/security/authn-policy/ |
There was a problem hiding this comment.
These links are likely to change. Not sure we want to be adding them to the code. That said, it would be nice to have a convention to map between the test and the doc.
| ex.AddFile("legacy", "samples/httpbin/httpbin.yaml") | ||
| ex.AddFile("legacy", "samples/sleep/sleep.yaml") | ||
|
|
||
| // This is missing from the docs, but it is necessary before continuing. |
There was a problem hiding this comment.
The idea of having a validation function for scripts (i.e. should return error, should not return error, should have text...) was suggested yesterday. Might be a nice to have that idea for files as well. Waiting for containers to be ready and verifying reachability are likely common activities.
| @@ -0,0 +1,14 @@ | |||
| #!/bin/bash | |||
| set -e | |||
There was a problem hiding this comment.
Having the test automatically insert them sounds good to me.
We discussed moving the scripts out of the docs yesterday and having them get embedded similar model to the current yaml files that get embedded from istio/samples/*. The scripts could then be used/embedded directly. |
istio-testing
added
the
needs-rebase
|
@rlenglet: 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. |
|
This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
@rlenglet: The following tests failed, say
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. |
|
This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
@rlenglet lots has changed since this, do you have time to rebase or should someone else pick this up? |
howardjohn
added
the
kind/need more info
|
I suspect this will need to be redone at this point. I don't have the bandwidth right now to work on it. Closing for now. |
Add istio.io docs test for security/authn-policy.
Small improvements to the test framework:
examplestest log.Example.AddScript(), check that the script exists and is executable.[ ] Configuration Infrastructure
[ ] Docs
[ ] Installation
[ ] Networking
[ ] Performance and Scalability
[ ] Policies and Telemetry
[ ] Security
[X] Test and Release
[ ] User Experience
[ ] Developer Infrastructure