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

clarify ambient mode installation required for dns-proxy #16230

Merged
merged 3 commits into from
Feb 17, 2025

Conversation

ilrudie
Copy link
Contributor

@ilrudie ilrudie commented Feb 10, 2025

Description

Reviewers

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

@istio-policy-bot
Copy link

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

  • You can learn about how we write and maintain documentation, our style guidelines,
    and 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 with
    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, sanitize the Markdown, ensure all hyperlinks point to a
    valid location, and more. If your PR doesn't pass one of these checks, you'll see a red X in the
    lint_istio.io entry in the status section. Click on the Details link to get a list of the
    problems with your PR. Fix those problems and push an update; this will automatically re-run the
    tests. 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).
    To publish them sooner, add a cherrypick/release-x.xx label, where x.xx is the current
    release of Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

@istio-testing istio-testing added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 10, 2025
@ilrudie ilrudie added cherrypick/release-1.24 Set this label on a PR to auto-merge it to the release-1.24 branch cherrypick/release-1.25 Set this label on a PR to auto-merge it to the release-1.25 branch labels Feb 10, 2025
@ilrudie ilrudie force-pushed the ambient-dns-capture branch from 97c0466 to c2a97ec Compare February 10, 2025 15:02
Comment on lines 55 to 58
{{< tip >}}
When using the ambient data plane in Istio 1.24 or earlier, DNS Capture is not enabled by default. To enable DNS Capture for ambient set `values.cni.ambient.dnsCapture=true` during installation.
{{< /tip >}}

Copy link
Contributor

Choose a reason for hiding this comment

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

PTAL at https://deploy-preview-16230--preliminary-istio.netlify.app/latest/docs/ops/configuration/traffic-management/dns-proxy/

It says "This isn't enabled by default, here's how you enable it"

and now has a tip that says "this isn't enabled by default in 1.24 or earlier in ambient mode"

(a) is this now on by default for everyone in 1.25? or only for ambient mode?
(b) can we reformat the instructions to say "Sidecar mode enablement", "Ambient mode enablement" etc
(c) can the sidecar mode enablement be as simple as the ambient mode enablement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(a) In 1.25 the cni dns redirect is enabled in the ambient profile as well as the auto-ip controller. IIRC the ztunnel dns proxy just always runs but whether or not it is used depends on the cni redirection. TL;DR These instructions are basically irrelevant for ambient SE functionality in 1.25 ambient.
(b) Maybe, I'm not sure how to do that exactly. Can we get /some/ useful tip up so users don't continue to struggle and take on a wider refresh as a follow up? See above re: whether or not special ambient instructions might be helpful. Maybe we just add a disclaimer/note/section/tip/? at the top that this page isn't really relevant for ambient 1.25+ instead?
(c) For HTTP traffic sidecar sort of already is as easy as ambient 1.25+. We read the headers and can route accordingly. This becomes critical path for some basic Istio functionality in Ambient though when we can't just read headers in ztunnel to figure it out which is why we enable out of the box for Ambient but not for Sidecar (at the moment)

@istio-testing istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 14, 2025
@craigbox
Copy link
Contributor

I've made some very presumptuous changes to your PR. Please have a look and see if you're happy with them. (It did involve renaming some headings, so I've had to update the tests too.)

I've also learned I did not know how to spell "presumptuous".


While Kubernetes provides DNS resolution for Kubernetes `Service`s out of the box, any custom `ServiceEntry`s will not be recognized. With this feature, `ServiceEntry` addresses can be resolved without requiring custom configuration of a DNS server. For Kubernetes `Service`s, the DNS response will be the same, but with reduced load on `kube-dns` and increased performance.

This functionality is also available for services running outside of Kubernetes. This means that all internal services can be resolved without clunky workarounds to expose Kubernetes DNS entries outside of the cluster.

## Getting started

In sidecar mode, all traffic passes through Layer 7 processing, and the `hosts` of a `ServiceEntry` can be matched to a HTTP header without requiring the use of DNS. In ambient mode, the ztunnel only sees traffic at Layer 4, and does not have access to HTTP headers. Therefore, DNS proxying is required to enable resolution of `ServiceEntry` addresses, especially in the case of [sending egress traffic to waypoints](https://github.com/istio/istio/wiki/Troubleshooting-Istio-Ambient#scenario-ztunnel-is-not-sending-egress-traffic-to-waypoints).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is fine but to nit pick a little:
Technically sidecars have DNS proxy capability because not all traffic is suitable for this form of routing and that is perhaps unclear in this paragraph. We could consider adding "If routing based on HTTP header is not possible (TCP traffic for instance), DNS proxy can be enabled." or something to this effect but written more clearly.

@ilrudie
Copy link
Contributor Author

ilrudie commented Feb 14, 2025

I've made some very presumptuous changes to your PR. Please have a look and see if you're happy with them. (It did involve renaming some headings, so I've had to update the tests too.)

I've also learned I did not know how to spell "presumptuous".

I think what you wrote is generally a nice improvement over what I had written. I left a possible alteration idea but I'm not convinced it's actually and improvement so much as its, "technically a bit more correct".

@ilrudie ilrudie self-assigned this Feb 17, 2025
@istio-testing istio-testing merged commit fff13e5 into istio:master Feb 17, 2025
13 checks passed
@ilrudie ilrudie deleted the ambient-dns-capture branch February 17, 2025 22:06
@istio-testing
Copy link
Contributor

In response to a cherrypick label: cannot checkout release-1.25: error checking out "release-1.25": exit status 1 error: pathspec 'release-1.25' did not match any file(s) known to git

@istio-testing
Copy link
Contributor

In response to a cherrypick label: #16230 failed to apply on top of branch "release-1.24":

Applying: clarify ambient mode installation required for dns-proxy
Applying: presumptious changes
Using index info to reconstruct a base tree...
M	content/en/docs/ops/configuration/traffic-management/dns-proxy/index.md
M	content/en/docs/ops/configuration/traffic-management/dns-proxy/snips.sh
M	content/en/docs/ops/configuration/traffic-management/dns-proxy/test.sh
Falling back to patching base and 3-way merge...
Auto-merging content/en/docs/ops/configuration/traffic-management/dns-proxy/test.sh
CONFLICT (content): Merge conflict in content/en/docs/ops/configuration/traffic-management/dns-proxy/test.sh
Auto-merging content/en/docs/ops/configuration/traffic-management/dns-proxy/snips.sh
CONFLICT (content): Merge conflict in content/en/docs/ops/configuration/traffic-management/dns-proxy/snips.sh
Auto-merging content/en/docs/ops/configuration/traffic-management/dns-proxy/index.md
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0002 presumptious changes

@istio-testing
Copy link
Contributor

In response to a cherrypick label: new issue created for failed cherrypick: #16253

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ambient cherrypick/release-1.24 Set this label on a PR to auto-merge it to the release-1.24 branch cherrypick/release-1.25 Set this label on a PR to auto-merge it to the release-1.25 branch kind/docs size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants