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

Istio API Feature Status using proto annotations #2013

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zhlsunshine
Copy link

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! ^_^

@zhlsunshine zhlsunshine requested a review from a team as a code owner June 1, 2021 07:00
@istio-policy-bot
Copy link

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

@google-cla 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 istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 1, 2021
@zhlsunshine zhlsunshine added the release-notes-none Indicates a PR that does not require release notes. label Jun 1, 2021
@zhlsunshine
Copy link
Author

/test release-notes_api

@istio-testing 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
@therealmitchconnors
Copy link
Contributor

Adding @jasonwzm who authored this design.

@jasonwzm
Copy link
Member

jasonwzm commented Jun 2, 2021

@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
Copy link
Author

@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. ^_^

@istio-testing istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 3, 2021
@istio-testing 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
@zhlsunshine
Copy link
Author

/test build_api

@google-cla
Copy link

google-cla bot commented Aug 26, 2021

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@@ -238,7 +239,7 @@ message DestinationRule {
// The value "." is reserved and defines an export to the same namespace that
// 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"}];
Copy link
Member

@linsun linsun Aug 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

google-cla bot commented Aug 30, 2021

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

1 similar comment
@google-cla
Copy link

google-cla bot commented Aug 30, 2021

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@howardjohn
Copy link
Member

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
Copy link
Author

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.

@esnible esnible mentioned this pull request Sep 2, 2021
11 tasks
@google-cla
Copy link

google-cla bot commented Sep 3, 2021

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Sep 11, 2021
@istio-testing istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Dec 9, 2021
@istio-policy-bot istio-policy-bot added the lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. label Dec 9, 2021
@zhlsunshine zhlsunshine reopened this Dec 9, 2021
@zhlsunshine
Copy link
Author

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.

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 9, 2022
@istio-testing istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 23, 2022
@zhlsunshine zhlsunshine reopened this Feb 23, 2022
@zhlsunshine
Copy link
Author

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! ^_^

@@ -238,7 +239,7 @@ message DestinationRule {
// The value "." is reserved and defines an export to the same namespace that
// 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"}];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@istio-testing
Copy link
Collaborator

@zhlsunshine: 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 istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 14, 2022
Copy link
Contributor

@jacob-delgado jacob-delgado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@zhlsunshine
Copy link
Author

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

great idea!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. needs-rebase Indicates a PR needs to be rebased before being merged release-notes-none Indicates a PR that does not require release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.