Add status subresource to HTTPProxy

This commit enables the status subresource for HTTPProxy.
This means that:
- normal HTTPProxy changes that include updates to the `status` stanza will have the status updates dropped
- updates to `status` can only be made using requests to the status subresource.

Also changes the HTTPProxy status updating to use the status subresource,
and adds RBAC for Contour to be able to update it.

Having the status subresource enabled is best-practice for CRDs and will be a foundation for further status
work.

It also means that HTTPProxy objects no longer need to include the `status` stanza to be valid.

Refactors the `k8s.UnstructuredConverter` to have FromUnstructured and ToUnstructured methods, and uses ToUnstructured where required.

Updates #2495

Signed-off-by: Nick Young <ynick@vmware.com>

apis/projectcontour/v1/httpproxy.go

@@ -423,6 +423,7 @@ type Status struct {
 // +kubebuilder:printcolumn:name="Status",type="string",JSONPath=".status.currentStatus",description="The current status of the HTTPProxy"
 // +kubebuilder:printcolumn:name="Status Description",type="string",JSONPath=".status.description",description="Description of the current status"
 // +kubebuilder:resource:scope=Namespaced,path=httpproxies,shortName=proxy;proxies,singular=httpproxy
+// +kubebuilder:subresource:status
 type HTTPProxy struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata"`

cmd/contour/serve.go

@@ -180,7 +180,8 @@ func doServe(log logrus.FieldLogger, ctx *serveContext) error {
 		HoldoffDelay:    100 * time.Millisecond,
 		HoldoffMaxDelay: 500 * time.Millisecond,
 		StatusClient: &k8s.StatusWriter{
-			Client: clients.DynamicClient(),
+			Client:    clients.DynamicClient(),
+			Converter: converter,
 		},
 		Builder: dag.Builder{
 			Source: dag.KubernetesCache{

examples/contour/01-crds.yaml

@@ -479,7 +479,8 @@ spec:
     - proxies
     singular: httpproxy
   scope: Namespaced
-  subresources: {}
+  subresources:
+    status: {}
   validation:
     openAPIV3Schema:
       description: HTTPProxy is an Ingress CRD specification

examples/contour/02-rbac.yaml

@@ -79,6 +79,12 @@ rules:
   - put
   - post
   - patch
+- apiGroups:
+  - "projectcontour.io"
+  resources:
+  - "httpproxies/status"
+  verbs:
+  - update
 - apiGroups: ["networking.x.k8s.io"]
   resources: ["gatewayclasses", "gateways", "httproutes", "tcproutes"]
   verbs:

examples/render/contour.yaml

@@ -571,7 +571,8 @@ spec:
     - proxies
     singular: httpproxy
   scope: Namespaced
-  subresources: {}
+  subresources:
+    status: {}
   validation:
     openAPIV3Schema:
       description: HTTPProxy is an Ingress CRD specification
@@ -1521,6 +1522,12 @@ rules:
   - put
   - post
   - patch
+- apiGroups:
+  - "projectcontour.io"
+  resources:
+  - "httpproxies/status"
+  verbs:
+  - update
 - apiGroups: ["networking.x.k8s.io"]
   resources: ["gatewayclasses", "gateways", "httproutes", "tcproutes"]
   verbs:

internal/k8s/converter.go

@@ -14,6 +14,7 @@
 package k8s
 
 import (
+	"context"
 	"fmt"
 
 	ingressroutev1 "github.com/projectcontour/contour/apis/contour/v1beta1"
@@ -43,7 +44,7 @@ type DynamicClientHandler struct {
 }
 
 func (d *DynamicClientHandler) OnAdd(obj interface{}) {
-	obj, err := d.Converter.Convert(obj)
+	obj, err := d.Converter.FromUnstructured(obj)
 	if err != nil {
 		d.Logger.Error(err)
 		return
@@ -52,12 +53,12 @@ func (d *DynamicClientHandler) OnAdd(obj interface{}) {
 }
 
 func (d *DynamicClientHandler) OnUpdate(oldObj, newObj interface{}) {
-	oldObj, err := d.Converter.Convert(oldObj)
+	oldObj, err := d.Converter.FromUnstructured(oldObj)
 	if err != nil {
 		d.Logger.Error(err)
 		return
 	}
-	newObj, err = d.Converter.Convert(newObj)
+	newObj, err = d.Converter.FromUnstructured(newObj)
 	if err != nil {
 		d.Logger.Error(err)
 		return
@@ -66,7 +67,7 @@ func (d *DynamicClientHandler) OnUpdate(oldObj, newObj interface{}) {
 }
 
 func (d *DynamicClientHandler) OnDelete(obj interface{}) {
-	obj, err := d.Converter.Convert(obj)
+	obj, err := d.Converter.FromUnstructured(obj)
 	if err != nil {
 		d.Logger.Error(err)
 		return
@@ -75,7 +76,8 @@ func (d *DynamicClientHandler) OnDelete(obj interface{}) {
 }
 
 type Converter interface {
-	Convert(obj interface{}) (interface{}, error)
+	FromUnstructured(obj interface{}) (interface{}, error)
+	ToUnstructured(obj interface{}) (*unstructured.Unstructured, error)
 }
 
 // UnstructuredConverter handles conversions between unstructured.Unstructured and Contour types
@@ -112,9 +114,9 @@ func NewUnstructuredConverter() (*UnstructuredConverter, error) {
 	return uc, nil
 }
 
-// Convert converts an unstructured.Unstructured to typed struct. If obj
+// FromUnstructured converts an unstructured.Unstructured to typed struct. If obj
 // is not an unstructured.Unstructured it is returned without further processing.
-func (c *UnstructuredConverter) Convert(obj interface{}) (interface{}, error) {
+func (c *UnstructuredConverter) FromUnstructured(obj interface{}) (interface{}, error) {
 	unstructured, ok := obj.(*unstructured.Unstructured)
 	if !ok {
 		return obj, nil
@@ -172,3 +174,16 @@ func (c *UnstructuredConverter) Convert(obj interface{}) (interface{}, error) {
 		return nil, fmt.Errorf("unsupported object type: %T", obj)
 	}
 }
+
+// ToUnstructured converts the supplied object to Unstructured, provided it's one of the types
+// registered in the UnstructuredConverter's Scheme.
+func (c *UnstructuredConverter) ToUnstructured(obj interface{}) (*unstructured.Unstructured, error) {
+
+	u := &unstructured.Unstructured{}
+
+	if err := c.scheme.Convert(obj, u, context.TODO()); err != nil {
+		return nil, err
+	}
+
+	return u, nil
+}

internal/k8s/converter_test.go

@@ -46,7 +46,7 @@ func TestConvertUnstructured(t *testing.T) {
 			if err != nil {
 				t.Fatal(err)
 			}
-			got, err := converter.Convert(tc.obj)
+			got, err := converter.FromUnstructured(tc.obj)
 
 			assert.Equal(t, tc.wantError, err)
 			assert.Equal(t, tc.want, got)

internal/k8s/status.go

@@ -112,7 +112,8 @@ func (c *StatusCacher) SetStatus(status, desc string, obj interface{}) error {
 
 // StatusWriter updates the object's Status field.
 type StatusWriter struct {
-	Client dynamic.Interface
+	Client    dynamic.Interface
+	Converter Converter
 }
 
 // GetStatus is not implemented for StatusWriter.
@@ -141,7 +142,7 @@ func (irs *StatusWriter) SetStatus(status, desc string, existing interface{}) er
 				CurrentStatus: status,
 				Description:   desc,
 			}
-			return irs.setHTTPProxyStatus(exist, updated)
+			return irs.setHTTPProxyStatus(updated)
 		}
 	}
 	return nil
@@ -176,24 +177,14 @@ func (irs *StatusWriter) setIngressRouteStatus(existing, updated *ingressroutev1
 	return err
 }
 
-func (irs *StatusWriter) setHTTPProxyStatus(existing, updated *projcontour.HTTPProxy) error {
-	existingBytes, err := json.Marshal(existing)
-	if err != nil {
-		return err
-	}
-	// Need to set the resource version of the updated endpoints to the resource
-	// version of the current service. Otherwise, the resulting patch does not
-	// have a resource version, and the server complains.
-	updated.ResourceVersion = existing.ResourceVersion
-	updatedBytes, err := json.Marshal(updated)
-	if err != nil {
-		return err
-	}
-	patchBytes, err := jsonpatch.CreateMergePatch(existingBytes, updatedBytes)
+func (irs *StatusWriter) setHTTPProxyStatus(updated *projcontour.HTTPProxy) error {
+
+	usUpdated, err := irs.Converter.ToUnstructured(updated)
 	if err != nil {
-		return err
+		return fmt.Errorf("unable to convert status update to HTTPProxy: %s", err)
 	}
 
-	_, err = irs.Client.Resource(projcontour.HTTPProxyGVR).Namespace(existing.GetNamespace()).Patch(context.TODO(), existing.GetName(), types.MergePatchType, patchBytes, metav1.PatchOptions{})
+	_, err = irs.Client.Resource(projcontour.HTTPProxyGVR).Namespace(updated.GetNamespace()).
+		UpdateStatus(context.TODO(), usUpdated, metav1.UpdateOptions{})
 	return err
 }

internal/k8s/status_test.go

@@ -131,46 +131,59 @@ func TestSetIngressRouteStatus(t *testing.T) {
 
 func TestSetHTTPProxyStatus(t *testing.T) {
 	type testcase struct {
-		msg           string
-		desc          string
-		existing      *projectcontour.HTTPProxy
-		expectedPatch string
-		expectedVerbs []string
+		msg      string
+		desc     string
+		existing *projectcontour.HTTPProxy
+		expected *projectcontour.HTTPProxy
 	}
 
 	run := func(t *testing.T, name string, tc testcase) {
 		t.Helper()
 
 		t.Run(name, func(t *testing.T) {
 			t.Helper()
-			var gotPatchBytes []byte
+			var gotObj runtime.Object
 			s := runtime.NewScheme()
 			projcontour.AddKnownTypes(s)
+			usc, err := NewUnstructuredConverter()
+			if err != nil {
+				t.Fatal(err)
+			}
+
 			client := fake.NewSimpleDynamicClient(s, tc.existing)
 
-			client.PrependReactor("patch", "httpproxies", func(action k8stesting.Action) (bool, runtime.Object, error) {
-				switch patchAction := action.(type) {
+			client.PrependReactor("*", "httpproxies", func(action k8stesting.Action) (bool, runtime.Object, error) {
+				switch updateAction := action.(type) {
 				default:
 					return true, nil, fmt.Errorf("got unexpected action of type: %T", action)
-				case k8stesting.PatchActionImpl:
-					gotPatchBytes = patchAction.GetPatch()
+				case k8stesting.UpdateActionImpl:
+					gotObj = updateAction.GetObject()
 					return true, tc.existing, nil
 				}
 			})
 
 			proxysw := StatusWriter{
-				Client: client,
+				Client:    client,
+				Converter: usc,
 			}
+
 			if err := proxysw.SetStatus(tc.msg, tc.desc, tc.existing); err != nil {
+				t.Fatal(fmt.Errorf("unable to set proxy status: %s", err))
+			}
+
+			toProxy, err := usc.FromUnstructured(gotObj)
+			if err != nil {
 				t.Fatal(err)
 			}
 
-			if len(client.Actions()) != len(tc.expectedVerbs) {
-				t.Fatalf("Expected verbs mismatch: want: %d, got: %d", len(tc.expectedVerbs), len(client.Actions()))
+			if toProxy == nil && tc.expected == nil {
+				return
 			}
 
-			if tc.expectedPatch != string(gotPatchBytes) {
-				t.Fatalf("expected patch: %s, got: %s", tc.expectedPatch, string(gotPatchBytes))
+			assert.Equal(t, toProxy, tc.expected)
+
+			if toProxy == nil && tc.expected != nil {
+				t.Fatalf("Did not get expected update, %#v", tc.expected)
 			}
 		})
 	}
@@ -188,8 +201,16 @@ func TestSetHTTPProxyStatus(t *testing.T) {
 				Description:   "",
 			},
 		},
-		expectedPatch: `{"status":{"currentStatus":"valid","description":"this is a valid HTTPProxy"}}`,
-		expectedVerbs: []string{"patch"},
+		expected: &projcontour.HTTPProxy{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "test",
+				Namespace: "default",
+			},
+			Status: projcontour.Status{
+				CurrentStatus: "valid",
+				Description:   "this is a valid HTTPProxy",
+			},
+		},
 	})
 
 	run(t, "no update", testcase{
@@ -205,8 +226,7 @@ func TestSetHTTPProxyStatus(t *testing.T) {
 				Description:   "this is a valid HTTPProxy",
 			},
 		},
-		expectedPatch: ``,
-		expectedVerbs: []string{},
+		expected: nil,
 	})
 
 	run(t, "replace existing status", testcase{
@@ -222,8 +242,16 @@ func TestSetHTTPProxyStatus(t *testing.T) {
 				Description:   "boo hiss",
 			},
 		},
-		expectedPatch: `{"status":{"currentStatus":"valid","description":"this is a valid HTTPProxy"}}`,
-		expectedVerbs: []string{"patch"},
+		expected: &projcontour.HTTPProxy{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "test",
+				Namespace: "default",
+			},
+			Status: projcontour.Status{
+				CurrentStatus: "valid",
+				Description:   "this is a valid HTTPProxy",
+			},
+		},
 	})
 }
 

internal/k8s/syncer.go

@@ -10,6 +10,7 @@
 // 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 k8s
 
 import (