adds imp for azure using filtertype security group#17
adds imp for azure using filtertype security group#17fahad-rauf wants to merge 8 commits intomasterfrom
Conversation
|
@fahad-rauf Image is available for testing. |
|
@fahad-rauf Image is available for testing. |
|
@fahad-rauf plz add tests |
|
also add in the readme |
|
|
||
| // Azure provider class implementing the Provider interface | ||
| type Azure struct { | ||
| ClientSet clientset.Interface |
There was a problem hiding this comment.
Maybe keep clients in this structure instead of credentials? e.g securityRulesClient, securityGroupClient etc. Then we don't need to create new clients on each reconcile loop.
| // WhiteListIps - Get List of IP addresses to whitelist | ||
| func (a *Azure) WhiteListIps(filter config.Filter, ipPermissions []utils.IpPermission) error { | ||
|
|
||
| resourceGroups, err := fetchResourceGroups(a, filter) |
There was a problem hiding this comment.
Shouldn't we fetch the resource group based on resourceGroupName that the user provides?
| } | ||
| for _, resourceGroup := range resourceGroups.Values() { | ||
| resourceName := *resourceGroup.Name | ||
| logrus.Infof("Name of the resource %s", *resourceGroup.Name) |
There was a problem hiding this comment.
this statement would get logged every few seconds and I don't see much use for it. Better to log resource group name when something actually happens, like rule added or removed or some other error.
| logrus.Error("Error fetching resource groups for given filter") | ||
| return err | ||
| } | ||
| for _, resourceGroup := range resourceGroups.Values() { |
| return &nsgClient, nil | ||
| } | ||
|
|
||
| func createSecurityRule(a *Azure, rulesClient network.SecurityRulesClient, resourceName string, ipDescription *string, fromPortStr string, toPortStr string, ipRange *string, ipProtocol string) error { |
There was a problem hiding this comment.
Would be better to move to create separate files for wrapper methods of clients e.g securityRuleClient and securityGroupClient.
|
|
||
| var networkProtocol network.SecurityRuleProtocol | ||
|
|
||
| switch ipProtocol { |
There was a problem hiding this comment.
Perhaps move this to a new method like validateSecurityRuleProtocol to keep this method small.
| return err | ||
| } | ||
| for _, resourceGroup := range resourceGroups.Values() { | ||
| resourceName := *resourceGroup.Name |
There was a problem hiding this comment.
I think its alright to access the property of an object instead of putting it in a variable.
| return nil | ||
| } | ||
|
|
||
| func isSecurityRuleToBeRetained(existingRuleName string, ipPermissions []utils.IpPermission, keepRuleDescriptionPrefix string, removeRule bool) bool { |
There was a problem hiding this comment.
This method should be replaced with two methods.
- One that verifies if this rule should be deleted based on the rules that user has provided i.e git repo.
- Second that verifies that the rule doesn't have the description to prevent it from being deleted.
Also these two methods should be wrapped in a third method which prevents these methods from being executed if removeRule is false.
…ster into sec-group-azure
|
@fahad-rauf Image is available for testing. |
|
@fahad-rauf Image is available for testing. |
|
@fahad-rauf Image is available for testing. |
|
@hamza3202 kindly review it when you have time |
hamza3202
left a comment
hamza3202
left a comment
There was a problem hiding this comment.
Looks almost done. @kahootali please deploy and verify with a working kubernetes cluster to make sure it works as expected.
Please verify that it doesn't delete and add the same rule again and again in each reconciliation cycle.
| params: | ||
| KeepRuleDescriptionPrefix: "DO NOT REMOVE -" | ||
| RemoveRule: true | ||
| SubscriptionID: "47c9180a-967d-4ba0-bfc0-7b12762f0779" |
There was a problem hiding this comment.
yes these are dummy
| securityGroupClient network.SecurityGroupsClient | ||
| securityRulesClient network.SecurityRulesClient | ||
| SubscriptionID string | ||
| ClientID string |
There was a problem hiding this comment.
We should not keep fields that will not be used in future in this structure, e.g ClientID, ClientSecret, TenantID and subscriptionID.
|
|
||
| func (a *Azure) updateSecurityRules(securityGroup network.SecurityGroup, ipPermissions []utils.IpPermission) error { | ||
| if a.RemoveRule { | ||
| err := deleteRedundantSecurityRules(a, securityGroup, ipPermissions) |
There was a problem hiding this comment.
Method name can be improved IMO. The rules are not redundant but perhaps expired or old or extra or invalid.
There was a problem hiding this comment.
redundant means : not or no longer needed or useful; superfluous.
|
|
||
| func getSecurityGroupsClient(a *Azure) (*network.SecurityGroupsClient, error) { | ||
|
|
||
| authorizer, err := auth.NewClientCredentialsConfig(a.ClientID, a.ClientSecret, a.TenantID).Authorizer() |
There was a problem hiding this comment.
authorizer should be created once and passed to all client creators.
|
@fahad-rauf Image is available for testing. |
4 participants
No description provided.