kubectl: Verify dry run support #69449

apelisse · 2018-10-05T00:16:05Z

We want to verify if the resource supports dry-run before we send dry-run queries to it, because old servers are just ignoring the query-param and will persist the objects. Not good.

Even though the heuristic isn't perfect yet, I think it's obviously better than what we have today.

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

NONE

k8s-ci-robot · 2018-10-05T00:16:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse

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:

~~pkg/kubectl/OWNERS~~ [apelisse]

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

apelisse · 2018-10-05T18:41:33Z

/kind bug

seans3 · 2018-10-05T18:51:03Z

pkg/kubectl/cmd/apply.go

@@ -684,6 +693,22 @@ type patcher struct {
 }

 func (p *patcher) patchSimple(obj runtime.Object, modified []byte, source, namespace, name string, errOut io.Writer) ([]byte, runtime.Object, error) {
+	if p.serverDryRun {
+		doc, err := p.discoveryClient.OpenAPISchema()


patchSimple is already a large function. Should we break this check into it's own patcher method? Additionally, can we add significant comments to describe why this is necessary (what bug this fixes).

liggitt · 2018-10-05T19:40:35Z

pkg/kubectl/cmd/util/openapi/dryrun.go

+
+func checkExtension(extensions []*openapi_v2.NamedAny, gvk schema.GroupVersionKind) bool {
+	for _, extension := range extensions {
+		if extension.GetValue().GetYaml() == "" ||


I'm not familiar with this method of getting data out of the openapi doc... it's yaml-encoded?

cc @mbohlool for review

Since vendor extensions can be any type of yaml data in the openapi spec, this framework is just keeping it as bytes that can be decoded into whatever needed.

Antonie used gnostic openapi structs more than me, if this works, I am fine with it. If I want to be on the safe side, I will only check the name and if the value is empty, log error or fail or something because that is not usual.

mbohlool · 2018-10-05T22:55:09Z

pkg/kubectl/cmd/apply.go

+// supports dryRun. Sending dryRun requests to apiserver that don't
+// support it will result in objects being unwillingly persisted.
+//
+// If the GVK can not be found (it can be a CRD or a resource coming


I won't do this assumption specially for aggregated server. I would say it safer to say it does not support dry-run than assuming it does.

Agreed. I guess my argument is that this piece of code is safer than what we have today (nothing is checked at all).

mbohlool · 2018-10-05T22:58:19Z

pkg/kubectl/cmd/util/openapi/dryrun.go

+	"k8s.io/apimachinery/pkg/runtime/schema"
+)
+
+func checkExtension(extensions []*openapi_v2.NamedAny, gvk schema.GroupVersionKind) bool {


I would use a better name here.e.g., hasGVKExtension?

Good suggestion, thank you!

apelisse · 2018-10-09T03:35:58Z

I've been caught by the gofmt change! :-) Fixed all the things.

seans3 · 2018-10-09T19:58:45Z

/retest
/lgtm

fejta-bot · 2018-10-10T00:08:00Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.