Istio API Feature Status using proto annotations by zhlsunshine · Pull Request #2013 · istio/api

zhlsunshine · 2021-06-01T07:00:29Z

According to proposal design doc, FieldOptions of proto3 can be used to implement this feature label, and I just change the code for Export To feature as PoC work.

Thank you for your time for reviewing this PR! ^_^

istio-policy-bot · 2021-06-01T07:00:31Z

😊 Welcome @zhlsunshine! This is either your first contribution to the Istio api repo, or it's been
awhile since you've been here.

You can learn more about the Istio working groups, code of conduct, and contributing guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

zhlsunshine · 2021-06-01T07:02:12Z

/test release-notes_api

therealmitchconnors · 2021-06-02T01:49:26Z

Adding @jasonwzm who authored this design.

jasonwzm · 2021-06-02T23:42:18Z

@zhlsunshine Thanks for working on this! The feature name should be from the features defined in Istio repos. Do you plan to help implement the proto feature status? If so, we should definitely sync before more work is done this. :) Your contribution is much appreciated!

zhlsunshine · 2021-06-03T02:48:27Z

@jasonwzm yes, it will be great if I can help on the proto feature status. And I also hope to sync with u @jasonwzm @therealmitchconnors for the next step. ^_^

zhlsunshine · 2021-07-02T06:32:15Z

/test build_api

google-cla · 2021-07-30T07:16:08Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

zhlsunshine · 2021-07-30T07:19:31Z

I have already completed the re-based, thanks!

google-cla · 2021-08-17T07:30:31Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

google-cla · 2021-08-17T10:16:21Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

common-protos/istio.io/extensions/field_feature.proto

brian-avery · 2021-08-25T14:23:05Z

This looks good to me.

howardjohn · 2021-08-25T15:57:26Z

networking/v1alpha3/destination_rule.proto

  // the destination rule is declared in. Similarly, the value "*" is reserved and
  // defines an export to all namespaces.
-  repeated string export_to = 4;
+  repeated string export_to = 4 [(istio.extensions.feature) = {status: "alpha", name: "dr-export-to", id: "traffic.sidecar"}]; 


Putting a name on every field seems extremely tedious. Do we need it? It already has a name

Yes, sure, it's tedious, however besides helping to categorize all Isito features in alpha, beta or maturity, it will also be more cautious when changing or creating features for Istio API, and the owners of feature need to add this label by themselves.

How is it helpful to give it another name though? It seems we are just repeating the name (in a slightly different, form, that is likely inconsistent between every field). For example, we could automatically derive this is name: "DestinationRule.spec.exportTo".

This form already has meaning as well:

$ kubectl explain DestinationRule.spec.exportTo KIND: DestinationRule VERSION: networking.istio.io/v1beta1 FIELD: exportTo <[]string> DESCRIPTION: A list of namespaces to which this destination rule is exported.

there are 3 fields in optional properties: status, name and id for field exportTo. I think both status and id are clear. It seems that every feature name in features.yaml is high level and large-scope, so name could contain more detail information for specified field to some features. A field of specified resource may support more than one features and one feature may also include more than one fields. So I think both id comes from features.yaml and name defined by feature author can help to accurately reflect the relationship between feature and field of the resource.

So:

Status - this is clear, its the new identifier we are adding

id - this is clear, its how we associate a field with a feature

But name - the field already has a name. It doesn't need two names.

name defined by feature author can help to accurately reflect the relationship between feature and field of the resource.

This sounds like you are saying its the name of the feature, not the field? In which case, why do we need id and name?

Isn't traffic.sidecar the name of the feature?

I also don't see how this allows for multiple features?

traffic.sidecar should be a high level feature name, and more detail may be helpful and easy for user to trace the status and progress. Such as override injection for XXX for traffic.sidecar.
For multiple features, I think the optional id can be traffic.sidecar|traffic.control, and name can be feature_name_1|feature_name_2 for multiple features.

Hi @howardjohn, how about change like this:

make the field option looks like this repeated string export_to = 4 [(istio.extensions.feature) = {status: "alpha", id: "traffic.sidecar"}]; and keep the definition of field option istio.extensions.feature.name.

init istio.extensions.feature.name with its message name + field name when using this field option in later.
Does it make sense?

I am not sure what that helps with or how we will fill in that setting?

Hi @howardjohn according to the design doc, the property istio.extensions.feature.name should be kept, and at least this property can support more detail of new feature even if it can not be filled for now. So @therealmitchconnors & @jasonwzm, any idea from you?

networking/v1alpha3/service_entry.proto

louiscryan · 2021-08-25T17:40:24Z

common-protos/istio.io/extensions/field_feature.proto

+	// label the feature's status, available status values may be alpha, beta and stable 
+	// there would be experimental in future.
+	optional string status = 1;
+	// show the feature field for specified resource, such as vs-export-to shows the virtualservice's field export_to,


