Add feature to create a Network Security Group in Azure #200

escortnotice · 2020-11-23T05:51:49Z

Description of your changes

Create a Network Security Group, which can restrict inbound and outgoing traffic using 5tuple rules. Also restrict traffic based on Service tagging and Application Security Group.

Issue: #168

Fixes #

I have:

Read and followed Crossplane's [contribution process].
Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Currently we have tested the code manually on azure cloud, all the configuration files (.yaml) are in the examples folder which can be referred for use.

Signed-off-by: shreya <[email protected]>

… and create.

…le definitions.

Feature nsg unit test to feature msg

hasheddan

@escortnotice thanks for your work here :) I have given a first pass review below, but also wanted to point you to these resources:

Also, I believe you will need an update to crossplane-runtime version here 👍 The errors in CI should give you some indication about other existing issues. Let me know if you have any questions!

hasheddan · 2021-01-08T22:39:27Z

apis/network/v1alpha3/types.go

+
+	Spec   SecurityGroupSpec   `json:"spec"`
+	Status SecurityGroupStatus `json:"status,omitempty"`
+	///Properties SecurityGroupPropertiesFormat   `json:"properties,omitempty"`


Suggested change

///Properties SecurityGroupPropertiesFormat `json:"properties,omitempty"`

We have implemented this change

hasheddan · 2021-01-08T22:40:44Z

apis/network/v1alpha3/types.go

+}
+
+// A SecurityGroupSpec defines the desired state of a SecurityGroup.
+type SecurityGroupSpec struct {


fields that are not imported from crossplane-runtime should be under forProvider struct (ref: https://github.com/crossplane/provider-azure/blob/b55e3a0dabc627726d2f5275d80f662650932d45/apis/cache/v1beta1/redis_types.go#L139)

Just curious to know what is the significance of adding forProvider as it seems to be a level addition to me , and this is also not present in Virtualnetwork Resource.

we have added forProvided in the SecurityGroupSpec Struct also.

hasheddan · 2021-01-08T22:41:15Z

apis/network/v1alpha3/types.go

+// SecurityRulePropertiesFormat security rule resource.
+type SecurityRulePropertiesFormat struct {
+	// Description - A description for this rule. Restricted to 140 chars.
+	Description string `json:"description,omitempty"`


All optional spec fields should be pointer types.

Suggested change

Description string `json:"description,omitempty"`

Description *string `json:"description,omitempty"`

We have implemented this change

hasheddan · 2021-01-08T22:41:42Z

apis/network/v1alpha3/types.go

+	SourcePortRange string `json:"sourcePortRange,omitempty"`
+	// DestinationPortRange - The destination port or range. Integer or range between 0 and 65535. Asterisk '*' can also be used to match all ports.
+	DestinationPortRange string `json:"destinationPortRange,omitempty"`
+	// SourceAddressPrefix - The CIDR or source IP range. Asterisk '*' can also be used to match all source IPs. Default tags such as 'VirtualNetwork', 'AzureLoadBalancer' and 'Internet' can also be used. If this is an ingress rule, specifies where network traffic originates from.


Prefer that comments be wrapped at 80 chars :)

hasheddan · 2021-01-08T22:42:22Z

apis/network/v1alpha3/register.go

 func init() {
 	SchemeBuilder.Register(&VirtualNetwork{}, &VirtualNetworkList{})
 	SchemeBuilder.Register(&Subnet{}, &SubnetList{})
+	SchemeBuilder.Register(&SecurityGroup{}, &SecurityGroupList{})
+	SchemeBuilder.Register(&SecurityRule{})


Guessing there should also be a SecurityRuleList?

Or is it intended that a SecurityRule just be in the spec of a SecurityGroup? If so, it should not be its own object, but it they are separate APIs on Azure then they should be separate Crossplane objects and the SecurityGroup should reference SecurityRule.

Refactored SecurityRule as it is just a part of Security Group not a independent entity.

hasheddan · 2021-01-08T22:47:27Z

pkg/controller/azure.go

@@ -17,6 +17,7 @@ limitations under the License.
 package controller

 import (
+	SecurityGroup "github.com/crossplane/provider-azure/pkg/controller/network/securitygroup"


Suggested change

SecurityGroup "github.com/crossplane/provider-azure/pkg/controller/network/securitygroup"

"github.com/crossplane/provider-azure/pkg/controller/network/securitygroup"

We have implemented this change

hasheddan · 2021-01-08T22:47:40Z

pkg/controller/azure.go

@@ -56,6 +57,7 @@ func Setup(mgr ctrl.Manager, l logging.Logger) error {
 		resourcegroup.Setup,
 		account.Setup,
 		container.Setup,
+		SecurityGroup.Setup,


Suggested change

SecurityGroup.Setup,

securitygroup.Setup,

We have implemented this change

hasheddan · 2021-01-08T22:48:29Z

pkg/controller/network/securitygroup/managed.go

+	//"github.com/crossplane/provider-azure/pkg/clients/network"
+
+	//"github.com/Azure/azure-sdk-for-go/services/network/mgmt/2019-12-01/network/networkapi"


Suggested change

//"github.com/crossplane/provider-azure/pkg/clients/network"

//"github.com/Azure/azure-sdk-for-go/services/network/mgmt/2019-12-01/network/networkapi"

We have implemented this change

hasheddan · 2021-01-08T22:48:59Z

pkg/controller/network/securitygroup/managed.go

+
+type external struct {
+	client networkapi.SecurityGroupsClientAPI
+	//client azurenetwork.SecurityGroupsClient


Suggested change

//client azurenetwork.SecurityGroupsClient

We have implemented this change

hasheddan · 2021-01-08T22:52:43Z

pkg/controller/network/securitygroup/managed_test.go

@@ -0,0 +1,470 @@
+package SecurityGroup
+
+import (


these imports will fail linting https://github.com/crossplane/provider-azure/blob/b55e3a0dabc627726d2f5275d80f662650932d45/.golangci.yml#L38

Sorry I am unable to understand this statement , could you please share more details what changes are expected.

We require imports to be structured in groups separated by a blank line in the following order:

stdlib packages

external packages (i.e. not from crossplane org)

other crossplane org packages (i.e. crossplane-runtime)

provider-azure packages (i.e. from within this repo)

First running:
goimports -local github.com/crossplane/provider-azure -w pkg/controller/network/securitygroup/managed_test.go pkg/controller/network/securitygroup/managed_test.go,
and then running:
goimports -local github.com/crossplane/crossplane-runtime -w pkg/controller/network/securitygroup/managed_test.go pkg/controller/network/securitygroup/managed_test.go,
should put them into shape.

hasheddan · 2021-02-18T12:49:04Z

apis/network/v1alpha3/register.go

 func init() {
 	SchemeBuilder.Register(&VirtualNetwork{}, &VirtualNetworkList{})
 	SchemeBuilder.Register(&Subnet{}, &SubnetList{})
+	SchemeBuilder.Register(&SecurityGroup{}, &SecurityGroupList{})
+	//SchemeBuilder.Register(&SecurityRule{})


Should this be removed? Is it possible to create a SecurityRule independent of a SecurityGroup? If so, it should be a separate resource type.

ShreyNamdeo added 4 commits November 23, 2020 11:15

Initial Commit

3b8d7f9

Signed-off-by: shreya <[email protected]>

Adding Network security Group functionality to Azure Provider.

2247ff9

Signed-off-by: shreya <[email protected]>

Adding Network Security Group auto-generated CRDs to Azure Provider.

b600fb1

Signed-off-by: shreya <[email protected]>

Adding network security group example yaml files to Azure Provider.

932c4f0

Signed-off-by: shreya <[email protected]>

escortnotice changed the title ~~Feature nsg~~ Add feature to create a Network Security Group in Azure #168 Nov 23, 2020

escortnotice changed the title ~~Add feature to create a Network Security Group in Azure #168~~ Add feature to create a Network Security Group in Azure Nov 23, 2020

muvaf requested review from hasheddan and kasey November 23, 2020 14:58

shnamde1 and others added 5 commits December 14, 2020 12:34

Unit test cases for network security group controller methods observe…

1a7506d

… and create.

Added conditions on client methods to check variations in security ru…

d6f24cf

…le definitions.

Completed unit test cases for controller methods update and delete.

07738ef

Completed unit test cases for all client methods.

3924c14

Merge pull request #8 from escortnotice/feature-nsg-unit-test

c6f3902

Feature nsg unit test to feature msg

hasheddan requested changes Jan 8, 2021

View reviewed changes

shnamde1 added 2 commits January 12, 2021 19:37

Code refactored as per review comments on PR crossplane-contrib#200

43a70f6

Network Security Group examples updated.

81c036e

hasheddan reviewed Feb 18, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add feature to create a Network Security Group in Azure #200

Add feature to create a Network Security Group in Azure #200

escortnotice commented Nov 23, 2020 •

edited

Loading

hasheddan left a comment

hasheddan Jan 8, 2021

ShreyNamdeo Jan 11, 2021

hasheddan Jan 8, 2021

ShreyNamdeo Jan 12, 2021

ShreyNamdeo Jan 12, 2021

hasheddan Jan 8, 2021

ShreyNamdeo Jan 12, 2021

hasheddan Jan 8, 2021

hasheddan Jan 8, 2021

hasheddan Jan 8, 2021

ShreyNamdeo Jan 11, 2021

hasheddan Jan 8, 2021

ShreyNamdeo Jan 11, 2021

hasheddan Jan 8, 2021

ShreyNamdeo Jan 11, 2021

hasheddan Jan 8, 2021

ShreyNamdeo Jan 11, 2021

hasheddan Jan 8, 2021

ShreyNamdeo Jan 11, 2021

hasheddan Jan 8, 2021

ShreyNamdeo Jan 12, 2021

hasheddan Feb 18, 2021

ulucinar Sep 23, 2021

hasheddan Feb 18, 2021

	Description string `json:"description,omitempty"`
	Description *string `json:"description,omitempty"`

	SecurityGroup "github.com/crossplane/provider-azure/pkg/controller/network/securitygroup"
	"github.com/crossplane/provider-azure/pkg/controller/network/securitygroup"

		//"github.com/crossplane/provider-azure/pkg/clients/network"

		//"github.com/Azure/azure-sdk-for-go/services/network/mgmt/2019-12-01/network/networkapi"

Add feature to create a Network Security Group in Azure #200

Are you sure you want to change the base?

Add feature to create a Network Security Group in Azure #200

Conversation

escortnotice commented Nov 23, 2020 • edited Loading

Description of your changes

How has this code been tested

hasheddan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

escortnotice commented Nov 23, 2020 •

edited

Loading