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.

Sign up for GitHub

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

AKS: enable isVnetManaged, add caching #2543

Merged
merged 1 commit into from
Aug 24, 2022
Merged

AKS: enable isVnetManaged, add caching #2543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 18 additions & 1 deletion azure/scope/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ type ClusterScopeParams struct {
Client client.Client
Cluster *clusterv1.Cluster
AzureCluster *infrav1.AzureCluster
Cache *ClusterCache
}

// NewClusterScope creates a new Scope from the supplied parameters.
Expand Down Expand Up @@ -87,6 +88,10 @@ func NewClusterScope(ctx context.Context, params ClusterScopeParams) (*ClusterSc
}
}

if params.Cache == nil {
CecileRobertMichon marked this conversation as resolved.
Show resolved Hide resolved
params.Cache = &ClusterCache{}
}

helper, err := patch.NewHelper(params.AzureCluster, params.Client)
if err != nil {
return nil, errors.Errorf("failed to init patch helper: %v", err)
Expand All @@ -98,19 +103,26 @@ func NewClusterScope(ctx context.Context, params ClusterScopeParams) (*ClusterSc
Cluster: params.Cluster,
AzureCluster: params.AzureCluster,
patchHelper: helper,
cache: params.Cache,
}, nil
}

// ClusterScope defines the basic context for an actuator to operate upon.
type ClusterScope struct {
Client client.Client
patchHelper *patch.Helper
cache *ClusterCache

AzureClients
Cluster *clusterv1.Cluster
AzureCluster *infrav1.AzureCluster
}

// ClusterCache stores ClusterCache data locally so we don't have to hit the API multiple times within the same reconcile loop.
type ClusterCache struct {
isVnetManaged *bool
}

