Vpnaas: Create IPSec Policy #768

simonre · 2018-02-16T18:13:57Z

Links to the line numbers/files in the OpenStack source code that support the
code in this PR:

https://github.com/openstack/neutron-vpnaas/blob/058469e1b99b647537a5228c6a384d93df5484df/neutron_vpnaas/db/vpn/vpn_db.py#L396

https://github.com/openstack/neutron-vpnaas/blob/058469e1b99b647537a5228c6a384d93df5484df/neutron_vpnaas/db/vpn/vpn_db.py#L377

API:
https://developer.openstack.org/api-ref/network/v2/#create-ipsec-policy

coveralls · 2018-02-16T18:20:50Z

Coverage increased (+0.003%) to 73.444% when pulling b895234 on simonre:vpnaas-ipsecpolicy-create into d693a2e on gophercloud:master.

theopenlab-ci · 2018-02-16T18:53:29Z

Build succeeded.

gophercloud-unittest : SUCCESS in 2m 07s
gophercloud-acceptance-test : SUCCESS in 36m 16s

theopenlab-ci · 2018-02-16T22:35:46Z

Build succeeded.

gophercloud-unittest : SUCCESS in 2m 16s
gophercloud-acceptance-test : SUCCESS in 45m 41s

simonre · 2018-02-16T22:38:53Z

@jtopjian this is ready for review

jtopjian

@simonre This is looking good, too. :)

I've left some comments inline about some changes to fields. Let me know if you have questions.

Another thing I noticed is that VPNaaS is defining a number of enum db columns here:

https://github.com/openstack/neutron-vpnaas/blob/master/neutron_vpnaas/db/vpn/vpn_models.py#L38-L62

Because there are only a finite number of choices for these columns, we can make some dedicated types to pass into CreateOpts. For example:

type AuthAlgorithm string
const (
  AuthSHA1 AuthAlgorithem = "sha1"
  ...
)

And then for CreateOpts, you would change the field to:

AuthAlgorithm AuthAlgorithm `json:"auth_algorithm,omitempty"`

We should be able to have dedicated types for TransformProtocol, AuthAlgorithm, EncryptionAlgorithm, EncapsulationMode, LifetimeUnits, and PFS.

However, the values in the result struct should be kept as a string. These types are only for CreateOpts.

See the security group rules requests.go file as an example.

Let me know if you have any questions or need any help.

jtopjian · 2018-02-16T23:59:40Z

openstack/networking/v2/extensions/vpnaas/ipsecpolicies/results.go

+	TransformProtocol string `json:"transform_protocol"`
+
+	// Lifetime is the lifetime of the security association
+	Lifetime *Lifetime `json:"lifetime"`


This should be:

Lifetime Lifetime `json:"lifetime"`

jtopjian · 2018-02-16T23:59:51Z

openstack/networking/v2/extensions/vpnaas/ipsecpolicies/results.go

