migrate bgp_instance framework to Plugin Framework (ddelnano#188)

* migrate bgp_instance framework to Plugin Framework

* fix import test

* add ErrorHandler() interface

* implement ErrorHandler interface to catch error on newer RouterOS versions

client/bgp_instance.go

@@ -23,24 +23,24 @@ func legacyBgpUnsupported(err error) bool {
 
 // BgpInstance Mikrotik resource
 type BgpInstance struct {
-	Id                       string `mikrotik:".id"`
-	Name                     string `mikrotik:"name"`
-	As                       int    `mikrotik:"as"`
-	ClientToClientReflection bool   `mikrotik:"client-to-client-reflection"`
-	Comment                  string `mikrotik:"comment"`
-	ConfederationPeers       string `mikrotik:"confederation-peers"`
-	Disabled                 bool   `mikrotik:"disabled"`
-	IgnoreAsPathLen          bool   `mikrotik:"ignore-as-path-len"`
-	OutFilter                string `mikrotik:"out-filter"`
-	RedistributeConnected    bool   `mikrotik:"redistribute-connected"`
-	RedistributeOspf         bool   `mikrotik:"redistribute-ospf"`
-	RedistributeOtherBgp     bool   `mikrotik:"redistribute-other-bgp"`
-	RedistributeRip          bool   `mikrotik:"redistribute-rip"`
-	RedistributeStatic       bool   `mikrotik:"redistribute-static"`
-	RouterID                 string `mikrotik:"router-id"`
-	RoutingTable             string `mikrotik:"routing-table"`
-	ClusterID                string `mikrotik:"cluster-id"`
-	Confederation            int    `mikrotik:"confederation"`
+	Id                       string `mikrotik:".id" codegen:"id,mikrotikID"`
+	Name                     string `mikrotik:"name" codegen:"name,required,terraformID"`
+	As                       int    `mikrotik:"as" codegen:"as,required"`
+	ClientToClientReflection bool   `mikrotik:"client-to-client-reflection" codegen:"client_to_client_reflection"`
+	Comment                  string `mikrotik:"comment" codegen:"comment"`
+	ConfederationPeers       string `mikrotik:"confederation-peers" codegen:"confederation_peers"`
+	Disabled                 bool   `mikrotik:"disabled" codegen:"disabled"`
+	IgnoreAsPathLen          bool   `mikrotik:"ignore-as-path-len" codegen:"ignore_as_path_len"`
+	OutFilter                string `mikrotik:"out-filter" codegen:"out_filter"`
+	RedistributeConnected    bool   `mikrotik:"redistribute-connected" codegen:"redistribute_connected"`
+	RedistributeOspf         bool   `mikrotik:"redistribute-ospf" codegen:"redistribute_ospf"`
+	RedistributeOtherBgp     bool   `mikrotik:"redistribute-other-bgp" codegen:"redistribute_other_bgp"`
+	RedistributeRip          bool   `mikrotik:"redistribute-rip" codegen:"redistribute_rip"`
+	RedistributeStatic       bool   `mikrotik:"redistribute-static" codegen:"redistribute_static"`
+	RouterID                 string `mikrotik:"router-id" codegen:"router_id,required"`
+	RoutingTable             string `mikrotik:"routing-table" codegen:"routing_table"`
+	ClusterID                string `mikrotik:"cluster-id" codegen:"cluster_id"`
+	Confederation            int    `mikrotik:"confederation" codegen:"confederation"`
 }
 
 var _ Resource = (*BgpInstance)(nil)
@@ -86,12 +86,19 @@ func (b *BgpInstance) DeleteFieldValue() string {
 	return b.Name
 }
 
+// HandleError intercepts errors during CRUD operations.
+// It is used to catch "no such command prefix" on RouterOS >= v7.0
+func (b *BgpInstance) HandleError(err error) error {
+	if legacyBgpUnsupported(err) {
+		return LegacyBgpUnsupported{}
+	}
+
+	return err
+}
+
 // Typed wrappers
 func (c Mikrotik) AddBgpInstance(r *BgpInstance) (*BgpInstance, error) {
 	res, err := c.Add(r)
-	if legacyBgpUnsupported(err) {
-		return nil, LegacyBgpUnsupported{}
-	}
 	if err != nil {
 		return nil, err
 	}
@@ -101,9 +108,6 @@ func (c Mikrotik) AddBgpInstance(r *BgpInstance) (*BgpInstance, error) {
 
 func (c Mikrotik) UpdateBgpInstance(r *BgpInstance) (*BgpInstance, error) {
 	res, err := c.Update(r)
-	if legacyBgpUnsupported(err) {
-		return nil, LegacyBgpUnsupported{}
-	}
 	if err != nil {
 		return nil, err
 	}
@@ -113,10 +117,6 @@ func (c Mikrotik) UpdateBgpInstance(r *BgpInstance) (*BgpInstance, error) {
 
 func (c Mikrotik) FindBgpInstance(name string) (*BgpInstance, error) {
 	res, err := c.Find(&BgpInstance{Name: name})
-	if legacyBgpUnsupported(err) {
-		return nil, LegacyBgpUnsupported{}
-	}
-
 	if err != nil {
 		return nil, err
 	}
@@ -126,9 +126,5 @@ func (c Mikrotik) FindBgpInstance(name string) (*BgpInstance, error) {
 
 func (c Mikrotik) DeleteBgpInstance(name string) error {
 	err := c.Delete(&BgpInstance{Name: name})
-	if legacyBgpUnsupported(err) {
-		return LegacyBgpUnsupported{}
-	}
-
 	return err
 }

client/client_crud.go

@@ -17,49 +17,55 @@ const (
 )
 
 type (
-	// Action represents possible action on resource
+	// Action represents possible action on resource.
 	Action string
 
-	// Resource interface defines a contract for abstract RouterOS resource
+	// Resource interface defines a contract for abstract RouterOS resource.
 	Resource interface {
-		// ActionToCommand translates CRUD action to RouterOS command path
+		// ActionToCommand translates CRUD action to RouterOS command path.
 		ActionToCommand(Action) string
 
-		// IDField reveals name of ID field to use in requests to MikroTik router
-		// It is used in operations like Find
+		// IDField reveals name of ID field to use in requests to MikroTik router.
+		// It is used in operations like Find.
 		IDField() string
 
-		// ID returns value of the ID field
+		// ID returns value of the ID field.
 		ID() string
 
-		// SetID updates a value of the ID field
+		// SetID updates a value of the ID field.
 		SetID(string)
 	}
 
-	// Adder defines contract for resources which require custom behaviour during resource creation
+	// Adder defines contract for resources which require custom behaviour during resource creation.
 	Adder interface {
-		// AfterAddHook is called right after the resource successfully added
-		// This hook is mainly used to set resource's ID field based on reply from RouterOS
+		// AfterAddHook is called right after the resource successfully added.
+		// This hook is mainly used to set resource's ID field based on reply from RouterOS.
 		AfterAddHook(r *routeros.Reply)
 	}
 
-	// Finder defines contract for resources which provide custom behaviour during resource retrieval
+	// Finder defines contract for resources which provide custom behaviour during resource retrieval.
 	Finder interface {
-		// FindField retrieves a name of a field to use as key for resource retrieval
+		// FindField retrieves a name of a field to use as key for resource retrieval.
 		FindField() string
 
-		// FindFieldValue retrieves a value to use for resource retrieval
+		// FindFieldValue retrieves a value to use for resource retrieval.
 		FindFieldValue() string
 	}
 
-	// Deleter defines contract for resources which require custom behaviour during resource deletion
+	// Deleter defines contract for resources which require custom behaviour during resource deletion.
 	Deleter interface {
-		// DeleteField retrieves a name of a field which is used for resource deletion
+		// DeleteField retrieves a name of a field which is used for resource deletion.
 		DeleteField() string
 
-		// DeleteFieldValue retrieves a value for DeleteField field
+		// DeleteFieldValue retrieves a value for DeleteField field.
 		DeleteFieldValue() string
 	}
+
+	// ErrorHandler Defines contract to handle errors returned by RouterOS.
+	// It can either return another error, or supress original error by returning nil.
+	ErrorHandler interface {
+		HandleError(error) error
+	}
 )
 
 // Add creates new resource on remote system
@@ -72,6 +78,9 @@ func (client Mikrotik) Add(d Resource) (Resource, error) {
 	cmd := Marshal(d.ActionToCommand(Add), d)
 	log.Printf("[INFO] Running the mikrotik command: `%s`", cmd)
 	r, err := c.RunArgs(cmd)
+	if eh, ok := d.(ErrorHandler); ok {
+		err = eh.HandleError(err)
+	}
 	if err != nil {
 		return nil, err
 	}
@@ -93,6 +102,9 @@ func (client Mikrotik) List(d Resource) ([]Resource, error) {
 		return nil, err
 	}
 	r, err := c.RunArgs(cmd)
+	if eh, ok := d.(ErrorHandler); ok {
+		err = eh.HandleError(err)
+	}
 	if err != nil {
 		return nil, err
 	}
@@ -135,7 +147,9 @@ func (client Mikrotik) Update(resource Resource) (Resource, error) {
 	cmd := Marshal(resource.ActionToCommand(Update), resource)
 	log.Printf("[INFO] Running the mikrotik command: `%s`", cmd)
 	_, err = c.RunArgs(cmd)
-
+	if eh, ok := resource.(ErrorHandler); ok {
+		err = eh.HandleError(err)
+	}
 	if err != nil {
 		return nil, err
 	}
@@ -159,6 +173,9 @@ func (client Mikrotik) Delete(d Resource) error {
 	cmd := []string{d.ActionToCommand(Delete), "=" + deleteField + "=" + deleteFieldValue}
 	log.Printf("[INFO] Running the mikrotik command: `%s`", cmd)
 	_, err = c.RunArgs(cmd)
+	if eh, ok := d.(ErrorHandler); ok {
+		err = eh.HandleError(err)
+	}
 
 	return err
 }
@@ -172,6 +189,9 @@ func (client Mikrotik) findByField(d Resource, field, value string) (Resource, e
 		return nil, err
 	}
 	r, err := c.RunArgs(cmd)
+	if eh, ok := d.(ErrorHandler); ok {
+		err = eh.HandleError(err)
+	}
 	if err != nil {
 		return nil, err
 	}
@@ -180,6 +200,9 @@ func (client Mikrotik) findByField(d Resource, field, value string) (Resource, e
 	targetStruct := client.newTargetStruct(d)
 	targetStructInterface := targetStruct.Interface()
 	err = Unmarshal(*r, targetStructInterface)
+	if eh, ok := d.(ErrorHandler); ok {
+		err = eh.HandleError(err)
+	}
 	if err != nil {
 		return nil, err
 	}

docs/resources/bgp_instance.md

@@ -26,24 +26,24 @@ resource "mikrotik_bgp_instance" "instance" {
 
 ### Optional
 
-- `client_to_client_reflection` (Boolean) The comment of the IP Pool to be created. Default: `true`.
+- `client_to_client_reflection` (Boolean) In case this instance is a route reflector: whether to redistribute routes learned from one routing reflection client to other clients.
 - `cluster_id` (String) In case this instance is a route reflector: cluster ID of the router reflector cluster this instance belongs to.
 - `comment` (String) The comment of the BGP instance to be created.
 - `confederation` (Number) In case of BGP confederations: autonomous system number that identifies the [local] confederation as a whole.
 - `confederation_peers` (String) List of AS numbers internal to the [local] confederation. For example: `10,20,30-50`.
 - `disabled` (Boolean) Whether instance is disabled.
-- `ignore_as_path_len` (Boolean) Whether to ignore AS_PATH attribute in BGP route selection algorithm. Default: `false`.
-- `out_filter` (String) Output routing filter chain used by all BGP peers belonging to this instance. Default: `""`.
-- `redistribute_connected` (Boolean) If enabled, this BGP instance will redistribute the information about connected routes. Default: `false`.
-- `redistribute_ospf` (Boolean) If enabled, this BGP instance will redistribute the information about routes learned by OSPF. Default: `false`.
-- `redistribute_other_bgp` (Boolean) If enabled, this BGP instance will redistribute the information about routes learned by other BGP instances. Default: `false`.
-- `redistribute_rip` (Boolean) If enabled, this BGP instance will redistribute the information about routes learned by RIP. Default: `false`.
-- `redistribute_static` (Boolean) If enabled, the router will redistribute the information about static routes added to its routing database. Default: `false`.
-- `routing_table` (String) Name of routing table this BGP instance operates on.  Default: `""`.
+- `ignore_as_path_len` (Boolean) Whether to ignore AS_PATH attribute in BGP route selection algorithm.
+- `out_filter` (String) Output routing filter chain used by all BGP peers belonging to this instance.
+- `redistribute_connected` (Boolean) If enabled, this BGP instance will redistribute the information about connected routes.
+- `redistribute_ospf` (Boolean) If enabled, this BGP instance will redistribute the information about routes learned by OSPF.
+- `redistribute_other_bgp` (Boolean) If enabled, this BGP instance will redistribute the information about routes learned by other BGP instances.
+- `redistribute_rip` (Boolean) If enabled, this BGP instance will redistribute the information about routes learned by RIP.
+- `redistribute_static` (Boolean) If enabled, the router will redistribute the information about static routes added to its routing database.
+- `routing_table` (String) Name of routing table this BGP instance operates on.
 
 ### Read-Only
 
-- `id` (String) The ID of this resource.
+- `id` (String) ID of this resource.
 
 ## Import
 Import is supported using the following syntax:

mikrotik/provider.go

@@ -63,9 +63,7 @@ func Provider(client *mt.Mikrotik) *schema.Provider {
 				Description: "Insecure connection does not verify MikroTik's TLS certificate",
 			},
 		},
-		ResourcesMap: map[string]*schema.Resource{
-			"mikrotik_bgp_instance": resourceBgpInstance(),
-		},
+		ResourcesMap: map[string]*schema.Resource{},
 	}
 
 	provider.ConfigureContextFunc = func(ctx context.Context, d *schema.ResourceData) (interface{}, diag.Diagnostics) {

mikrotik/provider_framework.go

@@ -182,6 +182,7 @@ func (p *ProviderFramework) DataSources(ctx context.Context) []func() datasource
 
 func (p *ProviderFramework) Resources(ctx context.Context) []func() resource.Resource {
 	return []func() resource.Resource{
+		NewBgpInstanceResource,
 		NewBgpPeerResource,
 		NewBridgePortResource,
 		NewBridgeResource,

mikrotik/provider_test.go

@@ -2,7 +2,6 @@ package mikrotik
 
 import (
 	"context"
-	"fmt"
 	"os"
 	"testing"
 
@@ -11,10 +10,7 @@ import (
 	"github.com/hashicorp/terraform-plugin-go/tfprotov5"
 	"github.com/hashicorp/terraform-plugin-mux/tf5muxserver"
 	"github.com/hashicorp/terraform-plugin-mux/tf6to5server"
-	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
-	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
-	"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
 )
 
 const (
@@ -61,37 +57,3 @@ func testAccPreCheck(t *testing.T) {
 		t.Fatal("The MIKROTIK_PASSWORD environment variable must be set")
 	}
 }
-
-func testAccDeleteResource(resource *schema.Resource, d *schema.ResourceData, meta interface{}) error {
-	if resource.DeleteContext != nil {
-		var diags diag.Diagnostics
-
-		diags = resource.DeleteContext(context.Background(), d, meta)
-
-		for i := range diags {
-			if diags[i].Severity == diag.Error {
-				return fmt.Errorf("error deleting resource: %s", diags[i].Summary)
-			}
-		}
-
-		return nil
-	}
-
-	return resource.Delete(d, meta)
-}
-
-func testAccCheckResourceDisappears(provider *schema.Provider, resource *schema.Resource, resourceName string) resource.TestCheckFunc {
-	return func(s *terraform.State) error {
-		resourceState, ok := s.RootModule().Resources[resourceName]
-
-		if !ok {
-			return fmt.Errorf("resource not found: %s", resourceName)
-		}
-
-		if resourceState.Primary.ID == "" {
-			return fmt.Errorf("resource ID missing: %s", resourceName)
-		}
-
-		return testAccDeleteResource(resource, resource.Data(resourceState.Primary), provider.Meta())
-	}
-}