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 istioctl install page for ambient and improve install docs experience #15483

Merged
merged 18 commits into from
Aug 21, 2024

Conversation

craigbox
Copy link
Contributor

Add docs on how to install ambient with istioctl.
When @bleggett's changes to Helm docs are in, I'll modify ths further to bring the two in line.

  • Ambient
  • Docs

@craigbox craigbox requested a review from a team as a code owner July 29, 2024 09:18
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 29, 2024
@craigbox craigbox added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jul 29, 2024
@craigbox craigbox requested review from a team as code owners August 19, 2024 02:53
@istio-testing istio-testing added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 19, 2024
@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 19, 2024
@craigbox craigbox changed the title first draft of istioctl docs add istioctl install page for ambient and improve install docs experience Aug 19, 2024
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Aug 19, 2024
@craigbox craigbox mentioned this pull request Aug 19, 2024
11 tasks
@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 Aug 20, 2024
@craigbox
Copy link
Contributor Author

/test doc.test.profile-default

(#15576)


## CNI
The following configurations apply to all platforms, when certain {{< gloss "CNI" >}}CNI plugins{{< /gloss >}} are used:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The following configurations apply to all platforms, when certain {{< gloss "CNI" >}}CNI plugins{{< /gloss >}} are used:
The following configurations apply to any platform when specific {{< gloss "CNI" >}}CNIs{{< /gloss >}} are used:

Copy link
Contributor

@bleggett bleggett Aug 20, 2024

Choose a reason for hiding this comment

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

plugin is an impl detail that's not relevant to any of this, and I want to continually reinforce that you already have a CNI (which may ship one or more plugins + lots of other bits), which is not Istio.

We can say "CNI provider" here if we want.

When we say "CNI" or "CNI provider" -> that's the universe of not-Istio
When we say "CNI plugin" -> that's the only thing Istio does that's CNI related, and the phrase "CNI plugin" should be used very sparingly outside of the CNI detailed docs.

People are already confused enough about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be more correct to say "CNI runtime"? But less understood, I would think. Kubernetes uses the term 'CNI plugin', as do many of the people that make them:

Kubernetes uses the Container Network Interface (CNI) to interact with networking providers like Calico. The Calico binary that presents this API to Kubernetes is called the CNI plugin and must be installed on every node in the Kubernetes cluster.

and thus I think this is a fight that is lost outside of the universe of people who actually write CNI implementations.

We were trying to have our thing be called the 'CNI node agent' to differentiate it. Is that not sensible?

(p.s. I see you)

Screenshot 2024-08-21 at 12 16 44 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

CNI node agent is the most sensible, I mostly just don't want to say CNI plugin and have it not be clear if we're talking about our thing (which is not a CNI, and consists of more than a plugin) or the thing you already have (which is, and consists of more than a plugin).

content/en/docs/ambient/install/_index.md Show resolved Hide resolved
content/en/docs/ambient/install/helm/index.md Outdated Show resolved Hide resolved
content/en/docs/ambient/install/istioctl/index.md Outdated Show resolved Hide resolved
Comment on lines +42 to +44
`istioctl` supports a number of [configuration profiles](/docs/setup/additional-setup/config-profiles/) that include different default options,
and can be customized for your production needs. Support for ambient mode is included in the `ambient` profile. Install Istio with the
following command:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`istioctl` supports a number of [configuration profiles](/docs/setup/additional-setup/config-profiles/) that include different default options,
and can be customized for your production needs. Support for ambient mode is included in the `ambient` profile. Install Istio with the
following command:
Support for ambient mode is included in the `ambient` profile. Install Istio with the
following command:

The profiles support between istioctl and helm is 1-to-1 - they are the same profiles, in fact.

So either we need to blurb this on the helm page too ("hey, we have other profiles, go look") - or do what that page does, and don't blurb it.

Either way, should be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sidecar installation docs haven't been meaningfully updated from the time when they were the only installation method.

I agree that a complete overhaul of this is a good idea but I'm not going to boil the ocean in this PR. I haven't created an issue for this one because I believe that istio/istio#52702 should be general enough for sidecars too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the topic of ambient specifically, with istioctl you need profile=ambient in order to tell it to install ztunnel and istio-cni. It's reasonable to point that out here - if you only did istioctl install, you wouldn't get working ambient mode.

Given that, in the Helm docs, we explicitly say you need to install Helm charts A B C and D, individually including ztunnel and CNI. What use are profiles there? Why are these two lines different?

$ helm install istiod istio/istiod --namespace istio-system --set profile=ambient --wait
$ helm install ztunnel istio/ztunnel -n istio-system --wait 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, it's explained in https://github.com/istio/istio/blob/master/manifests/helm-profiles/ambient.yaml that they are just sets of values, not groups of components.

an IstioOperator profile is a list of components and a link to a Helm profile.

this isn't half confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Profiles for both are almost entirely (and primarily) sets of values, and in fact both the Istio and helm ones are derived from the same file IIRC.

(they could be both sets of values and of components for both helm and istioctl pretty easily if we did chart composition in helm, but we do not).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ambient Helm profile contains environment variables and flags.

The ambient istioctl profile contains a list of components to enable, and an instruction to Helm to use the ambient Helm profile.

So yes, you're mostly right, but in the specific case of ambient, the istioctl profile is necessary to say "install ztunnel", where that is spelled out in the Helm docs.

This has been a useful exercise to understand the distinction, and if we could get chart composition - I assume you get that through dependencies? - then it would be possible to get the same outcome yes, though not through something called a profile. :)

Copy link
Member

Choose a reason for hiding this comment

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

I doubt the helm profiles would work via dependencies unfortunately... helm doesn't have native 'profile' concept so we hack it in a way that would apply the config after it has chosen which dependencies to enable I suspect (untested) (https://blog.howardjohn.info/posts/advanced-helm/)

Copy link
Contributor

@bleggett bleggett Aug 21, 2024

Choose a reason for hiding this comment

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

Dependencies in helm can be enabled or disabled based on values.yaml properties, so I would expect if you disabled a component but a profile still supplied values for it, that would just no-op in the templating and give you ~roughly the same behavior as istioctl install <specific component> -f large-operator-yaml-with-many-settings-that-do-not-apply-to-that-component.yaml today.

but yeah, need to test. Helm can do what istioctl install does, we just don't use Helm in a way that lets us use those features.

content/en/docs/ambient/upgrade/helm/index.md Outdated Show resolved Hide resolved
@craigbox craigbox requested a review from a team as a code owner August 21, 2024 00:04
@craigbox
Copy link
Contributor Author

I've addressed all of Ben's comments with a bunch of "will fix later" issues, and what would you know, I think I've fixed #15576.

@dhawton for approval, or telling me to split the fix into its own PR first.

@kfaseela
Copy link
Member

LGTM

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.

LGTM

1. If you are using [Minikube](https://kubernetes.io/docs/tasks/tools/install-minikube/) with the [Docker driver](https://minikube.sigs.k8s.io/docs/drivers/docker/),
you must append `--set cni.cniNetnsDir="/var/run/docker/netns"` to the `helm install` command so that the `istio-cni` node agent can correctly manage
and capture pods on the node.
On GKE, Istio components with the [system-node-critical](https://kubernetes.io/docs/tasks/administer-cluster/guaranteed-scheduling-critical-addon-pods/) `priorityClassName` can only be installed in namespaces that have a [ResourceQuota](https://kubernetes.io/docs/concepts/policy/resource-quotas/) defined. By default in GKE, only `kube-system` has a defined ResourceQuota for the `node-critical` class. The Istio CNI node agent and `ztunnel` both require the `node-critical` class, and so in GKE, both components must either:
Copy link
Member

Choose a reason for hiding this comment

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

off topic but we should automate this or warn in istioctl


### K3D
### k3d
Copy link
Member

Choose a reason for hiding this comment

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

also off topic, we should automagically detect this, k3s, and microk8s

@craigbox
Copy link
Contributor Author

/test doc.test.profile-demo

@istio-testing istio-testing merged commit 66494b4 into istio:master Aug 21, 2024
13 checks passed
@craigbox
Copy link
Contributor Author

/cherry-pick release-1.23

@istio-testing
Copy link
Contributor

@craigbox: new pull request created: #15597

In response to this:

/cherry-pick release-1.23

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-sigs/prow repository.

wilsonwu added a commit to wilsonwu/istio.io that referenced this pull request Aug 22, 2024
wilsonwu added a commit to wilsonwu/istio.io that referenced this pull request Aug 22, 2024
istio-testing pushed a commit that referenced this pull request Aug 22, 2024
…l docs into Chinese (#15599)

* Sync #15483 add istioctl install page for ambient and improve install docs experience into Chinese

* translate

* translate

* fix lint

* fix lint
istio-testing pushed a commit that referenced this pull request Aug 22, 2024
* Sync #15483 part of improve ambient docs into Chinese

* translate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ambient kind/docs size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants