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.

Sign up for GitHub

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

Jump to bottom

Account for mutating webhooks with side effects when batch dry-run server-side apply #798

Merged
merged 28 commits into from
Feb 9, 2021
Merged

Account for mutating webhooks with side effects when batch dry-run server-side apply #798

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
b41b31c
initial attempt
timothysmith0609 Feb 2, 2021
f09f26a
more work
timothysmith0609 Feb 2, 2021
911dc8a
refinement
timothysmith0609 Feb 2, 2021
3713af8
test fix
timothysmith0609 Feb 2, 2021
a63e5d2
readd comment
timothysmith0609 Feb 2, 2021
7062e27
cleanup
timothysmith0609 Feb 2, 2021
a9b9bc4
more fix
timothysmith0609 Feb 2, 2021
f120b8e
grr
timothysmith0609 Feb 2, 2021
eb50b0e
Update lib/krane/kubernetes_resource.rb
timothysmith0609 Feb 3, 2021
a79eb66
dup resources to avoid mutating issues
timothysmith0609 Feb 3, 2021
78d2b1d
fix test
timothysmith0609 Feb 3, 2021
25b7727
Merge branch 'server_dry_run_mutating_webhooks_side_effects' of https…
timothysmith0609 Feb 3, 2021
3c2e91e
empty commit
timothysmith0609 Feb 3, 2021
893d6c3
object-oriented MutatingWebhookConfiguration"
timothysmith0609 Feb 3, 2021
4d9c65c
still need to validate all definitions, but not necessarily dry run
timothysmith0609 Feb 3, 2021
53dbcf3
Fix test
timothysmith0609 Feb 3, 2021
8ed6125
MutatingWebhookConfiguration unit test
timothysmith0609 Feb 3, 2021
cb38c18
test fix
timothysmith0609 Feb 3, 2021
df9dc68
File organization + test for EXACT matching
timothysmith0609 Feb 4, 2021
d0adc58
map domain name objects more closely + newline
timothysmith0609 Feb 5, 2021
d464370
resources field may also be wildcard
timothysmith0609 Feb 8, 2021
f787151
typo
timothysmith0609 Feb 8, 2021
deb2738
Use actual webhookconfiguration in-cluster for tests + fixes
timothysmith0609 Feb 8, 2021
2225cea
cop
timothysmith0609 Feb 8, 2021
86370b2
more test fix
timothysmith0609 Feb 8, 2021
7e20830
comment
timothysmith0609 Feb 8, 2021
dd15453
cleanup fixtures
timothysmith0609 Feb 8, 2021
c0677d2
changelog
timothysmith0609 Feb 9, 2021
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
10 changes: 10 additions & 0 deletions lib/krane/cluster_resource_discovery.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,16 @@ def fetch_resources(namespaced: false)
end
end

def fetch_mutating_webhook_configurations
command = %w(get mutatingwebhookconfigurations)
raw_json, err, st = kubectl.run(*command, output: "json", attempts: 5, use_namespace: false)
if st.success?
JSON.parse(raw_json)["items"]
else
raise FatalKubeAPIError, "Error retrieving mutatingwebhookconfigurations: #{err}"
end
end

private

# kubectl api-versions returns a list of group/version strings e.g. autoscaling/v2beta2
Expand Down
42 changes: 34 additions & 8 deletions lib/krane/deploy_task.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,16 +277,42 @@ def validate_configuration(prune:)
end
measure_method(:validate_configuration)

def partition_dry_run_resources(resources)
individuals = []
mutating_webhooks = cluster_resource_discoverer.fetch_mutating_webhook_configurations
mutating_webhooks.each do |spec|
spec.dig('webhooks').each do |webhook|
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes me think we want a MutatingWebhookConfigurations class instead of putting all of the logic in this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made a few attempts at this. Modelling it as a KubernetesResource didn't seem right, but neither did modelling it in the top-level namespace. If I had to make a choice, I'm tempted to do the latter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe its time for an Internal namespace for objects that are K8s objects but aren't designed for having deploy logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm loath to make the separation... ultimately this is still just a kubernetes object. For now, I've made it a subclass of KubernetesResource and added the appropriate overrides should one of these ever be deployed.

match_policy = webhook.dig('matchPolicy')
webhook.dig('rules').each do |rule|
next if %w(None NoneOnDryRun).include?(rule.dig('sideEffects'))
groups = rule.dig('apiGroups')
versions = rule.dig('apiVersions')
kinds = rule.dig('resources').map(&:singularize)
groups.each do |group|
versions.each do |version|
kinds.each do |kind|
individuals += resources.select do |r|
(r.group == group || group == '*' || match_policy == "Equivalent") &&
(r.version == version || version == '*' || match_policy == "Equivalent") &&
(r.type.downcase == kind.downcase)
end
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 a straightforward implementation of the rules defined here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea how to ask the api server what GVKs it knows to convert between? Not saying we use them, but this adds to our list of things that break with non-unique kinds.

Copy link
Contributor Author

@timothysmith0609 timothysmith0609 Feb 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Any idea how to ask the api server what GVKs it knows to convert between?

From my understanding, the Equivalent condition simply disregards gv (I'm tempted to open an upstream issue since this can have undesirable side-effects as we've found). I don't think this is something for Krane to deal with.

From the official reference:

Equivalent: match a request if modifies a resource listed in rules, even via another API group or version. For example, if deployments can be modified via apps/v1, apps/v1beta1, and extensions/v1beta1, and "rules" only included apiGroups:["apps"], apiVersions:["v1"], resources: ["deployments"], a request to apps/v1beta1 or extensions/v1beta1 would be converted to apps/v1 and sent to the webhook.

resources -= individuals
Copy link
Contributor

Choose a reason for hiding this comment

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

I can never remember the pass by semantics of ruby but, Is resources a copy or does this impact validate_resources

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's pass by reference, however the -= operator returns a new object, not a mutation of the old one. If we used, e.g. <<, then we would mutate the incoming object. As a matter of caution, I'll dup the resources array to avoid any issues in the future.

end
end
end
end
end
end
[resources, individuals]
end

def validate_resources(resources)
validate_globals(resources)
batch_dry_run_success = kubectl.server_dry_run_enabled? && validate_dry_run(resources)
Krane::Concurrency.split_across_threads(resources) do |r|
# No need to pass in kubectl (and do per-resource dry run apply) if batch dry run succeeded
if batch_dry_run_success
r.validate_definition(kubectl: nil, selector: @selector, dry_run: false)
else
r.validate_definition(kubectl: kubectl, selector: @selector, dry_run: true)
end
batchable_resources, individuals = partition_dry_run_resources(resources)
batch_dry_run_success = kubectl.server_dry_run_enabled? && validate_dry_run(batchable_resources)
individuals += batchable_resources unless batch_dry_run_success
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, we still fall back to per-resource dry-running if the batch fails

Krane::Concurrency.split_across_threads(individuals) do |r|
r.validate_definition(kubectl: kubectl, selector: @selector, dry_run: true)
end
failed_resources = resources.select(&:validation_failed?)
if failed_resources.present?
Expand Down
5 changes: 5 additions & 0 deletions lib/krane/kubernetes_resource.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,11 @@ def group
version ? grouping : "core"
end

def version
prefix, version = @definition.dig("apiVersion").split("/")
version ? version : prefix
timothysmith0609 marked this conversation as resolved.
Show resolved Hide resolved
end

def kubectl_resource_type
type
end
Expand Down
4 changes: 0 additions & 4 deletions lib/krane/kubernetes_resource/config_map.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,6 @@ def deploy_succeeded?
exists?
end

def status
exists? ? "Available" : "Not Found"
end

def deploy_failed?
false
end
Expand Down
1 change: 0 additions & 1 deletion lib/krane/resource_deployer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,6 @@ def apply_all(resources, prune, dry_run: false)
global_mode = resources.all?(&:global?)
out, err, st = kubectl.run(*command, log_failure: false, output_is_sensitive: output_is_sensitive,
attempts: 2, use_namespace: !global_mode)

tags = statsd_tags + (dry_run ? ['dry_run:true'] : ['dry_run:false'])
Krane::StatsD.client.distribution('apply_all.duration', Krane::StatsD.duration(start), tags: tags)
if st.success?
Expand Down
71 changes: 71 additions & 0 deletions test/fixtures/for_serial_deploy_tests/ingress_hook.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
{
"apiVersion": "v1",
"items": [
{
"apiVersion": "admissionregistration.k8s.io/v1",
"kind": "MutatingWebhookConfiguration",
"metadata": {
"creationTimestamp": "2020-08-04T19:49:44Z",
"generation": 2,
"name": "oauthboss-webhook-configuration",
"resourceVersion": "3115431",
"selfLink": "/apis/admissionregistration.k8s.io/v1/mutatingwebhookconfigurations/oauthboss-webhook-configuration",
"uid": "7afce875-5175-430d-95aa-de8ec97f15b0"
},
"webhooks": [
{
"admissionReviewVersions": [
"v1beta1"
],
"clientConfig": {
"caBundle": "LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUMwakNDQWJxZ0F3SUJBZ0lCQURBTkJna3Foa2lHOXcwQkFRc0ZBREFhTVJnd0ZnWURWUVFERXc5M1pXSm8KYjI5ckxXTmxjblF0WTJFd0hoY05NakF3T0RBME1UazBPVFExV2hjTk16QXdPREF5TVRrME9UUTFXakFhTVJndwpGZ1lEVlFRREV3OTNaV0pvYjI5ckxXTmxjblF0WTJFd2dnRWlNQTBHQ1NxR1NJYjNEUUVCQVFVQUE0SUJEd0F3CmdnRUtBb0lCQVFDOGs1MjNobEFsald4RitITk8wWk9qQTZ4MGNSa1RQVHdkZm9nK3JGOXZ3Y21XRTBFT3kyYlUKZ3BNRU5SQzVpVzRxS1pCYUJ6MEU1bXcvbWNjcGNrbXR1V05pODVML0hzUjVlbWR2aWdUamJYb0RMeWxEdjNMLwpRSlVWOVRIYWFhaXh5cmFMNWhIRUw4bTBhTFNZcWc5MktxODJnWXB0aTB5azBnU1BqdjhUTHBFazhvdWp2L01YCnVJZWtQQzZOTWFDaU1BY1FBTVRyc1NiSmZqL0x0WGpwWk9FM2xCNWpTazNDUW1sT0t5Mm8zaDd6WTZ1RFNtN28KY3orMkhiUXhybCt1SkcrY1B4WVdoWXZReXpLWGViWU11ajd5Wm5FRmI2WFc3dC9ROVo2VmFzMi9DS1Z4Yll4LwppdXEvbU1xZk4xZnNvUitJTmVXVlJ3YlJ0dUJ5VkljRkFnTUJBQUdqSXpBaE1BNEdBMVVkRHdFQi93UUVBd0lDCnBEQVBCZ05WSFJNQkFmOEVCVEFEQVFIL01BMEdDU3FHU0liM0RRRUJDd1VBQTRJQkFRQXc4VWQwOWRYR09kQkwKYnQ5M3JuYk41VzBkMkJUZk43OXJ6SVpadHlmMjREQVQ1SGc1QVV2Qmc4QjJNaFRqdU50VlN6dzN4bGV3TDk3aQo0Wk9JWGJPL3hZTWp3OXRibldDY2o0MlVKRnJUOEN2eVZEekExeS9VVE1lMFpyU3lOUGpWTlBaMmUrb3IvRFdkCmRtbmg0THZ6c3VNbVBPZk96NmUwZmljMzdtLzZjL3JZV0l6aTJYWWRwSllHa1J1eW51UlVUbDZpUjBOa0xXaGYKc25aNll6TnFXT2YvNi9xdHduNkhVckttYWN3L2Q3blBNSDRpQTFId1MyZFh6WkxVR3NVZkZIREFkU2FVSWR3cApsZDFUUjErQjVyMnhiUW95eVo4YnhuTk5Ud04rU2dFZGc5MjJKUFlaRkZFWldDL1NXOWVYcUZnTWM3RlQyMHh4ClE1NW1qdHRICi0tLS0tRU5EIENFUlRJRklDQVRFLS0tLS0K",
"service": {
"name": "oauthboss",
"namespace": "cloudbosses",
"path": "/oauthboss",
"port": 443
}
},
"failurePolicy": "Ignore",
"matchPolicy": "Exact",
"name": "oauthboss.cloudbosses.admission.online.shopify.com",
"namespaceSelector": {
"matchExpressions": [
{
"key": "control-plane",
"operator": "DoesNotExist"
}
]
},
"objectSelector": {},
"reinvocationPolicy": "Never",
"rules": [
{
"apiGroups": [
"extensions"
],
"apiVersions": [
"v1beta1"
],
"operations": [
"CREATE",
"UPDATE"
],
"resources": [
"ingresses"
],
"scope": "*"
}
],
"sideEffects": "Unknown",
"timeoutSeconds": 30
}
]
}
],
"kind": "List",
"metadata": {
"resourceVersion": "",
"selfLink": ""
}
}
71 changes: 71 additions & 0 deletions test/fixtures/for_serial_deploy_tests/secret_hook.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
{
"apiVersion": "v1",
"items": [
{
"apiVersion": "admissionregistration.k8s.io/v1",
"kind": "MutatingWebhookConfiguration",
"metadata": {
"creationTimestamp": "2020-08-04T19:49:44Z",
"generation": 2,
"name": "oauthboss-webhook-configuration",
"resourceVersion": "3115431",
"selfLink": "/apis/admissionregistration.k8s.io/v1/mutatingwebhookconfigurations/oauthboss-webhook-configuration",
"uid": "7afce875-5175-430d-95aa-de8ec97f15b0"
},
"webhooks": [
{
"admissionReviewVersions": [
"v1beta1"
],
"clientConfig": {
"caBundle": "LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUMwakNDQWJxZ0F3SUJBZ0lCQURBTkJna3Foa2lHOXcwQkFRc0ZBREFhTVJnd0ZnWURWUVFERXc5M1pXSm8KYjI5ckxXTmxjblF0WTJFd0hoY05NakF3T0RBME1UazBPVFExV2hjTk16QXdPREF5TVRrME9UUTFXakFhTVJndwpGZ1lEVlFRREV3OTNaV0pvYjI5ckxXTmxjblF0WTJFd2dnRWlNQTBHQ1NxR1NJYjNEUUVCQVFVQUE0SUJEd0F3CmdnRUtBb0lCQVFDOGs1MjNobEFsald4RitITk8wWk9qQTZ4MGNSa1RQVHdkZm9nK3JGOXZ3Y21XRTBFT3kyYlUKZ3BNRU5SQzVpVzRxS1pCYUJ6MEU1bXcvbWNjcGNrbXR1V05pODVML0hzUjVlbWR2aWdUamJYb0RMeWxEdjNMLwpRSlVWOVRIYWFhaXh5cmFMNWhIRUw4bTBhTFNZcWc5MktxODJnWXB0aTB5azBnU1BqdjhUTHBFazhvdWp2L01YCnVJZWtQQzZOTWFDaU1BY1FBTVRyc1NiSmZqL0x0WGpwWk9FM2xCNWpTazNDUW1sT0t5Mm8zaDd6WTZ1RFNtN28KY3orMkhiUXhybCt1SkcrY1B4WVdoWXZReXpLWGViWU11ajd5Wm5FRmI2WFc3dC9ROVo2VmFzMi9DS1Z4Yll4LwppdXEvbU1xZk4xZnNvUitJTmVXVlJ3YlJ0dUJ5VkljRkFnTUJBQUdqSXpBaE1BNEdBMVVkRHdFQi93UUVBd0lDCnBEQVBCZ05WSFJNQkFmOEVCVEFEQVFIL01BMEdDU3FHU0liM0RRRUJDd1VBQTRJQkFRQXc4VWQwOWRYR09kQkwKYnQ5M3JuYk41VzBkMkJUZk43OXJ6SVpadHlmMjREQVQ1SGc1QVV2Qmc4QjJNaFRqdU50VlN6dzN4bGV3TDk3aQo0Wk9JWGJPL3hZTWp3OXRibldDY2o0MlVKRnJUOEN2eVZEekExeS9VVE1lMFpyU3lOUGpWTlBaMmUrb3IvRFdkCmRtbmg0THZ6c3VNbVBPZk96NmUwZmljMzdtLzZjL3JZV0l6aTJYWWRwSllHa1J1eW51UlVUbDZpUjBOa0xXaGYKc25aNll6TnFXT2YvNi9xdHduNkhVckttYWN3L2Q3blBNSDRpQTFId1MyZFh6WkxVR3NVZkZIREFkU2FVSWR3cApsZDFUUjErQjVyMnhiUW95eVo4YnhuTk5Ud04rU2dFZGc5MjJKUFlaRkZFWldDL1NXOWVYcUZnTWM3RlQyMHh4ClE1NW1qdHRICi0tLS0tRU5EIENFUlRJRklDQVRFLS0tLS0K",
"service": {
"name": "oauthboss",
"namespace": "cloudbosses",
"path": "/oauthboss",
"port": 443
}
},
"failurePolicy": "Ignore",
"matchPolicy": "Equivalent",
"name": "oauthboss.cloudbosses.admission.online.shopify.com",
"namespaceSelector": {
"matchExpressions": [
{
"key": "control-plane",
"operator": "DoesNotExist"
}
]
},
"objectSelector": {},
"reinvocationPolicy": "Never",
"rules": [
{
"apiGroups": [
"core"
],
"apiVersions": [
"v1"
],
"operations": [
"CREATE",
"UPDATE"
],
"resources": [
"secrets"
],
"scope": "*"
}
],
"sideEffects": "Unknown",
"timeoutSeconds": 30
}
]
}
],
"kind": "List",
"metadata": {
"resourceVersion": "",
"selfLink": ""
}
}
29 changes: 26 additions & 3 deletions test/integration-serial/serial_deploy_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,12 +525,35 @@ def test_batch_dry_run_apply_failure_falls_back_to_individual_resource_dry_run_v
end

def test_batch_dry_run_apply_success_precludes_individual_resource_dry_run_validation
Krane::KubernetesResource.any_instance.expects(:validate_definition).with do |kwargs|
kwargs[:kubectl].nil? && !kwargs[:dry_run]
end
Krane::KubernetesResource.any_instance.expects(:validate_definition).times(0)
deploy_fixtures("hello-cloud", subset: %w(secret.yml))
end

def test_resources_with_side_effect_inducing_webhooks_are_not_batched_server_side_dry_run
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not seeing how this test verifies are_not_batched

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've fleshed out this test a bit more

resp = JSON.parse(File.read(File.join(fixture_path("for_serial_deploy_tests"), "ingress_hook.json")))["items"]
Krane::ClusterResourceDiscovery.any_instance.expects(:fetch_mutating_webhook_configurations).returns(resp)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not actually deploy this into the cluster?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't realize I could just set failurePolicy: ignore with a small timeout to circumvent the need for an actual webhook server. I've changed the tests to actually deploy a real MutatingWebhookConfiguration and use that to test things out.

Worth noting that the partitioning logic is only really necessary for v1beta1 resources, since v1 only allows (and requires) either None or NoneOnDryRun to be specified for the sideEffects field.

Krane::ResourceDeployer.any_instance.expects(:dry_run).with do |params|
params.length == 2 && (params.map(&:type) - ["Deployment", "Service"]).empty?
end
Krane::Ingress.any_instance.expects(:validate_definition)
result = deploy_fixtures('hello-cloud', subset: %w(web.yml.erb), render_erb: true)
end

def test_resources_with_side_effect_inducing_webhooks_with_transitive_dependency_does_not_fail_batch_running
resp = JSON.parse(File.read(File.join(fixture_path("for_serial_deploy_tests"), "secret_hook.json")))["items"]
Krane::ClusterResourceDiscovery.any_instance.expects(:fetch_mutating_webhook_configurations).returns(resp)
Krane::KubernetesResource.any_instance.expects(:validate_definition).times(1) # Only secret should call this
deploy_fixtures('hello-cloud', subset: %w(web.yml.erb secret.yml), render_erb: true) do |fixtures|
container = fixtures['web.yml.erb']['Deployment'][0]['spec']['template']['spec']
container['volumes'] = [{
'name' => 'secret',
'secret' => {
'secretName' => fixtures['secret.yml']["Secret"][0]['metadata']['name'],
},
}]
end
end

private

def rollout_conditions_annotation_key
Expand Down