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

Added cert-controller for easy webhook cert generation #319

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

p-strusiewiczsurmacki-mobica
Copy link
Contributor

This PR implements #318

Certificates can be now generated and injected automatically using open-policy-agent/cert-controller.

Copy link
Contributor

@chez-shanpu chez-shanpu left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!

I commented to several lines. Please check :)

Copy link
Contributor

@chez-shanpu chez-shanpu left a comment

Choose a reason for hiding this comment

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

I left some comments.
And AFAIU, cert-controller isn't tested in e2e on github workflows.
Could you add tests if it is not?

@@ -46,6 +46,8 @@ spec:
command: ["coil-egress-controller"]
args:
- --zap-stacktrace-level=panic
# [CERTS] Following line should be uncommented if automatic cert generation is used.
# - --enable-cert-rotation=true
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have to comment on --enable-restart-on-cert-refresh?

And I would like to enable/disable this option with v2/kustomization.yaml like config/pod/compat_calico.yaml do.

patchesStrategicMerge:
# Uncomment the following if you want to run Coil with Calico network policy.
#- config/pod/compat_calico.yaml

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've moved as much of the kustomization to separate patch files as possible, I think.

As for the --enable-restart-on-cert-refresh - I don-t think it is required here. This is mostly added for esthetic purposes - without restart cert-controller might emit some errors into the logs. The errors are recoverable so they don;t break anything, just don't look nice in the logs. I am currently trying to push a small PR to fix this.

Comment on lines 15 to 23
- apiGroups:
- ""
resources:
- secrets
verbs:
- get
- list
- update
- watch
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this rule is only needed when cert-controller is enabled, so it is better to make it a separate file and patch it as needed using v2/kustomization.yaml.

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've moved this to separate patch file. However, this is generated with kubebuilder so I also had to remove the check of this file (git diff) from make check-generate target. Contributors will also have to be careful to not commit the generated file.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about making a new role resource file for a cert-controller with controller-gen? And apply it (and its role-binding) when make enable-certs-generation is executed by using Kustomize's resource.
I think this way, we can check with git diff.

@p-strusiewiczsurmacki-mobica p-strusiewiczsurmacki-mobica force-pushed the add-cert-controller branch 7 times, most recently from 1bb8127 to 9061c02 Compare March 26, 2025 13:37
@p-strusiewiczsurmacki-mobica
Copy link
Contributor Author

And AFAIU, cert-controller isn't tested in e2e on github workflows. Could you add tests if it is not?

I've added a make enable-certs-generation target that configures the kustomization files and added the tests to the CI.

@p-strusiewiczsurmacki-mobica p-strusiewiczsurmacki-mobica force-pushed the add-cert-controller branch 3 times, most recently from 2efc178 to 11e3afb Compare March 26, 2025 14:05
Copy link
Contributor

@chez-shanpu chez-shanpu left a comment

Choose a reason for hiding this comment

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

I replied to your comment and left a few minor comment :)

Comment on lines 15 to 23
- apiGroups:
- ""
resources:
- secrets
verbs:
- get
- list
- update
- watch
Copy link
Contributor

Choose a reason for hiding this comment

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

How about making a new role resource file for a cert-controller with controller-gen? And apply it (and its role-binding) when make enable-certs-generation is executed by using Kustomize's resource.
I think this way, we can check with git diff.

@p-strusiewiczsurmacki-mobica p-strusiewiczsurmacki-mobica force-pushed the add-cert-controller branch 2 times, most recently from 7b08f0f to 336113a Compare March 28, 2025 12:25
@p-strusiewiczsurmacki-mobica
Copy link
Contributor Author

@chez-shanpu

How about making a new role resource file for a cert-controller with controller-gen? And apply it (and its role-binding) when make enable-certs-generation is executed by using Kustomize's resource.
I think this way, we can check with git diff.

Done :) Let me know what you think!

Signed-off-by: Patryk Strusiewicz-Surmacki <[email protected]>

Co-authored-by: Tomoki Sugiura <[email protected]>
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.

2 participants