From a02e35308b97d43964ebcf7fda040da418c04ddc Mon Sep 17 00:00:00 2001 From: Thong Kuah Date: Fri, 7 Sep 2018 23:48:06 +1200 Subject: [PATCH] Always create `gitlab` service account and service account token regardless of ABAC/RBAC This also solves the async nature of the automatic creation of default service tokens for service accounts. It also makes explicit which service account token we always use. create cluster role binding only if the provider has legacy_abac disabled. --- .../clusters/gcp/finalize_creation_service.rb | 11 +- app/services/clusters/gcp/kubernetes.rb | 1 + .../create_service_account_service.rb | 25 ++- .../fetch_kubernetes_token_service.rb | 26 +--- lib/gitlab/kubernetes/kube_client.rb | 1 + .../lib/gitlab/kubernetes/kube_client_spec.rb | 1 + .../gcp/finalize_creation_service_spec.rb | 143 ++++++++---------- .../create_service_account_service_spec.rb | 73 +++++++-- .../fetch_kubernetes_token_service_spec.rb | 48 +++--- spec/support/helpers/kubernetes_helpers.rb | 42 ++--- 10 files changed, 197 insertions(+), 174 deletions(-) diff --git a/app/services/clusters/gcp/finalize_creation_service.rb b/app/services/clusters/gcp/finalize_creation_service.rb index 8170e732d48..3ae0a4a19d0 100644 --- a/app/services/clusters/gcp/finalize_creation_service.rb +++ b/app/services/clusters/gcp/finalize_creation_service.rb @@ -8,9 +8,8 @@ module Clusters def execute(provider) @provider = provider - create_gitlab_service_account! - configure_provider + create_gitlab_service_account! configure_kubernetes cluster.save! @@ -25,9 +24,7 @@ module Clusters private def create_gitlab_service_account! - if create_rbac_cluster? - Clusters::Gcp::Kubernetes::CreateServiceAccountService.new(kube_client).execute - end + Clusters::Gcp::Kubernetes::CreateServiceAccountService.new(kube_client, rbac: create_rbac_cluster?).execute end def configure_provider @@ -47,9 +44,7 @@ module Clusters end def request_kubernetes_token - service_account_name = create_rbac_cluster? ? Clusters::Gcp::Kubernetes::SERVICE_ACCOUNT_NAME : 'default' - - Clusters::Gcp::Kubernetes::FetchKubernetesTokenService.new(kube_client, service_account_name).execute + Clusters::Gcp::Kubernetes::FetchKubernetesTokenService.new(kube_client).execute end def authorization_type diff --git a/app/services/clusters/gcp/kubernetes.rb b/app/services/clusters/gcp/kubernetes.rb index 74ef68eb58f..21a09891ac4 100644 --- a/app/services/clusters/gcp/kubernetes.rb +++ b/app/services/clusters/gcp/kubernetes.rb @@ -4,6 +4,7 @@ module Clusters module Gcp module Kubernetes SERVICE_ACCOUNT_NAME = 'gitlab' + SERVICE_ACCOUNT_TOKEN_NAME = 'gitlab-token' CLUSTER_ROLE_BINDING_NAME = 'gitlab-admin' CLUSTER_ROLE_NAME = 'cluster-admin' end diff --git a/app/services/clusters/gcp/kubernetes/create_service_account_service.rb b/app/services/clusters/gcp/kubernetes/create_service_account_service.rb index 8d87bd7b5c8..4c43b94d911 100644 --- a/app/services/clusters/gcp/kubernetes/create_service_account_service.rb +++ b/app/services/clusters/gcp/kubernetes/create_service_account_service.rb @@ -4,25 +4,32 @@ module Clusters module Gcp module Kubernetes class CreateServiceAccountService - attr_reader :kubeclient + attr_reader :kubeclient, :rbac - def initialize(kubeclient) + def initialize(kubeclient, rbac:) @kubeclient = kubeclient + @rbac = rbac end def execute kubeclient.create_service_account(service_account_resource) - kubeclient.create_cluster_role_binding(cluster_role_binding_resource) + kubeclient.create_secret(service_account_token_resource) + kubeclient.create_cluster_role_binding(cluster_role_binding_resource) if rbac end private def service_account_resource - Gitlab::Kubernetes::ServiceAccount.new(SERVICE_ACCOUNT_NAME, 'default').generate + Gitlab::Kubernetes::ServiceAccount.new(service_account_name, namespace).generate + end + + def service_account_token_resource + Gitlab::Kubernetes::ServiceAccountToken.new( + SERVICE_ACCOUNT_TOKEN_NAME, service_account_name, namespace).generate end def cluster_role_binding_resource - subjects = [{ kind: 'ServiceAccount', name: SERVICE_ACCOUNT_NAME, namespace: 'default' }] + subjects = [{ kind: 'ServiceAccount', name: service_account_name, namespace: namespace }] Gitlab::Kubernetes::ClusterRoleBinding.new( CLUSTER_ROLE_BINDING_NAME, @@ -30,6 +37,14 @@ module Clusters subjects ).generate end + + def service_account_name + SERVICE_ACCOUNT_NAME + end + + def namespace + 'default' + end end end end diff --git a/app/services/clusters/gcp/kubernetes/fetch_kubernetes_token_service.rb b/app/services/clusters/gcp/kubernetes/fetch_kubernetes_token_service.rb index c16ce451aaf..877dc1de89b 100644 --- a/app/services/clusters/gcp/kubernetes/fetch_kubernetes_token_service.rb +++ b/app/services/clusters/gcp/kubernetes/fetch_kubernetes_token_service.rb @@ -4,37 +4,25 @@ module Clusters module Gcp module Kubernetes class FetchKubernetesTokenService - attr_reader :kubeclient, :service_account_name + attr_reader :kubeclient - def initialize(kubeclient, service_account_name) + def initialize(kubeclient) @kubeclient = kubeclient - @service_account_name = service_account_name end def execute - read_secrets.each do |secret| - name = secret.dig('metadata', 'name') - if token_regex =~ name - token_base64 = secret.dig('data', 'token') - return Base64.decode64(token_base64) if token_base64 - end - end - - nil + token_base64 = get_secret&.dig('data', 'token') + Base64.decode64(token_base64) if token_base64 end private - def token_regex - /#{service_account_name}-token/ - end - - def read_secrets - kubeclient.get_secrets.as_json + def get_secret + kubeclient.get_secret(SERVICE_ACCOUNT_TOKEN_NAME).as_json rescue Kubeclient::HttpError => err raise err unless err.error_code == 404 - [] + nil end end end diff --git a/lib/gitlab/kubernetes/kube_client.rb b/lib/gitlab/kubernetes/kube_client.rb index 2f03d73dfb4..588238de608 100644 --- a/lib/gitlab/kubernetes/kube_client.rb +++ b/lib/gitlab/kubernetes/kube_client.rb @@ -25,6 +25,7 @@ module Gitlab :get_config_map, :get_namespace, :get_pod, + :get_secret, :get_service, :get_service_account, :delete_pod, diff --git a/spec/lib/gitlab/kubernetes/kube_client_spec.rb b/spec/lib/gitlab/kubernetes/kube_client_spec.rb index 7c057e66673..53c5a4e7c94 100644 --- a/spec/lib/gitlab/kubernetes/kube_client_spec.rb +++ b/spec/lib/gitlab/kubernetes/kube_client_spec.rb @@ -116,6 +116,7 @@ describe Gitlab::Kubernetes::KubeClient do :get_config_map, :get_pod, :get_namespace, + :get_secret, :get_service, :get_service_account, :delete_pod, diff --git a/spec/services/clusters/gcp/finalize_creation_service_spec.rb b/spec/services/clusters/gcp/finalize_creation_service_spec.rb index 1ea41b41771..0f484222228 100644 --- a/spec/services/clusters/gcp/finalize_creation_service_spec.rb +++ b/spec/services/clusters/gcp/finalize_creation_service_spec.rb @@ -12,9 +12,11 @@ describe Clusters::Gcp::FinalizeCreationService do let(:zone) { provider.zone } let(:cluster_name) { cluster.name } + subject { described_class.new.execute(provider) } + shared_examples 'success' do it 'configures provider and kubernetes' do - described_class.new.execute(provider) + subject expect(provider).to be_created end @@ -22,7 +24,7 @@ describe Clusters::Gcp::FinalizeCreationService do shared_examples 'error' do it 'sets an error to provider object' do - described_class.new.execute(provider) + subject expect(provider.reload).to be_errored end @@ -33,6 +35,7 @@ describe Clusters::Gcp::FinalizeCreationService do let(:api_url) { 'https://' + endpoint } let(:username) { 'sample-username' } let(:password) { 'sample-password' } + let(:secret_name) { 'gitlab-token' } before do stub_cloud_platform_get_zone_cluster( @@ -43,124 +46,98 @@ describe Clusters::Gcp::FinalizeCreationService do password: password } ) - - stub_kubeclient_discover(api_url) end - context 'when suceeded to fetch kuberenetes token' do - let(:secret_name) { 'default-token-Y1a' } - let(:token) { 'sample-token' } - + context 'service account and token created' do before do - stub_kubeclient_get_secrets( - api_url, - { - metadata_name: secret_name, - token: Base64.encode64(token) - } ) + stub_kubeclient_discover(api_url) + stub_kubeclient_create_service_account(api_url) + stub_kubeclient_create_secret(api_url) end - it_behaves_like 'success' - - it 'has corresponded data' do - described_class.new.execute(provider) - cluster.reload - provider.reload - platform.reload - - expect(provider.endpoint).to eq(endpoint) - expect(platform.api_url).to eq(api_url) - expect(platform.ca_cert).to eq(Base64.decode64(load_sample_cert)) - expect(platform.username).to eq(username) - expect(platform.password).to eq(password) - expect(platform.authorization_type).to eq('abac') - expect(platform.token).to eq(token) - end - - context 'rbac_clusters feature enabled' do - let(:secret_name) { 'gitlab-token-Y1a' } + shared_context 'kubernetes token successfully fetched' do + let(:token) { 'sample-token' } before do - provider.legacy_abac = false - - stub_kubeclient_create_service_account(api_url) - stub_kubeclient_create_cluster_role_binding(api_url) + stub_kubeclient_get_secret( + api_url, + { + metadata_name: secret_name, + token: Base64.encode64(token) + } ) end + end + + context 'provider legacy_abac is enabled' do + include_context 'kubernetes token successfully fetched' it_behaves_like 'success' - it 'has corresponded data' do - described_class.new.execute(provider) + it 'properly configures database models' do + subject + cluster.reload - provider.reload - platform.reload expect(provider.endpoint).to eq(endpoint) expect(platform.api_url).to eq(api_url) expect(platform.ca_cert).to eq(Base64.decode64(load_sample_cert)) expect(platform.username).to eq(username) expect(platform.password).to eq(password) - expect(platform.authorization_type).to eq('rbac') + expect(platform).to be_abac + expect(platform.authorization_type).to eq('abac') expect(platform.token).to eq(token) end end - end - context 'when no matching token is found' do - before do - stub_kubeclient_get_secrets(api_url, metadata_name: 'not-default-not-gitlab') - end - - it_behaves_like 'error' - - context 'rbac_clusters feature enabled' do + context 'provider legacy_abac is disabled' do before do provider.legacy_abac = false + end - stub_kubeclient_create_service_account(api_url) - stub_kubeclient_create_cluster_role_binding(api_url) + include_context 'kubernetes token successfully fetched' + + context 'cluster role binding created' do + before do + stub_kubeclient_create_cluster_role_binding(api_url) + end + + it_behaves_like 'success' + + it 'properly configures database models' do + subject + + cluster.reload + + expect(provider.endpoint).to eq(endpoint) + expect(platform.api_url).to eq(api_url) + expect(platform.ca_cert).to eq(Base64.decode64(load_sample_cert)) + expect(platform.username).to eq(username) + expect(platform.password).to eq(password) + expect(platform).to be_rbac + expect(platform.token).to eq(token) + end + end + end + + context 'when token is empty' do + before do + stub_kubeclient_get_secret(api_url, token: '', metadata_name: secret_name) end it_behaves_like 'error' end - end - - context 'when token is empty' do - let(:secret_name) { 'default-token-123' } - - before do - stub_kubeclient_get_secrets(api_url, token: '', metadata_name: secret_name) - end - - it_behaves_like 'error' - - context 'rbac_clusters feature enabled' do - let(:secret_name) { 'gitlab-token-321' } + context 'when failed to fetch kubernetes token' do before do - provider.legacy_abac = false - - stub_kubeclient_create_service_account(api_url) - stub_kubeclient_create_cluster_role_binding(api_url) + stub_kubeclient_get_secret_error(api_url, secret_name) end it_behaves_like 'error' end - end - context 'when failed to fetch kuberenetes token' do - before do - stub_kubeclient_get_secrets_error(api_url) - end - - it_behaves_like 'error' - - context 'rbac_clusters feature enabled' do + context 'when service account fails to create' do before do - provider.legacy_abac = false - - stub_kubeclient_create_service_account(api_url) - stub_kubeclient_create_cluster_role_binding(api_url) + stub_kubeclient_create_service_account_error(api_url) end it_behaves_like 'error' diff --git a/spec/services/clusters/gcp/kubernetes/create_service_account_service_spec.rb b/spec/services/clusters/gcp/kubernetes/create_service_account_service_spec.rb index 2dd4eedaf56..5268ae8a6d7 100644 --- a/spec/services/clusters/gcp/kubernetes/create_service_account_service_spec.rb +++ b/spec/services/clusters/gcp/kubernetes/create_service_account_service_spec.rb @@ -5,11 +5,12 @@ require 'spec_helper' describe Clusters::Gcp::Kubernetes::CreateServiceAccountService do include KubernetesHelpers - let(:service) { described_class.new(kubeclient) } + let(:service) { described_class.new(kubeclient, rbac: rbac) } describe '#execute' do subject { service.execute } + let(:rbac) { false } let(:api_url) { 'http://111.111.111.111' } let(:username) { 'admin' } let(:password) { 'xxx' } @@ -25,29 +26,69 @@ describe Clusters::Gcp::Kubernetes::CreateServiceAccountService do before do stub_kubeclient_discover(api_url) stub_kubeclient_create_service_account(api_url) - stub_kubeclient_create_cluster_role_binding(api_url) + stub_kubeclient_create_secret(api_url) end - it 'creates a kubernetes service account' do - subject + shared_examples 'creates service account and token' do + it 'creates a kubernetes service account' do + subject - expect(WebMock).to have_requested(:post, api_url + '/api/v1/namespaces/default/serviceaccounts').with( - body: hash_including( - metadata: { name: 'gitlab', namespace: 'default' } + expect(WebMock).to have_requested(:post, api_url + '/api/v1/namespaces/default/serviceaccounts').with( + body: hash_including( + kind: 'ServiceAccount', + metadata: { name: 'gitlab', namespace: 'default' } + ) ) - ) + end + + it 'creates a kubernetes secret of type ServiceAccountToken' do + subject + + expect(WebMock).to have_requested(:post, api_url + '/api/v1/namespaces/default/secrets').with( + body: hash_including( + kind: 'Secret', + metadata: { + name: 'gitlab-token', + namespace: 'default', + annotations: { + 'kubernetes.io/service-account.name': 'gitlab' + } + }, + type: 'kubernetes.io/service-account-token' + ) + ) + end end - it 'creates a kubernetes cluster role binding' do - subject + context 'abac enabled cluster' do + it_behaves_like 'creates service account and token' + end - expect(WebMock).to have_requested(:post, api_url + '/apis/rbac.authorization.k8s.io/v1/clusterrolebindings').with( - body: hash_including( - metadata: { name: 'gitlab-admin' }, - roleRef: { apiGroup: 'rbac.authorization.k8s.io', kind: 'ClusterRole', name: 'cluster-admin' }, - subjects: [{ kind: 'ServiceAccount', namespace: 'default', name: 'gitlab' }] + context 'rbac enabled cluster' do + let(:rbac) { true } + + before do + stub_kubeclient_create_cluster_role_binding(api_url) + end + + it_behaves_like 'creates service account and token' + + it 'creates a kubernetes cluster role binding' do + subject + + expect(WebMock).to have_requested(:post, api_url + '/apis/rbac.authorization.k8s.io/v1/clusterrolebindings').with( + body: hash_including( + kind: 'ClusterRoleBinding', + metadata: { name: 'gitlab-admin' }, + roleRef: { + apiGroup: 'rbac.authorization.k8s.io', + kind: 'ClusterRole', + name: 'cluster-admin' + }, + subjects: [{ kind: 'ServiceAccount', namespace: 'default', name: 'gitlab' }] + ) ) - ) + end end end end diff --git a/spec/services/clusters/gcp/kubernetes/fetch_kubernetes_token_service_spec.rb b/spec/services/clusters/gcp/kubernetes/fetch_kubernetes_token_service_spec.rb index 74d58a6d206..4c34f21c1bc 100644 --- a/spec/services/clusters/gcp/kubernetes/fetch_kubernetes_token_service_spec.rb +++ b/spec/services/clusters/gcp/kubernetes/fetch_kubernetes_token_service_spec.rb @@ -1,10 +1,9 @@ -require 'spec_helper' +require 'fast_spec_helper' describe Clusters::Gcp::Kubernetes::FetchKubernetesTokenService do describe '#execute' do - subject { described_class.new(kubeclient, service_account_name).execute } + subject { described_class.new(kubeclient).execute } - let(:service_account_name) { 'gitlab-sa' } let(:api_url) { 'http://111.111.111.111' } let(:username) { 'admin' } let(:password) { 'xxx' } @@ -18,42 +17,39 @@ describe Clusters::Gcp::Kubernetes::FetchKubernetesTokenService do end context 'when params correct' do - let(:token) { 'xxx.token.xxx' } + let(:decoded_token) { 'xxx.token.xxx' } + let(:token) { Base64.encode64(decoded_token) } - let(:secrets_json) do - [ - { - 'metadata': { - name: 'default-token-123' - }, - 'data': { - 'token': Base64.encode64('yyy.token.yyy') - } + let(:secret_json) do + { + 'metadata': { + name: 'gitlab-token' }, - { - 'metadata': { - name: metadata_name - }, - 'data': { - 'token': Base64.encode64(token) - } + 'data': { + 'token': token } - ] + } end before do allow_any_instance_of(Kubeclient::Client) - .to receive(:get_secrets).and_return(secrets_json) + .to receive(:get_secret).and_return(secret_json) end - context 'when token for service account exists' do - let(:metadata_name) { 'gitlab-sa-token-123' } + context 'when gitlab-token exists' do + let(:metadata_name) { 'gitlab-token' } - it { is_expected.to eq(token) } + it { is_expected.to eq(decoded_token) } end context 'when gitlab-token does not exist' do - let(:metadata_name) { 'another-token-123' } + let(:secret_json) { {} } + + it { is_expected.to be_nil } + end + + context 'when token is nil' do + let(:token) { nil } it { is_expected.to be_nil } end diff --git a/spec/support/helpers/kubernetes_helpers.rb b/spec/support/helpers/kubernetes_helpers.rb index 30af1e7928c..2fde5c8fde4 100644 --- a/spec/support/helpers/kubernetes_helpers.rb +++ b/spec/support/helpers/kubernetes_helpers.rb @@ -33,13 +33,15 @@ module KubernetesHelpers WebMock.stub_request(:get, deployments_url).to_return(response || kube_deployments_response) end - def stub_kubeclient_get_secrets(api_url, **options) - WebMock.stub_request(:get, api_url + '/api/v1/secrets') - .to_return(kube_response(kube_v1_secrets_body(options))) + def stub_kubeclient_get_secret(api_url, **options) + options[:metadata_name] ||= "default-token-1" + + WebMock.stub_request(:get, api_url + "/api/v1/secrets/#{options[:metadata_name]}") + .to_return(kube_response(kube_v1_secret_body(options))) end - def stub_kubeclient_get_secrets_error(api_url) - WebMock.stub_request(:get, api_url + '/api/v1/secrets') + def stub_kubeclient_get_secret_error(api_url, name) + WebMock.stub_request(:get, api_url + "/api/v1/secrets/#{name}") .to_return(status: [404, "Internal Server Error"]) end @@ -48,26 +50,32 @@ module KubernetesHelpers .to_return(kube_response({})) end + def stub_kubeclient_create_service_account_error(api_url, namespace: 'default') + WebMock.stub_request(:post, api_url + "/api/v1/namespaces/#{namespace}/serviceaccounts") + .to_return(status: [500, "Internal Server Error"]) + end + + def stub_kubeclient_create_secret(api_url, namespace: 'default') + WebMock.stub_request(:post, api_url + "/api/v1/namespaces/#{namespace}/secrets") + .to_return(kube_response({})) + end + def stub_kubeclient_create_cluster_role_binding(api_url) WebMock.stub_request(:post, api_url + '/apis/rbac.authorization.k8s.io/v1/clusterrolebindings') .to_return(kube_response({})) end - def kube_v1_secrets_body(**options) + def kube_v1_secret_body(**options) { "kind" => "SecretList", "apiVersion": "v1", - "items" => [ - { - "metadata": { - "name": options[:metadata_name] || "default-token-1", - "namespace": "kube-system" - }, - "data": { - "token": options[:token] || Base64.encode64('token-sample-123') - } - } - ] + "metadata": { + "name": options[:metadata_name] || "default-token-1", + "namespace": "kube-system" + }, + "data": { + "token": options[:token] || Base64.encode64('token-sample-123') + } } end