METAL-1715: Enforce 1-year validity and 30-day auto-rotation for Ironic TLS certs#570
METAL-1715: Enforce 1-year validity and 30-day auto-rotation for Ironic TLS certs#570mabulgu wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@mabulgu: This pull request references METAL-1715 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
1 similar comment
|
@mabulgu: This pull request references METAL-1715 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@mabulgu: This pull request references METAL-1715 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest |
mabulgu
force-pushed
the
story/METAL-1715
branch
from
f10fb32 to
1176936
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughReduced TLS certificate lifetime from 2 years to 1 year and refresh window from 180 days to 30 days; introduced time-injectable expiration check Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
@mabulgu: This pull request references METAL-1715 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
provisioning/baremetal_secrets_test.go (1)
397-644: Good test coverage, but missing error scenario test.The test cases comprehensively cover various SAN construction scenarios including HyperShift mode, external IPs, API VIPs, and edge cases like empty IPs. However, no test case exercises the
expectError: truepath (e.g., whengetServerInternalIPsfails).Consider adding a test case that triggers the error path in
buildTlsHoststo ensure proper error propagation:🧪 Example error scenario test case
{ name: "error-from-api-server-ips", info: &ProvisioningInfo{ Namespace: "openshift-machine-api", ProvConfig: &metal3iov1alpha1.Provisioning{ Spec: metal3iov1alpha1.ProvisioningSpec{ ProvisioningNetwork: metal3iov1alpha1.ProvisioningNetworkDisabled, }, }, OSClient: fakeconfigclientset.NewSimpleClientset(), // No Infrastructure object }, expectError: true, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provisioning/baremetal_secrets_test.go` around lines 397 - 644, Add a test case to exercise the error path of buildTlsHosts by creating a ProvisioningInfo that forces getServerInternalIPs to fail (e.g., ProvConfig.Spec.ProvisioningNetwork = metal3iov1alpha1.ProvisioningNetworkDisabled and OSClient set to an empty fakeconfigclientset with no Infrastructure object) and set expectError: true; this new case should reference buildTlsHosts and ProvisioningInfo so the test asserts an error is returned instead of comparing SANs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@provisioning/baremetal_secrets_test.go`:
- Around line 397-644: Add a test case to exercise the error path of
buildTlsHosts by creating a ProvisioningInfo that forces getServerInternalIPs to
fail (e.g., ProvConfig.Spec.ProvisioningNetwork =
metal3iov1alpha1.ProvisioningNetworkDisabled and OSClient set to an empty
fakeconfigclientset with no Infrastructure object) and set expectError: true;
this new case should reference buildTlsHosts and ProvisioningInfo so the test
asserts an error is returned instead of comparing SANs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db50c48b-5ff7-4f89-9938-cc34ec1e597e
📒 Files selected for processing (5)
provisioning/baremetal_crypto.goprovisioning/baremetal_crypto_test.goprovisioning/baremetal_secrets.goprovisioning/baremetal_secrets_test.goprovisioning/utils.go
mabulgu
force-pushed
the
story/METAL-1715
branch
from
1176936 to
b71de7a
Compare
|
@mabulgu: This pull request references METAL-1715 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
provisioning/baremetal_crypto_test.go (1)
305-305: UsetlsExpiration/tlsRefreshconstants in tests to avoid policy drift.Line 305 and the table cases hardcode 365/30/180-day values that are already defined in production constants. Reusing those constants will keep tests aligned if policy changes again.
Suggested refactor
- expectedValidity := 365 * 24 * time.Hour + expectedValidity := tlsExpiration @@ - certLifetime: 29 * 24 * time.Hour, + certLifetime: tlsRefresh - 24*time.Hour, @@ - certLifetime: 30 * 24 * time.Hour, + certLifetime: tlsRefresh, @@ - certLifetime: 31 * 24 * time.Hour, + certLifetime: tlsRefresh + 24*time.Hour, @@ - certLifetime: 180 * 24 * time.Hour, + certLifetime: tlsRefresh + 150*24*time.Hour, @@ - certLifetime: 365 * 24 * time.Hour, + certLifetime: tlsExpiration,As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 319-340
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provisioning/baremetal_crypto_test.go` at line 305, The test hardcodes duration values (e.g., expectedValidity := 365 * 24 * time.Hour and several table-case values) which should instead reference the production constants tlsExpiration and tlsRefresh to avoid policy drift; update expectedValidity and the affected table cases to use tlsExpiration and tlsRefresh (or tlsRefresh/ other named constants) directly, making sure to respect their types (if they are time.Duration use them as-is; if they are day counts multiply by time.Hour*24) and run the test to verify types align.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@provisioning/baremetal_crypto_test.go`:
- Around line 323-326: The test relies on wall-clock timing and doesn't exercise
the exact equality boundary used by isTlsCertificateExpired (which checks
NotAfter.Before(refreshAfter)), so change the expiry check to accept an injected
reference time (add a now/time parameter to isTlsCertificateExpired or overload
it) and use that injected now in computing refreshAfter; then update the tests
"rotation-triggered-at-30-days" and the similar case around lines 349-352 in
provisioning/baremetal_crypto_test.go to pass a deterministic now value and
explicitly assert the equality case (now == tlsRefresh) and the before/after
cases to verify strict '<' behavior. Ensure you update all callers of
isTlsCertificateExpired (or provide a wrapper) so production call sites default
to time.Now() while tests supply the fixed timestamp.
---
Nitpick comments:
In `@provisioning/baremetal_crypto_test.go`:
- Line 305: The test hardcodes duration values (e.g., expectedValidity := 365 *
24 * time.Hour and several table-case values) which should instead reference the
production constants tlsExpiration and tlsRefresh to avoid policy drift; update
expectedValidity and the affected table cases to use tlsExpiration and
tlsRefresh (or tlsRefresh/ other named constants) directly, making sure to
respect their types (if they are time.Duration use them as-is; if they are day
counts multiply by time.Hour*24) and run the test to verify types align.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 647af12d-8322-45e8-b853-538d8bbb99ba
📒 Files selected for processing (2)
provisioning/baremetal_crypto.goprovisioning/baremetal_crypto_test.go
✅ Files skipped from review due to trivial changes (1)
- provisioning/baremetal_crypto.go
|
/refresh-required |
|
/test e2e-agnostic-ovn |
The Ironic TLS certificate was previously valid for 2 years with rotation triggering at 180 days before expiration. Per METAL-1687, update these to 1-year validity and 30-day rotation window. This uses the existing custom rotation mechanism (Option C from the spike) which was already sound — only the timing constants needed adjustment. Addresses METAL-1715. Made-with: Cursor
mabulgu
force-pushed
the
story/METAL-1715
branch
from
b71de7a to
1cd18e7
Compare
|
/test e2e-metal-ipi-ovn-ipv6 |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: elfosardo, mabulgu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/lgtm |
|
/verified later @jadhaj |
openshift-ci-robot
added
verified-later
verified
|
@jadhaj: This PR has been marked to be verified later by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@mabulgu: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
openshift-ci
bot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
6 participants
Summary
Background
Per METAL-1687, the Ironic TLS certificate must be valid for exactly 1 year and automatically rotated 30 days before expiration. The METAL-1713 spike confirmed the existing custom rotation mechanism (Option C) is already sound — only the timing constants needed adjustment.
tlsExpirationtlsRefreshDependencies
This PR depends on #569 (METAL-1714: Populate complete SANs in Ironic TLS certificate). Please merge that PR first, then this branch will need a rebase.
Test plan
TestCertificateValidityPeriodverifies generated certificates are valid for ~365 daysTestCertificateRotationBoundarycovers 5 scenarios: rotation triggers at 29 and 30 days before expiration, no rotation at 31, 180, or 365 daysgo test ./...passesAddresses: METAL-1715
Assisted-by: Claude 4.6 Opus High