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

Upgrade aws-sdk-go for IRSA #67

Merged
merged 2 commits into from
Dec 6, 2019

Conversation

anarcher
Copy link
Contributor

@anarcher anarcher commented Dec 5, 2019

Fixes #68

Update go.mod for upgrading AWS SDK[1] for IRSA
IRSA can then provide AWS permissions to the instance-manager controller pod that uses that service account. :-)

[1] https://docs.aws.amazon.com/eks/latest/userguide/iam-roles-for-service-accounts-minimum-sdk.html

I think the controller works well with upgrading aws-sdk-go

time="2019-12-05T16:22:43Z" level=info msg="reconcile started"
time="2019-12-05T16:22:43Z" level=info msg="resource document version: 12214434"
time="2019-12-05T16:22:43Z" level=info msg="resource namespace: addon-instance-manager"
time="2019-12-05T16:22:43Z" level=info msg="resource name: hello-world"
time="2019-12-05T16:22:43Z" level=info msg="resource provisioner: eks-cf"
time="2019-12-05T16:22:43Z" level=info msg="upgrade strategy: rollingupdate"

@anarcher anarcher requested a review from a team as a code owner December 5, 2019 16:30
@claassistantio
Copy link

claassistantio commented Dec 5, 2019

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Dec 5, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #67   +/-   ##
=======================================
  Coverage   73.09%   73.09%           
=======================================
  Files           6        6           
  Lines         825      825           
=======================================
  Hits          603      603           
  Misses        162      162           
  Partials       60       60

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 26126d0...23ca4ea. Read the comment docs.

@eytan-avisror
Copy link
Collaborator

Thanks for the PR!

This change should be well tested before merging.
We've already hit this bug:
aws/aws-sdk-go#2972

in upgrade manager we had to roll this back:
keikoproj/upgrade-manager#31

Did you get a chance to test full reconciles with this new SDK version?

@anarcher
Copy link
Contributor Author

anarcher commented Dec 6, 2019

Thanks for the review! Oh, I haven't known the issue (aws/aws-sdk-go#2972). With [email protected] and IRSA, The reconciling work well. But I'm not sure that the slow issue is existed or not. ㅠ_ㅠ

Like the upgrade-manager, I will change to [email protected]. :->

$k get ig
NAME          STATE   MIN   MAX   GROUP NAME                   PROVISIONER   STRATEGY        LIFECYCLE   AGE
hello-world   Ready   1     1     dev-hello-world-NodeGroup-   eks-cf        rollingUpdate   normal      10h
time="2019-12-06T02:05:28Z" level=info msg="reconcile started"
time="2019-12-06T02:05:28Z" level=info msg="resource document version: 12219322"
time="2019-12-06T02:05:28Z" level=info msg="resource namespace: addon-instance-manager"
time="2019-12-06T02:05:28Z" level=info msg="resource name: hello-world"
time="2019-12-06T02:05:28Z" level=info msg="resource provisioner: eks-cf"
time="2019-12-06T02:05:28Z" level=info msg="upgrade strategy: rollingupdate"
time="2019-12-06T02:05:49Z" level=info msg="state transitioned to: Init"
time="2019-12-06T02:05:49Z" level=info msg="starting cloud discovery"
time="2019-12-06T02:05:49Z" level=info msg="stack state: CREATE_COMPLETE"
time="2019-12-06T02:05:49Z" level=info msg="starting state discovery"
time="2019-12-06T02:05:49Z" level=info msg="state transitioned to: InitUpdate"
time="2019-12-06T02:05:49Z" level=info msg="starting update"
time="2019-12-06T02:05:50Z" level=info msg="update not required"
time="2019-12-06T02:05:50Z" level=info msg="state transitioned to: ReconcileModified"
time="2019-12-06T02:05:50Z" level=info msg="stack state: CREATE_COMPLETE"
time="2019-12-06T02:05:50Z" level=info msg="starting bootstrap"
time="2019-12-06T02:05:50Z" level=info msg="state transitioned to: Ready"
time="2019-12-06T02:05:50Z" level=info msg="reconcile completed"
2019-12-06T02:05:50.902Z	DEBUG	controller-runtime.controller	Successfully Reconciled	{"controller": "instancegroup", "request": "addon-instance-manager/hello-world"}

Copy link
Collaborator

@eytan-avisror eytan-avisror left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for adding this.

@eytan-avisror eytan-avisror merged commit 9a21c95 into keikoproj:master Dec 6, 2019
@anarcher anarcher deleted the pr/irsa-aws-sdk branch December 6, 2019 05:32
@eytan-avisror eytan-avisror mentioned this pull request Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update AWS sdk version to support IRSA
3 participants