Skip to content

Commit

Permalink
fix: Only process owner references for known kinds of owners. (#245)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
hessjcg committed Mar 8, 2023
1 parent f2ba903 commit 12be1dc
Show file tree
Hide file tree
Showing 4 changed files with 246 additions and 63 deletions.
17 changes: 9 additions & 8 deletions internal/controller/authproxyworkload_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand Down Expand Up @@ -73,7 +74,7 @@ func TestReconcileDeleted(t *testing.T) {
addFinalizers(p)
addPodWorkload(p)

cb, err := clientBuilder()
cb, _, err := clientBuilder()
if err != nil {
t.Error(err) // shouldn't ever happen
}
Expand Down Expand Up @@ -308,7 +309,7 @@ func TestReconcileDeleteUpdatesWorkload(t *testing.T) {
}

// Build a client with the resource and deployment
cb, err := clientBuilder()
cb, _, err := clientBuilder()
if err != nil {
t.Error(err) // shouldn't ever happen
}
Expand Down Expand Up @@ -361,7 +362,7 @@ func TestReconcileDeleteUpdatesWorkload(t *testing.T) {
}

func runReconcileTestcase(p *v1alpha1.AuthProxyWorkload, clientObjects []client.Object, wantRequeue bool, wantStatus metav1.ConditionStatus, wantReason string) (client.WithWatch, context.Context, error) {
cb, err := clientBuilder()
cb, _, err := clientBuilder()
if err != nil {
return nil, nil, err // shouldn't ever happen
}
Expand Down Expand Up @@ -400,20 +401,20 @@ func runReconcileTestcase(p *v1alpha1.AuthProxyWorkload, clientObjects []client.
return c, ctx, nil
}

func clientBuilder() (*fake.ClientBuilder, error) {
func clientBuilder() (*fake.ClientBuilder, *runtime.Scheme, error) {
scheme, err := v1alpha1.SchemeBuilder.Build()
if err != nil {
return nil, err
return nil, nil, err
}
err = corev1.AddToScheme(scheme)
if err != nil {
return nil, err
return nil, nil, err
}
err = appsv1.AddToScheme(scheme)
if err != nil {
return nil, err
return nil, nil, err
}
return fake.NewClientBuilder().WithScheme(scheme), nil
return fake.NewClientBuilder().WithScheme(scheme), scheme, nil

}

Expand Down
83 changes: 42 additions & 41 deletions internal/controller/pod_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,77 +49,78 @@ func (a *PodAdmissionWebhook) InjectDecoder(d *admission.Decoder) error {
// the proxy sidecars on all workloads to match the AuthProxyWorkload config.
func (a *PodAdmissionWebhook) Handle(ctx context.Context, req admission.Request) admission.Response {
l := logf.FromContext(ctx)
wl := &workload.PodWorkload{
Pod: &corev1.Pod{},
}
err := a.decoder.Decode(req, wl.Object())
p := corev1.Pod{}
err := a.decoder.Decode(req, &p)
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)
if err != nil {
return admission.Errored(http.StatusInternalServerError, err)
}

if updatedPod == nil {
return admission.Allowed("no changes to pod")
}

// Marshal the updated Pod and prepare to send a response
marshaledRes, err := json.Marshal(updatedPod)
if err != nil {
l.Error(err, "Unable to marshal workload result in webhook",
"kind", req.Kind.Kind, "ns", req.Namespace, "name", req.Name)
return admission.Errored(http.StatusInternalServerError,
fmt.Errorf("unable to marshal workload result"))
}

return admission.PatchResponseFromRaw(req.Object.Raw, marshaledRes)
}

// handleCreatePodRequest Finds relevant AuthProxyWorkload resources and updates the pod
// with matching resources, returning a non-nil pod when the pod was updated.
func (a *PodAdmissionWebhook) handleCreatePodRequest(ctx context.Context, p corev1.Pod) (*corev1.Pod, error) {
var (
instList = &cloudsqlapi.AuthProxyWorkloadList{}
proxies []*cloudsqlapi.AuthProxyWorkload
wlConfigErr error
l = logf.FromContext(ctx)
wl = &workload.PodWorkload{Pod: &p}
)

// List all the AuthProxyWorkloads in the same namespace.
// To avoid privilege escalation, the operator requires that the AuthProxyWorkload
// may only affect pods in the same namespace.
err = a.Client.List(ctx, instList, client.InNamespace(wl.Object().GetNamespace()))
err := a.Client.List(ctx, instList, client.InNamespace(wl.Object().GetNamespace()))
if err != nil {
l.Error(err, "Unable to list CloudSqlClient resources in webhook",
"kind", req.Kind.Kind, "ns", req.Namespace, "name", req.Name)
return admission.Errored(http.StatusInternalServerError,
fmt.Errorf("unable to list CloudSqlClient resources"))
"kind", wl.Pod.Kind, "ns", wl.Pod.Namespace, "name", wl.Pod.Name)
return nil, fmt.Errorf("unable to list AuthProxyWorkloads, %v", err)
}

// List the owners of this pod.
owners, err := a.listOwners(ctx, wl.Object())
if err != nil {
return admission.Errored(http.StatusInternalServerError,
fmt.Errorf("there is an AuthProxyWorkloadConfiguration error reconciling this workload %v", err))
return nil, fmt.Errorf("there is an AuthProxyWorkloadConfiguration error reconciling this workload %v", err)
}

// Find matching AuthProxyWorkloads for this pod
proxies = a.updater.FindMatchingAuthProxyWorkloads(instList, wl, owners)
if len(proxies) == 0 {
return admission.PatchResponseFromRaw(req.Object.Raw, req.Object.Raw)
return nil, nil // no change
}

// Configure the pod, adding containers for each of the proxies
wlConfigErr = a.updater.ConfigureWorkload(wl, proxies)

if wlConfigErr != nil {
l.Error(wlConfigErr, "Unable to reconcile workload result in webhook: "+wlConfigErr.Error(),
"kind", req.Kind.Kind, "ns", req.Namespace, "name", req.Name)
return admission.Errored(http.StatusInternalServerError,
fmt.Errorf("there is an AuthProxyWorkloadConfiguration error reconciling this workload %v", wlConfigErr))
}

// Log some information about the pod update
l.Info(fmt.Sprintf("Workload operation %s on kind %s named %s/%s required an update",
req.Operation, req.Kind, req.Namespace, req.Name))
for _, inst := range proxies {
l.Info(fmt.Sprintf("inst %v %v/%v updated at instance resource version %v",
wl.Object().GetObjectKind().GroupVersionKind().String(),
wl.Object().GetNamespace(), wl.Object().GetName(),
inst.GetResourceVersion()))
}

// Marshal the updated Pod and prepare to send a response
result := wl.Object()
marshaledRes, err := json.Marshal(result)
if err != nil {
l.Error(err, "Unable to marshal workload result in webhook",
"kind", req.Kind.Kind, "ns", req.Namespace, "name", req.Name)
return admission.Errored(http.StatusInternalServerError,
fmt.Errorf("unable to marshal workload result"))
"kind", wl.Pod.Kind, "ns", wl.Pod.Namespace, "name", wl.Pod.Name)
return nil, fmt.Errorf("there is an AuthProxyWorkloadConfiguration error reconciling this workload %v", wlConfigErr)
}

return admission.PatchResponseFromRaw(req.Object.Raw, marshaledRes)
return wl.Pod, nil // updated
}

// listOwners returns the list of this object's owners and its extended owners.
Expand All @@ -134,14 +135,14 @@ func (a *PodAdmissionWebhook) listOwners(ctx context.Context, object client.Obje

wl, err := workload.WorkloadForKind(r.Kind)
if err != nil {
owner = &metav1.PartialObjectMetadata{
TypeMeta: metav1.TypeMeta{Kind: r.Kind, APIVersion: r.APIVersion},
}
} else {
owners = append(owners, wl)
owner = wl.Object()
// If the operator doesn't recognize the owner's Kind, then ignore
// that owner.
continue
}

owners = append(owners, wl)
owner = wl.Object()

err = a.Client.Get(ctx, key, owner)
if err != nil {
switch t := err.(type) {
Expand Down
158 changes: 158 additions & 0 deletions internal/controller/pod_controller_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
// Copyright 2023 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package controller

import (
"context"
"testing"

cloudsqlapi "github.com/GoogleCloudPlatform/cloud-sql-proxy-operator/internal/api/v1alpha1"
"github.com/GoogleCloudPlatform/cloud-sql-proxy-operator/internal/testhelpers"
"github.com/GoogleCloudPlatform/cloud-sql-proxy-operator/internal/workload"
appsv1 "k8s.io/api/apps/v1"
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
"sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
)

func TestPodWebhookWithDeploymentOwners(t *testing.T) {
_, scheme, err := clientBuilder()
if err != nil {
t.Fatal(err)
}

// Proxy workload
p := testhelpers.BuildAuthProxyWorkload(types.NamespacedName{
Namespace: "default",
Name: "test",
}, "project:region:db")
addFinalizers(p)
addSelectorWorkload(p, "Deployment", "app", "webapp")

// Deployment that matches the proxy
dMatch := testhelpers.BuildDeployment(types.NamespacedName{
Namespace: "default",
Name: "test",
}, "webapp")
dMatch.ObjectMeta.Labels = map[string]string{"app": "webapp"}

// Deployment that does not match the proxy
dNoMatch := testhelpers.BuildDeployment(types.NamespacedName{
Namespace: "default",
Name: "test",
}, "webapp")
dNoMatch.ObjectMeta.Labels = map[string]string{"app": "other"}

// Deployment matches the proxy and is owned by another resource
// called CustomApp
dWithOwner := testhelpers.BuildDeployment(types.NamespacedName{
Namespace: "default",
Name: "test",
}, "webapp")
dWithOwner.ObjectMeta.Labels = map[string]string{"app": "webapp"}
deploymentOwner := &v1.PartialObjectMetadata{
TypeMeta: v1.TypeMeta{Kind: "CustomApp", APIVersion: "v1"},
ObjectMeta: v1.ObjectMeta{Name: "custom-app", Namespace: "default"},
}
err = controllerutil.SetOwnerReference(deploymentOwner, dWithOwner, scheme)
if err != nil {
t.Fatal(err)
}

data := []struct {
name string
p *cloudsqlapi.AuthProxyWorkload
d *appsv1.Deployment
wantUpdate bool
}{
{
name: "Deployment Pod with matching Workload",
p: p,
d: dMatch,
wantUpdate: true,
},
{
name: "Deployment Pod with no match",
p: p,
d: dNoMatch,
wantUpdate: false,
},
{
name: "Deployment Pod with unknown owner",
p: p,
d: dWithOwner,
wantUpdate: true,
},
}
for _, tc := range data {
t.Run(tc.name, func(t *testing.T) {
cb, scheme, err := clientBuilder()
if err != nil {
t.Fatal(err)
}

rs, hash, err := testhelpers.BuildDeploymentReplicaSet(tc.d, scheme)
if err != nil {
t.Fatal(err)
}
pods, err := testhelpers.BuildDeploymentReplicaSetPods(tc.d, rs, hash, scheme)
if err != nil {
t.Fatal(err)
}

c := cb.WithObjects(p).WithObjects(rs).WithObjects(tc.d).Build()
wh, ctx, err := podWebhookController(c)
if err != nil {
t.Fatal(err)
}

pod, errRes := wh.handleCreatePodRequest(ctx, *pods[0])

if errRes != nil {
t.Fatal("got error, want no error")
}
if tc.wantUpdate && pod == nil {
t.Fatal("got nil, want not nil workload indicating pod updates")
}
if !tc.wantUpdate && pod != nil {
t.Fatal("got non-nil workload, want nil indicating no pod updates")
}

if err != nil {
t.Fatal(err)
}

})
}

}

func podWebhookController(cb client.Client) (*PodAdmissionWebhook, context.Context, error) {
ctx := log.IntoContext(context.Background(), logger)
d, err := admission.NewDecoder(cb.Scheme())
if err != nil {
return nil, nil, err
}
r := &PodAdmissionWebhook{
Client: cb,
decoder: d,
updater: workload.NewUpdater("cloud-sql-proxy-operator/dev"),
}

return r, ctx, nil
}
Loading

0 comments on commit 12be1dc

Please sign in to comment.