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.

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

Add Kind Example #7155

Closed
wants to merge 1 commit into from
Closed

Conversation

shaneutt
Copy link

@shaneutt shaneutt commented Apr 24, 2020

The purpose of this PR is to add an example of deploying Istio in KIND for testing Istio. The example covers using a LoadBalancer type Service in this environment as well as setting up certificates and configuring Istio gateways with SSL.

@shaneutt shaneutt requested a review from a team as a code owner April 24, 2020 19:21
@istio-policy-bot
Copy link

😊 Welcome! This is either your first contribution to the Istio documentation repo, or
it's been awhile since you've been here. A few things you should know:

  • You can learn about how we write and maintain documentation, about our style guidelines,
    and about all the available web site features by visiting Contributing to the Docs.

  • In the next few minutes, an automatic preview of your change will be
    built as a full copy of the istio.io website. You can find this preview by clicking on
    the Details link next to the deploy/netlify entry in the Status section of this
    page.

  • We care about quality, so we've put in place a number of checks to ensure our documentation
    is top notch. We do spell checking, we sanitize the markdown, we ensure all hyperlinks point
    to valid location, and more. If your PR doesn't pass one of these checks, you'll see a red X in the
    status section of the page. Click on the Details link to get a list of the problems with your PR.
    Fix those problems and push an update to your PR. This will automatically rerun the tests and
    hopefully this time everything will be perfect.

  • Once your changes are accepted and merged into the repository, they will initially show up
    on https://preliminary.istio.io. The changes will be published to https://istio.io
    the next time we do a major release (which typically happens every 3 months or so).

Thanks for contributing!

Courtesy of your friendly welcome wagon.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. label Apr 24, 2020
@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test labels Apr 24, 2020
@istio-testing
Copy link
Contributor

Hi @shaneutt. 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.

@shaneutt shaneutt force-pushed the shaneutt/kind-example branch from d739615 to 145b9d0 Compare April 27, 2020 13:59
@shaneutt shaneutt force-pushed the shaneutt/kind-example branch 3 times, most recently from 572077c to 4d43c24 Compare April 27, 2020 14:13
@shaneutt shaneutt requested a review from frankbu April 27, 2020 14:13
@frankbu
Copy link
Collaborator

frankbu commented Apr 27, 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 Apr 27, 2020
@shaneutt shaneutt force-pushed the shaneutt/kind-example branch from a3fd3ef to 74251c2 Compare April 27, 2020 17:03
@shaneutt shaneutt requested a review from frankbu April 27, 2020 17:04
@istio-testing istio-testing added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 27, 2020
@shaneutt
Copy link
Author

@frankbu I noticed that the lint test failed on /docs type links:

https://prow.istio.io/view/gcs/istio-prow/pr-logs/pull/istio_istio.io/7155/lint_istio.io/6083

However I noticed other docs use these kinds of links. Is there something more I need to do in this case?

@shaneutt shaneutt force-pushed the shaneutt/kind-example branch from e9252f8 to fade29b Compare April 27, 2020 17:30
@frankbu
Copy link
Collaborator

frankbu commented Apr 28, 2020

@shaneutt the /doc/... links are internal links within the istio.io site. The ones that are failing are files that have recently moved. Moved files have redirect aliases from the old to new URL, so they can still be accessed with the old URL, but the lint checker doesn't follow aliases and requires all internal links be kept up to date. To fix the broken ones in you PR, prepend preliminary.istio.io to the broken URL and then paste in a browser to see where it redirects to.

@shaneutt shaneutt requested a review from frankbu April 29, 2020 18:16
Start by using `istioctl` to deploy the base Istio installation with [SDS](/docs/tasks/traffic-management/ingress/secure-ingress/) and [Ingress with HTTPS](/docs/ops/integrations/certmanager/) enabled:

{{< text bash >}}
$ istioctl manifest apply \
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just realized that this is outdated and fails to work on latest 1.6 builds. For one, sds is now enabled by default and not sure about the k8sIngress stuff (cc @howardjohn).

This would need to work with https://github.com/istio/istio/releases/tag/1.6.0-beta.0, before we can merge to master.

I think you might need to tweak some of it.

Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

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

I don't think this documentation is appropriate here. If there is additional context on why it is can you add it to the PR description?

  • We already document how to setup kind, and other platforms, at https://istio.io/docs/setup/platform-setup/kind/
  • MetalLB is completely orthogonal to Istio. At best we should link to metalLB's documentation. There are many applications that "extend" Istio, and we cannot maintain full documentation for all of them. Instead we should be documenting what Istio specific changes are needed to integrate with these products (in metal lb case - there is none). Please see https://preliminary.istio.io/docs/ops/integrations/ for some existing examples.
  • Same comment for cert-manager. We should be documenting Istio, not cert-manager. The documentation on how to integrate Istio with cert-manager can be found in the link above
  • We are re-documenting how to install Istio. We already have an install document, we don't need to duplicate this just because we are now documenting kind - it is the same steps for any platform. If there were kind specific settings, they would belong in the platform setup page
  • Next we go on to document a bunch of stuff about traffic routing, which is covered already in https://istio.io/docs/tasks/traffic-management/

Note the above is my opinion - others may disagree. I appreciate the work you have put in here but I feel like it would be better spent by enhancing existing documents rather than creating a brand new one

@frankbu
Copy link
Collaborator

frankbu commented Apr 30, 2020

@howardjohn I see your point. What I like about this doc is that it's a nice quick-start type all-in-one setup guide that produces a more functional cluster than our quick start does. I think you do bring up a good point that this might not be appropriate for an Example, because of the platform and other component specifics. I had been thinking that maybe it would be better suited as a Blog, and now I'm thinking that even more so. WDYT?

@howardjohn
Copy link
Member

I have no problem with this as a blog, as its a point in time snapshot (ie no maintenance)

@frankbu
Copy link
Collaborator

frankbu commented Apr 30, 2020

@shaneutt Could you please move this from Examples to Blog. The only significant difference is that you'll need to add your name as author.

The real difference for us, as John pointed out, is that a blog can be outdated and stop working in a future release. If we make it an example, somebody would now be responsible for testing and updating it in future releases of Istio. The 1.6 incompatibility that I noticed, above, is an example of what may happen next release again.

Btw, the more you can point to other docs to do certain steps (e.g., install Istio), the less likely your blog will fail to work in the future. But, I also realize that there needs to be a balance between giving the instructions and pointing the reader all over the place at each step.

@shaneutt
Copy link
Author

I don't think this documentation is appropriate here. If there is additional context on why it is can you add it to the PR description?

This documentation was very intentionally created as an "example" as per the bar set for what examples seems to come from the tagline:

"A variety of fully working example uses for Istio that you can experiment with."

In this context "fully working" seems to suggest that it represents something with end-to-end functionality, and as you can see from the documentation provided the "that you can experiment with" was taken very obviously in spirit, but also practically exampled through some basic tests you can run on the deployment.

From my review the contents of this example do not seem to stray far from the scope established by other examples like bookinfo, wherein your ultimate goal is to access a web app (in this case more simple just an NGinx instance) and providing all the resources and configurations prescriptively along the way needed to reach that goal.

* We already document how to setup kind, and other platforms, at https://istio.io/docs/setup/platform-setup/kind/

Yes, but that documentation isn't exactly "fully working" in the sense that you can't route HTTPS traffic through Istio into the cluster. This documentation makes specific reference to that documentation to point out that we're using kind very pointedly for the purposes of this example, but suggests reviewing that documentation as well.

* MetalLB is completely orthogonal to Istio. At best we should link to metalLB's documentation. There are many applications that "extend" Istio, and we cannot maintain full documentation for all of them. Instead we should be documenting what Istio specific changes are needed to integrate with these products (in metal lb case - there is none). Please see https://preliminary.istio.io/docs/ops/integrations/ for some existing examples.

MetalLB (as stated in the document) is a means to an end. In other examples the provisioning of LoadBalancer type Service resources is a given, but in a kind cluster it is not. This was the chosen implemented to enable a "fully working" example, but unlike the other integrations you've linked this isn't related directly to Istio but instead to Kubernetes.

Should people writing examples not feel free to include setup for components that don't directly relate to Istio itself, but relate ultimately to the ecosystem that is being exampled that Istio is being used in? There seems to be precent with MySQL and some other components throughout examples.

* Same comment for cert-manager. We should be documenting Istio, not cert-manager. The documentation on how to integrate Istio with cert-manager can be found in the link above

The reason for not simply linking directly to the existing documentation is that without specifics this wont be a "fully working" example, in the sense that the user would need to figure out cert-manager from the cert-manager documentation, and make choices based on the options available there. This provides a solution that should work in any environment, and even provides tips about how one could experiment beyond what's provided. I would like to submit that the specificity provided here is key to making this example simple and quick and allows them to get the desired result quickly, and go back and investigate how the underlying bits work later if desired.

* We are re-documenting how to install Istio. We already have an install document, we don't need to duplicate this just because we are now documenting `kind` - it is the same steps for any platform. If there were kind specific settings, they would belong in the platform setup page

This makes specific choices relevant to the goal of having fully working HTTPS gateways upon completion. Is it not valid to provide something that can be learned about elsewhere in example documentation? Is there not some way I could improve this section such that it would be more prescriptive or informational?

* Next we go on to document a bunch of stuff about traffic routing, which is covered already in https://istio.io/docs/tasks/traffic-management/

That documentation is great in the case that an end-user is coming to the docs trying to very specifically understand how traffic routing is done. But in this case a more prescriptive example is provided. Is that not basically the spirit of "example" documentation, such as to provide a specific implementation for a higher-level goal?

Look at this part of the bookinfo example for instance. In this example specific destination rules are provided could you not also argue that would be a place where we could have instead linked to existing documentation and/or expanded that documentation to cover the needs here? But then would it not be harder to follow for the person who wants to start with an end result (a working webserver) and then work back through the implementation as opposed to needing to know how to come up with these manifests themselves?

Note the above is my opinion - others may disagree. I appreciate the work you have put in here but I feel like it would be better spent by enhancing existing documents rather than creating a brand new one.

@shaneutt Could you please move this from Examples to Blog. The only significant difference is that you'll need to add your name as author.

The real difference for us, as John pointed out, is that a blog can be outdated and stop working in a future release. If we make it an example, somebody would now be responsible for testing and updating it in future releases of Istio. The 1.6 incompatibility that I noticed, above, is an example of what may happen next release again.

I would submit that "example" documentation takes a high level goal and provides a very specific set of implementations to reach that goal. To that end, I believe what I have submitted qualifies as an example and if possible I would like to keep this as an example rather than a blog post.

If you are not persuaded by my explanations and comparisons above, could you please help me to better understand what qualifies as an example and under what conditions you would accept something as example documentation?

If there's a criteria I have not met for what an example ultimately is, and what it takes to get one accepted here, I would like to know what that is so that I can have the opportunity to contribute real examples, if somehow different from what I've provided so far here.

@shaneutt
Copy link
Author

shaneutt commented Apr 30, 2020

The real difference for us, as John pointed out, is that a blog can be outdated and stop working in a future release. If we make it an example, somebody would now be responsible for testing and updating it in future releases of Istio. The 1.6 incompatibility that I noticed, above, is an example of what may happen next release again.

Could this problem not be solved another way? Can the guarantee of backwards compatibility be made with the other examples that already exist? Could perhaps documentation have to be specifically tagged with the versions it's been tested with, and only display for the versions provided? To me it seems that you would want to test these things continue to work in newer versions, no? And if they don't, make the necessary version-specific corrections for the docs?

@howardjohn
Copy link
Member

Could this problem not be solved another way? Can the guarantee of backwards compatibility be made with the other examples that already exist? Could perhaps documentation have to be specifically tagged with the versions it's been tested with, and only display for the versions provided? To me it seems that you would want to test these things continue to work in newer versions, no? And if they don't, make the necessary version-specific corrections for the docs?

Yes, we absolutely want to make sure that the Istio docs are accurate as Istio changes. With this PR, we need to do far more than that. Now when cert-manager updates, we need to update the doc. When metallb, nginx, kind, ... update, we need to update the docs.

Even if it was purely Istio, we have a task that describes a large set of this already: https://preliminary.istio.io/docs/tasks/traffic-management/ingress/secure-ingress/. Anytime we change how this works, now we have 2 places to change instead of just one. Its also confusing to users in my opinion. If I want to set up https should I use this guide? What if I don't want to use kind or cert-manager, now the example only sort of works.

If you are not persuaded by my explanations and comparisons above, could you please help me to better understand what qualifies as an example and under what conditions you would accept something as example documentation?

I am not a docs maintainer so my opinions don't hold any more weight than yours here, but personally the whole "examples" section seems like something we should get rid of for the reasons I mentioned above

@howardjohn
Copy link
Member

Put another way, why do we have "Examples" and "Tasks" when an example is "A variety of fully working example uses for Istio that you can experiment with." and a Task is also literally a step by step copy+pastable example of uses of Istio features you can experiment with

@joejulian
Copy link

As a side note (I work with Shane and have been following along out of interest), I'd tried to experiment with Istio before and the documentation was so vague and hard to find or follow that I gave up. Shane didn't have that luxury and was required to get it working in order to write up an evaluation. This PR is a response to his similar frustration with the documentation for this project and an attempt to make it better for future users.

If this documentation had existed when I tried, I would have not given up and I would have been successful. If you have criteria for documentation, I suggest you re-evaluate those criteria because they're lacking from the perspective of a new user.

@joejulian
Copy link

"I suggest you re-evaluate" ... sorry, that sounds way more prescriptive than I intended. I don't mean to be antagonistic.

@shaneutt
Copy link
Author

shaneutt commented May 4, 2020

@googlebot I signed it!

@shaneutt shaneutt force-pushed the shaneutt/kind-example branch from 99aafd4 to 2744373 Compare May 5, 2020 13:29
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. and removed cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. labels May 5, 2020
@shaneutt
Copy link
Author

shaneutt commented May 5, 2020

@frankbu & @howardjohn I have taken care of the CLA, this PR is ready for re-review.

I would appreciate if you could review my above comments and provide a path forward where I could make modifications to this documentation that for you would make it acceptable as example documentation as it was originally intended.

Thank you for your time and considerations.

@shaneutt shaneutt requested review from frankbu and howardjohn May 5, 2020 13:34
@frankbu
Copy link
Collaborator

frankbu commented May 5, 2020

@joejulian thanks for the feedback. You're definitely not the first one to tell us that our getting started documentation is bad.

We've been struggling all along with the exact purpose of the examples section of the docs. Over the last couple of years we've removed as many docs from that section as we have added new ones. We've had as many complaints about docs that our outdated and don't work as lack of docs. The problem, I suppose, is that Istio has had a lot of changes, some breaking changes (not so good), but mostly just a lot of improved and different ways to do things. As a result, maintaining the docs has been a bit of a nightmare.

To try and get this under control, we've recently started to put in place a framework for testing docs, to make sure they work as described (https://github.com/istio/istio.io/blob/master/tests/README.md). @shaneutt, you ran into this, when you initially copied another example md file to use as a template and it had the test=true settings in its metadata, which resulted in a snip file being generated for your doc.

So, now the question of what to do with this doc, which personally I think a very useful getting started for evaluation setup. There are several ways we could approach it:

  1. Turn it into a blog so there is no guarantee it is still working after release 1.6, although it can be updated and kept working and referenced from the mainline docs. We do have a few examples where we've done that, including this one that I wrote myself, some time ago.
  2. Generate the snips and write a test for it. I think this will be complicated by the fact that the framework is more tuned to testing istio features vs. installing components, but it may be possible.
  3. Think about publishing it somewhere else, and adding a section in the Istio docs (e.g. community examples) that is just a bunch of links to helpful documents. I suppose some of these links could be to Istio.io blogs.

I'm thinking that maybe option 1 is the easiest, and we can look at also doing number 3, where this blog would then be one of the links. Let me know what you guys think.

Also, most importantly, the article still needs to be updated to make sure it works with Istio 1.6, which will be the target for publishing it.

@shaneutt
Copy link
Author

shaneutt commented May 5, 2020

/retest

@shaneutt
Copy link
Author

shaneutt commented May 5, 2020

I'm thinking that maybe option 1 is the easiest, and we can look at also doing number 3, where this blog would then be one of the links. Let me know what you guys think.

Also, most importantly, the article still needs to be updated to make sure it works with Istio 1.6, which will be the target for publishing it.

@frankbu Your comment seems to suggest that you would prefer 1 or 3, but personally I am in favor of number 2, updating this for 1.6 and then adding the necessary tests. What are your thoughts?

@frankbu
Copy link
Collaborator

frankbu commented May 5, 2020

@shaneutt If you are willing to write a test, I'm OK with putting it in the examples section, at least initially. If we end up restructuring the examples section in the future, it might have to be moved, but having a test that confirms it's working as written will always be great, even if it moves to blog (which have no guarantees) or moves somewhere else entirely. Eventually, some kind of "certified working" stamp on documents, based on the test: true attribute is our goal for docs on the site, regardless of where they live.

@istio-testing
Copy link
Contributor

@shaneutt: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
k8s-tests_istio.io 2744373 link /test k8s-tests_istio.io

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.

@shaneutt
Copy link
Author

shaneutt commented May 5, 2020

@shaneutt If you are willing to write a test, I'm OK with putting it in the examples section, at least initially. If we end up restructuring the examples section in the future, it might have to be moved, but having a test that confirms it's working as written will always be great, even if it moves to blog (which have no guarantees) or moves somewhere else entirely. Eventually, some kind of "certified working" stamp on documents, based on the test: true attribute is our goal for docs on the site, regardless of where they live.

This sounds good to me, thank you @frankbu 👍

For the moment I'm going to close this as I don't have cycles to do this in the next several days, but the intention will be to open/re-open in the future with the tests.

Thanks again everyone involved for your time. :)

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. kind/docs 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
Development

Successfully merging this pull request may close these issues.

8 participants