(delta/sds): Mark secret as ready if server says the resource doesn't…

… exist (envoyproxy#33362) As discussed in Slack, a delta removal should complete cluster warming Risk Level: Low Testing: integration Signed-off-by: Keith Mattix II <[email protected]>
alyssawilk · Apr 29, 2024 · 9bb57fc · 9bb57fc
1 parent 3d10275
commit 9bb57fc
Show file tree

Hide file tree

Showing 2 changed files with 27 additions and 21 deletions.
diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc
@@ -160,6 +160,10 @@ SdsApi::onConfigUpdate(const std::vector<Config::DecodedResourceRef>& added_reso
         trace,
         "Server sent a delta SDS update attempting to remove a resource (name: {}). Ignoring.",
         removed_resources[0]);
+
+    // Even if we ignore this resource, the owning resource (LDS/CDS) should still complete
+    // warming.
+    init_target_.ready();
     return absl::OkStatus();
   }
   return onConfigUpdate(added_resources, added_resources[0].get().version());

diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc
@@ -261,34 +261,40 @@ TEST_P(AdsIntegrationTest, DeltaSdsRemovals) {
             name: validation_context
             sds_config:
               resource_api_version: V3
+              initial_fetch_timeout: 5s
               ads: {}
   )EOF",
                             sds_transport_socket);
   auto cluster = ConfigHelper::buildStaticCluster("cluster", 8000, "127.0.0.1");
   *cluster.mutable_transport_socket() = sds_transport_socket;
   cluster.set_name("cluster_0");
 
-  // Initial * Delta ADS subscription
+  // Initial * Delta ADS subscription.
   EXPECT_TRUE(compareDeltaDiscoveryRequest(cds_type_url, {}, {}));
   sendDeltaDiscoveryResponse<envoy::config::cluster::v3::Cluster>(cds_type_url, {cluster}, {}, "1");
 
-  // The cluster needs this secret, so it's going to request it
+  // The cluster needs this secret, so it's going to request it.
   EXPECT_TRUE(compareDeltaDiscoveryRequest(sds_type_url, {"validation_context"}, {}, {}));
 
-  // Ack the original CDS sub
+  // Cluster should start off warming as the secret is being requested.
+  test_server_->waitForGaugeEq("cluster.cluster_0.warming_state", 1);
+
+  // Ack the original CDS sub.
   EXPECT_TRUE(compareDeltaDiscoveryRequest(cds_type_url, {}, {}));
 
-  // Before we send the secret, we'll send a delta removal to make sure we don't get a NACK
+  // Before we send the secret, we'll send a delta removal to make sure we don't get a NACK.
   sendDeltaDiscoveryResponse<envoy::extensions::transport_sockets::tls::v3::Secret>(
-      sds_type_url, {}, {"validation_context"}, "2");
+      sds_type_url, {}, {"validation_context"}, "1");
 
-  // Ack the removal
-  EXPECT_TRUE(compareDeltaDiscoveryRequest(sds_type_url, {}, {}));
+  // The cluster shouldn't be warming anymore since the server signaled
+  // that the requested resource doesn't exist.
+  test_server_->waitForGaugeEq("cluster.cluster_0.warming_state", 0);
 
-  // Cluster should still be warming
-  test_server_->waitForGaugeGe("cluster_manager.warming_clusters", 1);
-  test_server_->waitForGaugeEq("cluster.cluster_0.warming_state", 1);
-  test_server_->waitForCounterEq("cluster_manager.cluster_removed", 0);
+  // Ack the original LDS subscription.
+  EXPECT_TRUE(compareDeltaDiscoveryRequest(lds_type_url, {}, {}));
+
+  // Ack the removal itself.
+  EXPECT_TRUE(compareDeltaDiscoveryRequest(sds_type_url, {}, {}));
 
   envoy::extensions::transport_sockets::tls::v3::Secret validation_context;
   TestUtility::loadFromYaml(fmt::format(R"EOF(
@@ -301,27 +307,23 @@ TEST_P(AdsIntegrationTest, DeltaSdsRemovals) {
                                             "test/config/integration/certs/upstreamcacert.pem")),
                             validation_context);
 
-  // Now actually send the secret
+  // Now actually send the secret.
   sendDeltaDiscoveryResponse<envoy::extensions::transport_sockets::tls::v3::Secret>(
       sds_type_url, {validation_context}, {}, "2");
 
-  // we're no longer warming
-  test_server_->waitForGaugeEq("cluster_manager.warming_clusters", 0);
-  // Ack the original LDS (now that the cluster is warmed?)
-  EXPECT_TRUE(compareDeltaDiscoveryRequest(lds_type_url, {}, {}));
-  // Ack the secret we just sent
+  // Ack the secret we just sent.
   EXPECT_TRUE(compareDeltaDiscoveryRequest(sds_type_url, {}, {}));
 
-  // Remove the cluster that owns the secret
+  // Remove the cluster that owns the secret.
   sendDeltaDiscoveryResponse<envoy::config::cluster::v3::Cluster>(cds_type_url, {}, {"cluster_0"},
                                                                   "1");
-  // Follow that up with a secret removal
+  // Follow that up with a secret removal.
   sendDeltaDiscoveryResponse<envoy::extensions::transport_sockets::tls::v3::Secret>(
       sds_type_url, {}, {"validation_context"}, "3");
   test_server_->waitForCounterEq("cluster_manager.cluster_removed", 1);
-  // Ack the CDS removal
+  // Ack the CDS removal.
   EXPECT_TRUE(compareDeltaDiscoveryRequest(cds_type_url, {}, {}));
-  // Should be an ACK, not a NACK since the SDS removal is ignored
+  // Should be an ACK, not a NACK since the SDS removal is ignored.
   EXPECT_TRUE(compareDeltaDiscoveryRequest(sds_type_url, {}, {}));
 }