From 85bea82f56791ef330c2512001b385bd0c8636e9 Mon Sep 17 00:00:00 2001 From: jdelnano Date: Wed, 9 Dec 2020 02:56:35 +0000 Subject: [PATCH 1/2] Add Marshalling and BGP Instance resource support --- client/bgp_instance.go | 127 +++++++++++++ client/bgp_instance_test.go | 125 +++++++++++++ client/client.go | 46 +++++ client/client_test.go | 36 +++- client/script.go | 8 - docs/resources/bgp_instance.md | 42 +++++ mikrotik/provider.go | 11 +- mikrotik/resource_bgp_instance.go | 239 +++++++++++++++++++++++++ mikrotik/resource_bgp_instance_test.go | 226 +++++++++++++++++++++++ 9 files changed, 846 insertions(+), 14 deletions(-) create mode 100644 client/bgp_instance.go create mode 100644 client/bgp_instance_test.go create mode 100644 docs/resources/bgp_instance.md create mode 100644 mikrotik/resource_bgp_instance.go create mode 100644 mikrotik/resource_bgp_instance_test.go diff --git a/client/bgp_instance.go b/client/bgp_instance.go new file mode 100644 index 00000000..00a115a2 --- /dev/null +++ b/client/bgp_instance.go @@ -0,0 +1,127 @@ +package client + +import ( + "fmt" + "log" + "strings" +) + +// 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"` +} + +// AddBgpInstance Mikrotik resource +func (client Mikrotik) AddBgpInstance(b *BgpInstance) (*BgpInstance, error) { + c, err := client.getMikrotikClient() + + if err != nil { + return nil, err + } + + attributes := Marshal(b) + cmd := strings.Split(fmt.Sprintf("/routing/bgp/instance/add %s", attributes), " ") + + log.Printf("[INFO] Running the mikrotik command: `%s`", cmd) + r, err := c.RunArgs(cmd) + log.Printf("[DEBUG] /routing/bgp/instance/add returned %v", r) + + if err != nil { + return nil, err + } + + return client.FindBgpInstance(b.Name) +} + +// FindBgpInstance Mikrotik resource +func (client Mikrotik) FindBgpInstance(name string) (*BgpInstance, error) { + c, err := client.getMikrotikClient() + if err != nil { + return nil, err + } + + cmd := strings.Split(fmt.Sprintf("/routing/bgp/instance/print ?name=%s", name), " ") + log.Printf("[INFO] Running the mikrotik command: `%s`", cmd) + r, err := c.RunArgs(cmd) + + if err != nil { + return nil, err + } + + log.Printf("[DEBUG] Find bgp instance: `%v`", cmd) + + bgpInstance := BgpInstance{} + + err = Unmarshal(*r, &bgpInstance) + + if err != nil { + return nil, err + } + + if bgpInstance.Name == "" { + return nil, NewNotFound(fmt.Sprintf("bgp instance `%s` not found", name)) + } + + return &bgpInstance, nil +} + +// UpdateBgpInstance Mikrotik resource +func (client Mikrotik) UpdateBgpInstance(b *BgpInstance) (*BgpInstance, error) { + c, err := client.getMikrotikClient() + + if err != nil { + return nil, err + } + + _, err = client.FindBgpInstance(b.Name) + + if err != nil { + return b, err + } + attributes := Marshal(b) + cmd := strings.Split(fmt.Sprintf("/routing/bgp/instance/set %s", attributes), " ") + + log.Printf("[INFO] Running the mikrotik command: `%s`", cmd) + _, err = c.RunArgs(cmd) + + if err != nil { + return nil, err + } + + return client.FindBgpInstance(b.Name) +} + +// DeleteBgpInstance Mikrotik resource +func (client Mikrotik) DeleteBgpInstance(name string) error { + c, err := client.getMikrotikClient() + + bgpInstance, err := client.FindBgpInstance(name) + + if err != nil { + return err + } + + cmd := strings.Split(fmt.Sprintf("/routing/bgp/instance/remove =numbers=%s", bgpInstance.Name), " ") + log.Printf("[INFO] Running the mikrotik command: `%s`", cmd) + r, err := c.RunArgs(cmd) + log.Printf("[DEBUG] Remove bgp instance via mikrotik api: %v", r) + + return err +} diff --git a/client/bgp_instance_test.go b/client/bgp_instance_test.go new file mode 100644 index 00000000..06e783a6 --- /dev/null +++ b/client/bgp_instance_test.go @@ -0,0 +1,125 @@ +package client + +import ( + "fmt" + "reflect" + "testing" +) + +var bgpName string = "test-bgp" +var as int = 65533 +var updatedAs int = 65534 +var clientToClientReflection bool = true +var clusterID string = "172.21.16.1" +var noClusterID string = "" +var bgpComment string = "test-comment" +var confederation int = 8 +var updatedConfederation int = 5 +var confederationPeers string = "" +var disabled bool = false +var ignoreAsPathLen bool = false +var outFilter string = "" +var redistributeConnected bool = false +var redistributeOspf bool = false +var redistributeOtherBgp bool = false +var redistributeRip bool = false +var redistributeStatic bool = false +var routerID string = "172.21.16.2" +var routingTable string = "" + +func TestAddBgpInstanceAndDeleteBgpInstance(t *testing.T) { + c := NewClient(GetConfigFromEnv()) + + expectedBgpInstance := &BgpInstance{ + Name: bgpName, + As: as, + ClientToClientReflection: clientToClientReflection, + IgnoreAsPathLen: ignoreAsPathLen, + OutFilter: outFilter, + RedistributeConnected: redistributeConnected, + RedistributeOspf: redistributeOspf, + RedistributeOtherBgp: redistributeOtherBgp, + RedistributeRip: redistributeRip, + RedistributeStatic: redistributeStatic, + RouterID: routerID, + RoutingTable: routingTable, + } + bgpInstance, err := c.AddBgpInstance(expectedBgpInstance) + if err != nil { + t.Fatalf("Error creating a bpg instance with: %v", err) + } + + expectedBgpInstance.ID = bgpInstance.ID + + if !reflect.DeepEqual(bgpInstance, expectedBgpInstance) { + t.Errorf("The bgp instance does not match what we expected. actual: %v expected: %v", bgpInstance, expectedBgpInstance) + } + + err = c.DeleteBgpInstance(bgpInstance.Name) + + if err != nil { + t.Errorf("Error deleting bgp instance with: %v", err) + } +} + +func TestAddAndUpdateBgpInstanceWithOptionalFieldsAndDeleteBgpInstance(t *testing.T) { + c := NewClient(GetConfigFromEnv()) + + expectedBgpInstance := &BgpInstance{ + Name: bgpName, + As: as, + ClientToClientReflection: clientToClientReflection, + Comment: bgpComment, + ConfederationPeers: confederationPeers, + Disabled: disabled, + IgnoreAsPathLen: ignoreAsPathLen, + OutFilter: outFilter, + RedistributeConnected: redistributeConnected, + RedistributeOspf: redistributeOspf, + RedistributeOtherBgp: redistributeOtherBgp, + RedistributeRip: redistributeRip, + RedistributeStatic: redistributeStatic, + RouterID: routerID, + RoutingTable: routingTable, + ClusterID: clusterID, + Confederation: confederation, + } + bgpInstance, err := c.AddBgpInstance(expectedBgpInstance) + if err != nil { + t.Fatalf("Error creating a bpg instance with: %v", err) + } + + expectedBgpInstance.ID = bgpInstance.ID + + if !reflect.DeepEqual(bgpInstance, expectedBgpInstance) { + t.Errorf("The bgp instance does not match what we expected. actual: %v expected: %v", bgpInstance, expectedBgpInstance) + } + + // update fields + expectedBgpInstance.Confederation = updatedConfederation + expectedBgpInstance.As = updatedAs + + bgpInstance, err = c.UpdateBgpInstance(expectedBgpInstance) + + if !reflect.DeepEqual(bgpInstance, expectedBgpInstance) { + t.Errorf("The bgp instance does not match what we expected. actual: %v expected: %v", bgpInstance, expectedBgpInstance) + } + + err = c.DeleteBgpInstance(bgpInstance.Name) + + if err != nil { + t.Errorf("Error deleting bgp instance with: %v", err) + } +} + +func TestFindBgpInstance_onNonExistantBgpInstance(t *testing.T) { + c := NewClient(GetConfigFromEnv()) + + name := "bgp instance does not exist" + _, err := c.FindBgpInstance(name) + + expectedErrStr := fmt.Sprintf("bgp instance `%s` not found", name) + if err == nil || err.Error() != expectedErrStr { + t.Errorf("client should have received error indicating the following bgp instance `%s` was not found. Instead error was nil", name) + } +} diff --git a/client/client.go b/client/client.go index 55037547..affe9f51 100644 --- a/client/client.go +++ b/client/client.go @@ -83,6 +83,9 @@ func parseStruct(v *reflect.Value, sentence proto.Sentence) { case reflect.Int: if contains(tags, "ttlToSeconds") { field.SetInt(int64(ttlToSeconds(pair.Value))) + } else { + intValue, _ := strconv.Atoi(pair.Value) + field.SetInt(int64(intValue)) } } @@ -163,3 +166,46 @@ func (client Mikrotik) getMikrotikClient() (c *routeros.Client, err error) { return } + +func boolToMikrotikBool(b bool) string { + if b { + return "yes" + } else { + return "no" + } +} + +func Marshal(s interface{}) string { + rv := reflect.ValueOf(s) + elem := rv.Elem() + + if rv.Kind() != reflect.Ptr { + panic("Command attribute construction cannot work without a pointer") + } + + var attributes []string + + for i := 0; i < elem.NumField(); i++ { + value := elem.Field(i) + fieldType := elem.Type().Field(i) + // supports multiple struct tags--assumes first is mikrotik field name + tag := strings.Split(fieldType.Tag.Get("mikrotik"), ",")[0] + + if tag != "" && (!value.IsZero() || value.Kind() == reflect.Bool) { + switch value.Kind() { + case reflect.Int: + intValue := elem.Field(i).Interface().(int) + attributes = append(attributes, fmt.Sprintf("=%s=%d", tag, intValue)) + case reflect.String: + stringValue := elem.Field(i).Interface().(string) + attributes = append(attributes, fmt.Sprintf("=%s=%s", tag, stringValue)) + case reflect.Bool: + boolValue := elem.Field(i).Interface().(bool) + stringBoolValue := boolToMikrotikBool(boolValue) + attributes = append(attributes, fmt.Sprintf("=%s=%s", tag, stringBoolValue)) + } + } + } + + return strings.Join(attributes, " ") +} diff --git a/client/client_test.go b/client/client_test.go index aa96d205..0dc2de41 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -34,10 +34,12 @@ func TestTtlToSeconds(t *testing.T) { func TestUnmarshal(t *testing.T) { name := "testing script" owner := "admin" + runCount := "3" allowed := "true" testStruct := struct { Name string NotNamedOwner string `mikrotik:"owner"` + RunCount int `mikrotik:"run-count"` Allowed bool }{} reply := routeros.Reply{ @@ -53,6 +55,10 @@ func TestUnmarshal(t *testing.T) { Key: "owner", Value: owner, }, + { + Key: "run-count", + Value: runCount, + }, { Key: "allowed", Value: allowed, @@ -72,7 +78,12 @@ func TestUnmarshal(t *testing.T) { } if strings.Compare(owner, testStruct.NotNamedOwner) != 0 { - t.Errorf("Failed to unmarshal name '%s' and testStruct.name '%s' should match", name, testStruct.Name) + t.Errorf("Failed to unmarshal name '%s' and testStruct.Name '%s' should match", name, testStruct.Name) + } + + intRunCount, err := strconv.Atoi(runCount) + if intRunCount != testStruct.RunCount || err != nil { + t.Errorf("Failed to unmarshal run-count '%s' and testStruct.RunCount '%d' should match", runCount, testStruct.RunCount) } b, _ := strconv.ParseBool(allowed) @@ -179,3 +190,26 @@ func TestUnmarshal_ttlToSeconds(t *testing.T) { t.Errorf("Failed to unmarshal ttl field with `ttlToSeconds`. Expected: '%d', received: %v", expectedTtl, testStruct.Ttl) } } + +func TestMarshal(t *testing.T) { + name := "test owner" + owner := "admin" + runCount := 3 + allowed := true + retain := false + testStruct := struct { + Name string `mikrotik:"name"` + NotNamedOwner string `mikrotik:"owner,extraTagNotUsed"` + RunCount int `mikrotik:"run-count"` + Allowed bool `mikrotik:"allowed-or-not"` + Retain bool `mikrotik:"retain"` + SecondaryOwner string `mikrotik:"secondary-owner"` + }{name, owner, runCount, allowed, retain, ""} + + expectedAttributes := "=name=test owner =owner=admin =run-count=3 =allowed-or-not=yes =retain=no" + attributes := Marshal(&testStruct) + + if attributes != expectedAttributes { + t.Errorf("Failed to marshal: %v does not equal expected %v", attributes, expectedAttributes) + } +} diff --git a/client/script.go b/client/script.go index 564f03f4..f2afe722 100644 --- a/client/script.go +++ b/client/script.go @@ -119,11 +119,3 @@ func (client Mikrotik) FindScript(name string) (*Script, error) { return script, err } - -func boolToMikrotikBool(b bool) string { - if b { - return "yes" - } else { - return "no" - } -} diff --git a/docs/resources/bgp_instance.md b/docs/resources/bgp_instance.md new file mode 100644 index 00000000..88d97f16 --- /dev/null +++ b/docs/resources/bgp_instance.md @@ -0,0 +1,42 @@ +# mikrotik_pool + +Creates a Mikrotik [BGP Instance](https://wiki.mikrotik.com/wiki/Manual:Routing/BGP#Instance). + +## Example Usage + +```hcl +resource "mikrotik_bgp_instance" "instance" { + name = "bgp-instance-name" + as = 65533 + router_id = 172.21.16.20 + comment = "test comment" +} +``` + +## Argument Reference +* name - (Required) The name of the BGP instance. +* as - (Required) The 32-bit BGP autonomous system number. Must be a value within 0 to 4294967295. +* router_id - (Required) BGP Router ID (for this instance). If set to 0.0.0.0, BGP will use one of router's IP addresses. +* routing_table - (Optional, Default: `""`) Name of routing table this BGP instance operates on. +* client_to_client_reflection - (Optional, Default: `true`) The comment of the IP Pool to be created +* comment - (Optional) The comment of the BGP instance to be created. +* confederation - (Optional) Autonomous system number that identifies the [local] confederation as a whole. Must be a value within 0 to 4294967295. +* confederation_peers - (Optional) List of AS numbers internal to the [local] confederation. For example: `"10,20,30-50"` +* disabled - (Optional, Default: `true`) Whether instance is disabled. +* ignore_as_path_len - (Optional, Default: `false`) Whether to ignore AS_PATH attribute in BGP route selection algorithm. +* out_filter - (Optional, Default: `""`) Output routing filter chain used by all BGP peers belonging to this instance. +* redistribute_connected - (Optional, Default: `false`) If enabled, this BGP instance will redistribute the information about connected routes. +* redistribute_ospf - (Optional, Default: `false`) If enabled, this BGP instance will redistribute the information about routes learned by OSPF. +* redistribute_other-bgp - (Optional, Default: `false`) If enabled, this BGP instance will redistribute the information about routes learned by other BGP instances. +* redistribute_rip - (Optional, Default: `false`) If enabled, this BGP instance will redistribute the information about routes learned by RIP. +* redistribute_static - (Optional, Default: `false`) If enabled, the router will redistribute the information about static routes added to its routing database. + + +## Attributes Reference + +## Import Reference + +```bash +# import with name of bgp instance +terraform import mikrotik_bgp_instance.instance bgp-instance-name +``` diff --git a/mikrotik/provider.go b/mikrotik/provider.go index 060d6bb2..c47c8914 100644 --- a/mikrotik/provider.go +++ b/mikrotik/provider.go @@ -29,11 +29,12 @@ func Provider() terraform.ResourceProvider { }, }, ResourcesMap: map[string]*schema.Resource{ - "mikrotik_dns_record": resourceRecord(), - "mikrotik_dhcp_lease": resourceLease(), - "mikrotik_scheduler": resourceScheduler(), - "mikrotik_script": resourceScript(), - "mikrotik_pool": resourcePool(), + "mikrotik_dns_record": resourceRecord(), + "mikrotik_dhcp_lease": resourceLease(), + "mikrotik_scheduler": resourceScheduler(), + "mikrotik_script": resourceScript(), + "mikrotik_pool": resourcePool(), + "mikrotik_bgp_instance": resourceBgpInstance(), }, ConfigureFunc: mikrotikConfigure, } diff --git a/mikrotik/resource_bgp_instance.go b/mikrotik/resource_bgp_instance.go new file mode 100644 index 00000000..4fcc85e3 --- /dev/null +++ b/mikrotik/resource_bgp_instance.go @@ -0,0 +1,239 @@ +package mikrotik + +import ( + "github.com/ddelnano/terraform-provider-mikrotik/client" + "github.com/hashicorp/terraform-plugin-sdk/helper/schema" +) + +func resourceBgpInstance() *schema.Resource { + return &schema.Resource{ + Create: resourceBgpInstanceCreate, + Read: resourceBgpInstanceRead, + Update: resourceBgpInstanceUpdate, + Delete: resourceBgpInstanceDelete, + Importer: &schema.ResourceImporter{ + State: schema.ImportStatePassthrough, + }, + + Schema: map[string]*schema.Schema{ + "name": &schema.Schema{ + Type: schema.TypeString, + Required: true, + }, + "as": &schema.Schema{ + Type: schema.TypeInt, + Required: true, + }, + "client_to_client_reflection": &schema.Schema{ + Type: schema.TypeBool, + Optional: true, + Default: true, + }, + "comment": &schema.Schema{ + Type: schema.TypeString, + Optional: true, + }, + "confederation_peers": &schema.Schema{ + Type: schema.TypeString, + Optional: true, + }, + "disabled": &schema.Schema{ + Type: schema.TypeBool, + Optional: true, + }, + "ignore_as_path_len": &schema.Schema{ + Type: schema.TypeBool, + Optional: true, + Default: false, + }, + "out_filter": &schema.Schema{ + Type: schema.TypeString, + Optional: true, + Default: "", + }, + "redistribute_connected": &schema.Schema{ + Type: schema.TypeBool, + Optional: true, + Default: false, + }, + "redistribute_ospf": &schema.Schema{ + Type: schema.TypeBool, + Optional: true, + Default: false, + }, + "redistribute_other_bgp": &schema.Schema{ + Type: schema.TypeBool, + Optional: true, + Default: false, + }, + "redistribute_rip": &schema.Schema{ + Type: schema.TypeBool, + Optional: true, + Default: false, + }, + "redistribute_static": &schema.Schema{ + Type: schema.TypeBool, + Optional: true, + Default: false, + }, + "router_id": &schema.Schema{ + Type: schema.TypeString, + Required: true, + }, + "routing_table": &schema.Schema{ + Type: schema.TypeString, + Optional: true, + Default: "", + }, + "cluster_id": &schema.Schema{ + Type: schema.TypeString, + Optional: true, + }, + "confederation": &schema.Schema{ + Type: schema.TypeInt, + Optional: true, + }, + }, + } +} + +func resourceBgpInstanceCreate(d *schema.ResourceData, m interface{}) error { + instance := prepareBgpInstance(d) + + c := m.(client.Mikrotik) + + bgpInstance, err := c.AddBgpInstance(instance) + if err != nil { + return err + } + + return bgpInstanceToData(bgpInstance, d) +} + +func resourceBgpInstanceRead(d *schema.ResourceData, m interface{}) error { + c := m.(client.Mikrotik) + + bgpInstance, err := c.FindBgpInstance(d.Id()) + + // TODO: Ignoring this error can cause all resources to think they + // need to be created. We should more appropriately handle this. The + // error where the record is not found is not actually an error and + // needs to be disambiguated from real failures + if err != nil { + d.SetId("") + return nil + } + + return bgpInstanceToData(bgpInstance, d) +} + +func resourceBgpInstanceUpdate(d *schema.ResourceData, m interface{}) error { + c := m.(client.Mikrotik) + + currentBgpInstance, err := c.FindBgpInstance(d.Get("name").(string)) + + instance := prepareBgpInstance(d) + instance.ID = currentBgpInstance.ID + + bgpInstance, err := c.UpdateBgpInstance(instance) + + if err != nil { + return err + } + + return bgpInstanceToData(bgpInstance, d) +} + +func resourceBgpInstanceDelete(d *schema.ResourceData, m interface{}) error { + c := m.(client.Mikrotik) + + err := c.DeleteBgpInstance(d.Get("name").(string)) + + if err != nil { + return err + } + + d.SetId("") + return nil +} + +func bgpInstanceToData(b *client.BgpInstance, d *schema.ResourceData) error { + d.SetId(b.Name) + + if err := d.Set("name", b.Name); err != nil { + return err + } + if err := d.Set("as", b.As); err != nil { + return err + } + if err := d.Set("client_to_client_reflection", b.ClientToClientReflection); err != nil { + return err + } + if err := d.Set("comment", b.Comment); err != nil { + return err + } + if err := d.Set("confederation_peers", b.ConfederationPeers); err != nil { + return err + } + if err := d.Set("disabled", b.Disabled); err != nil { + return err + } + if err := d.Set("ignore_as_path_len", b.IgnoreAsPathLen); err != nil { + return err + } + if err := d.Set("out_filter", b.OutFilter); err != nil { + return err + } + if err := d.Set("redistribute_connected", b.RedistributeConnected); err != nil { + return err + } + if err := d.Set("redistribute_ospf", b.RedistributeOspf); err != nil { + return err + } + if err := d.Set("redistribute_other_bgp", b.RedistributeOtherBgp); err != nil { + return err + } + if err := d.Set("redistribute_rip", b.RedistributeRip); err != nil { + return err + } + if err := d.Set("redistribute_static", b.RedistributeStatic); err != nil { + return err + } + if err := d.Set("router_id", b.RouterID); err != nil { + return err + } + if err := d.Set("routing_table", b.RoutingTable); err != nil { + return err + } + if err := d.Set("cluster_id", b.ClusterID); err != nil { + return err + } + if err := d.Set("confederation", b.Confederation); err != nil { + return err + } + return nil +} + +func prepareBgpInstance(d *schema.ResourceData) *client.BgpInstance { + bgpInstance := new(client.BgpInstance) + + bgpInstance.Name = d.Get("name").(string) + bgpInstance.As = d.Get("as").(int) + bgpInstance.ClientToClientReflection = d.Get("client_to_client_reflection").(bool) + bgpInstance.Comment = d.Get("comment").(string) + bgpInstance.ConfederationPeers = d.Get("confederation_peers").(string) + bgpInstance.Disabled = d.Get("disabled").(bool) + bgpInstance.IgnoreAsPathLen = d.Get("ignore_as_path_len").(bool) + bgpInstance.OutFilter = d.Get("out_filter").(string) + bgpInstance.RedistributeConnected = d.Get("redistribute_connected").(bool) + bgpInstance.RedistributeOspf = d.Get("redistribute_ospf").(bool) + bgpInstance.RedistributeOtherBgp = d.Get("redistribute_other_bgp").(bool) + bgpInstance.RedistributeRip = d.Get("redistribute_rip").(bool) + bgpInstance.RedistributeStatic = d.Get("redistribute_static").(bool) + bgpInstance.RouterID = d.Get("router_id").(string) + bgpInstance.RoutingTable = d.Get("routing_table").(string) + bgpInstance.ClusterID = d.Get("cluster_id").(string) + bgpInstance.Confederation = d.Get("confederation").(int) + + return bgpInstance +} diff --git a/mikrotik/resource_bgp_instance_test.go b/mikrotik/resource_bgp_instance_test.go new file mode 100644 index 00000000..a2a1a059 --- /dev/null +++ b/mikrotik/resource_bgp_instance_test.go @@ -0,0 +1,226 @@ +package mikrotik + +import ( + "fmt" + "testing" + + "github.com/ddelnano/terraform-provider-mikrotik/client" + "github.com/hashicorp/terraform-plugin-sdk/helper/resource" + "github.com/hashicorp/terraform-plugin-sdk/terraform" +) + +var originalBgpName string = "test-bgp-instance" +var originalConfederation string = "8" +var originalAs string = "65532" +var updatedAs string = "65533" +var originalRouterId string = "172.21.16.1" +var originalClusterId string = "172.21.17.1" +var updatedRouterId string = "172.21.16.2" +var commentBgpInstance string = "test-comment" + +func TestAccMikrotikBgpInstance_create(t *testing.T) { + resourceName := "mikrotik_bgp_instance.bar" + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckMikrotikBgpInstanceDestroy, + Steps: []resource.TestStep{ + { + Config: testAccBgpInstance(), + Check: resource.ComposeAggregateTestCheckFunc( + testAccBgpInstanceExists(resourceName), + resource.TestCheckResourceAttrSet(resourceName, "id"), + resource.TestCheckResourceAttr(resourceName, "name", originalBgpName), + resource.TestCheckResourceAttr(resourceName, "as", originalAs), + resource.TestCheckResourceAttr(resourceName, "router_id", originalRouterId), + ), + }, + }, + }) +} + +func TestAccMikrotikBgpInstance_createAndPlanWithNonExistantBgpInstance(t *testing.T) { + resourceName := "mikrotik_bgp_instance.bar" + removeBgpInstance := func() { + + c := client.NewClient(client.GetConfigFromEnv()) + bgpInstance, err := c.FindBgpInstance(originalBgpName) + if err != nil { + t.Fatalf("Error finding the bgp instance by name: %s", err) + } + err = c.DeleteBgpInstance(bgpInstance.Name) + if err != nil { + t.Fatalf("Error removing the bgp instance: %s", err) + } + } + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckMikrotikBgpInstanceDestroy, + Steps: []resource.TestStep{ + { + Config: testAccBgpInstance(), + Check: resource.ComposeAggregateTestCheckFunc( + testAccBgpInstanceExists(resourceName), + resource.TestCheckResourceAttrSet(resourceName, "id")), + }, + { + PreConfig: removeBgpInstance, + Config: testAccBgpInstance(), + ExpectNonEmptyPlan: false, + }, + }, + }) +} + +func TestAccMikrotikBgpInstance_updateBgpInstance(t *testing.T) { + resourceName := "mikrotik_bgp_instance.bar" + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckMikrotikBgpInstanceDestroy, + Steps: []resource.TestStep{ + { + Config: testAccBgpInstance(), + Check: resource.ComposeAggregateTestCheckFunc( + testAccBgpInstanceExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "name", originalBgpName), + resource.TestCheckResourceAttr(resourceName, "as", originalAs), + resource.TestCheckResourceAttr(resourceName, "router_id", originalRouterId), + ), + }, + { + Config: testAccBgpInstanceUpdatedAsAndRouterId(), + Check: resource.ComposeAggregateTestCheckFunc( + testAccBgpInstanceExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "name", originalBgpName), + resource.TestCheckResourceAttr(resourceName, "as", updatedAs), + resource.TestCheckResourceAttr(resourceName, "router_id", updatedRouterId), + ), + }, + { + Config: testAccBgpInstanceUpdatedOptionalFields(), + Check: resource.ComposeAggregateTestCheckFunc( + testAccBgpInstanceExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "name", originalBgpName), + resource.TestCheckResourceAttr(resourceName, "as", updatedAs), + resource.TestCheckResourceAttr(resourceName, "router_id", updatedRouterId), + resource.TestCheckResourceAttr(resourceName, "comment", commentBgpInstance), + resource.TestCheckResourceAttr(resourceName, "cluster_id", originalClusterId), + resource.TestCheckResourceAttr(resourceName, "client_to_client_reflection", "false"), + resource.TestCheckResourceAttr(resourceName, "confederation", originalConfederation), + ), + }, + }, + }) +} + +func TestAccMikrotikBgpInstance_import(t *testing.T) { + resourceName := "mikrotik_bgp_instance.bar" + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckMikrotikBgpInstanceDestroy, + Steps: []resource.TestStep{ + { + Config: testAccBgpInstance(), + Check: resource.ComposeAggregateTestCheckFunc( + testAccBgpInstanceExists(resourceName), + resource.TestCheckResourceAttrSet(resourceName, "id")), + }, + // TODO: figure out why this fails + { + ResourceName: resourceName, + // tried adding this field, but didn't help + ImportStateId: originalBgpName, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + +func testAccBgpInstance() string { + return fmt.Sprintf(` +resource "mikrotik_bgp_instance" "bar" { + name = "%s" + as = 65532 + router_id = "%s" +} +`, originalBgpName, originalRouterId) +} + +func testAccBgpInstanceUpdatedAsAndRouterId() string { + return fmt.Sprintf(` +resource "mikrotik_bgp_instance" "bar" { + name = "%s" + as = 65533 + router_id = "%s" +} +`, originalBgpName, updatedRouterId) +} + +func testAccBgpInstanceUpdatedOptionalFields() string { + return fmt.Sprintf(` +resource "mikrotik_bgp_instance" "bar" { + name = "%s" + as = 65533 + router_id = "%s" + comment = "%s" + cluster_id = "%s" + client_to_client_reflection = false + confederation = 8 +} +`, originalBgpName, updatedRouterId, commentBgpInstance, originalClusterId) +} + +func testAccBgpInstanceExists(resourceName string) resource.TestCheckFunc { + return func(s *terraform.State) error { + rs, ok := s.RootModule().Resources[resourceName] + if !ok { + return fmt.Errorf("Not found: %s", resourceName) + } + + if rs.Primary.ID == "" { + return fmt.Errorf("mikrotik_bgp_instance does not exist in the statefile") + } + + c := client.NewClient(client.GetConfigFromEnv()) + + bgpInstance, err := c.FindBgpInstance(rs.Primary.ID) + + if err != nil { + return fmt.Errorf("Unable to get the bgp instance with error: %v", err) + } + + if bgpInstance == nil { + return fmt.Errorf("Unable to get the bgp instance") + } + + if bgpInstance.Name == rs.Primary.ID { + return nil + } + return nil + } +} + +func testAccCheckMikrotikBgpInstanceDestroy(s *terraform.State) error { + c := client.NewClient(client.GetConfigFromEnv()) + for _, rs := range s.RootModule().Resources { + if rs.Type != "mikrotik_bgp_instance" { + continue + } + + bgpInstance, err := c.FindBgpInstance(rs.Primary.ID) + + _, ok := err.(*client.NotFound) + if !ok && err != nil { + return err + } + + if bgpInstance != nil { + return fmt.Errorf("bgp instance (%s) still exists", bgpInstance.Name) + } + } + return nil +} From 385538fefffa28263dee563dd3e6bb7faa1afb26 Mon Sep 17 00:00:00 2001 From: jdelnano Date: Tue, 26 Jan 2021 17:43:12 -0500 Subject: [PATCH 2/2] Improve Marshalling & error handling for BGP creation --- client/bgp_instance.go | 5 ----- client/bgp_instance_test.go | 6 ++--- client/client.go | 9 +++++--- client/client_test.go | 31 ++++++++++++++++++++++++++ mikrotik/resource_bgp_instance.go | 6 +---- mikrotik/resource_bgp_instance_test.go | 5 +---- 6 files changed, 41 insertions(+), 21 deletions(-) diff --git a/client/bgp_instance.go b/client/bgp_instance.go index 00a115a2..0444740b 100644 --- a/client/bgp_instance.go +++ b/client/bgp_instance.go @@ -90,11 +90,6 @@ func (client Mikrotik) UpdateBgpInstance(b *BgpInstance) (*BgpInstance, error) { return nil, err } - _, err = client.FindBgpInstance(b.Name) - - if err != nil { - return b, err - } attributes := Marshal(b) cmd := strings.Split(fmt.Sprintf("/routing/bgp/instance/set %s", attributes), " ") diff --git a/client/bgp_instance_test.go b/client/bgp_instance_test.go index 06e783a6..7456e0bc 100644 --- a/client/bgp_instance_test.go +++ b/client/bgp_instance_test.go @@ -1,7 +1,6 @@ package client import ( - "fmt" "reflect" "testing" ) @@ -118,8 +117,7 @@ func TestFindBgpInstance_onNonExistantBgpInstance(t *testing.T) { name := "bgp instance does not exist" _, err := c.FindBgpInstance(name) - expectedErrStr := fmt.Sprintf("bgp instance `%s` not found", name) - if err == nil || err.Error() != expectedErrStr { - t.Errorf("client should have received error indicating the following bgp instance `%s` was not found. Instead error was nil", name) + if _, ok := err.(*NotFound); !ok { + t.Errorf("Expecting to receive NotFound error for bgp instance `%s`, instead error was nil.", name) } } diff --git a/client/client.go b/client/client.go index affe9f51..56cae2f8 100644 --- a/client/client.go +++ b/client/client.go @@ -176,11 +176,14 @@ func boolToMikrotikBool(b bool) string { } func Marshal(s interface{}) string { + var elem reflect.Value rv := reflect.ValueOf(s) - elem := rv.Elem() - if rv.Kind() != reflect.Ptr { - panic("Command attribute construction cannot work without a pointer") + if rv.Kind() == reflect.Ptr { + // get Value of what pointer points to + elem = rv.Elem() + } else { + elem = rv } var attributes []string diff --git a/client/client_test.go b/client/client_test.go index 0dc2de41..d3035c84 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -207,9 +207,40 @@ func TestMarshal(t *testing.T) { }{name, owner, runCount, allowed, retain, ""} expectedAttributes := "=name=test owner =owner=admin =run-count=3 =allowed-or-not=yes =retain=no" + // Marshal by passing pointer to struct attributes := Marshal(&testStruct) if attributes != expectedAttributes { t.Errorf("Failed to marshal: %v does not equal expected %v", attributes, expectedAttributes) } + + // Marshal by passing by struct value + attributes = Marshal(testStruct) + + if attributes != expectedAttributes { + t.Errorf("Failed to marshal: %v does not equal expected %v", attributes, expectedAttributes) + } +} + +func TestMarshalStructWithoutTags(t *testing.T) { + name := "test owner" + owner := "admin" + runCount := 3 + allowed := true + retain := false + testStruct := struct { + Name string `example:"name"` + NotNamedOwner string `json:"not-named"` + RunCount int + Allowed bool + Retain bool + SecondaryOwner string + }{name, owner, runCount, allowed, retain, ""} + + expectedAttributes := "" + attributes := Marshal(&testStruct) + + if attributes != expectedAttributes { + t.Errorf("Marshaling with a struct without tags shoudl return empty attributes for command: %v does not equal expected %v", attributes, expectedAttributes) + } } diff --git a/mikrotik/resource_bgp_instance.go b/mikrotik/resource_bgp_instance.go index 4fcc85e3..330fae7a 100644 --- a/mikrotik/resource_bgp_instance.go +++ b/mikrotik/resource_bgp_instance.go @@ -115,11 +115,7 @@ func resourceBgpInstanceRead(d *schema.ResourceData, m interface{}) error { bgpInstance, err := c.FindBgpInstance(d.Id()) - // TODO: Ignoring this error can cause all resources to think they - // need to be created. We should more appropriately handle this. The - // error where the record is not found is not actually an error and - // needs to be disambiguated from real failures - if err != nil { + if _, ok := err.(*client.NotFound); ok { d.SetId("") return nil } diff --git a/mikrotik/resource_bgp_instance_test.go b/mikrotik/resource_bgp_instance_test.go index a2a1a059..b56549a9 100644 --- a/mikrotik/resource_bgp_instance_test.go +++ b/mikrotik/resource_bgp_instance_test.go @@ -128,11 +128,8 @@ func TestAccMikrotikBgpInstance_import(t *testing.T) { testAccBgpInstanceExists(resourceName), resource.TestCheckResourceAttrSet(resourceName, "id")), }, - // TODO: figure out why this fails { - ResourceName: resourceName, - // tried adding this field, but didn't help - ImportStateId: originalBgpName, + ResourceName: resourceName, ImportState: true, ImportStateVerify: true, },