Skip to content

[WIP] Bug 1936511 Do not override net uuid if subnet uuid is defined. #177

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.

Sign up for GitHub

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

Closed
adduarte wants to merge 1 commit into from
Closed

[WIP] Bug 1936511 Do not override net uuid if subnet uuid is defined. #177

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion pkg/cloud/openstack/clients/machineservice.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,9 @@ func (is *InstanceService) InstanceCreate(clusterName string, name string, clust

for _, snetParam := range net.Subnets {
sopts := subnets.ListOpts(snetParam.Filter)
sopts.ID = snetParam.UUID
if snetParam.UUID == "" {
sopts.ID = snetParam.UUID
}
sopts.NetworkID = netID
Copy link
Copy Markdown
Author

@adduarte adduarte Apr 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If you look a the code before for networks the following entry will fail if the subnet does not belong to the network

ProviderSpec: ...
Networks: 
- UUID: my_net_uuid
  subnets:
         - uuid_of_subnet_not_under_my_net_uuid

It is even possible to create a problem if the subnet DOES belong to the net_uuid but the network filter returns more than one net.
For example, if for some reason there are two networks called "my_network" then the following entry will cause problems:

Providerspec:
     Networks: 
         - filter: 
               name: my_network
            subnets:
               - uuid: uuid_of_subnet_belonging_to_one_of_the_networks

Copy link
Copy Markdown
Author

@adduarte adduarte Apr 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

and one more It think:
again two networks with name "my_network"

Providerspec:
 Networks:
   - filter:
        - name: my_network
     subnets:
        - tags: primry-network

In this case oneport will be created correctly, while a second port will be created incorrectly (probably fail with an openstack error).
Note that I used network "name" to create the problem, but any filter under Networks, that returns more than one network will cause the same problem.

Copy link
Copy Markdown

@iamemilio iamemilio Apr 16, 2021

Choose a reason for hiding this comment

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

So, while it might not be the best user experience, because subnet is a neseted field under networks, it makes sense that if you define a network, then define a subnet that is not in that network, it will fail to create the resource. The easy solution is to just not set the network ID since it is not required, and a subnet can always be found by its ID. Also, all this code does is make it so you can never set the subnet ID...

Copy link
Copy Markdown
Author

@adduarte adduarte Apr 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

"all this code does is make it so you can never set the subnet iD" << that is incorrect.
What this code does is use the subnet.uuid defined by the user. Which is probably what the user intends.
If the user, does not define the subnet.uuid, then the network filters/and uuid (if provided) prevails.
But the moment a user defines a subnet.uuid under a network filter, that is what should override anything else. We don't need any other piece of information to attach the port. The subnet.uuid is the only piece of information we need to create the ports because there exists one and only one network with that subnet.uuid in its list of subnets.

The problem we have been seen as of late is that users are defining a network through a filter, then adding a subnet.uuid. In that case the subnet.uuid is what should be used because it is the more exact match.
Anything else in the providerspec.networks is superfluous to the subnet lookup. There is one and only one subnet that will match the subnet.uuid.

So, if the user is providing a subnet.uuid then there already exists a network uuid associated with that subnet.uuid. We should not be overriding the network.uuid in the struct representing the subnet. If we do so, we basically make that struct invalid, and no port will get connected, that is specially true if the user has defined a network filter which will return two or more networks. There can only be one network that matches the network filter and the subnet.uuid filter. Every other network will not be attached because the subnet.uuid will only match one network.
That becomes a problem has, for example, defined two networks with the same tag, and the providerspc.network filter is matching on tags. If the user defines a -subnet.uuid entry then only one network will be connected, which probably not what the user intended.

// Inherit portSecurity from network if unset on subnet
portSecurity := net.PortSecurity
Expand Down