Skip to content

SG rules: implement bulk create#3267

Merged
pierreprinetti merged 2 commits intogophercloud:masterfrom
shiftstack:sg-rule-bulk-create
Dec 10, 2024
Merged

SG rules: implement bulk create#3267
pierreprinetti merged 2 commits intogophercloud:masterfrom
shiftstack:sg-rule-bulk-create

Conversation

@mandre
Copy link
Copy Markdown
Contributor

@mandre mandre commented Dec 6, 2024

Implement bulk creation of security group rules [1].

[1] https://docs.openstack.org/api-ref/network/v2/#bulk-create-security-group-rule

@github-actions github-actions bot added edit:networking This PR updates networking code semver:minor Backwards-compatible change labels Dec 6, 2024
// CreateBulk is an operation which adds new security group rules and associates them
// with an existing security group (whose ID is specified in CreateOpts).
func CreateBulk(ctx context.Context, c *gophercloud.ServiceClient, opts []CreateOpts) (r CreateResult) {
createOpts := make([]map[string]any, len(opts))
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Alternatively, we can modify gophercloud.BuildRequestBody() to accept an array/slice. Perhaps this would be more user friendly? Do we have a preference?

The nice thing if we decide to make gophercloud.BuildRequestBody() accept slices or arrays, is that it does not require changing the signature function, allowing us to backport the change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@coveralls
Copy link
Copy Markdown

coveralls commented Dec 6, 2024

Coverage Status

coverage: 78.648% (-0.03%) from 78.681%
when pulling 6e6dbe5 on shiftstack:sg-rule-bulk-create
into 1678b7f on gophercloud:master.

@mandre mandre force-pushed the sg-rule-bulk-create branch 2 times, most recently from 2671e82 to 324a110 Compare December 6, 2024 17:14
Copy link
Copy Markdown
Member

@pierreprinetti pierreprinetti left a comment

Choose a reason for hiding this comment

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

What happens when you pass two rules.CreateOpts that refer each to a different, existing security group ID?

@mandre
Copy link
Copy Markdown
Contributor Author

mandre commented Dec 6, 2024

What happens when you pass two rules.CreateOpts that refer each to a different, existing security group ID?

I haven't tried but I understand they can be different, from the doc.

@pierreprinetti
Copy link
Copy Markdown
Member

nope.

$ curl -sS \
        -X POST \
        -H "X-Auth-Token: $TOKEN" \
        -H "content-type: application/json" \
        --data-binary @sg.json \
        "${NEUTRON_URL}/v2.0/security-group-rules" \
        | jq
{
  "NeutronError": {
    "type": "SecurityGroupNotSingleGroupRules",
    "message": "Only allowed to update rules for one security profile at a time",
    "detail": ""
  }
}

@mandre
Copy link
Copy Markdown
Contributor Author

mandre commented Dec 8, 2024

Perhaps, but that's none of gophercloud concerns, is it?

@pierreprinetti
Copy link
Copy Markdown
Member

I also woulnd't preemptively block invalid calls; however it may be useful to note the limitation in a comment

When passing an array or a slice to `BuildRequestBody()`, the `parent`
string becomes mandatory.

This makes it possible to build requests for bulk operations.
@mandre mandre force-pushed the sg-rule-bulk-create branch from 324a110 to b1f8bbb Compare December 8, 2024 21:44
@github-actions github-actions bot added the edit:gophercloud This PR updates common Gophercloud code label Dec 8, 2024
@pierreprinetti pierreprinetti added the backport-v2 This PR will be backported to v2 label Dec 9, 2024
@pierreprinetti pierreprinetti self-assigned this Dec 9, 2024
@pierreprinetti
Copy link
Copy Markdown
Member

TL;DR: The Extract() method of the return type of CreateBulkResult should return a slice of rules and an error.

In other words, I would suggest that CreateBulkResult(/* ... */) returns CreateBulkResult and that CreateBulkResult has a method with signature Extract() ([]rules.SecGroupRule, error).


I did a couple tests with cURL. The call looked like this:

curl -sSi \
        -X POST \
        -H "X-Auth-Token: $(openstack token issue -f value -c id)" \
        -H "content-type: application/json" \
        --data-binary @sg.json \
        "$(openstack catalog show network --format json | jq -r '.endpoints[] | select(.interface == "public") | .url')/v2.0/security-group-rules"

When the endpoint is called with a valid payload, it returns a JSON slice containing all the created rules.

For example, when sg.json looks like this (valid payload):

{
    "security_group_rules": [
        {
            "direction": "ingress",
            "port_range_min": "443",
            "ethertype": "IPv4",
            "port_range_max": "443",
            "protocol": "tcp",
            "security_group_id": "f9202b90-a34e-4804-9b72-60a9cda7bcc1"
        },
        {
            "direction": "ingress",
            "port_range_min": "80",
            "ethertype": "IPv4",
            "port_range_max": "80",
            "protocol": "tcp",
            "security_group_id": "f9202b90-a34e-4804-9b72-60a9cda7bcc1"
        }
    ]
}

...then the response looks like this:

HTTP/1.1 201 Created
Content-Type: application/json
Content-Length: 996
X-Openstack-Request-Id: req-bb55ef49-93e7-4ac3-9b9f-9fd424d50212
Date: Mon, 09 Dec 2024 14:22:03 GMT

{"security_group_rules": [{"id": "cf2ce71c-4c36-40d9-ab03-13afde7cb4ea", "tenant_id": "90dce24f8e6748bfba319a9223d0a7a6", "security_group_id": "f9202b90-a34e-4804-9b72-60a9cda7bcc1", "ethertype": "IPv4", "direction": "ingress", "protocol": "tcp", "port_range_min": 443, "port_range_max": 443, "remote_ip_prefix": null, "remote_group_id": null, "description": "", "created_at": "2024-12-09T14:22:02Z", "updated_at": "2024-12-09T14:22:02Z", "revision_number": 0, "project_id": "90dce24f8e6748bfba319a9223d0a7a6"}, {"id": "4d6432c9-e1a0-4861-acf0-8be423f68f25", "tenant_id": "90dce24f8e6748bfba319a9223d0a7a6", "security_group_id": "f9202b90-a34e-4804-9b72-60a9cda7bcc1", "ethertype": "IPv4", "direction": "ingress", "protocol": "tcp", "port_range_min": 80, "port_range_max": 80, "remote_ip_prefix": null, "remote_group_id": null, "description": "", "created_at": "2024-12-09T14:22:02Z", "updated_at": "2024-12-09T14:22:02Z", "revision_number": 0, "project_id": "90dce24f8e6748bfba319a9223d0a7a6"}]}

However when the payload contains one invalid security group rule, in whichever position:

{
    "security_group_rules": [
        {
            "direction": "ingress",
            "port_range_min": "443",
            "ethertype": "IPv4",
            "port_range_max": "443",
            "protocol": "tcp",
            "security_group_id": "f9202b90-a34e-4804-9b72-60a9cda7bcc1"
        },
        {
            "direction": "ingress",
            "port_range_min": "81",
            "ethertype": "IPv4",
            "port_range_max": "79",
            "protocol": "tcp",
            "security_group_id": "f9202b90-a34e-4804-9b72-60a9cda7bcc1"
        }
    ]
}

...then none of them is created, and the response looks like this:

HTTP/1.1 400 Bad Request
Content-Type: application/json
Content-Length: 151
X-Openstack-Request-Id: req-867397b4-56e2-4f20-842a-3f92c9aae658
Date: Mon, 09 Dec 2024 14:24:46 GMT

{"NeutronError": {"type": "SecurityGroupInvalidPortRange", "message": "For TCP/UDP protocols, port_range_min must be <= port_range_max", "detail": ""}}

I have also tested a payload with two errored rules:

  1. one valid rule
  2. one rule with a different secgroup ID
  3. one rule with ports max<min

and only one error is returned (in my case that was the first encountered, which is the different secgroup ID).

Lastly, an empty slice of rules:

{"security_group_rules":[]}

Result:

HTTP/1.1 400 Bad Request
Content-Length: 91
Content-Type: application/json
X-Openstack-Request-Id: req-e493c35c-a5da-4f36-baf1-6c40a1aac118
Date: Mon, 09 Dec 2024 14:32:02 GMT

{"NeutronError": {"type": "HTTPBadRequest", "message": "Resources required", "detail": ""}}

Based on these experiments, I think it's fair to say that as long as Neutron is healthy there are only two possible results to a call to the bulk-create endpoint:

  • EITHER an HTTP code of 201 and a JSON array of the created rules;
  • OR exactly one error.

@pierreprinetti
Copy link
Copy Markdown
Member

oh you have added ExtractRules 😬 that's cheating

@pierreprinetti
Copy link
Copy Markdown
Member

Can you please change the return type of CreateBulk so that its Extract method returns the slice'? Having a shared object on which you can call both Extract and ExtractRules seems a footgun.

@mandre mandre force-pushed the sg-rule-bulk-create branch from b1f8bbb to 6e6dbe5 Compare December 9, 2024 18:10
Copy link
Copy Markdown
Member

@pierreprinetti pierreprinetti left a comment

Choose a reason for hiding this comment

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

Nice, thank you.

Tested OK. Rules correctly applied, return values populated OK.
I still wonder what happens if you reach the pagination threshold (provided there is any), but I couldn't reach that with "just" 1200 rules (which took 9m22s to apply 😅)

@pierreprinetti pierreprinetti merged commit db06a86 into gophercloud:master Dec 10, 2024
@pierreprinetti pierreprinetti deleted the sg-rule-bulk-create branch December 10, 2024 13:26
@pierreprinetti
Copy link
Copy Markdown
Member

FTR tested with this.

package main

import (
	"context"
	"fmt"
	"slices"
	"strconv"

	"github.com/gophercloud/gophercloud/v2/openstack"
	"github.com/gophercloud/gophercloud/v2/openstack/config"
	"github.com/gophercloud/gophercloud/v2/openstack/config/clouds"
	"github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/rules"
)

const sgID = "5eddd6ae-bad2-4290-9361-5ae5552fc1e7"

func main() {
	ctx := context.Background()
	authOptions, endpointOptions, tlsConfig, err := clouds.Parse()
	if err != nil {
		panic(err)
	}

	providerClient, err := config.NewProviderClient(ctx, authOptions, config.WithTLSConfig(tlsConfig))
	if err != nil {
		panic(err)
	}

	client, err := openstack.NewNetworkV2(providerClient, endpointOptions)
	if err != nil {
		panic(err)
	}

	createOpts := make([]rules.CreateOpts, 1200)

	for i := range createOpts {
		createOpts[i] = rules.CreateOpts{
			Direction:    rules.DirIngress,
			Description:  "Rule number " + strconv.Itoa(i),
			EtherType:    rules.EtherType6,
			SecGroupID:   sgID,
			PortRangeMax: i,
			PortRangeMin: i,
			Protocol:     rules.ProtocolUDP,
		}
	}

	sgs, err := rules.CreateBulk(ctx, client, createOpts).Extract()
	if err != nil {
		panic(err)
	}

	if want, got := len(createOpts), len(sgs); want != got {
		fmt.Printf("ERROR: expected %d rules, got %d\n", want, got)
	}

	slices.SortFunc(sgs, func(a, b rules.SecGroupRule) int { return a.PortRangeMin - b.PortRangeMin })

	for i := range sgs {
		if want, got := createOpts[i].PortRangeMin, sgs[i].PortRangeMin; want != got {
			fmt.Printf("Expected %d, got %d\n", want, got)
		}
	}

	fmt.Printf("DONE. Successfully created %d rules and got %d returned rules.\n", len(createOpts), len(sgs))
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-v2 This PR will be backported to v2 edit:gophercloud This PR updates common Gophercloud code edit:networking This PR updates networking code semver:minor Backwards-compatible change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants