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

apiextensions: ignore ObjectMeta from webhook converted objects other than labels and annotations #77743

Merged

Conversation

sttts
Copy link
Contributor

@sttts sttts commented May 10, 2019

Fixes #72160

Specified in kubernetes/enhancements#1063.

TODOs:

  • unit tests
  • webhook integration test
In CRD webhook conversion ignore changes to metadata other than for labels and annotations.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 10, 2019
@sttts
Copy link
Contributor Author

sttts commented May 10, 2019

/assign @liggitt @jpbetz

@k8s-ci-robot k8s-ci-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 10, 2019
@sttts sttts added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label May 10, 2019
@k8s-ci-robot k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label May 10, 2019
@sttts sttts added this to the v1.15 milestone May 10, 2019
@fejta-bot
Copy link

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.

Copy link
Contributor

@jpbetz jpbetz left a comment

Choose a reason for hiding this comment

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

As a FYI, As part of #77756 I'll be adding integration tests for converters that do invalid things (return various illegal responses). I've taken notes to include metadata label and annotations test cases.

@liggitt
Copy link
Member

liggitt commented May 14, 2019

a few comments, nothing major

@sttts sttts force-pushed the sttts-crd-webhook-conversion-metadata branch from 210a9c3 to 8c79150 Compare May 14, 2019 15:10
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 14, 2019
@sttts sttts force-pushed the sttts-crd-webhook-conversion-metadata branch 4 times, most recently from f7bf465 to 6cef4d4 Compare May 25, 2019 21:38
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 25, 2019
@sttts sttts force-pushed the sttts-crd-webhook-conversion-metadata branch 2 times, most recently from 4caa3b4 to 2f40b7d Compare May 25, 2019 22:30
@sttts sttts force-pushed the sttts-crd-webhook-conversion-metadata branch from 2f40b7d to 4f6d755 Compare May 28, 2019 08:21
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 28, 2019
@sttts
Copy link
Contributor Author

sttts commented May 28, 2019

Rebased

@liggitt @jpbetz please take a look.

@sttts sttts force-pushed the sttts-crd-webhook-conversion-metadata branch from 4f6d755 to 9814914 Compare May 28, 2019 08:22
@sttts
Copy link
Contributor Author

sttts commented May 28, 2019

/retest

empty verify log

@liggitt
Copy link
Member

liggitt commented May 28, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 28, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 28, 2019
@jpbetz
Copy link
Contributor

jpbetz commented May 28, 2019

/lgtm

@liggitt
Copy link
Member

liggitt commented May 28, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit 358bfe3 into kubernetes:master May 29, 2019
stringMap[k] = v.(string)
}
var errs field.ErrorList
if fld == "labels" {
Copy link
Contributor

@tedyu tedyu May 29, 2019

Choose a reason for hiding this comment

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

It seems these 5 lines can be rewritten as:

diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/webhook_converter.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/webhook_converter.go
index 8cc7e78e22..121654846d 100644
--- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/webhook_converter.go
+++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/webhook_converter.go
@@ -317,7 +317,11 @@ func restoreObjectMeta(original, converted *unstructured.Unstructured) error {
                return fmt.Errorf("invalid metadata of type %T in input object", obj)
        }

-       for _, fld := range []string{"labels", "annotations"} {
+       type validatorType = func(map[string]string, *field.Path) field.ErrorList
+       fldToValidator := make(map[string]validatorType)
+       fldToValidator["labels"] = metav1validation.ValidateLabels
+       fldToValidator["annotations"] = apivalidation.ValidateAnnotations
+       for fld, validator := range fldToValidator {
                obj, found := responseMetaData[fld]
                if !found || obj == nil {
                        delete(convertedMetaData, fld)
@@ -349,11 +353,7 @@ func restoreObjectMeta(original, converted *unstructured.Unstructured) error {
                                stringMap[k] = v.(string)
                        }
                        var errs field.ErrorList
-                       if fld == "labels" {
-                               errs = metav1validation.ValidateLabels(stringMap, field.NewPath("metadata", "labels"))
-                       } else {
-                               errs = apivalidation.ValidateAnnotations(stringMap, field.NewPath("metadata", "annotation"))
-                       }
+                       errs = validator(stringMap, field.NewPath("metadata", "labels"))
                        if len(errs) > 0 {
                                return errs.ToAggregate()
                        }

The above is expandable: it is trivial to add more supported fields.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/custom-resources area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CRD webhook conversion can change metadata.generation
6 participants