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

Fix name in protocolAny #2442

Closed
HappyFX opened this issue Jul 19, 2022 · 17 comments · Fixed by #3157
Closed

Fix name in protocolAny #2442

HappyFX opened this issue Jul 19, 2022 · 17 comments · Fixed by #3157
Assignees

Comments

@HappyFX
Copy link
Contributor

HappyFX commented Jul 19, 2022

In documentation:
valid value for openstack cli is 'any'. In fact it's converted in to None and then in null:

debug from openstack call:

openstack security group rule create --egress --remote-ip 8.8.8.8/32 --protocol 'any' eece1c60-baf3-4ea9-88bf-54cc40290720

REQ: curl -g -i -X POST http://test:9696/v2.0/security-group-rules -H "Content-Type: application/json" -H "User-Agent: openstacksdk/0.99.0 keystoneauth1/4.6.0 python-requests/2.28.1 CPython/3.9.13" -H "X-Auth-Token: {SHA256}547fc1809352d6d9431590520fb0923bbf92d25fbe73af21eb488e1b71e67e52" -d '{"security_group_rule": {"security_group_id": "eece1c60-baf3-4ea9-88bf-54cc40290720", "ethertype": "IPv4", "protocol": null, "direction": "egress", "remote_ip_prefix": "8.8.8.8/32"}}'

@pierreprinetti
Copy link
Contributor

You are saying that the openstack CLI is converting "any" to null.

However, the docs explicitly mention that "any" is like 0, and seem to imply that not passing the property (or passing null as a value) has the same effect. That is, applying the security group rule regardless of the protocol.

What improvement do you expect once Gophercloud sends null instead of "any" to the Neutron API?

@HappyFX
Copy link
Contributor Author

HappyFX commented Jul 20, 2024

Hi, general in wiki 0 protocol is valid number for IPv6 Hop-by-Hop Option

So far I found in horizon valid also -1 (instead of openstack cli) and it's the correct way but different endpoint is used (/project/security_groups/9c516eff-e9d0-4fea-8bd5-424d1e4dd726/add_rule/):

rule_menu=custom&description=&direction=ingress&ip_protocol=-1&port_or_range=port&port=&from_port=&to_port=&icmp_type=&icmp_code=&remote=cidr&cidr=0.0.0.0%2F0&security_group=1daa9847-9e30-4911-99db-b170f9a60ac1&ethertype=IPv4

For any and 0 results are different in database

  1. with 0:
openstack security group rule create --egress --remote-ip 8.8.8.8/32 --protocol 0 9c516eff-e9d0-4fea-8bd5-424d1e4dd726
REQ: curl -g -i -X POST https://test:9696/v2.0/security-group-rules -H "Content-Type: application/json" -H "User-Agent: openstacksdk/0.61.0 keystoneauth1/5.2.1 python-requests/2.31.0 CPython/3.11.7" -H "X-Auth-Token: {SHA256}752e3f1550a3673beed7e5dff6f1213a096619b792c1785cf6f0355b85abef99" -d '{"security_group_rule": {"security_group_id": "9c516eff-e9d0-4fea-8bd5-424d1e4dd726", "ethertype": "IPv4", "direction": "egress", "protocol": "0", "remote_ip_prefix": "8.8.8.8/32"}}'
RESP BODY: {"security_group_rule": {"id": "475444d9-3ff4-4504-9a00-08da7b3b6410", "tenant_id": "a4e7c4dfbf314a2baab5689ce3a6ee64", "security_group_id": "9c516eff-e9d0-4fea-8bd5-424d1e4dd726", "ethertype": "IPv4", "direction": "egress", "protocol": "0", "port_range_min": null, "port_range_max": null, "remote_ip_prefix": "8.8.8.8/32", "remote_address_group_id": null, "normalized_cidr": "8.8.8.8/32", "remote_group_id": null, "description": "", "created_at": "2024-07-20T14:47:46Z", "updated_at": "2024-07-20T14:47:46Z", "revision_number": 0, "project_id": "a4e7c4dfbf314a2baab5689ce3a6ee64"}}
+-------------------------+--------------------------------------+
| Field                   | Value                                |
+-------------------------+--------------------------------------+
| created_at              | 2024-07-20T14:47:46Z                 |
| description             |                                      |
| direction               | egress                               |
| ether_type              | IPv4                                 |
| id                      | 475444d9-3ff4-4504-9a00-08da7b3b6410 |
| name                    | None                                 |
| port_range_max          | None                                 |
| port_range_min          | None                                 |
| project_id              | a4e7c4dfbf314a2baab5689ce3a6ee64     |
| protocol                | 0                                    |
| remote_address_group_id | None                                 |
| remote_group_id         | None                                 |
| remote_ip_prefix        | 8.8.8.8/32                           |
| revision_number         | 0                                    |
| security_group_id       | 9c516eff-e9d0-4fea-8bd5-424d1e4dd726 |
| tags                    | []                                   |
| tenant_id               | a4e7c4dfbf314a2baab5689ce3a6ee64     |
| updated_at              | 2024-07-20T14:47:46Z                 |
+-------------------------+--------------------------------------+
  1. with any:
openstack security group rule create --egress --remote-ip 8.8.8.8/32 --protocol any 9c516eff-e9d0-4fea-8bd5-424d1e4dd726
REQ: curl -g -i -X POST https://test:9696/v2.0/security-group-rules -H "Content-Type: application/json" -H "User-Agent: openstacksdk/0.61.0 keystoneauth1/5.2.1 python-requests/2.31.0 CPython/3.11.7" -H "X-Auth-Token: {SHA256}ba5f60743374d5809fa394479b28866ac71ed97e1a11b888391fc68770692dc4" -d '{"security_group_rule": {"remote_ip_prefix": "8.8.8.8/32", "ethertype": "IPv4", "protocol": null, "direction": "egress", "security_group_id": "9c516eff-e9d0-4fea-8bd5-424d1e4dd726"}}'
RESP BODY: {"security_group_rule": {"id": "bb6aa7b7-1b10-4ae0-84d0-fe080a8c623b", "tenant_id": "a4e7c4dfbf314a2baab5689ce3a6ee64", "security_group_id": "9c516eff-e9d0-4fea-8bd5-424d1e4dd726", "ethertype": "IPv4", "direction": "egress", "protocol": null, "port_range_min": null, "port_range_max": null, "remote_ip_prefix": "8.8.8.8/32", "remote_address_group_id": null, "normalized_cidr": "8.8.8.8/32", "remote_group_id": null, "description": "", "created_at": "2024-07-20T14:48:03Z", "updated_at": "2024-07-20T14:48:03Z", "revision_number": 0, "project_id": "a4e7c4dfbf314a2baab5689ce3a6ee64"}}
+-------------------------+--------------------------------------+
| created_at              | 2024-07-20T14:48:03Z                 |
| description             |                                      |
| direction               | egress                               |
| ether_type              | IPv4                                 |
| id                      | bb6aa7b7-1b10-4ae0-84d0-fe080a8c623b |
| name                    | None                                 |
| port_range_max          | None                                 |
| port_range_min          | None                                 |
| project_id              | a4e7c4dfbf314a2baab5689ce3a6ee64     |
| protocol                | None                                 |
| remote_address_group_id | None                                 |
| remote_group_id         | None                                 |
| remote_ip_prefix        | 8.8.8.8/32                           |
| revision_number         | 0                                    |
| security_group_id       | 9c516eff-e9d0-4fea-8bd5-424d1e4dd726 |
| tags                    | []                                   |
| tenant_id               | a4e7c4dfbf314a2baab5689ce3a6ee64     |
| updated_at              | 2024-07-20T14:48:03Z                 |
+-------------------------+--------------------------------------+
  1. with -1:
openstack security group rule create --egress --remote-ip 8.8.8.8/32 --protocol -1 9c516eff-e9d0-4fea-8bd5-424d1e4dd726

