Delete Route CRD upon cf delete-route

Sannidhi Jalukar · sethboyles · sethboyles · commit 1a2f8b315063 · 2020-05-11T16:40:00.000-07:00
[#171828842]

Authored-by: Sannidhi Jalukar &lt;sjalukar@pivotal.io&gt;
Co-authored-by: Seth Boyles &lt;sboyles@pivotal.io&gt;
Co-authored-by: Sannidhi Jalukar &lt;sjalukar@pivotal.io&gt;
diff --git a/app/actions/app_delete.rb b/app/actions/app_delete.rb
@@ -52,7 +52,7 @@ def delete(apps, record_event: true)
         end
         logger.info("Deleted app: #{app.guid}")
 
-        if kubernetes_api_configured?
+        if VCAP::CloudController::Config.kubernetes_api_configured?
           logger.info('Deleting associated kpack image')
           kpack_client.delete_image(
             app.guid,
@@ -72,10 +72,6 @@ def kpack_client
       CloudController::DependencyLocator.instance.kpack_client
     end
 
-    def kubernetes_api_configured?
-      !!VCAP::CloudController::Config.config.get(:kubernetes, :host_url)
-    end
-
     def record_audit_event(app)
       Repositories::AppEventRepository.new.record_app_delete_request(
         app,
diff --git a/app/actions/route_create.rb b/app/actions/route_create.rb
@@ -28,7 +28,7 @@ def create(message:, space:, domain:)
         manifest_triggered: false,
       )
 
-      if kubernetes_api_configured?
+      if VCAP::CloudController::Config.kubernetes_api_configured?
         route_crd_client.create_route(route)
       end
 
@@ -43,10 +43,6 @@ def route_crd_client
       @route_crd_client ||= CloudController::DependencyLocator.instance.route_crd_client
     end
 
-    def kubernetes_api_configured?
-      !!VCAP::CloudController::Config.config.get(:kubernetes, :host_url)
-    end
-
     def validation_error!(error, host, path, space, domain)
       if error.errors.on(:domain)&.include?(:invalid_relation)
         error!("Invalid domain. Domain '#{domain.name}' is not available in organization '#{space.organization.name}'.")
diff --git a/app/actions/update_route_destinations.rb b/app/actions/update_route_destinations.rb
@@ -97,7 +97,7 @@ def update(route, to_add, to_delete, user_audit_info, manifest_triggered)
 
         route.reload
 
-        if kubernetes_api_configured?
+        if VCAP::CloudController::Config.kubernetes_api_configured?
           client = route_crd_client
           client.update_destinations(route)
         end
@@ -171,10 +171,6 @@ def route_crd_client
         CloudController::DependencyLocator.instance.route_crd_client
       end
 
-      def kubernetes_api_configured?
-        !!VCAP::CloudController::Config.config.get(:kubernetes, :host_url)
-      end
-
       def validate_unique!(new_route_mappings)
         raise DuplicateDestinationError.new('Destinations cannot contain duplicate entries') if new_route_mappings.any? { |rm| new_route_mappings.count(rm) > 1 }
       end
diff --git a/app/actions/v2/route_create.rb b/app/actions/v2/route_create.rb
@@ -14,7 +14,7 @@ def create_route(route_hash:)
           r
         end
 
-        if kubernetes_api_configured?
+        if VCAP::CloudController::Config.kubernetes_api_configured?
           route_crd_client.create_route(route)
         end
 
@@ -28,10 +28,6 @@ def create_route(route_hash:)
       def route_crd_client
         @route_crd_client ||= CloudController::DependencyLocator.instance.route_crd_client
       end
-
-      def kubernetes_api_configured?
-        !!VCAP::CloudController::Config.config.get(:kubernetes, :host_url)
-      end
     end
   end
 end
diff --git a/app/actions/v2/route_mapping_create.rb b/app/actions/v2/route_mapping_create.rb
@@ -48,7 +48,7 @@ def add
           )
         end
 
-        if kubernetes_api_configured?
+        if VCAP::CloudController::Config.kubernetes_api_configured?
           route_crd_client.update_destinations(route_mapping.route)
         end
 
@@ -129,10 +129,6 @@ def validate_routing_api_enabled!
           raise RoutingApiDisabledError.new('Routing API is disabled')
         end
       end
-
-      def kubernetes_api_configured?
-        !!VCAP::CloudController::Config.config.get(:kubernetes, :host_url)
-      end
     end
   end
 end
diff --git a/app/actions/v3/route_create.rb b/app/actions/v3/route_create.rb
@@ -4,7 +4,7 @@ class RouteCreate
       class << self
         def create_route(route_hash:, logger:, user_audit_info:, manifest_triggered: false)
           route = Route.create_from_hash(route_hash)
-          if kubernetes_api_configured?
+          if VCAP::CloudController::Config.kubernetes_api_configured?
             client = route_crd_client
             client.create_route(route)
           end
@@ -17,10 +17,6 @@ def create_route(route_hash:, logger:, user_audit_info:, manifest_triggered: fal
         def route_crd_client
           CloudController::DependencyLocator.instance.route_crd_client
         end
-
-        def kubernetes_api_configured?
-          !!VCAP::CloudController::Config.config.get(:kubernetes, :host_url)
-        end
       end
     end
   end
diff --git a/app/models/runtime/route.rb b/app/models/runtime/route.rb
@@ -247,6 +247,9 @@ def around_destroy
       loaded_apps.each do |app|
         ProcessRouteHandler.new(app).notify_backend_of_route_update
       end
+      if VCAP::CloudController::Config.kubernetes_api_configured?
+        CloudController::DependencyLocator.instance.route_crd_client.delete_route(self)
+      end
     end
 
     def validate_host_and_domain_in_different_space
diff --git a/lib/cloud_controller/config.rb b/lib/cloud_controller/config.rb
@@ -34,6 +34,10 @@ def schema_class_for_context(context)
         const_get("VCAP::CloudController::ConfigSchemas::#{context.to_s.camelize}Schema")
       end
 
+      def kubernetes_api_configured?
+        !!config.get(:kubernetes, :host_url)
+      end
+
       private
 
       def merge_defaults(config)
diff --git a/lib/kubernetes/route_crd_client.rb b/lib/kubernetes/route_crd_client.rb
@@ -55,6 +55,17 @@ def update_destinations(route)
       raise error
     end
 
+    def delete_route(route)
+      @client.delete_route(route.guid, VCAP::CloudController::Config.config.get(:kubernetes, :workloads_namespace))
+    rescue Kubeclient::ResourceNotFoundError
+      nil
+    rescue Kubeclient::HttpError => e
+      error = CloudController::Errors::ApiError.new_from_details('KubernetesRouteResourceError', route.guid)
+      Steno.logger('cc.action.route_create').info("Failed to Delete Route CRD: #{e}")
+      error.set_backtrace(e.backtrace)
+      raise error
+    end
+
     private
 
     def get_destinations(route)
diff --git a/spec/request/organizations_spec.rb b/spec/request/organizations_spec.rb
@@ -16,6 +16,7 @@ module VCAP::CloudController
       organization2.add_user(user)
       organization3.add_user(user)
       Domain.dataset.destroy # this will clean up the seeded test domains
+      TestConfig.override(kubernetes: {})
     end
 
     describe 'POST /v3/organizations' do
diff --git a/spec/request/spaces_spec.rb b/spec/request/spaces_spec.rb
@@ -15,6 +15,7 @@
     space1.add_developer(user)
     space2.add_developer(user)
     space3.add_developer(user)
+    TestConfig.override(kubernetes: {})
   end
 
   describe 'POST /v3/spaces' do
diff --git a/spec/request/v2/organizations_spec.rb b/spec/request/v2/organizations_spec.rb
@@ -4,6 +4,10 @@
   let(:user) { VCAP::CloudController::User.make }
   let(:org) { VCAP::CloudController::Organization.make }
 
+  before do
+    TestConfig.override(kubernetes: {})
+  end
+
   describe 'GET /v2/organizations/:guid/services' do
     let!(:space) { VCAP::CloudController::Space.make(organization: org) }
     let!(:service_1) { VCAP::CloudController::Service.make }
diff --git a/spec/unit/actions/domain_delete_spec.rb b/spec/unit/actions/domain_delete_spec.rb
@@ -10,6 +10,10 @@ module VCAP::CloudController
     let(:org) { VCAP::CloudController::Organization.make }
     let(:space) { VCAP::CloudController::Space.make(organization: org) }
 
+    before do
+      TestConfig.override(kubernetes: {})
+    end
+
     describe '#delete' do
       let!(:domain) { Domain.make(owning_organization: org) }
 
diff --git a/spec/unit/actions/route_delete_spec.rb b/spec/unit/actions/route_delete_spec.rb
@@ -6,6 +6,7 @@ module VCAP::CloudController
     let(:user_audit_info) { instance_double(UserAuditInfo) }
     let(:route_event_repo) { instance_double(Repositories::RouteEventRepository) }
     let(:space) { Space.make }
+    let(:route_crd_client) { instance_double(Kubernetes::RouteCrdClient) }
 
     subject(:route_delete) { RouteDeleteAction.new(user_audit_info) }
 
@@ -15,6 +16,8 @@ module VCAP::CloudController
       before do
         allow(Repositories::RouteEventRepository).to receive(:new).and_return(route_event_repo)
         allow(route_event_repo).to receive(:record_route_delete_request)
+        allow(CloudController::DependencyLocator.instance).to receive(:route_crd_client).and_return(route_crd_client)
+        allow(route_crd_client).to receive(:delete_route)
       end
 
       it 'deletes the route record' do
@@ -90,6 +93,28 @@ module VCAP::CloudController
           expect(route.exists?).to be_falsey
         end
       end
+
+      context 'when targeting a Kubernetes API' do
+        it 'deletes the route resource in Kubernetes' do
+          expect {
+            route_delete.delete([route])
+            expect(route_crd_client).to have_received(:delete_route)
+          }.to change { Route.count }.by(-1)
+        end
+      end
+
+      context 'when not targeting a Kubernetes API' do
+        before do
+          TestConfig.override(
+            kubernetes: {}
+          )
+        end
+
+        it 'does not delete the route resource in Kubernetes' do
+          route_delete.delete([route])
+          expect(route_crd_client).not_to have_received(:delete_route)
+        end
+      end
     end
   end
 end
diff --git a/spec/unit/actions/routing/route_delete_spec.rb b/spec/unit/actions/routing/route_delete_spec.rb
@@ -15,6 +15,12 @@ module VCAP::CloudController
     let(:user_audit_info) { UserAuditInfo.new(user_guid: 'user-guid', user_email: 'user-email') }
     let(:recursive) { false }
     let!(:route) { Route.make }
+    let(:route_crd_client) { instance_double(Kubernetes::RouteCrdClient) }
+
+    before do
+      allow(CloudController::DependencyLocator.instance).to receive(:route_crd_client).and_return(route_crd_client)
+      allow(route_crd_client).to receive(:delete_route)
+    end
 
     describe 'delete_unmapped_route' do
       it 'deletes the route' do
@@ -125,6 +131,31 @@ module VCAP::CloudController
           end
         end
       end
+
+      context 'when targeting a Kubernetes API' do
+        before do
+          allow(CloudController::DependencyLocator.instance).to receive(:route_crd_client).and_return(route_crd_client)
+          allow(route_crd_client).to receive(:delete_route)
+        end
+        it 'deletes the route resource in Kubernetes' do
+          expect {
+            route_delete_action.delete_sync(route: route, recursive: false)
+            expect(route_crd_client).to have_received(:delete_route)
+          }.to change { Route.count }.by(-1)
+        end
+      end
+
+      context 'when not targeting a Kubernetes API' do
+        before do
+          TestConfig.override(
+            kubernetes: {}
+          )
+        end
+        it 'does not delete the route resource in Kubernetes' do
+          route_delete_action.delete_sync(route: route, recursive: false)
+          expect(route_crd_client).not_to have_received(:delete_route)
+        end
+      end
     end
 
     describe 'delete_async' do
diff --git a/spec/unit/controllers/runtime/organizations_controller_spec.rb b/spec/unit/controllers/runtime/organizations_controller_spec.rb
@@ -9,6 +9,7 @@ module VCAP::CloudController
 
     before do
       allow(UaaClient).to receive(:new).and_return(uaa_client)
+      TestConfig.override(kubernetes: {})
     end
 
     describe 'Query Parameters' do
diff --git a/spec/unit/controllers/runtime/spaces_controller_spec.rb b/spec/unit/controllers/runtime/spaces_controller_spec.rb
@@ -881,7 +881,10 @@ def decoded_guids
       context 'when recursive is false' do
         let(:space) { Space.make }
 
-        before { set_current_user_as_admin }
+        before do
+          set_current_user_as_admin
+          TestConfig.override(kubernetes: {})
+        end
 
         it 'successfully deletes spaces with no associations' do
           delete "/v2/spaces/#{space.guid}"
@@ -954,6 +957,7 @@ def decoded_guids
           allow(CloudController::DependencyLocator.instance).to receive(:kpack_client).and_return(kpack_client)
           stub_deprovision(service_instance, accepts_incomplete: true)
           set_current_user(user, admin: true)
+          TestConfig.override(kubernetes: {})
         end
 
         it 'successfully deletes spaces with v3 app associations' do
diff --git a/spec/unit/lib/kubernetes/route_crd_client_spec.rb b/spec/unit/lib/kubernetes/route_crd_client_spec.rb
@@ -2,8 +2,10 @@
 require 'kubernetes/kpack_client'
 
 RSpec.describe Kubernetes::RouteCrdClient do
+  let(:kube_client) { double(Kubeclient) }
+  subject(:route_crd_client) { Kubernetes::RouteCrdClient.new(kube_client) }
+
   describe '#create_route' do
-    let(:kube_client) { double(Kubeclient) }
     let(:route) { VCAP::CloudController::Route.make }
     let(:internal_domain?) { route.internal? }
     let(:route_crd_hash) do
@@ -36,8 +38,6 @@
       }
     end
 
-    subject(:route_crd_client) { Kubernetes::RouteCrdClient.new(kube_client) }
-
     it 'create a route resource in Kubernetes' do
       allow(kube_client).to receive(:create_route).with(any_args)
 
@@ -75,8 +75,19 @@
     end
   end
 
+  describe '#delete_route' do
+    let(:route) { VCAP::CloudController::Route.make(guid: 'theguid') }
+
+    it 'deletes a route resource in Kubernetes' do
+      allow(kube_client).to receive(:delete_route).with(any_args)
+
+      subject.delete_route(route)
+
+      expect(kube_client).to have_received(:delete_route).with('theguid', 'cf-workloads')
+    end
+  end
+
   describe '#update_destinations' do
-    let(:kube_client) { double(Kubeclient) }
     let(:route) { VCAP::CloudController::Route.make(path: '/some/path') }
     let(:route_cr) do
       Kubeclient::Resource.new({
@@ -110,8 +121,6 @@
       })
     end
 
-    subject(:route_crd_client) { Kubernetes::RouteCrdClient.new(kube_client) }
-
     before do
       allow(kube_client).to receive(:get_route).with(route.guid, 'cf-workloads').and_return(route_cr)
       allow(kube_client).to receive(:update_route).with(any_args)
diff --git a/spec/unit/models/runtime/organization_spec.rb b/spec/unit/models/runtime/organization_spec.rb
@@ -80,6 +80,10 @@ module VCAP::CloudController
     describe 'destroying' do
       let(:org) { Organization.make }
 
+      before do
+        TestConfig.override(kubernetes: {})
+      end
+
       context 'when there are isolation segments in the allowed list' do
         let(:isolation_segment_model) { IsolationSegmentModel.make }
         let(:isolation_segment_model2) { IsolationSegmentModel.make }
@@ -231,6 +235,7 @@ module VCAP::CloudController
           org = space.organization
           org.add_private_domain(private_domain)
           route = Route.make(space: space, domain: private_domain)
+          TestConfig.override(kubernetes: {})
 
           expect {
             org.remove_private_domain(private_domain)
diff --git a/spec/unit/models/runtime/private_domain_spec.rb b/spec/unit/models/runtime/private_domain_spec.rb
@@ -183,6 +183,9 @@ module VCAP::CloudController
 
     describe '#destroy' do
       let(:space) { Space.make(organization: private_domain.owning_organization) }
+      before do
+        TestConfig.override(kubernetes: {})
+      end
 
       it 'destroys the routes' do
         route = Route.make(domain: private_domain, space: space)
diff --git a/spec/unit/models/runtime/route_spec.rb b/spec/unit/models/runtime/route_spec.rb
diff --git a/spec/unit/models/runtime/shared_domain_spec.rb b/spec/unit/models/runtime/shared_domain_spec.rb
diff --git a/spec/unit/models/runtime/space_spec.rb b/spec/unit/models/runtime/space_spec.rb
diff --git a/spec/unit/repositories/route_event_repository_spec.rb b/spec/unit/repositories/route_event_repository_spec.rb
