Skip to content

translator: listener hostnames unset#286

Merged
danehans merged 1 commit intoenvoyproxy:mainfrom
zirain:listener-hostnames
Aug 30, 2022
Merged

translator: listener hostnames unset#286
danehans merged 1 commit intoenvoyproxy:mainfrom
zirain:listener-hostnames

Conversation

@zirain
Copy link
Copy Markdown
Member

@zirain zirain commented Aug 30, 2022

fixes: #272
depends on: #276

Signed-off-by: hejianpeng hejianpeng2@huawei.com

@zirain zirain requested a review from a team as a code owner August 30, 2022 08:50
Signed-off-by: hejianpeng <hejianpeng2@huawei.com>
@zirain zirain force-pushed the listener-hostnames branch from 17dfd36 to f0bffbb Compare August 30, 2022 09:28
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #286 (f0bffbb) into main (4215ceb) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #286      +/-   ##
==========================================
+ Coverage   58.14%   58.18%   +0.04%     
==========================================
  Files          31       31              
  Lines        2788     2791       +3     
==========================================
+ Hits         1621     1624       +3     
  Misses       1069     1069              
  Partials       98       98              
Impacted Files Coverage Δ
internal/gatewayapi/translator.go 85.51% <100.00%> (+0.07%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

}

irListener := xdsIR.GetListener(irListenerName(listener))
if len(irListener.Hostnames) == 0 {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

unsure why this condition is not being met via

if listener.Hostname != nil {

@skriss can you help review this one

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, in Gateway API the absense of any hostname means "match all hostnames" (see https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io/v1beta1.Listener -- "when unspecified, all hostnames are matched"). We should probably add an else clause after L429 to explicitly add the * to the IR Listener if no hostname is specified on the Gateway API listener.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@zirain can you incorporate steve's comments (add else after L429) as well as add some comments in the code stating the WHY. TIA !

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@skriss @arkodg since this PR is required for RC1, I created #293 to track this issue. Hopefully, we can get the issue fixed in RC1, but e2e is functional without it.

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.

@zirain thanks for the PR. If you have time please resolve #293. Otherwise, I'll

@danehans danehans merged commit 71139b7 into envoyproxy:main Aug 30, 2022
@zirain zirain deleted the listener-hostnames branch August 31, 2022 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

XDS: Envoy Fails to Add Router

5 participants