[WIP] Start building out shim layer for Tinkerbell #8

detiber · 2020-12-10T01:36:31Z

Description

Adds start of types and other scaffolding for a thinkerbell shim layer

detiber

Hardware type still needs to be built out.
Scaffolding for workflow and hardware controllers needs to be built out
Tinkerbell informers need to be setup for all types and plumbed through to trigger reconciliations of appropriate resources.
Add support/rbac permissions for creating k8s resources to mirror Tinkerbell resources when the k8s resources do not yet exist.

detiber · 2020-12-10T01:38:03Z

api/v1alpha3/doc.go

+limitations under the License.
+*/
+
+package v1alpha3


Added this mainly for completeness

detiber · 2020-12-10T01:38:22Z

go.mod

-	k8s.io/component-base v0.17.14
-	k8s.io/klog v1.0.0
-	k8s.io/utils v0.0.0-20201110183641-67b214c5f920
+	k8s.io/utils v0.0.0-20201110183641-67b214c5f920 // indirect


These changes came from go mod tidy

It should be committed separately I think, together with go.sum. And there should be a CI check for that.

Edit: #10

detiber · 2020-12-10T01:39:20Z

tink/api/v1alpha1/hardware_types.go

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


Didn't get far enough today to finish out the Hardware types

detiber · 2020-12-10T01:41:36Z

tink/api/v1alpha1/template_types.go

