BUGFIX: accidentally altering 'decorators' slice

[inteon]

I spent my afternoon trying to find the root cause for kubernetes/kubernetes#114661. And this led me to this bug.

The factory function incorrectly calls append(decorators, ...) which alters the original decorators slice.
This can result in the costTracker being shared across instances in a concurrent scenario, further causing bugs.

The bug (kubernetes/kubernetes#114661) originated from calling the tracker.stack.dropArgs(...) function from multiple goroutines at the same time (which shouldn't be possible normally), breaking multithreading safety.

[liggitt]

cc @jpbetz @TristonianJones

[liggitt]

@jpbetz @cici37 which versions of cel-go will you need this ported to?

Looks like:

• v0.10.x (for Kubernetes 1.24)
• v0.12.x (for Kubernetes 1.25 and 1.26)

it looks like there aren't release-0.x branches for backports... just to experiment, in my fork, I created branches off the latest tags:

• https://github.com/liggitt/cel-go/tree/release-0.10
• https://github.com/liggitt/cel-go/tree/release-0.11
• https://github.com/liggitt/cel-go/tree/release-0.12

and opened PRs with this cherry-picked against those branches at https://github.com/liggitt/cel-go/pulls

[inteon]

I just found out that go test -race would have directly shown me what the cause of this bug is:

WARNING: DATA RACE
Write at 0x00c0005f86f8 by goroutine 25:
  github.com/google/cel-go/cel.newProgram.func1()
      /home/username/go/pkg/mod/github.com/google/cel-go@v0.12.5/cel/program.go:231 +0x4d3
  github.com/google/cel-go/cel.(*progGen).ContextEval()
      /home/username/go/pkg/mod/github.com/google/cel-go@v0.12.5/cel/program.go:393 +0x1b0
  k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel.(*Validator).validateExpressions()
      /home/username/go/pkg/mod/k8s.io/apiextensions-apiserver@v0.25.4/pkg/apiserver/schema/cel/validation.go:216 +0xc31
  k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel.(*Validator).Validate()
      /home/username/go/pkg/mod/k8s.io/apiextensions-apiserver@v0.25.4/pkg/apiserver/schema/cel/validation.go:147 +0x1dc
  k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel.(*Validator).validateMap()
      /home/username/go/pkg/mod/k8s.io/apiextensions-apiserver@v0.25.4/pkg/apiserver/schema/cel/validation.go:342 +0x10bc
  k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel.(*Validator).Validate()

[liggitt]

I just found out that go test -race would have directly shown me what the cause of this bug is:

@TristonianJones is parallel evaluation tested in this repo? if so, are there CI tests that run those in race mode?

[TristonianJones]

@liggitt There are parallel evaluation tests for the parser, checker, and interpreter, but the top-level cel package is missing them. I'll add some. I've also been meaning to move away from bazel tests in our CI setup since they continue to time out when trying to resolve the go-genproto library dependency. I'll shift over to go test and add race checks.