Extension: fix pointer error

[AliceProxy]

previously I was under the false impression that despite the linter warning about an ineffectual assignment that things were working as intended. This was because the test cases directly operated on passed-in pointers instead of ensuring that the returned pointer was being used.

I added a new func copyPtr() func to assist with the copying of xDS resource pointer fields instead of doing the following because of the mutex copy.

*x = *y

I don't like how copyPtr() exists twice in two different files and would like to move it to a helpers/utils file/package. All the existing helpers/utils files seem pretty package specific so I didn't think it would fit in any of them. I might just make a more general/top-level utils package and move it there so let me know if you're a +1/-1 on that plan or if you have a better idea for where this helper func can live.

[arkodg]

can we use https://pkg.go.dev/github.com/golang/protobuf/proto#Clone here instead ?

[AliceProxy]

I used it to get rid of the duplicated function in the testutils functions since those setup new variables, but it won't work for this file since it returns a value, so you end up in the same scenario I described where *x = *y and *x = *proto.Clone(y) cause lint errors because of the mutex inside all the xDS resource messages, and doing x = proto.Clone(y) results in x not really getting updated outside of the function since it's a local variable.

I was looking at proto.Merge as a potential solution, but unfortunately that won't work either since if you don't .Reset() the initial Route/VirtualHost first, then Merge() results in all the fields that are lists (like add request headers) getting duplicate entries. If you do call .Reset() on the Route/VirtualHost first, then proto.Merge() does nothing since it only merges fields that are set on the source and dest.

[Xunzhuo]

why not just ?

*route = *modifiedRoute

And

*vHost = *modifiedVH

[AliceProxy]

This is what I initially wanted to do, but unfortunately it causes a lint error because all the xDS resources have mutexes in the message state fields 😅

copylocks: assignment copies lock value to *route: github.com/envoyproxy/go-control-plane/envoy/config/route/v3.Route contains google.golang.org/protobuf/internal/impl.MessageState contains sync.Mutex

[codecov]

Codecov Report

Merging #1323 (2236152) into main (7a116c9) will decrease coverage by 0.29%.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##             main    #1323      +/-   ##
==========================================
- Coverage   61.94%   61.65%   -0.29%     
==========================================
  Files          85       85              
  Lines       10854    10871      +17     
==========================================
- Hits         6723     6703      -20     
- Misses       3688     3717      +29     
- Partials      443      451       +8     

Impacted Files	Coverage Δ
internal/xds/translator/extension.go	74.04% <40.00%> (-9.29%)	⬇️

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[AliceProxy]

@arkodg I know you approved, but since codecov/patch failed I'm happy to add individual coverage for the func deepCopyPtr() I added by moving it into the utils package at /internal/xds/utils/utils.go (I didn't initially do that since I don't think it's really an "xDS themed" util function, nor do I think it needs to be exported) or by adding tests in translator_test.go (wouldn't really fit the theme with testing translation bits). Lmk if you have any thoughts.

[arkodg]

I'm fine with this going in, I believe there are tests testing this diff