Create Port Forwarding For Floating IP#1651
Conversation
|
@ozerovandrei @jtopjian This is ready for review I think. The acceptance test will fail because the port forwarding can't get deleted (fixed with #1652 ) |
|
Build failed.
|
jtopjian
left a comment
There was a problem hiding this comment.
@simonre Thank you for working on this 🙂
I think this should be moved to a dedicated extension at openstack/networking/v2/extensions/layer3/portforwarding. That should simplify the function names (Create instead of CreatePortForwarding) and simplify the results.go file to prevent any overlap/overloading.
Let me know if you have any questions.
| // resource. All attributes are required. | ||
| type CreatePortForwardingOpts struct { | ||
| InternalPortID string `json:"internal_port_id"` | ||
| InternalIPAdress string `json:"internal_ip_address"` |
|
|
||
| // CreatePortForwardingOpts contains all the values needed to create a new port forwarding | ||
| // resource. All attributes are required. | ||
| type CreatePortForwardingOpts struct { |
There was a problem hiding this comment.
It looks like ProjectID is also a valid field here: https://github.com/openstack/neutron-lib/blob/master/neutron_lib/api/definitions/floating_ip_port_forwarding.py#L86
| return r.(FloatingIPPage).Result.ExtractIntoSlicePtr(v, "floatingips") | ||
| } | ||
|
|
||
| type PortForwarding struct { |
There was a problem hiding this comment.
ProjectID can be added here.
It's also possible that CreatedAt and UpdatedAt are valid fields, too. I can't find the exact reference, but most Neutron results have those fields as standard. It's worth adding them to see if they get populated. Or validate with debug output.
There was a problem hiding this comment.
I tried it out, they do not get populated. This is the Openstack response body when running the acceptance test:
[DEBUG] OpenStack Response Body: { "port_forwarding": { "external_port": 2230, "id": "b97059a6-29ab-40d7-80b4-86db42cc6104", "internal_ip_address": "192.168.36.152", "internal_port": 25, "internal_port_id": "2d973a15-e95f-432e-bdc8-993bb69c993f", "protocol": "tcp" }
There was a problem hiding this comment.
Thank you for confirming. 🙂
| } | ||
|
|
||
| // ExtractPortForwarding will extract a Port Forwarding resource from a result. | ||
| func (r commonResult) ExtractPortForwarding() (*PortForwarding, error) { |
There was a problem hiding this comment.
This is inheriting from the Floating IP create result which is different than the PortForwarding result.
To prevent overlap like this, I recommend creating a new neutron extension at openstack/networking/v2/extensions/layer3/portforwarding.
|
|
||
| // PortFowardingPage is the page returned by a pager when traversing over a | ||
| // collection of Port Forwardings. | ||
| type PortForwardingPage struct { |
There was a problem hiding this comment.
This is only needed if there is a paginated List call which I don't see in this PR? Probably useful for a future PR/commit, though.
There was a problem hiding this comment.
I'll remove it and add it in the List PR
|
@jtopjian Is there a rule about when resources should be added as separate resources as opposed to integrating them into other resources? I thought this would be integrated, similar to the keymanager/secrets metadata resource. I'll move it to a dedicated extension, just wondering for future PRs |
|
@simonre It all depends on how they're defined in the API. For neutron, most all features are defined as separate extensions: |
b8dd42f to
51917d9
Compare
|
@jtopjian Sorry for taking so long with this. This is ready for review. |
|
Build failed.
|
|
@simonre Thanks! It looks like OpenLab doesn't have port forwarding enabled right now, but I'll see if I can get that resolved. I'll also have to update my test environment to Rocky so that I can test this offline. |
jtopjian
left a comment
There was a problem hiding this comment.
I had a chance to test this. Looks good - just two small comments.
| package portforwarding enables management and retrieval of port forwarding resources for Floating IPs from the | ||
| OpenStack Networking service. | ||
|
|
||
| Example to Create a Port Forwarding |
There was a problem hiding this comment.
It looks like this needs updated?
| InternalIPAddress string `json:"internal_ip_address"` | ||
|
|
||
| // The project ID | ||
| ProjectID string `json:"project_id"` |
There was a problem hiding this comment.
I'm not seeing project_id in either the create results or a GET result:
2019/09/01 00:17:47 [DEBUG] OpenStack Response Body: {
"port_forwarding": {
"external_port": 2230,
"id": "ece88ca6-0d5b-4fbd-86fb-859104a1fbb6",
"internal_ip_address": "192.168.21.6",
"internal_port": 25,
"internal_port_id": "6bf31ca9-b05e-44b2-8238-45a71086351e",
"protocol": "tcp"
}
}
Are you seeing it in your environment?
There was a problem hiding this comment.
No, I am not, I added it as per your comment here: #1651 (comment) . I removed it again in the latest commit.
There was a problem hiding this comment.
This is the first I've run into Neutron not following a defined model like this. I apologize about that.
|
@simonre I opened an issue with OpenLab about enabling port forwarding in the test environment. Until that's configured, we might have to do something like this: |
|
Build failed.
|
|
@jtopjian I fixed the things you mentioned, I'm not entirely sure why the acceptance test is failing though. |
There's a problem with the Ironic acceptance test suite. I opened a ticket about it, but might disable it for the time being. |
For #1637
Links to the line numbers/files in the OpenStack source code that support the
code in this PR:
Code:
https://github.com/openstack/neutron/blob/f21444f247108212dc835036c98e354d6e6f7ace/neutron/services/portforwarding/pf_plugin.py#L322
API:
https://developer.openstack.org/api-ref/network/v2/?expanded=create-port-forwarding-detail#create-port-forwarding