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

helm: Don't give operator permissions to create CRDs if not needed #2326

Merged
merged 1 commit into from
Jul 12, 2024

Conversation

itsCheithanya
Copy link
Contributor

@itsCheithanya itsCheithanya commented Apr 12, 2024

Don't give operator permissions to create CRDs if not needed
Result of the change :

image
Reproduce the result:
create a local values.yaml file with CRD creation disabled:

tetragonOperator:
  skipCRDCreation: true

and then run
and install Tetragon with using local Helm chart and your values.yaml file:

./contrib/localdev/install-tetragon.sh --values values.yaml Then, check the operator ClusterRole using kubectl:

kubectl get clusterrole tetragon-operator -oyaml

Fixes: #2226

@itsCheithanya itsCheithanya requested a review from a team as a code owner April 12, 2024 05:58
@lambdanis lambdanis added area/helm Related to the Helm chart release-note/minor This PR introduces a minor user-visible change labels Apr 14, 2024
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.

Hi @itsCheithanya, thank you for the PR. The code change looks good, but the CI is complaining about the long commit message subject:

{[1/1] Don't give operator permissions to create CRDs if not needed ,add if block to stop it}

  Error: ERROR:CUSTOM: Please avoid long commit subjects (max: 75, found: 85)

https://github.com/cilium/tetragon/actions/runs/8657663468/job/23747314419?pr=2326

Could you amend the commit message and force push? You can put details in the commit message body.

@lambdanis lambdanis self-assigned this Apr 14, 2024
@lambdanis
Copy link
Contributor

lambdanis commented Apr 25, 2024

@itsCheithanya It seems you pushed an extra empty commit instead of amending the previous one, so the CI still fails. You can fix it like this:

git reset --hard HEAD~1  # delete the second commit
git commit --amend  # now edit the commit message of the first commit
git push -f  # you need to force push because you edited a commit

When done please re-request review with this button
re-request-review-comment

Thanks!

Copy link

netlify bot commented Apr 28, 2024

Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit 6f3a225
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/662e7e0ebcb9cb000830585b
😎 Deploy Preview https://deploy-preview-2326--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.

Copy link

netlify bot commented Apr 28, 2024

Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit cb87f57
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/667072adec8ef10008b27db5
😎 Deploy Preview https://deploy-preview-2326--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.

@lambdanis
Copy link
Contributor

@itsCheithanya I see you merged the upstream main branch into your branch, could you rebase instead? Also your commit needs to be signed-off, see https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#dev-coo

@lambdanis lambdanis marked this pull request as draft May 6, 2024 12:27
@lambdanis
Copy link
Contributor

I marked this PR a draft for now, @itsCheithanya please click the "Ready for review" button when you fix the commits.

@lambdanis lambdanis removed their assignment May 22, 2024
@itsCheithanya itsCheithanya marked this pull request as ready for review June 9, 2024 11:40
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.

This needs to be updated to reflect the recent changes in the Helm values. Sorry for the mess :)

Could you also amend the commit message to be more precise? We're not avoiding CRD creation here, only avoiding unnecessary permissions. A commit message like "Don't give operator permissions to create CRDs if not needed" would be good.

@@ -26,12 +26,14 @@ rules:
- patch
- update
- watch
{{- if not .Values.tetragonOperator.skipCRDCreation }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{{- if not .Values.tetragonOperator.skipCRDCreation }}
{{- if eq .Values.crds.installMethod "operator" }}

It needs to be changed as tetragonOperator.skipCRDCreation value got deprecated & removed in the meantime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I've done the changes as per that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @itsCheithanya I don't see new changes, did you forget to push maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ohhh by mistake i pushed it to my main branch and not 'cheithanya' branch
i will do now,sorry for that

Copy link
Contributor

Choose a reason for hiding this comment

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

looks good now, 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.

@itsCheithanya The CI job is failing because the sign-off line in the commit message doesn't match the commit author:

Missing Signed-off-by: line by nominal patch author 'itsCheithanya <[email protected]>'

https://github.com/cilium/tetragon/actions/runs/9552330690/job/26365160167?pr=2326

Could you amend the commit so that the sign-off matches the commit author?

@lambdanis lambdanis changed the title Don't give operator permissions to create CRDs if not needed ,add if … helm: Don't give operator permissions to create CRDs if not needed Jul 12, 2024
@lambdanis
Copy link
Contributor

Thanks!

@lambdanis lambdanis merged commit 21ae8bc into cilium:main Jul 12, 2024
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Related to the Helm chart release-note/minor This PR introduces a minor user-visible change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't give operator permissions to create CRDs if not needed
2 participants