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

tetragon: run Tetragon without access to CRD #1931

Merged
merged 2 commits into from
Jan 23, 2024
Merged

Conversation

aohoyd
Copy link
Contributor

@aohoyd aohoyd commented Jan 4, 2024

Fixes #1880

Tetragon

  • new flag enable-tracing-policy-crd was added and enabled by default

Tetragon operator

  • new flag skip-tracing-policy-crd was added and disabled by default

Helm chart

  • two flags above were added into configmaps and values file

@aohoyd aohoyd requested a review from a team as a code owner January 4, 2024 17:57
@aohoyd aohoyd requested a review from tpapagian January 4, 2024 17:57
@aohoyd aohoyd changed the title Run Tetragon without access to CRD tetragon: run Tetragon without access to CRD Jan 4, 2024
@aohoyd aohoyd force-pushed the main branch 2 times, most recently from 558a9dd to cd894c7 Compare January 4, 2024 18:27
@jrfastab
Copy link
Contributor

jrfastab commented Jan 8, 2024

/test

Copy link

netlify bot commented Jan 8, 2024

Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit a94d9a0
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/65aba1d2505f070008830638
😎 Deploy Preview https://deploy-preview-1931--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.

@aohoyd aohoyd force-pushed the main branch 2 times, most recently from 9cd7919 to 3bfee91 Compare January 8, 2024 20:25
@aohoyd
Copy link
Contributor Author

aohoyd commented Jan 8, 2024

I've synced with main and re-generated doc files

@mtardy mtardy added the release-note/minor This PR introduces a minor user-visible change label Jan 9, 2024
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, this looks good to me. Could you please split the patch into two patches: one for the agent changes, and one for the operator changes?

@aohoyd
Copy link
Contributor Author

aohoyd commented Jan 10, 2024

Thanks, this looks good to me. Could you please split the patch into two patches: one for the agent changes, and one for the operator changes?

Sure, done (I've splitted commit)

@kkourt did you mean to open separated PR?

@mtardy
Copy link
Member

mtardy commented Jan 10, 2024

Thanks, this looks good to me. Could you please split the patch into two patches: one for the agent changes, and one for the operator changes?

Sure, done (I've splitted commit)

@kkourt did you mean to open separated PR?

I can't speak for Kornilios but I would say separate commits, so looking all good!

@kkourt
Copy link
Contributor

kkourt commented Jan 10, 2024

Thanks, this looks good to me. Could you please split the patch into two patches: one for the agent changes, and one for the operator changes?

Sure, done (I've splitted commit)

@kkourt did you mean to open separated PR?

I mean splitting the commits. Thanks!

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.

LGTM, thanks!

@aohoyd
Copy link
Contributor Author

aohoyd commented Jan 15, 2024

synced fork

@lambdanis
Copy link
Contributor

@aohoyd Can you rebase your branch with cilium:main instead of merging it? It makes the git history easier to read. At the moment the checkpath CI job is failing because of the merge commit (https://github.com/cilium/tetragon/actions/runs/7526335727/job/20488255210?pr=1931).

@aohoyd
Copy link
Contributor Author

aohoyd commented Jan 15, 2024

@lambdanis sorry, did this via UI. I've reverted merge commit and made a rebase

@aohoyd
Copy link
Contributor Author

aohoyd commented Jan 16, 2024

I'm not sure, but failed test looks like spontaneous error. Can anyone restart it?

@kkourt
Copy link
Contributor

kkourt commented Jan 17, 2024

Tests look all good! ✅

@mtardy, @tpapagian, @lambdanis: do you plan to review?
Otherwise, I would suggest we merge.

Copy link
Member

@tpapagian tpapagian left a comment

Choose a reason for hiding this comment

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

Not super-familiar with this part of the codebase, but LGTM! Thanks

Copy link
Contributor

@lambdanis lambdanis left a comment

Choose a reason for hiding this comment

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

Small note: if all CRDs are disabled, then I believe Tetragon operator has nothing to do, but it still runs, and there is no way to disable it. Unless someone corrects me, we can update the Helm chart to allow disabling the whole operator in such cases. But this can be a follow-up, not blocking for this PR.

@aohoyd
Copy link
Contributor Author

aohoyd commented Jan 18, 2024

@lambdanis I can do it in separate PR, not a problem. Correct me if I'm wrong:
Operator should be disabled in next case:

  • if tetragonOperator.podInfo.enabled is false and tetragonOperator.tracingPolicy.enabled is false

Didn't I miss something? Is tetragon.enableK8sAPI related?

@lambdanis
Copy link
Contributor

Operator should be disabled in next case:

  • if tetragonOperator.podInfo.enabled is false and tetragonOperator.tracingPolicy.enabled is false

Yes. One option is to introduce tetragonOperator.enabled Helm value, then it'll be easier to maintain I think if new CRDs are introduced. cc @michi-covalent to confirm/deny/comment.

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.

adding tetragonOperator.enabled seems reasonable if we want to be able to disable the operator altogether. we can do that as a follow-up PR ✅

Fixes: cilium#1880

Signed-off-by: Alexey Olshanskiy <gh@aohoy.dev>
Fixes: cilium#1880

Signed-off-by: Alexey Olshanskiy <gh@aohoy.dev>
@aohoyd
Copy link
Contributor Author

aohoyd commented Jan 20, 2024

I've added the new flag in #2004

@mtardy
Copy link
Member

mtardy commented Jan 23, 2024

Failure in the CI is unrelated #2010 and we can merge this one.

@mtardy
Copy link
Member

mtardy commented Jan 23, 2024

Tests look all good! ✅
@mtardy, @tpapagian, @lambdanis: do you plan to review?
Otherwise, I would suggest we merge.

So I'll merge this, I was just pulled-in by GitHub because of the docs change.

@mtardy mtardy merged commit 03beec7 into cilium:main Jan 23, 2024
37 of 38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/minor This PR introduces a minor user-visible change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run Tetragon without access to CRD
7 participants