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

OpenStack: allow to specify additional networks and security groups for masters and workers #3291

Merged
merged 4 commits into from
Apr 8, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 4 additions & 3 deletions data/data/openstack/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,10 @@ module "masters" {
var.openstack_master_extra_sg_ids,
[module.topology.master_sg_id],
)
root_volume_size = var.openstack_master_root_volume_size
root_volume_type = var.openstack_master_root_volume_type
server_group_id = var.openstack_master_server_group_id
root_volume_size = var.openstack_master_root_volume_size
root_volume_type = var.openstack_master_root_volume_type
server_group_id = var.openstack_master_server_group_id
additional_network_ids = var.openstack_additional_network_ids
}

module "topology" {
Expand Down
8 changes: 8 additions & 0 deletions data/data/openstack/masters/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,14 @@ resource "openstack_compute_instance_v2" "master_conf" {
group = var.server_group_id
}

dynamic network {
for_each = var.additional_network_ids

content {
uuid = network.value
}
}

metadata = {
# FIXME(mandre) shouldn't it be "${var.cluster_id}-master-${count.index}" ?
Name = "${var.cluster_id}-master"
Expand Down
5 changes: 5 additions & 0 deletions data/data/openstack/masters/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,8 @@ variable "server_group_id" {
type = string
description = "ID of the server group to assign the servers to."
}

variable "additional_network_ids" {
type = list(string)
description = "IDs of additional networks for master nodes."
}
6 changes: 6 additions & 0 deletions data/data/openstack/variables-openstack.tf
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,12 @@ variable "openstack_external_dns" {
default = []
}

variable "openstack_additional_network_ids" {
type = list(string)
description = "IDs of additional networks for master nodes."
default = []
}

variable "openstack_master_flavor_name" {
type = string
description = "Instance size for the master node(s). Example: `m1.medium`."
Expand Down
64 changes: 64 additions & 0 deletions docs/user/openstack/customization.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ Beyond the [platform-agnostic `install-config.yaml` properties](../customization
* [Minimal](#minimal)
* [Custom-machine-pools](#custom-machine-pools)
* [Image Overrides](#image-overrides)
* [Additional Networks](#additional-networks)
* [Additional Security Groups](#additional-security-groups)
* [Further customization](#further-customization)

## Cluster-scoped properties
Expand All @@ -27,6 +29,8 @@ Beyond the [platform-agnostic `install-config.yaml` properties](../customization

## Machine pools

* `additionalNetworkIDs` (optional list of strings): IDs of additional networks for machines.
* `additionalSecurityGroupIDs` (optional list of strings): IDs of additional security groups for machines.
* `type` (optional string): The OpenStack flavor name for machines in the pool.
* `rootVolume` (optional object): Defines the root volume for instances in the machine pool. The instances use ephemeral disks if not set.
* `size` (required integer): Size of the root volume in GB.
Expand Down Expand Up @@ -128,6 +132,66 @@ platform:
clusterOSImage: my-rhcos
```

## Additional Networks

You can set additional networks for your machines by defining `additionalNetworkIDs` parameter in the machine configuration. The parameter is a list of strings with additional network IDs:

```yaml
additionalNetworkIDs:
- <network1_uuid>
- <network2_uuid>
```

You can attach this parameter for both `controlPlane` and `compute` machines:

Example:

```yaml
compute:
- name: worker
platform:
openstack:
additionalNetworkIDs:
- fa806b2f-ac49-4bce-b9db-124bc64209bf
controlPlane:
name: master
platform:
openstack:
additionalNetworkIDs:
- fa806b2f-ac49-4bce-b9db-124bc64209bf
```

**NOTE:** Allowed address pairs won't be created for the additional networks.

## Additional Security Groups

You can set additional security groups for your machines by defining `additionalSecurityGroupIDs` parameter in the machine configuration. The parameter is a list of strings with additional security group IDs:

```yaml
additionalSecurityGroupIDs:
- <security_group1_id>
- <security_group2_id>
```

You can attach this parameter for both `controlPlane` and `compute` machines:

Example:

```yaml
compute:
- name: worker
platform:
openstack:
additionalSecurityGroupIDs:
- 7ee219f3-d2e9-48a1-96c2-e7429f1b0da7
controlPlane:
name: master
platform:
openstack:
additionalSecurityGroupIDs:
- 7ee219f3-d2e9-48a1-96c2-e7429f1b0da7
```

## Further customization

For customizing the installation beyond what is possible with `openshift-install`, refer to the [UPI (User Provided Infrastructure) documentation](./install_upi.md).
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -162,5 +162,5 @@ replace (
k8s.io/sample-apiserver => k8s.io/sample-apiserver v0.17.1 // Replaced by MCO/CRI-O
sigs.k8s.io/cluster-api-provider-aws => github.com/openshift/cluster-api-provider-aws v0.2.1-0.20200316201703-923caeb1d0d8 // Pin OpenShift fork
sigs.k8s.io/cluster-api-provider-azure => github.com/openshift/cluster-api-provider-azure v0.1.0-alpha.3.0.20200120114645-8a9592f1f87b // Pin OpenShift fork
sigs.k8s.io/cluster-api-provider-openstack => github.com/openshift/cluster-api-provider-openstack v0.0.0-20200221124403-d699c3611b0c // Pin OpenShift fork
sigs.k8s.io/cluster-api-provider-openstack => github.com/openshift/cluster-api-provider-openstack v0.0.0-20200323110431-3311de91e078 // Pin OpenShift fork
)
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -1792,6 +1792,8 @@ github.com/openshift/cluster-api-provider-openstack v0.0.0-20200130125124-ef82ce
github.com/openshift/cluster-api-provider-openstack v0.0.0-20200130125124-ef82ce374112/go.mod h1:ntMRKZlv++TExGO4g2jgsVIaHKJt8kKe72BAvMPV5vA=
github.com/openshift/cluster-api-provider-openstack v0.0.0-20200221124403-d699c3611b0c h1:Rn/Ip2nbWUhvOF9/EZaorxKVcQnm427cSOJQJIFXuHQ=
github.com/openshift/cluster-api-provider-openstack v0.0.0-20200221124403-d699c3611b0c/go.mod h1:ntMRKZlv++TExGO4g2jgsVIaHKJt8kKe72BAvMPV5vA=
github.com/openshift/cluster-api-provider-openstack v0.0.0-20200323110431-3311de91e078 h1:Irj9ROcWhbeH6t2DEUDIpdIJgSLBaXww6AP/FMCmGmw=
github.com/openshift/cluster-api-provider-openstack v0.0.0-20200323110431-3311de91e078/go.mod h1:ntMRKZlv++TExGO4g2jgsVIaHKJt8kKe72BAvMPV5vA=
github.com/openshift/cluster-api-provider-ovirt v0.1.1-0.20200128081049-840376ca5c09 h1:QJxGgIB7f5BqNPEZOCgV29NsDf1P439Bs3q0B5O3fP8=
github.com/openshift/cluster-api-provider-ovirt v0.1.1-0.20200128081049-840376ca5c09/go.mod h1:NcvJT99IauPosghKCineBG8yswe9JjuddiWuvsqge64=
github.com/openshift/cluster-autoscaler-operator v0.0.0-20190521201101-62768a6ba480/go.mod h1:/XmV44Fh28Vo3Ye93qFrxAbcFJ/Uy+7LPD+jGjmfJYc=
Expand Down
1 change: 1 addition & 0 deletions pkg/asset/cluster/tfvars.go
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,7 @@ func (t *TerraformVariables) Generate(parents asset.Parents) error {
clusterID.InfraID,
caCert,
bootstrapIgn,
installConfig.Config.ControlPlane.Platform.OpenStack,

Choose a reason for hiding this comment

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

You dont have to do this in this patch, but could we just pass the install config here, or make a new struct to pass tfvars? The number of arguments to this function is becoming ludicrous :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know... Abhinav asked the same question #3291 (comment)
But I think we don't have other options, because we need to differenciate between primary and additional objects, and the only way is to read them from the machine pool :(

)
if err != nil {
return errors.Wrapf(err, "failed to get %s Terraform variables", platform)
Expand Down
58 changes: 36 additions & 22 deletions pkg/asset/machines/openstack/machines.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,35 +86,49 @@ func Machines(clusterID string, config *types.InstallConfig, pool *types.Machine
}

func generateProvider(clusterID string, platform *openstack.Platform, mpool *openstack.MachinePool, osImage string, az string, role, userDataSecret string, trunk string) *openstackprovider.OpenstackProviderSpec {
networks := []openstackprovider.NetworkParam{
{
Subnets: []openstackprovider.SubnetParam{
{
Filter: openstackprovider.SubnetFilter{
Name: fmt.Sprintf("%s-nodes", clusterID),
Tags: fmt.Sprintf("%s=%s", "openshiftClusterID", clusterID),
},
},
},
},
}
Comment on lines +89 to +100
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
networks := []openstackprovider.NetworkParam{
{
Subnets: []openstackprovider.SubnetParam{
{
Filter: openstackprovider.SubnetFilter{
Name: fmt.Sprintf("%s-nodes", clusterID),
Tags: fmt.Sprintf("%s=%s", "openshiftClusterID", clusterID),
},
},
},
},
}
networks := []openstackprovider.NetworkParam{{
Subnets: []openstackprovider.SubnetParam{{
Filter: openstackprovider.SubnetFilter{
Name: fmt.Sprintf("%s-nodes", clusterID),
Tags: fmt.Sprintf("%s=%s", "openshiftClusterID", clusterID),
},
}},
}}

for _, networkID := range mpool.AdditionalNetworkIDs {
networks = append(networks, openstackprovider.NetworkParam{
UUID: networkID,
NoAllowedAddressPairs: true,
})
}

securityGroups := []openstackprovider.SecurityGroupParam{
{
Name: fmt.Sprintf("%s-%s", clusterID, role),
},
}
for _, sg := range mpool.AdditionalSecurityGroupIDs {
securityGroups = append(securityGroups, openstackprovider.SecurityGroupParam{
UUID: sg,
})
}

spec := openstackprovider.OpenstackProviderSpec{
TypeMeta: metav1.TypeMeta{
APIVersion: openstackprovider.SchemeGroupVersion.String(),
Kind: "OpenstackProviderSpec",
},
Flavor: mpool.FlavorName,
CloudName: CloudName,
CloudsSecret: &corev1.SecretReference{Name: cloudsSecret, Namespace: cloudsSecretNamespace},
UserDataSecret: &corev1.SecretReference{Name: userDataSecret},
Networks: []openstackprovider.NetworkParam{
{
Subnets: []openstackprovider.SubnetParam{
{
Filter: openstackprovider.SubnetFilter{
Name: fmt.Sprintf("%s-nodes", clusterID),
Tags: fmt.Sprintf("%s=%s", "openshiftClusterID", clusterID),
},
},
},
},
},
Flavor: mpool.FlavorName,
CloudName: CloudName,
CloudsSecret: &corev1.SecretReference{Name: cloudsSecret, Namespace: cloudsSecretNamespace},
UserDataSecret: &corev1.SecretReference{Name: userDataSecret},
Networks: networks,
AvailabilityZone: az,
SecurityGroups: []openstackprovider.SecurityGroupParam{
{
Name: fmt.Sprintf("%s-%s", clusterID, role),
},
},
Trunk: trunkSupportBoolean(trunk),
SecurityGroups: securityGroups,
Trunk: trunkSupportBoolean(trunk),
Tags: []string{
fmt.Sprintf("openshiftClusterID=%s", clusterID),
},
Expand Down
49 changes: 33 additions & 16 deletions pkg/tfvars/openstack/openstack.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,31 +14,34 @@ import (
"github.com/gophercloud/utils/openstack/clientconfig"
"github.com/openshift/installer/pkg/rhcos"
"github.com/openshift/installer/pkg/tfvars/internal/cache"
types_openstack "github.com/openshift/installer/pkg/types/openstack"
"github.com/pkg/errors"

"sigs.k8s.io/cluster-api-provider-openstack/pkg/apis/openstackproviderconfig/v1alpha1"
)

type config struct {
BaseImageName string `json:"openstack_base_image_name,omitempty"`
ExternalNetwork string `json:"openstack_external_network,omitempty"`
Cloud string `json:"openstack_credentials_cloud,omitempty"`
FlavorName string `json:"openstack_master_flavor_name,omitempty"`
LbFloatingIP string `json:"openstack_lb_floating_ip,omitempty"`
APIVIP string `json:"openstack_api_int_ip,omitempty"`
DNSVIP string `json:"openstack_node_dns_ip,omitempty"`
IngressVIP string `json:"openstack_ingress_ip,omitempty"`
TrunkSupport string `json:"openstack_trunk_support,omitempty"`
OctaviaSupport string `json:"openstack_octavia_support,omitempty"`
RootVolumeSize int `json:"openstack_master_root_volume_size,omitempty"`
RootVolumeType string `json:"openstack_master_root_volume_type,omitempty"`
BootstrapShim string `json:"openstack_bootstrap_shim_ignition,omitempty"`
ExternalDNS []string `json:"openstack_external_dns,omitempty"`
MasterServerGroupID string `json:"openstack_master_server_group_id,omitempty"`
BaseImageName string `json:"openstack_base_image_name,omitempty"`
ExternalNetwork string `json:"openstack_external_network,omitempty"`
Cloud string `json:"openstack_credentials_cloud,omitempty"`
FlavorName string `json:"openstack_master_flavor_name,omitempty"`
LbFloatingIP string `json:"openstack_lb_floating_ip,omitempty"`
APIVIP string `json:"openstack_api_int_ip,omitempty"`
DNSVIP string `json:"openstack_node_dns_ip,omitempty"`
IngressVIP string `json:"openstack_ingress_ip,omitempty"`
TrunkSupport string `json:"openstack_trunk_support,omitempty"`
OctaviaSupport string `json:"openstack_octavia_support,omitempty"`
RootVolumeSize int `json:"openstack_master_root_volume_size,omitempty"`
RootVolumeType string `json:"openstack_master_root_volume_type,omitempty"`
BootstrapShim string `json:"openstack_bootstrap_shim_ignition,omitempty"`
ExternalDNS []string `json:"openstack_external_dns,omitempty"`
MasterServerGroupID string `json:"openstack_master_server_group_id,omitempty"`
AdditionalNetworkIDs []string `json:"openstack_additional_network_ids,omitempty"`
AdditionalSecurityGroupIDs []string `json:"openstack_master_extra_sg_ids,omitempty"`
}

// TFVars generates OpenStack-specific Terraform variables.
func TFVars(masterConfig *v1alpha1.OpenstackProviderSpec, cloud string, externalNetwork string, externalDNS []string, lbFloatingIP string, apiVIP string, dnsVIP string, ingressVIP string, trunkSupport string, octaviaSupport string, baseImage string, infraID string, userCA string, bootstrapIgn string) ([]byte, error) {
func TFVars(masterConfig *v1alpha1.OpenstackProviderSpec, cloud string, externalNetwork string, externalDNS []string, lbFloatingIP string, apiVIP string, dnsVIP string, ingressVIP string, trunkSupport string, octaviaSupport string, baseImage string, infraID string, userCA string, bootstrapIgn string, mpool *types_openstack.MachinePool) ([]byte, error) {

cfg := &config{
ExternalNetwork: externalNetwork,
Expand Down Expand Up @@ -126,6 +129,20 @@ func TFVars(masterConfig *v1alpha1.OpenstackProviderSpec, cloud string, external

cfg.MasterServerGroupID = masterConfig.ServerGroupID

cfg.AdditionalNetworkIDs = []string{}
if mpool.AdditionalNetworkIDs != nil {
for _, networkID := range mpool.AdditionalNetworkIDs {
cfg.AdditionalNetworkIDs = append(cfg.AdditionalNetworkIDs, networkID)
}
}
abhinavdahiya marked this conversation as resolved.
Show resolved Hide resolved

cfg.AdditionalSecurityGroupIDs = []string{}
if mpool.AdditionalSecurityGroupIDs != nil {
for _, sgID := range mpool.AdditionalSecurityGroupIDs {
cfg.AdditionalSecurityGroupIDs = append(cfg.AdditionalSecurityGroupIDs, sgID)
}
}
Fedosin marked this conversation as resolved.
Show resolved Hide resolved

return json.MarshalIndent(cfg, "", " ")
}

Expand Down
19 changes: 19 additions & 0 deletions pkg/types/openstack/machinepool.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,17 @@ type MachinePool struct {
// The instances use ephemeral disks if not set.
// +optional
RootVolume *RootVolume `json:"rootVolume,omitempty"`

// AdditionalNetworkIDs contains IDs of additional networks for machines,
// where each ID is presented in UUID v4 format.
// Allowed address pairs won't be created for the additional networks.
// +optional
Fedosin marked this conversation as resolved.
Show resolved Hide resolved
AdditionalNetworkIDs []string `json:"additionalNetworkIDs,omitempty"`

// AdditionalSecurityGroupIDs contains IDs of additional security groups for machines,
// where each ID is presented in UUID v4 format.
// +optional
AdditionalSecurityGroupIDs []string `json:"additionalSecurityGroupIDs,omitempty"`
}

// Set sets the values from `required` to `a`.
Expand All @@ -30,6 +41,14 @@ func (o *MachinePool) Set(required *MachinePool) {
o.RootVolume.Size = required.RootVolume.Size
o.RootVolume.Type = required.RootVolume.Type
}

if required.AdditionalNetworkIDs != nil {
o.AdditionalNetworkIDs = append(required.AdditionalNetworkIDs[:0:0], required.AdditionalNetworkIDs...)
}

if required.AdditionalSecurityGroupIDs != nil {
o.AdditionalSecurityGroupIDs = append(required.AdditionalSecurityGroupIDs[:0:0], required.AdditionalSecurityGroupIDs...)
}
}

// RootVolume defines the storage for an instance.
Expand Down
31 changes: 31 additions & 0 deletions pkg/types/openstack/validation/machinepool.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package validation
import (
"k8s.io/apimachinery/pkg/util/validation/field"

guuid "github.com/google/uuid"
"github.com/openshift/installer/pkg/types/openstack"
)

Expand All @@ -20,5 +21,35 @@ func ValidateMachinePool(p *openstack.MachinePool, fldPath *field.Path) field.Er
}
}

allErrs = append(allErrs, validateUUIDV4s(p.AdditionalNetworkIDs, fldPath.Child("additionalNetworkIDs"))...)

Choose a reason for hiding this comment

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

Should we be using the valid values fetcher here to validate that these networks and security groups actually exist in openstack?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be done as a part of our Validation epic

allErrs = append(allErrs, validateUUIDV4s(p.AdditionalSecurityGroupIDs, fldPath.Child("additionalSecurityGroupIDs"))...)

return allErrs
}

func validateUUIDV4s(input []string, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}
for idx, uuid := range input {
if !validUUIDv4(uuid) {
allErrs = append(allErrs, field.Invalid(fldPath.Index(idx), uuid, "valid UUID v4 must be specified"))
}
}

return allErrs
}

// validUUIDv4 checks if string is in UUID v4 format
// For more information: https://en.wikipedia.org/wiki/Universally_unique_identifier#Version_4_(random)
func validUUIDv4(s string) bool {
uuid, err := guuid.Parse(s)
if err != nil {
return false
}

// check that version of the uuid
if uuid.Version().String() != "VERSION_4" {
return false
}

return true
}