From 069062365aa79880621058065c882ae0dbf90e0d Mon Sep 17 00:00:00 2001 From: Ashley Gau Date: Fri, 25 May 2018 14:19:00 -0700 Subject: [PATCH 1/3] use fakeGCECloud instead of gce address fakes --- .../providers/gce/cloud/mock/mock.go | 2 +- .../providers/gce/gce_address_manager_test.go | 67 ++--- .../providers/gce/gce_addresses_fakes.go | 239 ------------------ 3 files changed, 39 insertions(+), 269 deletions(-) delete mode 100644 pkg/cloudprovider/providers/gce/gce_addresses_fakes.go diff --git a/pkg/cloudprovider/providers/gce/cloud/mock/mock.go b/pkg/cloudprovider/providers/gce/cloud/mock/mock.go index f0e87cf0ce7..523691c635e 100644 --- a/pkg/cloudprovider/providers/gce/cloud/mock/mock.go +++ b/pkg/cloudprovider/providers/gce/cloud/mock/mock.go @@ -204,7 +204,7 @@ func convertAndInsertAlphaAddress(key *meta.Key, obj gceObject, mAddrs map[meta. errorCode = http.StatusBadRequest } - return false, &googleapi.Error{Code: errorCode, Message: msg} + return true, &googleapi.Error{Code: errorCode, Message: msg} } } diff --git a/pkg/cloudprovider/providers/gce/gce_address_manager_test.go b/pkg/cloudprovider/providers/gce/gce_address_manager_test.go index 3c7d60dc564..3b7409c6c19 100644 --- a/pkg/cloudprovider/providers/gce/gce_address_manager_test.go +++ b/pkg/cloudprovider/providers/gce/gce_address_manager_test.go @@ -26,95 +26,104 @@ import ( ) const testSvcName = "my-service" -const testRegion = "us-central1" const testSubnet = "/projects/x/testRegions/us-central1/testSubnetworks/customsub" const testLBName = "a111111111111111" +var vals = DefaultTestClusterValues() + // TestAddressManagerNoRequestedIP tests the typical case of passing in no requested IP func TestAddressManagerNoRequestedIP(t *testing.T) { - svc := NewFakeCloudAddressService() + svc, err := fakeGCECloud(vals) + require.NoError(t, err) targetIP := "" - mgr := newAddressManager(svc, testSvcName, testRegion, testSubnet, testLBName, targetIP, cloud.SchemeInternal) - testHoldAddress(t, mgr, svc, testLBName, testRegion, targetIP, string(cloud.SchemeInternal)) - testReleaseAddress(t, mgr, svc, testLBName, testRegion) + mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal) + testHoldAddress(t, mgr, svc, testLBName, vals.Region, targetIP, string(cloud.SchemeInternal)) + testReleaseAddress(t, mgr, svc, testLBName, vals.Region) } // TestAddressManagerBasic tests the typical case of reserving and unreserving an address. func TestAddressManagerBasic(t *testing.T) { - svc := NewFakeCloudAddressService() + svc, err := fakeGCECloud(vals) + require.NoError(t, err) targetIP := "1.1.1.1" - mgr := newAddressManager(svc, testSvcName, testRegion, testSubnet, testLBName, targetIP, cloud.SchemeInternal) - testHoldAddress(t, mgr, svc, testLBName, testRegion, targetIP, string(cloud.SchemeInternal)) - testReleaseAddress(t, mgr, svc, testLBName, testRegion) + mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal) + testHoldAddress(t, mgr, svc, testLBName, vals.Region, targetIP, string(cloud.SchemeInternal)) + testReleaseAddress(t, mgr, svc, testLBName, vals.Region) } // TestAddressManagerOrphaned tests the case where the address exists with the IP being equal // to the requested address (forwarding rule or loadbalancer IP). func TestAddressManagerOrphaned(t *testing.T) { - svc := NewFakeCloudAddressService() + svc, err := fakeGCECloud(vals) + require.NoError(t, err) targetIP := "1.1.1.1" addr := &compute.Address{Name: testLBName, Address: targetIP, AddressType: string(cloud.SchemeInternal)} - err := svc.ReserveRegionAddress(addr, testRegion) + err = svc.ReserveRegionAddress(addr, vals.Region) require.NoError(t, err) - mgr := newAddressManager(svc, testSvcName, testRegion, testSubnet, testLBName, targetIP, cloud.SchemeInternal) - testHoldAddress(t, mgr, svc, testLBName, testRegion, targetIP, string(cloud.SchemeInternal)) - testReleaseAddress(t, mgr, svc, testLBName, testRegion) + mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal) + testHoldAddress(t, mgr, svc, testLBName, vals.Region, targetIP, string(cloud.SchemeInternal)) + testReleaseAddress(t, mgr, svc, testLBName, vals.Region) } // TestAddressManagerOutdatedOrphan tests the case where an address exists but points to // an IP other than the forwarding rule or loadbalancer IP. func TestAddressManagerOutdatedOrphan(t *testing.T) { - svc := NewFakeCloudAddressService() + svc, err := fakeGCECloud(vals) + require.NoError(t, err) previousAddress := "1.1.0.0" targetIP := "1.1.1.1" addr := &compute.Address{Name: testLBName, Address: previousAddress, AddressType: string(cloud.SchemeExternal)} - err := svc.ReserveRegionAddress(addr, testRegion) + err = svc.ReserveRegionAddress(addr, vals.Region) require.NoError(t, err) - mgr := newAddressManager(svc, testSvcName, testRegion, testSubnet, testLBName, targetIP, cloud.SchemeInternal) - testHoldAddress(t, mgr, svc, testLBName, testRegion, targetIP, string(cloud.SchemeInternal)) - testReleaseAddress(t, mgr, svc, testLBName, testRegion) + mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal) + testHoldAddress(t, mgr, svc, testLBName, vals.Region, targetIP, string(cloud.SchemeInternal)) + testReleaseAddress(t, mgr, svc, testLBName, vals.Region) } // TestAddressManagerExternallyOwned tests the case where the address exists but isn't // owned by the controller. func TestAddressManagerExternallyOwned(t *testing.T) { - svc := NewFakeCloudAddressService() + svc, err := fakeGCECloud(vals) + require.NoError(t, err) targetIP := "1.1.1.1" addr := &compute.Address{Name: "my-important-address", Address: targetIP, AddressType: string(cloud.SchemeInternal)} - err := svc.ReserveRegionAddress(addr, testRegion) + err = svc.ReserveRegionAddress(addr, vals.Region) require.NoError(t, err) - mgr := newAddressManager(svc, testSvcName, testRegion, testSubnet, testLBName, targetIP, cloud.SchemeInternal) + mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal) ipToUse, err := mgr.HoldAddress() require.NoError(t, err) assert.NotEmpty(t, ipToUse) - _, err = svc.GetRegionAddress(testLBName, testRegion) + ad, err := svc.GetRegionAddress(testLBName, vals.Region) assert.True(t, isNotFound(err)) + require.Nil(t, ad) - testReleaseAddress(t, mgr, svc, testLBName, testRegion) + testReleaseAddress(t, mgr, svc, testLBName, vals.Region) } // TestAddressManagerExternallyOwned tests the case where the address exists but isn't // owned by the controller. However, this address has the wrong type. func TestAddressManagerBadExternallyOwned(t *testing.T) { - svc := NewFakeCloudAddressService() + svc, err := fakeGCECloud(vals) + require.NoError(t, err) targetIP := "1.1.1.1" addr := &compute.Address{Name: "my-important-address", Address: targetIP, AddressType: string(cloud.SchemeExternal)} - err := svc.ReserveRegionAddress(addr, testRegion) + err = svc.ReserveRegionAddress(addr, vals.Region) require.NoError(t, err) - mgr := newAddressManager(svc, testSvcName, testRegion, testSubnet, testLBName, targetIP, cloud.SchemeInternal) - _, err = mgr.HoldAddress() - assert.NotNil(t, err) + mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal) + ad, err := mgr.HoldAddress() + assert.NotNil(t, err) // FIXME + require.Equal(t, ad, "") } func testHoldAddress(t *testing.T, mgr *addressManager, svc CloudAddressService, name, region, targetIP, scheme string) { diff --git a/pkg/cloudprovider/providers/gce/gce_addresses_fakes.go b/pkg/cloudprovider/providers/gce/gce_addresses_fakes.go deleted file mode 100644 index 75dfa571c9c..00000000000 --- a/pkg/cloudprovider/providers/gce/gce_addresses_fakes.go +++ /dev/null @@ -1,239 +0,0 @@ -/* -Copyright 2017 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package gce - -import ( - "bytes" - "encoding/json" - "fmt" - "net/http" - - computealpha "google.golang.org/api/compute/v0.alpha" - computebeta "google.golang.org/api/compute/v0.beta" - compute "google.golang.org/api/compute/v1" - - "k8s.io/kubernetes/pkg/cloudprovider/providers/gce/cloud" -) - -// test - -type FakeCloudAddressService struct { - count int - // reservedAddrs tracks usage of IP addresses - // Key is the IP address as a string - reservedAddrs map[string]bool - // addrsByRegionAndName - // Outer key is for region string; inner key is for address name. - addrsByRegionAndName map[string]map[string]*computealpha.Address -} - -// FakeCloudAddressService Implements CloudAddressService -var _ CloudAddressService = &FakeCloudAddressService{} - -func NewFakeCloudAddressService() *FakeCloudAddressService { - return &FakeCloudAddressService{ - reservedAddrs: make(map[string]bool), - addrsByRegionAndName: make(map[string]map[string]*computealpha.Address), - } -} - -// SetRegionalAddresses sets the addresses of there region. This is used for -// setting the test environment. -func (cas *FakeCloudAddressService) SetRegionalAddresses(region string, addrs []*computealpha.Address) { - // Reset addresses in the region. - cas.addrsByRegionAndName[region] = make(map[string]*computealpha.Address) - - for _, addr := range addrs { - cas.reservedAddrs[addr.Address] = true - cas.addrsByRegionAndName[region][addr.Name] = addr - } -} - -func (cas *FakeCloudAddressService) ReserveAlphaRegionAddress(addr *computealpha.Address, region string) error { - if addr.Address == "" { - addr.Address = fmt.Sprintf("1.2.3.%d", cas.count) - cas.count++ - } - - if addr.AddressType == "" { - addr.AddressType = string(cloud.SchemeExternal) - } - - if cas.reservedAddrs[addr.Address] { - msg := "IP in use" - // When the IP is already in use, this call returns an error code based - // on the type (internal vs external) of the address. This is to be - // consistent with actual GCE API. - switch cloud.LbScheme(addr.AddressType) { - case cloud.SchemeExternal: - return makeGoogleAPIError(http.StatusBadRequest, msg) - default: - return makeGoogleAPIError(http.StatusConflict, msg) - } - } - - if _, exists := cas.addrsByRegionAndName[region]; !exists { - cas.addrsByRegionAndName[region] = make(map[string]*computealpha.Address) - } - - if _, exists := cas.addrsByRegionAndName[region][addr.Name]; exists { - return makeGoogleAPIError(http.StatusConflict, "name in use") - } - - cas.addrsByRegionAndName[region][addr.Name] = addr - cas.reservedAddrs[addr.Address] = true - return nil -} - -func (cas *FakeCloudAddressService) ReserveBetaRegionAddress(addr *computebeta.Address, region string) error { - alphaAddr := convertToAlphaAddress(addr) - return cas.ReserveAlphaRegionAddress(alphaAddr, region) -} - -func (cas *FakeCloudAddressService) ReserveRegionAddress(addr *compute.Address, region string) error { - alphaAddr := convertToAlphaAddress(addr) - return cas.ReserveAlphaRegionAddress(alphaAddr, region) -} - -func (cas *FakeCloudAddressService) GetAlphaRegionAddress(name, region string) (*computealpha.Address, error) { - if _, exists := cas.addrsByRegionAndName[region]; !exists { - return nil, makeGoogleAPINotFoundError("") - } - - if addr, exists := cas.addrsByRegionAndName[region][name]; !exists { - return nil, makeGoogleAPINotFoundError("") - } else { - return addr, nil - } -} - -func (cas *FakeCloudAddressService) GetBetaRegionAddress(name, region string) (*computebeta.Address, error) { - addr, err := cas.GetAlphaRegionAddress(name, region) - if addr != nil { - return convertToBetaAddress(addr), err - } - return nil, err -} - -func (cas *FakeCloudAddressService) GetRegionAddress(name, region string) (*compute.Address, error) { - addr, err := cas.GetAlphaRegionAddress(name, region) - if addr != nil { - return convertToV1Address(addr), err - } - return nil, err -} - -func (cas *FakeCloudAddressService) DeleteRegionAddress(name, region string) error { - if _, exists := cas.addrsByRegionAndName[region]; !exists { - return makeGoogleAPINotFoundError("") - } - - addr, exists := cas.addrsByRegionAndName[region][name] - if !exists { - return makeGoogleAPINotFoundError("") - } - - delete(cas.reservedAddrs, addr.Address) - delete(cas.addrsByRegionAndName[region], name) - return nil -} - -func (cas *FakeCloudAddressService) GetAlphaRegionAddressByIP(region, ipAddress string) (*computealpha.Address, error) { - if _, exists := cas.addrsByRegionAndName[region]; !exists { - return nil, makeGoogleAPINotFoundError("") - } - - for _, addr := range cas.addrsByRegionAndName[region] { - if addr.Address == ipAddress { - return addr, nil - } - } - return nil, makeGoogleAPINotFoundError("") -} - -func (cas *FakeCloudAddressService) GetBetaRegionAddressByIP(name, region string) (*computebeta.Address, error) { - addr, err := cas.GetAlphaRegionAddressByIP(name, region) - if addr != nil { - return convertToBetaAddress(addr), nil - } - return nil, err -} - -func (cas *FakeCloudAddressService) GetRegionAddressByIP(name, region string) (*compute.Address, error) { - addr, err := cas.GetAlphaRegionAddressByIP(name, region) - if addr != nil { - return convertToV1Address(addr), nil - } - return nil, err -} - -func (cas *FakeCloudAddressService) getNetworkTierFromAddress(name, region string) (string, error) { - addr, err := cas.GetAlphaRegionAddress(name, region) - if err != nil { - return "", err - } - return addr.NetworkTier, nil -} - -func convertToV1Address(object gceObject) *compute.Address { - enc, err := object.MarshalJSON() - if err != nil { - panic(fmt.Sprintf("Failed to encode to json: %v", err)) - } - var addr compute.Address - if err := json.Unmarshal(enc, &addr); err != nil { - panic(fmt.Sprintf("Failed to convert GCE apiObject %v to v1 address: %v", object, err)) - } - return &addr -} - -func convertToAlphaAddress(object gceObject) *computealpha.Address { - enc, err := object.MarshalJSON() - if err != nil { - panic(fmt.Sprintf("Failed to encode to json: %v", err)) - } - var addr computealpha.Address - if err := json.Unmarshal(enc, &addr); err != nil { - panic(fmt.Sprintf("Failed to convert GCE apiObject %v to alpha address: %v", object, err)) - } - // Set the default values for the Alpha fields. - addr.NetworkTier = cloud.NetworkTierDefault.ToGCEValue() - return &addr -} - -func convertToBetaAddress(object gceObject) *computebeta.Address { - enc, err := object.MarshalJSON() - if err != nil { - panic(fmt.Sprintf("Failed to encode to json: %v", err)) - } - var addr computebeta.Address - if err := json.Unmarshal(enc, &addr); err != nil { - panic(fmt.Sprintf("Failed to convert GCE apiObject %v to beta address: %v", object, err)) - } - return &addr -} - -func (cas *FakeCloudAddressService) String() string { - var b bytes.Buffer - for region, regAddresses := range cas.addrsByRegionAndName { - b.WriteString(fmt.Sprintf("%v:\n", region)) - for name, addr := range regAddresses { - b.WriteString(fmt.Sprintf(" %v: %v\n", name, addr.Address)) - } - } - return b.String() -} From a96c5f2884739176a545313f670cfb6d57ff1e24 Mon Sep 17 00:00:00 2001 From: Ashley Gau Date: Fri, 25 May 2018 14:33:33 -0700 Subject: [PATCH 2/3] mocks must return true in order to trigger err --- .../providers/gce/cloud/mock/mock.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/cloudprovider/providers/gce/cloud/mock/mock.go b/pkg/cloudprovider/providers/gce/cloud/mock/mock.go index 523691c635e..6a28c9f8e4b 100644 --- a/pkg/cloudprovider/providers/gce/cloud/mock/mock.go +++ b/pkg/cloudprovider/providers/gce/cloud/mock/mock.go @@ -94,7 +94,7 @@ func RemoveInstanceHook(ctx context.Context, key *meta.Key, req *ga.TargetPoolsR func convertAndInsertAlphaForwardingRule(key *meta.Key, obj gceObject, mRules map[meta.Key]*cloud.MockForwardingRulesObj, version meta.Version, projectID string) (bool, error) { if !key.Valid() { - return false, fmt.Errorf("invalid GCE key (%+v)", key) + return true, fmt.Errorf("invalid GCE key (%+v)", key) } if _, ok := mRules[*key]; ok { @@ -102,16 +102,16 @@ func convertAndInsertAlphaForwardingRule(key *meta.Key, obj gceObject, mRules ma Code: http.StatusConflict, Message: fmt.Sprintf("MockForwardingRule %v exists", key), } - return false, err + return true, err } enc, err := obj.MarshalJSON() if err != nil { - return false, err + return true, err } var fwdRule alpha.ForwardingRule if err := json.Unmarshal(enc, &fwdRule); err != nil { - return false, err + return true, err } // Set the default values for the Alpha fields. if fwdRule.NetworkTier == "" { @@ -162,7 +162,7 @@ type AddressAttributes struct { func convertAndInsertAlphaAddress(key *meta.Key, obj gceObject, mAddrs map[meta.Key]*cloud.MockAddressesObj, version meta.Version, projectID string, addressAttrs AddressAttributes) (bool, error) { if !key.Valid() { - return false, fmt.Errorf("invalid GCE key (%+v)", key) + return true, fmt.Errorf("invalid GCE key (%+v)", key) } if _, ok := mAddrs[*key]; ok { @@ -170,16 +170,16 @@ func convertAndInsertAlphaAddress(key *meta.Key, obj gceObject, mAddrs map[meta. Code: http.StatusConflict, Message: fmt.Sprintf("MockAddresses %v exists", key), } - return false, err + return true, err } enc, err := obj.MarshalJSON() if err != nil { - return false, err + return true, err } var addr alpha.Address if err := json.Unmarshal(enc, &addr); err != nil { - return false, err + return true, err } // Set default address type if not present. From cf393d7a7bc68338a072cc93353c356ed7462966 Mon Sep 17 00:00:00 2001 From: Ashley Gau Date: Fri, 25 May 2018 14:39:27 -0700 Subject: [PATCH 3/3] remove gce_address_fakes.go from BUILD file --- pkg/cloudprovider/providers/gce/BUILD | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/cloudprovider/providers/gce/BUILD b/pkg/cloudprovider/providers/gce/BUILD index 86b2c9e6c67..0185082ebca 100644 --- a/pkg/cloudprovider/providers/gce/BUILD +++ b/pkg/cloudprovider/providers/gce/BUILD @@ -13,7 +13,6 @@ go_library( "gce.go", "gce_address_manager.go", "gce_addresses.go", - "gce_addresses_fakes.go", "gce_alpha.go", "gce_annotations.go", "gce_backendservice.go",