feat(k8s): add service account and irsa support for in-cluster-builder #3384
Conversation
stefreak
changed the title
stefreak
force-pushed
the
irsa
branch
3 times, most recently
from
b733634 to
fff9644
Compare
stefreak
force-pushed
the
irsa
branch
2 times, most recently
from
a79e55b to
da9bebd
Compare
There was a problem hiding this comment.
Reviewed the docs! Thanks for implementing this, this is super useful!
I have two suggestions concerning the structure of the docs as they contain significantly more content now than before:
- Split into documentation about pulling images and pushing images.
- For pushing images, keep the existing docs on how to allow all nodes to push to ECR and display using IRSA as an improved and recommended alternative.
docs/guides/in-cluster-building.md
Outdated
| buildMode: kaniko | ||
| kaniko: | ||
| serviceAccountAnnotations: | ||
| eks.amazonaws.com/role-arn: arn:aws:iam::<account-id>:role/<web-identity-role-name> |
stefreak
dismissed
twelvemo’s stale review
Dismissing Annas review, because she's on vacation until next year and I implemented her suggestions
|
This PR solves an issue we're having as well. Any chance this will make the next release? |
|
@jamesloosli I absolutely want to finish this but we won't make it before the 0.13 release (will happen this month). |
|
Changed the base back to main, as this would need to land in main and not 0.12 |
1 similar comment
|
Changed the base back to main, as this would need to land in main and not 0.12 |
|
@highb right now we are focused on bug fixes and stability improvements– but this is definitely something I want to look back into in the coming months. |
|
Picking this up again now and also making sure this works with GCP workload identity for the in-cluster builders as well. |
twelvemo
force-pushed
the
irsa
branch
2 times, most recently
from
c9c994e to
436a8b2
Compare
| log, | ||
| }) | ||
| // Both annotations should be present | ||
| expect(isEqualAnnotations(originalServiceAccount, status.remoteResources[0])) |
There was a problem hiding this comment.
I'm not sure if expect will actually fail if you do not explicitly assert for true
| expect(isEqualAnnotations(originalServiceAccount, status.remoteResources[0])) | |
| expect(isEqualAnnotations(originalServiceAccount, status.remoteResources[0])).to.be.true |
There was a problem hiding this comment.
We should install https://www.npmjs.com/package/eslint-plugin-chai-expect
There was a problem hiding this comment.
Fixed the missing assertions, but will create a new PR for adding the chai expect linter plugin because some existing tests fail the new linter rules.
| @@ -2,6 +2,6 @@ kind: Module | |||
| type: container | |||
| name: skopeo | |||
| description: Used by the kubernetes provider for interacting with container registries within a cluster | |||
| image: gardendev/skopeo:1.41.0-3 | |||
| image: gardendev/skopeo:1.41.0-4 | |||
There was a problem hiding this comment.
We do not seem to reference this image from Garden, but this can be cleaned up in a separate PR
|
Addressed all comments, could you take a look again @vvagaytsev ? |
core/test/integ/src/plugins/kubernetes/container/build/build.ts
Outdated
Show resolved
Hide resolved
core/test/unit/src/plugins/kubernetes/container/build/kaniko.ts
Outdated
Show resolved
Hide resolved
What this PR does / why we need it:
This PR introduces a new service account, and makes the annotations
configurable in kaniko and buildkit in-cluster-builders.
This change enables the use of IRSA for in-cluter-building which makes
in-cluster building more secure.
Which issue(s) this PR fixes:
Fixes #2931
Special notes for your reviewer: