-
Notifications
You must be signed in to change notification settings - Fork 657
v0.7.0 API Review #1923
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
v0.7.0 API Review #1923
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -112,6 +112,8 @@ type TLSRouteRule struct { | |
| // | ||
| // Support: Core for Kubernetes Service | ||
| // | ||
| // Support: Extended for Kubernetes ServiceImport | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: While we all understand what ServiceImport is, a new reader may not. So better to call out that these are objects. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment applies in a lot of places.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To clarify, are you saying that "Extended for Kubernetes ServiceImport" should be replaced with "Extended for Kubernetes ServiceImport Objects", would the same be true for "Core for Kubernetes Service Objects"? I'm not that familiar with the proper terminology here, but would "Resource(s)" work as well? |
||
| // | ||
| // Support: Implementation-specific for any other resource | ||
| // | ||
| // Support for weight: Extended | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,7 +17,9 @@ limitations under the License. | |
| package validation | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "net/http" | ||
| "regexp" | ||
| "strings" | ||
|
|
||
| "k8s.io/apimachinery/pkg/util/validation/field" | ||
|
|
@@ -31,6 +33,10 @@ var ( | |
| repeatableGRPCRouteFilters = []gatewayv1a2.GRPCRouteFilterType{ | ||
| gatewayv1a2.GRPCRouteFilterExtensionRef, | ||
| } | ||
| validServiceName = `^(?i)\.?[a-z_][a-z_0-9]*(\.[a-z_][a-z_0-9]*)*$` | ||
| validServiceNameRegex = regexp.MustCompile(validServiceName) | ||
| validMethodName = `^[A-Za-z_][A-Za-z_0-9]*$` | ||
| validMethodNameRegex = regexp.MustCompile(validMethodName) | ||
| ) | ||
|
|
||
| // ValidateGRPCRoute validates GRPCRoute according to the Gateway API specification. | ||
|
|
@@ -63,13 +69,26 @@ func validateGRPCRouteRules(rules []gatewayv1a2.GRPCRouteRule, path *field.Path) | |
| return errs | ||
| } | ||
|
|
||
| // validateRuleMatches validates that at least one of the fields Service or Method of | ||
| // GRPCMethodMatch to be specified | ||
| // validateRuleMatches validates GRPCMethodMatch | ||
| func validateRuleMatches(matches []gatewayv1a2.GRPCRouteMatch, path *field.Path) field.ErrorList { | ||
| var errs field.ErrorList | ||
| for i, m := range matches { | ||
| if m.Method != nil && m.Method.Service == nil && m.Method.Method == nil { | ||
| errs = append(errs, field.Required(path.Index(i).Child("method"), "one or both of `service` or `method` must be specified")) | ||
| if m.Method != nil { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, it's optional, but there is defaulting involved so it seems unlikely/impossible that this would ever be empty. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We often add the |
||
| if m.Method.Service == nil && m.Method.Method == nil { | ||
| errs = append(errs, field.Required(path.Index(i).Child("method"), "one or both of `service` or `method` must be specified")) | ||
| } | ||
| // GRPCRoute method matcher admits two types: Exact and RegularExpression. | ||
| // If not specified, the match will be treated as type Exact (also the default value for this field). | ||
| if m.Method.Type == nil || *m.Method.Type == gatewayv1a2.GRPCMethodMatchExact { | ||
| if m.Method.Service != nil && !validServiceNameRegex.MatchString(*m.Method.Service) { | ||
| errs = append(errs, field.Invalid(path.Index(i).Child("method"), *m.Method.Service, | ||
| fmt.Sprintf("must only contain valid characters (matching %s)", validServiceName))) | ||
| } | ||
| if m.Method.Method != nil && !validMethodNameRegex.MatchString(*m.Method.Method) { | ||
| errs = append(errs, field.Invalid(path.Index(i).Child("method"), *m.Method.Method, | ||
| fmt.Sprintf("must only contain valid characters (matching %s)", validMethodName))) | ||
| } | ||
| } | ||
| } | ||
| if m.Headers != nil { | ||
| errs = append(errs, validateGRPCHeaderMatches(m.Headers, path.Index(i).Child("headers"))...) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we drop HTTP from this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like because of the above change to make the GRPC variant
type GRPCHeaderName v1beta1.HeaderNameinstead of its ownstringalias?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, this was trying to ensure that our types and validation were consistent since by definition a GRPC Header name is going to have the same validation and constraints as HTTP.