Skip to content

refresh certs if needed in reconciler #81

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Jun 30, 2023
Merged

refresh certs if needed in reconciler #81

merged 9 commits into from
Jun 30, 2023

Conversation

acpana
Copy link
Contributor

@acpana acpana commented Jun 8, 2023 •
edited
Loading

Basically following Max's advice here: open-policy-agent/gatekeeper#2477 (comment) to call refresh certs if needed in the reconciler.

I also ran gofumpt, gci and godot on the files.

@acpana acpana requested a review from maxsmythe June 8, 2023 17:39
@codecov-commenter
Copy link

codecov-commenter commented Jun 8, 2023 •
edited
Loading

Codecov Report

Patch coverage: 70.58% and project coverage change: +2.40 🎉

Comparison is base (3b09cd3) 51.78% compared to head (299d680) 54.19%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master      #81      +/-   ##
==========================================
+ Coverage   51.78%   54.19%   +2.40%     
==========================================
  Files           1        1              
  Lines         531      548      +17     
==========================================
+ Hits          275      297      +22     
+ Misses        190      185       -5     
  Partials       66       66
Flag Coverage Δ
unittests 54.19% <70.58%> (+2.40%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/rotator/rotator.go 54.19% <70.58%> (+2.40%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

maxsmythe
maxsmythe reviewed Jun 9, 2023
View reviewed changes
Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

For testing, one way would be to create a mode that enables only the controller, not the rotator (just don't add the rotator to the manager), then there is no race condition.

@acpana
adds nonBP mode
275c6b2 
Signed-off-by: Alex Pana <[email protected]>
@acpana acpana requested a review from maxsmythe June 13, 2023 19:20
@acpana
Copy link
Contributor Author

acpana commented Jun 13, 2023

For testing, one way would be to create a mode that enables only the controller, not the rotator (just don't add the rotator to the manager), then there is no race condition.

@maxsmythe thanks for the suggestion! I added a field on the CertRotator to control whether or not to run in it the background for a reconciler. Let me know what you think 🙏🏼

@acpana
dont export test knob
c4773c2 
Signed-off-by: Alex Pana <[email protected]>
maxsmythe
maxsmythe reviewed Jun 13, 2023
View reviewed changes
acpana added 3 commits June 15, 2023 00:59
@acpana
e2e for ca expiry
cf1d6ac 
Signed-off-by: Alex Pana <[email protected]>
@acpana
review: cmmt reword, return err
281aec8 
Signed-off-by: Alex Pana <[email protected]>
@acpana
fix test for caDuration
c39c066 
Signed-off-by: Alex Pana <[email protected]>
@acpana acpana requested a review from maxsmythe June 15, 2023 01:41
maxsmythe
maxsmythe reviewed Jun 20, 2023
View reviewed changes
Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

LGTM after removal of requeue

acpana
acpana commented Jun 21, 2023
View reviewed changes
@acpana
dont requeue
299d680 
Signed-off-by: Alex Pana <[email protected]>
@acpana acpana requested a review from maxsmythe June 21, 2023 16:17
maxsmythe
maxsmythe approved these changes Jun 21, 2023
View reviewed changes
Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

LGTM

@acpana acpana requested review from ritazh and sozercan June 21, 2023 20:03
ritazh
ritazh reviewed Jun 23, 2023
View reviewed changes
ritazh
ritazh reviewed Jun 23, 2023
View reviewed changes
ritazh
ritazh reviewed Jun 23, 2023
View reviewed changes
ritazh
ritazh reviewed Jun 23, 2023
View reviewed changes
acpana and others added 2 commits June 28, 2023 16:54
@acpana @ritazh
Apply suggestions from code review
274344c 
Co-authored-by: Rita Zhang <[email protected]>
Signed-off-by: Alex Pana <[email protected]>
@acpana
remove commnet
5030b09 
Signed-off-by: Alex Pana <[email protected]>
@acpana acpana force-pushed the acpana/refresh-certs branch from 842a840 to 5030b09 Compare June 28, 2023 16:55
@acpana acpana requested a review from ritazh June 28, 2023 16:55
ritazh
ritazh approved these changes Jun 30, 2023
View reviewed changes
Copy link
Member

@ritazh ritazh left a comment

Choose a reason for hiding this comment

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

LGTM

@acpana acpana merged commit 87e4e2f into open-policy-agent:master Jun 30, 2023
@acpana acpana mentioned this pull request Jul 13, 2023
Merged
@alculquicondor alculquicondor mentioned this pull request Dec 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maxsmythe maxsmythe maxsmythe approved these changes

@ritazh ritazh ritazh approved these changes

@sozercan sozercan Awaiting requested review from sozercan

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@acpana @codecov-commenter @ritazh @maxsmythe