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.

Sign up for GitHub

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

Jump to bottom

Ambient: should use istio.io/dataplane-mode=ambient for enrolling individual pods #50355

Closed
bleggett opened this issue Apr 10, 2024 · 5 comments · Fixed by #50683
Closed

Ambient: should use istio.io/dataplane-mode=ambient for enrolling individual pods #50355

bleggett opened this issue Apr 10, 2024 · 5 comments · Fixed by #50683
Labels
Ambient Beta Must have for Beta of Ambient Mesh area/ambient Issues related to ambient mesh area/networking/cni Istio CNI-related issues area/user experience good first issue Indicates a good first issue for new contributors kind/docs kind/enhancement

Comments

@bleggett
Copy link
Contributor

bleggett commented Apr 10, 2024 •
edited

Describe the feature request

Right now, adding the annotation ambient.istio.io/redirection: enabled to an individual pod implicitly enrolls that pod into ambient. This annotation is added by istio-cni to indicate it has already enrolled a pod, and is not really intended to be user-applied.

Adding the label istio.io/dataplane-mode=ambient to namespaces enrolls everything in the namespace.

This is a bit backwards and unintuitive, and sort of an unintended side effect - the ambient.istio.io/redirection: enabled annotation should not add or remove pods from ambient or munged by users at all, it is largely intended to be a status managed only by istio-cni, not a mechanism of user intent.

We should update the istio-cni logic to check for the label istio.io/dataplane-mode=ambient on both namespaces and individual workloads, for the sake of consistency, and if that label is found in either spot, that pod should be enrolled by istio-cni and get the annotation ambient.istio.io/redirection: enabled

Alternatively (implementer's choice/others can make cases) we can make both be labels, as described in the "alternatives" section - either would be fine, we just need to be consistent, and have one field for intent, and one field for status.

Describe alternatives you've considered

  • Make the ambient.istio.io/redirection: enabled annotation a label instead (both of them labels).

    • We don't have a specific need to do this at this time
    • It makes sense to keep labels as "intent" and annotations as "status"
    • We would be writing (heavier) labels on every pod add, as this label would continue to be a controller-written status.
    • Could theoretically make indexed querying easier (but we have no need to do that for this field right now, as the other label can be used for that)
    • This option IS on the table and would also be acceptable.

  • Make the istio.io/dataplane-mode=ambient label an annotation instead (both of them annotations)

    • Makes querying hard

Affected product area (please put an X in all that apply)

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

Affected features (please put an X in all that apply)

[ ] Multi Cluster
[ ] Virtual Machine
[ ] Multi Control Plane

Additional context
https://istio.slack.com/archives/C049TCZMPCP/p1712672952578429
@bleggett bleggett added area/networking/cni Istio CNI-related issues good first issue Indicates a good first issue for new contributors area/user experience Ambient Beta Must have for Beta of Ambient Mesh labels Apr 10, 2024
@bleggett
Copy link
Contributor Author

bleggett commented Apr 10, 2024 •
edited

(if we do not think this belongs as an ambient beta blocker, can remove, but as it concerns UX, I think it might should be)

@bleggett bleggett mentioned this issue Apr 12, 2024
Merged
@eoinfennessy
Copy link
Contributor

eoinfennessy commented Apr 24, 2024 •
edited

Hi @bleggett, I'd like to work on this issue.

Thoughts/questions after an initial read-through:

  1. In cni/pkg/util/podutil.go, update the PodRedirectionEnabled function to account for Pods labelled istio.io/dataplane-mode=ambient. This looks like it should (?) cover all necessary checks in cni/pkg/nodeagent/informers.go (in functions GetPodIfAmbient, GetAmbientPods, and reconcilePods), allowing all Pods with this label to become enrolled in ambient and receive the annotation ambient.istio.io/redirection: enabled.
  2. Do we want to update the PodInAmbient function in pkg/config/analysis/analyzers/utils/in_mesh.go to account for this label? (i.e. return true if either the annotation ambient.istio.io/redirection: enabled or the label istio.io/dataplane-mode=ambient is set)

Does this sound reasonable? Anything I might be missing here?

@bleggett
Copy link
Contributor Author

bleggett commented Apr 24, 2024 •
edited

Hi @bleggett, I'd like to work on this issue.

Thoughts/questions after an initial read-through:

  1. In cni/pkg/util/podutil.go, update the PodRedirectionEnabled function to account for Pods labelled istio.io/dataplane-mode=ambient. This looks like it should (?) cover all necessary checks in cni/pkg/nodeagent/informers.go (in functions GetPodIfAmbient, GetAmbientPods, and reconcilePods), allowing all Pods with this label to become enrolled in ambient and receive the annotation ambient.istio.io/redirection: enabled.

Yep.

  1. Do we want to update the PodInAmbient function in pkg/config/analysis/analyzers/utils/in_mesh.go to account for this label? (i.e. return true if either the annotation ambient.istio.io/redirection: enabled or the label istio.io/dataplane-mode=ambient is set)

We should not need to change that - the CNI logic should add the annotation automatically, as a side effect of getting triggered to enroll the pod via the label.

The label is intent, the annotation is status, and PodInAmbient should only ever look at actual status.

@bleggett
Copy link
Contributor Author

@eoinfennessy happy to have you work on it, ping me if/when you open a PR and I will assign this issue to you, and feel free to reach out if you have questions!

@eoinfennessy
Copy link
Contributor

@bleggett, thank you! I should have a PR for this before the week is out.

@eoinfennessy eoinfennessy mentioned this issue Apr 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Ambient Beta Must have for Beta of Ambient Mesh area/ambient Issues related to ambient mesh area/networking/cni Istio CNI-related issues area/user experience good first issue Indicates a good first issue for new contributors kind/docs kind/enhancement
Projects
None yet
Milestone
No milestone
3 participants
@bleggett @istio-policy-bot @eoinfennessy