fix: Only process owner references for known kinds of owners. #245
Conversation
| if err != nil { | ||
| l.Info("/mutate-pod request can't be processed", | ||
| "kind", req.Kind.Kind, "ns", req.Namespace, "name", req.Name) | ||
| return admission.Errored(http.StatusInternalServerError, err) | ||
| } | ||
|
|
||
| updatedPod, err := a.handleCreatePodRequest(ctx, p) |
There was a problem hiding this comment.
I split the Handle function into 2 separate responsibilities:
func Handle(ctx, req)does all the transformation betweenadmission.Requestandadmission.Responseinto useful domain objects.func handleCreatePodRequest(ctx, pod)has the business logic to decide when and how to add Proxy containers to a pod as it is being created.
This made it a lot easier to write unit tests for the business logic without having to implement a lot of extraneous json handling code.
| "sigs.k8s.io/controller-runtime/pkg/webhook/admission" | ||
| ) | ||
|
|
||
| func TestPodWebhookWithDeploymentOwners(t *testing.T) { |
There was a problem hiding this comment.
This is the new unit test for the pod create webhook business logic.
| wantUpdate: false, | ||
| }, | ||
| { | ||
| name: "Deployment Pod with unknown owner", |
There was a problem hiding this comment.
This is the test case for the bug fix.
| }, "webapp") | ||
| dWithOwner.ObjectMeta.Labels = map[string]string{"app": "webapp"} | ||
| deploymentOwner := &v1.PartialObjectMetadata{ | ||
| TypeMeta: v1.TypeMeta{Kind: "BuildToolOperatror", APIVersion: "v1"}, |
There was a problem hiding this comment.
sp: BuildToolOperatror
nit: Maybe just "DontTouchThisThing" or something obviously not of a known kind. BuildToolOperator looks legit.
There was a problem hiding this comment.
Fixed. I added more comments to make this easier to understand.
| name: "Deployment Pod with unknown owner", | ||
| p: p, | ||
| d: dWithOwner, | ||
| wantUpdate: true, |
There was a problem hiding this comment.
No, this should be true. The AuthProxyWorkload should act on the Deployment's pods, even if the deployment is owned by a different operator. We have a user who wants the Cloud SQL Proxy Operator to add proxy containers to pods that are related to a deployment managed by another operator.
There was a problem hiding this comment.
Is there a test covering the change you made to ignoring owners of an unknown kind in that case?
There was a problem hiding this comment.
Yes, this is that test.
hessjcg
force-pushed
the
gh-244-unknown-owner-ref
branch
from
d7f9503 to
5764fbc
Compare
hessjcg
force-pushed
the
gh-244-unknown-owner-ref
branch
from
05de7a8 to
b6bad6d
Compare
|
/gcbrun |
|
/gcbrun |
Pods can be owned by multiple owners. We only want the operator to traverse the owners where it knows
the kind of workload: ReplicaSet, Deployment, etc. We don't want the operator to try to travers other
kinds of owner resources that it does not understand, because the operator was not granted privileges
to access those resources.
Fixes #244