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.

Sign up for GitHub

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

Jump to bottom

Fix providerID update validation #51761

Merged
merged 1 commit into from
Sep 3, 2017
Merged

Fix providerID update validation #51761

merged 1 commit into from
Sep 3, 2017
+37 −0

Conversation

karataliu
Copy link
Contributor

What this PR does / why we need it:
Cloud controller manager supports updating providerID in #50730, but the node updating was blocked by
validation rule.

This is to propose a fix for updating the validation rule by allowing altering spec.providerID if not set.

Please check #51596 for detail

Which issue this PR fixes
fixes #51596

Special notes for your reviewer:

Release note:

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 1, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @karataliu. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 1, 2017
@k8s-github-robot k8s-github-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Sep 1, 2017
@karataliu karataliu mentioned this pull request Sep 1, 2017
Closed
andrewsykim
andrewsykim reviewed Sep 1, 2017
View reviewed changes
pkg/api/validation/validation.go
@@ -3437,6 +3437,16 @@ func ValidateNodeUpdate(node, oldNode *api.Node) field.ErrorList {
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "podCIDR"), "node updates may not change podCIDR except from \"\" to valid"))
}
}

// Allow controller manager updating provider ID when not set
if len(oldNode.Spec.ProviderID) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

if oldNode.Spec.ProviderID == ""

@luxas luxas added this to the v1.8 milestone Sep 1, 2017
@luxas
Copy link
Member

luxas commented Sep 1, 2017

/ok-to-test
/release-note
/sig cluster-lifecycle
/area cloudprovider

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. area/cloudprovider and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 1, 2017
@luxas
Copy link
Member

luxas commented Sep 1, 2017

oops, mis-clicked on "Clear assignees" for a moment; added back bot-assignees as well
PTAL @liggitt @thockin @wlan0 @cheftako as well

cheftako
cheftako reviewed Sep 1, 2017
View reviewed changes
pkg/api/validation/validation.go
oldNode.Spec.ProviderID = node.Spec.ProviderID
} else {
if oldNode.Spec.ProviderID != node.Spec.ProviderID {
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "providerID"), "node updates may not change providerID except from \"\" to valid"))
Copy link
Member

Choose a reason for hiding this comment

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

Can we add the new ProviderID to the error to make this easier to debug?

Copy link
Member

Choose a reason for hiding this comment

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

We don't usually do that for Forbidden errors because ... I forget why. If we want to do that, we should add a field to the error struct and do it all over.

cheftako
cheftako reviewed Sep 1, 2017
View reviewed changes
Copy link
Member

@cheftako cheftako left a comment

Choose a reason for hiding this comment

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

Nice version of the change.

@thockin
Copy link
Member

thockin commented Sep 1, 2017

/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 Sep 1, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: karataliu, thockin

Associated issue: 50730

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 1, 2017
@luxas
Copy link
Member

luxas commented Sep 1, 2017

/retest

@wlan0 wlan0 mentioned this pull request Sep 3, 2017
Closed
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 51819, 51706, 51761, 51818, 51500)

@k8s-github-robot k8s-github-robot merged commit 3c621d6 into kubernetes:master Sep 3, 2017
@karataliu karataliu deleted the ccmupdatenode branch September 4, 2017 01:57
@prydie prydie mentioned this pull request Sep 5, 2017
Merged
@wojtek-t wojtek-t modified the milestones: v1.7, v1.8 Sep 6, 2017
@wojtek-t wojtek-t added cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cherrypick-candidate labels Sep 6, 2017
k8s-github-robot pushed a commit that referenced this pull request Sep 7, 2017
Merge pull request #51954 from prydie/automated-cherry-pick-of-#51761…
f032a8f 
…-upstream-release-1.7

Automatic merge from submit-queue

Automated cherry pick of #51761

Cherry pick of #51761 on release-1.7.

#51761: Fix providerID update validation
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.7" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

@karataliu karataliu mentioned this pull request Sep 15, 2017
Closed
25 tasks
@Arvinderpal Arvinderpal mentioned this pull request Apr 9, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cheftako cheftako cheftako left review comments

@thockin thockin thockin left review comments

@andrewsykim andrewsykim andrewsykim left review comments

Assignees

@pmorie pmorie

@cheftako cheftako

@wlan0 wlan0

@thockin thockin

@derekwaynecarr derekwaynecarr

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cloudprovider cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.7
Development

Successfully merging this pull request may close these issues.

Cloud Controller Manager updating ProviderID blocked by validation rule