REQ: curl -g -i -X POST https://test:9696/v2.0/security-group-rules -H "Content-Type: application/json" -H "User-Agent: openstacksdk/0.61.0 keystoneauth1/5.2.1 python-requests/2.31.0 CPython/3.11.7" -H "X-Auth-Token: {SHA256}475ee8619e3304d95ecf5a91b35adbc45db0ba067e9b087591c4df8ff0df5f38" -d '{"security_group_rule": {"ethertype": "IPv4", "remote_ip_prefix": "8.8.8.8/32", "security_group_id": "9c516eff-e9d0-4fea-8bd5-424d1e4dd726", "protocol": "-1", "direction": "egress"}}'
RESP BODY: {"NeutronError": {"type": "SecurityGroupRuleInvalidProtocol", "message": "Security group rule protocol -1 not supported. Only protocol values [None, 'ah', 'dccp', 'egp', 'esp', 'gre', 'hopopt', 'icmp', 'igmp', 'ip', 'ipip', 'ipv6-encap', 'ipv6-frag', 'ipv6-icmp', 'icmpv6', 'ipv6-nonxt', 'ipv6-opts', 'ipv6-route', 'ospf', 'pgm', 'rsvp', 'sctp', 'tcp', 'udp', 'udplite', 'vrrp'] and integer representations [0 to 255] are supported.", "detail": ""}}

Error while executing command: BadRequestException: 400, Security group rule protocol -1 not supported. Only protocol values [None, 'ah', 'dccp', 'egp', 'esp', 'gre', 'hopopt', 'icmp', 'igmp', 'ip', 'ipip', 'ipv6-encap', 'ipv6-frag', 'ipv6-icmp', 'icmpv6', 'ipv6-nonxt', 'ipv6-opts', 'ipv6-route', 'ospf', 'pgm', 'rsvp', 'sctp', 'tcp', 'udp', 'udplite', 'vrrp'] and integer representations [0 to 255] are supported.

@HappyFX
Copy link
Contributor Author

HappyFX commented Jul 20, 2024

What improvement do you expect once Gophercloud sends null instead of "any" to the Neutron API?

In terraform provider no option to create security group with protocol "any" - please see the issue

@pierreprinetti
Copy link
Contributor

pierreprinetti commented Jul 21, 2024

RuleProtocol is a string in Gopherloud; using nil will simply not compile. If you need to pass null (or to not pass the property), you can override the property in a custom CreateOpts:

EDIT: see below. The example that was here was not useful

Note that if you find a functional reason to do that, we might as well change Protocol to be a pointer in v3 to accomodate for a nil value. Or maybe, rather, raise a bug in Neutron about it.

@pierreprinetti pierreprinetti self-assigned this Jul 21, 2024
@pierreprinetti pierreprinetti added the needinfo Additional information requested label Jul 22, 2024
@HappyFX
Copy link
Contributor Author

HappyFX commented Jul 24, 2024

raise a bug in Neutron about it

I fear it won't be fixed, because it's not a bug for python, only problem for typization in go.

Correct way is to add in gophercloud supporting passing nil value in json body

@kayrus
Copy link
Contributor

kayrus commented Jul 24, 2024

There is a way to implement an own CreateOptsBuilder interface that does this. It could be done in openstack terraform provider as well, without dependencies from gophercloud.

@pierreprinetti
Copy link
Contributor

pierreprinetti commented Jul 24, 2024

There is a way to implement an own CreateOptsBuilder interface that does this

There you go:

type RuleCreateOpts rules.CreateOpts

func (o RuleCreateOpts) ToSecGroupRuleCreateMap() (map[string]interface{}, error) {
	m, err := rules.CreateOpts(o).ToSecGroupRuleCreateMap()

	if err != nil {
		return nil, err
	}

	if o.Protocol == "" {
		m["security_group_rule"].(map[string]any)["protocol"] = nil
	}
	return m, nil
}

kmlebedev pushed a commit to kmlebedev/gophercloud that referenced this issue Jul 24, 2024
@pierreprinetti
Copy link
Contributor

raise a bug in Neutron about it

