Add Interface Wireguard Support #153

DawoudMahmud · 2023-05-10T22:44:07Z

Closes #137

This resource only exists for RouterOS v7+ so logic was included to exclude it for RouterOS v6 builds.

ddelnano

@DawoudMahmud nice work on this! This is a pretty large code change and involves some challenges that we haven't tackled in other parts of this provider yet (sensitive properties, etc), so don't be discouraged by the number of comments :)

ddelnano · 2023-05-11T05:15:42Z

go.mod

@@ -5,8 +5,8 @@ go 1.18
 require (
 	github.com/ddelnano/terraform-provider-mikrotik/client v0.0.0-00010101000000-000000000000
 	github.com/hashicorp/terraform-plugin-docs v0.13.0
-	github.com/hashicorp/terraform-plugin-framework v1.0.1
-	github.com/hashicorp/terraform-plugin-go v0.14.2
+	github.com/hashicorp/terraform-plugin-framework v1.2.0


Upgrading these dependencies are not needed for adding the new resources as far as I can tell, so they should be done in another PR (if needed). Please let me know if you have trouble preventing the go toolchain from making these changes automatically and exclude the go.sum and go.mod changes from this PR.

After reviewing this and running the tests locally, I realized this was necessary for the changes you made. Because our existing code has significant acceptance test coverage, upgrading this in place is fine. However, with most projects bumping the core sdk can be a large change that is usually best done prior to the dependent change (adding the resource functionality that needs it).

ddelnano · 2023-05-11T05:21:30Z

client/interface_wireguard.go

+	ListenPort int    `mikrotik:"listen-port"`
+	Mtu        int    `mikrotik:"mtu"`
+	PrivateKey string `mikrotik:"private-key"`
+	PublicKey  string `mikrotik:"public-key"`       //read only property


It seems the readonly mikrotik struct tag is missing here. When @jdelnano implemented this, it seems that was to avoid a situation where our client interface would try to change properties that are readonly. Any idea why this isn't causing an issue with the current test?

Ah good catch, I'll add the readonly tag to public key as well. I believe the test was passing because PublicKey defaults to an empty string so it didn't show up when adding a new interface wireguard. However since Running defaults to false, the test was complaining that I was trying to create an interface wireguard with a parameter that shouldn't exist (below).

ddelnano · 2023-05-11T05:26:37Z

client/interface_wireguard_test.go

+	err = c.Delete(found.(Resource))
+	if err != nil {
+		t.Errorf("expected no error, got %v", err)
+	}
+
+	_, err = c.Find(findInterface)
+	if err == nil {
+		t.Errorf("expected error, got nothing")
+		return
+	}
+
+	target := &NotFound{}
+	if !errors.As(err, &target) {
+		t.Errorf("expected error to be of type %T, got %T", &NotFound{}, err)
+	}


Let's wrap this in a defer right after the .Add() call is deemed successful (on line 40). See this client test as an example.

We are a bit inconsistent with how to structure these client tests, but if this assertion fails we should still trigger the delete logic (which the defer will take care of).

Makes sense, I updated the defer to mirror the style of the Bridge client test. Let me know if you have any issues with this approach.

ddelnano · 2023-05-11T05:49:44Z

mikrotik/resource_interface_wireguard.go

+			"private_key": schema.StringAttribute{
+				Optional:    true,
+				Computed:    true,
+				Description: "A base64 private key. If not specified, it will be automatically generated upon interface creation.",
+			},


This private key is important to keep secret. Terraform has a concept of sensitive fields, and I think this needs to be one. Here is the godoc link showing what attributes are available for the StringAttribute struct.

Please update this property to be sensitive and ideally our acceptance test would validate this is in some way as well.

In addition to this all of the provider logging (such as this) will be print this value. We likely need to introduce a way to hide fields like this. One possibility could be adding a sensitive struct tag similarly to how the readonly tag works (added in #143).

Added sensitive attribute to PrivateKey but still need to create a sensitive struct tag

mikrotik/resource_interface_wireguard.go

ddelnano · 2023-05-11T06:43:34Z

mikrotik/resource_interface_wireguard.go

+// Schema defines the schema for the resource.
+func (i *interfaceWireguard) Schema(_ context.Context, _ resource.SchemaRequest, resp *resource.SchemaResponse) {
+	resp.Schema = schema.Schema{
+		Description: "Creates a Mikrotik interface wireguard.",


Add the explanation about Router OS v7+ here. This will be populated in the docs written by tfplugindocs.

Do I just add "only supported by RouterOS v7+" at the end of the description or is there a different way to add it to let tfplugindocs populate it in the docs? I'm not too familiar with tfplugindocs.

To circle back on this, I've generated the doc file with tfplugindocs

ddelnano · 2023-05-11T06:44:16Z

docs/resources/interface_wireguard.md

+# mikrotik_interface_wireguard (Resource)
+Creates a Mikrotik interface_wireguard.
+
+!> This resource is supported for RouterOS v7+.


It looks like these docs were edited by hand. They must be generated with the tfplugindocs tool. See my comment in the mikrotik/resource_interface_wireguard.go file on how to add the RouterOS v7 information to the autogenerated file.

If the default resource templating isn't appropriate, you can create resource specific templates as well (like the bgp_peer resource).

ddelnano · 2023-05-12T22:20:38Z

client/interface_wireguard_test.go

+	defer func() {
+		err = c.Delete(found.(Resource))
+		if err != nil {
+			t.Errorf("expected no error, got %v", err)
+		}

-	_, err = c.Find(findInterface)
-	if err == nil {
-		t.Errorf("expected error, got nothing")
-		return
-	}
+		_, err = c.Find(findInterface)
+		if err == nil {
+			t.Errorf("expected error, got nothing")
+			return
+		}

-	target := &NotFound{}
-	if !errors.As(err, &target) {
-		t.Errorf("expected error to be of type %T, got %T", &NotFound{}, err)
-	}
+		target := &NotFound{}
+		if !errors.As(err, &target) {
+			t.Errorf("expected error to be of type %T, got %T", &NotFound{}, err)
+		}
+	}()


I'm pretty confident the placement of the defer matters. For example, an error from line 43 will cause the defer to get skipped completely.

We should place the defer right after the creation happens to ensure it is registered after the creation completes.

Here's an example that showcases this.

ddelnano · 2023-05-24T03:32:36Z

This PR was completed in #159. An earlier change was merged that introduced a conflict to the go.{mod,sum} files and #159 addressed that rebase.

Thanks @DawoudMahmud for the contribution! WireGuard support is a great addition considering its growing popularity!

DawoudMahmud added 5 commits March 30, 2023 16:40

Add interface_wireguard client code

90090ff

Add provider side code

e931905

Add markdown file

62d7d6d

Edit markdown file

dc12c8f

Merge branch 'master' of https://github.com/ddelnano/terraform-provid…

30d259c

…er-mikrotik

DawoudMahmud force-pushed the interface-wireguard branch from f191db9 to f878e87 Compare May 10, 2023 23:33

Merge branch 'master' into interface-wireguard

70fdd97

DawoudMahmud force-pushed the interface-wireguard branch from f878e87 to 70fdd97 Compare May 10, 2023 23:39

DawoudMahmud changed the title ~~Interface wireguard~~ Add Interface Wireguard Support May 10, 2023

jdelnano requested review from ddelnano and jdelnano May 10, 2023 23:40

ddelnano reviewed May 11, 2023

View reviewed changes

Fix first round of review comments

6e3f449

ddelnano reviewed May 12, 2023

View reviewed changes

ddelnano mentioned this pull request May 16, 2023

Code generation #150

Merged

1 task

DawoudMahmud added 2 commits May 17, 2023 17:12

Correct defer() placement

1c8793c

Fix defer function and md file

bc5aef0

DawoudMahmud requested a review from ddelnano May 23, 2023 00:26

ddelnano mentioned this pull request May 24, 2023

Add Wireguard Interface resource (continuation) #159

Merged

ddelnano closed this May 24, 2023

ddelnano added the enhancement New feature or request label May 24, 2023

jdelnano mentioned this pull request May 24, 2023

Create mikrotik_interface_wireguard resource #137

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Interface Wireguard Support #153

Add Interface Wireguard Support #153

DawoudMahmud commented May 10, 2023 •

edited

Loading

ddelnano left a comment

ddelnano May 11, 2023

ddelnano May 11, 2023

ddelnano May 11, 2023

DawoudMahmud May 12, 2023

ddelnano May 11, 2023

DawoudMahmud May 23, 2023

ddelnano May 11, 2023

DawoudMahmud May 12, 2023 •

edited

Loading

ddelnano May 11, 2023

DawoudMahmud May 12, 2023

DawoudMahmud May 23, 2023

ddelnano May 11, 2023

ddelnano May 11, 2023

ddelnano May 12, 2023

ddelnano May 13, 2023

ddelnano commented May 24, 2023

Add Interface Wireguard Support #153

Add Interface Wireguard Support #153

Conversation

DawoudMahmud commented May 10, 2023 • edited Loading

ddelnano 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

DawoudMahmud May 12, 2023 • edited Loading

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

ddelnano commented May 24, 2023

DawoudMahmud commented May 10, 2023 •

edited

Loading

DawoudMahmud May 12, 2023 •

edited

Loading