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

Add Registration logic for Pod Info Custom Resource Definition. #1369

Merged
merged 1 commit into from
Aug 31, 2023
Merged

Add Registration logic for Pod Info Custom Resource Definition. #1369

merged 1 commit into from
Aug 31, 2023

Conversation

prateek041
Copy link
Contributor

@prateek041 prateek041 commented Aug 18, 2023 •
edited
Loading

Currently tetragon uses cilium Endoints to get Pod Labels. Pod Info CRD is a mapping between Pod IPs and Pod metadata therefore, instead of using cilium endpoints, it can use the pod info CRs, hence removing the dependency on cilium.

This PR:

  • Defines the Custom Resource Definition.
  • Operator is being used to register the Tracing Polify CRDs with API server, hence integrated the logic to register Pod Info CRD as well.
  • Added flag to skip PodInfo CRD in the operator, tetragon will use endpoints in this case.

@prateek041 prateek041 requested a review from a team as a code owner August 18, 2023 16:03
@prateek041 prateek041 requested a review from tpapagian August 18, 2023 16:03
@michi-covalent
Copy link
Contributor

@prateek041 could you take a look at this failure https://github.com/cilium/tetragon/actions/runs/5904686326/job/16017296464?pr=1369 you probably just need to run make vendor 👀

@kkourt kkourt self-requested a review August 21, 2023 16:17
@prateek041
Copy link
Contributor Author

Contributor

sure let me do that. I am quite sure I did it though.

@netlify
Copy link

netlify bot commented Aug 21, 2023 •
edited
Loading

✅ Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit 5c4ad17
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/64ee79216ee5d20008aa6ef9
😎 Deploy Preview https://deploy-preview-1369--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.

@michi-covalent
Copy link
Contributor

i'll look into what's causing these failure today. moving it to draft for now

@michi-covalent michi-covalent marked this pull request as draft August 23, 2023 14:54
@michi-covalent michi-covalent force-pushed the feat/podinfo-registration branch 7 times, most recently from 8588fe8 to a79180a Compare August 23, 2023 17:58
@michi-covalent
Copy link
Contributor

ok this should be good to go. @prateek041 could you update the commit message to explain why we need this CRD, also linking to this github issue? #794

then mark it ready for review 🚀

@prateek041 prateek041 force-pushed the feat/podinfo-registration branch from a79180a to dd97bb8 Compare August 24, 2023 06:40
kkourt
kkourt reviewed Aug 24, 2023
View reviewed changes
Copy link
Contributor

@kkourt kkourt left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks!

The structure of the new object looks good to me, but I have some small comments in the code details.

operator/crd/crd.go Outdated Show resolved Hide resolved
operator/crd/crd.go Outdated Show resolved Hide resolved
pkg/k8s/apis/cilium.io/client/register_v1_crd.go Outdated Show resolved Hide resolved
@prateek041 prateek041 force-pushed the feat/podinfo-registration branch from dd97bb8 to 3b4269c Compare August 24, 2023 10:54
@prateek041
Copy link
Contributor Author

prateek041 commented Aug 24, 2023 •
edited
Loading

I think I am not understanding one very basic thing about coding in Go. Why do I get these vendoring issues out of nowhere ? What are the practices I should use to avoid these ?

@kkourt
Copy link
Contributor

kkourt commented Aug 24, 2023

I think I am not understanding one very basic thing about coding in Go. Why do I get these vendoring issues out of nowhere ? What are the practices I should use to avoid these ?

I'm not sure what the underlying reason is, but one thing that you can do is:

  • run make vendor
  • adding all the new files (e.g., under the vendor dir in your patch
    Before submitting the PR.

@prateek041 prateek041 force-pushed the feat/podinfo-registration branch from 3b4269c to e81dd9a Compare August 24, 2023 15:25
@prateek041 prateek041 marked this pull request as ready for review August 24, 2023 16:32
kkourt
kkourt requested changes Aug 25, 2023
View reviewed changes
Copy link
Contributor

@kkourt kkourt left a comment

Choose a reason for hiding this comment

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

Thanks!

It seems to me that we are still using the old naming, so it would be nice to be consistent.
I also have a suggestion on how to refactor createCRD.

operator/crd/crd.go Outdated Show resolved Hide resolved
operator/crd/crd.go Outdated Show resolved Hide resolved
operator/flags.go Outdated Show resolved Hide resolved
operator/flags.go Outdated Show resolved Hide resolved
operator/option/config.go Outdated Show resolved Hide resolved
operator/option/config.go Outdated Show resolved Hide resolved
vendor/github.com/cilium/tetragon/pkg/k8s/apis/cilium.io/client/register_v1_crd.go Outdated Show resolved Hide resolved
@prateek041 prateek041 requested a review from kkourt August 28, 2023 15:56
@prateek041 prateek041 force-pushed the feat/podinfo-registration branch 3 times, most recently from 10c7aad to c9e7537 Compare August 28, 2023 17:50
@prateek041 prateek041 added the release-note/minor This PR introduces a minor user-visible change label Aug 28, 2023
@prateek041 prateek041 force-pushed the feat/podinfo-registration branch from c9e7537 to 6df51ad Compare August 28, 2023 17:56
@prateek041 prateek041 force-pushed the feat/podinfo-registration branch from 6df51ad to bcd7caf Compare August 28, 2023 18:02
@michi-covalent michi-covalent force-pushed the feat/podinfo-registration branch from bcd7caf to 5c4ad17 Compare August 29, 2023 23:02
michi-covalent
michi-covalent approved these changes Aug 29, 2023
View reviewed changes
Copy link
Contributor

@michi-covalent michi-covalent left a comment

Choose a reason for hiding this comment

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

lgtm minor nits

operator/crd/crd.go Outdated Show resolved Hide resolved
pkg/k8s/apis/cilium.io/v1alpha1/types.go Outdated Show resolved Hide resolved
kkourt
kkourt approved these changes Aug 30, 2023
View reviewed changes
Copy link
Contributor

@kkourt kkourt left a comment

Choose a reason for hiding this comment

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

Thanks! I have a question and a minor nit.
I'm OK with using the same plural name as the singular if it's OK with @michi-covalent.
If the Unwrap is unnecessary, please remove it. If it is needed, please let me know why.

Other than that, everything LGTM so I will pre-approve.
Awesome work! 🚀

pkg/k8s/Makefile Show resolved Hide resolved
pkg/k8s/apis/cilium.io/client/register_v1_crd.go Outdated Show resolved Hide resolved
@prateek041 prateek041 force-pushed the feat/podinfo-registration branch 2 times, most recently from 7c5a2c9 to 2ca3c62 Compare August 30, 2023 18:19
@prateek041
Add logic to register pod Info CRD
7e03785 
Currently tetragon uses cilium Endoints to get Pod Labels. Pod Info CRD
is a mapping between Pod IPs and Pod metadata therefore, instead of
using cilium endpoints, it can use the pod info CRs, hence removing the
dependency on cilium.

This commit:
- Added flag to skip PodInfo CRD in the operator, tetragon will use
  endpoints in this case.
- Integrated  logic to register the PodInfo CRD with the API server,
  similar to tracing Policies CRD.

Signed-off-by: Prateek Singh <prateeksingh9741@gmail.com>
Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
@prateek041 prateek041 force-pushed the feat/podinfo-registration branch from 2ca3c62 to 7e03785 Compare August 30, 2023 18:29
@michi-covalent michi-covalent merged commit 7ab6171 into cilium:main Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tpapagian tpapagian Awaiting requested review from tpapagian tpapagian is a code owner automatically assigned from cilium/tetragon

@michi-covalent michi-covalent Awaiting requested review from michi-covalent

@kkourt kkourt Awaiting requested review from kkourt

Assignees
No one assigned
Labels
release-note/minor This PR introduces a minor user-visible change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@prateek041 @michi-covalent @kkourt