Skip to content
This repository was archived by the owner on Jun 29, 2022. It is now read-only.

Add Admission webhook to lokomotive#704

Merged
knrt10 merged 6 commits intomasterfrom
knrt10/prevent-mounting-default-sa
Aug 20, 2020
Merged

Add Admission webhook to lokomotive#704
knrt10 merged 6 commits intomasterfrom
knrt10/prevent-mounting-default-sa

Conversation

@knrt10
Copy link
Contributor

@knrt10 knrt10 commented Jul 7, 2020 •
edited
Loading

  • Add security to components and new namespace created by the user by preventing user to mount default ServiceAccount to their pods.

  • Add tests for components.

My findings trying to solve this issue were

  • Add mutating admission webhook, which patches default Service account that is created either by lokomotive components or user. It is now a part of system-component and run in lokomotive's lokomotive-system namespace and is bootstrapped by bootkube during cluster creation.

Fixes #669

Signed-off-by: knrt10 kautilya@kinvolk.io

@knrt10 knrt10 force-pushed the knrt10/prevent-mounting-default-sa branch 2 times, most recently from 1888ae5 to 85c1a11 Compare July 7, 2020 13:01
@knrt10 knrt10 requested review from iaguis and johananl July 7, 2020 13:29
iaguis
iaguis suggested changes Jul 7, 2020
View reviewed changes
Copy link
Contributor

@iaguis iaguis left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

In general it looks fine to me but I know some others were a bit hesitant about adding this kind of logic to lokoctl so I'd like to hear more opinions.

I also added some comments.

@knrt10 knrt10 force-pushed the knrt10/prevent-mounting-default-sa branch 6 times, most recently from 4bf8ebd to f42c175 Compare July 8, 2020 07:52
@knrt10 knrt10 requested a review from iaguis July 8, 2020 08:16
@knrt10 knrt10 force-pushed the knrt10/prevent-mounting-default-sa branch from f42c175 to 830fb3d Compare July 8, 2020 09:11
@knrt10
Copy link
Contributor Author

knrt10 commented Jul 8, 2020

Updated

@knrt10 knrt10 force-pushed the knrt10/prevent-mounting-default-sa branch 2 times, most recently from 9f93c2a to 1a45671 Compare July 8, 2020 09:37
iaguis
iaguis suggested changes Jul 8, 2020
View reviewed changes
@knrt10 knrt10 force-pushed the knrt10/prevent-mounting-default-sa branch from 1a45671 to 7935baf Compare July 8, 2020 10:56
@knrt10
Copy link
Contributor Author

knrt10 commented Jul 8, 2020 •
edited
Loading

Updated @iaguis. Regarding https://github.com/kinvolk/lokomotive/pull/704/files#diff-ce65c7c95a7f9d631a0a13b7ae8f1835R90. CI for openebs-operator fails unexpectedly sometimes on either Packet or AWS, so this is a fix for that. While applying locally on a cluster a problem never occurs. So to solve CI error, doing this

@knrt10 knrt10 requested a review from iaguis July 8, 2020 19:24
invidian
invidian reviewed Jul 9, 2020
View reviewed changes
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

I think patching the manifests and also reporting those patches upstream would be simple and better.

@iaguis
Copy link
Contributor

iaguis commented Jul 10, 2020

I think patching the manifests and also reporting those patches upstream would be simple and better.

The point of this is to have the default ServiceAccount in all namespaces we create to not get mounted automatically in pods.

Did you mean patching all the pod templates with automountServiceAccountToken=false? I think this won't work because it will break pods that define a custom service account. If you mean adding them to pods that don't declare a ServiceAccount I think we don't have any (is that right, @knrt10?).

I was thinking that a better solution might be to have a mutating admission controller that ensures default ServiceAccounts for all namespaces have automountServiceAccountToken=false. This is the approach taken by Karydia and will apply this even to namespaces created by users.

@knrt10
Copy link
Contributor Author

knrt10 commented Jul 10, 2020

Yes @iaguis. Chart templates seem fine to me. And as for solution by karydia, I saw it a couple days back and wanted to try it. Let me try and implement it

@knrt10 knrt10 marked this pull request as draft July 20, 2020 08:29
@knrt10 knrt10 force-pushed the knrt10/prevent-mounting-default-sa branch 3 times, most recently from 8f2cd53 to 4cd7a49 Compare July 22, 2020 09:36
ipochi
ipochi previously requested changes Aug 11, 2020
View reviewed changes
Copy link
Member

@ipochi ipochi left a comment

Choose a reason for hiding this comment

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

Great work @knrt10.

Minor nits.

surajssd
surajssd suggested changes Aug 12, 2020
View reviewed changes
invidian
invidian suggested changes Aug 12, 2020
View reviewed changes
invidian
invidian suggested changes Aug 13, 2020
View reviewed changes
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

@knrt10 can you please try the following bootkube image for this PR: quay.io/kinvolk/bootkube:0233d235a24a6eb94a9646a2d5a5327f5f7005b9 ? It includes https://github.com/kinvolk/bootkube/pull/9, which I think should resolve the issue with failing webhook and the namespace creation.

@invidian invidian mentioned this pull request Aug 16, 2020
Merged
invidian
invidian suggested changes Aug 16, 2020
View reviewed changes
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

:)

surajssd
surajssd previously approved these changes Aug 17, 2020
View reviewed changes
Copy link
Member

@surajssd surajssd left a comment

Choose a reason for hiding this comment

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

Just one thing otherwise LGTM

surajssd
surajssd previously approved these changes Aug 17, 2020
View reviewed changes
Copy link
Member

@surajssd surajssd left a comment

Choose a reason for hiding this comment

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

ok

invidian
invidian suggested changes Aug 18, 2020
View reviewed changes
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

Just some remaining nits, otherwise LGTM.

invidian
invidian suggested changes Aug 19, 2020
View reviewed changes
invidian
invidian suggested changes Aug 19, 2020
View reviewed changes
@knrt10
Copy link
Contributor Author

knrt10 commented Aug 19, 2020

Updated

invidian
invidian previously approved these changes Aug 19, 2020
View reviewed changes
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks @knrt10 for your patience and effort.

As this is a significant change, 2nd person should have a thorough look at it before merging.

iaguis
iaguis suggested changes Aug 19, 2020
View reviewed changes
Copy link
Contributor

@iaguis iaguis left a comment

Choose a reason for hiding this comment

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

Some small comments. Otherwise it looks pretty nice!

@knrt10
Copy link
Contributor Author

knrt10 commented Aug 19, 2020

Updated @iaguis

iaguis
iaguis previously approved these changes Aug 19, 2020
View reviewed changes
Copy link
Contributor

@iaguis iaguis left a comment

Choose a reason for hiding this comment

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

lgtm

knrt10 and others added 6 commits August 20, 2020 00:51
@knrt10
Add mutating admission webhook
8fbdac4 
Lokomotive now has admission webhook which adds security to existing
components and any new namespace created by user, by preventing Pods to
automount default service account

Add tests for components

Fixes #669

Signed-off-by: knrt10 <kautilya@kinvolk.io>
@knrt10
assets/: format using terraform fmt
ecf29bd 
Signed-off-by: knrt10 <kautilya@kinvolk.io>
@knrt10
Add go code for admission webhook
7d1f0a1 
Signed-off-by: knrt10 <kautilya@kinvolk.io>
Ensure upgrade for lokomotive component
474abab 
Previously we had upgradeComponent for releases in kube-system
namespace, but now after introduction for lokomotive's own webhook and
namespace, this will fix and upgrade release according to their own
namespaces.

Signed-off-by: knrt10 <kautilya@kinvolk.io>
Update bootkube image
41b63b0 
With this change, we are adding a feature where helm can create
namespace while bootstraping.

Signed-off-by: knrt10 <kautilya@kinvolk.io>
Fix install values while upgrading releases
1ae44f6 
Earlier while re-applying cluster if a release did not exist, an empty
map was passed as values due to which helm got null value and upgrading
of release failed.

Signed-off-by: knrt10 <kautilya@kinvolk.io>
@knrt10
Copy link
Contributor Author

knrt10 commented Aug 19, 2020 •
edited
Loading

@iaguis @invidian can you please review again. Small change was made in deployment. Tests were failing, fixed now

@knrt10
Copy link
Contributor Author

knrt10 commented Aug 20, 2020

Merging this, as Mateusz already approved it before. Thank you all for your reviews.

@invidian invidian mentioned this pull request Aug 27, 2020
Open
@knrt10 knrt10 mentioned this pull request Sep 10, 2020
Merged
@iaguis iaguis mentioned this pull request Oct 20, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@johananl johananl Awaiting requested review from johananl

@ipochi ipochi Awaiting requested review from ipochi

@invidian invidian Awaiting requested review from invidian

2 more reviewers

@iaguis iaguis iaguis approved these changes

@surajssd surajssd surajssd approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Don't mount the default Service Account in pods by default

6 participants

@knrt10 @iaguis @ipochi @johananl @surajssd @invidian