search

istio / api

API definitions for the Istio project
Apache License 2.0
456 stars 551 forks source link

Rename `v1alpha2.RouteRule` --> `v1alpha2.VirtualHost` #342

Closed mandarjog closed 6 years ago

mandarjog commented 6 years ago

@mandarjog commented on Thu Feb 01 2018

The v2 route rule is a collection of rules and associated hosts. This is usually understood and modeled as Virtual host.

We should rename this.

@mandarjog commented on Thu Feb 01 2018

/cc @louiscryan @PiotrSikora @rshriram

@PiotrSikora commented on Thu Feb 01 2018

Agreed.

@kyessenov commented on Thu Feb 01 2018

How does VirtualHost transpire other protocols? In proxies, usually virtualhosts are nested under HTTP manager.

On Thu, Feb 1, 2018 at 1:37 PM Piotr Sikora notifications@github.com wrote:

Agreed.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/istio/istio/issues/3073#issuecomment-362410063, or mute the thread https://github.com/notifications/unsubscribe-auth/AJGIxiGM955A5ngFMAa4-AVAiq8Pn-Zmks5tQi6VgaJpZM4R2T9_ .

@PiotrSikora commented on Thu Feb 01 2018

Yeah, something like Host or Server might make more sense.

@rshriram commented on Thu Feb 01 2018

Which is what we have at top of the rule. A list of hosts. And route rules will apply for TCP/UDP/HTTP and other protocols as well.

@rshriram commented on Thu Feb 01 2018

can you close and move this issue to istio/api ?

kyessenov commented 6 years ago

@rshriram it's just a perception mismatch. The route rule content is a virtual host with a bunch of route rules. Calling that content a route rule is confusing (a thing that contains many things of the same kind). VirtualHost/Host/Server/RouteRuleSet would all be better I think.

kyessenov commented 6 years ago

A side benefit of the rename would be that we don't have to worry about RouteRule deprecation. v1alpha2 offers an entirely new type of a config object.

rshriram commented 6 years ago

On Fri, Feb 2, 2018 at 12:34 AM Kuat notifications@github.com wrote:

A side benefit of the rename would be that we don't have to worry about RouteRule deprecation. v1alpha2 offers an entirely new type of a config object.

I would much rather call it ClientRule instead of overloading http virtual host or a generic server or any such thing.

Remember the top level hosts can have sub nets and the rule could be all about tcp. A host or server does not really make any sense in this case because we are looking at an entire network, matching by ports and routing traffic.

If this becomes clientrule, we can change destination rule to service rule that encompasses subsets, etc. I think me Louis and zack went through this motion once and we dropped the idea iirc.

—

You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/istio/api/issues/342#issuecomment-362491254, or mute the thread https://github.com/notifications/unsubscribe-auth/AH0qdy4_jFgetguaezOiXhKLQ91M9d6wks5tQp5dgaJpZM4R2WUW .

mandarjog commented 6 years ago

The current name RouteRule does not make sense in any of those cases. It certainly is not a single rule; should at least be a RuleSet and I think client is redundant here.

kyessenov commented 6 years ago

We can also drop "rule" from the name, and call it "Route", with a potential to confuse users with OpenShift Routes. It doesn't make sense to call a rule something that has many rules.

rshriram commented 6 years ago

I am neutral on the question of whether to rename at all. Will let @louiscryan decide.

But if we do decide to rename RouteRule, I think we should pick what expresses this concept better than worrying about product specific name collisions.

TrafficRuleSet? RouteRuleSet? I think this one strikes a nice compromise when it comes to users who have already become familiar with route rules.

On Fri, Feb 2, 2018 at 1:21 PM Kuat notifications@github.com wrote:

We can also drop "rule" from the name, and call it "Route", with a potential to confuse users with OpenShift Routes. It doesn't make sense to call a rule something that has many rules.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/istio/api/issues/342#issuecomment-362650038, or mute the thread https://github.com/notifications/unsubscribe-auth/AH0qd1eazT3KUSvyOqVAAgsBQSCEdo95ks5tQ0V-gaJpZM4R2WUW .

wora commented 6 years ago

I took a look at the routing package and found it is a bit hard to understand. There is no README.md at the root directory, which is typically required to provide an overview of the entire API. It should document a couple of key use cases for people to get started to understand the API.

Once we have proper documentation, we can ask a few people around and see what is the best name could be. My concern is more about the over complexity than what is the proper name.

rshriram commented 6 years ago

On Sat, Feb 3, 2018 at 11:53 PM Hong Zhang notifications@github.com wrote:

I took a look at the routing package and found it is a bit hard to understand. There is no README.md at the root directory, which is typically required to provide an overview of the entire API. It should document a couple of key use cases for people to get started to understand the API.

Once we have proper documentation, we can ask a few people around and see what is the best name could be. My concern is more about the over complexity than what is the proper name.

We have lots of docs. Not just in the readme as we found most people (outside google) don’t refer to protos to write configs. The readme content is present in the protos itself, as we now have an excellent proto doc gen thanks to @geeknoid. It makes it easy to maintain docs and config in same file.

General overview is here.

https://istio.io/docs/concepts/traffic-management/request-routing.html

—

You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/istio/api/issues/342#issuecomment-362880861, or mute the thread https://github.com/notifications/unsubscribe-auth/AH0qd-GE8oCJkM5SfOgsY4FICj2xtYF-ks5tRTetgaJpZM4R2WUW .

wora commented 6 years ago

Please create a README.md and point people to the public documentation. I know all Google APIs are documented using the proto comments. At certain complexity level, the comments become too hard to understand. I think the overview is quite easy to understand, but this proto schema is quite complicated. I may offer some feedback if I can spend more time to read through it.

geeknoid commented 6 years ago

​Each proto package now includes a proto file with the marker $location in it. This indicates the exact URL where the documentation for the package can be found.​

On Mon, Feb 5, 2018 at 8:06 AM, Hong Zhang notifications@github.com wrote:

Please create a README.md and point people to the public documentation. I know all Google APIs are documented using the proto comments. At certain complexity level, the comments become too hard to understand. I think the overview is quite easy to understand, but this proto schema is quite complicated. I may offer some feedback if I can spend more time to read through it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/istio/api/issues/342#issuecomment-363131265, or mute the thread https://github.com/notifications/unsubscribe-auth/AVucHfbGyr2uzbHxB73BgGF6_yP-bXbQks5tRycagaJpZM4R2WUW .

louiscryan commented 6 years ago

I think the arguments about plurality here are mostly a red herring. A single 'rule' can represent multiple outcomes without needing to be plural per se. Also I think ClientRule is out because of the property of Gateway binding.

VirtualHost is somewhat attractive, as would be VirtualService

kyessenov commented 6 years ago

VirtualService it is.