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

fix certreloader update logic #111

Merged
merged 8 commits into from
Mar 29, 2024
Merged

Conversation

t4niwa
Copy link
Contributor

@t4niwa t4niwa commented Mar 22, 2024

Description

panic occurred because cert reloader attempted to update even though certificate retrieval failed

This PR adds the red box in the following diagram:
image

Type of change

  • Bug fix
  • New feature
  • Refactoring (no functional changes, no api changes)
  • Non-code changes (update documentation, pipeline, etc.)

Flags

  • Breaks backward compatibility
  • Requires a documentation update
  • Has untestable code

Related issue/PR

Delete this section if there are no issues or pull requests that relate to this pull request.

  • Fixes #issue
  • Closes #PR

Checklist

  • Followed the guidelines in the CONTRIBUTING document
  • Added prefix [skip ci]/[ci skip]/[no ci]/[skip actions]/[actions skip] in the PR title if necessary
  • Tested and linted the code
  • Commented the code
  • Made corresponding changes to the documentation
  • Passed all pipeline checking

Checklist for maintainer

  • Use Squash and merge
  • Double-confirm the merge message has prefix [skip ci]/[ci skip]/[no ci]/[skip actions]/[actions skip]
  • Delete the branch after merge

@t4niwa t4niwa force-pushed the fix-certreloader-update branch from 627d67b to 27d95e8 Compare March 22, 2024 10:29
Signed-off-by: taniwa <taniwa@lycorp.co.jp>
@t4niwa t4niwa force-pushed the fix-certreloader-update branch from 27d95e8 to 0b89c12 Compare March 22, 2024 10:30
@mlajkim mlajkim added the bug Something isn't working label Mar 26, 2024
pkg/certificate/service.go Outdated Show resolved Hide resolved
pkg/certificate/service.go Outdated Show resolved Hide resolved
@mlajkim
Copy link
Contributor

mlajkim commented Mar 26, 2024

  • handle
if config.ProviderService == "" && config.CertFile != "" && config.KeyFile != "" {
go r.pollRefresh()
}

mlajkim and others added 3 commits March 27, 2024 09:38
Signed-off-by: taniwa <taniwa@lycorp.co.jp>
Signed-off-by: taniwa <taniwa@lycorp.co.jp>
Copy link
Contributor

@mlajkim mlajkim left a comment

Choose a reason for hiding this comment

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

review done

pkg/certificate/service.go Outdated Show resolved Hide resolved
mlajkim
mlajkim previously approved these changes Mar 27, 2024
Copy link
Contributor

@mlajkim mlajkim left a comment

Choose a reason for hiding this comment

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

lgtm, but I want the following members to approve too.

@mlajkim mlajkim requested review from WindzCUHK and y-myajima March 27, 2024 08:04
y-myajima
y-myajima previously approved these changes Mar 27, 2024
Copy link
Contributor

@y-myajima y-myajima left a comment

Choose a reason for hiding this comment

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

LGTM

mlajkim and others added 2 commits March 27, 2024 18:51
Signed-off-by: taniwa <taniwa@lycorp.co.jp>
@t4niwa t4niwa dismissed stale reviews from y-myajima and mlajkim via 7a8d1f2 March 27, 2024 09:51
@t4niwa t4niwa force-pushed the fix-certreloader-update branch from 4233af8 to 7a8d1f2 Compare March 27, 2024 09:51
pkg/util/tls-reload.go Outdated Show resolved Hide resolved
@t4niwa t4niwa force-pushed the fix-certreloader-update branch from 5ce8380 to 270c3fb Compare March 27, 2024 10:03
pkg/util/tls-reload.go Outdated Show resolved Hide resolved
@t4niwa t4niwa force-pushed the fix-certreloader-update branch from 99c1812 to 8adeac9 Compare March 28, 2024 02:14
Signed-off-by: taniwa <taniwa@lycorp.co.jp>
@t4niwa t4niwa force-pushed the fix-certreloader-update branch from 8adeac9 to a50f2ab Compare March 28, 2024 02:15
@mlajkim
Copy link
Contributor

mlajkim commented Mar 28, 2024

Probably do the similar changes as @t4niwa you've done in 0b89c12

BUT, make sure not to use err = ... but instead use a different name not to override the errSomething := ...

  • Do the task above
  • Make a PR (or issue) that brings the reload before the roleCertProvisioning

Copy link
Contributor

@mlajkim mlajkim left a comment

Choose a reason for hiding this comment

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

ok

@mlajkim mlajkim merged commit 2c8f242 into AthenZ:main Mar 29, 2024
8 checks passed
y-myajima pushed a commit that referenced this pull request Sep 3, 2024
* fix certreloader update

Signed-off-by: taniwa <taniwa@lycorp.co.jp>

* update tls-reload

Signed-off-by: taniwa <taniwa@lycorp.co.jp>

* log: small (#112)

* revert update cert reloader logic

Signed-off-by: taniwa <taniwa@lycorp.co.jp>

* rollback tls-reload

Signed-off-by: taniwa <taniwa@lycorp.co.jp>

---------

Signed-off-by: taniwa <taniwa@lycorp.co.jp>
Co-authored-by: Aaron Jeongwoo Kim <53258958+mlajkim@users.noreply.github.com>
Signed-off-by: myajima <myajima@lycorp.co.jp>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants