[OS-Kubespray] Set up security group for openstack instances #353
Conversation
katyafervent
force-pushed
the
resolve-ports
branch
3 times, most recently
from
d0d0183
to
9891969
Compare
katyafervent
force-pushed
the
resolve-ports
branch
4 times, most recently
from
ab23fd4
to
a711bde
Compare
| @@ -807,7 +810,45 @@ def _get_userdata(self): | |||
| } | |||
| return "#cloud-config\n" + yaml.dump(userdata) | |||
|
|
|||
| def _boot_servers(self, *, name, servers_range, image, flavor, network, | |||
| def _set_up_security_groups(self): | |||
| master_sg = self.c.get_security_group("kqueen_master") | |||
There was a problem hiding this comment.
I suggest defining "kqueen_master" as a variable. At least you use it twice, and maybe you'll need to configure it someday
| port_range_min="179", | ||
| port_range_max="179") | ||
|
|
||
| slave_sg = self.c.get_security_group("kqueen_slave") |
katyafervent
force-pushed
the
resolve-ports
branch
from
a711bde
to
a1958be
Compare
katyafervent
force-pushed
the
resolve-ports
branch
4 times, most recently
from
7b29ce1
to
e4c8dfd
Compare
| @@ -20,6 +20,9 @@ | |||
| logger = logging.getLogger("kqueen_api") | |||
| config = current_config() | |||
|
|
|||
| MASTER_SECURITY_GR = "kqueen_master" | |||
| COMMON_SECURITY_GR = "kqueen_common" | |||
There was a problem hiding this comment.
what is it? why its hardcoded? if it permanent change, we should document it and provide possibility of change
There was a problem hiding this comment.
imo we should provide an option on UI , smth like 'create personal SC' otherwise it should use 'default'
its just one of useful ideas, need to talk about it with u
There was a problem hiding this comment.
without specific SG, openstack provisioner will not work
There was a problem hiding this comment.
so need to document it as 'pre-step' or provide kqueen-OPS for that
| @@ -807,7 +813,40 @@ def _get_userdata(self): | |||
| } | |||
| return "#cloud-config\n" + yaml.dump(userdata) | |||
|
|
|||
| def _boot_servers(self, *, name, servers_range, image, flavor, network, | |||
| def _set_up_security_groups(self): | |||
| master_sg = self.c.get_security_group(MASTER_SECURITY_GR) | |||
There was a problem hiding this comment.
imo all getters should be in one place in init method like any previous engines
There was a problem hiding this comment.
in init method data, collected from user is evaluated
| def _set_up_security_groups(self): | ||
| master_sg = self.c.get_security_group(MASTER_SECURITY_GR) | ||
| if not master_sg: | ||
| master_sg = self.c.create_security_group(name=MASTER_SECURITY_GR, |
There was a problem hiding this comment.
imo we need a try/catch here
There was a problem hiding this comment.
error catching is made in the above level (_run_provisioning function)
|
|
||
| common_sg = self.c.get_security_group(COMMON_SECURITY_GR) | ||
| if not common_sg: | ||
| common_sg = self.c.create_security_group(name=COMMON_SECURITY_GR, |
| self.c.create_security_group_rule(common_sg.id, protocol="tcp", | ||
| port_range_min="30000", | ||
| port_range_max="32767") | ||
| return master_sg, common_sg |
There was a problem hiding this comment.
there is no 'deprovision' step for SC
There was a problem hiding this comment.
you will not be able to delete sg if other cluster are using it. I think it will increase deletion time witch is so big now, but sg is not occupied space so fr me it is ok to leave it
| @@ -20,6 +20,9 @@ | |||
| logger = logging.getLogger("kqueen_api") | |||
| config = current_config() | |||
|
|
|||
| MASTER_SECURITY_GR = "kqueen_master" | |||
| COMMON_SECURITY_GR = "kqueen_common" | |||
There was a problem hiding this comment.
so need to document it as 'pre-step' or provide kqueen-OPS for that
katyafervent
force-pushed
the
resolve-ports
branch
8 times, most recently
from
fbbbd03
to
35674ce
Compare
katyafervent
force-pushed
the
resolve-ports
branch
2 times, most recently
from
bd91885
to
9b716d8
Compare
* Also provide full path to the inventory file
katyafervent
force-pushed
the
resolve-ports
branch
2 times, most recently
from
51a6e97
to
50f784a
Compare
|
@katyafervent what we should do with that PR? if we delay it, pls set additional labels |
No description provided.