From e12ec1eb78a6d16c888c589ba38ba7a0ae7c32f3 Mon Sep 17 00:00:00 2001 From: Nick Young Date: Mon, 4 May 2020 17:00:33 +1000 Subject: [PATCH] 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 --- apis/projectcontour/v1/httpproxy.go | 1 + cmd/contour/serve.go | 3 +- examples/contour/01-crds.yaml | 3 +- examples/contour/02-rbac.yaml | 6 +++ examples/render/contour.yaml | 9 +++- internal/k8s/converter.go | 29 +++++++++--- internal/k8s/converter_test.go | 2 +- internal/k8s/status.go | 27 ++++------- internal/k8s/status_test.go | 70 ++++++++++++++++++++--------- internal/k8s/syncer.go | 1 + 10 files changed, 101 insertions(+), 50 deletions(-) diff --git a/apis/projectcontour/v1/httpproxy.go b/apis/projectcontour/v1/httpproxy.go index 6951a98109f..c7ae244da59 100644 --- a/apis/projectcontour/v1/httpproxy.go +++ b/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"` diff --git a/cmd/contour/serve.go b/cmd/contour/serve.go index a02e3cd5d25..2cd06991b3f 100644 --- a/cmd/contour/serve.go +++ b/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{ diff --git a/examples/contour/01-crds.yaml b/examples/contour/01-crds.yaml index 4f5d71f3045..3190b8b0e62 100644 --- a/examples/contour/01-crds.yaml +++ b/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 diff --git a/examples/contour/02-rbac.yaml b/examples/contour/02-rbac.yaml index bfcb123346c..0ec7f944c0c 100644 --- a/examples/contour/02-rbac.yaml +++ b/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: diff --git a/examples/render/contour.yaml b/examples/render/contour.yaml index 025dfcc7195..bf2405b7298 100644 --- a/examples/render/contour.yaml +++ b/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: diff --git a/internal/k8s/converter.go b/internal/k8s/converter.go index 9706739f1b0..ed4a13f44d0 100644 --- a/internal/k8s/converter.go +++ b/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 +} diff --git a/internal/k8s/converter_test.go b/internal/k8s/converter_test.go index c6ae4e8ac39..93a1da771ad 100644 --- a/internal/k8s/converter_test.go +++ b/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) diff --git a/internal/k8s/status.go b/internal/k8s/status.go index e7be313d31b..3a50cb15d67 100644 --- a/internal/k8s/status.go +++ b/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 } diff --git a/internal/k8s/status_test.go b/internal/k8s/status_test.go index 0b86a152101..d869a0160f3 100644 --- a/internal/k8s/status_test.go +++ b/internal/k8s/status_test.go @@ -131,11 +131,10 @@ 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) { @@ -143,34 +142,48 @@ func TestSetHTTPProxyStatus(t *testing.T) { 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", + }, + }, }) } diff --git a/internal/k8s/syncer.go b/internal/k8s/syncer.go index 52f70e4afab..1d71b6ef49a 100644 --- a/internal/k8s/syncer.go +++ b/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 (