Fix k8s deployment namespace resolution

The namespace shown on the job show page was sometimes incorrect for
deployments to non-managed clusters when
environment:kubernetes:namespace was specified in the CI YAML. This was
due to the last successful deployment being used to resolve the CI YAML
namespace, instead of the CI YAML for the deployment being constructed.

Note that the correct namespace was still being used for the actual
deployment.

app/models/clusters/cluster.rb

@@ -249,9 +249,13 @@ module Clusters
       platform_kubernetes.kubeclient if kubernetes?
     end
 
-    def kubernetes_namespace_for(environment)
+    def kubernetes_namespace_for(environment, deployable: environment.last_deployable)
+      if deployable && environment.project_id != deployable.project_id
+        raise ArgumentError, 'environment.project_id must match deployable.project_id'
+      end
+
       managed_namespace(environment) ||
-        ci_configured_namespace(environment) ||
+        ci_configured_namespace(deployable) ||
         default_namespace(environment)
     end
 
@@ -318,8 +322,11 @@ module Clusters
       ).execute&.namespace
     end
 
-    def ci_configured_namespace(environment)
-      environment.last_deployable&.expanded_kubernetes_namespace
+    def ci_configured_namespace(deployable)
+      # YAML configuration of namespaces not supported for managed clusters
+      return if managed?
+
+      deployable&.expanded_kubernetes_namespace
     end
 
     def default_namespace(environment)

changelogs/unreleased/fix-deployment-namespace-resolution.yml

+---
+title: Fix Kubernetes namespace resolution for new DeploymentCluster records
+merge_request: 25853
+author:
+type: fixed

lib/gitlab/ci/pipeline/seed/deployment.rb

@@ -23,12 +23,12 @@ module Gitlab
             # non-environment job.
             return unless deployment.valid? && deployment.environment.persisted?
 
-            if cluster_id = deployment.environment.deployment_platform&.cluster_id
+            if cluster = deployment.environment.deployment_platform&.cluster
               # double write cluster_id until 12.9: https://gitlab.com/gitlab-org/gitlab/issues/202628
-              deployment.cluster_id = cluster_id
+              deployment.cluster_id = cluster.id
               deployment.deployment_cluster = ::DeploymentCluster.new(
-                cluster_id: cluster_id,
-                kubernetes_namespace: deployment.environment.deployment_namespace
+                cluster_id: cluster.id,
+                kubernetes_namespace: cluster.kubernetes_namespace_for(deployment.environment, deployable: job)
               )
             end
 

spec/factories/deployments.rb

@@ -7,7 +7,7 @@ FactoryBot.define do
     tag { false }
     user { nil }
     project { nil }
-    deployable factory: :ci_build
+    deployable { association :ci_build, environment: environment.name, project: environment.project }
     environment factory: :environment
 
     after(:build) do |deployment, evaluator|

spec/lib/gitlab/ci/pipeline/seed/deployment_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 describe Gitlab::Ci::Pipeline::Seed::Deployment do
-  let_it_be(:project) { create(:project, :repository) }
+  let_it_be(:project, refind: true) { create(:project, :repository) }
   let(:pipeline) do
     create(:ci_pipeline, project: project,
            sha: 'b83d6e391c22777fca1ed3012fce84f633d7fed0')
@@ -25,10 +25,12 @@ describe Gitlab::Ci::Pipeline::Seed::Deployment do
       let(:attributes) do
         {
           environment: 'production',
-          options: { environment: { name: 'production' } }
+          options: { environment: { name: 'production', **kubernetes_options } }
         }
       end
 
+      let(:kubernetes_options) { {} }
+
       it 'returns a deployment object with environment' do
         expect(subject).to be_a(Deployment)
         expect(subject.iid).to be_present
@@ -38,14 +40,30 @@ describe Gitlab::Ci::Pipeline::Seed::Deployment do
       end
 
       context 'when environment has deployment platform' do
-        let!(:cluster) { create(:cluster, :provided_by_gcp, projects: [project]) }
+        let!(:cluster) { create(:cluster, :provided_by_gcp, projects: [project], managed: managed_cluster) }
+        let(:managed_cluster) { true }
 
         it 'sets the cluster and deployment_cluster' do
           expect(subject.cluster).to eq(cluster) # until we stop double writing in 12.9: https://gitlab.com/gitlab-org/gitlab/issues/202628
-          expect(subject.deployment_cluster).to have_attributes(
-            cluster_id: cluster.id,
-            kubernetes_namespace: subject.environment.deployment_namespace
-          )
+          expect(subject.deployment_cluster.cluster).to eq(cluster)
+        end
+
+        context 'when a custom namespace is given' do
+          let(:kubernetes_options) { { kubernetes: { namespace: 'the-custom-namespace' } } }
+
+          context 'when cluster is managed' do
+            it 'does not set the custom namespace' do
+              expect(subject.deployment_cluster.kubernetes_namespace).not_to eq('the-custom-namespace')
+            end
+          end
+
+          context 'when cluster is not managed' do
+            let(:managed_cluster) { false }
+
+            it 'sets the custom namespace' do
+              expect(subject.deployment_cluster.kubernetes_namespace).to eq('the-custom-namespace')
+            end
+          end
         end
       end
 

spec/models/clusters/cluster_spec.rb

@@ -674,59 +674,59 @@ describe Clusters::Cluster, :use_clean_rails_memory_store_caching do
   end
 
   describe '#kubernetes_namespace_for' do
-    let(:cluster) { create(:cluster, :group) }
-    let(:environment) { create(:environment, last_deployable: build) }
-    let(:build) { create(:ci_build) }
-
-    subject { cluster.kubernetes_namespace_for(environment) }
+    subject { cluster.kubernetes_namespace_for(environment, deployable: build) }
 
-    before do
-      expect(Clusters::KubernetesNamespaceFinder).to receive(:new)
-        .with(cluster, project: environment.project, environment_name: environment.name)
-        .and_return(double(execute: persisted_namespace))
+    let(:environment_name) { 'the-environment-name' }
+    let(:environment) { create(:environment, name: environment_name, project: cluster.project, last_deployable: build) }
+    let(:build) { create(:ci_build, environment: environment_name, project: cluster.project) }
+    let(:cluster) { create(:cluster, :project, managed: managed_cluster) }
+    let(:managed_cluster) { true }
+    let(:default_namespace) { Gitlab::Kubernetes::DefaultNamespace.new(cluster, project: cluster.project).from_environment_slug(environment.slug) }
+    let(:build_options) { {} }
 
-      allow(build).to receive(:expanded_kubernetes_namespace)
-        .and_return(ci_configured_namespace)
+    it 'validates the project id' do
+      environment.project_id = build.project_id + 1
+      expect { subject }.to raise_error ArgumentError, 'environment.project_id must match deployable.project_id'
     end
 
-    context 'no persisted namespace exists and namespace is not specified in CI template' do
-      let(:persisted_namespace) { nil }
-      let(:ci_configured_namespace) { nil }
+    context 'when environment has no last_deployable' do
+      let(:build) { nil }
 
-      let(:namespace_generator) { double }
-      let(:default_namespace) { 'a-default-namespace' }
+      it { is_expected.to eq default_namespace }
+    end
 
+    context 'when cluster is managed' do
       before do
-        expect(Gitlab::Kubernetes::DefaultNamespace).to receive(:new)
-          .with(cluster, project: environment.project)
-          .and_return(namespace_generator)
-        expect(namespace_generator).to receive(:from_environment_slug)
-          .with(environment.slug)
-          .and_return(default_namespace)
+        build.options = { environment: { kubernetes: { namespace: 'ci yaml namespace' } } }
       end
 
-      it { is_expected.to eq default_namespace }
+      it 'returns the cached namespace if present, ignoring CI config' do
+        cached_namespace = create(:cluster_kubernetes_namespace, cluster: cluster, environment: environment, namespace: 'the name', service_account_token: 'some token')
+        expect(subject).to eq cached_namespace.namespace
       end
 
-    context 'persisted namespace exists' do
-      let(:persisted_namespace) { create(:cluster_kubernetes_namespace) }
-      let(:ci_configured_namespace) { nil }
-
-      it { is_expected.to eq persisted_namespace.namespace }
+      it 'returns the default namespace when no cached namespace, ignoring CI config' do
+        expect(subject).to eq default_namespace
+      end
     end
 
-    context 'namespace is specified in CI template' do
-      let(:persisted_namespace) { nil }
-      let(:ci_configured_namespace) { 'ci-configured-namespace' }
+    context 'when cluster is not managed' do
+      let(:managed_cluster) { false }
 
-      it { is_expected.to eq ci_configured_namespace }
+      it 'returns the cached namespace if present, regardless of CI config' do
+        cached_namespace = create(:cluster_kubernetes_namespace, cluster: cluster, environment: environment, namespace: 'the name', service_account_token: 'some token')
+        build.options = { environment: { kubernetes: { namespace: 'ci yaml namespace' } } }
+        expect(subject).to eq cached_namespace.namespace
       end
 
-    context 'persisted namespace exists and namespace is also specifed in CI template' do
-      let(:persisted_namespace) { create(:cluster_kubernetes_namespace) }
-      let(:ci_configured_namespace) { 'ci-configured-namespace' }
+      it 'returns the CI YAML namespace when configured' do
+        build.options = { environment: { kubernetes: { namespace: 'ci yaml namespace' } } }
+        expect(subject).to eq 'ci yaml namespace'
+      end
 
-      it { is_expected.to eq persisted_namespace.namespace }
+      it 'returns the default namespace when no namespace is configured' do
+        expect(subject).to eq default_namespace
+      end
     end
   end