I fear it won't be fixed, because it's not a bug for python, only problem for typization in go.

Correct way is to add in gophercloud supporting passing nil value in json body

I will assume you mean null in the JSON body.

Can you please explain why you say that null would be the correct value of protocol when you want to create a security group rule that applies to all protocols?

The documentation of the Neutron API clearly states:

The string any (or integer 0) means all IP protocols

@HappyFX
Copy link
Contributor Author

HappyFX commented Jul 24, 2024

Can you please explain why you say that null would be the correct value of protocol when you want to create a security group rule that applies to all protocols?

I have checked in openstack, when you push 0 and null in field protocol it's different results.

Also it's looks like a bug in documentation openstack about 0 is any. Take a look in openstack python client converting any to null

Check neutron lib it's also bug with two valid NUM for 0:

PROTO_NUM_HOPOPT = 0
PROTO_NUM_IP = 0

and I have created in my openstack two groups. One with null and one with 0.

With 0 - not working, with null working. In web interface looks like:

Screenshot 2024-07-24 at 19 46 05

@HappyFX
Copy link
Contributor Author

HappyFX commented Jul 24, 2024

I have managed to record gif, if no access to running openstack.
sec group

@pierreprinetti
Copy link
Contributor

pierreprinetti commented Jul 25, 2024

OK I may have found the problem you want to fix.

I have tried creating a group and a rule with this code:

	group, err := groups.Create(ctx, networkClient, groups.CreateOpts{
		Name:        "this-is-a-test",
		Description: "delete me",
	}).Extract()
	if err != nil {
		panic(err)
	}

	rule1, err := rules.Create(ctx, networkClient, rules.CreateOpts{
		Direction:    rules.DirEgress,
		Description:  "delete me",
		EtherType:    rules.EtherType4,
		SecGroupID:   group.ID,
		PortRangeMax: 4000,
		PortRangeMin: 4000,
		Protocol:     rules.ProtocolAny,
	}).Extract()
	if err != nil {
		panic(err)
	}

And what I got is this error message:

panic: Expected HTTP response code [201 202] when accessing [POST https://neutron.example:13696/v2.0/security-group-rules], but got 400 instead: {"NeutronError": {"type": "SecurityGroupRuleInvalidProtocol", "message": "Security group rule protocol any not supported. Only protocol values [None, 'ah', 'dccp', 'egp', 'esp', 'gre', 'hopopt', 'icmp', 'igmp', 'ip', 'ipip', 'ipv6-encap', 'ipv6-frag', 'ipv6-icmp', 'icmpv6', 'ipv6-nonxt', 'ipv6-opts', 'ipv6-route', 'ospf', 'pgm', 'rsvp', 'sctp', 'tcp', 'udp', 'udplite', 'vrrp'] and integer representations [0 to 255] are supported.", "detail": ""}}

@pierreprinetti
Copy link
Contributor

pierreprinetti commented Jul 25, 2024

I have filed this bug against Neutron: https://bugs.launchpad.net/neutron/+bug/2074056

Based on my current understanding of the issue (which I ideally want confirmed by the Neutron team), you can use Gophercloud to create a rule that applies regardless of the protocol, by not setting the Protocol property:

	rule, err := rules.Create(ctx, networkClient, rules.CreateOpts{
		Direction:    rules.DirIngress,
		EtherType:    rules.EtherType4,
		SecGroupID:   group.ID,
	}).Extract()

Not setting Property means that you implicitly set it to the zero value for the type, which is not sent over the wire by default. Not sending it seems to be equivalent to sending null.

This is the code with which I have tested that protocol is not sent when not set in Gophercloud:

package main

import (
	"context"
	"fmt"
	"io"
	"net/http"
	"os"

	"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/groups"
	"github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/rules"
)

type logger struct {
	http.RoundTripper
}

func newLogger(rt http.RoundTripper) *logger {
	return &logger{rt}
}

type readcloser struct {
	io.Reader
	io.Closer
}

func (l *logger) RoundTrip(req *http.Request) (*http.Response, error) {
	if req.Body != nil {
		r := io.TeeReader(req.Body, os.Stderr)
		req.Body = readcloser{r, req.Body}
	}
	res, err := l.RoundTripper.RoundTrip(req)
	os.Stderr.Write([]byte{'\n'})
	return res, err
}

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)
	}

	httpClient := providerClient.HTTPClient
	httpClient.Transport = newLogger(httpClient.Transport)
	providerClient.HTTPClient = httpClient

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

	group, err := groups.Create(ctx, networkClient, groups.CreateOpts{
		Name:        "rules-any-test",
		Description: "delete me",
	}).Extract()
	if err != nil {
		panic(err)
	}

	fmt.Printf("Created group %q\n", group.ID)

	rule, err := rules.Create(ctx, networkClient, rules.CreateOpts{
		Direction:  rules.DirIngress,
		EtherType:  rules.EtherType4,
		SecGroupID: group.ID,
	}).Extract()
	if err != nil {
		panic(err)
	}

	fmt.Printf("Created rule %q\n", rule.ID)
}