What do we mean by 'show' here? I can't quite tell if it's

As a link in user-facing docs to the feature definition & status (seems like that would be derived via 'id')

As a means to 'name' the above or at least ensure that there is a reconciled name defined either here or in features.yaml

@louiscryan already existing features.yaml is for user-facing docs, and this change is used for doing statistics of API feature according to feature status. Generally, feature author needs to add this optional properties based on API field level when they are working on a new feature. Moreover, this optional properties should be mapped with features.yaml via its id, so there is a optional property named id in message IstioFeature. For your question, the name here would contains more detail information than in features.yaml because a new feature maybe include more than one API fields.

google-cla · 2021-08-26T09:54:42Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

linsun · 2021-08-28T01:02:29Z

networking/v1alpha3/destination_rule.proto

  // the destination rule is declared in. Similarly, the value "*" is reserved and
  // defines an export to all namespaces.
-  repeated string export_to = 4;
+  repeated string export_to = 4 [(istio.extensions.feature) = {status: "alpha", name: "dr-export-to", id: "traffic.control"}]; 


what is a bit confusing is that you highlight this export_to feature status as alpha and API is also alpha. Do you mean this feature is alpha while the rest of DR is beta?

What is the criteria for adding the istio feature status out of so many fields in DR or any other resource?

Q: what is a bit confusing is that you highlight this export_to feature status as alpha and API is also alpha. Do you mean this feature is alpha while the rest of DR is beta?
A: In fact, user may pay more attention on the status of these new imported features which may be alpha or beta or experimental, and user also want to know more details about the status and progress for these features, so it should not be key point for the rest which is not highlighted. And @jasonwzm please correct me if I am wrong.

Q: What is the criteria for adding the istio feature status out of so many fields in DR or any other resource?
A: Thanks for this question. In spite of more details for field based on feature status, I think this doc of the criteria still can be a great reference.

common-protos/istio.io/extensions/field_feature.proto

linsun · 2021-08-28T01:05:48Z

common-protos/istio.io/extensions/field_feature.proto

+	// there would be experimental in future.
+	optional string status = 1;
+	// show the feature field for specified resource, such as vs-export-to shows the virtualservice's field export_to,
+	// it can be defined by feature owner.


any approval process needed? or just the normal PR approval?

also, how a feature owner know which ones they should add this status field? (asked below in the example below)

Q: any approval process needed? or just the normal PR approval?
A: I think normal PR approval is enough, because API repo change is a significant thing. Any other idea?

Q: how a feature owner know which ones they should add this status field? (asked below in the example below)
A: I think doc should be a great referenced criteria for feature owner. Beside that, I think proposal doc or PR review would be a right place to discuss the status field.

google-cla · 2021-08-30T05:52:13Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

google-cla · 2021-08-30T05:57:38Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

howardjohn · 2021-08-30T15:17:32Z

Can we build the tooling that makes these fields useful before merging it into the API? my concern is we will end up like the test frameworks features thing - it adds a significant burden to developers, but has yet to be useful. Before we go add another hurdle to merging a PR, we need to make sure its actually giving us a benefit today

zhlsunshine · 2021-08-31T02:38:20Z

Can we build the tooling that makes these fields useful before merging it into the API? my concern is we will end up like the test frameworks features thing - it adds a significant burden to developers, but has yet to be useful. Before we go add another hurdle to merging a PR, we need to make sure its actually giving us a benefit today

Hi @howardjohn, I understand your worry about it. However, this PR is the first phase to this API feature status, and I will build a tool to make these fields useful based on the design doc once this PR can be merged. According to the design, there would be a sub-command named istioctl feature-usage to handle Istio features and use these fields.

google-cla · 2021-09-03T02:55:56Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

zhlsunshine · 2021-12-09T08:11:31Z

Hi @howardjohn new submit to remove proto annotations of name based on comment here , I will fetch the necessary information related to name as kubectl explain DestinationRule.spec.exportTo does, then list them for display purpose.

zhlsunshine · 2022-02-23T10:08:35Z

Hi @howardjohn, after some thinking and discussion, I think it's an agree on dropping the IstioFeature field name, and just 2 fields left as below:

message IstioFeature {
	// label the feature's status, available status values may be alpha, beta and stable 
	// there would be experimental in future.
	optional FeatureStatus status = 1;
	// id means the feature id which can be mapped here: https://github.com/istio/enhancements/blob/master/features.yaml
	// it should be contained in id section of features under this yaml file.
	optional string id = 2;
}

Could you please help to check and approve it? Thanks! ^_^

common-protos/istio.io/extensions/istio_feature.proto

howardjohn · 2022-03-01T23:21:26Z

