PR for Security group for early feedback#156
PR for Security group for early feedback#156er1cthe0ne wants to merge 4 commits intofuturewei-cloud:masterfrom
Conversation
| // | ||
| // Created by Administrator on 2020/10/12. | ||
| // |
There was a problem hiding this comment.
please remove the first three lines of extra header.
|
|
||
| class Aca_Security_Group_Rule { | ||
| public: | ||
| static Aca_Security_Group_Rule &get_instance(); |
There was a problem hiding this comment.
ACA uses .clang-format to do automatic formatting. I have the setting in my IDE to auto-format on file save. Please see if you can do similar settings in your IDE.
| auto ovsdb_client_start = chrono::steady_clock::now(); | ||
|
|
||
| string ovsdb_cmd_string = "/usr/bin/ovs-vsctl " + cmd_string; | ||
| string ovsdb_cmd_string = "ovs-vsctl " + cmd_string; |
There was a problem hiding this comment.
we already removed "/usr/bin/" in the latest master.
| Aca_Security_Group::Aca_Security_Group(Aca_Security_Group &sg) { | ||
| this->id = sg.get_id(); | ||
| this->name = sg.get_name(); | ||
| this->format_version = sg.get_format_version(); |
There was a problem hiding this comment.
format_version will be removed from the schema soon because it is not effective to help with grpc message compatibility (we are keeping the one in the top level GoalState message.
| aca_sg = siter->second; | ||
| } | ||
|
|
||
| aca_sg->add_port_id(port_id); |
There was a problem hiding this comment.
note that we also keep track of ports inside vlan manager for other usage. see ACA_Vlan_Manager::get_instance().add_ovs_port. I think it is okay for security group manager to track ports seperately for now but can consider merging in the future.
| string port_id = input_port.get_id(); | ||
| string sg_id = input_sg.get_id(); | ||
|
|
||
| //TODO: do we need to update the port ? |
There was a problem hiding this comment.
Not sure if we need to update port when security group is updated.
|
|
||
| aca_sg_rule = sg.get_security_group_rule(rule_id); | ||
| if (aca_sg_rule != NULL) { | ||
| TRN_LOG_WARN("Security group rule(id:%s) already exist", rule_id.data()); |
There was a problem hiding this comment.
use ACA_LOG_WARN instead of TRN_LOG_WARN
| int Aca_Security_Group_Manager::set_remote_group(Aca_Security_Group_Rule &sg_rule) | ||
| { | ||
| map<string, Aca_Security_Group *>::iterator iter; | ||
| string remote_grou_id = sg_rule.get_remote_group_id(); |
There was a problem hiding this comment.
typo: remote_grou_id :)
| // Drop all remaining egress connections | ||
| sprintf(flow, "table=%d,priority=10,in_port=%d,reg%d=%d,actions=ct_clear,resubmit(,%d)", | ||
| BASE_EGRESS_TABLE, ofport, REG_PORT, ofport, DROPPED_TRAFFIC_TABLE); | ||
| controller.add_flow(BR_INT, flow); |
There was a problem hiding this comment.
add_flow can fail and returns and error code.
Do you want to add the flows as bundle so that it is "all or nothing"
@cj-chung - FYI.
er1cthe0ne
left a comment
er1cthe0ne
left a comment
There was a problem hiding this comment.
This is a great start. I have left some early feedback.
…#156) This PR adds quite a few design docs including * Key System Flows * Alcor Controller Microservices - Mac Manager * Alcor Database and Cache services * Alcor Control Agent - major components design * Communication - Fast path, normal path and rescue path * System Monitoring * Communication Protocol with Compute
|
closing this old draft PR. |
|
Here is my additional comments on this draft security group implementation:
|
2 participants
This is the PR for Security group implementation so that we can provide some early feedback.