Added check for egress rule to allow traffic on all ports in MS NSG. #300
Conversation
oracle-contributor-agreement
bot
added
the
OCA Verified
|
Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA). To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application. When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated. If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public. |
oracle-contributor-agreement
bot
added
OCA Required
ManviGupta-prog
force-pushed
the
topic_manvigup-JCS-14691
branch
from
1c0ae26 to
6f73111
Compare
oracle-contributor-agreement
bot
added
OCA Verified
|
Can you explain how you tested check_egress_all_traffic_in_seclist_or_nsg() function? The reason I find this important is that if I look at this code submission only the "nsg" option would ever be passed to this function. |
| continue | ||
| fi | ||
|
|
||
| if [[ $egress_destination == "0.0.0.0/0" && ( $egress_protocol == "all" || $egress_protocol == "1" ) ]]; then |
There was a problem hiding this comment.
It would be nice if the "1" was assigned to a descriptive variable. In other parts of the code a variable is defined for tcp_protocol="6". This tells a reader of the code that TCP is being looked for when $tcp_protocol is referenced. I am reading this and I have no clue what "1" protocol is.
| @@ -347,6 +347,52 @@ function check_udp_port_open_in_seclist_or_nsg() { | |||
| echo 1 | |||
| fi | |||
| } | |||
There was a problem hiding this comment.
The copyright header needs to change to
Copyright (c) 2023, 2025, Oracle and/or its affiliates.
| @@ -347,6 +347,52 @@ function check_udp_port_open_in_seclist_or_nsg() { | |||
| echo 1 | |||
| fi | |||
| } | |||
| # Checks if there is an egress rule to ensure that the network can establish outbound communication to any destination, utilizing all available protocols and ports. | |||
| Args: | |||
There was a problem hiding this comment.
I suggest to use the same format for function comments used in other functions.
In addition to that, looks like this line does not begin with comment sign (#) . Doesn't this cause an error when the script is run?
| @@ -347,6 +347,52 @@ function check_udp_port_open_in_seclist_or_nsg() { | |||
| echo 1 | |||
| fi | |||
| } | |||
| # Checks if there is an egress rule to ensure that the network can establish outbound communication to any destination, utilizing all available protocols and ports. | |||
| Args: | |||
| # nsg_ocid: OCID for the nsg. | |||
There was a problem hiding this comment.
I see the code expects two parameters:
- seclist_or_nsg_ocid
- ocid_type
and here I see only one listed
| local seclist_or_nsg_ocid=$1 | ||
| local ocid_type=$2 | ||
| local icmp_protocol="1" | ||
| local egress_is_open=false |
There was a problem hiding this comment.
is this variable really needed? I see is declared here, and is set to true in line 383, and then the code returns in line 385. Si, this variable is never read. Unless I am missing something
There was a problem hiding this comment.
I mean, variable in line 359:
local egress_is_open=false
| egress_destination_type=$(echo $egress_rules | jq -r --arg i "$j" '.[$i|tonumber]."destination-type"') | ||
|
|
||
| if [[ $egress_destination_type != "CIDR_BLOCK" ]]; then | ||
| nsg_sec_list_array[$j]="WARNING: Source type is either NSG or Service. Skipping the validation check for ${egress_destination}." |
There was a problem hiding this comment.
should this be destination type or target type (I do not know the correct names) instead of source type? we are checking they type of egress destination type
| # nsg_ocid: OCID for the nsg. | ||
| # Returns: | ||
| # 0|1 | ||
| function check_egress_all_traffic_in_nsg() { |
There was a problem hiding this comment.
I see you renamed the function in your second commit, to remove the seclist part from the function, and leave nsg only,
But I see the code still supports sect lists.
Why was this done? is this intended?
No description provided.