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.

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

Enhance MySQL firewall public IP for local PC #5163

Merged
merged 22 commits into from
Apr 28, 2021

Conversation

shenqianjin
Copy link
Contributor

No description provided.

@@ -213,8 +213,8 @@ public static boolean isAllowAccessFromLocalMachine(final String subscriptionId,
}

public static boolean isAllowAccessFromLocalMachine(final List<FirewallRuleInner> firewallRules) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's weird that we have to pass firewallRules as the parameter. I naturally think we should pass server as the parameter.
e.g. isLocalMachineAllowedAccess(server)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isLocalMachineAllowedAccess(server) existed as well. it is a polymorphic one. in order to void to list firewall rules twice when when need to calculate access to local PC and other azure services.

Copy link
Collaborator

Choose a reason for hiding this comment

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

then this method may be renamed to something like isLocalMachineIncluded(rules)

@wangmingliang-ms
Copy link
Collaborator

wangmingliang-ms commented Apr 27, 2021

it's basically OK for me about the logic part.
but I would like to rename some methods like:
getAccessFromLocalRuleName
disableAllowAccessFromLocalMachine/enableAllowAccessFromLocalMachine

and you may simplify the code by only providing these meta methods for firewall rules:

getPublicIp();

getAzureServicesFirewallRuleName();
getLocalMachineFirewallRuleName();

isRuleExisted(ruleName, server)
addRule(ruleName, ip, server);
removeRule(ruleName, server);

so that, e.g.

isAccessFromAzureServicesAllowed  = {
  ruleName = getAzureServicesFirewallRuleName();
  return isRuleExisted(ruleName, server)
}
enableAccessFromAzureServices  = {
  ruleName = getAzureServicesFirewallRuleName();
  addRule(ruleName, '0.0.0.0', server)
}
disableAccessFromAzureServices  = {
  ruleName = getAzureServicesFirewallRuleName();
  removeRule(ruleName, server)
}

isAccessFromLocalMachineAllowed  = {
  ruleName = getLocalMachineFirewallRuleName();
  return isRuleExisted(ruleName, server)
}
enableAccessFromLocalMachine = {
  ip = getPublicIp()
  ruleName = getLocalMachineFirewallRuleName();
  addRule(ruleName, ip, server)
}
disableAccessFromLocalMachine = {
  ruleName = getLocalMachineFirewallRuleName();
  removeRule(ruleName, server)
}

@Flanker32 Flanker32 self-requested a review April 28, 2021 01:34
@shenqianjin shenqianjin changed the base branch from develop to endgame-s186 April 28, 2021 02:33
@shenqianjin shenqianjin changed the base branch from endgame-s186 to develop April 28, 2021 03:12
@shenqianjin shenqianjin dismissed wangmingliang-ms’s stale review April 28, 2021 03:12

The base branch was changed.

@shenqianjin shenqianjin changed the base branch from develop to endgame-s186 April 28, 2021 03:13
@shenqianjin shenqianjin merged commit a8efdfa into endgame-s186 Apr 28, 2021
@wangmingliang-ms wangmingliang-ms deleted the qianjin-bugfix-mysql branch April 28, 2021 05:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants