From f682a3b181b24b38cfb72ac2df64ea9094e464a3 Mon Sep 17 00:00:00 2001 From: Kyle Cronin Date: Sun, 23 Apr 2023 13:48:06 -0400 Subject: [PATCH] fix(infoblox): set view and zone query parameters The `zone` and `view` search query parameters are not included in record search requests. This can result in more records returned in the query than necessary. For example if more than one zone is returned, rather than issuing a query on each respective zone it is searching all zones on each iteration. --- provider/infoblox/infoblox.go | 37 +++++---- provider/infoblox/infoblox_test.go | 129 +++++++++++++++++++++++++++-- 2 files changed, 142 insertions(+), 24 deletions(-) diff --git a/provider/infoblox/infoblox.go b/provider/infoblox/infoblox.go index 306d6672d5..f93334d458 100644 --- a/provider/infoblox/infoblox.go +++ b/provider/infoblox/infoblox.go @@ -183,6 +183,18 @@ func NewInfobloxProvider(ibStartupCfg StartupConfig) (*ProviderConfig, error) { return providerCfg, nil } +func (p *ProviderConfig) recordsQueryParams(zone string, view string) *ibclient.QueryParams { + searchFields := map[string]string{} + if zone != "" { + searchFields["zone"] = zone + } + + if view != "" { + searchFields["view"] = view + } + return ibclient.NewQueryParams(false, searchFields) +} + // Records gets the current records. func (p *ProviderConfig) Records(ctx context.Context) (endpoints []*endpoint.Endpoint, err error) { zones, err := p.zones() @@ -194,9 +206,8 @@ func (p *ProviderConfig) Records(ctx context.Context) (endpoints []*endpoint.End logrus.Debugf("fetch records from zone '%s'", zone.Fqdn) var resA []ibclient.RecordA objA := ibclient.NewEmptyRecordA() - objA.View = p.view - objA.Zone = zone.Fqdn - err = p.client.GetObject(objA, "", nil, &resA) + objAQueryParams := p.recordsQueryParams(zone.Fqdn, p.view) + err = p.client.GetObject(objA, "", objAQueryParams, &resA) if err != nil && !isNotFoundError(err) { return nil, fmt.Errorf("could not fetch A records from zone '%s': %s", zone.Fqdn, err) } @@ -240,9 +251,8 @@ func (p *ProviderConfig) Records(ctx context.Context) (endpoints []*endpoint.End // Include Host records since they should be treated synonymously with A records var resH []ibclient.HostRecord objH := ibclient.NewEmptyHostRecord() - objH.View = p.view - objH.Zone = zone.Fqdn - err = p.client.GetObject(objH, "", nil, &resH) + objHQueryParams := p.recordsQueryParams(zone.Fqdn, p.view) + err = p.client.GetObject(objH, "", objHQueryParams, &resH) if err != nil && !isNotFoundError(err) { return nil, fmt.Errorf("could not fetch host records from zone '%s': %s", zone.Fqdn, err) } @@ -262,9 +272,8 @@ func (p *ProviderConfig) Records(ctx context.Context) (endpoints []*endpoint.End var resC []ibclient.RecordCNAME objC := ibclient.NewEmptyRecordCNAME() - objC.View = p.view - objC.Zone = zone.Fqdn - err = p.client.GetObject(objC, "", nil, &resC) + objCQueryParams := p.recordsQueryParams(zone.Fqdn, p.view) + err = p.client.GetObject(objC, "", objCQueryParams, &resC) if err != nil && !isNotFoundError(err) { return nil, fmt.Errorf("could not fetch CNAME records from zone '%s': %s", zone.Fqdn, err) } @@ -294,13 +303,9 @@ func (p *ProviderConfig) Records(ctx context.Context) (endpoints []*endpoint.End } var resT []ibclient.RecordTXT - objT := ibclient.NewRecordTXT( - ibclient.RecordTXT{ - Zone: zone.Fqdn, - View: p.view, - }, - ) - err = p.client.GetObject(objT, "", nil, &resT) + objT := ibclient.NewRecordTXT(ibclient.RecordTXT{}) + objTQueryParams := p.recordsQueryParams(zone.Fqdn, p.view) + err = p.client.GetObject(objT, "", objTQueryParams, &resT) if err != nil && !isNotFoundError(err) { return nil, fmt.Errorf("could not fetch TXT records from zone '%s': %s", zone.Fqdn, err) } diff --git a/provider/infoblox/infoblox_test.go b/provider/infoblox/infoblox_test.go index 13b5e0b52e..90970cc524 100644 --- a/provider/infoblox/infoblox_test.go +++ b/provider/infoblox/infoblox_test.go @@ -17,10 +17,12 @@ limitations under the License. package infoblox import ( + "bytes" "context" "encoding/base64" "fmt" "net/http" + "net/url" "regexp" "strings" "testing" @@ -41,6 +43,60 @@ type mockIBConnector struct { createdEndpoints []*endpoint.Endpoint deletedEndpoints []*endpoint.Endpoint updatedEndpoints []*endpoint.Endpoint + getObjectRequests []*getObjectRequest + requestBuilder ExtendedRequestBuilder +} + +type getObjectRequest struct { + obj string + ref string + queryParams string + url url.URL + verified bool +} + +func (req *getObjectRequest) ExpectRequestURLQueryParam(t *testing.T, name string, value string) *getObjectRequest { + if req.url.Query().Get(name) != value { + t.Errorf("Expected GetObject Request URL to contain query parameter %s=%s, Got: %v", name, value, req.url.Query()) + } + + return req +} + +func (client *mockIBConnector) verifyGetObjectRequest(t *testing.T, obj string, ref string, query *map[string]string) *getObjectRequest { + qp := "" + if query != nil { + qp = fmt.Sprint(ibclient.NewQueryParams(false, *query)) + } + + for _, req := range client.getObjectRequests { + if !req.verified && req.obj == obj && req.ref == ref && req.queryParams == qp { + req.verified = true + return req + } + } + + t.Errorf("Expected GetObject obj=%s, query=%s, ref=%s", obj, qp, ref) + return &getObjectRequest{} +} + +// verifyNoMoreGetObjectRequests will assert that all "GetObject" calls have been verified. +func (client *mockIBConnector) verifyNoMoreGetObjectRequests(t *testing.T) { + unverified := []getObjectRequest{} + for _, req := range client.getObjectRequests { + if !req.verified { + unverified = append(unverified, *req) + } + } + + if len(unverified) > 0 { + b := new(bytes.Buffer) + for _, req := range unverified { + fmt.Fprintf(b, "obj=%s, ref=%s, params=%s (url=%s)\n", req.obj, req.ref, req.queryParams, req.url.String()) + } + + t.Errorf("Unverified GetObject Requests: %v", unverified) + } } func (client *mockIBConnector) CreateObject(obj ibclient.IBObject) (ref string, err error) { @@ -115,6 +171,18 @@ func (client *mockIBConnector) CreateObject(obj ibclient.IBObject) (ref string, } func (client *mockIBConnector) GetObject(obj ibclient.IBObject, ref string, queryParams *ibclient.QueryParams, res interface{}) (err error) { + req := getObjectRequest{ + obj: obj.ObjectType(), + ref: ref, + } + if queryParams != nil { + req.queryParams = fmt.Sprint(queryParams) + } + r, _ := client.requestBuilder.BuildRequest(ibclient.GET, obj, ref, queryParams) + if r != nil { + req.url = *r.URL + } + client.getObjectRequests = append(client.getObjectRequests, &req) switch obj.ObjectType() { case "record:a": var result []ibclient.RecordA @@ -383,13 +451,14 @@ func createMockInfobloxObject(name, recordType, value string) ibclient.IBObject return nil } -func newInfobloxProvider(domainFilter endpoint.DomainFilter, zoneIDFilter provider.ZoneIDFilter, dryRun bool, createPTR bool, client ibclient.IBConnector) *ProviderConfig { +func newInfobloxProvider(domainFilter endpoint.DomainFilter, zoneIDFilter provider.ZoneIDFilter, view string, dryRun bool, createPTR bool, client ibclient.IBConnector) *ProviderConfig { return &ProviderConfig{ client: client, domainFilter: domainFilter, zoneIDFilter: zoneIDFilter, dryRun: dryRun, createPTR: createPTR, + view: view, } } @@ -417,7 +486,7 @@ func TestInfobloxRecords(t *testing.T) { }, } - providerCfg := newInfobloxProvider(endpoint.NewDomainFilter([]string{"example.com"}), provider.NewZoneIDFilter([]string{""}), true, false, &client) + providerCfg := newInfobloxProvider(endpoint.NewDomainFilter([]string{"example.com"}), provider.NewZoneIDFilter([]string{""}), "", true, false, &client) actual, err := providerCfg.Records(context.Background()) if err != nil { t.Fatal(err) @@ -437,6 +506,50 @@ func TestInfobloxRecords(t *testing.T) { endpoint.NewEndpoint("host.example.com", endpoint.RecordTypeA, "125.1.1.1"), } validateEndpoints(t, actual, expected) + client.verifyGetObjectRequest(t, "zone_auth", "", nil) + client.verifyGetObjectRequest(t, "record:a", "", &map[string]string{"zone": "example.com"}).ExpectRequestURLQueryParam(t, "zone", "example.com") + client.verifyGetObjectRequest(t, "record:host", "", &map[string]string{"zone": "example.com"}).ExpectRequestURLQueryParam(t, "zone", "example.com") + client.verifyGetObjectRequest(t, "record:cname", "", &map[string]string{"zone": "example.com"}).ExpectRequestURLQueryParam(t, "zone", "example.com") + client.verifyGetObjectRequest(t, "record:txt", "", &map[string]string{"zone": "example.com"}).ExpectRequestURLQueryParam(t, "zone", "example.com") + client.verifyNoMoreGetObjectRequests(t) +} + +func TestInfobloxRecordsWithView(t *testing.T) { + client := mockIBConnector{ + mockInfobloxZones: &[]ibclient.ZoneAuth{ + createMockInfobloxZone("foo.example.com"), + createMockInfobloxZone("bar.example.com"), + }, + mockInfobloxObjects: &[]ibclient.IBObject{ + createMockInfobloxObject("cat.foo.example.com", endpoint.RecordTypeA, "123.123.123.122"), + createMockInfobloxObject("dog.bar.example.com", endpoint.RecordTypeA, "123.123.123.123"), + }, + } + + providerCfg := newInfobloxProvider(endpoint.NewDomainFilter([]string{"foo.example.com", "bar.example.com"}), provider.NewZoneIDFilter([]string{""}), "Inside", true, false, &client) + actual, err := providerCfg.Records(context.Background()) + if err != nil { + t.Fatal(err) + } + expected := []*endpoint.Endpoint{ + endpoint.NewEndpoint("cat.foo.example.com", endpoint.RecordTypeA, "123.123.123.122"), + endpoint.NewEndpoint("dog.bar.example.com", endpoint.RecordTypeA, "123.123.123.123"), + } + validateEndpoints(t, actual, expected) + client.verifyGetObjectRequest(t, "zone_auth", "", nil) + client.verifyGetObjectRequest(t, "record:a", "", &map[string]string{"zone": "foo.example.com", "view": "Inside"}). + ExpectRequestURLQueryParam(t, "zone", "foo.example.com"). + ExpectRequestURLQueryParam(t, "view", "Inside") + client.verifyGetObjectRequest(t, "record:host", "", &map[string]string{"zone": "foo.example.com", "view": "Inside"}) + client.verifyGetObjectRequest(t, "record:cname", "", &map[string]string{"zone": "foo.example.com", "view": "Inside"}) + client.verifyGetObjectRequest(t, "record:txt", "", &map[string]string{"zone": "foo.example.com", "view": "Inside"}) + client.verifyGetObjectRequest(t, "record:a", "", &map[string]string{"zone": "bar.example.com", "view": "Inside"}). + ExpectRequestURLQueryParam(t, "zone", "bar.example.com"). + ExpectRequestURLQueryParam(t, "view", "Inside") + client.verifyGetObjectRequest(t, "record:host", "", &map[string]string{"zone": "bar.example.com", "view": "Inside"}) + client.verifyGetObjectRequest(t, "record:cname", "", &map[string]string{"zone": "bar.example.com", "view": "Inside"}) + client.verifyGetObjectRequest(t, "record:txt", "", &map[string]string{"zone": "bar.example.com", "view": "Inside"}) + client.verifyNoMoreGetObjectRequests(t) } func TestInfobloxAdjustEndpoints(t *testing.T) { @@ -453,7 +566,7 @@ func TestInfobloxAdjustEndpoints(t *testing.T) { }, } - providerCfg := newInfobloxProvider(endpoint.NewDomainFilter([]string{"example.com"}), provider.NewZoneIDFilter([]string{""}), true, true, &client) + providerCfg := newInfobloxProvider(endpoint.NewDomainFilter([]string{"example.com"}), provider.NewZoneIDFilter([]string{""}), "", true, true, &client) actual, err := providerCfg.Records(context.Background()) if err != nil { t.Fatal(err) @@ -481,7 +594,7 @@ func TestInfobloxRecordsReverse(t *testing.T) { }, } - providerCfg := newInfobloxProvider(endpoint.NewDomainFilter([]string{"10.0.0.0/24"}), provider.NewZoneIDFilter([]string{""}), true, true, &client) + providerCfg := newInfobloxProvider(endpoint.NewDomainFilter([]string{"10.0.0.0/24"}), provider.NewZoneIDFilter([]string{""}), "", true, true, &client) actual, err := providerCfg.Records(context.Background()) if err != nil { t.Fatal(err) @@ -588,6 +701,7 @@ func testInfobloxApplyChangesInternal(t *testing.T, dryRun, createPTR bool, clie providerCfg := newInfobloxProvider( endpoint.NewDomainFilter([]string{""}), provider.NewZoneIDFilter([]string{""}), + "", dryRun, createPTR, client, @@ -653,7 +767,7 @@ func TestInfobloxZones(t *testing.T) { mockInfobloxObjects: &[]ibclient.IBObject{}, } - providerCfg := newInfobloxProvider(endpoint.NewDomainFilter([]string{"example.com", "1.2.3.0/24"}), provider.NewZoneIDFilter([]string{""}), true, false, &client) + providerCfg := newInfobloxProvider(endpoint.NewDomainFilter([]string{"example.com", "1.2.3.0/24"}), provider.NewZoneIDFilter([]string{""}), "", true, false, &client) zones, _ := providerCfg.zones() var emptyZoneAuth *ibclient.ZoneAuth assert.Equal(t, providerCfg.findZone(zones, "example.com").Fqdn, "example.com") @@ -677,7 +791,7 @@ func TestInfobloxReverseZones(t *testing.T) { mockInfobloxObjects: &[]ibclient.IBObject{}, } - providerCfg := newInfobloxProvider(endpoint.NewDomainFilter([]string{"example.com", "1.2.3.0/24", "10.0.0.0/8"}), provider.NewZoneIDFilter([]string{""}), true, false, &client) + providerCfg := newInfobloxProvider(endpoint.NewDomainFilter([]string{"example.com", "1.2.3.0/24", "10.0.0.0/8"}), provider.NewZoneIDFilter([]string{""}), "", true, false, &client) zones, _ := providerCfg.zones() var emptyZoneAuth *ibclient.ZoneAuth assert.Equal(t, providerCfg.findReverseZone(zones, "nomatch-example.com"), emptyZoneAuth) @@ -738,7 +852,6 @@ func TestExtendedRequestNameRegExBuilder(t *testing.T) { assert.True(t, req.URL.Query().Get("name~") == "") } - func TestExtendedRequestMaxResultsBuilder(t *testing.T) { hostCfg := ibclient.HostConfig{ Host: "localhost", @@ -774,7 +887,7 @@ func TestGetObject(t *testing.T) { requestor := mockRequestor{} client, _ := ibclient.NewConnector(hostCfg, authCfg, transportConfig, requestBuilder, &requestor) - providerConfig := newInfobloxProvider(endpoint.NewDomainFilter([]string{"mysite.com"}), provider.NewZoneIDFilter([]string{""}), true, true, client) + providerConfig := newInfobloxProvider(endpoint.NewDomainFilter([]string{"mysite.com"}), provider.NewZoneIDFilter([]string{""}), "", true, true, client) providerConfig.deleteRecords(infobloxChangeMap{ "myzone.com": []*endpoint.Endpoint{