Skip to content
This repository has been archived by the owner on Sep 30, 2020. It is now read-only.

Move IAMRoleWorker definition from network stack to node pool stack #1521

Merged
merged 1 commit into from
Jan 9, 2019

Conversation

ktateish
Copy link
Contributor

kube-aws fails to delete a node pool from stack because of
IAMRoleWorker definitions in network stack. This commit fixes it by
moving the definition from network stack to node pool stacks.

Closes #1518

kube-aws fails to delete a node pool from stack because of
IAMRoleWorker definitions in network stack.  This commit fixes it by
moving the definition from network stack to node pool stacks.

Closes kubernetes-retired#1518
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 28, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: danielfm

If they are not already assigned, you can assign the PR to them by writing /assign @danielfm in a comment when ready.

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

@codecov-io
Copy link

Codecov Report

Merging #1521 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1521   +/-   ##
=======================================
  Coverage   25.46%   25.46%           
=======================================
  Files          97       97           
  Lines        5003     5003           
=======================================
  Hits         1274     1274           
  Misses       3582     3582           
  Partials      147      147

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8b7380...bd2d1e7. Read the comment docs.

@davidmccormick
Copy link
Contributor

davidmccormick commented Jan 2, 2019

Hi many thanks for looking into this issue. One question, what happens for the users that have deployed already with the IAMRoleWorker in their network stacks? What does the migration look like?

Copy link
Contributor

@mumoshu mumoshu left a comment

Choose a reason for hiding this comment

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

Hey! You've done a great job fixing this!

To be clear this unfortunately breaks the aws-iam-authenticaator support for kubelet auth, but I think we have no easy fix for that. Perhaps we should update the authenticator config via a k8s pod that fetches all the worker roles? But that's an another story.

LGTM. Thanks again for your contribution ☺️

@mumoshu mumoshu merged commit 83fd851 into kubernetes-retired:master Jan 9, 2019
@mumoshu
Copy link
Contributor

mumoshu commented Jan 9, 2019

@davidmccormick Oops, I missed your comment.

I haven't actually tried it yet but I didn't taken so much care because it affects users who created clusters with the kube-aws built manually from the master after #1490.

But anyway, it should fail due to the network stack trying to delete the worker roles already referenced by the worker stacks. We'd need to retain the worker role definition and outputs, exports in the network stack to enable that upgrade path.

@mumoshu mumoshu added this to the v0.13.0 milestone Jan 9, 2019
kevtaylor pushed a commit to HotelsDotCom/kube-aws that referenced this pull request Jan 9, 2019
…ubernetes-retired#1521)

kube-aws fails to delete a node pool from stack because of
IAMRoleWorker definitions in network stack.  This commit fixes it by
moving the definition from network stack to node pool stacks.

Closes kubernetes-retired#1518
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deleting node pool fails with UPDATE_ROLLBACK_IN_PROGRESS
5 participants