+const (
+	// TemplateIDAnnotation is used by the controller to store the
+	// ID assigned to the workflow by Tinkerbell.
+	TemplateIDAnnotation = "template.tinkerbell.org/id"


Originally I was planning on adding a Spec field for the ID that is stored in Tinkerbell, but then I thought it might be confusing for users, so I figured an annotation might work. We can also add it to the Status to make it more easily visible to the user, but I also wanted to make sure that it was stored in a persistent way for idempotency.

detiber · 2020-12-10T01:43:01Z

tink/controllers/template_controller.go

+	client.Client
+	Log      logr.Logger
+	Recorder record.EventRecorder
+	Scheme   *runtime.Scheme


TODO: need to plumb through a tinkerbell client

detiber · 2020-12-10T01:45:01Z

tink/controllers/template_controller.go

+	err := ErrNotImplemented
+
+	logger.Error(err, "Not yet implemented")
+
+	return ctrl.Result{}, err


TODO: use the tinkerbell client to reconcile resource

detiber · 2020-12-10T01:45:20Z

tink/controllers/template_controller.go

+	logger := r.Log.WithValues("template", template.Name)
+	err := ErrNotImplemented
+
+	logger.Error(err, "Not yet implemented")
+
+	// controllerutil.RemoveFinalizer(template, tinkv1alpha1.TemplateFinalizer)
+
+	return ctrl.Result{}, err


TODO: use the tinkerbell client to delete resource and remove finalizer

invidian

Would be nice to get this through checks added in #9.

Left some thoughts and questions.

invidian · 2020-12-10T07:01:40Z

go.mod

-	k8s.io/component-base v0.17.14
-	k8s.io/klog v1.0.0
-	k8s.io/utils v0.0.0-20201110183641-67b214c5f920
+	k8s.io/utils v0.0.0-20201110183641-67b214c5f920 // indirect


It should be committed separately I think, together with go.sum. And there should be a CI check for that.

Edit: #10

invidian · 2020-12-10T07:03:37Z

tink/api/v1alpha1/doc.go

+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+


Why do we need doc.go file if it contains no doc? 😉 Perhaps we should move the docs from groupversion_info.go.

We don't necessarily, but we've had issues with web-based tools like godoc in the past with the lack of doc.go files when someone tries to access docs specific to the api types and the project wasn't already indexed

Godoc require at least one file in the package to have documentation, it doesn't necessarily be doc.go file. Though when you have many files in the package, IMO it's good to have a separate file for docs, so no other files becomes "special".

TL;DR I suggest moving the Package v1alpha1 .. comment from groupversion_info.go to doc.go file.

invidian · 2020-12-10T07:04:54Z

tink/api/v1alpha1/hardware_types.go

+// HardwareIDAnnotation is used by the controller to store the
+// ID assigned to the workflow by Tinkerbell.


Suggested change

// HardwareIDAnnotation is used by the controller to store the

// ID assigned to the workflow by Tinkerbell.

// HardwareIDAnnotation is used by the controller to store the

// hardware ID assigned to the workflow by Tinkerbell.

Ah, copypasta issue. Should have been 'ID assigned to the hardware by Tinkerbell'

invidian · 2020-12-10T07:18:54Z

tink/controllers/template_controller.go

+		before := template.DeepCopy()
+		controllerutil.AddFinalizer(template, tinkv1alpha1.TemplateFinalizer)
+
+		if err := r.Client.Patch(ctx, template, client.MergeFrom(before)); err != nil {
+			logger.Error(err, "Failed to add finalizer to template")
+
+			return ctrl.Result{}, fmt.Errorf("failed to add finalizer: %w", err)
+		}
+
+		return ctrl.Result{}, nil


Seems to me that this all could go to separate function to keep Reconcile shorter.

invidian · 2020-12-10T07:24:47Z

tink/api/v1alpha1/hardware_types.go

+// +kubebuilder:resource:path=hardware,scope=Cluster,categories=tinkerbell
+// +kubebuilder:storageversion
+
+// Hardware is the Schema for the Hardware API.


If I understand correctly, those types will be exposed by Kubernetes API, so cluster API controller can create those instead of Tinkerbell structs directly.

I would like to know:

What is the benefit of this indirection? Other than visibility (ability to see Tinkerbell records using Kubernetes API). I guess also separation of concerns as creating Tinkerbell records is now independent from Cluster API.

If I understand correctly, Tinkerbell controllers could live as a separate project, to allow interacting with Tinkerbell via Kubernetes API. But for now they will live here? What if we have a separate controller in the future and we would like create entries for multiple Tinkerbell instances. Is this solution extensible for such use case? Perhaps having a separate controller per namespace would allow that?

If I understand correctly, those types will be exposed by Kubernetes API, so cluster API controller can create those instead of Tinkerbell structs directly.

I would like to know:

What is the benefit of this indirection? Other than visibility (ability to see Tinkerbell records using Kubernetes API). I guess also separation of concerns as creating Tinkerbell records is now independent from Cluster API.

It is indeed mostly a separation of concerns. This also aligns with some of the discussions that have taken place upstream about trying to leverage similar tooling for other providers, such as the AWS Controllers for Kubernetes as an implementation detail of cluster-api-provider-aws.

If I understand correctly, Tinkerbell controllers could live as a separate project, to allow interacting with Tinkerbell via Kubernetes API. But for now they will live here?

Yes, longer term I would expect this to move out to a separate project. There will need to be some thought put into how to reconcile with similar work for the tinkerbell crossplane provider to avoid too much unnecessary duplication.

What if we have a separate controller in the future and we would like create entries for multiple Tinkerbell instances. Is this solution extensible for such use case? Perhaps having a separate controller per namespace would allow that?

That is definitely something that we need to consider. I created the resources as cluster scoped with the idea of a 1-1 mapping between Cluster API and a Tinkerbell instance, but I think we have a few options available if we want to remove that assumption:

Make the resources namespaced, when the controller reconciles a resource it pulls the connection information for the related tinkerbell instance from a secret stored in the namespace

this would simplify mapping of names between tinkerbell and the controller, but would complicate the interaction between CAPT and the controllers

Have a cluster scoped resource that can be used for storing connection information for multiple tinkerbell instances

Each resource can optionally specify which connection information to use

If not specified, have some method of falling back to a default

Will need to figure out how to handle conflicting naming between k8s resources and tinkerbell resources across multiple tinkerbell instances

The latter approach is what is done by https://github.com/aws/aws-controllers-k8s and likely the cleaner approach, however they are not doing a sync back the other way where naming could conflict. We could potentially avoid the two way sync and instead opt for having a way to import specific pre-existing resources from tinkerbell instead.

It is indeed mostly a separation of concerns. This also aligns with some of the discussions that have taken place upstream about trying to leverage similar tooling for other providers, such as the AWS Controllers for Kubernetes as an implementation detail of cluster-api-provider-aws.

Make sense, thanks for confirmation and insights.

Yes, longer term I would expect this to move out to a separate project. There will need to be some thought put into how to reconcile with similar work for the tinkerbell crossplane provider to avoid too much unnecessary duplication.

I don't know about the crossplane provider, so I trust your judgement here then!

Make the resources namespaced, when the controller reconciles a resource it pulls the connection information for the related tinkerbell instance from a secret stored in the namespace
this would simplify mapping of names between tinkerbell and the controller, but would complicate the interaction between CAPT and the controllers

Yeah, namespacing seemed like an easy solution to me.

It seems to me that Tinkerbell is sort of special, as with cloud providers you usually have just one set of credentials and single API endpoint, while with Tinkerbell, you could have multiple.

There are similar issues that are being tracked for various cloud providers to support the idea of multiple accounts, which isn't too far removed from what we'd need for tinkerbell, for example here's the tracking issue for cluster-api-provider-aws: kubernetes-sigs/cluster-api-provider-aws#1552

Signed-off-by: Jason DeTiberus <[email protected]>

invidian · 2020-12-23T09:45:53Z

tink/api/v1alpha1/hardware_types.go

+// HardwareSpec defines the desired state of Hardware.
+type HardwareSpec struct {
+	// ID is the ID of the hardware in Tinkerbell
+	// +kubebuilder:validation:MinLength=1


This should actually have a fixed length I think as it's an UUID.

invidian · 2020-12-23T09:47:15Z

go.mod

-    	k8s.io/api v0.17.14
+	github.com/google/uuid v1.1.2
+	github.com/onsi/gomega v1.10.1
+	github.com/tinkerbell/tink v0.0.0-20201210163923-6d9159b63857


Perhaps we should use the same version as is used in sandbox repo right now: https://github.com/tinkerbell/sandbox/blob/master/current_versions.sh#L8.

Oh, I suppose this version does not have informers implemented yet 😞

invidian · 2020-12-23T09:53:58Z

tink/internal/client/common.go

+// ErrNotFound is returned if a requested resource is not found.
+var ErrNotFound = errors.New("resource not found")


Perhaps we could follow https://travix.io/errors-derived-from-constants-in-go-fda6748b4072, so we avoid having exported global variable and we can use errors.Is().

invidian · 2020-12-23T09:55:20Z

tink/internal/client/fake/hardware.go

+		f.Objs[in.Id] = proto.Clone(in).(*hardware.Hardware)
+
+		return nil
+	}
+
+	return errors.New("nobody home")


I'd swap the logic to keep the happy path on the left side instead of indented.

invidian · 2020-12-23T09:58:39Z

tink/internal/client/fake/hardware.go

+}
+
+// Get gets a Hardware from Tinkerbell.
+func (f *Hardware) Get(ctx context.Context, id, ip, mac string) (*hardware.Hardware, error) {


Can we have a struct here for selecting the hardware? Otherwise you get this ambigous calls:

f.Get(ctx, "", "", mac) f.Get(ctx, "", ip, "")

It seems to me that it's easy to mess up the order.

With struct, you would have something like:

f.Get(ctx, &fake.HardwareSelector{Mac: mac})

Also, is it desired to return first met hardware if selectors are contradicting?

invidian · 2020-12-23T11:02:16Z

tink/internal/controllers/hardware/controller.go

+	}
+
+	// Deletion is a noop.
+	if !hardware.DeletionTimestamp.IsZero() {


Perhaps we should log or emit event here?

invidian · 2020-12-23T11:03:06Z

tink/internal/controllers/hardware/controller.go

+		logger.Error(err, "Failed to get hardware from Tinkerbell")
+
+		return ctrl.Result{}, fmt.Errorf("failed to get hardware from Tinkerbell: %w", err)


Ditto. I think controller manager will log the returned error anyway.

invidian · 2020-12-23T11:04:24Z

tink/internal/controllers/template/controller.go

+	client.Client
+	TemplateClient templateClient
+	Log            logr.Logger
+	Scheme         *runtime.Scheme


Why is Scheme field needed here?

invidian · 2020-12-23T11:10:59Z

tink/internal/controllers/template/controller_internal_test.go

+				Scheme:         scheme,
+			}
+
+			got, err := r.reconcileDelete(context.Background(), tt.in)


