m/n/k/reconciler: refactor resource interface
Replace interface{} with meta.Object, an interface which provides
accessors for and is implemented by meta.ObjectMeta. List now returns
the objects themselves instead of their names. This makes the reconciler
slightly less generic, as it now only supports kubernetes objects.
This is a refactoring in preparation for implementing updates in the
reconciler. There should be no change in behavior.
Change-Id: I97a4b1c0166a1e6fd0f247ee04e7c44cff570fd7
Reviewed-on: https://review.monogon.dev/c/monogon/+/2891
Reviewed-by: Lorenz Brun <lorenz@monogon.tech>
Tested-by: Jenkins CI
Vouch-Run-CI: Lorenz Brun <lorenz@monogon.tech>
diff --git a/metropolis/node/kubernetes/reconciler/reconciler_test.go b/metropolis/node/kubernetes/reconciler/reconciler_test.go
index ba2f4e8..b72ccb9 100644
--- a/metropolis/node/kubernetes/reconciler/reconciler_test.go
+++ b/metropolis/node/kubernetes/reconciler/reconciler_test.go
@@ -21,72 +21,19 @@
"fmt"
"testing"
- node "k8s.io/api/node/v1beta1"
- policy "k8s.io/api/policy/v1beta1"
- rbac "k8s.io/api/rbac/v1"
- storage "k8s.io/api/storage/v1"
meta "k8s.io/apimachinery/pkg/apis/meta/v1"
)
-// kubernetesMeta unwraps an interface{} that might contain a Kubernetes
-// resource of type that is managed by the reconciler. Any time a new
-// Kubernetes type is managed by the reconciler, the following switch should be
-// extended to cover that type.
-func kubernetesMeta(v interface{}) *meta.ObjectMeta {
- switch v2 := v.(type) {
- case *rbac.ClusterRole:
- return &v2.ObjectMeta
- case *rbac.ClusterRoleBinding:
- return &v2.ObjectMeta
- case *storage.CSIDriver:
- return &v2.ObjectMeta
- case *storage.StorageClass:
- return &v2.ObjectMeta
- case *policy.PodSecurityPolicy:
- return &v2.ObjectMeta
- case *node.RuntimeClass:
- return &v2.ObjectMeta
- }
- return nil
-}
-
-// TestExpectedNamedCorrectly ensures that all the Expected objects of all
-// resource types have a correspondence between their returned key and inner
-// name. This contract must be met in order for the reconciler to not create
-// runaway resources. This assumes all managed resources are Kubernetes
-// resources.
-func TestExpectedNamedCorrectly(t *testing.T) {
- for reconciler, r := range allResources(nil) {
- for outer, v := range r.Expected() {
- meta := kubernetesMeta(v)
- if meta == nil {
- t.Errorf("reconciler %q, object %q: could not decode kubernetes metadata", reconciler, outer)
- continue
- }
- if inner := meta.Name; outer != inner {
- t.Errorf("reconciler %q, object %q: inner name mismatch (%q)", reconciler, outer, inner)
- continue
- }
- }
- }
-}
-
// TestExpectedLabeledCorrectly ensures that all the Expected objects of all
// resource types have a Kubernetes metadata label that signifies it's a
// builtin object, to be retrieved afterwards. This contract must be met in
// order for the reconciler to not keep overwriting objects (and possibly
// failing), when a newly created object is not then retrievable using a
-// selector corresponding to this label. This assumes all managed resources are
-// Kubernetes objects.
+// selector corresponding to this label.
func TestExpectedLabeledCorrectly(t *testing.T) {
for reconciler, r := range allResources(nil) {
for outer, v := range r.Expected() {
- meta := kubernetesMeta(v)
- if meta == nil {
- t.Errorf("reconciler %q, object %q: could not decode kubernetes metadata", reconciler, outer)
- continue
- }
- if data := meta.Labels[BuiltinLabelKey]; data != BuiltinLabelValue {
+ if data := v.GetLabels()[BuiltinLabelKey]; data != BuiltinLabelValue {
t.Errorf("reconciler %q, object %q: %q=%q, wanted =%q", reconciler, outer, BuiltinLabelKey, data, BuiltinLabelValue)
continue
}
@@ -94,27 +41,41 @@
}
}
-// testResource is a resource type used for testing. The inner type is a string
-// that is equal to its name (key). It simulates a target (ie. k8s apiserver
-// mock) that always acts nominally (all resources are created, deleted as
-// requested, and the state is consistent with requests).
+// testObject is the object type managed by testResource.
+type testObject struct {
+ meta.ObjectMeta
+}
+
+func makeTestObject(name string) *testObject {
+ return &testObject{
+ ObjectMeta: meta.ObjectMeta{
+ Name: name,
+ Labels: builtinLabels(nil),
+ },
+ }
+}
+
+// testResource is a resource type used for testing. It simulates a target
+// (ie. k8s apiserver mock) that always acts nominally (all resources are
+// created, deleted as requested, and the state is consistent with requests).
type testResource struct {
// current is the simulated state of resources in the target.
- current map[string]string
+ current map[string]*testObject
// expected is what this type will report as the Expected() resources.
- expected map[string]string
+ expected map[string]*testObject
}
-func (r *testResource) List(ctx context.Context) ([]string, error) {
- var keys []string
- for k, _ := range r.current {
- keys = append(keys, k)
+func (r *testResource) List(ctx context.Context) ([]meta.Object, error) {
+ var cur []meta.Object
+ for _, v := range r.current {
+ v_copy := *v
+ cur = append(cur, &v_copy)
}
- return keys, nil
+ return cur, nil
}
-func (r *testResource) Create(ctx context.Context, el interface{}) error {
- r.current[el.(string)] = el.(string)
+func (r *testResource) Create(ctx context.Context, el meta.Object) error {
+ r.current[el.GetName()] = el.(*testObject)
return nil
}
@@ -123,41 +84,41 @@
return nil
}
-func (r *testResource) Expected() map[string]interface{} {
- exp := make(map[string]interface{})
- for k, v := range r.expected {
- exp[k] = v
+func (r *testResource) Expected() []meta.Object {
+ var exp []meta.Object
+ for _, v := range r.expected {
+ v_copy := *v
+ exp = append(exp, &v_copy)
}
return exp
}
-// newTestResource creates a test resource with a list of expected resource
-// strings.
-func newTestResource(want ...string) *testResource {
- expected := make(map[string]string)
+// newTestResource creates a test resource with a list of expected objects.
+func newTestResource(want ...*testObject) *testResource {
+ expected := make(map[string]*testObject)
for _, w := range want {
- expected[w] = w
+ expected[w.GetName()] = w
}
return &testResource{
- current: make(map[string]string),
+ current: make(map[string]*testObject),
expected: expected,
}
}
-// currentDiff returns a human-readable string showing the different between
-// the current state and the given resource strings. If no difference is
+// currentDiff returns a human-readable string showing the difference between
+// the current state and the given objects. If no difference is
// present, the returned string is empty.
-func (r *testResource) currentDiff(want ...string) string {
- expected := make(map[string]string)
+func (r *testResource) currentDiff(want ...*testObject) string {
+ expected := make(map[string]*testObject)
for _, w := range want {
- if _, ok := r.current[w]; !ok {
- return fmt.Sprintf("%q missing in current", w)
+ if _, ok := r.current[w.GetName()]; !ok {
+ return fmt.Sprintf("%q missing in current", w.GetName())
}
- expected[w] = w
+ expected[w.GetName()] = w
}
for _, g := range r.current {
- if _, ok := expected[g]; !ok {
- return fmt.Sprintf("%q spurious in current", g)
+ if _, ok := expected[g.GetName()]; !ok {
+ return fmt.Sprintf("%q spurious in current", g.GetName())
}
}
return ""
@@ -167,7 +128,7 @@
// a target state based on a set of expected resources.
func TestBasicReconciliation(t *testing.T) {
ctx := context.Background()
- r := newTestResource("foo", "bar", "baz")
+ r := newTestResource(makeTestObject("foo"), makeTestObject("bar"), makeTestObject("baz"))
// nothing should have happened yet (testing the test)
if diff := r.currentDiff(); diff != "" {
@@ -178,7 +139,7 @@
t.Fatalf("reconcile: %v", err)
}
// everything requested should have been created
- if diff := r.currentDiff("foo", "bar", "baz"); diff != "" {
+ if diff := r.currentDiff(makeTestObject("foo"), makeTestObject("bar"), makeTestObject("baz")); diff != "" {
t.Fatalf("wrong state after reconciliation: %s", diff)
}
@@ -186,8 +147,8 @@
if err := reconcile(ctx, r); err != nil {
t.Fatalf("reconcile: %v", err)
}
- // foo should not be missing
- if diff := r.currentDiff("bar", "baz"); diff != "" {
+ // foo should now be missing
+ if diff := r.currentDiff(makeTestObject("bar"), makeTestObject("baz")); diff != "" {
t.Fatalf("wrong state after deleting foo: %s", diff)
}
}