Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: allow hostname to use subdomain with single label/character #4803

Merged
merged 15 commits into from
Jan 6, 2025

Conversation

nothinux
Copy link
Contributor

@nothinux nothinux commented Nov 28, 2024

Fixes #4793

Release Notes: No

Signed-off-by: Taufik Mulyana <nothinux@gmail.com>
@nothinux nothinux requested a review from a team as a code owner November 28, 2024 03:33
@zhaohuabing
Copy link
Member

zhaohuabing commented Nov 28, 2024

@nothinux Thanks for fixing this! Could you please add a test to test/cel-validation/backend_test.go?

Signed-off-by: Taufik Mulyana <nothinux@gmail.com>
@nothinux
Copy link
Contributor Author

added

nothinux and others added 2 commits November 28, 2024 11:09
Signed-off-by: Taufik Mulyana <nothinux@gmail.com>
@nothinux
Copy link
Contributor Author

nothinux commented Nov 28, 2024

@zhaohuabing the coverage test failed because only 4 items are allowed. can we double this limit?

// +kubebuilder:validation:MaxItems=4
// +kubebuilder:validation:XValidation:rule="self.all(f, has(f.fqdn)) || !self.exists(f, has(f.fqdn))",message="fqdn addresses cannot be mixed with other address types"
Endpoints []BackendEndpoint `json:"endpoints,omitempty"`

@zhaohuabing
Copy link
Member

@zhaohuabing the coverage test failed because only 4 items are allowed. can we double this limit?

// +kubebuilder:validation:MaxItems=4
// +kubebuilder:validation:XValidation:rule="self.all(f, has(f.fqdn)) || !self.exists(f, has(f.fqdn))",message="fqdn addresses cannot be mixed with other address types"
Endpoints []BackendEndpoint `json:"endpoints,omitempty"`

I think it's fine to make it 8 cc @guydc

@arkodg
Copy link
Contributor

arkodg commented Nov 30, 2024

@zhaohuabing the coverage test failed because only 4 items are allowed. can we double this limit?

// +kubebuilder:validation:MaxItems=4
// +kubebuilder:validation:XValidation:rule="self.all(f, has(f.fqdn)) || !self.exists(f, has(f.fqdn))",message="fqdn addresses cannot be mixed with other address types"
Endpoints []BackendEndpoint `json:"endpoints,omitempty"`

I think it's fine to make it 8 cc @guydc

64, so it account for cases where users are auto generating Backend resources to route to custom backends ? Can we do that in a separate PR @nothinux ?

@nothinux
Copy link
Contributor Author

@arkodg I've created separate PR to Increase it #4822

Signed-off-by: Taufik Mulyana <17433202+nothinux@users.noreply.github.com>
Copy link

codecov bot commented Dec 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.28%. Comparing base (2385672) to head (6030b6c).
Report is 59 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4803      +/-   ##
==========================================
+ Coverage   66.25%   66.28%   +0.03%     
==========================================
  Files         209      209              
  Lines       31922    31922              
==========================================
+ Hits        21149    21159      +10     
+ Misses       9524     9517       -7     
+ Partials     1249     1246       -3     

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

@zirain
Copy link
Member

zirain commented Dec 4, 2024

can you run make -k gen-check to make CI happy?

@nothinux
Copy link
Contributor Author

nothinux commented Dec 4, 2024

gen-check was fine, not sure why it's failing on lint.shellcheck

@zirain
Copy link
Member

zirain commented Dec 5, 2024

gen-check was fine, not sure why it's failing on lint.shellcheck

I believed crd files should be updated?

@zhaohuabing
Copy link
Member

@nothinux

test/cel-validation/backend_test.go:90: File is not gci-ed with --skip-generated -s standard -s default -s prefix(github.com/envoyproxy/gateway) (gci)

Signed-off-by: Taufik Mulyana <nothinux@gmail.com>
@nothinux
Copy link
Contributor Author

nothinux commented Dec 6, 2024

test/cel-validation/backend_test.go:90: File is not gci-ed with --skip-generated -s standard -s default -s prefix(github.com/envoyproxy/gateway) (gci)

Thanks for pointing that out, updated

Copy link
Member

@zhaohuabing zhaohuabing left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@arkodg arkodg merged commit fcdbafa into envoyproxy:main Jan 6, 2025
24 checks passed
zhaohuabing pushed a commit to zhaohuabing/gateway that referenced this pull request Jan 13, 2025
…oyproxy#4803)

* fix: allow hostname to use subdomain with single label/character

Signed-off-by: Taufik Mulyana <nothinux@gmail.com>
(cherry picked from commit fcdbafa)
Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hostname doesn't support subdomain with single character in Backend Object
4 participants