From efa412f0c1c93c6611c70d83e9ba8177d04e4ff4 Mon Sep 17 00:00:00 2001 From: Saverio Proto Date: Mon, 24 Feb 2020 21:00:35 +0100 Subject: [PATCH] Use compute v1 api to specify network tier Drop the use of the alpha api for operations that are supported by compute v1 The switch/case logic was wrong because the user can set the default tier for a project: https://cloud.google.com/network-tiers/docs/using-network-service-tiers#setting_the_tier_for_all_resources_in_a_project The assumption that the default tier is always PREMIUM is wrong This patch uses the explicit network tier when possible, or it falls back to the project default. Signed-off-by: Saverio Proto --- .../gce/gce_loadbalancer_external.go | 61 ++++++------------- .../gce/gce_loadbalancer_external_test.go | 41 ++++++------- 2 files changed, 38 insertions(+), 64 deletions(-) diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_external.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_external.go index ffb6d985d98c..21cc74dad094 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_external.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_external.go @@ -33,7 +33,6 @@ import ( servicehelpers "k8s.io/cloud-provider/service/helpers" utilnet "k8s.io/utils/net" - computealpha "google.golang.org/api/compute/v0.alpha" compute "google.golang.org/api/compute/v1" "k8s.io/klog" ) @@ -932,30 +931,18 @@ func createForwardingRule(s CloudForwardingRuleService, name, serviceName, regio desc := makeServiceDescription(serviceName) ipProtocol := string(ports[0].Protocol) - switch netTier { - case cloud.NetworkTierPremium: - rule := &compute.ForwardingRule{ - Name: name, - Description: desc, - IPAddress: ipAddress, - IPProtocol: ipProtocol, - PortRange: portRange, - Target: target, - } - err = s.CreateRegionForwardingRule(rule, region) - default: - rule := &computealpha.ForwardingRule{ - Name: name, - Description: desc, - IPAddress: ipAddress, - IPProtocol: ipProtocol, - PortRange: portRange, - Target: target, - NetworkTier: netTier.ToGCEValue(), - } - err = s.CreateAlphaRegionForwardingRule(rule, region) + rule := &compute.ForwardingRule{ + Name: name, + Description: desc, + IPAddress: ipAddress, + IPProtocol: ipProtocol, + PortRange: portRange, + Target: target, + NetworkTier: netTier.ToGCEValue(), } + err = s.CreateRegionForwardingRule(rule, region) + if err != nil && !isHTTPErrorCode(err, http.StatusConflict) { return err } @@ -1045,27 +1032,15 @@ func ensureStaticIP(s CloudAddressService, name, serviceName, region, existingIP desc := makeServiceDescription(serviceName) var creationErr error - switch netTier { - case cloud.NetworkTierPremium: - addressObj := &compute.Address{ - Name: name, - Description: desc, - } - if existingIP != "" { - addressObj.Address = existingIP - } - creationErr = s.ReserveRegionAddress(addressObj, region) - default: - addressObj := &computealpha.Address{ - Name: name, - Description: desc, - NetworkTier: netTier.ToGCEValue(), - } - if existingIP != "" { - addressObj.Address = existingIP - } - creationErr = s.ReserveAlphaRegionAddress(addressObj, region) + addressObj := &compute.Address{ + Name: name, + Description: desc, + NetworkTier: netTier.ToGCEValue(), + } + if existingIP != "" { + addressObj.Address = existingIP } + creationErr = s.ReserveRegionAddress(addressObj, region) if creationErr != nil { // GCE returns StatusConflict if the name conflicts; it returns diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_external_test.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_external_test.go index 7efbbca69e97..2a6b75d77deb 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_external_test.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_external_test.go @@ -26,7 +26,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - computealpha "google.golang.org/api/compute/v0.alpha" compute "google.golang.org/api/compute/v1" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud" @@ -96,9 +95,9 @@ func TestEnsureStaticIPWithTier(t *testing.T) { assert.NotEqual(t, ip, "") // Get the Address from the fake address service and verify that the tier // is set correctly. - alphaAddr, err := s.GetAlphaRegionAddress(tc.name, s.region) + Addr, err := s.GetRegionAddress(tc.name, s.region) require.NoError(t, err) - assert.Equal(t, tc.expected, alphaAddr.NetworkTier) + assert.Equal(t, tc.expected, Addr.NetworkTier) }) } } @@ -112,14 +111,14 @@ func TestVerifyRequestedIP(t *testing.T) { requestedIP string fwdRuleIP string netTier cloud.NetworkTier - addrList []*computealpha.Address + addrList []*compute.Address expectErr bool expectUserOwned bool }{ "requested IP exists": { requestedIP: "1.1.1.1", netTier: cloud.NetworkTierPremium, - addrList: []*computealpha.Address{{Name: "foo", Address: "1.1.1.1", NetworkTier: "PREMIUM"}}, + addrList: []*compute.Address{{Name: "foo", Address: "1.1.1.1", NetworkTier: "PREMIUM"}}, expectErr: false, expectUserOwned: true, }, @@ -142,7 +141,7 @@ func TestVerifyRequestedIP(t *testing.T) { "requested IP exists, but network tier does not match": { requestedIP: "1.1.1.1", netTier: cloud.NetworkTierStandard, - addrList: []*computealpha.Address{{Name: "foo", Address: "1.1.1.1", NetworkTier: "PREMIUM"}}, + addrList: []*compute.Address{{Name: "foo", Address: "1.1.1.1", NetworkTier: "PREMIUM"}}, expectErr: true, }, } { @@ -151,7 +150,7 @@ func TestVerifyRequestedIP(t *testing.T) { require.NoError(t, err) for _, addr := range tc.addrList { - s.ReserveAlphaRegionAddress(addr, s.region) + s.ReserveRegionAddress(addr, s.region) } isUserOwnedIP, err := verifyUserRequestedIP(s, s.region, tc.requestedIP, tc.fwdRuleIP, lbRef, tc.netTier) assert.Equal(t, tc.expectErr, err != nil, fmt.Sprintf("err: %v", err)) @@ -173,11 +172,11 @@ func TestCreateForwardingRuleWithTier(t *testing.T) { for desc, tc := range map[string]struct { netTier cloud.NetworkTier - expectedRule *computealpha.ForwardingRule + expectedRule *compute.ForwardingRule }{ "Premium tier": { netTier: cloud.NetworkTierPremium, - expectedRule: &computealpha.ForwardingRule{ + expectedRule: &compute.ForwardingRule{ Name: "lb-1", Description: `{"kubernetes.io/service-name":"foo-svc"}`, IPAddress: "1.1.1.1", @@ -190,7 +189,7 @@ func TestCreateForwardingRuleWithTier(t *testing.T) { }, "Standard tier": { netTier: cloud.NetworkTierStandard, - expectedRule: &computealpha.ForwardingRule{ + expectedRule: &compute.ForwardingRule{ Name: "lb-2", Description: `{"kubernetes.io/service-name":"foo-svc"}`, IPAddress: "2.2.2.2", @@ -198,7 +197,7 @@ func TestCreateForwardingRuleWithTier(t *testing.T) { PortRange: "123-123", Target: target, NetworkTier: "STANDARD", - SelfLink: fmt.Sprintf(baseLinkURL, "alpha", vals.ProjectID, vals.Region, "lb-2"), + SelfLink: fmt.Sprintf(baseLinkURL, "v1", vals.ProjectID, vals.Region, "lb-2"), }, }, } { @@ -212,9 +211,9 @@ func TestCreateForwardingRuleWithTier(t *testing.T) { err = createForwardingRule(s, lbName, serviceName, s.region, ipAddr, target, ports, tc.netTier) assert.NoError(t, err) - alphaRule, err := s.GetAlphaRegionForwardingRule(lbName, s.region) + Rule, err := s.GetRegionForwardingRule(lbName, s.region) assert.NoError(t, err) - assert.Equal(t, tc.expectedRule, alphaRule) + assert.Equal(t, tc.expectedRule, Rule) }) } } @@ -233,35 +232,35 @@ func TestDeleteAddressWithWrongTier(t *testing.T) { for desc, tc := range map[string]struct { addrName string netTier cloud.NetworkTier - addrList []*computealpha.Address + addrList []*compute.Address expectDelete bool }{ "Network tiers (premium) match; do nothing": { addrName: "foo1", netTier: cloud.NetworkTierPremium, - addrList: []*computealpha.Address{{Name: "foo1", Address: "1.1.1.1", NetworkTier: "PREMIUM"}}, + addrList: []*compute.Address{{Name: "foo1", Address: "1.1.1.1", NetworkTier: "PREMIUM"}}, }, "Network tiers (standard) match; do nothing": { addrName: "foo2", netTier: cloud.NetworkTierStandard, - addrList: []*computealpha.Address{{Name: "foo2", Address: "1.1.1.2", NetworkTier: "STANDARD"}}, + addrList: []*compute.Address{{Name: "foo2", Address: "1.1.1.2", NetworkTier: "STANDARD"}}, }, "Wrong network tier (standard); delete address": { addrName: "foo3", netTier: cloud.NetworkTierPremium, - addrList: []*computealpha.Address{{Name: "foo3", Address: "1.1.1.3", NetworkTier: "STANDARD"}}, + addrList: []*compute.Address{{Name: "foo3", Address: "1.1.1.3", NetworkTier: "STANDARD"}}, expectDelete: true, }, "Wrong network tier (premium); delete address": { addrName: "foo4", netTier: cloud.NetworkTierStandard, - addrList: []*computealpha.Address{{Name: "foo4", Address: "1.1.1.4", NetworkTier: "PREMIUM"}}, + addrList: []*compute.Address{{Name: "foo4", Address: "1.1.1.4", NetworkTier: "PREMIUM"}}, expectDelete: true, }, } { t.Run(desc, func(t *testing.T) { for _, addr := range tc.addrList { - s.ReserveAlphaRegionAddress(addr, s.region) + s.ReserveRegionAddress(addr, s.region) } // Sanity check to ensure we inject the right address. @@ -421,13 +420,13 @@ func TestLoadBalancerWrongTierResourceDeletion(t *testing.T) { ) require.NoError(t, err) - addressObj := &computealpha.Address{ + addressObj := &compute.Address{ Name: lbName, Description: serviceName.String(), NetworkTier: cloud.NetworkTierStandard.ToGCEValue(), } - err = gce.ReserveAlphaRegionAddress(addressObj, gce.region) + err = gce.ReserveRegionAddress(addressObj, gce.region) require.NoError(t, err) _, err = createExternalLoadBalancer(gce, svc, []string{"test-node-1"}, vals.ClusterName, vals.ClusterID, vals.ZoneName)