Remove PodOwnership option #1585

willbeason · 2021-09-28T17:43:51Z

This was introduced for tests, but is unnecessary. All that is needed
for setting ownerreferences to work properly is to set the UID of fake
Pods used in testing.

This is because ownerreference validation requires that UID be set on
the reference (to deduplicate objects with identical names/namespaces
which existed at different points in time).

This raises the point that we should probably have a general-purpose
method for instantiating a fake Pod for use in testing and debugging. It
isn't obvious to me where that is. Feel free to suggest a location. Do
note that it would be in production code since the debug setting is
compiled in non-test files.

Note: the fake Pod UID I've added for tests is arbitrary - it just has to be
something, but the specific value is meaningless.

This PR is a follow-up based on suggestions ritazh and julianKatz made on #1569

Signed-off-by: Will Beason willbeason@google.com

codecov-commenter · 2021-09-28T19:11:19Z

Codecov Report

Merging #1585 (f248000) into master (86da9a8) will decrease coverage by 0.32%.
The diff coverage is 78.37%.

@@            Coverage Diff             @@
##           master    #1585      +/-   ##
==========================================
- Coverage   54.25%   53.92%   -0.33%     
==========================================
  Files          93       93              
  Lines        8082     8070      -12     
==========================================
- Hits         4385     4352      -33     
- Misses       3356     3374      +18     
- Partials      341      344       +3

Flag	Coverage Δ
unittests	`53.92% <78.37%> (-0.33%)`	⬇️

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

Impacted Files	Coverage Δ
apis/status/v1beta1/util.go	`76.31% <0.00%> (-4.12%)`	⬇️
pkg/controller/constraint/constraint_controller.go	`5.72% <0.00%> (+0.03%)`	⬆️
apis/status/v1beta1/constraintpodstatus_types.go	`80.64% <50.00%> (ø)`
...tatus/v1beta1/constrainttemplatepodstatus_types.go	`73.91% <50.00%> (ø)`
apis/status/v1beta1/mutatorpodstatus_types.go	`80.00% <50.00%> (ø)`
...onstrainttemplate/constrainttemplate_controller.go	`54.54% <100.00%> (-4.65%)`	⬇️
pkg/controller/mutators/core/reconciler.go	`85.64% <100.00%> (-0.07%)`	⬇️
pkg/readiness/list.go	`79.41% <0.00%> (-11.77%)`	⬇️
... and 2 more

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 86da9a8...f248000. Read the comment docs.

pkg/controller/config/config_controller_test.go

julianKatz · 2021-09-28T19:53:36Z

pkg/controller/config/config_controller_test.go

-	g.Expect(err).NotTo(gomega.HaveOccurred())
+
+	createInstance := func() client.Object {
+		return configFor([]schema.GroupVersionKind{nsGVK, configMapGVK})


Isn't this redefining instance? How do these declarations compare?

We need a fresh copy of Instance each time we send one to a call to client.Create/client.Delete as they modify instance.

Perhaps use instance.DeepCopy() then? So we know they are the same?

Ok. I don't see a distinction in the approaches. All that matters is that for each call to a client.Client method, we have a new copy.

To Max's suggestion - no, we can't just ensureCreated(instance.DeepCopy()) as this would use the same object for multiple client.Client calls. Each time we want to make a new call to any client.Client method, we must be using a completely fresh copy of instance. Thus, we must instance.DeepCopy every single time we are about to run a client.Client method.

ensureCreated(instance.DeepCopy()) only copies instance once. Instead, we must pass a reference of instance to ensureCreated and then copy instance within ensureCreated so the method can guarantee that a fresh copy is supplied each time Eventually invokes the functor ensureCreated returns.

Thanks for hunting that down.

I'm mostly just concerned about comprehensibility in the tests because the setup ends up being a big part of explaining what's actually being tested. For example, the re-use of the API server across tests isn't obvious until you've experienced the pain it causes.

Recording the lesson you've learned in the code is (IMO) very valuable, as it saves a future engineer from walking the same (I'm sure time consuming) path. If you see a good way to do that, that future engineer would appreciate it!

julianKatz · 2021-09-28T20:01:03Z

Perhaps you'd rather change this in a subsequent PR, but this feels to me like we're sprinkling some complexity throughout the codebase. If we were to move the pod generation into one package for generating test fakes, that single place could hold this useful lesson you derived (that a fake UID will make ownerrefs work, preventing you from having to disable them during testing).

maxsmythe

LGTM, with some nits on ensureCreated

pkg/controller/config/config_controller_test.go

willbeason · 2021-09-29T15:43:52Z

FYI added pkg/fakes package per Julian's suggestion.

pkg/fakes/opts.go

shomron

minor non-blocking comments in test code, lgtm 👍🏻

pkg/controller/config/config_controller_test.go

shomron · 2021-10-03T15:23:46Z

apis/status/v1beta1/constraintpodstatus_types.go

