Skip to content

[ir] Init Structure#115

Merged
arkodg merged 9 commits into
envoyproxy:mainfrom
arkodg:init-ir
Jun 28, 2022
Merged

[ir] Init Structure#115
arkodg merged 9 commits into
envoyproxy:mainfrom
arkodg:init-ir

Conversation

@arkodg
Copy link
Copy Markdown
Contributor

@arkodg arkodg commented Jun 19, 2022

Relates to #32

Signed-off-by: Arko Dasgupta arko@tetrate.io

@arkodg arkodg requested a review from a team as a code owner June 19, 2022 23:36
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 19, 2022 •
edited by codecov Bot
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (212bdce) to head (38decee).
⚠️ Report is 4359 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff            @@
##              main      #115   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines            5         5           
=========================================
  Hits             5         5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

danehans
danehans reviewed Jun 20, 2022
View reviewed changes
Comment thread pkg/ir/ir.go Outdated
danehans
danehans reviewed Jun 20, 2022
View reviewed changes
Comment thread pkg/ir/ir.go Outdated
danehans
danehans reviewed Jun 20, 2022
View reviewed changes
Comment thread pkg/ir/ir.go Outdated
danehans
danehans reviewed Jun 20, 2022
View reviewed changes
Comment thread pkg/ir/ir.go Outdated
Copy link
Copy Markdown
Contributor

@danehans danehans Jun 20, 2022

Choose a reason for hiding this comment

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

Why doesn't HttpListener have an address field that represents an IPv4/IPv6 address?

Copy link
Copy Markdown
Contributor

@youngnick youngnick Jun 21, 2022

Choose a reason for hiding this comment

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

I actually think we may need to put that in the parent structure maybe? In the Gateway API, it's possible to describe:

  • listeners bound to no specific address (that is, the data plane should just figure it out)
  • one or more listeners bound to one or more addresses.

So, we will need to be able to describe "this listener or these listeners should be attached to this address or these addresses" in the IR somehow.

Copy link
Copy Markdown
Contributor Author

@arkodg arkodg Jun 22, 2022

Choose a reason for hiding this comment

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

I added Address into HttpListener for now, both the approaches of moving address+port to top level or within listener spec all depend on what Gateway API translator prefers when breaking up the cases referred by
@youngnick, will defer to @skriss for his preference since he is designing the GW API translator

danehans
danehans reviewed Jun 20, 2022
View reviewed changes
Comment thread pkg/ir/ir.go Outdated
danehans
danehans reviewed Jun 20, 2022
View reviewed changes
Comment thread pkg/ir/ir.go Outdated
danehans
danehans reviewed Jun 20, 2022
View reviewed changes
Comment thread pkg/ir/ir.go Outdated
danehans
danehans reviewed Jun 20, 2022
View reviewed changes
Comment thread pkg/ir/ir.go Outdated
danehans
danehans reviewed Jun 20, 2022
View reviewed changes
Comment thread pkg/ir/ir.go Outdated
danehans
danehans suggested changes Jun 20, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@danehans danehans left a comment

Choose a reason for hiding this comment

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

This is a good start. I provided feedback throughout.

youngnick
youngnick suggested changes Jun 21, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

I agree this is a great start, my feedback is below.

Comment thread pkg/ir/ir.go Outdated
Comment thread pkg/ir/ir.go Outdated
Comment thread pkg/ir/ir.go Outdated
Copy link
Copy Markdown
Contributor

@youngnick youngnick Jun 21, 2022

Choose a reason for hiding this comment

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

I actually think we may need to put that in the parent structure maybe? In the Gateway API, it's possible to describe:

  • listeners bound to no specific address (that is, the data plane should just figure it out)
  • one or more listeners bound to one or more addresses.

So, we will need to be able to describe "this listener or these listeners should be attached to this address or these addresses" in the IR somehow.

Comment thread pkg/ir/ir.go Outdated
Copy link
Copy Markdown
Contributor

@youngnick youngnick Jun 21, 2022

Choose a reason for hiding this comment

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

I think we should try to keep xDS-isms out of the IR as much as possible, so this should probably have a different name.

We could just call it Gateway, GatewayConfig or something that doesn't overload a name, maybe?

Copy link
Copy Markdown
Contributor Author

@arkodg arkodg Jun 21, 2022

Choose a reason for hiding this comment

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

I envisioned this IR to be specific to xDS (consumed by the xDS translator that generates xDS), and another one for the Provisioner, if we want to club the two into one, agree, we need to come up with a higher level noun

@arkodg arkodg requested review from danehans and youngnick June 22, 2022 05:41
danehans
danehans suggested changes Jun 23, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@danehans danehans left a comment

Choose a reason for hiding this comment

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

The IR API semantics appear to align more closely with Istio Gateway and Virtual Service APIs than Gateway API Gateway and HTTPRoute APIs. Why?

