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

Pass {Operation}Options to Webhooks #77563

Merged
merged 3 commits into from May 14, 2019

Conversation

@jpbetz
Copy link
Contributor

commented May 7, 2019

Implements the Pass {Operation}Option to Webhooks feature from the admission webhooks to GA KEP.

Special notes for your reviewer:

For readability, PR is split across three commits, one for the feature, another for the generated protobuf bindings, and another for test updates.

Modified our admission integration test to use a client with a custom username, and then ignore all admission webhook requests not originating from that client. This fixed a issue where controllers were sending requests that looked similar enough to the ones from the client that they were overwriting entries in the "holder" record map.

Note that we always send Options of the type that matches the "operation" field. E.g. if operation is "create", we send "CreateOptions".

Add `Option` field to the admission webhook `AdmissionReview` API that provides the operation options (e.g. `DeleteOption` or `CreateOption`) for the operation being performed.

/kind feature

@jpbetz

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

/assign @roycaihw @liggitt

Anyone else I should have review?

@k8s-ci-robot k8s-ci-robot requested review from brendandburns and deads2k May 7, 2019

@jpbetz jpbetz force-pushed the jpbetz:admission-webhook-options branch from c5152e9 to 7e3a088 May 8, 2019

@k8s-ci-robot k8s-ci-robot added size/XXL and removed size/XL labels May 8, 2019

@jpbetz jpbetz force-pushed the jpbetz:admission-webhook-options branch from 7e3a088 to 816bc62 May 8, 2019

@jpbetz jpbetz changed the title [WIP] Pass {Operation}Option to Webhooks Pass {Operation}Option to Webhooks May 8, 2019

@jpbetz jpbetz force-pushed the jpbetz:admission-webhook-options branch from 816bc62 to fe96fea May 8, 2019

@jpbetz

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

Rebased

@fejta-bot

This comment has been minimized.

Copy link

commented May 8, 2019

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@jpbetz jpbetz force-pushed the jpbetz:admission-webhook-options branch from 12277eb to 78e7582 May 14, 2019

@jpbetz jpbetz force-pushed the jpbetz:admission-webhook-options branch from 78e7582 to c71aeeb May 14, 2019

@liggitt

This comment has been minimized.

Copy link
Member

commented May 14, 2019

/retest

if c.resource.Namespaced {
ns = testNamespace
}
// 1st patch is a create

This comment has been minimized.

Copy link
@liggitt

liggitt May 14, 2019

Member

this isn't testing what you think:

  1. we test create before patch, so the stub object is already created (createOrGetResource would typically have just returned the existing resource here)
  2. even if the object did not already exist, the patch request is not sending enough information to create the object (no metadata.name, or any spec fields, etc), so the create would have failed
  3. most objects do not allow create on update for merge patches, so even if the object didn't exist and we updated the patch to have sufficient data, most objects would reject this patch request
  4. setting c.admissionHolder.expect is only half of the equation... you have to call c.admissionHolder.verify to actually verify your expectation was met. otherwise, the next expect call (on line 529) wipes out the first expectation (which is the only reason this is passing)

If we want to exercise create-on-update on all resources, the only method that supports that consistently is server-side-apply, so I'd probably defer adding that aspect of the test to that feature

This comment has been minimized.

Copy link
@jpbetz

jpbetz May 14, 2019

Author Contributor

So if I had added the verify I could have seen this fail. I'm starting to understand patch a bit better. Thanks for explaining @liggitt!

@liggitt

This comment has been minimized.

Copy link
Member

commented May 14, 2019

drop the create-on-patch test change, for the reasons listed in #77563 (comment), then lgtm

@jpbetz jpbetz force-pushed the jpbetz:admission-webhook-options branch from c71aeeb to 900d652 May 14, 2019

@jpbetz

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

drop the create-on-patch test change, for the reasons listed in #77563 (comment), then lgtm

Dropped it. Thanks for the review!

@liggitt

This comment has been minimized.

Copy link
Member

commented May 14, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label May 14, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpbetz, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@liggitt liggitt moved this from In progress to Completed, 1.15 in API Reviews May 14, 2019

@jpbetz

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

/priority important-soon

@jpbetz

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit 0b6ad8b into kubernetes:master May 14, 2019

17 of 20 checks passed

pull-kubernetes-bazel-test Job triggered.
Details
pull-kubernetes-verify Job triggered.
Details
tide Not mergeable. Jobs pull-kubernetes-bazel-test, pull-kubernetes-verify have not succeeded.
Details
cla/linuxfoundation jpbetz authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-publishing-bot-validate Skipped.
liggitt added a commit to liggitt/kubernetes that referenced this pull request May 25, 2019
TMP: do not merge - update AdmissionReview and v1.Status roundtrip fi…
…xtures

FAIL: TestRoundTripExternalTypes/.v1.Status (0.02s)
--- FAIL: TestRoundTripExternalTypes/.v1.Status/fixtures-v1.14.0 (0.02s)
    fixtures.go:267: proto data differed from expected output
    fixtures.go:271: Updated data in testdata/v1.14.0/core.v1.Status.pb.roundtripped.pb. Verify, commit, and rerun tests.
    fixtures.go:287: expected output vs actual output:
          strings.Join({
          	... // 7 identical lines
          	`    2: "11154013587666973726"`,
          	`    3: ""`,
        + 	"    4: 0",
          	"  }",
          	`  2: "2"`,
          	... // 19 identical lines
          }, "\n")
FAIL: TestRoundTripExternalTypes/admission.k8s.io.v1beta1.AdmissionReview (0.04s)

Caused by http://issue.k8s.io/78318

--- FAIL: TestRoundTripExternalTypes/admission.k8s.io.v1beta1.AdmissionReview/fixtures-v1.14.0 (0.02s)
    fixtures.go:223: json data differed from expected output
    fixtures.go:227: Updated data in testdata/v1.14.0/admission.k8s.io.v1beta1.AdmissionReview.json.roundtripped.json. Verify, commit, and rerun tests.
    fixtures.go:234:   strings.Join({
          	... // 31 identical lines
          	`    "object": {"kind":"Status","apiVersion":"v1","metadata":{"selfLink":"ĒȐw*ċ冎ǣ","resourceVersion":"5137558822374847458"},"status":"16","message":"17","reason":"(D疻翋膗h各%!¿(MISSING)嘪v","details":{"name":"18","group":"19","kind":"20","uid":"ʙ","causes":[{"reason":"ŷʔťǷF幾í鿃攴Ųę","message":"21","field":"22"}],"retryAfterSeconds":127437182},"code":12210768},`,
          	`    "oldObject": {"kind":"APIGroup","apiVersion":"v1","name":"23","versions":[{"groupVersion":"24","version":"25"}],"preferredVersion":{"groupVersion":"26","version":"27"},"serverAddressByClientCIDRs":[{"clientCIDR":"28","serverAddress":"29"}]},`,
        - 	`    "dryRun": true`,
        + 	`    "dryRun": true,`,
        + 	`    "options": null`,
          	"  },",
          	`  "response": {`,
          	... // 32 identical lines
          }, "\n")
    fixtures.go:267: proto data differed from expected output
    fixtures.go:271: Updated data in testdata/v1.14.0/admission.k8s.io.v1beta1.AdmissionReview.pb.roundtripped.pb. Verify, commit, and rerun tests.
    fixtures.go:287: expected output vs actual output:
          strings.Join({
          	... // 37 identical lines
          	"    }",
          	"    11: 1",
        + 	`    12: ""`,
          	"  }",
          	"  2 {",
          	... // 5 identical lines
          	`        2: "10931978707228436810"`,
          	`        3: ""`,
        + 	"        4: 0",
          	"      }",
          	`      2: "30"`,
          	... // 30 identical lines
          }, "\n")

Caused by kubernetes#77563 (comment) (for tag 12 and null options) and http://issue.k8s.io/78318 (for tag 4)
liggitt added a commit to liggitt/kubernetes that referenced this pull request May 29, 2019
Update expected AdmissionReview
        --- FAIL: TestCompatibility/admission.k8s.io.v1beta1.AdmissionReview/v1.14.0 (0.02s)
            compatibility.go:460: json differs
            compatibility.go:461:   strings.Join({
                  	... // 31 identical lines
                  	`    "object": {"apiVersion":"example.com/v1","kind":"CustomType","spec":{"replicas":1},"status":{"available":1}},`,
                  	`    "oldObject": {"apiVersion":"example.com/v1","kind":"CustomType","spec":{"replicas":1},"status":{"available":1}},`,
                - 	`    "dryRun": true`,
                + 	`    "dryRun": true,`,
                + 	`    "options": null`,
                  	"  },",
                  	`  "response": {`,
                  	... // 33 identical lines
                  }, "\n")

            compatibility.go:466: yaml differs
            compatibility.go:467:   strings.Join({
                  	... // 23 identical lines
                  	"      available: 1",
                  	"  operation: 祈¡ıŵDz廔ȇ{sŊƏp饏姥呄鐊",
                + 	"  options: null",
                  	"  resource:",
                  	`    group: "5"`,
                  	... // 38 identical lines
                  }, "\n")

            compatibility.go:472: proto differs
            compatibility.go:474:   strings.Join({
                  	... // 37 identical lines
                  	"    }",
                  	"    11: 1",
                + 	`    12: ""`,
                + 	`    15: ""`,
                  	"  }",
                  	"  2 {",
                  	... // 36 identical lines
                  }, "\n")

Null `options` in json and additional proto tag 12 caused by kubernetes#77563 (comment)

Proto tag 15 caused by https://github.com/kubernetes/kubernetes/pull/78135/files#diff-3bc4acaf71b6b50648da046287017c54R82
liggitt added a commit to liggitt/kubernetes that referenced this pull request May 29, 2019
Update expected AdmissionReview
        --- FAIL: TestCompatibility/admission.k8s.io.v1beta1.AdmissionReview/v1.14.0 (0.02s)
            compatibility.go:460: json differs
            compatibility.go:461:   strings.Join({
                  	... // 31 identical lines
                  	`    "object": {"apiVersion":"example.com/v1","kind":"CustomType","spec":{"replicas":1},"status":{"available":1}},`,
                  	`    "oldObject": {"apiVersion":"example.com/v1","kind":"CustomType","spec":{"replicas":1},"status":{"available":1}},`,
                - 	`    "dryRun": true`,
                + 	`    "dryRun": true,`,
                + 	`    "options": null`,
                  	"  },",
                  	`  "response": {`,
                  	... // 33 identical lines
                  }, "\n")

            compatibility.go:466: yaml differs
            compatibility.go:467:   strings.Join({
                  	... // 23 identical lines
                  	"      available: 1",
                  	"  operation: 祈¡ıŵDz廔ȇ{sŊƏp饏姥呄鐊",
                + 	"  options: null",
                  	"  resource:",
                  	`    group: "5"`,
                  	... // 38 identical lines
                  }, "\n")

            compatibility.go:472: proto differs
            compatibility.go:474:   strings.Join({
                  	... // 37 identical lines
                  	"    }",
                  	"    11: 1",
                + 	`    12: ""`,
                + 	`    15: ""`,
                  	"  }",
                  	"  2 {",
                  	... // 36 identical lines
                  }, "\n")

Null `options` in json and additional proto tag 12 caused by kubernetes#77563 (comment)

Proto tag 15 caused by https://github.com/kubernetes/kubernetes/pull/78135/files#diff-3bc4acaf71b6b50648da046287017c54R82
liggitt added a commit to liggitt/kubernetes that referenced this pull request May 29, 2019
Update expected AdmissionReview
        --- FAIL: TestCompatibility/admission.k8s.io.v1beta1.AdmissionReview/v1.14.0 (0.02s)
            compatibility.go:460: json differs
            compatibility.go:461:   strings.Join({
                  	... // 31 identical lines
                  	`    "object": {"apiVersion":"example.com/v1","kind":"CustomType","spec":{"replicas":1},"status":{"available":1}},`,
                  	`    "oldObject": {"apiVersion":"example.com/v1","kind":"CustomType","spec":{"replicas":1},"status":{"available":1}},`,
                - 	`    "dryRun": true`,
                + 	`    "dryRun": true,`,
                + 	`    "options": null`,
                  	"  },",
                  	`  "response": {`,
                  	... // 33 identical lines
                  }, "\n")

            compatibility.go:466: yaml differs
            compatibility.go:467:   strings.Join({
                  	... // 23 identical lines
                  	"      available: 1",
                  	"  operation: 祈¡ıŵDz廔ȇ{sŊƏp饏姥呄鐊",
                + 	"  options: null",
                  	"  resource:",
                  	`    group: "5"`,
                  	... // 38 identical lines
                  }, "\n")

            compatibility.go:472: proto differs
            compatibility.go:474:   strings.Join({
                  	... // 37 identical lines
                  	"    }",
                  	"    11: 1",
                + 	`    12: ""`,
                + 	`    15: ""`,
                  	"  }",
                  	"  2 {",
                  	... // 36 identical lines
                  }, "\n")

Null `options` in json and additional proto tag 12 caused by kubernetes#77563 (comment)

Proto tag 15 caused by https://github.com/kubernetes/kubernetes/pull/78135/files#diff-3bc4acaf71b6b50648da046287017c54R82
liggitt added a commit to liggitt/kubernetes that referenced this pull request May 30, 2019
Update expected AdmissionReview
        --- FAIL: TestCompatibility/admission.k8s.io.v1beta1.AdmissionReview/v1.14.0 (0.02s)
            compatibility.go:460: json differs
            compatibility.go:461:   strings.Join({
                  	... // 31 identical lines
                  	`    "object": {"apiVersion":"example.com/v1","kind":"CustomType","spec":{"replicas":1},"status":{"available":1}},`,
                  	`    "oldObject": {"apiVersion":"example.com/v1","kind":"CustomType","spec":{"replicas":1},"status":{"available":1}},`,
                - 	`    "dryRun": true`,
                + 	`    "dryRun": true,`,
                + 	`    "options": null`,
                  	"  },",
                  	`  "response": {`,
                  	... // 33 identical lines
                  }, "\n")

            compatibility.go:466: yaml differs
            compatibility.go:467:   strings.Join({
                  	... // 23 identical lines
                  	"      available: 1",
                  	"  operation: 祈¡ıŵDz廔ȇ{sŊƏp饏姥呄鐊",
                + 	"  options: null",
                  	"  resource:",
                  	`    group: "5"`,
                  	... // 38 identical lines
                  }, "\n")

            compatibility.go:472: proto differs
            compatibility.go:474:   strings.Join({
                  	... // 37 identical lines
                  	"    }",
                  	"    11: 1",
                + 	`    12: ""`,
                + 	`    15: ""`,
                  	"  }",
                  	"  2 {",
                  	... // 36 identical lines
                  }, "\n")

Null `options` in json and additional proto tag 12 caused by kubernetes#77563 (comment)

Proto tag 15 caused by https://github.com/kubernetes/kubernetes/pull/78135/files#diff-3bc4acaf71b6b50648da046287017c54R82
liggitt added a commit to liggitt/kubernetes that referenced this pull request Jun 2, 2019
Update expected AdmissionReview
        --- FAIL: TestCompatibility/admission.k8s.io.v1beta1.AdmissionReview/v1.14.0 (0.02s)
            compatibility.go:460: json differs
            compatibility.go:461:   strings.Join({
                  	... // 31 identical lines
                  	`    "object": {"apiVersion":"example.com/v1","kind":"CustomType","spec":{"replicas":1},"status":{"available":1}},`,
                  	`    "oldObject": {"apiVersion":"example.com/v1","kind":"CustomType","spec":{"replicas":1},"status":{"available":1}},`,
                - 	`    "dryRun": true`,
                + 	`    "dryRun": true,`,
                + 	`    "options": null`,
                  	"  },",
                  	`  "response": {`,
                  	... // 33 identical lines
                  }, "\n")

            compatibility.go:466: yaml differs
            compatibility.go:467:   strings.Join({
                  	... // 23 identical lines
                  	"      available: 1",
                  	"  operation: 祈¡ıŵDz廔ȇ{sŊƏp饏姥呄鐊",
                + 	"  options: null",
                  	"  resource:",
                  	`    group: "5"`,
                  	... // 38 identical lines
                  }, "\n")

            compatibility.go:472: proto differs
            compatibility.go:474:   strings.Join({
                  	... // 37 identical lines
                  	"    }",
                  	"    11: 1",
                + 	`    12: ""`,
                + 	`    15: ""`,
                  	"  }",
                  	"  2 {",
                  	... // 36 identical lines
                  }, "\n")

Null `options` in json and additional proto tag 12 caused by kubernetes#77563 (comment)

Proto tag 15 caused by https://github.com/kubernetes/kubernetes/pull/78135/files#diff-3bc4acaf71b6b50648da046287017c54R82
k8s-publishing-bot added a commit to kubernetes/api that referenced this pull request Jun 2, 2019
Update expected AdmissionReview
        --- FAIL: TestCompatibility/admission.k8s.io.v1beta1.AdmissionReview/v1.14.0 (0.02s)
            compatibility.go:460: json differs
            compatibility.go:461:   strings.Join({
                  	... // 31 identical lines
                  	`    "object": {"apiVersion":"example.com/v1","kind":"CustomType","spec":{"replicas":1},"status":{"available":1}},`,
                  	`    "oldObject": {"apiVersion":"example.com/v1","kind":"CustomType","spec":{"replicas":1},"status":{"available":1}},`,
                - 	`    "dryRun": true`,
                + 	`    "dryRun": true,`,
                + 	`    "options": null`,
                  	"  },",
                  	`  "response": {`,
                  	... // 33 identical lines
                  }, "\n")

            compatibility.go:466: yaml differs
            compatibility.go:467:   strings.Join({
                  	... // 23 identical lines
                  	"      available: 1",
                  	"  operation: 祈¡ıŵDz廔ȇ{sŊƏp饏姥呄鐊",
                + 	"  options: null",
                  	"  resource:",
                  	`    group: "5"`,
                  	... // 38 identical lines
                  }, "\n")

            compatibility.go:472: proto differs
            compatibility.go:474:   strings.Join({
                  	... // 37 identical lines
                  	"    }",
                  	"    11: 1",
                + 	`    12: ""`,
                + 	`    15: ""`,
                  	"  }",
                  	"  2 {",
                  	... // 36 identical lines
                  }, "\n")

Null `options` in json and additional proto tag 12 caused by kubernetes/kubernetes#77563 (comment)

Proto tag 15 caused by https://github.com/kubernetes/kubernetes/pull/78135/files#diff-3bc4acaf71b6b50648da046287017c54R82

Kubernetes-commit: fcea91c1650b23ec07a96b5a62d08466a3771f29
k8s-publishing-bot added a commit to kubernetes/api that referenced this pull request Jun 4, 2019
Update expected AdmissionReview
        --- FAIL: TestCompatibility/admission.k8s.io.v1beta1.AdmissionReview/v1.14.0 (0.02s)
            compatibility.go:460: json differs
            compatibility.go:461:   strings.Join({
                  	... // 31 identical lines
                  	`    "object": {"apiVersion":"example.com/v1","kind":"CustomType","spec":{"replicas":1},"status":{"available":1}},`,
                  	`    "oldObject": {"apiVersion":"example.com/v1","kind":"CustomType","spec":{"replicas":1},"status":{"available":1}},`,
                - 	`    "dryRun": true`,
                + 	`    "dryRun": true,`,
                + 	`    "options": null`,
                  	"  },",
                  	`  "response": {`,
                  	... // 33 identical lines
                  }, "\n")

            compatibility.go:466: yaml differs
            compatibility.go:467:   strings.Join({
                  	... // 23 identical lines
                  	"      available: 1",
                  	"  operation: 祈¡ıŵDz廔ȇ{sŊƏp饏姥呄鐊",
                + 	"  options: null",
                  	"  resource:",
                  	`    group: "5"`,
                  	... // 38 identical lines
                  }, "\n")

            compatibility.go:472: proto differs
            compatibility.go:474:   strings.Join({
                  	... // 37 identical lines
                  	"    }",
                  	"    11: 1",
                + 	`    12: ""`,
                + 	`    15: ""`,
                  	"  }",
                  	"  2 {",
                  	... // 36 identical lines
                  }, "\n")

Null `options` in json and additional proto tag 12 caused by kubernetes/kubernetes#77563 (comment)

Proto tag 15 caused by https://github.com/kubernetes/kubernetes/pull/78135/files#diff-3bc4acaf71b6b50648da046287017c54R82

Kubernetes-commit: fcea91c1650b23ec07a96b5a62d08466a3771f29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.