Skip to content

Commit

Permalink
Use compute v1 api to specify network tier
Browse files Browse the repository at this point in the history 
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 <saverioproto@google.com>
  • Loading branch information
@zioproto
zioproto committed Feb 25, 2020
1 parent 4e79344 commit bdc54eb
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 64 deletions.
61 changes: 18 additions & 43 deletions staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_external.go
View file
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Expand Down
41 changes: 20 additions & 21 deletions staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_external_test.go
View file
Expand Up @@ -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"
Expand Down Expand Up @@ -103,9 +102,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)
})
}
}
Expand All @@ -119,14 +118,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,
},
Expand All @@ -149,7 +148,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,
},
} {
Expand All @@ -158,7 +157,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))
Expand All @@ -180,11 +179,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",
Expand All @@ -197,15 +196,15 @@ 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",
IPProtocol: "TCP",
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"),
},
},
} {
Expand All @@ -219,9 +218,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)
})
}
}
Expand All @@ -240,35 +239,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.
Expand Down Expand Up @@ -428,13 +427,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)
Expand Down

0 comments on commit bdc54eb

Please sign in to comment.