-
Notifications
You must be signed in to change notification settings - Fork 39
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
Add PortSecurityEnabled to NetworksV2. #44
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, a few questions.
nova/nova.go
Outdated
Id: e.Id, | ||
Name: e.Name, | ||
} | ||
result := make([]SecurityGroup, len(*serverDetails.Groups)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to check for serverDetails.Groups
being nil here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, done.
External bool `json:"router:external"` // is this network connected to an external router | ||
AvailabilityZones []string `json:"availability_zones"` | ||
TenantId string `json:"tenant_id"` | ||
PortSecurityEnabled *bool `json:"port_security_enabled"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why a *bool
here rather than a bool
? (I guess you need for it to be nil-able?) Should External
be *bool
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PortSecurityEnabled is a *bool because it's optional depending on whether you are using the ML2 extension to neutron. If it's nil, you don't aren't using the extension. External is not part of an extension.
testservices/novaservice/service.go
Outdated
@@ -304,6 +305,16 @@ func (n *Nova) server(serverId string) (*nova.ServerDetail, error) { | |||
if !ok { | |||
return nil, testservices.NewServerByIDNotFoundError(serverId) | |||
} | |||
groups := n.allServerSecurityGroups(serverId) | |||
if len(groups) > 0 { | |||
groupNames := make([]nova.SecurityGroupName, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can leave out the 0 here. Alternatively if you want to avoid growing the slice you can specify the length of groups:
groupNames := make([]nova.SecurityGroupName, len(groups))
for i, group := range groups {
groupNames[i] = nova.SecurityGroupName{Name: group.Name})
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
tests accordiningly. Add PortSecurityEnabled to NetworksV2. Update testservices to not add SecurityGroups if PortSecurityEnabled is false and add server SecurityGroups to the ServerDetail returned.
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-goose |
... Update testservices to not add
SecurityGroups if PortSecurityEnabled is false and add server SecurityGroups
to the ServerDetail returned.
NeutronFilter should not be a regexp to match real life behavior. Update tests accordingly.
This is required to fix juju openstack provider bug: https://bugs.launchpad.net/juju/+bug/1680787.
Tested with test service and with juju against openstack networks with port_security_enabled set to true and false.