validator: use ingress validator with dataclient #2757
Conversation
dataclients/kubernetes/ingressv1.go
Outdated
| ingressValidator := &definitions.IngressV1Validator{} | ||
| if err := ingressValidator.Validate(i); err != nil { | ||
| logger.Errorf("Ingress validation failed: %v", err) | ||
| return nil, nil // Drop the resource without failing LoadAll |
There was a problem hiding this comment.
If we returned and error here LoadAll will fail which doesn't make sense, so either move on with this or think of another solution.
There was a problem hiding this comment.
I think we can reduce message to logger.Errorf("Validation failed: %v", err) because this logger has ingress context.
Also I am not sure why we create ingressValidator for each ingressV1Route call, I think we should make it a field of ing *ingress.
MustafaSaber
force-pushed
the
use-ingress-validator-in-skipper
branch
from
9a4f1c4
to
04347f1
Compare
|
We can also start ignoring errors in https://github.com/zalando/skipper/blob/master/dataclients/kubernetes/ingress.go#L226 & https://github.com/zalando/skipper/blob/master/dataclients/kubernetes/ingress.go#L258 and then we can remove loggers |
MustafaSaber
changed the title
dataclients/kubernetes/testdata/ingressV1/ingress-data/ing-with-invalid-routes-annotation.yaml
Outdated
Show resolved
Hide resolved
dataclients/kubernetes/testdata/ingressV1/ingress-data/ing-with-invalid-routes-annotation.yaml
Outdated
Show resolved
Hide resolved
dataclients/kubernetes/testdata/ingressV1/ingress-data/ing-with-invalid-routes-annotation.log
Outdated
Show resolved
Hide resolved
MustafaSaber
added
the
major
|
Please rebase |
MustafaSaber
force-pushed
the
use-ingress-validator-in-skipper
branch
from
9974501
to
e644392
Compare
Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>
MustafaSaber
force-pushed
the
use-ingress-validator-in-skipper
branch
from
e644392
to
7121f32
Compare
|
This is safe to merge according to our reports. |
|
👍 |
1 similar comment
|
👍 |
Relates to #1618
IngressValidator was introduced in #2493 while we still validate inside skipper with some functions that will only drop the annotation but Ingress will still pass and work for example https://github.com/zalando/skipper/blob/master/dataclients/kubernetes/ingress.go#L257C1-L269C2 which will drop the annotation only if parsing fail.
This PR should fix this and at least drop the Ingress resource if validation for skipper annotations fail.