// BaseURI returns the Azure ResourceManagerEndpoint.
func (s *ClusterScope) BaseURI() string {
return s.ResourceManagerEndpoint
Expand Down Expand Up @@ -524,7 +536,12 @@ func (s *ClusterScope) Vnet() *infrav1.VnetSpec {

// IsVnetManaged returns true if the vnet is managed.
func (s *ClusterScope) IsVnetManaged() bool {
return s.Vnet().ID == "" || s.Vnet().Tags.HasOwned(s.ClusterName())
if s.cache.isVnetManaged != nil {
return to.Bool(s.cache.isVnetManaged)
}
isVnetManaged := s.Vnet().ID == "" || s.Vnet().Tags.HasOwned(s.ClusterName())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should change this behavior to be the same as managed clusters eventually, although I agree this is the least dangerous change for now since it preserves existing behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was honestly a bit confused when implementing this for clusters, so glad to hear that suggestion.

I can throw up really quick PR after this one lands so that the scope of this PR leans a bit more towards managed clusters only.

s.cache.isVnetManaged = to.BoolPtr(isVnetManaged)
return isVnetManaged
}

// IsIPv6Enabled returns true if IPv6 is enabled.
Expand Down
127 changes: 127 additions & 0 deletions azure/scope/cluster_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -793,6 +793,7 @@ func TestRouteTableSpecs(t *testing.T) {
},
},
},
cache: &ClusterCache{},
},
want: nil,
},
Expand Down Expand Up @@ -828,6 +829,7 @@ func TestRouteTableSpecs(t *testing.T) {
},
},
},
cache: &ClusterCache{},
},
want: []azure.ResourceSpecGetter{
&routetables.RouteTableSpec{
Expand Down Expand Up @@ -875,6 +877,7 @@ func TestNatGatewaySpecs(t *testing.T) {
},
},
},
cache: &ClusterCache{},
},
want: nil,
},
Expand Down Expand Up @@ -922,6 +925,7 @@ func TestNatGatewaySpecs(t *testing.T) {
},
},
},
cache: &ClusterCache{},
},
want: []azure.ResourceSpecGetter{
&natgateways.NatGatewaySpec{
Expand Down Expand Up @@ -999,6 +1003,7 @@ func TestNatGatewaySpecs(t *testing.T) {
},
},
},
cache: &ClusterCache{},
},
want: []azure.ResourceSpecGetter{
&natgateways.NatGatewaySpec{
Expand Down Expand Up @@ -1075,6 +1080,7 @@ func TestNatGatewaySpecs(t *testing.T) {
},
},
},
cache: &ClusterCache{},
},
want: []azure.ResourceSpecGetter{
&natgateways.NatGatewaySpec{
Expand Down Expand Up @@ -1154,6 +1160,7 @@ func TestNSGSpecs(t *testing.T) {
},
},
},
cache: &ClusterCache{},
},
want: []azure.ResourceSpecGetter{
&securitygroups.NSGSpec{
Expand Down Expand Up @@ -1199,6 +1206,7 @@ func TestSubnetSpecs(t *testing.T) {
},
},
},
cache: &ClusterCache{},
},
want: []azure.ResourceSpecGetter{},
},
Expand Down Expand Up @@ -1260,6 +1268,7 @@ func TestSubnetSpecs(t *testing.T) {
},
},
},
cache: &ClusterCache{},
},
want: []azure.ResourceSpecGetter{
&subnets.SubnetSpec{
Expand Down Expand Up @@ -1362,6 +1371,7 @@ func TestSubnetSpecs(t *testing.T) {
},
},
},
cache: &ClusterCache{},
},
want: []azure.ResourceSpecGetter{
&subnets.SubnetSpec{
Expand Down Expand Up @@ -1404,6 +1414,122 @@ func TestSubnetSpecs(t *testing.T) {
}
}

func TestIsVnetManaged(t *testing.T) {
tests := []struct {
name string
clusterScope ClusterScope
want bool
}{
{
name: "VNET ID is empty",
clusterScope: ClusterScope{
Cluster: &clusterv1.Cluster{
ObjectMeta: metav1.ObjectMeta{
Name: "my-cluster",
},
},
AzureCluster: &infrav1.AzureCluster{
Spec: infrav1.AzureClusterSpec{
NetworkSpec: infrav1.NetworkSpec{
Vnet: infrav1.VnetSpec{
ID: "",
},
},
},
},
cache: &ClusterCache{},
},
want: true,
},
{
name: "Wrong tags",
clusterScope: ClusterScope{
Cluster: &clusterv1.Cluster{
ObjectMeta: metav1.ObjectMeta{
Name: "my-cluster",
},
},
AzureCluster: &infrav1.AzureCluster{
Spec: infrav1.AzureClusterSpec{
NetworkSpec: infrav1.NetworkSpec{
Vnet: infrav1.VnetSpec{
ID: "my-id",
VnetClassSpec: infrav1.VnetClassSpec{Tags: map[string]string{
"key": "value",
}},
},
},
},
},
cache: &ClusterCache{},
},
want: false,
},
{
name: "Has owning tags",
clusterScope: ClusterScope{
Cluster: &clusterv1.Cluster{
ObjectMeta: metav1.ObjectMeta{
Name: "my-cluster",
},
},
AzureCluster: &infrav1.AzureCluster{
Spec: infrav1.AzureClusterSpec{
NetworkSpec: infrav1.NetworkSpec{
Vnet: infrav1.VnetSpec{
ID: "my-id",
VnetClassSpec: infrav1.VnetClassSpec{Tags: map[string]string{
"sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": "owned",
}},
},
},
},
},
cache: &ClusterCache{},
},
want: true,
},
{
name: "Has cached value of false",
clusterScope: ClusterScope{
AzureCluster: &infrav1.AzureCluster{
Spec: infrav1.AzureClusterSpec{},
},
cache: &ClusterCache{
isVnetManaged: to.BoolPtr(false),
},
},
want: false,
},
{
name: "Has cached value of true",
clusterScope: ClusterScope{
AzureCluster: &infrav1.AzureCluster{
Spec: infrav1.AzureClusterSpec{},
},
cache: &ClusterCache{
isVnetManaged: to.BoolPtr(true),
},
},
want: true,
},
}

for _, tt := range tests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
got := tt.clusterScope.IsVnetManaged()
if !reflect.DeepEqual(got, tt.want) {
t.Errorf("IsVnetManaged() = \n%t, want \n%t", got, tt.want)
}
if to.Bool(tt.clusterScope.cache.isVnetManaged) != got {
t.Errorf("IsVnetManaged() = \n%t, cache = \n%t", got, to.Bool(tt.clusterScope.cache.isVnetManaged))
}
})
}
}

