Skip to content

Commit

Permalink
fix: don't update the Pod after it's been created (#35)
Browse files Browse the repository at this point in the history 
Closes #27.

The original code would createOrUpdate the `Pod` resource. The problem
is that we were then overwriting the `metadata.annotations` field on
updates.

The issue we ran into was this...

1. Oz creates the Pod
2. Istio's Webhook Endpoint mutates the Pod Labels and Annotations
3. Oz's secondary reconcile loop immediately comes in and replaces the
metadata.annotations with the original empty annotations
4. Istio doesn't re-apply the annotations because the metadata.labels
were mutated and indicate that the webhook has already happened.
5. Istio-validation container won't start up
  • Loading branch information
@diranged
diranged authored Nov 28, 2022
1 parent ec8f90b commit 17a7a0c
Show file tree
Hide file tree
Showing 10 changed files with 247 additions and 55 deletions.
18 changes: 3 additions & 15 deletions controllers/base_request_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
. "github.com/onsi/gomega"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"

"sigs.k8s.io/controller-runtime/pkg/client"
Expand All @@ -37,7 +36,7 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() {
r = &BaseRequestReconciler{
BaseReconciler: BaseReconciler{
Client: fakeClient,
Scheme: &runtime.Scheme{},
Scheme: fakeClient.Scheme(),
APIReader: fakeClient,
ReconcililationInterval: 0,
},
Expand Down Expand Up @@ -66,7 +65,6 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() {
BaseBuilder: builders.BaseBuilder{
Client: fakeClient,
Ctx: ctx,
Scheme: &runtime.Scheme{},
APIReader: fakeClient,
Request: request,
},
Expand Down Expand Up @@ -175,7 +173,7 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() {
r = &BaseRequestReconciler{
BaseReconciler: BaseReconciler{
Client: fakeClient,
Scheme: &runtime.Scheme{},
Scheme: fakeClient.Scheme(),
APIReader: fakeClient,
ReconcililationInterval: 0,
},
Expand Down Expand Up @@ -216,7 +214,6 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() {
builder = &builders.BaseBuilder{
Client: fakeClient,
Ctx: ctx,
Scheme: &runtime.Scheme{},
APIReader: fakeClient,
Request: request,
}
Expand Down Expand Up @@ -261,7 +258,6 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() {
builder = &builders.BaseBuilder{
Client: fakeClient,
Ctx: ctx,
Scheme: &runtime.Scheme{},
APIReader: fakeClient,
Request: request,
}
Expand Down Expand Up @@ -309,7 +305,6 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() {
builder = &builders.BaseBuilder{
Client: fakeClient,
Ctx: ctx,
Scheme: &runtime.Scheme{},
APIReader: fakeClient,
Request: request,
}
Expand Down Expand Up @@ -365,7 +360,7 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() {
r = &BaseRequestReconciler{
BaseReconciler: BaseReconciler{
Client: fakeClient,
Scheme: &runtime.Scheme{},
Scheme: fakeClient.Scheme(),
APIReader: fakeClient,
logger: logger,
ReconcililationInterval: 0,
Expand Down Expand Up @@ -396,7 +391,6 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() {
builder = &builders.BaseBuilder{
Client: fakeClient,
Ctx: ctx,
Scheme: &runtime.Scheme{},
APIReader: fakeClient,
Template: template,
Request: request,
Expand Down Expand Up @@ -448,7 +442,6 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() {
builder = &builders.BaseBuilder{
Client: fakeClient,
Ctx: ctx,
Scheme: &runtime.Scheme{},
APIReader: fakeClient,
Template: template,
Request: request,
Expand Down Expand Up @@ -502,7 +495,6 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() {
builder = &builders.BaseBuilder{
Client: fakeClient,
Ctx: ctx,
Scheme: &runtime.Scheme{},
APIReader: fakeClient,
Template: template,
Request: request,
Expand Down Expand Up @@ -556,7 +548,6 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() {
builder = &builders.BaseBuilder{
Client: fakeClient,
Ctx: ctx,
Scheme: &runtime.Scheme{},
APIReader: fakeClient,
Template: template,
Request: request,
Expand Down Expand Up @@ -622,7 +613,6 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() {
builder = &builders.BaseBuilder{
Client: fakeClient,
Ctx: ctx,
Scheme: &runtime.Scheme{},
APIReader: fakeClient,
Template: template,
Request: request,
Expand Down Expand Up @@ -687,7 +677,6 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() {
builder = &builders.BaseBuilder{
Client: fakeClient,
Ctx: ctx,
Scheme: &runtime.Scheme{},
APIReader: fakeClient,
Template: template,
Request: request,
Expand Down Expand Up @@ -740,7 +729,6 @@ var _ = Describe("BaseRequestReconciler", Ordered, func() {
builder = &builders.BaseBuilder{
Client: fakeClient,
Ctx: ctx,
Scheme: &runtime.Scheme{},
APIReader: fakeClient,
Template: template,
Request: request,
Expand Down
11 changes: 1 addition & 10 deletions controllers/base_template_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"

"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
Expand Down Expand Up @@ -72,7 +71,6 @@ var _ = Describe("BaseTemplateReconciler", Ordered, func() {
r = &BaseTemplateReconciler{
BaseReconciler: BaseReconciler{
Client: fakeClient,
Scheme: &runtime.Scheme{},
APIReader: fakeClient,
logger: logger,
ReconcililationInterval: 0,
Expand Down Expand Up @@ -109,7 +107,6 @@ var _ = Describe("BaseTemplateReconciler", Ordered, func() {
builder = &builders.BaseBuilder{
Client: fakeClient,
Ctx: ctx,
Scheme: &runtime.Scheme{},
APIReader: fakeClient,
Template: template,
}
Expand Down Expand Up @@ -153,7 +150,6 @@ var _ = Describe("BaseTemplateReconciler", Ordered, func() {
r = &BaseTemplateReconciler{
BaseReconciler: BaseReconciler{
Client: fakeClient,
Scheme: &runtime.Scheme{},
APIReader: fakeClient,
logger: logger,
ReconcililationInterval: 0,
Expand All @@ -164,7 +160,6 @@ var _ = Describe("BaseTemplateReconciler", Ordered, func() {
builder = &builders.BaseBuilder{
Client: fakeClient,
Ctx: ctx,
Scheme: &runtime.Scheme{},
APIReader: fakeClient,
Template: template,
}
Expand Down Expand Up @@ -201,7 +196,7 @@ var _ = Describe("BaseTemplateReconciler", Ordered, func() {
r = &BaseTemplateReconciler{
BaseReconciler: BaseReconciler{
Client: fakeClient,
Scheme: &runtime.Scheme{},
Scheme: fakeClient.Scheme(),
APIReader: fakeClient,
logger: logger,
ReconcililationInterval: 0,
Expand Down Expand Up @@ -237,7 +232,6 @@ var _ = Describe("BaseTemplateReconciler", Ordered, func() {
builder = &builders.BaseBuilder{
Client: fakeClient,
Ctx: ctx,
Scheme: &runtime.Scheme{},
APIReader: fakeClient,
Template: template,
}
Expand Down Expand Up @@ -287,7 +281,6 @@ var _ = Describe("BaseTemplateReconciler", Ordered, func() {
builder = &builders.BaseBuilder{
Client: fakeClient,
Ctx: ctx,
Scheme: &runtime.Scheme{},
APIReader: fakeClient,
Template: template,
}
Expand Down Expand Up @@ -339,7 +332,6 @@ var _ = Describe("BaseTemplateReconciler", Ordered, func() {
builder = &builders.BaseBuilder{
Client: fakeClient,
Ctx: ctx,
Scheme: &runtime.Scheme{},
APIReader: fakeClient,
Template: template,
}
Expand Down Expand Up @@ -391,7 +383,6 @@ var _ = Describe("BaseTemplateReconciler", Ordered, func() {
builder = &builders.BaseBuilder{
Client: fakeClient,
Ctx: ctx,
Scheme: &runtime.Scheme{},
APIReader: fakeClient,
Template: template,
}
Expand Down
49 changes: 26 additions & 23 deletions controllers/builders/base_builder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ type BaseBuilder struct {

Client client.Client
Ctx context.Context
Scheme *runtime.Scheme

// APIReader should be generated with mgr.GetAPIReader() to create a non-cached client object. This is used
// for certain Get() calls where we need to ensure we are getting the latest version from the API, and not a cached
Expand Down Expand Up @@ -120,7 +119,7 @@ func (b *BaseBuilder) GetCtx() context.Context {
//
// *runtime.Scheme: A pointer back to the runtime.Scheme from the controller-runtime struct.
func (b *BaseBuilder) GetScheme() *runtime.Scheme {
return b.Scheme
return b.Client.Scheme()
}

// GetTemplate provides an access method to the generic api.OzTemplateResource interface
Expand Down Expand Up @@ -339,7 +338,7 @@ func (b *BaseBuilder) createAccessRole(podName string) (*rbacv1.Role, error) {

// Set the ownerRef for the Deployment
// More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/owners-dependents/
if err := ctrlutil.SetControllerReference(b.Request, role, b.Scheme); err != nil {
if err := ctrlutil.SetControllerReference(b.Request, role, b.GetScheme()); err != nil {
return nil, err
}

Expand Down Expand Up @@ -385,7 +384,7 @@ func (b *BaseBuilder) createAccessRoleBinding() (*rbacv1.RoleBinding, error) {

// Set the ownerRef for the Deployment
// More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/owners-dependents/
if err := ctrlutil.SetControllerReference(b.Request, rb, b.Scheme); err != nil {
if err := ctrlutil.SetControllerReference(b.Request, rb, b.GetScheme()); err != nil {
return nil, err
}

Expand Down Expand Up @@ -413,24 +412,39 @@ func (b *BaseBuilder) createAccessRoleBinding() (*rbacv1.RoleBinding, error) {
func (b *BaseBuilder) createPod(podTemplateSpec corev1.PodTemplateSpec) (*corev1.Pod, error) {
logger := log.FromContext(b.Ctx)

// Desired pod
// We'll populate this pod object
pod := &corev1.Pod{}
pod.Name = generateResourceName(b.Request)
pod.Namespace = b.Template.GetNamespace()

// Verify first whether or not a pod already exists with this name. If it
// does, we just return it back. The issue here is that updating a Pod is
// an unusual thing to do once it's alive, and can cause race condition
// issues if you do not do the updates properly.
//
// https://github.com/diranged/oz/issues/27
err := b.Client.Get(b.Ctx, types.NamespacedName{
Name: pod.Name,
Namespace: pod.Namespace,
}, pod)

// If there was no error on this get, then the object already exists in K8S
// and we need to just return that.
if err == nil {
return pod, err
}

// Finish filling out the desired PodSpec at this point.
pod.Spec = *podTemplateSpec.Spec.DeepCopy()
pod.ObjectMeta.Annotations = podTemplateSpec.ObjectMeta.Annotations

// Disabled until we implement a selectorLabel filter.
//
// pod.ObjectMeta.Labels = podTemplateSpec.Labels

// Generate an emptyPod resource. We use this to define the type, name and namespace of the resource, which
// will be passed into the CreateOrUpdate function.
emptyPod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: pod.Name, Namespace: pod.Namespace}}

// Set the ownerRef for the Deployment
// More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/owners-dependents/
if err := ctrlutil.SetControllerReference(b.Request, pod, b.Scheme); err != nil {
if err := ctrlutil.SetControllerReference(b.Request, pod, b.GetScheme()); err != nil {
return nil, err
}

Expand All @@ -439,19 +453,7 @@ func (b *BaseBuilder) createPod(podTemplateSpec corev1.PodTemplateSpec) (*corev1
// In an update event, wec an only update the Annotations and the OwnerReference. Nothing else.
logger.V(1).Info(fmt.Sprintf("Creating or Updating Pod %s (ns: %s)", pod.Name, pod.Namespace))
logger.V(1).Info("Pod Json", "json", ObjectToJSON(pod))
if _, err := ctrlutil.CreateOrUpdate(b.Ctx, b.Client, emptyPod, func() error {
// Mutable fields that we can update on each iteration.
emptyPod.ObjectMeta.Annotations = pod.GetObjectMeta().GetAnnotations()
emptyPod.OwnerReferences = pod.OwnerReferences

// We can only set the PodSpec once. If the PodSpec is empty, then we pass in the desired
// PodSpec. After that, we ignore all future updates to it.
if len(emptyPod.Spec.Containers) < 1 {
emptyPod.Spec = pod.Spec
}

return nil
}); err != nil {
if err := b.Client.Create(b.Ctx, pod); err != nil {
return nil, err
}

Expand All @@ -466,6 +468,7 @@ func (b *BaseBuilder) createPod(podTemplateSpec corev1.PodTemplateSpec) (*corev1
//
// shortUID: A 10-digit long shortened UID
func getShortUID(obj client.Object) string {
// TODO: If the UID isn't there, we should generate something random OR throw an error.
return string(obj.GetUID())[0:shortUIDLength]
}

Expand Down
Loading

0 comments on commit 17a7a0c

Please sign in to comment.