[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: frankbu, vadimeisenbergibm

The full list of commands accepted by this bot can be found here.

The pull request process is described here

@duderino Could you please merge this PR? It updates one yaml and updates a corresponding test.

@vadimeisenbergibm: The following tests failed, say /retest to rerun them all:

/test e2e-simpleTests

/test istio-pilot-e2e-envoyv2-v1alpha3

@vadimeisenbergibm I think this has a small blast radius, but I think at this point 1.1 should start slowing down and attention should shift to master and eventually 1.2. So what would be the implication of not merging this?

CC @geeknoid

@duderino The problem with not merging this one is that the source code in the release-1.1 branch of the bookinfo sample is not the same as the version that is being used in the yamls. It's true that the new function in the code is not being used yet, but it's still preferable to keep the source in sync with the images in the release.

@duderino cc @frankbu The problem is that the bookinfo images currently do not match the documentation of https://istio.io/blog/2018/egress-https/#tls-origination-by-istio.

@vadimeisenbergibm Your blog says it was last updated on August 9, 2018. If it's using the new feature, you must have updated it more recently. In that case, you should also change the date in the blog.

Bottom line is that we really should merge this PR to sync up with the images that are expected.

@frankbu I have submitted a PR to update the updated dates of all the egress blog posts - istio/istio.io#4063.

@vadimeisenbergibm @frankbu this sounds like blog about something that isn't merged yet, then use the blog to force the merge. only the most critical fixes should go into 1.1 now. might I suggest an alternative of changing the blog to reflect what is actually in 1.1?

@vadimeisenbergibm, @frankbu how about this... can you create an issue that shows what the user would see if this doesn't get merged? And also explain how this cannot be addressed by changing the documentation?

@duderino @frankbu The implication of not merging it is that the following section will not work for the user: https://preliminary.istio.io/blog/2018/egress-https/#bookinfo-with-tls-origination-to-a-google-books-web-service, the traffic to the external service will be blocked. The alternative is to revert the documentation on the istio.io to match the images.

@duderino #13316 (comment)

this sounds like blog about something that isn't merged yet, then use the blog to force the merge.

No, the blog post was written in January 2018, I just updated it in istio/istio.io#3979. The alternative to not merging this PR is to revert the update on istio.io, and also point the blog to release 1.1.1 (the blog post references the sample code on istio/istio).

The more I think about it, this discussion is pointing out a serious problem with our documentation and sample process. Ignoring the details in this particular issue, the fundamental issue here is that we seem to have a process in place that prevents us from adding missing documentation in a point release if it requires any, even additive, change to one of our samples.

The general process we've been using with documentation (esp. tasks, examples, and blogs) is to:

• Use the released samples to demonstrate various features of Istio
• When we add new documentation that can't be demonstrated with the existing samples, we enhance them to do something extra (for example, add a version that can be configured to make an http call to an external https site).
• Push the new doc and sample code to master if it is future doc, or the release branch if we want to publish the doc now.

Notice that the above process in no way involves any change to Istio's runtime, it's simply enhancing a sample to allow it demonstrate something with new documentation.

The fundamental issue here is that the process @duderino is enforcing is preventing doc improvements in point releases. Instead of adding this new documentation in 1.1.x, this documentation needs to be put on hold until 1.2, even though the functionality has been in Istio since 1.1.0. This seems like a pretty screwed up process IMO, and esp in this particular case where the updated sample images have been held up from being merged for almost 3 weeks now, because of process-related delays.

@geeknoid WDYT? It seem to me that we need a better samples/documentation process for point releases.

So a couple observations:

• First, I think we need to stop treating blog posts as living documents. They are point-in-time material. If we feel the need for them to be updated, then that implies to me that the blog post content needs to migrate to our example or task sections.

• Given the rigors of product release branches, I think it would be wise for us to move all sample content to a different repo such that it can have a different release cadence & criteria than the product itself. In fact, perhaps we should move all samples to the istio/istio.io repo directly or into a stand-alone istio/examples repo? We would need to setup enough infrastructure to run tests on that repo. Examples are really documentation, so it seems reasonable to put them in the istio.io repo, but from a discoverability standpoint, it might be better to go with a standalone example repo.

@geeknoid I agree with you on both points.

As we already started the discussion of refactoring blogs/examples/tasks better in the doc WG call last week, I think this just adds another example of where we need to straighten things out.

We also already talked about setting up a separate istio/examples repo, and I think this issue, which will be even worse as we address the Code Mauve requirement that 90% of the example code in istio.io be tested, really adds extra urgency to getting the new examples repo setup. I'll add this to the agenda for the next doc WG call.

@duderino, in the meantime, can we agree on a process that gives sample code a little more leniency in the current process. For example, how about if it's labeled kind/docs, which means it is only a sample enhancement and doesn't touch any actual Istio functionality? This will only be used sparingly in 1.1.x. A new samples repo will be setup for 1.2 forward.

I am okay with the bump the version change as it doesn't affect istio runtime but more having consistency for our samples and docs. I would like to understand the port 80 piece and why it is needed for our users to have the sample working.

I think having a samples repo is great however the sample problem still exists with our high profile bookinfo sample, we do want to ensure we can always point out which version of bookinfo works with which istio release and the corresponding documentation.

I would not worry about if a blog works at the moment. We have decided if a blog is useful enough to keep it accurate all the time, it needs to be our official doc.

My two cents: moving samples to new repo is a good idea. Let’s make the istio/samples as a go dependency, just like api. This way, all samples are vendored into istio/istio and our tests will literally have to simply change the paths instead of doing things like pulling in the repo for every tests.

go modules do not support non-go modules, so please don't vendor it in istio/istio.

My general concern is that adding splitting samples to a new repo presents an extra challenge in automating documented tasks. As a person who moved policies & telemetry samples from istio.io to istio, it was difficult enough to synchronize the two repos across two branches. I can only imagine it's going to be even more difficult to work with three repos at a time, across multiple release branches, with cross-referenced files. Developers already avoid the hassle altogether by copying samples to tests and to docs, instead of sharing them. I think adding a new repo is not going to simplify the process.

A new repo also does not solve the issue with stale samples. If we want a released version of istio to work with the latest samples, and keep istio build reproducible, then there has to be a commit to istio to link against the updated samples. So we just moved the problem to "update the samples repo" from "update the sample themselves". Go modules do not allow pointing to head of a repo (for a very good reason).