Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch namespaced deploy to pruning blacklist #616

Merged
merged 3 commits into from
Nov 15, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 35 additions & 24 deletions lib/krane/cluster_resource_discovery.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,41 +16,19 @@ def crds
end
end

def global_resource_kinds
@globals ||= fetch_resources(namespaced: false).map { |g| g["kind"] }
end

def prunable_resources(namespaced:)
black_list = %w(Namespace Node)
api_versions = fetch_api_versions

fetch_resources(namespaced: namespaced).map do |resource|
next unless resource['verbs'].one? { |v| v == "delete" }
next if black_list.include?(resource['kind'])
version = api_versions[resource['apigroup'].to_s].last
group_versions = api_versions[resource['apigroup'].to_s]
version = version_for_kind(group_versions, resource['kind'])
[resource['apigroup'], version, resource['kind']].compact.join("/")
end.compact
end

private

# kubectl api-versions returns a list of group/version strings e.g. autoscaling/v2beta2
# A kind may not exist in all versions of the group.
def fetch_api_versions
raw, _, st = kubectl.run("api-versions", attempts: 5, use_namespace: false)
# The "core" group is represented by an empty string
versions = { "" => %w(v1) }
if st.success?
rows = raw.split("\n")
rows.each do |group_version|
group, version = group_version.split("/")
versions[group] ||= []
versions[group] << version
end
end
versions
end

# kubectl api-resources -o wide returns 5 columns
# NAME SHORTNAMES APIGROUP NAMESPACED KIND VERBS
# SHORTNAMES and APIGROUP may be blank
Expand Down Expand Up @@ -85,6 +63,39 @@ def fetch_resources(namespaced: false)
end
end

private

# kubectl api-versions returns a list of group/version strings e.g. autoscaling/v2beta2
# A kind may not exist in all versions of the group.
def fetch_api_versions
raw, _, st = kubectl.run("api-versions", attempts: 5, use_namespace: false)
# The "core" group is represented by an empty string
versions = { "" => %w(v1) }
if st.success?
rows = raw.split("\n")
rows.each do |group_version|
group, version = group_version.split("/")
versions[group] ||= []
versions[group] << version
end
end
versions
end

def version_for_kind(versions, kind)
# Override list for kinds that don't appear in the lastest version of a group
version_override = { "CronJob" => "v1beta1", "VolumeAttachment" => "v1beta1",
"CSIDriver" => "v1beta1", "Ingress" => "v1beta1", "CSINode" => "v1beta1" }

pattern = /v(?<major>\d+)(?<pre>alpha|beta)?(?<minor>\d+)?/
latest = versions.sort_by do |version|
match = version.match(pattern)
pre = { "alpha" => 0, "beta" => 1, nil => 2 }.fetch(match[:pre])
[match[:major].to_i, pre, match[:minor].to_i]
end.last
version_override.fetch(kind, latest)
end

def fetch_crds
raw_json, _, st = kubectl.run("get", "CustomResourceDefinition", output: "json", attempts: 5,
use_namespace: false)
Expand Down
4 changes: 4 additions & 0 deletions lib/krane/deploy_task.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,9 @@ def initialize(**args)
raise "Use Krane::DeployGlobalTask to deploy global resources" if args[:allow_globals]
super(args.merge(allow_globals: false))
end

def prune_whitelist
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we adding this here (instead of updating Krane::DeprecatedDeployTask) to avoid changing KubernetesDeploy::DeployTask?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

cluster_resource_discoverer.prunable_resources(namespaced: true)
end
end
end
2 changes: 1 addition & 1 deletion lib/krane/task_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ def initialize(context, namespace, logger = nil)
def global_kinds
@global_kinds ||= begin
cluster_resource_discoverer = ClusterResourceDiscovery.new(task_config: self)
cluster_resource_discoverer.global_resource_kinds
cluster_resource_discoverer.fetch_resources(namespaced: false).map { |g| g["kind"] }
end
end
end
Expand Down
31 changes: 31 additions & 0 deletions test/fixtures/for_unit_tests/api_resources_namespaced.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
NAME SHORTNAMES APIGROUP NAMESPACED KIND VERBS
bindings true Binding [create]
configmaps cm true ConfigMap [create delete deletecollection get list patch update watch]
endpoints ep true Endpoints [create delete deletecollection get list patch update watch]
events ev true Event [create delete deletecollection get list patch update watch]
limitranges limits true LimitRange [create delete deletecollection get list patch update watch]
persistentvolumeclaims pvc true PersistentVolumeClaim [create delete deletecollection get list patch update watch]
pods po true Pod [create delete deletecollection get list patch update watch]
podtemplates true PodTemplate [create delete deletecollection get list patch update watch]
replicationcontrollers rc true ReplicationController [create delete deletecollection get list patch update watch]
resourcequotas quota true ResourceQuota [create delete deletecollection get list patch update watch]
secrets true Secret [create delete deletecollection get list patch update watch]
serviceaccounts sa true ServiceAccount [create delete deletecollection get list patch update watch]
services svc true Service [create delete get list patch update watch]
controllerrevisions apps true ControllerRevision [create delete deletecollection get list patch update watch]
daemonsets ds apps true DaemonSet [create delete deletecollection get list patch update watch]
deployments deploy apps true Deployment [create delete deletecollection get list patch update watch]
replicasets rs apps true ReplicaSet [create delete deletecollection get list patch update watch]
statefulsets sts apps true StatefulSet [create delete deletecollection get list patch update watch]
localsubjectaccessreviews authorization.k8s.io true LocalSubjectAccessReview [create]
horizontalpodautoscalers hpa autoscaling true HorizontalPodAutoscaler [create delete deletecollection get list patch update watch]
cronjobs cj batch true CronJob [create delete deletecollection get list patch update watch]
jobs batch true Job [create delete deletecollection get list patch update watch]
leases coordination.k8s.io true Lease [create delete deletecollection get list patch update watch]
events ev events.k8s.io true Event [create delete deletecollection get list patch update watch]
ingresses ing extensions true Ingress [create delete deletecollection get list patch update watch]
ingresses ing networking.k8s.io true Ingress [create delete deletecollection get list patch update watch]
networkpolicies netpol networking.k8s.io true NetworkPolicy [create delete deletecollection get list patch update watch]
poddisruptionbudgets pdb policy true PodDisruptionBudget [create delete deletecollection get list patch update watch]
rolebindings rbac.authorization.k8s.io true RoleBinding [create delete deletecollection get list patch update watch]
roles rbac.authorization.k8s.io true Role [create delete deletecollection get list patch update watch]
3 changes: 1 addition & 2 deletions test/helpers/fixture_deploy_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,7 @@ def namespace_globals(fixtures, selector)
fixtures.each do |_, kinds_map|
kinds_map.each do |_, resources|
resources.each do |resource|
resource["metadata"]["name"] = (resource["metadata"]["name"] + @namespace)[0..63]
resource["metadata"]["name"] += "0" if resource["metadata"]["name"].end_with?("-")
resource["metadata"]["name"] = (resource["metadata"]["name"] + Digest::MD5.hexdigest(@namespace))[0..63]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to fix a bug that was causing resources to have the same name across different tests. Its triggered when the real name was long as the start of two tests were the same. We need the value appended to the name to be stable across tests hence the hashing of @namespace.

resource["metadata"]["labels"] ||= {}
resource["metadata"]["labels"][selector_key] = selector_value
end
Expand Down
55 changes: 40 additions & 15 deletions test/unit/cluster_resource_discovery_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,41 +2,62 @@
require 'test_helper'

class ClusterResourceDiscoveryTest < Krane::TestCase
def test_global_resource_kinds_failure
def test_fetch_resources_failure
crd = mocked_cluster_resource_discovery(nil, success: false)
kinds = crd.global_resource_kinds
assert_equal(kinds, [])
resources = crd.fetch_resources
assert_equal(resources, [])
end

def test_global_resource_kinds_success
crd = mocked_cluster_resource_discovery(api_resources_full_response)
kinds = crd.global_resource_kinds
assert_equal(kinds.length, api_resources_full_response.split("\n").length - 1)
def test_fetch_resources_not_namespaced
crd = mocked_cluster_resource_discovery(api_resources_not_namespaced_full_response)
kinds = crd.fetch_resources(namespaced: false).map { |r| r['kind'] }
assert_equal(kinds.length, api_resources_not_namespaced_full_response.split("\n").length - 1)
%w(MutatingWebhookConfiguration ComponentStatus CustomResourceDefinition).each do |kind|
assert_includes(kinds, kind)
end
end

def test_prunable_resources
def test_fetch_resources_namespaced
crd = mocked_cluster_resource_discovery(api_resources_namespaced_full_response, namespaced: true)
kinds = crd.fetch_resources(namespaced: true).map { |r| r['kind'] }
assert_equal(kinds.length, api_resources_namespaced_full_response.split("\n").length - 1)
%w(ConfigMap CronJob Deployment).each do |kind|
assert_includes(kinds, kind)
end
end

def test_prunable_global_resources
Krane::Kubectl.any_instance.stubs(:run).with("api-versions", attempts: 5, use_namespace: false)
.returns([api_versions_full_response, "", stub(success?: true)])
crd = mocked_cluster_resource_discovery(api_resources_full_response)
crd = mocked_cluster_resource_discovery(api_resources_not_namespaced_full_response)
kinds = crd.prunable_resources(namespaced: false)

assert_equal(kinds.length, 13)
%w(scheduling.k8s.io/v1beta1/PriorityClass storage.k8s.io/v1beta1/StorageClass).each do |kind|
assert_includes(kinds, kind)
%w(PriorityClass StorageClass).each do |expected_kind|
assert kinds.one? { |k| k.include?(expected_kind) }
Comment on lines +36 to +37
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just a sanity check or is there a reason for these 2 in particular?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a sanity check, the assert_equal(kinds.length, 13) is the check to ensure list has the right number of items.

end
%w(node namespace).each do |black_lised_kind|
assert_empty kinds.select { |k| k.downcase.include?(black_lised_kind) }
end
end

def test_prunable_namespaced_resources
Krane::Kubectl.any_instance.stubs(:run).with("api-versions", attempts: 5, use_namespace: false)
.returns([api_versions_full_response, "", stub(success?: true)])
crd = mocked_cluster_resource_discovery(api_resources_namespaced_full_response, namespaced: true)
kinds = crd.prunable_resources(namespaced: true)

assert_equal(kinds.length, 28)
%w(ConfigMap CronJob Deployment).each do |expected_kind|
assert kinds.one? { |k| k.include?(expected_kind) }
end
end

private

def mocked_cluster_resource_discovery(response, success: true)
def mocked_cluster_resource_discovery(response, success: true, namespaced: false)
Krane::Kubectl.any_instance.stubs(:run)
.with("api-resources", "--namespaced=false", attempts: 5, use_namespace: false, output: "wide")
.with("api-resources", "--namespaced=#{namespaced}", attempts: 5, use_namespace: false, output: "wide")
.returns([response, "", stub(success?: success)])
Krane::ClusterResourceDiscovery.new(task_config: task_config, namespace_tags: [])
end
Expand All @@ -45,7 +66,11 @@ def api_versions_full_response
File.read(File.join(fixture_path('for_unit_tests'), "api_versions.txt"))
end

def api_resources_full_response
File.read(File.join(fixture_path('for_unit_tests'), "api_resources.txt"))
def api_resources_namespaced_full_response
File.read(File.join(fixture_path('for_unit_tests'), "api_resources_namespaced.txt"))
end

def api_resources_not_namespaced_full_response
File.read(File.join(fixture_path('for_unit_tests'), "api_resources_global.txt"))
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,6 @@
class HorizontalPodAutoscalerTest < Krane::TestCase
include ResourceCacheTestHelper

# We can't get integration coverage for HPA right now because the metrics server just isn't reliable enough on our CI
def test_hpa_is_whitelisted_for_pruning
Krane::Kubectl.any_instance.expects("run")
.with("get", "CustomResourceDefinition", output: "json", attempts: 5, use_namespace: false)
.returns(['{ "items": [] }', "", SystemExit.new(0)])
task = Krane::DeployTask.new(namespace: 'test', context: KubeclientHelper::TEST_CONTEXT,
current_sha: 'foo', template_paths: [''], logger: logger)
assert(task.prune_whitelist.one? { |whitelisted_type| whitelisted_type.include?("HorizontalPodAutoscaler") })
end

def test_hpa_succeeds_when_scaling_is_active
conditions = [{
"lastTransitionTime" => 5.seconds.ago,
Expand Down