I guess we could just call Reconcile here and avoid having this test as internal? As we set deletion timestamp anyway.

invidian · 2020-12-23T11:24:11Z

tink/internal/controllers/workflow/controller.go

+*/
+
+// Package controllers contains controllers for Tinkerbell.
+package controllers


Shouldn't this package be called workflow?

invidian · 2020-12-23T14:11:31Z

tink/internal/client_test/hardware_test.go

+	"github.com/tinkerbell/cluster-api-provider-tinkerbell/tink/internal/client"
+)
+
+func TestHardwareLifecycle(t *testing.T) {


I think it would be good to break down those super large tests, so we can give them a better name and make them more focused on what we actually test here.

invidian · 2020-12-23T14:11:34Z

tink/internal/client_test/hardware_test.go

+func TestHardwareLifecycle(t *testing.T) {
+	g := NewWithT(t)
+	ctx := context.Background()
+	hardwareClient := client.NewHardwareClient(realHardwareClient(t))


Hm, we have a fake client for Tinkerbell, I guess we can use it here so we don't require real tinkerbell server?

BTW, if we want tests with Tinkerbell server, we can probably borrow the setup from https://github.com/tinkerbell/terraform-provider-tinkerbell/tree/master/test.

invidian · 2020-12-23T14:13:11Z

tink/internal/client_test/hardware_test.go

@@ -0,0 +1,123 @@
+/*


I wonder, why tink/internal for directory structure and not internal/tink?

Also, maybe tinkerbell instead of tink?

invidian · 2020-12-23T14:27:00Z

tink/internal/controllers/hardware/controller.go

+)
+
+type hardwareClient interface {
+	// Create(ctx context.Context, h *hardware.Hardware) error


Being able to push hardware to Tinkerbell via K8s API would be very useful for testing the CAPT controllers, as then you can push some test hardware via fake client-go.

Without that, one will need to push the hardware to Tinkerbell fake client directly I think.

invidian · 2021-01-05T13:30:56Z

main.go

+	templateClient := client.NewTemplateClient(tinkclient.TemplateClient)
+	workflowClient := client.NewWorkflowClient(tinkclient.WorkflowClient, hwClient)
+
+	stopCh := ctrl.SetupSignalHandler()

 	if webhookPort == 0 {


We should flip the logic here and remove the else, as this no longer make sense.

Actually addressed in #24. Hope it gets merged before this PR.

invidian · 2021-01-05T13:32:24Z

main.go

+		templateChan := make(chan event.GenericEvent)
+		workflowChan := make(chan event.GenericEvent)
+
+		if err := mgr.Add(&sources.TinkEventWatcher{


Maybe we can add those in the loop to deduplicate the exit logic? I'd also move it to separate function to avoid further extension of main() function, as it should be as minimal as possible.

invidian · 2021-01-05T13:48:14Z

go.mod

-    	k8s.io/api v0.17.14
+	github.com/google/uuid v1.1.2
+	github.com/onsi/gomega v1.10.1
+	github.com/tinkerbell/tink v0.0.0-20201210163923-6d9159b63857


Oh, I suppose this version does not have informers implemented yet 😞

invidian · 2021-01-05T14:20:34Z

main.go

@@ -104,7 +116,7 @@ func main() {

 	flag.Parse()

-	ctrl.SetLogger(zap.New(zap.UseDevMode(true)))


Why is this needed?

Included committed explicitly in #24. I guess klog is standard logger we should use.

invidian · 2021-01-07T10:03:46Z

tink/controllers/workflow/controller.go

+		}
+	}()
+
+	w.Status.Data = tinkWorkflow.GetData()


Hm, I guess this is content of rendered workflow, right? If so, it may contain secrets. And it will contain secrets when we use it for CAPT.

Should we then create a secret for each workflow and store the data there? This actually also applies to Templates and hardware selectors :/

invidian · 2021-01-08T14:25:23Z

tink/controllers/template/controller_internal_test.go

+	"testing"
+
+	. "github.com/onsi/gomega"
+	tinkv1alpha1 "github.com/tinkerbell/cluster-api-provider-tinkerbell/tink/api/v1alpha1"


Module-scoped imports should be separate from 3rd party modules.

detiber · 2021-02-16T20:55:39Z

Closed in favor of #32

invidian · 2021-02-16T20:59:23Z

@detiber so we close it? :) Has my feedback been addressed in #32 ?

detiber · 2021-02-16T21:14:55Z

@invidian yes, I had intended to close and hit the wrong button. I've created #34 to track making sure feedback on this PR is addressed.

detiber force-pushed the tinkshim branch from f6e867b to 53d950f Compare December 10, 2020 01:36

detiber assigned gianarb and invidian Dec 10, 2020

detiber commented Dec 10, 2020

View reviewed changes

invidian suggested changes Dec 10, 2020

View reviewed changes

detiber force-pushed the tinkshim branch from 53d950f to 74f5b3d Compare December 11, 2020 22:37

detiber mentioned this pull request Dec 21, 2020

CAPT (Cluster API for Tinkerbell) #1

Closed

detiber added 11 commits December 21, 2020 17:37

Start building out shim layer for Tinkerbell

5e04d71

Signed-off-by: Jason DeTiberus <[email protected]>

updates to templates

cfb3e70

Start building out template controller

24842b1

review feedback

c9ae7d6

Continue building out Tink types and controllers

38dd4d7

Refactor the shim a bit

651ee32

current state

4bffdc1

latest changes

fe98699

Start wiring up controllers and event handlers

dab620e

update generated code

303da8c

Add tests for tink informers, update statuses

1746f69

invidian suggested changes Dec 23, 2020

View reviewed changes

invidian reviewed Dec 23, 2020

View reviewed changes

detiber added 3 commits December 23, 2020 10:45

fixes

a40f06d

fixes/cleanup

9126feb

force requeueafter when workflow state is not success

923ef04

detiber force-pushed the tinkshim branch from b64fa46 to 923ef04 Compare December 23, 2020 21:30

de-lint

c7fa75d

invidian mentioned this pull request Jan 6, 2021

main.go fixes and improvements #24

Merged

3 tasks

invidian reviewed Jan 6, 2021

View reviewed changes

This was referenced Jan 6, 2021

Add controllers implementation #17

Closed

Figure out checking if given hardware ID is available to use #15

Closed

invidian reviewed Jan 7, 2021

View reviewed changes

invidian reviewed Jan 8, 2021

View reviewed changes

detiber mentioned this pull request Feb 16, 2021

Add controllers implementation #32

Merged

detiber mentioned this pull request Feb 16, 2021

Address feedback from https://github.com/tinkerbell/cluster-api-provider-tink/pull/8 #34

Open

detiber closed this Feb 16, 2021

gauravgahlot mentioned this pull request Mar 5, 2021

Group module imports #46

Merged

gauravgahlot mentioned this pull request Mar 23, 2021

Use fake clients in client_test #50

Closed

3 tasks

chrisdoherty4 deleted the tinkshim branch March 11, 2024 22:34

		// HardwareIDAnnotation is used by the controller to store the
		// ID assigned to the workflow by Tinkerbell.

		// ErrNotFound is returned if a requested resource is not found.
		var ErrNotFound = errors.New("resource not found")

		logger.Error(err, "Failed to get hardware from Tinkerbell")

		return ctrl.Result{}, fmt.Errorf("failed to get hardware from Tinkerbell: %w", err)

		@@ -104,7 +116,7 @@ func main() {

		flag.Parse()

		ctrl.SetLogger(zap.New(zap.UseDevMode(true)))

[WIP] Start building out shim layer for Tinkerbell #8

[WIP] Start building out shim layer for Tinkerbell #8

Conversation

detiber commented Dec 10, 2020

Description

detiber 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

invidian 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

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

Choose a reason for hiding this comment

detiber commented Feb 16, 2021

invidian commented Feb 16, 2021

detiber commented Feb 16, 2021