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

Cloud Controller Manager updating ProviderID blocked by validation rule #51596

Closed
karataliu opened this issue Aug 30, 2017 · 9 comments · Fixed by #51761
Closed

Cloud Controller Manager updating ProviderID blocked by validation rule #51596

karataliu opened this issue Aug 30, 2017 · 9 comments · Fixed by #51761
Labels
area/cloudprovider kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle.
Milestone
v1.8

Comments

@karataliu
Copy link
Contributor

Is this a BUG REPORT or FEATURE REQUEST?:
/kind bug

What happened:
Follow up for #49836 , #50730

With the change for cloud-controller-manager updating providerID, the node.spec.ProviderID is to be updated. But it will fail later in node update call:

E0830 05:54:08.576076   21643 node_controller.go:374] Node "k8s-agentpool1" is invalid: []: Forbidden: node updates may only change labels, taints, or capacity (or configSource, if the DynamicKubeletConfig feature gate is enabled)

Should also check validation rule:

kubernetes/pkg/api/validation/validation.go

Line 3432 in 12d73c3

if len(oldNode.Spec.PodCIDR) == 0 {

By adding following line will do the trick.

oldNode.Spec.ProviderID = node.Spec.ProviderID

But this will also allow users to manually edit node's providerID

Shall we enforce that only cloud-controller-manager can update the providerID?

What you expected to happen:

How to reproduce it (as minimally and precisely as possible):

Anything else we need to know?:
@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Aug 30, 2017
@k8s-github-robot
Copy link

@karataliu
There are no sig labels on this issue. Please add a sig label by:

  1. mentioning a sig: @kubernetes/sig-<group-name>-<group-suffix>
    e.g., @kubernetes/sig-contributor-experience-<group-suffix> to notify the contributor experience sig, OR

  2. specifying the label manually: /sig <label>
    e.g., /sig scalability to apply the sig/scalability label

Note: Method 1 will trigger an email to the group. You can find the group list here and label list here.
The <group-suffix> in the method 1 has to be replaced with one of these: bugs, feature-requests, pr-reviews, test-failures, proposals

@k8s-github-robot k8s-github-robot added the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Aug 30, 2017
@karataliu
Copy link
Contributor Author

/sig cluster-lifecycle

@k8s-ci-robot k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Aug 30, 2017
@k8s-github-robot k8s-github-robot removed the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Aug 30, 2017
@karataliu
Copy link
Contributor Author

cc @andrewsykim @luxas

@andrewsykim
Copy link
Member

/area cloudprovider

@andrewsykim
Copy link
Member

@karataliu being able to update node.Spec.ProviderID seems pretty reasonable to me. Can you get a PR in for that since we're close to code freeze?

@andrewsykim
Copy link
Member

cc @jhorwit2 @prydie @wlan0

@karataliu karataliu mentioned this issue Sep 1, 2017
Merged
@karataliu
Copy link
Contributor Author

@andrewsykim I found this issue when validating against a cluster running azure cloud provider, could you please also check whether it is the same case from you side?

providerID serves somewhat like an identity for nodes, thus it wouldn't expect arbitrarily updating. A compromise proposal is to allow updates when the value is not set. This gives controller manager a chance to set it during node register.

Created #51761 as a fix proposal.

@andrewsykim
Copy link
Member

@karataliu thanks for the patch, I think I'll have some time this weekend to verify this fix!

@luxas luxas added this to the v1.8 milestone Sep 1, 2017
@luxas luxas added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Sep 1, 2017
@k8s-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Labels Complete

@karataliu

Issue label settings:

  • sig/cluster-lifecycle: Issue will be escalated to these SIGs if needed.
  • priority/important-soon: Escalate to the issue owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
  • kind/bug: Fixes a bug discovered during the current release.
Additional instructions available here

k8s-github-robot pushed a commit that referenced this issue Sep 3, 2017
Merge pull request #51761 from karataliu/ccmupdatenode
3c621d6 
Automatic merge from submit-queue (batch tested with PRs 51819, 51706, 51761, 51818, 51500)

Fix providerID update validation

**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**:
```release-note
```
@karataliu karataliu mentioned this issue Sep 15, 2017
Closed
25 tasks
@identw identw mentioned this issue Mar 7, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/cloudprovider kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle.
Projects
None yet
Milestone
v1.8
Development

Successfully merging a pull request may close this issue.

Fix providerID update validation karataliu/kubernetes
5 participants
@luxas @karataliu @andrewsykim @k8s-github-robot @k8s-ci-robot