Skip to content

Conversation

@mlajkim
Copy link
Contributor

@mlajkim mlajkim commented Aug 24, 2025

Warning

Do NOT merge this PR as this PR is only intended for the sample code

Background

The current SIA agent fetches a new certificate from ZTS before attempting to write it to the filesystem. If the file write operation fails for any reason (e.g., incorrect permissions, non-existent directory, full disk), the agent enters a state of inconsistency: the new certificate exists in memory, but the old one remains on disk.

This inconsistency prevents future certificate renewals and requires complex manual intervention, such as deleting files and hard-rebooting the instance, to recover the agent.

What's done?

This PR introduces a robust pre-verification check that runs before the agent requests a certificate from ZTS. This check ensures that the followings are writable:

  • X.509 Cert File Path
  • X.509 Key Path

This PR does not include the following:

  • RoleCert
  • AccessTokens
  • RoleTokens

If file path is not ready, it will output the following as a log:

parent directory does not exist: /var/run/athenz (TODO: Sample Log)

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

@mlajkim mlajkim self-assigned this Aug 24, 2025
@mlajkim mlajkim added the enhancement New feature or request label Aug 24, 2025
@mlajkim mlajkim requested a review from Copilot August 24, 2025 04:53
@mlajkim mlajkim marked this pull request as draft August 24, 2025 04:53
@mlajkim mlajkim changed the title Prototype: Validate File Path [DNM] Prototype: Validate File Path Aug 24, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces file path validation functionality to ensure that certificate and key file paths are writable before attempting to use them. The implementation adds a utility function to validate file paths and integrates it into the certificate service workflow.

  • Adds a new utility function ValidateFilePath to check if file paths are writable
  • Integrates path validation into the certificate service's run cycle
  • Creates a validator function to check all certificate-related file paths

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
pkg/util/validate-file-path.go New utility function for validating file path writability
pkg/certificate/service.go Integration of path validation into the service run cycle
pkg/certificate/path-validator.go Validator function that checks all certificate-related paths

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@codecov-commenter
Copy link

codecov-commenter commented Aug 24, 2025

Codecov Report

❌ Patch coverage is 0% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 9.41%. Comparing base (cdb74dd) to head (c535e89).

Files with missing lines Patch % Lines
pkg/certificate/path-validator.go 0.00% 14 Missing ⚠️
pkg/util/validate-file-path.go 0.00% 12 Missing ⚠️
pkg/certificate/service.go 0.00% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##            main    #196      +/-   ##
========================================
- Coverage   9.50%   9.41%   -0.09%     
========================================
  Files         34      36       +2     
  Lines       3326    3356      +30     
========================================
  Hits         316     316              
- Misses      2988    3018      +30     
  Partials      22      22              
Flag Coverage Δ
unittests 9.41% <0.00%> (-0.09%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mlajkim mlajkim mentioned this pull request Aug 24, 2025
2 tasks
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants