Added Kubernetes API types #542

micahhausler · 2021-09-29T15:44:19Z

Signed-off-by: Micah Hausler [email protected]

Description

This is an initial commit to support the Kubernetes resource model from tinkerbell/proposals#46. I'm starting out with just the API schema, and there are a lot of fields on the Hardware type that I do not understand. I'd like to get some consensus on the API shape and fields, but this is iterative, and structures can be modified in later PRs.

After this is merged, I'll have additional follow on PRs for

Tink Controller
Validating webhook
DB/GRPC server changes behind a feature flag
Migration tooling from Postgres

How Has This Been Tested?

Added CI test for CRD generation

How are existing users impacted? What migration steps/scripts do we need?

No user change required at this point

Checklist:

I have:

updated the documentation and/or roadmap (if required)
added unit or e2e tests
provided instructions on how to upgrade

scripts/go_install.sh

micahhausler

Left reviewer comments

micahhausler · 2021-09-29T15:46:06Z

go.mod

 )
-
-replace github.com/stormcat24/protodep => github.com/ackintosh/protodep v0.0.0-20200728152107-abf8eb579d6c


@mmlb You touched this last, but I could not get Tink to build without removing this. Is this OK?

micahhausler · 2021-09-29T15:46:57Z

kube.mk

@@ -0,0 +1,36 @@
+GO_INSTALL = ./scripts/go_install.sh


I dropped the K8s Make changes in its own file, does anyone have a place they'd rather see this?

I really like that you compartmentalize this. 📦

micahhausler · 2021-09-29T15:48:58Z

pkg/apis/core/v1alpha1/groupversion_info.go

+//nolint:gochecknoglobals
+var (
+	// GroupVersion is group version used to register these objects.
+	GroupVersion = schema.GroupVersion{Group: "tinkerbell.org", Version: "v1alpha1"}


@detiber I copied the CAPT API group, so this can't be installed side-by-side with CAPT. Is that OK for now? I think CAPT could migrate to these types once API functionality is in place

@micahhausler IMO this is fine, because I would image CAPT to be part of the kube-native Tinkerbell.

micahhausler · 2021-09-29T15:55:09Z

pkg/apis/core/v1alpha1/hardware_types.go

+}
+
+// HardwareSpec defines the desired state of Hardware.
+type HardwareSpec struct {


I largely copied the Spec from the existing Hardware Protobuf and injected the Packet Metadata proto structure into the Metadata field.

The Metadata structure seemed necessary to get Boots to play nice. It seems to duplicate some of the top level hardware spec fields. Is this just a carryover from internal Packet/Equanix code? How much of that do we really need? If the easiest answer is just "include it all for now and refactor later" I'm fine with that, but I'd appreciate someone weighing in

@micahhausler I like that you kept it rather open and my humble opinion is that for this PR it is fine. Later we might want to look at how to enforce hardware data that is compatible with rootio.

One possible point of friction that I see between Kubernetes and Tinkerbell is the overlap between the word metadata. Maybe it could be sensible to consider the entire hardware definition metadata and thus adjust the schema described here in our Tinkerbell docs in the future?

But I think I am sidetracking big time. I can set up a proposal if anyone seconds this.

micahhausler · 2021-09-29T15:57:26Z

pkg/apis/core/v1alpha1/workflow_data_types.go

+// +kubebuilder:storageversion
+
+// Workflow is the Schema for the Workflows API.
+type WorkflowData struct {


This type is meant to replicate the behavior of the WorkflowData RPCs.

micahhausler · 2021-09-29T15:59:45Z

pkg/apis/core/v1alpha1/workflow_types.go

+	TemplateRef string `json:"templateRef,omitempty"`
+
+	// A mapping of template devices to hadware mac addresses
+	HardwareMap map[string]string `json:"hardwareMap,omitempty"`


Same gyration about naming here. The type is kind of implied, any strong opinions on the current name vs hardware?

I am thinking that maybe HardwareBinding is more semantic? map tickles me in that it is very close to Go lingo.

micahhausler · 2021-09-29T16:01:04Z

pkg/apis/core/v1alpha1/workflow_types.go

+// WorkflowStatus defines the observed state of Workflow.
+type WorkflowStatus struct {
+	// State is the state of the workflow in Tinkerbell.
+	State string `json:"state,omitempty"`


We can add a WorkflowState type here with values for the State Protobuf enum later.

micahhausler · 2021-09-29T16:01:55Z

pkg/apis/core/v1alpha1/workflow_types.go

+	State string `json:"state,omitempty"`
+
+	// Data is the populated Workflow Data in Tinkerbell.
+	Data string `json:"data,omitempty"`


Should we drop this? Its just the evaluated, interpolated template with the hardware mappings. This is all encoded and strongly typed in the Tasks Task[] field below

I would second the idea of dropping this as this feels like noise to me.

micahhausler · 2021-09-29T16:05:06Z

pkg/apis/core/v1alpha1/workflow_types.go

+	GlobalTimeout int64 `json:"globalTimeout,omitempty"`
+
+	// Tasks are the tasks to be completed
+	Tasks []Task `json:"tasks,omitempty"`


I found it was easiest to translate the interpolated template steps by just using the same structure of Tasks[].Actions[]. This is a little bit of a departure from the data model used in Postgres, but since we're normalizing everything on the Workflow, this structure was the easiest to reason about for rewriting the worker APIs.

micahhausler · 2021-09-29T16:05:36Z

pkg/apis/core/v1alpha1/workflow_types.go

+	Volumes     []string          `json:"volumes,omitempty"`
+	Pid         string            `json:"pid,omitempty"`
+	Environment map[string]string `json:"environment,omitempty"`
+	Status      string            `json:"status,omitempty"`


This field could also be transitioned to the WorkflowStatus enums

If I understand the concept of actions correctly, I'd second your suggestion of moving it into the WorkflowStatus. The risk I see is otherwise that spec (desired state) and status (current state) get mixed, which I feel goes against Kubernetes' declarative nature.

thebsdbox · 2021-09-30T05:40:45Z

Woah! This looks excellent!

codecov · 2021-10-04T19:56:52Z

Codecov Report

Merging #542 (3121ba6) into main (187a3c7) will not change coverage.
The diff coverage is n/a.

❗ Current head 3121ba6 differs from pull request most recent head fbfa9b3. Consider uploading reports for the commit fbfa9b3 to get more accurate results

@@           Coverage Diff           @@
##             main     #542   +/-   ##
=======================================
  Coverage   34.76%   34.76%           
=======================================
  Files          44       44           
  Lines        3348     3348           
=======================================
  Hits         1164     1164           
  Misses       2085     2085           
  Partials       99       99

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 187a3c7...fbfa9b3. Read the comment docs.

jacobweinstock · 2021-10-06T15:53:49Z

Hey @tstromberg and @detiber, would either of you be able to help with this PR? It's big and I wanted to make sure it got some good eyes on it.

nicklasfrahm · 2021-10-06T13:02:17Z

pkg/apis/core/v1alpha1/hardware_types.go

+}
+
+// HardwareSpec defines the desired state of Hardware.
+type HardwareSpec struct {


@micahhausler I like that you kept it rather open and my humble opinion is that for this PR it is fine. Later we might want to look at how to enforce hardware data that is compatible with rootio.

One possible point of friction that I see between Kubernetes and Tinkerbell is the overlap between the word metadata. Maybe it could be sensible to consider the entire hardware definition metadata and thus adjust the schema described here in our Tinkerbell docs in the future?

But I think I am sidetracking big time. I can set up a proposal if anyone seconds this.

nicklasfrahm · 2021-10-06T13:06:02Z

pkg/apis/core/v1alpha1/groupversion_info.go

+//nolint:gochecknoglobals
+var (
+	// GroupVersion is group version used to register these objects.
+	GroupVersion = schema.GroupVersion{Group: "tinkerbell.org", Version: "v1alpha1"}


@micahhausler IMO this is fine, because I would image CAPT to be part of the kube-native Tinkerbell.

nicklasfrahm · 2021-10-06T13:18:43Z

pkg/apis/core/v1alpha1/workflow_types.go

+	TemplateRef string `json:"templateRef,omitempty"`
+
+	// A mapping of template devices to hadware mac addresses
+	HardwareMap map[string]string `json:"hardwareMap,omitempty"`


I am thinking that maybe HardwareBinding is more semantic? map tickles me in that it is very close to Go lingo.

nicklasfrahm · 2021-10-06T13:21:03Z

pkg/apis/core/v1alpha1/workflow_types.go

+	State string `json:"state,omitempty"`
+
+	// Data is the populated Workflow Data in Tinkerbell.
+	Data string `json:"data,omitempty"`


I would second the idea of dropping this as this feels like noise to me.

nicklasfrahm · 2021-10-06T13:26:10Z

pkg/apis/core/v1alpha1/workflow_types.go

+	Volumes     []string          `json:"volumes,omitempty"`
+	Pid         string            `json:"pid,omitempty"`
+	Environment map[string]string `json:"environment,omitempty"`
+	Status      string            `json:"status,omitempty"`


If I understand the concept of actions correctly, I'd second your suggestion of moving it into the WorkflowStatus. The risk I see is otherwise that spec (desired state) and status (current state) get mixed, which I feel goes against Kubernetes' declarative nature.

nicklasfrahm · 2021-10-06T13:28:03Z

kube.mk

@@ -0,0 +1,36 @@
+GO_INSTALL = ./scripts/go_install.sh


I really like that you compartmentalize this. 📦

jacobweinstock · 2021-11-02T15:40:33Z

.github/workflows/ci.yaml

@@ -26,6 +26,8 @@ jobs:
        uses: actions/setup-go@v2
        with:
          go-version: "1.14.6"
+      - name: Generate
+        run: make generate


is the idea there that we just validate that the generate works? Will we need this to run below so that this is available in crosscompile? maybe I just don't understand how github actions work?

Yes, we want to make sure that our generation tooling works, and the checked-in generated code is up-to-date. It runs before ci-checks.md, so that any differences in generated code are marked as a CI failure.

jacobweinstock · 2021-11-02T15:42:16Z

Approved on my end. Looks like there are just a few conflicts to address.

Signed-off-by: Micah Hausler <[email protected]>

micahhausler · 2021-11-15T17:42:01Z

@jacobweinstock Addressed comments, and fixed conflicts.

thebsdbox · 2021-11-19T14:19:55Z

Are we good to pull the trigger on this one?

Signed-off-by: Micah Hausler <[email protected]> ## Description This PR adds a new Kubernetes controller binary for the types merged in #542. * Adds a new `cmd/tink-controller` package with a Dockerfile and a `main.go` * Removes the `status.data` field on Workflows. This was just a string of the interpolated template, and was redundant given the strongly typed template actions already on the `.status` of Workflows. * Adds tests to the methods on the workflow API structure * Adds a new `pkg/controllers` package. This introduces a `Manager` abstraction that wraps `sigs.k8s.io/controller-runtime`'s [`Reconciler`](https://pkg.go.dev/sigs.k8s.io/[email protected]/pkg/reconcile?utm_source=gopls#Reconciler) type * Controllers for individual types (workflows, templates, hardware) will reside in individual sub-packages * Controllers patch incoming objects even in the event of an error: this is intentional as a controller in the [Kubernetes Resource Model](https://github.com/kubernetes/design-proposals-archive/blob/main/architecture/resource-management.md) attempts to converge on desired state * Adds a new `pkg/conversion` package for converting protobuf types to Kubernetes CRD types and vice versa * Adds an internal `pkg/internal/tests` package with internal testing utilities. * The first utility added is a `FrozenTime` interface with Before/After connivence methods for `time.Time`, as well as protobuf and K8s metav1 package time formats. ## How Has This Been Tested? * I added unit tests for nearly all new branching logic * I added a CI target for building the tink-controller ## How are existing users impacted? What migration steps/scripts do we need? There is no impact to existing users. Nothing form this change is called in any existing code path. ## Checklist: I have: - [ ] ~~updated the documentation and/or roadmap (if required)~~ - [x] added unit or e2e tests - [ ] ~~provided instructions on how to upgrade~~

sonatype-lift bot reviewed Sep 29, 2021

View reviewed changes

scripts/go_install.sh Show resolved Hide resolved

micahhausler commented Sep 29, 2021

View reviewed changes

micahhausler force-pushed the k8s-api branch from 15ed046 to bf95554 Compare September 29, 2021 18:30

micahhausler mentioned this pull request Oct 1, 2021

[Organization/member]: request for micahhausler tinkerbell/.github#35

Closed

7 tasks

micahhausler force-pushed the k8s-api branch 2 times, most recently from b30ec79 to 77314c4 Compare October 5, 2021 13:49

jacobweinstock requested review from tstromberg and detiber October 6, 2021 15:51

jacobweinstock self-requested a review October 6, 2021 15:54

micahhausler force-pushed the k8s-api branch 2 times, most recently from 22b8225 to 207b42e Compare October 6, 2021 18:34

This was referenced Oct 8, 2021

Fix all the dynamically generated code #547

Merged

Add support for the Kubernetes Resource Model tinkerbell/proposals#46

Merged

nicklasfrahm suggested changes Oct 9, 2021

View reviewed changes

jacobweinstock previously approved these changes Nov 2, 2021

View reviewed changes

Added Kubernetes API types

97596ba

Signed-off-by: Micah Hausler <[email protected]>

micahhausler dismissed jacobweinstock’s stale review via c76fadf November 15, 2021 16:56

micahhausler force-pushed the k8s-api branch from 207b42e to c76fadf Compare November 15, 2021 16:56

Make nixfmt happy

fbfa9b3

Signed-off-by: Micah Hausler <[email protected]>

micahhausler force-pushed the k8s-api branch from c76fadf to fbfa9b3 Compare November 15, 2021 17:20

jacobweinstock approved these changes Nov 15, 2021

View reviewed changes

jacobweinstock merged commit 4900279 into tinkerbell:main Nov 24, 2021

micahhausler mentioned this pull request Dec 2, 2021

Add Kubernetes Controller #563

Merged

3 tasks

displague added this to the 0.7.0 milestone Aug 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Kubernetes API types #542

Added Kubernetes API types #542

micahhausler commented Sep 29, 2021

micahhausler left a comment

micahhausler Sep 29, 2021

micahhausler Sep 29, 2021

nicklasfrahm Oct 6, 2021

micahhausler Sep 29, 2021

nicklasfrahm Oct 6, 2021

micahhausler Sep 29, 2021

nicklasfrahm Oct 6, 2021

micahhausler Sep 29, 2021

micahhausler Sep 29, 2021

nicklasfrahm Oct 6, 2021

micahhausler Sep 29, 2021

micahhausler Sep 29, 2021

nicklasfrahm Oct 6, 2021

micahhausler Sep 29, 2021

micahhausler Sep 29, 2021

nicklasfrahm Oct 6, 2021

thebsdbox commented Sep 30, 2021

codecov bot commented Oct 4, 2021 •

edited

Loading

jacobweinstock commented Oct 6, 2021

nicklasfrahm Oct 6, 2021

nicklasfrahm Oct 6, 2021

nicklasfrahm Oct 6, 2021

nicklasfrahm Oct 6, 2021

nicklasfrahm Oct 6, 2021

nicklasfrahm Oct 6, 2021

jacobweinstock Nov 2, 2021

micahhausler Nov 15, 2021

jacobweinstock commented Nov 2, 2021

micahhausler commented Nov 15, 2021

thebsdbox commented Nov 19, 2021

		)

		replace github.com/stormcat24/protodep => github.com/ackintosh/protodep v0.0.0-20200728152107-abf8eb579d6c

Added Kubernetes API types #542

Added Kubernetes API types #542

Conversation

micahhausler commented Sep 29, 2021

Description

How Has This Been Tested?

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

micahhausler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thebsdbox commented Sep 30, 2021

codecov bot commented Oct 4, 2021 • edited Loading

Codecov Report

jacobweinstock commented Oct 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobweinstock commented Nov 2, 2021

micahhausler commented Nov 15, 2021

thebsdbox commented Nov 19, 2021

codecov bot commented Oct 4, 2021 •

edited

Loading