fix unit tests

Author: ddelgrosso1

google/cloud/storage/bucket_ip_filter_test.cc

@@ -13,7 +13,6 @@
 // limitations under the License.
 
 #include "google/cloud/storage/bucket_ip_filter.h"
-#include "google/cloud/testing_util/is_proto_equal.h"
 #include <gmock/gmock.h>
 
 namespace google {

google/cloud/storage/internal/bucket_requests.cc

@@ -89,6 +89,16 @@ void DiffIamConfiguration(BucketMetadataPatchBuilder& builder,
   }
 }
 
+void DiffIpFilter(BucketMetadataPatchBuilder& builder, BucketMetadata const& o,
+                  BucketMetadata const& u) {
+  if (o.ip_filter_as_optional() == u.ip_filter_as_optional()) return;
+  if (u.has_ip_filter()) {
+    builder.SetIpFilter(u.ip_filter());
+  } else {
+    builder.ResetIpFilter();
+  }
+}
+
 void DiffLabels(BucketMetadataPatchBuilder& builder, BucketMetadata const& o,
                 BucketMetadata const& u) {
   if (o.labels() == u.labels()) return;
@@ -199,6 +209,7 @@ BucketMetadataPatchBuilder DiffBucketMetadata(BucketMetadata const& original,
   DiffDefaultObjectAcl(builder, original, updated);
   DiffEncryption(builder, original, updated);
   DiffIamConfiguration(builder, original, updated);
+  DiffIpFilter(builder, original, updated);
   DiffLabels(builder, original, updated);
   DiffLifecycle(builder, original, updated);
   DiffLogging(builder, original, updated);

google/cloud/storage/internal/bucket_requests_test.cc

@@ -464,11 +464,11 @@ TEST(PatchBucketRequestTest, DiffSetIpFilter) {
       BucketIpFilterPublicNetworkSource{{"1.2.3.4/32"}};
   ip_filter.vpc_network_sources =
       absl::make_optional<std::vector<BucketIpFilterVpcNetworkSource>>(
-          {BucketIpFilterVpcNetworkSource{"projects/p/global/networks/n",
-                                          {"5.6.7.8/32"}},
-           BucketIpFilterVpcNetworkSource{"projects/p/global/networks/m",
-                                          {"9.0.1.2/32"}}});
-  updated.set_ip_filter(ip_filter);
+          {BucketIpFilterVpcNetworkSource{
+               "projects/p/global/networks/n", {"5.6.7.8/32"}},
+           BucketIpFilterVpcNetworkSource{
+               "projects/p/global/networks/m", {"9.0.1.2/32"}}});
+  updated.set_ip_filter(std::move(ip_filter));
   PatchBucketRequest request("test-bucket", original, updated);
 
   auto patch = nlohmann::json::parse(request.payload());

google/cloud/storage/internal/grpc/bucket_request_parser.cc

@@ -209,6 +209,46 @@ Status PatchIamConfig(Bucket& b, nlohmann::json const& i) {
   return Status{};
 }
 
+Status PatchIpFilter(Bucket& b, nlohmann::json const& p) {
+  if (p.is_null()) {
+    b.clear_ip_filter();
+    return Status{};
+  }
+  auto& ip_filter = *b.mutable_ip_filter();
+  if (p.contains("mode")) {
+    ip_filter.set_mode(p.value("mode", ""));
+  }
+  if (p.contains("allowAllServiceAgentAccess")) {
+    ip_filter.set_allow_all_service_agent_access(
+        p.value("allowAllServiceAgentAccess", false));
+  }
+  if (p.contains("allowCrossOrgVpcs")) {
+    ip_filter.set_allow_cross_org_vpcs(p.value("allowCrossOrgVpcs", false));
+  }
+  if (p.contains("publicNetworkSource")) {
+    auto& pns = *ip_filter.mutable_public_network_source();
+    auto const& public_network_source = p["publicNetworkSource"];
+    if (public_network_source.contains("allowedIpCidrRanges")) {
+      for (auto const& v :
+           public_network_source["allowedIpCidrRanges"]) {
+        pns.add_allowed_ip_cidr_ranges(v.get<std::string>());
+      }
+    }
+  }
+  if (p.contains("vpcNetworkSources")) {
+    for (auto const& v : p["vpcNetworkSources"]) {
+      auto& entry = *ip_filter.add_vpc_network_sources();
+      entry.set_network(v.value("network", ""));
+      if (v.contains("allowedIpCidrRanges")) {
+        for (auto const& ip : v["allowedIpCidrRanges"]) {
+          entry.add_allowed_ip_cidr_ranges(ip.get<std::string>());
+        }
+      }
+    }
+  }
+  return Status{};
+}
+
 Status PatchAutoclass(Bucket& bucket, nlohmann::json const& p) {
   if (p.is_null()) {
     bucket.clear_autoclass();
@@ -558,6 +598,7 @@ StatusOr<google::storage::v2::UpdateBucketRequest> ToProto(
       {"billing", "", PatchBilling},
       {"retentionPolicy", "retention_policy", PatchRetentionPolicy},
       {"iamConfiguration", "iam_config", PatchIamConfig},
+      {"ipFilter", "ip_filter", PatchIpFilter},
       {"autoclass", "", PatchAutoclass},
   };
 

google/cloud/storage/internal/grpc/bucket_request_parser_test.cc

@@ -643,7 +643,7 @@ TEST(GrpcBucketRequestParser, PatchBucketRequestResetLabels) {
 TEST(GrpcBucketRequestParser, PatchBucketRequestResetIpFilter) {
   auto constexpr kTextProto = R"pb(
     bucket { name: "projects/_/buckets/bucket-name" }
-    update_mask {}
+    update_mask { paths: "ip_filter" }
   )pb";
   google::storage::v2::UpdateBucketRequest expected;
   ASSERT_TRUE(TextFormat::ParseFromString(kTextProto, &expected));
@@ -653,11 +653,6 @@ TEST(GrpcBucketRequestParser, PatchBucketRequestResetIpFilter) {
 
   auto actual = ToProto(req);
   ASSERT_STATUS_OK(actual);
-  // First check the paths. We do not care about their order, so checking them
-  // with IsProtoEqual does not work.
-  EXPECT_THAT(actual->update_mask().paths(), UnorderedElementsAre("ip_filter"));
-  // Clear the paths, which we already compared, and compare the proto.
-  actual->mutable_update_mask()->clear_paths();
   EXPECT_THAT(*actual, IsProtoEqual(expected));
 }
 
@@ -676,7 +671,7 @@ TEST(GrpcBucketRequestParser, PatchBucketRequestIpFilter) {
         }
       }
     }
-    update_mask {}
+    update_mask { paths: "ip_filter" }
   )pb";
   google::storage::v2::UpdateBucketRequest expected;
   ASSERT_TRUE(TextFormat::ParseFromString(kTextProto, &expected));
@@ -693,15 +688,10 @@ TEST(GrpcBucketRequestParser, PatchBucketRequestIpFilter) {
               "projects/p/global/networks/n", {"5.6.7.8/32"}}});
   storage::internal::PatchBucketRequest req(
       "bucket-name",
-      storage::BucketMetadataPatchBuilder().SetIpFilter(ip_filter));
+      storage::BucketMetadataPatchBuilder().SetIpFilter(std::move(ip_filter)));
 
   auto actual = ToProto(req);
   ASSERT_STATUS_OK(actual);
-  // First check the paths. We do not care about their order, so checking them
-  // with IsProtoEqual does not work.
-  EXPECT_THAT(actual->update_mask().paths(), UnorderedElementsAre("ip_filter"));
-  // Clear the paths, which we already compared, and compare the proto.
-  actual->mutable_update_mask()->clear_paths();
   EXPECT_THAT(*actual, IsProtoEqual(expected));
 }