+type Lifetime struct {
+	// LifetimeUnits is the unit for the lifetime
+	// Default is seconds
+	LifetimeUnits string `json:"units"`


Let's rename this to Units.

jtopjian · 2018-02-17T00:00:00Z

openstack/networking/v2/extensions/vpnaas/ipsecpolicies/results.go

+
+	// LifetimeValue is the lifetime
+	// Default is 3600
+	LifetimeValue int `json:"value"`


Let's rename this to Value.

jtopjian · 2018-02-17T00:00:39Z

openstack/networking/v2/extensions/vpnaas/ipsecpolicies/testing/requests_test.go

+	th.AssertEquals(t, "group5", actual.PFS)
+	th.AssertEquals(t, "", actual.Description)
+	th.AssertEquals(t, "seconds", actual.Lifetime.LifetimeUnits)
+	th.AssertEquals(t, 7200, actual.Lifetime.LifetimeValue)


Similar to our other discussion, we'll want to compare full structs. I've tested this out locally and it works, so let me know if you run into problems.

jtopjian · 2018-02-17T00:12:48Z

openstack/networking/v2/extensions/vpnaas/ipsecpolicies/requests.go

+type LifetimeCreateOpts struct {
+	// LifetimeUnits is the units for the lifetime of the security association
+	// Default unit is seconds
+	LifetimeUnits string `json:"units,omitempty"`


Let's change this to Units.

jtopjian · 2018-02-17T00:12:59Z

openstack/networking/v2/extensions/vpnaas/ipsecpolicies/requests.go

+	// The lifetime value.
+	// Must be a positive integer.
+	// Default value is 3600.
+	LifetimeValue int `json:"value,omitempty"`


Let's change this to Value.

theopenlab-ci · 2018-02-17T20:37:08Z

Build succeeded.

gophercloud-unittest : SUCCESS in 2m 17s
gophercloud-acceptance-test : SUCCESS in 28m 51s

jtopjian

@simonre This looks really good. I spotted a typo + this looks like it'll need to be rebased with master. After that, this is good to go.

jtopjian · 2018-02-18T02:54:44Z

openstack/networking/v2/extensions/vpnaas/ipsecpolicies/requests.go

+	TransformProtocolAHESP     TransformProtocol   = "ah-esp"
+	AuthAlgorithmSHA1          AuthAlgorithm       = "sha1"
+	AuthAlgorithmSHA256        AuthAlgorithm       = "sha256"
+	AuthAlgorithmHA384         AuthAlgorithm       = "sha384"


AuthAlgorithmSHA384

theopenlab-ci · 2018-02-19T11:09:25Z

Build succeeded.

gophercloud-unittest : SUCCESS in 4m 16s
gophercloud-acceptance-test : SUCCESS in 28m 43s

simonre · 2018-02-19T12:53:53Z

@jtopjian While writing the List function I noticed that this was missing an 'id' field in the resulting struct so I added it as well.

theopenlab-ci · 2018-02-19T13:26:53Z

Build succeeded.

gophercloud-unittest : SUCCESS in 2m 21s
gophercloud-acceptance-test : SUCCESS in 28m 48s

jtopjian · 2018-02-20T00:50:20Z

@simonre Nice catch - thanks.

It looks like this might need another rebase. The Vpnaas: Create VPN service (#760) commit should be omitted because if I squash these commits, including that one commit might make the git history look as though this commit added the Services resource.

simonre · 2018-02-20T11:45:58Z

@jtopjian Is this better?

theopenlab-ci · 2018-02-20T12:18:24Z

Build succeeded.

gophercloud-unittest : SUCCESS in 2m 29s
gophercloud-acceptance-test : SUCCESS in 29m 09s

jtopjian · 2018-02-21T03:20:02Z

@simonre This looks good, but unfortunately the merge of the VPN service delete PR caused a conflict with the acceptance/openstack/networking/v2/extensions/vpnaas/vpnaas.go file. Another rebase will resolve this.

Note that when you do the rebase, git will now complain that there's a merge issue. This is expected. You'll want to edit the vpnaas.go file and resolve the conflict, then continue with the rebase (git rebase --continue).

Let me know if you need help.

Once the conflict is resolved, this is good to go :)

…o LifetimeValue

…heir own structs

…est to compare struct instead of fields

…TransformProtocol

theopenlab-ci · 2018-02-21T14:22:37Z

Build succeeded.

gophercloud-unittest : SUCCESS in 2m 48s
gophercloud-acceptance-test : SUCCESS in 32m 40s

jtopjian · 2018-02-22T02:46:20Z

@simonre This looks good to me. Very nice work on this - especially with the types.

Just a heads up that the TestPolicyCRUD acceptance test should probably be renamed to TestIPSecPolicyCRUD or else it'll clash with the upcoming TestPolicyCRUD in the IKE PRs.

* Enable import instance Since openstack does not keep track of NIC ordering, the user must specify network in the order of the imported state. * Add a test case and documentation.

simonre changed the title ~~[WIP] Vpnaas: Create IPSec Policy~~ Vpnaas: Create IPSec Policy Feb 16, 2018

jtopjian requested changes Feb 17, 2018

View reviewed changes

jtopjian requested changes Feb 18, 2018

View reviewed changes

simonre force-pushed the vpnaas-ipsecpolicy-create branch from 6cedfe2 to 40efda3 Compare February 20, 2018 11:43

sreinkemeier added 14 commits February 21, 2018 14:43

Added requests.go file and necessary methods for ipsecpolicy creation

1159928

created results file

7edf2c8

Updated results description

c1a3835

Created urls.go file

a111c16

Put correct root and resource urls

2332518

Added unit test for ipsecpolicy creation. Also renamed LifetimeName t…

1385888

…o LifetimeValue

Formatted inline comments, put LifetimeCreateOpts and Lifetime into t…

6dcb6be

…heir own structs

Added acceptance test, formatted struct fields

f584745

Added doc.go file

c5037e7

Added comparison between expected and actual struct to tests

66c95ee

Removed Lifetime prefix before Units and Value fields. Updated unit t…

2966236

…est to compare struct instead of fields

added types to pass into createopts

329af7d

Added missing 'id' field to results struct, fixed typo in comment to …

31d451f

…TransformProtocol

fixed typo

b895234

simonre force-pushed the vpnaas-ipsecpolicy-create branch from c08e292 to b895234 Compare February 21, 2018 13:49

jtopjian approved these changes Feb 22, 2018

View reviewed changes

jtopjian merged commit 6c823a6 into gophercloud:master Feb 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vpnaas: Create IPSec Policy #768

Vpnaas: Create IPSec Policy #768

simonre commented Feb 16, 2018

coveralls commented Feb 16, 2018 •

edited

theopenlab-ci bot commented Feb 16, 2018

theopenlab-ci bot commented Feb 16, 2018

simonre commented Feb 16, 2018

jtopjian left a comment •

edited

jtopjian Feb 16, 2018

jtopjian Feb 16, 2018

jtopjian Feb 17, 2018

jtopjian Feb 17, 2018

jtopjian Feb 17, 2018

jtopjian Feb 17, 2018

theopenlab-ci bot commented Feb 17, 2018

jtopjian left a comment

jtopjian Feb 18, 2018

theopenlab-ci bot commented Feb 19, 2018

simonre commented Feb 19, 2018

theopenlab-ci bot commented Feb 19, 2018

jtopjian commented Feb 20, 2018

simonre commented Feb 20, 2018

theopenlab-ci bot commented Feb 20, 2018

jtopjian commented Feb 21, 2018

theopenlab-ci bot commented Feb 21, 2018

jtopjian commented Feb 22, 2018

Vpnaas: Create IPSec Policy #768

Vpnaas: Create IPSec Policy #768

Conversation

simonre commented Feb 16, 2018

coveralls commented Feb 16, 2018 • edited

theopenlab-ci bot commented Feb 16, 2018

theopenlab-ci bot commented Feb 16, 2018

simonre commented Feb 16, 2018

jtopjian left a comment • edited

Choose a reason for hiding this comment

jtopjian Feb 16, 2018

Choose a reason for hiding this comment

jtopjian Feb 16, 2018

Choose a reason for hiding this comment

jtopjian Feb 17, 2018

Choose a reason for hiding this comment

jtopjian Feb 17, 2018

Choose a reason for hiding this comment

jtopjian Feb 17, 2018

Choose a reason for hiding this comment

jtopjian Feb 17, 2018

Choose a reason for hiding this comment

theopenlab-ci bot commented Feb 17, 2018

jtopjian left a comment

Choose a reason for hiding this comment

jtopjian Feb 18, 2018

Choose a reason for hiding this comment

theopenlab-ci bot commented Feb 19, 2018

simonre commented Feb 19, 2018

theopenlab-ci bot commented Feb 19, 2018

jtopjian commented Feb 20, 2018

simonre commented Feb 20, 2018

theopenlab-ci bot commented Feb 20, 2018

jtopjian commented Feb 21, 2018

theopenlab-ci bot commented Feb 21, 2018

jtopjian commented Feb 22, 2018

coveralls commented Feb 16, 2018 •

edited

jtopjian left a comment •

edited