Based on my findings:

  • Without any change in Gophercloud, you can already achieve what you want by not setting the Property value of RuleCreateOpts.
  • The value rules.ProtocolAny does not work, and we will need to address the problem to avoid confusion.

@HappyFX @kmlebedev thoughts?

@pierreprinetti
Copy link
Contributor

There are further issues with listing, and as @kayrus notes, comparing rules with each other.

Listing all rules that apply to any protocol is tricky because based on how the rule was created, the value of protocol can either be 0, null or "". Filtering by 0 will only return the first case. Filtering by the empty string might work, but I haven't tried. Good luck with filtering by null against the HTTP API.

Comparing is something Gophercloud might assist with, by artificially setting a well-known value (like "") when getting any of the three possible values from Neutron. I am not 100% convinced by this solution, because a user might want the raw response in some cases.

I really look forward to an answer by the Neutron folks, if we ever get one.

@HappyFX
Copy link
Contributor Author

HappyFX commented Jul 25, 2024

Without any change in Gophercloud, you can already achieve what you want by not setting the Property value of RuleCreateOpts

I guess we need documentation for "hack" to know how it can go in terraform provider as well when protocol can be null, "" or omitted to have "what ever protocol"

I really look forward to an answer by the Neutron folks, if we ever get one.

Even if they fix this bug to use protocol -1 for example explicitly it will be not so fast :(

@pierreprinetti
Copy link
Contributor

pierreprinetti commented Jul 25, 2024

@HappyFX

Even if they fix this bug to use protocol -1 for example explicitly it will be not so fast :(

To be honest my ask is just that the API documentation be amended with one recommended (and working) way to set "all protocols".

If they really want to go the extra mile, they could make it so that whatever value of protocol you use to say "any", it is persisted the same way. But from the Gophercloud point of view that's not very relevant for now, as we will continue supporting old (unfixed) versions for a long time.

@HappyFX
Copy link
Contributor Author

HappyFX commented Jul 25, 2024

For now:

  1. omit protocol == any
  2. protocol = "0" == protocol = 0x00 it's HOPOPT and not act as any
  3. protocol = null == any

To clear all this cases and make it obvious, I suggest:

  1. make protocol no more optional
  2. cases when it was omitted or null convert to be any
  3. cases when it was a number (even 0) are still valid and act as it was

@pierreprinetti
Copy link
Contributor

Why would protocol not be optional any more? Not setting it is exactly like setting "": it will be omitted in the call. If you don't set a protocol, then it's "any"... exactly like in the HTTP API.

Regarding the numeric protocols: I have tried using curl to send numbers in strings (e.g. "6" for TCP) and Neutron seems to be fine with it. Which means that you can already send numeric protocols with Gophercloud like this:

	rule, err := rules.Create(ctx, networkClient, rules.CreateOpts{
		Direction:  rules.DirIngress,
		EtherType:  rules.EtherType4,
		SecGroupID: group.ID,
		Protocol: rules.RuleProtocol("6"), // for TCP
	}).Extract()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants