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

Vpnaas: Update Service #784

Merged
merged 1 commit into from
Mar 27, 2018
Merged

Conversation

simonre
Copy link
Contributor

@simonre simonre commented Feb 21, 2018

@simonre simonre changed the title Vpnaas service update [WIP] Vpnaas: Update Service Feb 21, 2018
@coveralls
Copy link

coveralls commented Feb 21, 2018

Coverage Status

Coverage increased (+0.005%) to 73.877% when pulling 3b8ddac on simonre:vpnaas-service-update into 2b0354d on gophercloud:master.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 21, 2018

Build succeeded.

@simonre
Copy link
Contributor Author

simonre commented Feb 21, 2018

@jtopjian This is ready for review

@simonre simonre changed the title [WIP] Vpnaas: Update Service Vpnaas: Update Service Feb 21, 2018
@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 21, 2018

Build succeeded.

Copy link
Contributor

@jtopjian jtopjian left a comment

Choose a reason for hiding this comment

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

@simonre I've left a few comments here. Let me know if you have any questions. :)

// UpdateOpts contains the values used when updating a VPN service
type UpdateOpts struct {
// Name is the human readable name of the service.
Name string `json:"name,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to not have a Name? If so, let's say a user originally set a name but then wants to remove the name. By having the field configured like the above, that's not possible because an empty string is string's zero-value and with omitempty, it won't be sent.

So we'll need to do:

Name *string `json:"name,omitempty"`

That way a pointer to an empty string will be sent and successfully clear the name.

Name string `json:"name,omitempty"`

// Description is the human readable description of the service.
Description string `json:"description,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

"name": "updatedname",
"admin_state_up": false,
"subnet_id": null,
"tenant_id": "10039663455a446d8ba2cbb058b0f578",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is odd. So in this test fixture project_id is not here...

Ignoring the API docs, because they can sometimes have old or incorrect information, what are you seeing in your working environment? You can view the raw API request/response by calling neutron with neutron --debug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The API is returning 'project_id' in the response in my working environment when running
neutron --debug vpn-service-list. Should I include it as a field in the result then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please. We always prefer actual API output over API references.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I was under the impression that I should only include the fields reference in the source code here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, sometimes there's a lot of mystery and investigation to this.

You're absolutely right: if the service code is showing fields, that's confident confirmation that the fields are valid.

If the API references are showing fields that the aren't in the code, then there are two possibilities:

  1. The API reference is wrong (it happens frequently enough)
  2. The field is being added somewhere else in the service code. This happens a lot with Neutron, specifically with project_id and tenant_id. So the next source of confident information is the API interaction between you and your OpenStack environment.

For fields like project_id, created_at, or some other "meta" field, we can be fairly confident that they're somewhere in Neutron and as long as you're seeing them in your API interaction, that's good enough for me. But let's say your API interaction was also showing something like router_id. Then we'll really want to dig into the Neutron code (by either grepping the entire code for router_id or using the github search box) and figuring out where it's coming from. That's ugly work, but it makes sure Gophercloud is correct.

With regard to test fixtures, please always use fixtures generated yourself rather than fixtures from the API references. We (gophercloud) agreed to enforce this a while back but I occasionally forget to check. Using "real" fixtures helps us make sure the data is correct.

Let me know if all of that makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that does make a lot of sense. I'll keep that in mind from now on. Thanks for the explanation!

@jtopjian
Copy link
Contributor

jtopjian commented Feb 22, 2018

@simonre Also, this file is a good resource to determine which fields can be used in both Create (allow_post) and Update (allow_put).

edit: Duh. I forgot to link to the file: https://github.com/openstack/neutron-vpnaas/blob/master/neutron_vpnaas/extensions/vpnaas.py

@simonre simonre changed the title Vpnaas: Update Service [WIP] Vpnaas: Update Service Feb 26, 2018
@simonre
Copy link
Contributor Author

simonre commented Feb 26, 2018

@jtopjian I put the state of this back to [WIP] because I noticed a problem with writing acceptance tests for this. A VPN service can only be updated after its state has been set to ACTIVE. The state of a VPN Service is only set to ACTIVE after an IPSec Site connection for it has been created. This means that there are two alternatives for this pull request:

  1. Merge without an acceptance test
  2. Create files for all other resources for VPNaas first, then create an IPSec policy, an IKE policy and an IPSec site connection inside the acceptance test and test the update method after waiting for the ACTIVE state of the VPN service.
    Let me know if this didn't make sense or I overlooked another way of acceptance testing this.

@jtopjian
Copy link
Contributor

@simonre Good catch. Let's go with Option 2.

I would prefer to merge the VPN suite in order of functionality. If it's not possible to Update a connection entirely through Gophercloud, let's hold off until it is.

@simonre
Copy link
Contributor Author

simonre commented Mar 26, 2018

@jtopjian I've been working on implementing this but there are some obstacles.
The service state does not turn to active unless the IPSec site connection using the service is ACTIVEas well. This does only happen when there is a connection on the 'other side' sending data through the connection.

I can create the two sides of the connection without a problem entirely through gophercloud but unless I'm missing something there doesn't seem to be an easy way to send data through the connection turning its state to ACTIVE and thereby turning the VPN state to active.
Do you have any idea how this could be done?
If not, how do you suggest we proceed with this PR?
Let me know if my explanation doesn't make sense.

@jtopjian
Copy link
Contributor

@simonre Makes sense to me - thank you for the detailed explanation. This sounds like a pretty complex test fixture. I'm sure there's some way to do it, but I'm not immediately sure how.

I can create the two sides of the connection without a problem entirely through gophercloud

This is good enough for me. You've done some great work with the other acceptance tests and if this is the only test omitted in the entire VPNaaS suite, I think that is acceptable. :)

The pieces will be in place for either you or someone else to add this in the future.

@simonre
Copy link
Contributor Author

simonre commented Mar 27, 2018

@jtopjian

if this is the only test omitted

It would actually be one of two tests omitted in the suite. The other one would be for the update function for ipsec site connections. Updating a site connection has the exact same issue(Can't be updated while the state is PENDING_CREATE).

One other solution I could think of would be to somehow get the connection state to either DOWN or ERROR as according to the documentation those might work with the update function. Problem is again that I am not sure how to provoke those states intentionally.

For now I'll omit both tests if that's ok.

@jtopjian
Copy link
Contributor

For now I'll omit both tests if that's ok.

Yes, that sounds good to me.

@simonre simonre force-pushed the vpnaas-service-update branch 2 times, most recently from af21f84 to 23dc8ba Compare March 27, 2018 15:57
@simonre
Copy link
Contributor Author

simonre commented Mar 27, 2018

@jtopjian This is ready for review

@simonre simonre changed the title [WIP] Vpnaas: Update Service Vpnaas: Update Service Mar 27, 2018
@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 27, 2018

Build succeeded.

@jtopjian
Copy link
Contributor

@simonre Looks good to me!

cardoe pushed a commit to cardoe/gophercloud that referenced this pull request Aug 27, 2020
Add "openstack_networking_qos_dscp_marking_rule_v2" resource with
tests and website documentation.
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 this pull request may close these issues.

None yet

3 participants