istio.io icon indicating copy to clipboard operation
istio.io copied to clipboard

ProxyConfig CRD to enable DNS Proxy by namespace

Open jcam opened this issue 2 years ago • 5 comments

With the new ProxyConfig CRD introduced in 1.13, it is now very simple to enable DNS Proxying on a per-namespace basis. This adds instructions to the docs with an example

Please provide a description for what this PR is for.

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

  • [ ] Configuration Infrastructure
  • [X] Docs
  • [ ] Installation
  • [ ] Networking
  • [ ] Performance and Scalability
  • [ ] Policies and Telemetry
  • [ ] Security
  • [ ] Test and Release
  • [ ] User Experience
  • [ ] Developer Infrastructure

jcam avatar Mar 16 '22 14:03 jcam

😊 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.

istio-policy-bot avatar Mar 16 '22 14:03 istio-policy-bot

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

istio-testing avatar Mar 16 '22 14:03 istio-testing

/ok-to-test

ericvn avatar Mar 16 '22 14:03 ericvn

@istio/wg-networking-maintainers thoughts?

ericvn avatar Mar 16 '22 15:03 ericvn

@Monkeyanator Thoughts as John cc'd you above?

ericvn avatar May 03 '22 13:05 ericvn

PR needs rebase.

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.

istio-testing avatar Mar 10 '23 22:03 istio-testing

I can rebase this, but it's a waste of my time if it's never going to get reviewed/merged

jcam avatar Mar 10 '23 22:03 jcam

i think this got already covered by : https://github.com/istio/istio.io/pull/12867

kfaseela avatar Mar 11 '23 10:03 kfaseela

Few notes ( on both #12867 and this PR): the env variables we use are internal/experimental, and the model of adding annotations to pods and injection is not ideal.

With ambient we will not have a sidecar - there is a proposal to enable the DNS in the istio-cni daemon set. There are also plans to define a proper CRD to control interception - replacing annotations on pods and giving cluster admin ability to use RBAC or OPA to allow namespaces to mess (or not) with this.

In this context, I think we should focus on a model that is aligned with Ambient and can be made 'stable' instead of exposing experimental env-vars.

We have DNS capture on by default on VMs - we can have the DNS server in pods enabled by default as well, with auto-allocate enabled as soon as we are confident with the test coverage and move it to 'beta'. I don't know if on pods we have the ability to listen on 53, but if we do - we can also have istio-agent default to that port ( I expect istio-cni to also listen on default port ). This would allow using Pod.dnsConfig.nameservers which is a stable K8S API for enabling istio DNS. Or we can focus on the CRD to control interception and use that if we run into problems with Pod.dnsConfig.

But TL;DR is that we should be very clear in docs that anything using env variables is NOT a stable and future proof API, and almost certainly will not work on ambient. It is great that it is broadly tested, used and we have docs on how to make it easy - but we need to be clear about the expectations ( not specific to DNS capture, a LOT of other features are in this category ).

I personally don't mind merging this - or the other PR - but I would appreciate the extra disclaimers, and maybe more focus on promoting the features to less experimental/alpha state.

On Sat, Mar 11, 2023 at 2:08 AM Faseela K @.***> wrote:

i think this got already covered by : #12867 https://github.com/istio/istio.io/pull/12867

— Reply to this email directly, view it on GitHub https://github.com/istio/istio.io/pull/11064#issuecomment-1464877145, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAUR2W77VM6L6OMOYXIXH3W3RFIHANCNFSM5Q37NFBQ . You are receiving this because you are on a team that was mentioned.Message ID: @.***>

costinm avatar Mar 11 '23 16:03 costinm

Few notes ( on both #12867 and this PR): the env variables we use are internal/experimental, and the model of adding annotations to pods and injection is not ideal. With ambient we will not have a sidecar - there is a proposal to enable the DNS in the istio-cni daemon set. There are also plans to define a proper CRD to control interception - replacing annotations on pods and giving cluster admin ability to use RBAC or OPA to allow namespaces to mess (or not) with this. In this context, I think we should focus on a model that is aligned with Ambient and can be made 'stable' instead of exposing experimental env-vars. We have DNS capture on by default on VMs - we can have the DNS server in pods enabled by default as well, with auto-allocate enabled as soon as we are confident with the test coverage and move it to 'beta'. I don't know if on pods we have the ability to listen on 53, but if we do - we can also have istio-agent default to that port ( I expect istio-cni to also listen on default port ). This would allow using Pod.dnsConfig.nameservers which is a stable K8S API for enabling istio DNS. Or we can focus on the CRD to control interception and use that if we run into problems with Pod.dnsConfig. But TL;DR is that we should be very clear in docs that anything using env variables is NOT a stable and future proof API, and almost certainly will not work on ambient. It is great that it is broadly tested, used and we have docs on how to make it easy - but we need to be clear about the expectations ( not specific to DNS capture, a LOT of other features are in this category ). I personally don't mind merging this - or the other PR - but I would appreciate the extra disclaimers, and maybe more focus on promoting the features to less experimental/alpha state. On Sat, Mar 11, 2023 at 2:08 AM Faseela K @.> wrote: i think this got already covered by : #12867 <#12867> — Reply to this email directly, view it on GitHub <#11064 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAUR2W77VM6L6OMOYXIXH3W3RFIHANCNFSM5Q37NFBQ . You are receiving this because you are on a team that was mentioned.Message ID: @.>

DNS proxying is already Beta : https://istio.io/latest/docs/releases/feature-stages/ Also, for any feature to be promoted, we do need enough documents and integration tests to be added for the same, to encourage people to use it. So if we don't add documents, or page tests, how do we ensure the same? We try to add experimental warnings to the beginning of the page, if the feature is experimental or alpha I guess. In the case of DNS proxying it is already Beta, and hence the warning is not there.

kfaseela avatar Mar 11 '23 18:03 kfaseela

DNS proxying is beta - but I certainly hope the API to configure it via env variables is not considered a beta API. The code and feature is pretty stable - but like many other features we make no guarantees on a stable API until a beta CRD or some other beta API is defined.

We should not encourage people to use the env variables or alpha features - except for testing and initial adoption, but with clear understanding that the 'opt in' via env variables may go away and be replaced by a stable API ( or be on by default). In particular for APIs exposed to 'namespace admin' or 'pod owner' roles.

On Sat, Mar 11, 2023 at 10:22 AM Faseela K @.***> wrote:

Few notes ( on both #12867 https://github.com/istio/istio.io/pull/12867 and this PR): the env variables we use are internal/experimental, and the model of adding annotations to pods and injection is not ideal. With ambient we will not have a sidecar - there is a proposal to enable the DNS in the istio-cni daemon set. There are also plans to define a proper CRD to control interception - replacing annotations on pods and giving cluster admin ability to use RBAC or OPA to allow namespaces to mess (or not) with this. In this context, I think we should focus on a model that is aligned with Ambient and can be made 'stable' instead of exposing experimental env-vars. We have DNS capture on by default on VMs - we can have the DNS server in pods enabled by default as well, with auto-allocate enabled as soon as we are confident with the test coverage and move it to 'beta'. I don't know if on pods we have the ability to listen on 53, but if we do - we can also have istio-agent default to that port ( I expect istio-cni to also listen on default port ). This would allow using Pod.dnsConfig.nameservers which is a stable K8S API for enabling istio DNS. Or we can focus on the CRD to control interception and use that if we run into problems with Pod.dnsConfig. But TL;DR is that we should be very clear in docs that anything using env variables is NOT a stable and future proof API, and almost certainly will not work on ambient. It is great that it is broadly tested, used and we have docs on how to make it easy - but we need to be clear about the expectations ( not specific to DNS capture, a LOT of other features are in this category ). I personally don't mind merging this

DNS proxying is already Beta : https://istio.io/latest/docs/releases/feature-stages/ Also, for any feature to be promoted, we do need enough documents and integration tests to be added for the same, to encourage people to use it. So if we don't add documents, or page tests, how do we ensure the same? We try to add experimental warnings to the beginning of the page, if the feature is experimental or alpha I guess. In the case of DNS proxying it is already Beta, and hence the warning is not there.

— Reply to this email directly, view it on GitHub https://github.com/istio/istio.io/pull/11064#issuecomment-1464973175, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAUR2VED5UJUB3WFXVB7NLW3S7E7ANCNFSM5Q37NFBQ . You are receiving this because you are on a team that was mentioned.Message ID: @.***>

costinm avatar Mar 11 '23 19:03 costinm

@jcam: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
doc.test.profile-none_istio.io 97b836506b3dd6269efc852626158935a7654d4f link true /test doc.test.profile-none
doc.test.profile-minimal_istio.io 97b836506b3dd6269efc852626158935a7654d4f link true /test doc.test.profile-minimal
doc.test.profile-default_istio.io 97b836506b3dd6269efc852626158935a7654d4f link true /test doc.test.profile-default
doc.test.profile-demo_istio.io 97b836506b3dd6269efc852626158935a7654d4f link true /test doc.test.profile-demo

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.

istio-testing avatar Jun 15 '23 17:06 istio-testing

I think it might be best to just close this PR, yes? It seems there is no appetite to actually document how to use these features, and keeping this PR updated when it won't ever be merged is a total waste of time.

jcam avatar Jun 15 '23 20:06 jcam

Correction: I just checked and the latest documentation includes the proxy.istio.io/config annotation for a pod. I guess that'll just be "good enough"

jcam avatar Jun 15 '23 20:06 jcam