Validate rewrite annotation #2734
Conversation
github-actions
bot
added
documentation
Codecov Report
@@ Coverage Diff @@
## main #2734 +/- ##
=======================================
Coverage 53.54% 53.55%
=======================================
Files 52 52
Lines 14764 14782 +18
=======================================
+ Hits 7905 7916 +11
- Misses 6597 6601 +4
- Partials 262 265 +3
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
haywoodsh
force-pushed
the
feature/validate-rewrites
branch
4 times, most recently
from
71ac774
to
74bce18
Compare
internal/configs/parsing_helpers.go
Outdated
| @@ -333,28 +333,38 @@ func parseRewrites(service string) (serviceName string, rewrite string, err erro | |||
| parts := strings.SplitN(strings.TrimSpace(service), " ", 2) | |||
|
|
|||
| if len(parts) != 2 { | |||
| return "", "", fmt.Errorf("Invalid rewrite format: %s", service) | |||
| return "", "", fmt.Errorf("'%s' is not valid rewrite format, e.g. 'serviceName=tea-svc rewrite=/'", service) | |||
There was a problem hiding this comment.
I would change to wording of this to be 'is not a valid'
| } | ||
|
|
||
| if !VerifyPath(rwPathParts[1]) { | ||
| return "", "", fmt.Errorf("path must start with '/' and must not include any whitespace character, '{', '}' or '$': '%s'", rwPathParts[1]) |
There was a problem hiding this comment.
'P' should be upper case for the word 'path' here
There was a problem hiding this comment.
leaving as it is after discussion
internal/k8s/validation_test.go
Outdated
| @@ -1951,7 +2141,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { | |||
| appProtectDosEnabled: false, | |||
| internalRoutesEnabled: true, | |||
| expectedErrors: []string{ | |||
| `annotations.nginx.org/rewrites: Invalid value: "not_a_rewrite": must be a semicolon-separated list of rewrites`, | |||
| `annotations.nginx.org/rewrites: Invalid value: "not_a_rewrite": 'not_a_rewrite' is not valid rewrite format, e.g. 'serviceName=tea-svc rewrite=/'`, | |||
There was a problem hiding this comment.
You will need to update this expected error message if you make the update from my first comment.
There was a problem hiding this comment.
Good job!
As refactoring touches files that use fmt.Errorf() it may be worth to consider going a step further and implement sentinel errors. This would allow to leverage error handling and wrapping introduced in Go 1.13:
errors.Is(err, errType)
errors.As(err, &errType)
instead of comparing strings in unit tests.
Just food for thought...
haywoodsh
force-pushed
the
feature/validate-rewrites
branch
from
92a5053
to
c7577fe
Compare
Proposed changes
Validate rewrite annotation to ensure (1) "serviceName" and "rewrite" present, (2) serviceName value in "spec", and (3) rewrite paths start with "/" and not include any whitespace character, "{", "}" or "$".
Checklist
Before creating a PR, run through this checklist and mark each as complete.