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

Adding certificate validity duration in Kafka user #980

Merged

Conversation

shubhamcoc
Copy link
Contributor

@shubhamcoc shubhamcoc commented May 24, 2023

Description

This MR is to add duration in Kafka user spec. It allows user to specify validity time for certificate created by cert manager.
It helps the user to create user certificate for longer duration than the default value of cert manager. #964

Type of Change

  • New Feature

Checklist

  • I have read the contributing guidelines
  • Existing issues have been referenced (where applicable)
  • All code style checks pass
  • New code contribution is covered by automated tests
  • All new and existing tests pass

@shubhamcoc shubhamcoc requested a review from a team as a code owner May 24, 2023 06:47
Copy link
Contributor

@bartam1 bartam1 left a comment

Choose a reason for hiding this comment

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

We also have to use the validation duration for the k8s CSR object:

@shubhamcoc
Copy link
Contributor Author

We also have to use the validation duration for the k8s CSR object:

Yes one validation is required as we have 2 different combination available. One for cert-manager with 1 hour as minimum duration and another with k8s CSR object with 10 mins as minimum duration.

@shubhamcoc shubhamcoc force-pushed the cert_duration_implementation branch 3 times, most recently from 92f9550 to 5f6d996 Compare May 26, 2023 11:39
@shubhamcoc shubhamcoc closed this May 26, 2023
@shubhamcoc shubhamcoc reopened this May 26, 2023
@shubhamcoc shubhamcoc force-pushed the cert_duration_implementation branch from 5f6d996 to ffb1fab Compare June 9, 2023 19:08
Copy link
Member

@panyuenlau panyuenlau left a comment

Choose a reason for hiding this comment

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

Can we also extend the existing tests to include the change in this PR?

func TestReconcileUserCertificate(t *testing.T) {

func TestReconcileUserCertificate(t *testing.T) {

Comment on lines +270 to +271
case banzaiv1alpha1.OperationStatus:
err = errors.NewWithDetails("Cruise Control operation not supported", "name", ccOperationExecution.GetName(), "namespace", ccOperationExecution.GetNamespace(), "operation", ccOperationExecution.CurrentTaskOperation(), "parameters", ccOperationExecution.CurrentTaskParameters())
Copy link
Member

Choose a reason for hiding this comment

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

Note: these two lines are not related to the PR and they are copied from #974 to make linter not complain the missing case in the switch statement.

@shubhamcoc
Copy link
Contributor Author

Can we also extend the existing tests to include the change in this PR?

func TestReconcileUserCertificate(t *testing.T) {

func TestReconcileUserCertificate(t *testing.T) {

Added it. Kindly review it

@shubhamcoc shubhamcoc force-pushed the cert_duration_implementation branch from 3b1dfb6 to 2e5da3b Compare June 12, 2023 16:02
@panyuenlau panyuenlau requested a review from bartam1 June 12, 2023 19:07
@panyuenlau panyuenlau merged commit b39e440 into banzaicloud:master Jun 13, 2023
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.

3 participants