validate all xds resources before returning the translation result #5148
Conversation
zhaohuabing
force-pushed
the
fix-5147
branch
2 times, most recently
from
befa1a4 to
4d53353
Compare
Codecov ReportAttention: Patch coverage is
❌ Your patch check has failed because the patch coverage (46.36%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #5148 +/- ##
==========================================
- Coverage 64.97% 64.97% -0.01%
==========================================
Files 214 213 -1
Lines 33532 33475 -57
==========================================
- Hits 21789 21751 -38
+ Misses 10400 10397 -3
+ Partials 1343 1327 -16 ☔ View full report in Codecov by Sentry. |
zhaohuabing
force-pushed
the
fix-5147
branch
3 times, most recently
from
6eb31de to
a03807e
Compare
| @@ -131,6 +131,12 @@ func (t *Translator) Translate(xdsIR *ir.Xds) (*types.ResourceVersionTable, erro | |||
| } | |||
| } | |||
|
|
|||
| // Validate all the xds resources in the table before returning | |||
| // This is necessary to catch any misconfigurations that might have been missed during translation | |||
| if err := tCtx.ValidateAll(); err != nil { | |||
There was a problem hiding this comment.
This is the key change, the rest are minor refactors around the proto validation.
There was a problem hiding this comment.
so are we validating twice ?
There was a problem hiding this comment.
because there's already a proto.Validate in resourceversiontable.go
There was a problem hiding this comment.
Yes, this is the final check to ensure no validations are missed, it serves as a safe guard to avoid invalid xDS being pushed to the Envoy fleet.
We still want to retain the original validation to catch up the invalid xDS earlier for proper error handling.
There was a problem hiding this comment.
It's common for an xds resource to be modified after being added to ResourceVersionTable, and we don't have a good way to prevent this from happening again in future code changes, so I prefer to add a final check in the translator.
There was a problem hiding this comment.
can we then keep this one and rm the one in AddXdsResource ?
There was a problem hiding this comment.
can we then keep this one and rm the one in AddXdsResource ?
I think it's reasonable to move the validation out of the AddXdsResource and into a dedicate method, as the current validation in AddXdsResource is implicit and could be easily ignored.
Would you mind if I address this in a follow-up PR? This is not a blocking issue for this PR. AddXdsResource method has been used everywhere and all the thrown validation errors need to be properly handled, addressing it here would make the scope of this PR too large.
Tracked in this issue: #5311
Since @arkodg mentioned that double validation could be a performance issue, I removed the cherrypick/release-v1.3.1 label, we can decide if we want to cherry-pick them to v1.3 after both this and #5311 are properly handled. cc @guydc
| @@ -82,10 +82,7 @@ func buildHCMBasicAuthFilter(basicAuth *ir.BasicAuth) (*hcmv3.HttpFilter, error) | |||
| }, | |||
| }, | |||
| } | |||
| if err = basicAuthProto.ValidateAll(); err != nil { | |||
There was a problem hiding this comment.
Validation in individual filer translators are not required anymore, as they'll be validated inside the proto.ToAnyWithValidation function before converting to any.
zhaohuabing
force-pushed
the
fix-5147
branch
3 times, most recently
from
abd87f7 to
5faed17
Compare
Signed-off-by: Huabing (Robin) Zhao <[email protected]>
|
should we CP to 1.3.0 ? |
Signed-off-by: Huabing (Robin) Zhao <[email protected]>
Signed-off-by: Huabing (Robin) Zhao <[email protected]>
This PR enforces validation for all generated xDS resources before sending them to the Envoy fleet.
Related to: #5147
Release Notes: No