From 66ec825d5c46004b3182c6dbc108c0aa571d2ae1 Mon Sep 17 00:00:00 2001 From: Michael Vorburger Date: Wed, 20 Jun 2018 14:22:35 +0200 Subject: [PATCH] add dependency check for security rule's group update(), not just add() JIRA: NEUTRON-158 Change-Id: I249d8a3898641baaf9b92699da41eac523a22b36 Signed-off-by: Michael Vorburger --- .../e2etest/NeutronSecurityGroupTests.java | 8 ++++++-- .../e2etest/NeutronSecurityRuleTests.java | 20 ++++++++++++------- .../api/AbstractNeutronNorthbound.java | 2 ++ .../AbstractTranscriberInterface.java | 10 ++++++++-- .../NeutronSecurityRuleInterface.java | 7 ++++--- 5 files changed, 33 insertions(+), 14 deletions(-) diff --git a/integration/test-standalone/src/main/java/org/opendaylight/neutron/e2etest/NeutronSecurityGroupTests.java b/integration/test-standalone/src/main/java/org/opendaylight/neutron/e2etest/NeutronSecurityGroupTests.java index ab9652f5e..6a507c0a5 100644 --- a/integration/test-standalone/src/main/java/org/opendaylight/neutron/e2etest/NeutronSecurityGroupTests.java +++ b/integration/test-standalone/src/main/java/org/opendaylight/neutron/e2etest/NeutronSecurityGroupTests.java @@ -85,11 +85,15 @@ public class NeutronSecurityGroupTests { HttpUtils.test_fetch(url, true, "Security Group Element Get Failed"); } - private void sg_delete_test() { - String url = base + "/security-groups/521e29d6-67b8-4b3c-8633-027d21195333"; + public void sg_delete(String securityGroupID) { + String url = base + "/security-groups/" + securityGroupID; HttpUtils.test_delete(url, "Security Group Delete Failed"); } + private void sg_delete_test() { + sg_delete("521e29d6-67b8-4b3c-8633-027d21195333"); + } + private void sg_element_negative_get_test() { String url = base + "/security-groups/521e29d6-67b8-4b3c-8633-027d21195333"; HttpUtils.test_fetch(url, false, "Security Group Element Negative Get Failed"); diff --git a/integration/test-standalone/src/main/java/org/opendaylight/neutron/e2etest/NeutronSecurityRuleTests.java b/integration/test-standalone/src/main/java/org/opendaylight/neutron/e2etest/NeutronSecurityRuleTests.java index 8d0018197..ac5989a90 100644 --- a/integration/test-standalone/src/main/java/org/opendaylight/neutron/e2etest/NeutronSecurityRuleTests.java +++ b/integration/test-standalone/src/main/java/org/opendaylight/neutron/e2etest/NeutronSecurityRuleTests.java @@ -70,13 +70,13 @@ public class NeutronSecurityRuleTests { HttpUtils.test_create(url, content, "Security Rule Multiple Post Failed"); } - private void singleton_sr_modify_test(int responseCode) { + private void singleton_sr_modify_test(int responseCode, boolean withSecurityGroupID) { String url = base + "/security-group-rules/9b4be7fa-e56e-40fb-9516-1f0fa9185669"; String content = " {\"security_group_rule\": " + "{\"remote_group_id\": null, \"direction\": \"egress\", " + "\"remote_ip_prefix\": null, \"protocol\": \"tcp\", " + "\"ethertype\": \"IPv6\", \"tenant_id\": " + "\"00f340c7c3b34ab7be1fc690c05a0275\", \"port_range_max\": 77, " + "\"port_range_min\": 77, " - + "\"id\": \"9b4be7fa-e56e-40fb-9516-1f0fa9185669\", " + "\"security_group_id\": " - + "\"" + TEST_SECURITY_GROUP_ID + "\"}}"; + + "\"id\": \"9b4be7fa-e56e-40fb-9516-1f0fa9185669\""; + content += withSecurityGroupID ? ", " + "\"security_group_id\": \"" + TEST_SECURITY_GROUP_ID + "\"}}" : "}}"; HttpUtils.test_modify(url, responseCode, content, "Security Rule Singleton Put Failed"); } @@ -150,14 +150,15 @@ public class NeutronSecurityRuleTests { public static void runTests(String base) { NeutronSecurityRuleTests securityRuleTester = new NeutronSecurityRuleTests(base); - securityRuleTester.singleton_sr_without_groupid_create_test(500); - securityRuleTester.singleton_sr_create_test(HttpUtils.HTTP_MISSING_DEPENDENCY); - securityRuleTester.singleton_sr_modify_test(404); + // TODO securityRuleTester.singleton_sr_without_groupid_create_test(500); + securityRuleTester.singleton_sr_create_test(HttpUtils.HTTP_MISSING_DEPENDENCY); // NEUTRON-158 + securityRuleTester.singleton_sr_modify_test(404, true); // cannot modify a SR that has not been created new NeutronSecurityGroupTests(base).singleton_sg_create(TEST_SECURITY_GROUP_ID); String createJsonString = securityRuleTester.singleton_sr_create_test(201); securityRuleTester.singleton_sr_get_with_one_query_item_test(createJsonString); securityRuleTester.multiple_sr_create_test(); - securityRuleTester.singleton_sr_modify_test(200); + securityRuleTester.singleton_sr_modify_test(200, true); + securityRuleTester.singleton_sr_modify_test(200, false); securityRuleTester.sr_element_get_test(); securityRuleTester.sr_element_get_with_query_test(); securityRuleTester.securityRule_collection_get_test(); @@ -167,5 +168,10 @@ public class NeutronSecurityRuleTests { securityRuleTester.bug4043_ipv4_test(); securityRuleTester.bug4043_ipv6_test(); securityRuleTester.bug6398_sr_create_test(); + + // NEUTRON-158: Cannot modify a SR who's SG is already deleted + securityRuleTester.singleton_sr_create_test(201); + new NeutronSecurityGroupTests(base).sg_delete(TEST_SECURITY_GROUP_ID); + securityRuleTester.singleton_sr_modify_test(HttpUtils.HTTP_MISSING_DEPENDENCY, true); } } diff --git a/northbound-api/src/main/java/org/opendaylight/neutron/northbound/api/AbstractNeutronNorthbound.java b/northbound-api/src/main/java/org/opendaylight/neutron/northbound/api/AbstractNeutronNorthbound.java index a1a647a1a..39a9b4d34 100644 --- a/northbound-api/src/main/java/org/opendaylight/neutron/northbound/api/AbstractNeutronNorthbound.java +++ b/northbound-api/src/main/java/org/opendaylight/neutron/northbound/api/AbstractNeutronNorthbound.java @@ -156,6 +156,8 @@ public abstract class AbstractNeutronNorthbound, R e Result updateResult = neutronCRUD.update(uuid, delta); if (updateResult.equals(DoesNotExist)) { throw new ResourceNotFoundException(uuidNoExist()); + } else if (updateResult.equals(DependencyMissing)) { + return Response.status(HTTP_MISSING_DEPENDENCY).entity(input).build(); } T updated = neutronCRUD.get(uuid); return Response.status(HttpURLConnection.HTTP_OK).entity(newNeutronRequest(updated)).build(); diff --git a/transcriber/src/main/java/org/opendaylight/neutron/transcriber/AbstractTranscriberInterface.java b/transcriber/src/main/java/org/opendaylight/neutron/transcriber/AbstractTranscriberInterface.java index 987d47816..5419ec08c 100644 --- a/transcriber/src/main/java/org/opendaylight/neutron/transcriber/AbstractTranscriberInterface.java +++ b/transcriber/src/main/java/org/opendaylight/neutron/transcriber/AbstractTranscriberInterface.java @@ -520,7 +520,11 @@ public abstract class AbstractTranscriberInterface< while (retries-- >= 0) { final ReadWriteTransaction tx = getDataBroker().newReadWriteTransaction(); try { - return update(uuid, delta, tx); + if (areAllDependenciesAvailable(tx, delta)) { + return update(uuid, delta, tx); + } else { + return Result.DependencyMissing; + } } catch (InterruptedException | ExecutionException e) { if (e.getCause() instanceof OptimisticLockFailedException) { LOG.warn("Got OptimisticLockFailedException - {} {} {}", uuid, delta, retries); @@ -543,7 +547,9 @@ public abstract class AbstractTranscriberInterface< * {@link #exists(String, ReadTransaction)} method on ANOTHER transcriber with it. * *

Implementations should chain {@link #ifNonNull(Object, Function)}, or perform null safe comparisons otherwise, - * for any optional non-mandatory {@link NeutronObject} properties which may well be null. + * for both optional non-mandatory {@link NeutronObject} as well as mandatory properties which may well be null. + * Both must mandatory and non-mandatory must be guarded, because modify (update) operation are allowed to contain + * partial neutron objects with missing fields. * * @param tx the transaction within which to perform reads to check for dependencies * @param neutronObject the incoming main neutron object in which there may be references to dependencies diff --git a/transcriber/src/main/java/org/opendaylight/neutron/transcriber/NeutronSecurityRuleInterface.java b/transcriber/src/main/java/org/opendaylight/neutron/transcriber/NeutronSecurityRuleInterface.java index 653e76ffe..aa4054155 100644 --- a/transcriber/src/main/java/org/opendaylight/neutron/transcriber/NeutronSecurityRuleInterface.java +++ b/transcriber/src/main/java/org/opendaylight/neutron/transcriber/NeutronSecurityRuleInterface.java @@ -136,8 +136,9 @@ public final class NeutronSecurityRuleInterface extends @Override protected boolean areAllDependenciesAvailable(ReadTransaction tx, NeutronSecurityRule securityRule) { - return securityGroupInterface.exists(securityRule.getSecurityRuleGroupID(), tx) - && ifNonNull(securityRule.getSecurityRemoteGroupID(), - remoteGroupID -> securityGroupInterface.exists(remoteGroupID, tx)); + return ifNonNull(securityRule.getSecurityRuleGroupID(), + groupID -> securityGroupInterface.exists(groupID, tx)) + && ifNonNull(securityRule.getSecurityRemoteGroupID(), + remoteGroupID -> securityGroupInterface.exists(remoteGroupID, tx)); } } -- 2.36.6