Skip to content

Various fixes for istio.io Ingress SDS test#18577

Merged
istio-testing merged 1 commit intoistio:masterfrom
nmittler:istioio-sds
Nov 15, 2019
Merged

Various fixes for istio.io Ingress SDS test#18577
istio-testing merged 1 commit intoistio:masterfrom
nmittler:istioio-sds

Conversation

@nmittler
Copy link
Copy Markdown
Contributor

@nmittler nmittler commented Nov 2, 2019

This required several changes and new features:

  • Support for multiple verifiers
  • Support for custom output
  • Separate streams of command output (stdout, stderr)
  • Disallow duplicate snippet names
  • Eliminate intermediate files when evaluating templates

@nmittler nmittler requested review from a team as code owners November 2, 2019 18:46
@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 Nov 2, 2019
@istio-testing istio-testing added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 2, 2019
@nmittler nmittler force-pushed the istioio-sds branch 4 times, most recently from ba8cb26 to b4d3135 Compare November 2, 2019 23:14
@nmittler
Copy link
Copy Markdown
Contributor Author

nmittler commented Nov 2, 2019

@howardjohn Is it possible to upgrade the version of curl we're using on the prow containers? The ingress SDS test is failing because I'm using the curl arg --retry-connrefused, which requires curl 7.52.0 or later.

@howardjohn
Copy link
Copy Markdown
Member

howardjohn commented Nov 3, 2019 via email

@nmittler
Copy link
Copy Markdown
Contributor Author

nmittler commented Nov 4, 2019

@howardjohn

Is that version of curl really new though, or can we assume users will have
it?

Looks like the latest curl version is 7.66.0. 7.53.0 was released on Feb 22, 2017.

This is only required for running one specific test for istio.io. I'm not sure how concerned we have to be about this.

Looks like the latest package available for xenial is 7.47.0, which seems to be what is installed on the container, judging by my logs

I guess we have two options:

  1. Use a different base image that will pull a newer curl.
  2. Build curl from source

How important is the base image for prow? Can we change it? If not, is there any concern with building curl ourselves?

nmittler added a commit to nmittler/tools that referenced this pull request Nov 4, 2019
nmittler added a commit to nmittler/tools that referenced this pull request Nov 4, 2019
istio-testing pushed a commit to istio/tools that referenced this pull request Nov 4, 2019
* Upgrade prow base OS to ubuntu bionic

This is needed to unblock istio/istio#18577

* removing commented command.
Copy link
Copy Markdown
Member

@brian-avery brian-avery Nov 5, 2019

Choose a reason for hiding this comment

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

I think it makes sense to continue describing the configuration options here. The docs format/bash shell commands pattern that we're following for test writing doesn't assume a lot of Go knowledge. I'm not sure that we want to tell writers to read the source to figure out how to write tests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm a little torn on this. I would say that the docs must exist here in the source code for certain. The question is whether or not we duplicate some/all of this in the MD file. My concern is that this class is a bit in flux and it's going to be painful to keep this and the MD file in sync. If we're asking the docs team to write code anyway, I would think that reading comments on the library they're using would not be an incredibly high bar.

@geeknoid thoughts?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tests are written developers, it's fine to assume access to the source code and knowledge of Go.

nmittler added a commit to nmittler/common-files that referenced this pull request Nov 5, 2019
nmittler added a commit to nmittler/common-files that referenced this pull request Nov 5, 2019
istio-testing pushed a commit to istio/common-files that referenced this pull request Nov 5, 2019
nmittler added a commit to nmittler/istio that referenced this pull request Nov 5, 2019
Includes a new image for prow based on ubuntu bionic.

Needed by istio#18577
nmittler added a commit to nmittler/test-infra that referenced this pull request Nov 8, 2019
istio-testing pushed a commit to istio/test-infra that referenced this pull request Nov 8, 2019
@nmittler
Copy link
Copy Markdown
Contributor Author

nmittler commented Nov 8, 2019

/retest

@nmittler
Copy link
Copy Markdown
Contributor Author

nmittler commented Nov 8, 2019

/test integ-istioio-k8s-tests_istio

@nmittler
Copy link
Copy Markdown
Contributor Author

nmittler commented Nov 9, 2019

@howardjohn

The error due to the fact that this command returns nothing in kind:

export INGRESS_HOST=$(kubectl -n istio-system \
get service istio-ingressgateway -o jsonpath='{.status.loadBalancer.ingress[0].ip}')

At the point this is called, we've deployed the istio ingress gateway. Do you know if ingresses behave differently in kind? Perhaps I need to add a wait before getting this field?

@nmittler nmittler force-pushed the istioio-sds branch 2 times, most recently from 8ded058 to e0cfca9 Compare November 9, 2019 17:12
@howardjohn
Copy link
Copy Markdown
Member

there is no load balancer IP in kind, like with minikube. In the test framework we have some option like --istio.test.minikube (poorly named) that makes this work by not looking for the external IP and instead looks for a different IP (not exactly sure which one)

@nmittler nmittler force-pushed the istioio-sds branch 8 times, most recently from 49f8a2d to ccbde29 Compare November 12, 2019 21:59
@nmittler
Copy link
Copy Markdown
Contributor Author

@geeknoid @brian-avery PTAL

@nmittler
Copy link
Copy Markdown
Contributor Author

/test pilot-e2e-envoyv2-v1alpha3_istio

@nmittler
Copy link
Copy Markdown
Contributor Author

/test gencheck_istio

This required several changes and new features:

- Support for multiple verifiers
- Support for custom output
- Separate streams of command output (stdout, stderr)
- Disallow duplicate snippet names
- Eliminate intermediate files when evaluating templates
@nmittler
Copy link
Copy Markdown
Contributor Author

/test integ-galley-k8s-tests_istio

@istio-testing istio-testing merged commit ce41bd1 into istio:master Nov 15, 2019
sdake pushed a commit to sdake/istio that referenced this pull request Dec 1, 2019
Includes a new image for prow based on ubuntu bionic.

Needed by istio#18577
sdake pushed a commit to sdake/istio that referenced this pull request Dec 1, 2019
This required several changes and new features:

- Support for multiple verifiers
- Support for custom output
- Separate streams of command output (stdout, stderr)
- Disallow duplicate snippet names
- Eliminate intermediate files when evaluating templates
Shuanglu pushed a commit to Shuanglu/istio-tools that referenced this pull request Jun 30, 2022
* Upgrade prow base OS to ubuntu bionic

This is needed to unblock istio/istio#18577

* removing commented command.
Shuanglu pushed a commit to Shuanglu/istio-tools that referenced this pull request Jul 6, 2022
* Upgrade prow base OS to ubuntu bionic

This is needed to unblock istio/istio#18577

* removing commented command.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants