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

timothysmith0609 · 2021-02-02T18:33:39Z

This PR adds some smarts to the batch dry-runner. In particular, it cross-references the supplied list of resources with those that have mutating admission webhooks which are not dry-run safe. Resources that we know will fail batch dry-running are thus pre-emptively added to the legacy dry-run flow.

timothysmith0609 · 2021-02-02T21:03:55Z

lib/krane/deploy_task.rb

+            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


This is a straightforward implementation of the rules defined here.

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.

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.

timothysmith0609 · 2021-02-02T21:13:02Z

test/integration-serial/serial_deploy_test.rb

+    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']['containers'][0]
+      container['volumes'] = [
+        name: 'secret',
+        secret: {
+          secretName: fixtures['secret.yml']["Secret"][0]['metadata']['name'],
+        },
+      ]


This does not fail the dry-run, as I would have thought, even though the Secret resource is not part of the dry-run apply (and is not present on the cluster beforehand). I will try a few more failure scenarios to see if I can actually induce this type of transitive failure

timothysmith0609 · 2021-02-02T21:14:47Z

lib/krane/deploy_task.rb

-        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


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

dturn

Some small thoughts but looking like its on the right direction.

lib/krane/kubernetes_resource.rb

dturn · 2021-02-02T23:41:10Z

lib/krane/deploy_task.rb

+      individuals = []
+      mutating_webhooks = cluster_resource_discoverer.fetch_mutating_webhook_configurations
+      mutating_webhooks.each do |spec|
+        spec.dig('webhooks').each do |webhook|


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

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.

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

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.

dturn · 2021-02-02T23:50:17Z

lib/krane/deploy_task.rb

+            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


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.

dturn · 2021-02-02T23:53:23Z

lib/krane/deploy_task.rb

+                    (r.version == version || version == '*' || match_policy == "Equivalent") &&
+                    (r.type.downcase == kind.downcase)
+                  end
+                  resources -= individuals


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

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.

Co-authored-by: Danny Turner <[email protected]>

…://github.com/Shopify/krane into server_dry_run_mutating_webhooks_side_effects

dturn

Some questions about the tests, code looks good

lib/krane/kubernetes_resource/mutating_webhook_configuration.rb

test/integration-serial/serial_deploy_test.rb

dturn · 2021-02-05T20:57:30Z

test/integration-serial/serial_deploy_test.rb

+
+  def test_resources_with_side_effect_inducing_webhooks_are_not_batched_server_side_dry_run
+    resp = mutating_webhook_fixture(File.join(fixture_path("mutating_webhook_configurations"), "ingress_hook.json"))
+    Krane::ClusterResourceDiscovery.any_instance.expects(:fetch_mutating_webhook_configurations).returns(resp)


Why not actually deploy this into the cluster?

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.

dturn · 2021-02-05T20:59:01Z

test/integration-serial/serial_deploy_test.rb

+    ], in_order: true)
+  end
+
+  def test_resources_with_side_effect_inducing_webhooks_are_not_batched_server_side_dry_run


I'm not seeing how this test verifies are_not_batched

I've fleshed out this test a bit more

test/integration-serial/serial_deploy_test.rb

dturn

👍

karanthukral

sorry for the delayed review

timothysmith0609 added 3 commits February 2, 2021 13:33

initial attempt

b41b31c

more work

f09f26a

refinement

911dc8a

timothysmith0609 commented Feb 2, 2021

View reviewed changes

test fix

3713af8

timothysmith0609 commented Feb 2, 2021

View reviewed changes

readd comment

a63e5d2

timothysmith0609 commented Feb 2, 2021

View reviewed changes

timothysmith0609 added 3 commits February 2, 2021 16:16

cleanup

7062e27

more fix

a9b9bc4

grr

f120b8e

timothysmith0609 marked this pull request as ready for review February 2, 2021 21:18

timothysmith0609 requested a review from a team as a code owner February 2, 2021 21:18

timothysmith0609 requested review from karanthukral and dturn February 2, 2021 21:20

dturn reviewed Feb 2, 2021

View reviewed changes

timothysmith0609 and others added 12 commits February 3, 2021 10:35

Update lib/krane/kubernetes_resource.rb

eb50b0e

Co-authored-by: Danny Turner <[email protected]>

dup resources to avoid mutating issues

a79eb66

fix test

78d2b1d

Merge branch 'server_dry_run_mutating_webhooks_side_effects' of https…

25b7727

…://github.com/Shopify/krane into server_dry_run_mutating_webhooks_side_effects

empty commit

3c2e91e

object-oriented MutatingWebhookConfiguration"

893d6c3

still need to validate all definitions, but not necessarily dry run

4d9c65c

Fix test

53dbcf3

MutatingWebhookConfiguration unit test

8ed6125

test fix

cb38c18

File organization + test for EXACT matching

df9dc68

map domain name objects more closely + newline

d0adc58

dturn reviewed Feb 5, 2021

View reviewed changes

resources field may also be wildcard

d464370

timothysmith0609 added 6 commits February 8, 2021 09:46

typo

f787151

Use actual webhookconfiguration in-cluster for tests + fixes

deb2738

cop

2225cea

more test fix

86370b2

comment

7e20830

cleanup fixtures

dd15453

dturn approved these changes Feb 9, 2021

View reviewed changes

changelog

c0677d2

karanthukral approved these changes Feb 9, 2021

View reviewed changes

timothysmith0609 merged commit 5f745df into master Feb 9, 2021

timothysmith0609 deleted the server_dry_run_mutating_webhooks_side_effects branch February 9, 2021 20:21

timothysmith0609 mentioned this pull request Feb 10, 2021

aggregate individuals, don't mistakenly replace #801

Merged

jpfourny mentioned this pull request Jan 11, 2022

update tests for K8s 1.22+ and remove support for K8s < 1.19 #862

Merged

d1egoaz mentioned this pull request Sep 23, 2022

Remove unsupported Kubernetes and Ruby versions from Krane and CI #905

Merged

d1egoaz added a commit that referenced this pull request Sep 28, 2022

Revert #798

9f1fe2a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

timothysmith0609 commented Feb 2, 2021 •

edited

Loading

timothysmith0609 Feb 2, 2021

dturn Feb 2, 2021

timothysmith0609 Feb 3, 2021 •

edited

Loading

timothysmith0609 Feb 2, 2021

timothysmith0609 Feb 2, 2021

dturn left a comment

dturn Feb 2, 2021

timothysmith0609 Feb 3, 2021

dturn Feb 3, 2021

timothysmith0609 Feb 3, 2021

dturn Feb 2, 2021

dturn Feb 2, 2021

timothysmith0609 Feb 3, 2021

dturn left a comment

dturn Feb 5, 2021

timothysmith0609 Feb 8, 2021

dturn Feb 5, 2021

timothysmith0609 Feb 8, 2021

dturn left a comment

karanthukral left a comment

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

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

Conversation

timothysmith0609 commented Feb 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothysmith0609 Feb 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dturn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dturn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dturn left a comment

Choose a reason for hiding this comment

karanthukral left a comment

Choose a reason for hiding this comment

timothysmith0609 commented Feb 2, 2021 •

edited

Loading

timothysmith0609 Feb 3, 2021 •

edited

Loading