-			return nil, err
-		}
+
+	if err = controllerutil.SetOwnerReference(pod, obj, scheme); err != nil {


I missed the conversation that motivated this PR so don't have full context.

When working on GK, I often run the code locally on my machine pointed at a remote cluster. If I read correctly, this will only be possible by setting POD_NAME and POD_NAMESPACE to point to a valid pod on the cluster?

No. If you are running the code locally (i.e. there is no real "Pod" on the cluster) then you do not need to set these environment variables.

The original code sort of obfuscated how this works.

To function properly, SetOwnerReference requires its first argument (pod in this case) to have metadata.name and metadata.uid set. Otherwise future API Server calls with obj fail with confusing errors.

If --debug-use-fake-pod is enabled, then we unconditionally set the POD_NAME env variable to a valid value before instantiating the fake Pod. So Pod is guaranteed to be a valid object.

util.GetPodNamespace falls back to gatekeeper-system if the env variable is not set. This prevents various crashes which can happen if the controller's Pod does not have metadata.namespace set.

Finally, the new fakes.Pod method sets a dummy metadata.uid which satisfies the fake API Server used in testing. The fake API Server validates that requests have metadata.uid, but does not ensure the UID is meaningful or valid.

Thus, all information required for the Pod to be used in debug mode is set.

This was introduced for tests, but is unnecessary. All that is needed for setting ownerreferences to work properly is to set the UID of fake Pods used in testing. This is because ownerreference validation requires that UID be set on the reference (to deduplicate objects with identical names/namespaces which existed at different points in time). This raises the point that we should probably have a general-purpose method for instantiating a fake Pod for use in testing and debugging. It isn't obvious to me where that is. Feel free to suggest a location. Do note that it would be in production code since the debug setting is compiled in non-test files. Signed-off-by: Will Beason <willbeason@google.com>

Signed-off-by: Will Beason <willbeason@google.com>

Also split ensureDeleted out of ensureCreated. Signed-off-by: Will Beason <willbeason@google.com>

Signed-off-by: Will Beason <willbeason@google.com>

willbeason requested review from ritazh, julianKatz and maxsmythe September 28, 2021 17:43

willbeason self-assigned this Sep 28, 2021

julianKatz reviewed Sep 28, 2021

View reviewed changes

pkg/controller/config/config_controller_test.go Show resolved Hide resolved

julianKatz reviewed Sep 28, 2021

View reviewed changes

maxsmythe approved these changes Sep 28, 2021

View reviewed changes

pkg/controller/config/config_controller_test.go Show resolved Hide resolved

pkg/controller/config/config_controller_test.go Outdated Show resolved Hide resolved

willbeason requested a review from julianKatz September 29, 2021 15:42

willbeason force-pushed the no-pod-ownership-mode branch 2 times, most recently from 96ddaae to 0a376cc Compare September 29, 2021 16:35

willbeason requested a review from sozercan September 29, 2021 17:48

julianKatz reviewed Sep 29, 2021

View reviewed changes

pkg/fakes/opts.go Show resolved Hide resolved

willbeason force-pushed the no-pod-ownership-mode branch 2 times, most recently from 5e910a4 to 32b75fb Compare September 29, 2021 20:03

willbeason requested a review from julianKatz September 30, 2021 14:48

willbeason force-pushed the no-pod-ownership-mode branch from 32b75fb to f248000 Compare September 30, 2021 14:50

julianKatz approved these changes Sep 30, 2021

View reviewed changes

shomron approved these changes Oct 3, 2021

View reviewed changes

Will Beason added 6 commits October 4, 2021 08:44

Fix Cache test

cce4d85

Signed-off-by: Will Beason <willbeason@google.com>

Make Cache test less flaky

0fe1a58

Signed-off-by: Will Beason <willbeason@google.com>

Use fakes for default-instantiating objects

4d9a928

Signed-off-by: Will Beason <willbeason@google.com>

Deepcopy instance inside of ensureX commands

28503c8

Also split ensureDeleted out of ensureCreated. Signed-off-by: Will Beason <willbeason@google.com>

Resolve reviewer nits

25f49c3

Signed-off-by: Will Beason <willbeason@google.com>

willbeason force-pushed the no-pod-ownership-mode branch from f248000 to 25f49c3 Compare October 4, 2021 15:44

willbeason merged commit 655278a into open-policy-agent:master Oct 4, 2021

willbeason deleted the no-pod-ownership-mode branch October 4, 2021 15:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove PodOwnership option #1585

Remove PodOwnership option #1585

willbeason commented Sep 28, 2021 •

edited

Loading

codecov-commenter commented Sep 28, 2021 •

edited

Loading

julianKatz Sep 28, 2021

willbeason Sep 28, 2021

julianKatz Sep 28, 2021

willbeason Sep 29, 2021

willbeason Sep 29, 2021 •

edited

Loading

julianKatz Sep 30, 2021

julianKatz commented Sep 28, 2021

maxsmythe left a comment

willbeason commented Sep 29, 2021

shomron left a comment

shomron Oct 3, 2021

willbeason Oct 4, 2021 •

edited

Loading

Remove PodOwnership option #1585

Remove PodOwnership option #1585

Conversation

willbeason commented Sep 28, 2021 • edited Loading

codecov-commenter commented Sep 28, 2021 • edited Loading

Codecov Report

julianKatz Sep 28, 2021

Choose a reason for hiding this comment

willbeason Sep 28, 2021

Choose a reason for hiding this comment

julianKatz Sep 28, 2021

Choose a reason for hiding this comment

willbeason Sep 29, 2021

Choose a reason for hiding this comment

willbeason Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

julianKatz Sep 30, 2021

Choose a reason for hiding this comment

julianKatz commented Sep 28, 2021

maxsmythe left a comment

Choose a reason for hiding this comment

willbeason commented Sep 29, 2021

shomron left a comment

Choose a reason for hiding this comment

shomron Oct 3, 2021

Choose a reason for hiding this comment

willbeason Oct 4, 2021 • edited Loading

Choose a reason for hiding this comment

willbeason commented Sep 28, 2021 •

edited

Loading

codecov-commenter commented Sep 28, 2021 •

edited

Loading

willbeason Sep 29, 2021 •

edited

Loading

willbeason Oct 4, 2021 •

edited

Loading