Update STYLE-GUIDE.md #343

wora · 2018-02-01T22:56:52Z

Mention the CRD kind must match proto message.

ayj · 2018-02-01T23:21:45Z

STYLE-GUIDE.md

@@ -38,7 +38,8 @@ naming convention:
 ## Versioning

 When defining Kubernetes Custom Resource Definition (CRD) using
-proto3, the proto versioning should match the Kubernetes versioning,
+proto3, the proto versioning must match the Kubernetes versioning,
+and the proto message type must match the CRD kind name.


Should group names and version also be consistent with CRDs? We've defined versioned package in istio/api but these all map to the same CRD group and version (config.istio.io).

For example, does the following mapping make sense?

group = routing.api.istio.io version = v1alpha2 kind = RouteRule group = rbac.api.istio.io version = v1alpha1 kind = ServiceRoleBinding

@ayj this is exactly what I am proposing: https://groups.google.com/d/msg/istio-config/CzH6nRBT_o0/3lvIBao_AgAJ

I like this to be consistent with api group too.

Yes. I should have made that more clear. The CRD apiVersion must match proto package name, minus the .io suffix, and the ordering. I will update the text.

These are apiGroup. not apiVersion.

Mention `apiVersion` must match proto package.

ayj · 2018-02-02T18:18:57Z

STYLE-GUIDE.md

+* The proto `package` name must match the Kubernetes `apiVersion`,
+  excluding the `.io` DNS suffix and reversing the DNS segment
+  ordering.
+* The proto message type must match the CRD `kind` name.


I'm not sure if its worth highlighting here, but the apiVersion as shown in the YAML snippet is the concatenation of the Kubernetes API group and version. In the example below, config.istio.io is the group and v1alpha is the version of that group. Together they identify the versioned schema the object should have.

Maybe in the yaml example, listing out a full CRD definition. We probably also need to make rule that CRD resource name should be in format of plurals + api group.

apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
name: serviceclasses.config.istio.io
spec:
group: config.istio.io
version: v1alpha1
names:
plural: serviceclasses
singular: serviceclass
kind: ServiceClass

Can we address this question in another PR? Let's make some incremental improvement first.

Explain apiVersion = group/version.

wora · 2018-02-02T21:10:03Z

I addressed the concern of apiVersion vs group/version.

wora · 2018-02-04T18:38:12Z

@geeknoid Can you merge this PR?

ozevren · 2018-02-05T17:32:06Z

With this guidance, is it possible to create meta-APIs such as LROs? It looks like requiring 1-1 mapping between proto package name and apigroup seems to be work against this.

geeknoid · 2018-02-05T19:01:54Z

STYLE-GUIDE.md

@@ -38,8 +38,14 @@ naming convention:
 ## Versioning

 When defining Kubernetes Custom Resource Definition (CRD) using
-proto3, the proto versioning should match the Kubernetes versioning,
-see the following example.
+`proto3`, follow the following guidelines:


, follow these guidelines:

geeknoid · 2018-02-05T19:08:20Z

@ozevren Doesn't the sentence "When defining Kubernetes Custom Resource Definition" cover the case you're worried about? LRO is specifically not a CRD.

ozevren · 2018-02-05T19:48:06Z

Meant to use LRO as an example. It is a meta-API. We're using CRDs to create a public API surface (which happens to be backed by the API Server). Adding this constraint implies you cannot create a meta set of protos to use as top-level mix-in resources. And, it also means you cannot re-use top-level CRD protos for another "API group" without violating this rule (i.e. think of the Config Click Stop-0 we've been talking about).

…

On Mon, Feb 5, 2018 at 11:08 AM, Martin Taillefer ***@***.***> wrote: @ozevren <https://github.com/ozevren> Doesn't the sentence "When defining Kubernetes Custom Resource Definition" cover the case you're worried about? LRO is specifically not a CRD. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#343 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQR8I9wNNBBl6rar1jC8o1Cb8cQiurUHks5tR1GlgaJpZM4R2azS> .

wora · 2018-02-05T21:31:50Z

K8s APIs support watch semantics, which is equivalent to LRO. I don't think watch semantics is implemented by any api group. It is more like a design pattern. We need to address it next, but I don't think it is blocked by this discussion.

geeknoid · 2018-02-05T22:16:56Z

