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

Bug 1809274: crd/kubelet: do not prune kubelet rawExtension fields #1524

Merged
merged 1 commit into from Mar 4, 2020

Conversation

yuqi-zhang
Copy link
Contributor

The kubelet config is now validated, but fields are being silently
dropped since its an "unknown field" (rawExtension), which by
default is pruned by the validator.

Signed-off-by: Yu Qi Zhang jerzhang@redhat.com

@openshift-ci-robot
Copy link
Contributor

@yuqi-zhang: This pull request references Bugzilla bug 1809274, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.5.0) matches configured target release for branch (4.5.0)
  • bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1809274: crd/kubelet: do not prune kubelet rawExtension fields

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 2, 2020
@yuqi-zhang
Copy link
Contributor Author

/cherry-pick release-4.4

@openshift-cherrypick-robot

@yuqi-zhang: once the present PR merges, I will cherry-pick it on top of release-4.4 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@yuqi-zhang
Copy link
Contributor Author

Did some manual validation and can confirm that with the change, the object is no longer being dropped.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 2, 2020
@kikisdeliveryservice
Copy link
Contributor

Need a make update

seems sane to me

CC: @rphillips

@ashcrow
Copy link
Member

ashcrow commented Mar 2, 2020

diff -Naupr hack/../_tmp/pkg/operator/assets/bindata.go hack/../pkg/operator/assets/bindata.go
--- hack/../_tmp/pkg/operator/assets/bindata.go	2020-03-02 21:03:36.448612384 +0000
+++ hack/../pkg/operator/assets/bindata.go	2020-03-02 21:03:37.176676283 +0000
@@ -1155,6 +1155,8 @@ spec:
           properties:
             kubeletConfig:
               type: object
+              x-kubernetes-embedded-resource: true
+              x-kubernetes-preserve-unknown-fields: true
             machineConfigPoolSelector:
               description: A label selector is a label query over a set of resources.
                 The result of matchLabels and matchExpressions are ANDed. An empty
hack/../pkg is out of date. Please run make update

@kikisdeliveryservice
Copy link
Contributor

@yuqi-zhang does this mean we also need to update the container runtime cfgs too?

@yuqi-zhang
Copy link
Contributor Author

yuqi-zhang commented Mar 2, 2020

Need a make update

Done

does this mean we also need to update the container runtime cfgs too?

I don't believe so. That has a defined set of fields. KubeletConfig was the only rawExtension field in our API

@yuqi-zhang
Copy link
Contributor Author

That said by default I don't think we generate a containerruntimeconfig so I wouldn't know what to expect for that one specifically

@yuqi-zhang
Copy link
Contributor Author

I did some testing and can confirm it does validation on valid containerruntimeconfig fields

@kikisdeliveryservice
Copy link
Contributor

hmm this is weird for TestKernelType:
mcd_test.go:278: Node ci-op-269qf-w-d-hkp2x.c.openshift-gce-devel-ci.internal did not rollback successfully

/test e2e-gcp-op

@runcom
Copy link
Member

runcom commented Mar 3, 2020

/approve

@ashcrow
Copy link
Member

ashcrow commented Mar 3, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2020
@ashcrow
Copy link
Member

ashcrow commented Mar 3, 2020

/hold

@kikisdeliveryservice noted she had a question

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 3, 2020
@kikisdeliveryservice
Copy link
Contributor

@yuqi-zhang is going to update commit message

/hold

The kubelet config is now validated, but fields are being silently
dropped since its an "unknown field" (rawExtension), which by
default is pruned by the validator.

Some more context, the 2 fields that are being added are from kube
specifications, that tell the validator to skip validation as well
as mark it as an embedded (rawExtention) object which cannot be
directly validated. See: https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/#rawextension

Signed-off-by: Yu Qi Zhang <jerzhang@redhat.com>
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2020
@yuqi-zhang
Copy link
Contributor Author

Updated comment for more context

@ashcrow
Copy link
Member

ashcrow commented Mar 3, 2020

Content didn't change

/lgtm

Will defer to @kikisdeliveryservice to remove hold

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2020
@kikisdeliveryservice
Copy link
Contributor

thank you, @yuqi-zhang !

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 3, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashcrow, runcom, yuqi-zhang

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:
  • OWNERS [ashcrow,runcom,yuqi-zhang]

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

@ashcrow
Copy link
Member

ashcrow commented Mar 3, 2020

/test unit

@yuqi-zhang
Copy link
Contributor Author

/retest

1 similar comment
@ashcrow
Copy link
Member

ashcrow commented Mar 3, 2020

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@runcom
Copy link
Member

runcom commented Mar 4, 2020

uhm

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@ashcrow
Copy link
Member

ashcrow commented Mar 4, 2020

/test e2e-gcp-upgrade

@yuqi-zhang
Copy link
Contributor Author

/retest

@rphillips
Copy link
Contributor

lgtm, thanks!

@openshift-merge-robot openshift-merge-robot merged commit 71db579 into openshift:master Mar 4, 2020
@openshift-ci-robot
Copy link
Contributor

@yuqi-zhang: All pull requests linked via external trackers have merged. Bugzilla bug 1809274 has been moved to the MODIFIED state.

In response to this:

Bug 1809274: crd/kubelet: do not prune kubelet rawExtension fields

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-cherrypick-robot

@yuqi-zhang: new pull request created: #1531

In response to this:

/cherry-pick release-4.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

kikisdeliveryservice added a commit to kikisdeliveryservice/machine-config-operator that referenced this pull request Mar 6, 2020
 PR openshift#1524 introduced kubeletconfig as an embedded resource:
 x-kubernetes-embedded-resource: true

 This validates our config against the upstream config which differs in the placement of api version and kind. The result
 is that our kubeletconfigs are no longer valid and are unable to be applied. This PR removes the check and unbreaks our
 kubeletconfigs.
kikisdeliveryservice added a commit to kikisdeliveryservice/machine-config-operator that referenced this pull request Mar 6, 2020
 PR openshift#1524 introduced kubeletconfig as an embedded resource:
 x-kubernetes-embedded-resource: true

 This validates our config against the upstream config which differs in the placement of api version and kind. The result
 is that our kubeletconfigs are no longer valid and are unable to be applied. This PR removes the check and unbreaks our
 kubeletconfigs.
@oshoval
Copy link

oshoval commented Mar 8, 2020

Hi,
how to know please when its part of the official OpenShift ?

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. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants