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

Exclude from certificate monitoring if SIA's Certificate File or Ca Cert File Path is Empty #171

Merged
merged 4 commits into from
Dec 2, 2024

Conversation

y-myajima
Copy link
Contributor

@y-myajima y-myajima commented Nov 18, 2024

Description

Fixed an issue where a warning log from the x509-certificate-exporter was unnecessarily output when CA_CERT_FILE was empty at startup.
#134

Assignees

  • Assignees is set

Type of changes

  • Apply one or more labels of the following that fits:
    • bug: Bug fix
    • dependencies: Dependency upgrades
    • documentation: Documentation changes
    • enhancement: New Feature
    • good first issue: First contribution
    • logging: Log changes
    • refactor: Refactoring (no functional changes, no api changes)

Flags

- [ ] Breaks backward compatibility
- [ ] Requires a documentation update
- [ ] Has untestable code

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

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

Signed-off-by: myajima <myajima@lycorp.co.jp>
@y-myajima y-myajima force-pushed the exclude-from-cert-monitoring branch from 0e0bfc2 to 88baa4b Compare November 18, 2024 01:46
@y-myajima y-myajima self-assigned this Nov 18, 2024
@y-myajima y-myajima added bug Something isn't working logging Changes in logger and removed bug Something isn't working labels Nov 18, 2024
@y-myajima y-myajima requested a review from t4niwa November 19, 2024 00:55
pkg/metrics/service.go Outdated Show resolved Hide resolved
@y-myajima y-myajima requested a review from mlajkim December 2, 2024 01:56
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.

  • Consider the following:
    I don't want to make changes in this PR here, but I found out that the current logic where exporter not being able to watch the service-cert or ca-cert just because it couldn't get the rolecert path is a bit over the top. prolly requires a new issue.

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.

@mlajkim
Copy link
Contributor

mlajkim commented Dec 2, 2024

  • This is more of enhancement, correct?

Signed-off-by: myajima <myajima@lycorp.co.jp>
@mlajkim
Copy link
Contributor

mlajkim commented Dec 2, 2024

Created an issue:
#175

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

@mlajkim mlajkim merged commit b29b3c1 into main Dec 2, 2024
8 checks passed
@mlajkim mlajkim deleted the exclude-from-cert-monitoring branch December 2, 2024 03:25
WindzCUHK pushed a commit that referenced this pull request Dec 3, 2024
…ert File Path is Empty (#171)

* exclude from cert monitoring

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

* use anonymous func

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

* only service cert

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

---------

Signed-off-by: myajima <myajima@lycorp.co.jp>
Signed-off-by: wfan <wfan@lycorp.co.jp>
WindzCUHK added a commit that referenced this pull request Dec 3, 2024
…own (#173)

* use config variable

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

* add forceful shutdown logic

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

* fix all shutdown

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

* add comment

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

* (revert later) confirm race resolved

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

* Exclude from certificate monitoring if SIA's Certificate File or Ca Cert File Path is Empty (#171)

* exclude from cert monitoring

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

* use anonymous func

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

* only service cert

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

---------

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

* Revert "(revert later) confirm race resolved"

This reverts commit 2e8bfe6.

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

---------

Signed-off-by: wfan <wfan@lycorp.co.jp>
Signed-off-by: myajima <myajima@lycorp.co.jp>
Co-authored-by: y-myajima <137997379+y-myajima@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request logging Changes in logger
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Must exclude from certificate monitoring if SIA's Certificate File or Ca Cert File Path is Empty
2 participants