Skip to content

[deploy] Fetch cert_check certs from the bucket region - #4393

Merged
imnasnainaec merged 2 commits into
masterfrom
fix-cert-check-s3-region
Oct 22, 2026
Merged

imnasnainaec merged 2 commits into
masterfrom
fix-cert-check-s3-region

Conversation

@imnasnainaec

@imnasnainaec imnasnainaec commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator

Follow-up to #4387.

Summary

The cert buckets are in us-east-2, but the AWS_DEFAULT_REGION secret is a different region. The CLI's redirect to us-east-2 was blocked by the harden-runner egress policy, so cert_check has failed on every run. It now requests us-east-2 directly.

 allowed-endpoints:
-  s3.${{ secrets.AWS_DEFAULT_REGION }}.amazonaws.com:443
+  s3.us-east-2.amazonaws.com:443
   sts.${{ secrets.AWS_DEFAULT_REGION }}.amazonaws.com:443

 Check the certs in S3
+  export AWS_REGION=us-east-2
   aws s3 cp ...

Evidence

  • Before: every scheduled run fails to download, before any cert is checked (latest run):
    endpoint called ip address:port 16.15.253.144:443, domain: s3.***.amazonaws.com.
    endpoint called ip address:port 54.185.253.63:443, domain: s3.us-east-2.amazonaws.com.
    fatal error: Could not connect to the endpoint URL: "https://s3.us-east-2.amazonaws.com/prod.thecombine.app/certs/nuc1-thecombine-app-tls/cert.pem"
    
    Both buckets report their region:
    $ curl -sI https://s3.amazonaws.com/prod.thecombine.app | grep -i region
    x-amz-bucket-region: us-east-2
    $ curl -sI https://s3.amazonaws.com/local.thecombine.app | grep -i region
    x-amz-bucket-region: us-east-2
    
    After: a workflow_dispatch run on this branch (run) downloads and checks all four certs, after prod renewed them (see comment below):
    prod.thecombine.app/certs/nuc1-thecombine-app-tls: notAfter=Jan 14 18:48:50 2027 GMT
    prod.thecombine.app/certs/nuc2-thecombine-app-tls: notAfter=Jan 14 18:48:51 2027 GMT
    prod.thecombine.app/certs/nuc3-thecombine-app-tls: notAfter=Jan 14 18:48:52 2027 GMT
    local.thecombine.app/certs/local-thecombine-app-tls: notAfter=Jan 14 18:48:52 2027 GMT
    

Merge Danger

Door: two-way

Blast Radius: none

Only the scheduled cert_check workflow changes. It reads from S3 and has no effect on deployments.

🤖 Generated with Claude Code


This change is Reviewable

Summary by CodeRabbit

  • Chores
    • Updated automated certificate checks to use the bucket’s configured region, helping them work reliably when the default AWS region differs.

The cert buckets are in us-east-2, but the AWS_DEFAULT_REGION secret is
another region. The CLI's redirect to s3.us-east-2.amazonaws.com was
blocked by the harden-runner egress policy, so every run failed to
download the certs. Request us-east-2 directly and allow that endpoint.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 6398e922-b56a-4aa9-a177-127b717bc0f7
📥 Commits

Reviewing files that changed from the base of the PR and between 1f1c80d and 6df8d27.

📒 Files selected for processing: 1
  • .github/workflows/cert_check.yml

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

The certificate-check workflow now allows S3 access through the us-east-2 endpoint and sets AWS_REGION=us-east-2 before AWS CLI commands.

Changes: Certificate check region

Layer / File(s) Summary
Set the S3 region for certificate checks
.github/workflows/cert_check.yml
The Harden Runner allowlist uses the fixed us-east-2 S3 endpoint. The certificate-check script sets AWS_REGION=us-east-2 before running AWS CLI commands.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~7 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 6df8d

The certificate-check downloads use the allowed us-east-2 S3 endpoint. No actionable merge risk remains.

Pre-merge checks: Passed 5
Check name Status Explanation
Docstring Coverage Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly identifies the main change: fetching cert_check certificates from the S3 bucket region. It is concise and specific.
Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.)

✨ Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the github_actions Pull requests that update GitHub Actions code label Oct 16, 2026
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@imnasnainaec
imnasnainaec force-pushed the fix-cert-check-s3-region branch from e357686 to 6df8d27 Compare October 16, 2026 19:35
@imnasnainaec imnasnainaec added bug Something isn't working 🟥High High-priority PR: please review this asap! labels Oct 16, 2026
@imnasnainaec imnasnainaec self-assigned this Oct 16, 2026
@imnasnainaec
imnasnainaec marked this pull request as ready for review October 16, 2026 19:38
@codecov

codecov Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.26%. Comparing base (1f1c80d) to head (6df8d27).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4393   +/-   ##
=======================================
  Coverage   76.26%   76.26%           
=======================================
  Files         305      305           
  Lines       11452    11452           
  Branches     1409     1409           
=======================================
  Hits         8734     8734           
  Misses       2324     2324           
  Partials      394      394           
Flag Coverage Δ
backend 87.55% <ø> (ø)
frontend 67.09% <ø> (ø)

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

☔ View full report in Codecov by Harness.
📢 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@imnasnainaec

Copy link
Copy Markdown
Collaborator Author

After deploying v3.0.1, the existing NUC Certificates kept renewBefore: 1440h even though their ingresses had the new renew-before-percentage: "92" annotation. In cert-manager v1.18.1, ingress-shim doesn't compare the renewal fields when it updates an existing Certificate. One-time fix:

for c in local nuc1 nuc2 nuc3; do
  kubectl --context <prod-context> -n combine-cert-proxy patch certificate "$c-thecombine-app-tls" \
    --type merge -p '{"spec":{"renewBefore":null,"renewBeforePercentage":92}}'
done

cert-manager reissued all four, monitor.py pushed them to S3, and cert_check passed on this branch.

@jasonleenaylor jasonleenaylor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:LGTM:

@jasonleenaylor reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on imnasnainaec).

@imnasnainaec
imnasnainaec merged commit d49d2e4 into master Oct 22, 2026
21 of 22 checks passed
@imnasnainaec
imnasnainaec deleted the fix-cert-check-s3-region branch October 22, 2026 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working github_actions Pull requests that update GitHub Actions code 🟥High High-priority PR: please review this asap!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants