Skip to content

Add targetRef field to Istio CRDs that use workloadSelector #2885

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

Closed
6 tasks done
Tracked by #45713
MorrisLaw opened this issue Aug 7, 2023 · 5 comments · Fixed by #2888
Closed
6 tasks done
Tracked by #45713

Add targetRef field to Istio CRDs that use workloadSelector #2885

MorrisLaw opened this issue Aug 7, 2023 · 5 comments · Fixed by #2888
Assignees

Comments

@MorrisLaw
Copy link
Contributor

MorrisLaw commented Aug 7, 2023

These tasks can be done in one PR or in multiple PRs. Only caveat is that the targetRef proto definition needs to exist first before being added as a field to the different places we want to update, so we'll need to add that as a first step.

Note: Be sure to run make gen-proto locally after a proto update to ensure the necessary protobuf related files are generated. Documentation in the proto is also referenced by some automation in place which will generate docs off of it. Look at some examples like the PR for jwt.proto to see how documentation within the proto can look. Last but not least, be sure to include release notes.

@MorrisLaw MorrisLaw self-assigned this Aug 7, 2023
@howardjohn
Copy link
Member

I would not put it under gateway but rather types. It is specific to Gateway today but likely not long term.

@jaellio
Copy link
Contributor

jaellio commented Aug 7, 2023

@howardjohn How do we version types? For example, the WorkloadSelector is v1beta1, and is referenced in the v1 AthorizationPolicy. Thanks!

@howardjohn
Copy link
Member

It's basically meaningless. Top level types (like AuthorizationPolicy) need to be identical (in yaml) across all versions.

Internal types like workload selector has no relevance for version and probably shouldn't have one at all (but does due to backwards compatible)

@hzxuzhonghu
Copy link
Member

Wanna to know what target do we need to support? If i read correctly from other doc, the target is gateway for waypoint.

how could targetref coexist with workload selector if we need to apply a policy both to a workload and service or someother object

@MorrisLaw
Copy link
Contributor Author

Wanna to know what target do we need to support? If i read correctly from other doc, the target is gateway for waypoint.

how could targetref coexist with workload selector if we need to apply a policy both to a workload and service or someother object

Hey @hzxuzhonghu, I'm not sure I fully understand both of your questions but I'm going to try. Are you referring to the following lines from the doc?

Each resource listed in the Scope section of this proposal will have a new targetRef
field added to their proto. At present gateway.networking.k8s.io/Gateway is the only valid Kind for this field.

It seems like the main question is "How can targetref coexist with workload selector if we need to also apply policy to a workload and service or other objects"?

The document addresses this I believe:

In the waypoint, this means binding policies to the waypoint itself. This will be done by introducing a TargetRef field into all workload selector API types, aligning with the "policy attachment" concept in Gateway API.

There are several Istio CRDs that use workloadSelector; however, not all of them are relevant for Ambient. Thus, the scope for this proposed change is limited to the following resources:
AuthorizationPolicy
Telemetry
WasmPlugin
ProxyConfig
RequestAuthentication

Waypoint proxies will accept label selectors if they match AND there is no targetRef specified

So I think that if NO targetRef is specified, then we'll default to workload selector. BUT if there is a targetRef specified then we'll use that instead. That's how they could co-exist initially. After some number of releases we'll want to switch over to only using targetRef and ignoring label selectors.

cc/ @keithmattix (please fill in any details, or correct me if any of this doesn't make sense)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants