Adding security groups in resource_pools instead of networks
we currently encounter a problem which has been discussed briefly before on the list : Adding security groups in resource pools should be possible. On a side note: we are dealing with openstack, so references below might be openstack specific.
Here is a use-case: having machines in the same network, but with different incoming/outgoing rules. Example: only the runners/DEAs of a CF deployment should be able to access some service VMs. Currently, this means that we have to have the same network configuration twice in our manifests, the only difference being the set of security groups. I’d like to propose a change to allow specifying them on resource_pool level and discuss some implementation-specifics and impacts before writing code, so we are all on the same page.
If we are introducing anything new, my assumption is that current behavior of specifying security groups in networks should not break or change. If you have a manifest specifying security groups, you probably expect it to work also when a new feature is added.
Analysis of current state
* global default security groups can be specified when setting up your director
* network security groups override those when specified for a deployment
* security groups are not a first-class concept. They are transported through the entities known to bosh within the cloud_properties of a network. Therefore, only methods dealing with the network entities obtained from the manifest or director db actually know about them implicitly.
* introduce ability to specify security groups for resource pools
* keep current behavior:
** if there are global default security groups only, use them.
** if there are network security groups, use them instead of anything else. Don’t care about global groups or the new resource pool groups
** if there are resource pool groups AND no network security groups, use them. Don’t care about global groups.
** probably remove security groups on networks at some point in time with a heads-up to everyone currently using it. I have no idea if this is feasible.
* create_vm and configure_networks of CPI seem to be the relevant calls setting up the security groups: the former or creating a vm, the latter for updating an existing one. Any changes done here would be CPI specific!
* adapting create_vm could be done straight forward: it is already using the network_configurator to merge the security groups , and has access to the resource_pool for the vm as well. We could simply add logic here to take security groups within a resource pool into account.
* adapting configure_networks is more tricky: It gets a network spec and compares the security groups in there with the ones currently present on a vm . It has no idea about the resource pool of that vm. The CPI is called by the director’s network_updater  which gets initialized for a specific instance and is called by the instance_updater .
* The instance entity combines all there is to know in terms of configuration for a specific vm (e.g. network settings ), so this could be the point to include the new feature
So, what could be changed now?
* introduce a new method security_groups on bosh-director/lib/bosh/director/deployment_plan/instance.rb called security_groups, providing information about security groups. If there are secrurity groups on networks, return them, otherwise return security groups defined on resource pools if there are any. Just like the desired behavior we assumed above.
* adapt create_vm and configure_networks to accept security_groups as an additional argument. Instead of having the CPIs extract security groups from the network’s cloud_properties, take them from the argument and keep the current logic of the methods.
What are your thoughts on this?
I would love to have the change isolated from the actual CPI coding, so we don’t need to adapt all of them at the same time. However, this seems like an API change might be in order, so I’m not sure on how to do it.
Given any form of agreement on how to proceed, we could provide a PR as a further means for discussion. However, this change will impact the API, so I wanted to get your feedback on this before actually implementing anything.