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

Access to revisions of an external control plane #31322

Closed
frankbu opened this issue Mar 8, 2021 · 8 comments
Closed

Access to revisions of an external control plane #31322

frankbu opened this issue Mar 8, 2021 · 8 comments
Labels
area/environments area/user experience kind/enhancement lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while

Comments

@frankbu
Copy link
Contributor

frankbu commented Mar 8, 2021

Describe the feature request

When using an external control plane, the remote istiod location is configured in several places, three of which (discoveryAddress, caAddress, and injectionURL) should, but currently do not, take revisions into account:

spec:
  meshConfig:
    defaultConfig:
      discoveryAddress: $EXTERNAL_ISTIOD_ADDR:15012
  values:
    global:
      caAddress: $EXTERNAL_ISTIOD_ADDR:15012
    istiodRemote:
      injectionURL: https://$EXTERNAL_ISTIOD_ADDR:15017/inject

For example, when the the injectionURL value is set, it will be used in the MutatingWebhookConfiguration as is, with no regard to the webhooks associated revision. Only the not-set ({{- else }}) uses the revision to select the correct istiod service.

If instead, we added a new variable (istiodRemote.istiodURL), we could pass the revision as a path prefix, which the external control plane could use to access the appropriate istiod revision:

    {{- if .Values.istiodRemote.injectionURL }}
    url: {{ .Values.istiodRemote.injectionURL }}
    {{- else if .Values.istiodRemote.istiodURL }}
    url: {{ .Values.istiodRemote.istiodURL }}:15017{{- if not (eq .Values.revision "") }}/{{ .Values.revision }}{{- end }}/inject
    {{- else }}
    service:
      name: istiod{{- if not (eq .Values.revision "") }}-{{ .Values.revision }}{{- end }}
      namespace: {{ .Release.Namespace }}
      path: "/inject"
    {{- end }}

If we make a similar change for all three variables, this would also simplify the external control plane configuration. Only one variable (istiodRemote.istiodURL) would need to be configured, instead of the current three:

spec:
  values:
    istiodRemote:
      istiodURL: https://$EXTERNAL_ISTIOD_ADDR

Describe alternatives you've considered

[ ] Docs
[x] Installation
[ ] Networking
[ ] Performance and Scalability
[ ] Extensions and Telemetry
[ ] Security
[ ] Test and Release
[x] User Experience
[ ] Developer Infrastructure

Additional context

@howardjohn
Copy link
Member

I think the path prefix would not work for CA/discovery address due to grpc

@esnible
Copy link
Contributor

esnible commented Mar 9, 2021

I don't think the discoveryAddress should take revisions into account.

I am working on #31342 which lets istioctl find the XDS address using the injector config. It uses the revision to determine which ConfigMap to use.

@frankbu
Copy link
Contributor Author

frankbu commented Mar 16, 2021

@howardjohn, good point, my suggestion will only work for the https one (injectionURL).

What about using wildcard subdomains instead, something like this:

    {{- if .Values.istiodRemote.injectionURL }}
    url: {{ .Values.istiodRemote.injectionURL }}
    {{- else if .Values.istiodRemote.istiodHost }}
    url: https://{{- if not (eq .Values.revision "") }}{{ .Values.revision }}.{{- end }}{{ .Values.istiodRemote.istiodHost }}:15017/inject
    {{- else }}
    service:
      name: istiod{{- if not (eq .Values.revision "") }}-{{ .Values.revision }}{{- end }}
      namespace: {{ .Release.Namespace }}
      path: "/inject"
    {{- end }}

Would that work?

Any other suggestions? @esnible, I'm not sure I understand how #31342 fixes this.

Seems that currently revisions can't be used with external control planes.

@howardjohn
Copy link
Member

That could work, but there is not one way that this can/should be implemented. Users will have their own routing requirements. Some examples of other ideas that may work better for other users (note: these are pretty old): https://gist.github.com/howardjohn/15618fd3a1d2ed54b95ae6fb50548433 https://gist.github.com/howardjohn/41e5c30816b82b6abc8c89f943393d96

I don't think we should enforce the https://revision.url format - but we can document that its one way to configure things.

@howardjohn
Copy link
Member

Another option is to set XDS_HEADER_REVISION env var, then you can do standard virtual service header routing

@istio-policy-bot istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Sep 16, 2021
@istio-policy-bot istio-policy-bot added the lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. label Oct 1, 2021
@frankbu
Copy link
Contributor Author

frankbu commented Oct 7, 2021

Not stale.

@frankbu frankbu reopened this Oct 7, 2021
@istio-policy-bot istio-policy-bot removed the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Oct 7, 2021
@istio-policy-bot istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Apr 6, 2022
@frankbu
Copy link
Contributor Author

frankbu commented Apr 21, 2022

not stale

@frankbu frankbu reopened this Apr 21, 2022
@istio-policy-bot istio-policy-bot removed lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while labels Apr 21, 2022
@istio-policy-bot istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Oct 19, 2022
@istio-policy-bot
Copy link

🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2022-04-21. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/environments area/user experience kind/enhancement lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while
Projects
None yet
Development

No branches or pull requests

4 participants