networking/v1alpha3/destination_rule.proto

  // the destination rule is declared in. Similarly, the value "*" is reserved and
  // defines an export to all namespaces.
-  repeated string export_to = 4;
+  repeated string export_to = 4 [(istio.extensions.feature) = {status: STABLE, id: "traffic.control"}];


what happens when we have a field that is associated with multiple features?

Generally speaking, multiple features should have the same feature status if they are included in the same field, even if feature status is not the same, it's okay to provide more information in https://github.com/istio/enhancements/blob/master/features.yaml or append additional feature id to field id as well. Moreover, this situation is uncommon.

jacob-delgado · 2022-03-14T17:25:47Z

common-protos/istio.io/extensions/istio_feature.proto

+	optional FeatureStatus status = 1;
+	// id means the feature id which can be mapped here: https://github.com/istio/enhancements/blob/master/features.yaml
+	// it should be contained in id section of features under this yaml file.
+	optional string id = 2;


Given John's comment below I think this would be best suited as repeated string id = 2;. A repeated item that's empty is essentially optional.

@jacob-delgado okay, I will add repeated for field id. BTW, I think field status should be necessary to add repeated as well.

istio-testing · 2022-03-14T22:10:17Z

@zhlsunshine: PR needs rebase.

Details

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.

jacob-delgado

LGTM. You should bring this to the APAC meeting next week to get the RFC approved.

zhlsunshine · 2022-03-18T00:31:38Z

LGTM. You should bring this to the APAC meeting next week to get the RFC approved.

great idea!

zhlsunshine requested a review from a team as a code owner June 1, 2021 07:00

google-cla bot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Jun 1, 2021

istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 1, 2021

zhlsunshine added the release-notes-none Indicates a PR that does not require release notes. label Jun 1, 2021

howardjohn assigned jasonwzm Jun 1, 2021

zhlsunshine requested review from dcberg, duderino, linsun, louiscryan, nrjpoddar and smawson as code owners June 2, 2021 01:44

istio-testing added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 2, 2021

zhlsunshine force-pushed the feature branch from a89bbab to 75bd2b4 Compare June 3, 2021 06:22

zhlsunshine requested review from brian-avery and howardjohn as code owners July 2, 2021 06:28

istio-testing added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 2, 2021

google-cla bot added cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. and removed cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. labels Jul 30, 2021

istio-testing removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 30, 2021

brian-avery reviewed Aug 25, 2021

View reviewed changes

common-protos/istio.io/extensions/field_feature.proto Show resolved Hide resolved

howardjohn reviewed Aug 25, 2021

View reviewed changes

networking/v1alpha3/service_entry.proto Outdated Show resolved Hide resolved

louiscryan reviewed Aug 25, 2021

View reviewed changes

linsun reviewed Aug 28, 2021

View reviewed changes

common-protos/istio.io/extensions/field_feature.proto Outdated Show resolved Hide resolved

linsun reviewed Aug 28, 2021

View reviewed changes

esnible mentioned this pull request Sep 2, 2021

Istio UX 1.12 roadmap istio/istio#34682

Closed

11 tasks

new submit for code rebasing

7048719

howardjohn reviewed Mar 1, 2022

View reviewed changes

jacob-delgado reviewed Mar 14, 2022

View reviewed changes

jacob-delgado reviewed Mar 17, 2022

View reviewed changes

Conversation

zhlsunshine commented Jun 1, 2021

Uh oh!

istio-policy-bot commented Jun 1, 2021

Uh oh!

zhlsunshine commented Jun 1, 2021

Uh oh!

therealmitchconnors commented Jun 2, 2021

Uh oh!

jasonwzm commented Jun 2, 2021

Uh oh!

zhlsunshine commented Jun 3, 2021

Uh oh!

zhlsunshine commented Jul 2, 2021

Uh oh!

google-cla bot commented Jul 30, 2021

Uh oh!

zhlsunshine commented Jul 30, 2021

Uh oh!

google-cla bot commented Aug 17, 2021

Uh oh!

google-cla bot commented Aug 17, 2021

Uh oh!

Uh oh!

brian-avery commented Aug 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhlsunshine Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhlsunshine Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-cla bot commented Aug 26, 2021

Uh oh!

linsun Aug 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-cla bot commented Aug 30, 2021

Uh oh!

google-cla bot commented Aug 30, 2021

Uh oh!

howardjohn commented Aug 30, 2021

Uh oh!

zhlsunshine commented Aug 31, 2021

Uh oh!

google-cla bot commented Sep 3, 2021

Uh oh!

zhlsunshine commented Dec 9, 2021

brian-avery commented Aug 25, 2021 •

edited

Loading

zhlsunshine Sep 3, 2021 •

edited

Loading

zhlsunshine Aug 26, 2021 •

edited

Loading

linsun Aug 28, 2021 •

edited

Loading