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

enable users to allow or deny the security rules #3878

Merged
merged 1 commit into from
Aug 31, 2023
Merged

enable users to allow or deny the security rules #3878

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
94 changes: 94 additions & 0 deletions api/v1beta1/azurecluster_default_test.go
View file
Open in desktop
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
Open in desktop
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". Defaults to "Allow".
// +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
Open in desktop
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),
nawazkh marked this conversation as resolved.
Show resolved Hide resolved
Priority: ptr.To[int32](rule.Priority),
},
}
Expand Down
2 changes: 2 additions & 0 deletions azure/scope/cluster.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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". Defaults to "Allow".
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". Defaults to "Allow".
enum:
- Allow
- Deny
type: string
description:
description: A description for this rule. Restricted
to 140 chars.
Expand Down
Loading