Extension: fix pointer error #1323
Conversation
internal/xds/translator/extension.go
Outdated
| // Overwrite the pointer for the original virtual host. | ||
| // Uses nolint because of the ineffectual assignment check | ||
| vHost = modifiedVH //nolint | ||
| copyPtr(modifiedVH, vHost) |
There was a problem hiding this comment.
can we use https://pkg.go.dev/github.com/golang/protobuf/proto#Clone here instead ?
There was a problem hiding this comment.
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.
| @@ -53,9 +55,7 @@ func processExtensionPostRouteHook(route *routev3.Route, vHost *routev3.VirtualH | |||
|
|
|||
| // If the extension returned a modified Route, then copy its to the one that was passed in as a reference | |||
| if modifiedRoute != nil { | |||
| // Overwrite the pointer for the original route. | |||
| // Uses nolint because of the ineffectual assignment check | |||
| route = modifiedRoute //nolint | |||
There was a problem hiding this comment.
why not just ?
*route = *modifiedRoute
And
*vHost = *modifiedVH
There was a problem hiding this comment.
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
previously I was under the false impression that despite the linter warning about an innefectual 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. Signed-off-by: AliceProxy <alicewasko@datawire.io>
Alice-Lilith
force-pushed
the
alicewasko/extension-logic-error
branch
from
69ddd23
to
2236152
Compare
Codecov Report
@@ 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
... 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. |
|
@arkodg I know you approved, but since codecov/patch failed I'm happy to add individual coverage for the |
|
I'm fine with this going in, I believe there are tests testing this diff |
(cherry picked from commit 2bf9607) Signed-off-by: AliceProxy <alicewasko@datawire.io>
* Extension: fix pointer error (#1323) (cherry picked from commit 2bf9607) Signed-off-by: AliceProxy <alicewasko@datawire.io> * fix: add the namespace resource within helm templates (#1332) Add the namespace resource within helm templates This is unfortunate workaround due the difference in UX between `helm template` and `helm install` The project recommends `helm install` as a way to install EG which supports a `--create-namespace` flag to create a namespace However we also generate a static YAML using `helm template` as part of the release artficat so a user can install the YAML directly using `kubectl` instead of `helm` . The issue here is `helm template` does not support `--create-namespace`, so instead this commit adds a knob called `createNamespace` to the Helm chart which is `false` by default, but turned on during `make generate-manifests` Fixes: #1307 Signed-off-by: Arko Dasgupta <arko@tetrate.io> (cherry picked from commit 9d6d699) Signed-off-by: AliceProxy <alicewasko@datawire.io> --------- Signed-off-by: Arko Dasgupta <arko@tetrate.io> Signed-off-by: AliceProxy <alicewasko@datawire.io> Co-authored-by: Arko Dasgupta <arkodg@users.noreply.github.com>
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()functo assist with the copying of xDS resource pointer fields instead of doing the following because of the mutex copy.I don't like how
copyPtr()exists twice in two different files and would like to move it to ahelpers/utilsfile/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 helperfunccan live.