Oz was using LRO as an example of a pattern, not as a specific case. As stated right now, the guide doesn't allow for common definitions that can be shared between multiple APIs because we are forcing a 1:1 mapping pattern. We just need to add more nuance to the text to allow for those cases.

…

On Mon, Feb 5, 2018 at 1:31 PM, Hong Zhang ***@***.***> wrote: K8s APIs support watch semantics, which is equivalent to LRO. I don't think watch semantics is implemented by any api group. It is more like a design pattern. We need to address it next, but I don't think it is blocked by this discussion. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#343 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AVucHej-MOnpDo5l7o2QY3Fw194ibVMaks5tR3NHgaJpZM4R2azS> .

rshriram · 2018-02-05T22:39:27Z

I have no idea about LROs, but as such, I am not against this proposal for routing section.

wora · 2018-02-09T03:32:27Z

Can someone merge this PR or let me any further feedback? Thanks.

* Update STYLE-GUIDE.md Mention the CRD kind must match proto message. * Update STYLE-GUIDE.md Mention `apiVersion` must match proto package. * Update STYLE-GUIDE.md Explain apiVersion = group/version.

* ApiManager uses ConfigManager to download the service config * Removed uncessary header file inclusion * Moved all initialization into Init() * Reverted copyright year to 2016 * Reverted copyright year to 2016 * Added a local auto variable for ServiceContext creation * Fixed backward compatibility * Added the rollout_strategy check * Added service_config argument to ConfigManager * Moved service_config_ variable to GlobalContext class * Fixed code formatting * Reverted storing service_config in GlobalContext * Renamed and changed data type of const * Moved callback function argument from constructor to Init

Adds configuration and API to support token exchange grants.

Update STYLE-GUIDE.md

2eacb18

Mention the CRD kind must match proto message.

wora requested review from douglas-reid, geeknoid and rshriram as code owners February 1, 2018 22:56

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Feb 1, 2018

douglas-reid approved these changes Feb 1, 2018

View reviewed changes

ayj reviewed Feb 1, 2018

View reviewed changes

Update STYLE-GUIDE.md

f3b04be

Mention `apiVersion` must match proto package.

ayj reviewed Feb 2, 2018

View reviewed changes

Update STYLE-GUIDE.md

cb0193b

Explain apiVersion = group/version.

geeknoid reviewed Feb 5, 2018

View reviewed changes

rshriram approved these changes Feb 5, 2018

View reviewed changes

xiaolanz mentioned this pull request Feb 6, 2018

Add Group/Version per schema istio/istio#3214

Merged

geeknoid approved these changes Feb 9, 2018

View reviewed changes

geeknoid merged commit c68809c into istio:master Feb 9, 2018

xiaolanz mentioned this pull request Feb 13, 2018

Istio API governance istio/istio#3457

Closed

5 tasks

xiaolanz mentioned this pull request Feb 17, 2018

Allow individual api group and version for each config resource kind in Pilot config store istio/istio#3586

Closed

nacx pushed a commit to nacx/api that referenced this pull request Feb 23, 2022

IAM: oauth2 token exchange grant (istio#343)

e5c1368

Adds configuration and API to support token exchange grants.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update STYLE-GUIDE.md #343

Update STYLE-GUIDE.md #343

wora commented Feb 1, 2018

ayj Feb 1, 2018

xiaolanz Feb 1, 2018

wora Feb 1, 2018

xiaolanz Feb 1, 2018

ayj Feb 2, 2018

xiaolanz Feb 2, 2018

wora Feb 2, 2018

wora commented Feb 2, 2018

wora commented Feb 4, 2018

ozevren commented Feb 5, 2018

geeknoid Feb 5, 2018

geeknoid commented Feb 5, 2018

ozevren commented Feb 5, 2018 via email

wora commented Feb 5, 2018

geeknoid commented Feb 5, 2018 via email

rshriram commented Feb 5, 2018

wora commented Feb 9, 2018

Update STYLE-GUIDE.md #343

Update STYLE-GUIDE.md #343

Conversation

wora commented Feb 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wora commented Feb 2, 2018

wora commented Feb 4, 2018

ozevren commented Feb 5, 2018

Choose a reason for hiding this comment

geeknoid commented Feb 5, 2018

ozevren commented Feb 5, 2018 via email

wora commented Feb 5, 2018

geeknoid commented Feb 5, 2018 via email

rshriram commented Feb 5, 2018

wora commented Feb 9, 2018