Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor ListResources to not require passing the Cluster object #14724

Merged
merged 2 commits into from
Dec 6, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/kops/delete_cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ func RunDeleteCluster(ctx context.Context, f *util.Factory, out io.Writer, optio
}

klog.Info("Looking for cloud resources to delete")
allResources, err := resourceops.ListResources(cloud, cluster, options.Region)
allResources, err := resourceops.ListResources(cloud, cluster)
if err != nil {
return err
}
Expand Down
3 changes: 1 addition & 2 deletions cmd/kops/toolbox_dump.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,7 @@ func RunToolboxDump(ctx context.Context, f commandutils.Factory, out io.Writer,
return err
}

region := "" // Use default
resourceMap, err := resourceops.ListResources(cloud, cluster, region)
resourceMap, err := resourceops.ListResources(cloud, cluster)
if err != nil {
return err
}
Expand Down
10 changes: 6 additions & 4 deletions pkg/resources/aws/aws.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import (
"github.com/aws/aws-sdk-go/service/route53"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/klog/v2"
kopsapi "k8s.io/kops/pkg/apis/kops"
"k8s.io/kops/pkg/dns"
"k8s.io/kops/pkg/featureflag"
"k8s.io/kops/pkg/resources"
"k8s.io/kops/pkg/resources/spotinst"
Expand All @@ -50,8 +50,10 @@ const (

type listFn func(fi.Cloud, string) ([]*resources.Resource, error)

func ListResourcesAWS(cloud awsup.AWSCloud, cluster *kopsapi.Cluster) (map[string]*resources.Resource, error) {
clusterName := cluster.Name
func ListResourcesAWS(cloud awsup.AWSCloud, clusterInfo resources.ClusterInfo) (map[string]*resources.Resource, error) {
clusterName := clusterInfo.Name
clusterUsesNoneDNS := clusterInfo.UsesNoneDNS

resourceTrackers := make(map[string]*resources.Resource)

// These are the functions that are used for looking up
Expand Down Expand Up @@ -87,7 +89,7 @@ func ListResourcesAWS(cloud awsup.AWSCloud, cluster *kopsapi.Cluster) (map[strin
ListEventBridgeRules,
}

if !cluster.IsGossip() && !cluster.UsesNoneDNS() {
if !dns.IsGossipClusterName(clusterName) && !clusterUsesNoneDNS {
// Route 53
listFunctions = append(listFunctions, ListRoute53Records)
}
Expand Down
23 changes: 11 additions & 12 deletions pkg/resources/azure/azure.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (
"github.com/Azure/azure-sdk-for-go/services/network/mgmt/2022-05-01/network"
authz "github.com/Azure/azure-sdk-for-go/services/preview/authorization/mgmt/2020-04-01-preview/authorization"
azureresources "github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2021-04-01/resources"
kopsapi "k8s.io/kops/pkg/apis/kops"
"k8s.io/kops/pkg/resources"
"k8s.io/kops/upup/pkg/fi"
"k8s.io/kops/upup/pkg/fi/cloudup/azure"
Expand All @@ -44,21 +43,21 @@ const (
)

// ListResourcesAzure lists all resources for the cluster by quering Azure.
func ListResourcesAzure(cloud azure.AzureCloud, cluster *kopsapi.Cluster) (map[string]*resources.Resource, error) {
func ListResourcesAzure(cloud azure.AzureCloud, clusterInfo resources.ClusterInfo) (map[string]*resources.Resource, error) {
g := resourceGetter{
cloud: cloud,
cluster: cluster,
cloud: cloud,
clusterInfo: clusterInfo,
}
return g.listResourcesAzure()
}

type resourceGetter struct {
cloud azure.AzureCloud
cluster *kopsapi.Cluster
cloud azure.AzureCloud
clusterInfo resources.ClusterInfo
}

func (g *resourceGetter) resourceGroupName() string {
return g.cluster.AzureResourceGroupName()
return g.clusterInfo.AzureResourceGroupName
}

func (g *resourceGetter) listResourcesAzure() (map[string]*resources.Resource, error) {
Expand Down Expand Up @@ -129,7 +128,7 @@ func (g *resourceGetter) toResourceGroupResource(rg *azureresources.Group) *reso
ID: *rg.Name,
Name: *rg.Name,
Deleter: g.deleteResourceGroup,
Shared: g.cluster.IsSharedAzureResourceGroup(),
Shared: g.clusterInfo.AzureResourceGroupShared,
}
}

Expand Down Expand Up @@ -168,7 +167,7 @@ func (g *resourceGetter) toVirtualNetworkResource(vnet *network.VirtualNetwork)
Name: *vnet.Name,
Deleter: g.deleteVirtualNetwork,
Blocks: []string{toKey(typeResourceGroup, g.resourceGroupName())},
Shared: g.cluster.SharedVPC(),
Shared: g.clusterInfo.AzureNetworkShared,
}
}

Expand Down Expand Up @@ -202,7 +201,7 @@ func (g *resourceGetter) toSubnetResource(subnet *network.Subnet, vnetName strin
toKey(typeVirtualNetwork, vnetName),
toKey(typeResourceGroup, g.resourceGroupName()),
},
Shared: g.cluster.SharedVPC(),
Shared: g.clusterInfo.AzureNetworkShared,
}
}

Expand Down Expand Up @@ -235,7 +234,7 @@ func (g *resourceGetter) toRouteTableResource(rt *network.RouteTable) *resources
Name: *rt.Name,
Deleter: g.deleteRouteTable,
Blocks: []string{toKey(typeResourceGroup, g.resourceGroupName())},
Shared: g.cluster.IsSharedAzureRouteTable(),
Shared: g.clusterInfo.AzureRouteTableShared,
}
}

Expand Down Expand Up @@ -469,7 +468,7 @@ func (g *resourceGetter) deletePublicIPAddress(_ fi.Cloud, r *resources.Resource
// isOwnedByCluster returns true if the resource is owned by the cluster.
func (g *resourceGetter) isOwnedByCluster(tags map[string]*string) bool {
for k, v := range tags {
if k == azure.TagClusterName && *v == g.cluster.Name {
if k == azure.TagClusterName && *v == g.clusterInfo.Name {
return true
}
}
Expand Down
23 changes: 6 additions & 17 deletions pkg/resources/azure/azure_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@ import (
authz "github.com/Azure/azure-sdk-for-go/services/preview/authorization/mgmt/2020-04-01-preview/authorization"
azureresources "github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2021-04-01/resources"
"github.com/Azure/go-autorest/autorest/to"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/kops/pkg/apis/kops"
"k8s.io/kops/pkg/resources"
"k8s.io/kops/upup/pkg/fi/cloudup/azure"
"k8s.io/kops/upup/pkg/fi/cloudup/azuretasks"
Expand Down Expand Up @@ -176,17 +174,10 @@ func TestListResourcesAzure(t *testing.T) {
// Call listResourcesAzure.
g := resourceGetter{
cloud: cloud,
cluster: &kops.Cluster{
ObjectMeta: metav1.ObjectMeta{
Name: clusterName,
},
Spec: kops.ClusterSpec{
CloudProvider: kops.CloudProviderSpec{
Azure: &kops.AzureSpec{
ResourceGroupName: rgName,
},
},
},
clusterInfo: resources.ClusterInfo{
Name: clusterName,
AzureResourceGroupName: rgName,
AzureResourceGroupShared: true,
},
}
actual, err := g.listResourcesAzure()
Expand Down Expand Up @@ -310,10 +301,8 @@ func TestIsOwnedByCluster(t *testing.T) {
for i, tc := range testCases {
t.Run(fmt.Sprintf("test case %d", i), func(t *testing.T) {
g := &resourceGetter{
cluster: &kops.Cluster{
ObjectMeta: metav1.ObjectMeta{
Name: clusterName,
},
clusterInfo: resources.ClusterInfo{
Name: clusterName,
},
}
a := g.isOwnedByCluster(tc.tags)
Expand Down
27 changes: 27 additions & 0 deletions pkg/resources/clusterinfo.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
Copyright 2022 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 resources

type ClusterInfo struct {
Name string
UsesNoneDNS bool
// Azure specific
AzureResourceGroupName string
AzureResourceGroupShared bool
AzureNetworkShared bool
AzureRouteTableShared bool
}
4 changes: 3 additions & 1 deletion pkg/resources/digitalocean/resources.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@ const (

type listFn func(fi.Cloud, string) ([]*resources.Resource, error)

func ListResources(cloud do.DOCloud, clusterName string) (map[string]*resources.Resource, error) {
func ListResources(cloud do.DOCloud, clusterInfo resources.ClusterInfo) (map[string]*resources.Resource, error) {
clusterName := clusterInfo.Name

resourceTrackers := make(map[string]*resources.Resource)

listFunctions := []listFn{
Expand Down
18 changes: 9 additions & 9 deletions pkg/resources/gce/gce.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,13 @@ const maxPrefixTokens = 5
// Maximum length of a GCE route name
const maxGCERouteNameLength = 63

func ListResourcesGCE(gceCloud gce.GCECloud, clusterName string, region string) (map[string]*resources.Resource, error) {
func ListResourcesGCE(gceCloud gce.GCECloud, clusterInfo resources.ClusterInfo) (map[string]*resources.Resource, error) {
clusterName := clusterInfo.Name
clusterUsesNoneDNS := clusterInfo.UsesNoneDNS

ctx := context.TODO()

if region == "" {
region = gceCloud.Region()
}
region := gceCloud.Region()

resources := make(map[string]*resources.Resource)

Expand Down Expand Up @@ -105,7 +106,6 @@ func ListResourcesGCE(gceCloud gce.GCECloud, clusterName string, region string)
d.listForwardingRules,
d.listFirewallRules,
d.listGCEDisks,
d.listGCEDNSZone,
// TODO: Find routes via instances (via instance groups)
d.listAddresses,
d.listSubnets,
Expand All @@ -115,6 +115,10 @@ func ListResourcesGCE(gceCloud gce.GCECloud, clusterName string, region string)
d.listBackendServices,
d.listHealthchecks,
}
if !dns.IsGossipClusterName(clusterName) && !clusterUsesNoneDNS {
listFunctions = append(listFunctions, d.listGCEDNSZone)
}

for _, fn := range listFunctions {
resourceTrackers, err := fn()
if err != nil {
Expand Down Expand Up @@ -1282,10 +1286,6 @@ func (d *clusterDiscoveryGCE) isKopsManagedDNSName(name string) bool {
}

func (d *clusterDiscoveryGCE) listGCEDNSZone() ([]*resources.Resource, error) {
if dns.IsGossipClusterName(d.clusterName) {
return nil, nil
}

var resourceTrackers []*resources.Resource

managedZones, err := d.gceCloud.CloudDNS().ManagedZones().List(d.gceCloud.Project())
Expand Down
4 changes: 3 additions & 1 deletion pkg/resources/hetzner/resources.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,9 @@ const (

type listFn func(fi.Cloud, string) ([]*resources.Resource, error)

func ListResources(cloud hetzner.HetznerCloud, clusterName string) (map[string]*resources.Resource, error) {
func ListResources(cloud hetzner.HetznerCloud, clusterInfo resources.ClusterInfo) (map[string]*resources.Resource, error) {
clusterName := clusterInfo.Name

resourceTrackers := make(map[string]*resources.Resource)

listFunctions := []listFn{
Expand Down
4 changes: 2 additions & 2 deletions pkg/resources/openstack/openstack.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,13 @@ type clusterDiscoveryOS struct {
}

// ListResources lists the OpenStack resources kops manages
func ListResources(cloud openstack.OpenstackCloud, clusterName string) (map[string]*resources.Resource, error) {
func ListResources(cloud openstack.OpenstackCloud, clusterInfo resources.ClusterInfo) (map[string]*resources.Resource, error) {
resources := make(map[string]*resources.Resource)

os := &clusterDiscoveryOS{
cloud: cloud,
osCloud: cloud,
clusterName: clusterName,
clusterName: clusterInfo.Name,
}

listFunctions := []openstackListFn{
Expand Down
24 changes: 16 additions & 8 deletions pkg/resources/ops/collector.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,21 +37,29 @@ import (
)

// ListResources collects the resources from the specified cloud
func ListResources(cloud fi.Cloud, cluster *kops.Cluster, region string) (map[string]*resources.Resource, error) {
clusterName := cluster.Name
func ListResources(cloud fi.Cloud, cluster *kops.Cluster) (map[string]*resources.Resource, error) {
clusterInfo := resources.ClusterInfo{
Name: cluster.Name,
UsesNoneDNS: cluster.UsesNoneDNS(),
}

switch cloud.ProviderID() {
case kops.CloudProviderAWS:
return aws.ListResourcesAWS(cloud.(awsup.AWSCloud), cluster)
return aws.ListResourcesAWS(cloud.(awsup.AWSCloud), clusterInfo)
case kops.CloudProviderDO:
return digitalocean.ListResources(cloud.(clouddo.DOCloud), clusterName)
return digitalocean.ListResources(cloud.(clouddo.DOCloud), clusterInfo)
case kops.CloudProviderGCE:
return gce.ListResourcesGCE(cloud.(cloudgce.GCECloud), clusterName, region)
return gce.ListResourcesGCE(cloud.(cloudgce.GCECloud), clusterInfo)
case kops.CloudProviderHetzner:
return hetzner.ListResources(cloud.(cloudhetzner.HetznerCloud), clusterName)
return hetzner.ListResources(cloud.(cloudhetzner.HetznerCloud), clusterInfo)
case kops.CloudProviderOpenstack:
return openstack.ListResources(cloud.(cloudopenstack.OpenstackCloud), clusterName)
return openstack.ListResources(cloud.(cloudopenstack.OpenstackCloud), clusterInfo)
case kops.CloudProviderAzure:
return azure.ListResourcesAzure(cloud.(cloudazure.AzureCloud), cluster)
clusterInfo.AzureResourceGroupName = cluster.AzureResourceGroupName()
clusterInfo.AzureResourceGroupShared = cluster.IsSharedAzureResourceGroup()
clusterInfo.AzureNetworkShared = cluster.SharedVPC()
clusterInfo.AzureRouteTableShared = cluster.IsSharedAzureRouteTable()
return azure.ListResourcesAzure(cloud.(cloudazure.AzureCloud), clusterInfo)
default:
return nil, fmt.Errorf("delete on clusters on %q not (yet) supported", cloud.ProviderID())
}
Expand Down