Reduce copying of admission.Request

[willbeason]

What this PR does / why we need it:
The admission.Request type is 400 bytes, making it huge to pass by value
to functions. Each time this is passed to another function, the runtime
fully copies this object, often many times due to the function depth. In
general, we should avoid such unnecessary struct copying.

This PR reduces this copying (for example) by limiting several functions
to only ask for the subfield of admission.Request they need - the 3 bytes
referencing the Raw object byte slice. Other changes in this PR are
analogous.

I've only done the optimization where it added no complexity to the
call sites. For example, it would be valid to pass a pointer to the
admission.Request, but this could potentially have side effects as
the function would be able to mutate the input. In other cases, the
admission.Request would need to be replaced by more than one
argument so I've not done so (I can if desired).

[sozercan]

@willbeason thanks for the PR! Looks like it needs DCO

[willbeason]

There is a corresponding linter that identifies problems like these (hugeParam). The downside is that it's noisy so it isn't always appropriate (for the reasons I mentioned above).

[sozercan]

LGTM

[shomron]

Thanks for the contribution! Is it possible to run before/after benchmarks in pkg/webhook to measure the effect of the change?

[willbeason]

Thanks for the contribution! Is it possible to run before/after benchmarks in pkg/webhook to measure the effect of the change?

That's a good idea - I'll do that now and post back.

[willbeason]

It looks like the difference between before/after is negligible at the scope of individual requests. Copying an admission.Request takes about 15ns:

(after change)

gatekeeper$ go test ./pkg/webhook -bench=BenchmarkValidateResourceName
goos: linux
goarch: amd64
pkg: github.com/open-policy-agent/gatekeeper/pkg/webhook
cpu: Intel(R) Xeon(R) W-2135 CPU @ 3.70GHz
BenchmarkValidateResourceName-12    	 3578606	       328.9 ns/op

(before change)

gatekeeper$ go test ./pkg/webhook -bench=BenchmarkValidateResourceName
goos: linux
goarch: amd64
pkg: github.com/open-policy-agent/gatekeeper/pkg/webhook
cpu: Intel(R) Xeon(R) W-2135 CPU @ 3.70GHz
BenchmarkValidateResourceName-12    	 3515958	       343.7 ns/op

Code I used for benchmarking:

func BenchmarkValidateResourceName(b *testing.B) {
	req := atypes.Request{}
	h := &validationHandler{}
	ctx := context.Background()
	b.ResetTimer()

	for i := 0; i < b.N; i++ {
		h.validateConfigResource(ctx, req)
	}
}

[willbeason]

The benchmarks run in milliseconds, so an 0.0001% change would be very difficult to measure directly:

gatekeeper$ go test ./pkg/webhook -bench=.
goos: linux
goarch: amd64
pkg: github.com/open-policy-agent/gatekeeper/pkg/webhook
cpu: Intel(R) Xeon(R) W-2135 CPU @ 3.70GHz
BenchmarkValidationHandler/psp:_100%_violations-12         	      56	  18723360 ns/op

[codecov-commenter]

Codecov Report

Merging #1291 (2bfd99b) into master (43fd48e) will increase coverage by 0.10%.
The diff coverage is 28.57%.

@@            Coverage Diff             @@
##           master    #1291      +/-   ##
==========================================
+ Coverage   48.56%   48.67%   +0.10%     
==========================================
  Files          68       68              
  Lines        4890     4890              
==========================================
+ Hits         2375     2380       +5     
+ Misses       2164     2160       -4     
+ Partials      351      350       -1     

Flag	Coverage Δ
unittests	48.67% <28.57%> (+0.10%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/webhook/common.go	66.66% <ø> (ø)
pkg/webhook/policy.go	28.62% <0.00%> (ø)
pkg/webhook/mutation.go	17.64% <66.66%> (ø)
...onstrainttemplate/constrainttemplate_controller.go	57.92% <0.00%> (+1.61%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43fd48e...2bfd99b. Read the comment docs.

[maxsmythe]

WDYT about using pointers to the request object?

I'm not against the change as-is, but using a pointer to the entire object might make it easier if we need to accommodate, say, more complex validation of the config resource.

[willbeason]

WDYT about using pointers to the request object?

I'm not against the change as-is, but using a pointer to the entire object might make it easier if we need to accommodate, say, more complex validation of the config resource.

I think that's a pretty good argument to just use pointers. There's the chance of someone mutating the Request, but we shouldn't ever be trying to modify a Request in the first place.

If added to this PR, it would reduce copying further.

[maxsmythe]

LMK when the pointer change has been made!

[maxsmythe]

LGTM