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

feat: Creating a Custom Resource Definition for a pod IP to metadata mapper. #1071

Closed
wants to merge 22 commits into from

Conversation

prateek041
Copy link
Contributor

@prateek041 prateek041 commented Jun 12, 2023

The entire issue requires implementation of Custom Resource that maps pod IP to metadata and eventually remove cilium endpoint custom resource. The issue resolution is to be done in four parts, this is the first step where I Create the Custom resource definition.

Next up, writing the controller logic.

This commit:

- Initializes a Project with Name podinfo and repo github.com/cilium/tetragon
- Provide builderplate code for creating the operator

Signed-off-by: Prateek Singh <[email protected]>
This commit:

- Defines the Custom Resource.

Signed-off-by: Prateek Singh <[email protected]>
By default, a custom resource has spec (desired state) as well as status (current state) fields and Kubebuilder auto-generates manifests and deepcopy files for it. But, for out PodInfo resource we don't want the status info, so we need to remove it

This commit:

- Removes the //+kubebuilder:subresource:status comment from podinfo_types.go so kubebuilder doesn't auto generate things regarding the status field.
- make manifests and make generate commands create updated manifests and deepcopy files.

Signed-off-by: Prateek Singh <[email protected]>
@prateek041 prateek041 requested a review from a team as a code owner June 12, 2023 16:55
@prateek041 prateek041 requested a review from tixxdz June 12, 2023 16:55
@prateek041
Copy link
Contributor Author

cc @michi-covalent

@michi-covalent
Copy link
Contributor

michi-covalent commented Jun 12, 2023

lgtm overall. a few comments

  • i'm having a second thought regarding the resource format. maybe it makes sense to just mirror k8s pod resource format instead of coming up with our own. for example:
apiVersion: cilium.io/alphav1
kind: PodInfo
metadata:
  annotations:
    pod: annotation
  labels:
    pod: label
  name: pod-name
  namespace: pod-namespace
spec:
  hostNetwork: false
status:
  podIP: 1.2.3.4
  podIPs:
  - ip: 1.2.3.4
  startTime: "2023-05-28T12:09:02Z"
  • still trying to come up with a better name than PodInfo. i don't like that plural is podinfoes. it doesn't sound right. not a blocker but we might end up changing the name later.
  • remove podinfo/go.mod. we don't need to add a separate go module. also remove github.com/onsi/ginkgo/v2 and github.com/onsi/gomega dependencies. tetragon uses github.com/stretchr/testify. we should avoid introducing additional test frameworks.
  • remove fixes: #794 from the pull request description. this pull request doesn't fix the issue yet.

@prateek041
Copy link
Contributor Author

prateek041 commented Jun 13, 2023

  • Replicate the original k8s pod Resource format instead of creating one of our own. (needs discussion)
  • Remove go.mod file along with ginkgo and gomega.
  • remove fixes: issue 749 from the description.

@kkourt
Copy link
Contributor

kkourt commented Jun 16, 2023

Hi,

One small comment from my side, would it make sense to add the files into pkg/k8s instead in podinfo?

i don't like that plural is podinfoes

😅
You should be able to define your own plural, e.g., podsInfo. That's what we did for TracingPolicyNamespaced's plural being TracingPoliciesNamespaced rather than TracingPolicyNamespaceds. See: 2ebd385.

@michi-covalent
Copy link
Contributor

You should be able to define your own plural, e.g., podsInfo. That's what we did for TracingPolicyNamespaced's plural being TracingPoliciesNamespaced rather than TracingPolicyNamespaceds. See: 2ebd385.

yeah that's also an option. we ended up going with TetragonPod as the name for now. open to suggestions though, we can always rename later 📝

@kkourt
Copy link
Contributor

kkourt commented Jun 19, 2023

yeah that's also an option. we ended up going with TetragonPod as the name for now.

TetragonPod sounds good to me.

@netlify
Copy link

netlify bot commented Jun 27, 2023

Deploy Preview for tetragon ready!

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

- Updated the spec field
- Added the status field
- Generated manifests

Signed-off-by: Prateek Singh <[email protected]>
@prateek041
Copy link
Contributor Author

I made the changes suggested, please have a look.

Should I change the name to TetragonPod right away or later ?

cc @michi-covalent

hostNetwork:
description: Host networking requested for this pod. Use the host's
network namespace. If this option is set, the ports that will be
used must be specified.
Copy link
Member

@tixxdz tixxdz Jun 27, 2023

Choose a reason for hiding this comment

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

Maybe my comment is stupid: doc says "the ports that will be used must be specified" but doesn't specify where or how? maybe if it makes sense add it here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing that out. I will make the changes.

@tixxdz

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't mad the changes there yet, will do right before the PR is ready.

@prateek041
Copy link
Contributor Author

prateek041 commented Jul 2, 2023

Tasks:

  • Rename the CRD to Tetragonpod.
  • Integrate deployment of CRD through the operator.

cc @michi-covalent

…e operator to register the CRD with kubernetes API.

- It has an embed.go file that will include the YAML file in the binary.
- Created a register.go that will take that YAML file create a CRD object and return.

Signed-off-by: Prateek Singh <[email protected]>
…ator

- renamed the register package to client package, and wrote logic that will register the CRD with the API server
- Used the tetragon client package to register the CRD

Signed-off-by: Prateek Singh <[email protected]>
tetragonpod/api/v1alpha1/groupversion_info.go Outdated Show resolved Hide resolved
tetragonpod/PROJECT Outdated Show resolved Hide resolved

func RegisterCRD(client *apiextclientset.Clientset) error {
crd := GetCRD()
_, err := client.ApiextensionsV1().CustomResourceDefinitions().Create(context.TODO(), crd, metav1.CreateOptions{})
Copy link
Contributor

Choose a reason for hiding this comment

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

this will fail if the CRD already exists. we need to call Update() if the CRD already exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh thanks for pointing that out, I totally missed that. Working on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made changes, the current implementation is like this:

  • The CRD is created only if it doesn't already exists.
  • If the CRD already exists, no changes are made.

I am having a few questions regarding the update logic, like how would it know that CRD needs to be updated ? I went through the TracingPolicy CRD logic, how it is handling it, but have a few doubts. Let's have a little discussion on it in the meeting today.

…n with the help of operator.

- The logic now handles the condition if the CustomResourceDefinition already exists in the cluster.

Signed-off-by: Prateek Singh <[email protected]>
Signed-off-by: Prateek Singh <[email protected]>
@@ -94,9 +95,16 @@ func operatorExecute() {
// Register the CRDs after validating that we are running on a supported
// version of K8s.
if !operatorOption.Config.SkipCRDCreation {
if err := client.RegisterCRDs(k8sAPIExtClient); err != nil {

// Register Tracing Policy CRD
Copy link
Contributor

Choose a reason for hiding this comment

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

let's run this logic if and only if tetragonPod.enabled flag is set to true.

@prateek041
Copy link
Contributor Author

  • Delete unused files
  • Resolve merge conflicts
  • Logic to enable tetragonpod through helm

cc @michi-covalent

@michi-covalent
Copy link
Contributor

@prateek041 just an fyi i pushed a copy of this branch to #1281 so that we can test the CI image build 🧑‍🔬

@prateek041 prateek041 closed this Sep 19, 2023
@prateek041 prateek041 deleted the feat/prateek041/PodInfo branch September 19, 2023 05:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants