Skip to content

Commit

Permalink
Ignore status patches (#219)
Browse files Browse the repository at this point in the history 
Patching the status property with Eno isn't supported for resources that
use the status subresource.

Unfortunately, taking a typical controller-runtime CRD struct and
encoding it as json will set `"status":{}` even though apiserver will
continue to return `"status":nil` until a client writes to the status
subresource.

So synthesizers that use Go structs will block composition readiness
until another controller updates the resource's status.

It doesn't make sense for Eno to patch status in the first place and
supporting the subresource would be somewhat complicated. So I think
it's best to just ignore the status field for now.

---------

Co-authored-by: Jordan Olshevski <[email protected]>
  • Loading branch information
@jveski
jveski and Jordan Olshevski authored Oct 14, 2024
1 parent 6e50610 commit 32db261
Show file tree
Hide file tree
Showing 3 changed files with 158 additions and 3 deletions.
2 changes: 1 addition & 1 deletion internal/controllers/reconciliation/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,6 @@ func (c *Controller) getCurrent(ctx context.Context, resource *reconstitution.Re
current := &unstructured.Unstructured{}
current.SetName(resource.Ref.Name)
current.SetNamespace(resource.Ref.Namespace)
current.SetKind(resource.Ref.Kind)
current.SetKind(resource.GVK.Kind)
current.SetAPIVersion(resource.GVK.GroupVersion().String())
err := c.upstreamClient.Get(ctx, client.ObjectKeyFromObject(current), current)
Expand All @@ -380,6 +379,7 @@ func mungePatch(patch []byte, rv string) ([]byte, error) {
if err != nil {
return nil, reconcile.TerminalError(err)
}
delete(patchMap, "status")

u := unstructured.Unstructured{Object: patchMap}
a, err := meta.Accessor(&u)
Expand Down
153 changes: 153 additions & 0 deletions internal/controllers/reconciliation/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,15 @@ import (
"testing"
"time"

apiv1 "github.com/Azure/eno/api/v1"
"github.com/Azure/eno/internal/discovery"
"github.com/Azure/eno/internal/flowcontrol"
"github.com/Azure/eno/internal/reconstitution"
"github.com/Azure/eno/internal/testutil"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/types"
)

func TestMungePatch(t *testing.T) {
Expand All @@ -18,11 +22,160 @@ func TestMungePatch(t *testing.T) {
}

func TestMungePatchEmpty(t *testing.T) {
patch, err := mungePatch([]byte(`{}`), "test-rv")
require.NoError(t, err)
assert.Nil(t, patch)
}

func TestMungePatchOnlyCreationTimestamp(t *testing.T) {
patch, err := mungePatch([]byte(`{"metadata":{"creationTimestamp":"2024-03-05T00:45:27Z"}}`), "test-rv")
require.NoError(t, err)
assert.Nil(t, patch)
}

func TestBuildPatchEmpty(t *testing.T) {
ctx := testutil.NewContext(t)
mgr := testutil.NewManager(t)
dc, err := discovery.NewCache(mgr.DownstreamRestConfig, 10)
require.NoError(t, err)
c := &Controller{discovery: dc}

tests := []struct {
Name string
Type types.PatchType
Next, Current map[string]any
}{
{
Name: "empty non-strategic",
Type: types.MergePatchType,
Current: map[string]any{
"apiVersion": "test.io/v1",
"kind": "anything",
"metadata": map[string]any{"name": "foo", "namespace": "default"},
"spec": map[string]any{"value": "initial"},
},
},
{
Name: "empty non-strategic with status",
Type: types.MergePatchType,
Current: map[string]any{
"apiVersion": "test.io/v1",
"kind": "anything",
"metadata": map[string]any{"name": "foo", "namespace": "default"},
"spec": map[string]any{"value": "initial"},
"status": map[string]any{"statusValue": "initial"},
},
},
{
Name: "empty strategic",
Type: types.StrategicMergePatchType,
Current: map[string]any{
"apiVersion": "v1",
"kind": "Pod",
"metadata": map[string]any{"name": "foo", "namespace": "default"},
"spec": map[string]any{"serviceAccountName": "initial"},
},
},
{
Name: "status mismatched non-strategic",
Type: types.MergePatchType,
Current: map[string]any{
"apiVersion": "test.io/v1",
"kind": "anything",
"metadata": map[string]any{"name": "foo", "namespace": "default"},
"spec": map[string]any{"value": "initial"},
"status": map[string]any{"statusValue": "initial"},
},
Next: map[string]any{
"apiVersion": "test.io/v1",
"kind": "anything",
"metadata": map[string]any{"name": "foo", "namespace": "default"},
"spec": map[string]any{"value": "initial"},
"status": map[string]any{"statusValue": "updated"},
},
},
{
Name: "status mismatched strategic",
Type: types.StrategicMergePatchType,
Current: map[string]any{
"apiVersion": "v1",
"kind": "Pod",
"metadata": map[string]any{"name": "foo", "namespace": "default"},
"status": map[string]any{"message": "initial"},
},
Next: map[string]any{
"apiVersion": "v1",
"kind": "Pod",
"metadata": map[string]any{"name": "foo", "namespace": "default"},
"status": map[string]any{"message": "updated"},
},
},
{
Name: "unordered non-strategic",
Type: types.MergePatchType,
Current: map[string]any{
"apiVersion": "test.io/v1",
"kind": "anything",
"metadata": map[string]any{"name": "foo", "namespace": "default"},
"spec": map[string]any{"one": "first", "two": "second"},
},
Next: map[string]any{
"apiVersion": "test.io/v1",
"kind": "anything",
"metadata": map[string]any{"name": "foo", "namespace": "default"},
"spec": map[string]any{"two": "second", "one": "first"},
},
},
{
Name: "unordered strategic",
Type: types.StrategicMergePatchType,
Current: map[string]any{
"apiVersion": "v1",
"kind": "Pod",
"metadata": map[string]any{"name": "foo", "namespace": "default"},
"spec": map[string]any{"serviceAccountName": "initial", "initContainers": []any{}},
},
Next: map[string]any{
"apiVersion": "v1",
"kind": "Pod",
"metadata": map[string]any{"name": "foo", "namespace": "default"},
"spec": map[string]any{"initContainers": []any{}, "serviceAccountName": "initial"},
},
},
}

for _, test := range tests {
t.Run(test.Name, func(t *testing.T) {
if test.Next == nil {
test.Next = test.Current
}

current, prev := mapToResource(t, test.Current)
_, next := mapToResource(t, test.Next)

patch, kind, err := c.buildPatch(ctx, prev, next, current)
require.NoError(t, err)

patch, err = mungePatch(patch, "random-rv")
require.NoError(t, err)
assert.Nil(t, patch)
assert.Equal(t, test.Type, kind)
})
}
}

func mapToResource(t *testing.T, res map[string]any) (*unstructured.Unstructured, *reconstitution.Resource) {
obj := &unstructured.Unstructured{Object: res}
js, err := obj.MarshalJSON()
require.NoError(t, err)

rr := &reconstitution.Resource{
Manifest: &apiv1.Manifest{Manifest: string(js)},
GVK: obj.GroupVersionKind(),
}
return obj, rr
}

func setupTestSubject(t *testing.T, mgr *testutil.Manager) *Controller {
rswb := flowcontrol.NewResourceSliceWriteBufferForManager(mgr.Manager, time.Millisecond*10, 1)
cache := reconstitution.NewCache(mgr.GetClient())
Expand Down
6 changes: 4 additions & 2 deletions internal/controllers/reconciliation/crud_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,15 +63,17 @@ var crudTests = []crudTestCase{
Protocol: corev1.ProtocolTCP,
}},
},
Status: corev1.ServiceStatus{LoadBalancer: corev1.LoadBalancerStatus{Ingress: []corev1.LoadBalancerIngress{}}},
},
AssertCreated: func(t *testing.T, obj client.Object) {
svc := obj.(*corev1.Service).Spec
svc := obj.(*corev1.Service)
assert.Equal(t, []corev1.ServicePort{{
Name: "first",
Port: 1234,
Protocol: corev1.ProtocolTCP,
TargetPort: intstr.FromInt(1234),
}}, svc.Ports)
}}, svc.Spec.Ports)
assert.Nil(t, svc.Status.LoadBalancer.Ingress)
},
ApplyExternalUpdate: func(t *testing.T, obj client.Object) client.Object {
svc := obj.(*corev1.Service).DeepCopy()
Expand Down

0 comments on commit 32db261

Please sign in to comment.