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
internal: Remove max_program_size from regex #3093
Conversation
GoogleRe2: &matcher.RegexMatcher_GoogleRE2{ | ||
MaxProgramSize: protobuf.UInt32(maxRegexProgramSize), | ||
}, | ||
GoogleRe2: &matcher.RegexMatcher_GoogleRE2{}, |
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.
This whole file could be deleted probably, these tests don't really test anything.
Codecov Report
@@ Coverage Diff @@
## main #3093 +/- ##
==========================================
+ Coverage 73.15% 73.19% +0.03%
==========================================
Files 103 103
Lines 6412 6421 +9
==========================================
+ Hits 4691 4700 +9
Misses 1618 1618
Partials 103 103
|
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 good, with jus a few nits.
|
||
// ValidateRegex returns an error if the supplied | ||
// regex is invalid. | ||
func ValidateRegex(regex string) error { |
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.
Nit: Best to add a comment that this is specifically validating RE2 regex syntax because that I what we are programming into Envoy (though Go isn't identicall apparently?).
internal/dag/ingress_processor.go
Outdated
p.WithError(err). | ||
WithField("name", ing.GetName()). | ||
WithField("namespace", ing.GetNamespace()). | ||
WithField("regex", path). | ||
Errorf("path regex is not valid: %s", err) |
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.
p.WithError(err). | |
WithField("name", ing.GetName()). | |
WithField("namespace", ing.GetNamespace()). | |
WithField("regex", path). | |
Errorf("path regex is not valid: %s", err) | |
p.WithError(err). | |
WithField("name", ing.GetName()). | |
WithField("namespace", ing.GetNamespace()). | |
WithField("regex", path). | |
Error("invalid path regular expression") |
since the error is already logged by WithError()
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.
👍
internal/dag/ingress_processor.go
Outdated
err := ValidateRegex(path) | ||
if err != nil { | ||
return nil, err | ||
} |
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.
err := ValidateRegex(path) | |
if err != nil { | |
return nil, err | |
} | |
if err := ValidateRegex(path); err != nil { | |
return nil, err | |
} |
nit :)
@@ -1509,6 +1509,48 @@ func TestRoutePrefixRouteRegex(t *testing.T) { | |||
TypeUrl: routeType, | |||
Nonce: "1", | |||
}) | |||
|
|||
// add default/kuard to translator. |
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.
Stale comment?
3b53497
to
fbd27f0
Compare
The Regex `max_program_size` is deprecated from Envoy and the control plane should do this validation now. Removes the max_program_size field and adds a regex compile check to the Ingress resource builder. Fixes projectcontour#2933 Signed-off-by: Steve Sloka <slokas@vmware.com>
fbd27f0
to
5776b2b
Compare
The Regex
max_program_size
is deprecated from Envoy and the control plane shoulddo this validation now. Removes the max_program_size field and adds a regex compile
check to the Ingress resource builder.
Fixes #2933
Signed-off-by: Steve Sloka slokas@vmware.com