Comment thread pkg/ir/xds.go Outdated
Comment on lines 24 to 28
Copy link
Copy Markdown
Contributor

@danehans danehans Jun 23, 2022

Choose a reason for hiding this comment

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

Can multiple listeners have the same hostname? I see that LDS and the Gateway spec require each listener to have a unique name, should we do the same with this IR?

Copy link
Copy Markdown
Contributor Author

@arkodg arkodg Jun 24, 2022

Choose a reason for hiding this comment

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

they can at the envoy layer https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto.html?highlight=virtualhost#config-route-v3-virtualhost

Comment thread pkg/ir/xds.go Outdated
Comment thread pkg/ir/xds.go Outdated
Comment thread pkg/ir/xds.go Outdated
Comment thread pkg/ir/xds.go Outdated
Comment thread pkg/ir/xds.go Outdated
Comment thread pkg/ir/xds.go Outdated
Copy link
Copy Markdown
Contributor

@danehans danehans Jun 23, 2022

Choose a reason for hiding this comment

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

With this structure, a route can contain only one rule. Why not support multiple rules where each rule can match, route, etc?

Copy link
Copy Markdown
Contributor Author

@arkodg arkodg Jun 24, 2022

Choose a reason for hiding this comment

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

below field Matchers []route.HeaderMatcher allows the Gateway API translator to add/ AND multiple match params, the Gateway API allows the user to express OR matches too which might be a legit user facing concern, at the IR layer, we are targeting layered translations and OR rules are not needed here, imho the Gateway API translator should handle this
cc @skriss

@danehans danehans mentioned this pull request Jun 23, 2022
Merged
@arkodg
Copy link
Copy Markdown
Contributor Author

arkodg commented Jun 24, 2022

The IR API semantics appear to align more closely with Istio Gateway and Virtual Service APIs than Gateway API Gateway and HTTPRoute APIs. Why?

IR is not meant to mimic an exact Front end API, but be similar enough that other front end APIs / translators can translate to it, with the same intent of converting the IR to xDS. I made these changes with translating Gateway API in mind, sounds like a bonus 😊 if it also is similar to Istio APIs .

@arkodg arkodg requested a review from danehans June 24, 2022 01:33
@danehans danehans mentioned this pull request Jun 24, 2022
Closed
danehans
danehans suggested changes Jun 24, 2022
View reviewed changes
Comment thread pkg/ir/xds.go Outdated
Comment thread pkg/ir/xds.go Outdated
Comment thread pkg/ir/xds.go Outdated
Comment thread pkg/ir/xds.go Outdated
@arkodg arkodg requested a review from danehans June 24, 2022 22:41
danehans
danehans previously approved these changes Jun 24, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@danehans danehans left a comment

Choose a reason for hiding this comment

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

Thanks for all the effort to get this done.

Arko Dasgupta added 7 commits June 27, 2022 09:46
[ir] Init Structure
4836226 
Relates to envoyproxy#32

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
rename to xds.go
8b8abff 
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
api field names
d479ce1 
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
address comments
734d1db 
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
fix lint
68588e7 
```
pkg/ir/xds.go:13:2: var-naming: struct field Http should be HTTP (revive)
	Http []HTTPListener
	^
pkg/ir/xds.go:28:2: var-naming: struct field Tls should be TLS (revive)
	Tls TLSListenerSettings
```

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
address comments
43670d5 
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
SimpleTLS
4c6583c 
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg arkodg requested a review from danehans June 27, 2022 16:48
rm doc edits
e09ddce 
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
danehans
danehans previously approved these changes Jun 27, 2022
View reviewed changes
skriss
skriss reviewed Jun 28, 2022
View reviewed changes
Comment thread pkg/ir/xds.go Outdated
Comment thread pkg/ir/xds.go Outdated
address steve's comments
38decee 
* rm TLSMode, bring it back when its needed

* use internal match field

* create more match fields for path and query params

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg arkodg requested review from danehans and skriss June 28, 2022 21:15
kflynn
kflynn approved these changes Jun 28, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@kflynn kflynn left a comment

Choose a reason for hiding this comment

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

LGTM, FWIW. 🙂

LukeShu
LukeShu reviewed Jun 28, 2022
View reviewed changes
Comment thread pkg/ir/xds.go
@arkodg arkodg merged commit d530327 into envoyproxy:main Jun 28, 2022
@arkodg arkodg deleted the init-ir branch June 28, 2022 22:41
danehans
danehans reviewed Jun 28, 2022
View reviewed changes
Comment thread go.mod
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@skriss skriss Awaiting requested review from skriss

5 more reviewers

@LukeShu LukeShu LukeShu left review comments

@danehans danehans danehans approved these changes

@alexgervais alexgervais alexgervais approved these changes

@kflynn kflynn kflynn approved these changes

@youngnick youngnick youngnick approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@arkodg @codecov-commenter @LukeShu @alexgervais @kflynn @danehans @skriss @youngnick