Skip to content

Commit

Permalink
Users can "Allow" or "Deny" to the security rules
Browse files Browse the repository at this point in the history 
- Users can "Allow" or "Deny" to the security rules of the Azure Network Security Group (NSG) that is associated with the Azure Cluster.
  • Loading branch information
@nawazkh
nawazkh committed Aug 30, 2023
1 parent e9b10cd commit e3696bd
Show file tree
Hide file tree
Showing 10 changed files with 270 additions and 8 deletions.
94 changes: 94 additions & 0 deletions api/v1beta1/azurecluster_default_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -696,6 +696,7 @@ func TestSubnetDefaults(t *testing.T) {
DestinationPorts: ptr.To("*"),
Source: ptr.To("*"),
Destination: ptr.To("*"),
Action: SecurityRuleActionAllow,
},
},
},
Expand Down Expand Up @@ -732,6 +733,99 @@ func TestSubnetDefaults(t *testing.T) {
Source: ptr.To("*"),
Destination: ptr.To("*"),
Direction: SecurityRuleDirectionInbound,
Action: SecurityRuleActionAllow,
},
},
},
Name: "my-custom-sg",
},
},
{
SubnetClassSpec: SubnetClassSpec{
Role: SubnetNode,
CIDRBlocks: []string{DefaultNodeSubnetCIDR},
Name: "cluster-test-node-subnet",
},
SecurityGroup: SecurityGroup{Name: "cluster-test-node-nsg"},
RouteTable: RouteTable{Name: "cluster-test-node-routetable"},
NatGateway: NatGateway{
NatGatewayIP: PublicIPSpec{
Name: "",
},
NatGatewayClassSpec: NatGatewayClassSpec{
Name: "cluster-test-node-natgw",
},
},
},
},
},
},
},
},
{
name: "subnets with custom security group to deny port 49999",
cluster: &AzureCluster{
ObjectMeta: metav1.ObjectMeta{
Name: "cluster-test",
},
Spec: AzureClusterSpec{
NetworkSpec: NetworkSpec{
Subnets: Subnets{
{
SubnetClassSpec: SubnetClassSpec{
Role: "control-plane",
Name: "cluster-test-controlplane-subnet",
},
SecurityGroup: SecurityGroup{
SecurityGroupClass: SecurityGroupClass{
SecurityRules: []SecurityRule{
{
Name: "deny_port_49999",
Description: "deny port 49999",
Protocol: "*",
Priority: 2201,
SourcePorts: ptr.To("*"),
DestinationPorts: ptr.To("*"),
Source: ptr.To("*"),
Destination: ptr.To("*"),
Action: SecurityRuleActionDeny,
},
},
},
Name: "my-custom-sg",
},
},
},
},
},
},
output: &AzureCluster{
ObjectMeta: metav1.ObjectMeta{
Name: "cluster-test",
},
Spec: AzureClusterSpec{
NetworkSpec: NetworkSpec{
Subnets: Subnets{
{
SubnetClassSpec: SubnetClassSpec{
Role: "control-plane",
CIDRBlocks: []string{DefaultControlPlaneSubnetCIDR},
Name: "cluster-test-controlplane-subnet",
},
SecurityGroup: SecurityGroup{
SecurityGroupClass: SecurityGroupClass{
SecurityRules: []SecurityRule{
{
Name: "deny_port_49999",
Description: "deny port 49999",
Protocol: "*",
Priority: 2201,
SourcePorts: ptr.To("*"),
DestinationPorts: ptr.To("*"),
Source: ptr.To("*"),
Destination: ptr.To("*"),
Direction: SecurityRuleDirectionInbound,
Action: SecurityRuleActionDeny,
},
},
},
Expand Down
16 changes: 16 additions & 0 deletions api/v1beta1/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,17 @@ const (
SecurityRuleDirectionOutbound = SecurityRuleDirection("Outbound")
)

// SecurityRuleAccess defines the action type for a security group rule.
type SecurityRuleAccess string

const (
// SecurityRuleActionAllow allows traffic defined in the rule.
SecurityRuleActionAllow SecurityRuleAccess = "Allow"

// SecurityRuleActionDeny denies traffic defined in the rule.
SecurityRuleActionDeny SecurityRuleAccess = "Deny"
)

// SecurityRule defines an Azure security rule for security groups.
type SecurityRule struct {
// Name is a unique name within the network security group.
Expand Down Expand Up @@ -297,6 +308,11 @@ type SecurityRule struct {
// Destination is the destination address prefix. CIDR or destination IP range. Asterix '*' can also be used to match all source IPs. Default tags such as 'VirtualNetwork', 'AzureLoadBalancer' and 'Internet' can also be used.
// +optional
Destination *string `json:"destination,omitempty"`
// Action specifies whether network traffic is allowed or denied. Can either be "Allow" or "Deny".
// +kubebuilder:default=Allow
// +kubebuilder:validation:Enum=Allow;Deny
//+optional
Action SecurityRuleAccess `json:"action"`
}

// SecurityRules is a slice of Azure security rules for security groups.
Expand Down
2 changes: 1 addition & 1 deletion azure/converters/rules.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func SecurityRuleToSDK(rule infrav1.SecurityRule) network.SecurityRule {
SourcePortRange: rule.SourcePorts,
DestinationAddressPrefix: rule.Destination,
DestinationPortRange: rule.DestinationPorts,
Access: network.SecurityRuleAccessAllow,
Access: network.SecurityRuleAccess(rule.Action),
Priority: ptr.To[int32](rule.Priority),
},
}
Expand Down
2 changes: 2 additions & 0 deletions azure/scope/cluster.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -932,6 +932,7 @@ func (s *ClusterScope) SetControlPlaneSecurityRules() {
SourcePorts: ptr.To("*"),
Destination: ptr.To("*"),
DestinationPorts: ptr.To("22"),
Action: infrav1.SecurityRuleActionAllow,
},
infrav1.SecurityRule{
Name: "allow_apiserver",
Expand All @@ -943,6 +944,7 @@ func (s *ClusterScope) SetControlPlaneSecurityRules() {
SourcePorts: ptr.To("*"),
Destination: ptr.To("*"),
DestinationPorts: ptr.To(strconv.Itoa(int(s.APIServerPort()))),
Action: infrav1.SecurityRuleActionAllow,
},
}
s.AzureCluster.Spec.NetworkSpec.UpdateControlPlaneSubnet(subnet)
Expand Down
5 changes: 5 additions & 0 deletions azure/services/securitygroups/securitygroups_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ var (
SourcePorts: ptr.To("*"),
Destination: ptr.To("*"),
DestinationPorts: ptr.To("22"),
Action: infrav1.SecurityRuleActionAllow,
}
securityRule2 = infrav1.SecurityRule{
Name: "other_rule",
Expand All @@ -81,6 +82,7 @@ var (
SourcePorts: ptr.To("*"),
Destination: ptr.To("*"),
DestinationPorts: ptr.To("80"),
Action: infrav1.SecurityRuleActionAllow,
}
errFake = errors.New("this is an error")
notDoneError = azure.NewOperationNotDoneError(&infrav1.Future{})
Expand Down Expand Up @@ -296,6 +298,7 @@ var (
SourceAddressPrefix: ptr.To("*"),
Priority: ptr.To[int32](100),
Direction: network.SecurityRuleDirectionInbound,
Access: network.SecurityRuleAccessAllow,
},
}
ruleB = network.SecurityRule{
Expand All @@ -309,6 +312,7 @@ var (
SourceAddressPrefix: ptr.To("*"),
Priority: ptr.To[int32](100),
Direction: network.SecurityRuleDirectionOutbound,
Access: network.SecurityRuleAccessAllow,
},
}
ruleBModified = network.SecurityRule{
Expand All @@ -322,6 +326,7 @@ var (
SourceAddressPrefix: ptr.To("*"),
Priority: ptr.To[int32](100),
Direction: network.SecurityRuleDirectionOutbound,
Access: network.SecurityRuleAccessAllow,
},
}
)
104 changes: 104 additions & 0 deletions azure/services/securitygroups/spec_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ var (
SourcePorts: ptr.To("*"),
Destination: ptr.To("*"),
DestinationPorts: ptr.To("22"),
Action: infrav1.SecurityRuleActionAllow,
}
otherRule = infrav1.SecurityRule{
Name: "other_rule",
Expand All @@ -49,6 +50,7 @@ var (
SourcePorts: ptr.To("*"),
Destination: ptr.To("*"),
DestinationPorts: ptr.To("80"),
Action: infrav1.SecurityRuleActionAllow,
}
customRule = infrav1.SecurityRule{
Name: "custom_rule",
Expand All @@ -60,6 +62,19 @@ var (
SourcePorts: ptr.To("*"),
Destination: ptr.To("*"),
DestinationPorts: ptr.To("80"),
Action: infrav1.SecurityRuleActionAllow,
}
denyRule = infrav1.SecurityRule{
Name: "deny_rule",
Description: "Deny Rule",
Priority: 510,
Protocol: infrav1.SecurityGroupProtocolTCP,
Direction: infrav1.SecurityRuleDirectionOutbound,
Source: ptr.To("*"),
SourcePorts: ptr.To("*"),
Destination: ptr.To("*"),
DestinationPorts: ptr.To("80"),
Action: infrav1.SecurityRuleActionDeny,
}
)

Expand Down Expand Up @@ -138,6 +153,48 @@ func TestParameters(t *testing.T) {
}))
},
},
{
name: "NSG already exists but missing a rule",
spec: &NSGSpec{
Name: "test-nsg",
Location: "test-location",
SecurityRules: infrav1.SecurityRules{
sshRule,
otherRule,
},
ResourceGroup: "test-group",
ClusterName: "my-cluster",
},
existing: network.SecurityGroup{
Name: ptr.To("test-nsg"),
Location: ptr.To("test-location"),
Etag: ptr.To("fake-etag"),
SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{
SecurityRules: &[]network.SecurityRule{
converters.SecurityRuleToSDK(sshRule),
converters.SecurityRuleToSDK(denyRule),
},
},
},
expect: func(g *WithT, result interface{}) {
g.Expect(result).To(BeAssignableToTypeOf(network.SecurityGroup{}))
g.Expect(result).To(Equal(network.SecurityGroup{
Location: ptr.To("test-location"),
Etag: ptr.To("fake-etag"),
SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{
SecurityRules: &[]network.SecurityRule{
converters.SecurityRuleToSDK(otherRule),
converters.SecurityRuleToSDK(sshRule),
converters.SecurityRuleToSDK(denyRule),
},
},
Tags: map[string]*string{
"sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": ptr.To("owned"),
"Name": ptr.To("test-nsg"),
},
}))
},
},
{
name: "NSG already exists and a rule is deleted",
spec: &NSGSpec{
Expand Down Expand Up @@ -185,6 +242,53 @@ func TestParameters(t *testing.T) {
}))
},
},
{
name: "NSG already exists and a deny rule is deleted",
spec: &NSGSpec{
Name: "test-nsg",
Location: "test-location",
SecurityRules: infrav1.SecurityRules{
sshRule,
customRule,
},
ResourceGroup: "test-group",
ClusterName: "my-cluster",
LastAppliedSecurityRules: map[string]interface{}{
"allow_ssh": sshRule,
"custom_rule": customRule,
"deny_rule": denyRule,
},
},
existing: network.SecurityGroup{
Name: ptr.To("test-nsg"),
Location: ptr.To("test-location"),
Etag: ptr.To("fake-etag"),
SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{
SecurityRules: &[]network.SecurityRule{
converters.SecurityRuleToSDK(sshRule),
converters.SecurityRuleToSDK(customRule),
converters.SecurityRuleToSDK(denyRule),
},
},
},
expect: func(g *WithT, result interface{}) {
g.Expect(result).To(BeAssignableToTypeOf(network.SecurityGroup{}))
g.Expect(result).To(Equal(network.SecurityGroup{
Location: ptr.To("test-location"),
Etag: ptr.To("fake-etag"),
SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{
SecurityRules: &[]network.SecurityRule{
converters.SecurityRuleToSDK(sshRule),
converters.SecurityRuleToSDK(customRule),
},
},
Tags: map[string]*string{
"sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": ptr.To("owned"),
"Name": ptr.To("test-nsg"),
},
}))
},
},
{
name: "NSG already exists and a rule not owned by CAPZ is present",
spec: &NSGSpec{
Expand Down
18 changes: 18 additions & 0 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_azureclusters.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,15 @@ spec:
description: SecurityRule defines an Azure security
rule for security groups.
properties:
action:
default: Allow
description: Action specifies whether network
traffic is allowed or denied. Can either be
"Allow" or "Deny".
enum:
- Allow
- Deny
type: string
description:
description: A description for this rule. Restricted
to 140 chars.
Expand Down Expand Up @@ -1022,6 +1031,15 @@ spec:
description: SecurityRule defines an Azure security
rule for security groups.
properties:
action:
default: Allow
description: Action specifies whether network
traffic is allowed or denied. Can either be
"Allow" or "Deny".
enum:
- Allow
- Deny
type: string
description:
description: A description for this rule. Restricted
to 140 chars.
Expand Down
Loading

0 comments on commit e3696bd

Please sign in to comment.