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

helm: add an rthooks serviceAccount section #2859

Merged
merged 1 commit into from
Sep 5, 2024
Merged

helm: add an rthooks serviceAccount section #2859

merged 1 commit into from
Sep 5, 2024

Conversation

kkourt
Copy link
Contributor

@kkourt kkourt commented Sep 2, 2024

No description provided.

@kkourt kkourt added the release-note/misc This PR makes changes that have no direct user impact. label Sep 2, 2024
@netlify Netlify
Copy link

netlify bot commented Sep 2, 2024 •
edited
Loading

✅ Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit d1dc341
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/66d84d4149c50900085f68cd
😎 Deploy Preview https://deploy-preview-2859--tetragon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@kkourt kkourt marked this pull request as ready for review September 2, 2024 09:48
@kkourt kkourt requested a review from a team as a code owner September 2, 2024 09:48
@kkourt kkourt requested a review from tixxdz September 2, 2024 09:48
mtardy
mtardy approved these changes Sep 2, 2024
View reviewed changes
Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

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

Looks reasonable!

I would be curious about why we need this service account since it seems to be associated with no role.

install/kubernetes/tetragon/templates/_helpers.tpl Outdated Show resolved Hide resolved
@kkourt kkourt force-pushed the pr/kkourt/rthooks-serviceaccount branch 2 times, most recently from 7dfb15e to 8169e10 Compare September 2, 2024 15:13
@kkourt
Copy link
Contributor Author

kkourt commented Sep 2, 2024

Thanks @mtardy! pushed a new version that sets the default value of create: to false as discussed offline.

@mtardy mtardy requested review from fgiloux and removed request for tixxdz September 3, 2024 08:20
fgiloux
fgiloux approved these changes Sep 4, 2024
View reviewed changes
Copy link
Collaborator

@fgiloux fgiloux left a comment

Choose a reason for hiding this comment

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

As Mahe wrote it may have been enough to make the SA name configurable in the rthooks DaemonSet but it still looks good to me.

mtardy
mtardy reviewed Sep 4, 2024
View reviewed changes
Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

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

If we can just make the ServiceAccountName configurable, I guess it would be enough?

install/kubernetes/tetragon/values.yaml Outdated Show resolved Hide resolved
install/kubernetes/tetragon/templates/rthooks_serviceaccount.yaml Outdated Show resolved Hide resolved
install/kubernetes/tetragon/templates/_helpers.tpl Outdated Show resolved Hide resolved
install/kubernetes/tetragon/templates/rthooks-daemonset.yaml Outdated Show resolved Hide resolved
@kkourt kkourt force-pushed the pr/kkourt/rthooks-serviceaccount branch from 8169e10 to d9ce585 Compare September 4, 2024 11:33
@kkourt
Copy link
Contributor Author

kkourt commented Sep 4, 2024

Thanks Mahé! pushed a new version, PTAL.

@kkourt
helm: add an rthooks serviceAccount section
d1dc341 
tested via:
helm template tetragon --set rthooks.enabled=true --set rthooks.interface=nri-hook ./install/kubernetes/tetragon | sed -n '/^.*tetragon\/templates\/rthooks-daemonset.yaml/,/^---$/p' | grep serviceAccountName
helm template tetragon --set rthooks.enabled=true --set rthooks.interface=nri-hook --set rthooks.serviceAccount.name=pizza  ./install/kubernetes/tetragon | sed -n '/^.*tetragon\/templates\/rthooks-daemonset.yaml/,/^---$/p' | grep serviceAccountName
      serviceAccountName: pizza

Signed-off-by: Kornilios Kourtis <[email protected]>
@kkourt kkourt force-pushed the pr/kkourt/rthooks-serviceaccount branch from d9ce585 to d1dc341 Compare September 4, 2024 12:06
@kkourt
Copy link
Contributor Author

kkourt commented Sep 4, 2024

Pushed a new version due to conflicts.

mtardy
mtardy approved these changes Sep 4, 2024
View reviewed changes
Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

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

great, can you recheck @fgiloux to make sure everything looks good on your side?

@mtardy mtardy requested a review from fgiloux September 4, 2024 14:09
fgiloux
fgiloux approved these changes Sep 5, 2024
View reviewed changes
install/kubernetes/tetragon/values.yaml
@@ -395,3 +395,6 @@ rthooks:
override: ~
repository: quay.io/cilium/tetragon-rthooks
tag: v0.3
# -- rthooks service account.
serviceAccount:
Copy link
Collaborator

Choose a reason for hiding this comment

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

IIRC in the previous version the default was the release name, like for the agent.
With the new version you don't get the credentials of the tetragon SA to access the API. An alternative approach to achieve the same is with adding automountServiceAccountToken: false to spec.template.spec of the rthooks daemonset. This may be very specific to OpenShift but the SA identity will be checked for admitting a pod using hostpath. Both the agent and the rthooks daemonset need it, which means that their respective SAs need to be bound to a convenient SCC.
The answer may be documentation but I wanted to mention this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the new version you don't get the credentials of the tetragon SA to access the API

API here being the k8s API server, correct? I think that's fine since rthooks does not need access to the API server. I think I'll just merge the change for now. Once/if we have a concrete problem, it would be easier to figure it out then.

Thanks!

@kkourt kkourt merged commit a38badb into main Sep 5, 2024
47 checks passed
@kkourt kkourt deleted the pr/kkourt/rthooks-serviceaccount branch September 5, 2024 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fgiloux fgiloux fgiloux approved these changes

@mtardy mtardy mtardy approved these changes

Assignees
No one assigned
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kkourt @mtardy @fgiloux