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

update istio-ingress installation name in helm install instructions #13516

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dhawton
Copy link
Member

@dhawton dhawton commented Jul 6, 2023

Please provide a description for what this PR is for.

Mesh Config has a default of looking for istio-ingressgateway, which is the name elsewhere and by other methods for installation. This PR changes the installation instructions to match the name used elsewhere. For example: https://istio.io/latest/docs/setup/additional-setup/gateway/#deploying-a-gateway, https://istio.io/latest/docs/setup/install/istioctl/#check-whats-installed.

Fixes #13497

And to help us figure out who should review this PR, please
put an X in all the areas that this PR affects.

  • Ambient
  • Docs
  • Installation
  • Networking
  • Performance and Scalability
  • Extensions and Telemetry
  • Security
  • Test and Release
  • User Experience
  • Developer Infrastructure

@dhawton dhawton requested review from a team as code owners July 6, 2023 21:29
@istio-testing istio-testing added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 6, 2023
Copy link
Member

@windsonsea windsonsea left a comment

Choose a reason for hiding this comment

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

lgtm for the zh part if the en change is correct.

@zirain
Copy link
Member

zirain commented Jul 7, 2023

FYI: #11762 (review)

@dhawton
Copy link
Member Author

dhawton commented Jul 7, 2023

FYI: #11762 (review)

His comment is directed at the namespace rather than installation name. The installation name is inconsistent in this doc vs another doc with helm installation (ie, https://istio.io/latest/docs/setup/additional-setup/gateway/#deploying-a-gateway). This change aligns the general install doc with your gateway doc change PR here: #11734.

@zirain
Copy link
Member

zirain commented Jul 8, 2023

@howardjohn ptal

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.

This was intentional. Using the same names across samples, IMO, is actively harmful. users should understand what fields link things up and set them to match when appropriate.

We should not rely on a demo world where we happened to name everything the same. It ends with users thinking istio: ingressgateway is some magic value, when really its a simple label selector. I have seen this countless times. I have never seen the same with Kubernetes Service -- but probably we would, if their docs always used hello: world in every sample and treated it as a bug if it wasn't that.

@dhawton
Copy link
Member Author

dhawton commented Jul 10, 2023

This was intentional. Using the same names across samples, IMO, is actively harmful. users should understand what fields link things up and set them to match when appropriate.

We should not rely on a demo world where we happened to name everything the same. It ends with users thinking istio: ingressgateway is some magic value, when really its a simple label selector. I have seen this countless times. I have never seen the same with Kubernetes Service -- but probably we would, if their docs always used hello: world in every sample and treated it as a bug if it wasn't that.

@howardjohn However the current documentation varies from the default value we specify in the meshConfig for ingressService, which is istio-ingressgateway (https://istio.io/latest/docs/reference/config/istio.mesh.v1alpha1/#MeshConfig). This makes it feel like a bug since a user could follow the documentation as-is, and not have a working Ingress controller without additional steps that are not documented in this page. For a new person coming in to the project, installation steps are a great way to get started and not always best practice. As far as the linked comment by @zirain, I agree, the changing of namespaces is a good move and we document that in the Deploying Gateway docs.

I guess an alternative could be to update the documentation to clarify that the name in this step differs from meshConfig's default value and additional steps are required. I'm trying to approach this as someone who knows Kubernetes and Helm, but doesn't know Istio. I would follow the guide to get a learning environment setup and play in, and nothing seems to indicate that this wouldn't work "out of the box" until they go to test the environment and see the ingress not working.

@dhawton dhawton requested a review from howardjohn July 18, 2023 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/docs size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

default helm installation steps doesn't work for k8s ingress
6 participants