func TestAzureBastionSpec(t *testing.T) {
tests := []struct {
name string
Expand Down Expand Up @@ -1510,6 +1636,7 @@ func TestAzureBastionSpec(t *testing.T) {
},
},
},
cache: &ClusterCache{},
},
want: &bastionhosts.AzureBastionSpec{
Name: "fake-azure-bastion-1",
Expand Down
28 changes: 27 additions & 1 deletion azure/scope/managedcontrolplane.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"strings"

"github.com/Azure/go-autorest/autorest"
"github.com/Azure/go-autorest/autorest/to"
"github.com/pkg/errors"
"golang.org/x/mod/semver"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -51,6 +52,7 @@ type ManagedControlPlaneScopeParams struct {
Cluster *clusterv1.Cluster
ControlPlane *infrav1exp.AzureManagedControlPlane
ManagedMachinePools []ManagedMachinePool
Cache *ManagedControlPlaneCache
}

// NewManagedControlPlaneScope creates a new Scope from the supplied parameters.
Expand Down Expand Up @@ -82,6 +84,10 @@ func NewManagedControlPlaneScope(ctx context.Context, params ManagedControlPlane
}
}

if params.Cache == nil {
params.Cache = &ManagedControlPlaneCache{}
}

helper, err := patch.NewHelper(params.ControlPlane, params.Client)
if err != nil {
return nil, errors.Wrap(err, "failed to init patch helper")
Expand All @@ -94,6 +100,7 @@ func NewManagedControlPlaneScope(ctx context.Context, params ManagedControlPlane
ControlPlane: params.ControlPlane,
ManagedMachinePools: params.ManagedMachinePools,
patchHelper: helper,
cache: params.Cache,
}, nil
}

Expand All @@ -102,13 +109,19 @@ type ManagedControlPlaneScope struct {
Client client.Client
patchHelper *patch.Helper
kubeConfigData []byte
cache *ManagedControlPlaneCache

AzureClients
Cluster *clusterv1.Cluster
ControlPlane *infrav1exp.AzureManagedControlPlane
ManagedMachinePools []ManagedMachinePool
}

// ManagedControlPlaneCache stores ManagedControlPlane data locally so we don't have to hit the API multiple times within the same reconcile loop.
type ManagedControlPlaneCache struct {
jackfrancis marked this conversation as resolved.
Show resolved Hide resolved
isVnetManaged *bool
}

// ResourceGroup returns the managed control plane's resource group.
func (s *ManagedControlPlaneScope) ResourceGroup() string {
if s.ControlPlane == nil {
Expand Down Expand Up @@ -328,7 +341,20 @@ func (s *ManagedControlPlaneScope) IsIPv6Enabled() bool {

// IsVnetManaged returns true if the vnet is managed.
func (s *ManagedControlPlaneScope) IsVnetManaged() bool {
return true
if s.cache.isVnetManaged != nil {
return to.Bool(s.cache.isVnetManaged)
}
// TODO refactor `IsVnetManaged` so that it is able to use an upstream context
// see https://github.com/kubernetes-sigs/cluster-api-provider-azure/issues/2581
ctx := context.Background()
CecileRobertMichon marked this conversation as resolved.
Show resolved Hide resolved
ctx, log, done := tele.StartSpanWithLogger(ctx, "scope.ManagedControlPlaneScope.IsVnetManaged")
defer done()
isManaged, err := virtualnetworks.New(s).IsManaged(ctx)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In cluster.go you called this variable isVnetManaged.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's nothing mysterious here, I'm just preferring to use a lowercase local var to capture the response from the method name that we're calling.

if err != nil {
log.Error(err, "Unable to determine if ManagedControlPlaneScope VNET is managed by capz", "AzureManagedCluster", s.ClusterName())
}
s.cache.isVnetManaged = to.BoolPtr(isManaged)
return isManaged
jackfrancis marked this conversation as resolved.
Show resolved Hide resolved
}

// APIServerLBName returns the API Server LB spec.
Expand Down
Loading