chore: Validate before adding Xds resource

[Ronnie-personal]

What type of PR is this? Chore: Validate before adding Xds resource

What this PR does / why we need it: This PR calls Validate() before adding the resource to Xds resource table.

Which issue(s) this PR fixes: #1647

Fixes #

[codecov]

Codecov Report

Merging #1662 (d886fe3) into main (ea18f8f) will increase coverage by 0.08%.
Report is 6 commits behind head on main.
The diff coverage is 67.91%.

@@            Coverage Diff             @@
##             main    #1662      +/-   ##
==========================================
+ Coverage   64.77%   64.86%   +0.08%     
==========================================
  Files          83       84       +1     
  Lines       11925    12081     +156     
==========================================
+ Hits         7725     7836     +111     
- Misses       3715     3744      +29     
- Partials      485      501      +16     

Files Changed	Coverage Δ
internal/xds/translator/jsonpatch.go	30.10% <0.00%> (+0.10%)	⬆️
internal/xds/translator/authentication.go	65.10% <25.00%> (-0.78%)	⬇️
internal/xds/translator/extension.go	72.38% <25.00%> (-1.66%)	⬇️
internal/xds/translator/ratelimit.go	87.94% <25.00%> (-0.91%)	⬇️
internal/xds/translator/translator.go	72.58% <51.02%> (-5.87%)	⬇️
internal/xds/types/resourceversiontable.go	84.31% <89.09%> (+2.83%)	⬆️
internal/xds/translator/accesslog.go	98.20% <100.00%> (+0.02%)	⬆️
internal/xds/translator/tracing.go	95.09% <100.00%> (+0.09%)	⬆️

... and 7 files with indirect coverage changes

[arkodg]

thanks for raising this PR @Ronnie-personal .
Imo attaching a ValidateAll() right before inserting the resource into tCtx.AddXdsResource is something the developer might forget to do, instead should we add ValidateAll() inside the tCtx.AddXdsResource and change the signature of it to return a error code instead ?

[Ronnie-personal]

thanks for raising this PR @Ronnie-personal . Imo attaching a ValidateAll() right before inserting the resource into tCtx.AddXdsResource is something the developer might forget to do, instead should we add ValidateAll() inside the tCtx.AddXdsResource and change the signature of it to return a error code instead ?

How about this code change?

`func (t *ResourceVersionTable) AddXdsResource(rType resourcev3.Type, xdsResource types.Resource) error {

// Perform type switch to handle different types of xdsResource
switch r := xdsResource.(type) {

case *listenerv3.Listener:
	// Handle Type specific operations
	if resourceOfType, ok := xdsResource.(*listenerv3.Listener); ok {
		if err := resourceOfType.ValidateAll(); err != nil {
			return fmt.Errorf("validation failed for xds resource %+v, err:%v", r, err)
		}
	} else {
		return fmt.Errorf("failed to cast xds resource %+v to Listener type", r)
	}

case *routev3.RouteConfiguration:
	// Handle Type specific operations
	if resourceOfType, ok := xdsResource.(*routev3.RouteConfiguration); ok {
		if err := resourceOfType.ValidateAll(); err != nil {
			return fmt.Errorf("validation failed for xds resource %+v, err:%v", r, err)
		}
	} else {
		return fmt.Errorf("failed to cast xds resource %+v to RouteConfiguration type", r)
	}

case *Type3:
	// Handle Type3 specific operations

// Add more cases for other types as needed
default:
	// Handle the case when the type is not recognized or supported
	return fmt.Errorf("Unsupported resource type: %v", rType)
}

if t.XdsResources == nil {
	t.XdsResources = make(XdsResources)
}
if t.XdsResources[rType] == nil {
	t.XdsResources[rType] = make([]types.Resource, 0, 1)
}

t.XdsResources[rType] = append(t.XdsResources[rType], xdsResource)
return nil

}`

[arkodg]

yeah @Ronnie-personal something similar to above logic is needed, you can probably use rType resourcev3.Type as in input to switch and then typecast from resource to the specific type before running ValidateAll

[Ronnie-personal]

@arkodg
The e2e ratelimit test failed. Please find detailed error at https://github.com/envoyproxy/gateway/actions/runs/5633849037/job/15263275737?pr=1662#step:6:1913.

This PR does not change e2e test code. Do you think the error is caused by any code change in this pull request?

[arkodg]

thanks for catching this, lets raise a issue to track this

[Ronnie-personal]

Sure, I have opened #1742. Here is the PR to fix the issue #1758

[arkodg]

LGTM @Ronnie-personal, this PR looks good, just added some minor comments, thanks again !

[Ronnie-personal]

@arkodg
Sorry, I pushed a minor change to fix the typo.
Here are the changes Ronnie-personal@d886fe3