OCPBUGS-57049: cert inspection: add an option to humanize refresh period annotation #1981

vrutkovs · 2025-07-24T06:56:43Z

This option converts time.Parse Duration-compatible string of duration (i.e. 77h30m00s) into
human-readable string (3d5h30m) so that TLS registry markdowns have improved readability

openshift-ci-robot · 2025-07-24T06:56:48Z

@vrutkovs: This pull request references Jira Issue OCPBUGS-57049, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.20.0) matches configured target version for branch (4.20.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @wangke19

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

This option converts time.Parse Duration-compatible string of duration (i.e. 77h30m00s) into
human-readable string (3d5h30m) so that TLS registry markdowns have improved readability

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.

p0lyn0mial

lgtm

p0lyn0mial · 2025-07-24T07:10:49Z

pkg/certs/cert-inspection/certgraphanalysis/metadata_options.go

 			return timestampReg.ReplaceAllString(path, "<timestamp>.pem")
 		},
 	}
+	HumanizeRewritePeriod = &metadataOptions{


ok, and how is this var being used ? by external tools that pull this library ?

origin would run this function to update secrets/configmap annotation when raw TLS data is collected

nit: how about changing the name to RewriteRefreshPeriod or ReformatRefreshPeriod or TransformRefreshPeriod ?

Yeah, lets go with Rewrite - its more similar to what other functions are named

p0lyn0mial · 2025-07-24T07:15:59Z

pkg/certs/cert-inspection/certgraphanalysis/metadata_options.go

+				return
+			}
+			if period, ok := secret.Annotations[certrotation.CertificateRefreshPeriodAnnotation]; ok {
+				if d, err := time.ParseDuration(period); err == nil {


nit: i would change to: (reads a bit easier)

d, err := time.ParseDuration(period) if err != nil { fmt.Fprintf(os.Stderr, "Failed to parse rewrite period %s: %v\n", period, err) return } humanReadableDate := durationToHumanReadableString(d) secret.Annotations[certrotation.CertificateRefreshPeriodAnnotation] = humanReadableDate secret.Annotations[rewritePrefix+"HumanizeRewritePeriod"] = period

p0lyn0mial · 2025-07-24T07:16:15Z

pkg/certs/cert-inspection/certgraphanalysis/metadata_options.go

+				return
+			}
+			if period, ok := configMap.Annotations[certrotation.CertificateRefreshPeriodAnnotation]; ok {
+				if d, err := time.ParseDuration(period); err == nil {


p0lyn0mial · 2025-07-24T07:25:31Z

pkg/certs/cert-inspection/certgraphanalysis/metadata_options.go

+				if d, err := time.ParseDuration(period); err == nil {
+					humanReadableDate := durationToHumanReadableString(d)
+					secret.Annotations[certrotation.CertificateRefreshPeriodAnnotation] = humanReadableDate
+					secret.Annotations[rewritePrefix+"HumanizeRewritePeriod"] = period


shouldn't this be rewritePrefix+certrotation.CertificateRefreshPeriodAnnotation ?

This annotation stores original value we may need for debugging. It should include the name of the function which did the rewrite (it's expected that the function would rewrite just one annotation, so it's more beneficial to save the function name instead of the annotation name, so that the source of the error could be found easier)

pkg/certs/cert-inspection/certgraphanalysis/metadata_options.go

wangke19 · 2025-07-25T07:21:30Z

pkg/certs/cert-inspection/certgraphanalysis/metadata_options.go

+	u := uint64(d)
+	if d < 0 {
+		u = -u
+	}
+
+	if u == 0 {
+		return "0s"
+	}


Suggested change

u := uint64(d)

if d < 0 {

u = -u

}

if u == 0 {

return "0s"

}

if d == 0 {

return "0s"

}

// Handle negative durations by taking absolute value

u := d

if d < 0 {

u = -d

}

I like the idea of checking for d == 0 first, but we also still want to do u := uint64(d) first to make sure its converted from the start

Actually, we don't need to store values as uint64 - it's best to be rounded to seconds

wangke19 · 2025-07-25T07:22:20Z

pkg/certs/cert-inspection/certgraphanalysis/metadata_options.go

+	year := uint64(time.Hour) * 24 * 365
+	month := uint64(time.Hour) * 24 * 30
+	day := uint64(time.Hour) * 24
+	hour := uint64(time.Hour)
+	minute := uint64(time.Minute)
+	second := uint64(time.Second)


Suggested change

year := uint64(time.Hour) * 24 * 365

month := uint64(time.Hour) * 24 * 30

day := uint64(time.Hour) * 24

hour := uint64(time.Hour)

minute := uint64(time.Minute)

second := uint64(time.Second)

totalSeconds := uint64(u.Seconds())

// Define time units in seconds

const (

year = 365 * 24 * 3600

month = 30 * 24 * 3600

day = 24 * 3600

hour = 3600

minute = 60

second = 1

)

wangke19 · 2025-07-25T07:23:33Z

pkg/certs/cert-inspection/certgraphanalysis/metadata_options.go

+	years := u / year
+	u %= year
+	if years > 0 {
+		b.WriteString(strconv.FormatUint(years, 10))
+		b.WriteString("y")
+	}
+
+	months := u / month
+	u %= month
+	if months > 0 {
+		b.WriteString(strconv.FormatUint(months, 10))
+		b.WriteString("mo")
+	}
+
+	days := u / day
+	u %= day
+	if days > 0 {
+		b.WriteString(strconv.FormatUint(days, 10))
+		b.WriteString("d")
+	}
+
+	hours := u / hour
+	u %= hour
+	if hours > 0 {
+		b.WriteString(strconv.FormatUint(hours, 10))
+		b.WriteString("h")
+	}
+
+	minutes := u / minute
+	u %= minute
+	if minutes > 0 {
+		b.WriteString(strconv.FormatUint(minutes, 10))
+		b.WriteString("m")
+	}
+
+	seconds := u / second
+	u %= second
+	if seconds > 0 {
+		b.WriteString(strconv.FormatUint(seconds, 10))
+		b.WriteString("s")
+	}
+
+	if b.Len() == 0 {
+		return "0s"
+	}


Suggested change

years := u / year

u %= year

if years > 0 {

b.WriteString(strconv.FormatUint(years, 10))

b.WriteString("y")

}

months := u / month

u %= month

if months > 0 {

b.WriteString(strconv.FormatUint(months, 10))

b.WriteString("mo")

}

days := u / day

u %= day

if days > 0 {

b.WriteString(strconv.FormatUint(days, 10))

b.WriteString("d")

}

hours := u / hour

u %= hour

if hours > 0 {

b.WriteString(strconv.FormatUint(hours, 10))

b.WriteString("h")

}

minutes := u / minute

u %= minute

if minutes > 0 {

b.WriteString(strconv.FormatUint(minutes, 10))

b.WriteString("m")

}

seconds := u / second

u %= second

if seconds > 0 {

b.WriteString(strconv.FormatUint(seconds, 10))

b.WriteString("s")

}

if b.Len() == 0 {

return "0s"

}

years := u / year

writeUnit := func(value uint64, suffix string) {

if value > 0 {

b.WriteString(strconv.FormatUint(value, 10))

b.WriteString(suffix)

}

}

years := totalSeconds / year

totalSeconds %= year

writeUnit(years, "y")

months := totalSeconds / month

totalSeconds %= month

writeUnit(months, "mo")

days := totalSeconds / day

totalSeconds %= day

writeUnit(days, "d")

hours := totalSeconds / hour

totalSeconds %= hour

writeUnit(hours, "h")

minutes := totalSeconds / minute

totalSeconds %= minute

writeUnit(minutes, "m")

seconds := totalSeconds

writeUnit(seconds, "s")

wangke19 · 2025-07-25T07:25:57Z

pkg/certs/cert-inspection/certgraphanalysis/metadata_test.go

+		{time.Hour * 24 * 365 * 9, "9y"},
+		{time.Hour * 24 * 365, "1y"},
+		{time.Hour * 24 * 30 * 5, "5mo"},
+		{time.Hour * 24 * 30, "1mo"},
+		{time.Hour * 24 * 2, "2d"},
+		{time.Hour * 2, "2h"},
+		{time.Hour*24*3 + time.Hour*2, "3d2h"},
+		{time.Hour*24*5 + time.Hour*4 + time.Minute*25, "5d4h25m"},


covers various cases including zero, positive, negative, and composite durations:

Suggested change

{time.Hour * 24 * 365 * 9, "9y"},

{time.Hour * 24 * 365, "1y"},

{time.Hour * 24 * 30 * 5, "5mo"},

{time.Hour * 24 * 30, "1mo"},

{time.Hour * 24 * 2, "2d"},

{time.Hour * 2, "2h"},

{time.Hour*24*3 + time.Hour*2, "3d2h"},

{time.Hour*24*5 + time.Hour*4 + time.Minute*25, "5d4h25m"},

{0, "0s"},

{time.Second, "1s"},

{2 * time.Second, "2s"},

{time.Minute, "1m"},

{time.Minute + 30*time.Second, "1m30s"},

{time.Hour, "1h"},

{25 * time.Hour, "1d1h"},

{30 * 24 * time.Hour, "1mo"},

{365 * 24 * time.Hour, "1y"},

{400 * 24 * time.Hour, "1y1mo5d"},

{-time.Minute, "1m"}, // negative duration

{-400 * 24 * time.Hour, "1y1mo5d"}, // negative composite

{3*time.Minute + 4*time.Second, "3m4s"},

p0lyn0mial

a few more comments/questions

pkg/certs/cert-inspection/certgraphanalysis/metadata_options.go

p0lyn0mial · 2025-07-25T09:57:03Z

pkg/certs/cert-inspection/certgraphanalysis/metadata_options.go

+const (
+	rewritePrefix = "rewritten.cert-info.openshift.io/"
+	// Unit values in seconds
+	year   = 60 * 60 * 24 * 365


please move this vars inside durationToHumanReadableString so that they are private to that method.

Wouldn't we reinitialize them every time we call a function?

good point, we would, but these var will only be used by that function and it would be nice to keep them inside that function for clarity. i don't think this code must be lightning fast. an alternative would be to create an internal pkg for that function.

ah, okay, sure

p0lyn0mial · 2025-07-25T09:58:28Z

pkg/certs/cert-inspection/certgraphanalysis/metadata_options.go


+func humanizeRefreshPeriodFromMetadata(annotations map[string]string) {
+	if annotations == nil {
+		annotations = map[string]string{}


let's return here, in go a nil map is equivalent to an empty map from which reading is possible.

ah, in that case we don't need this check at all - annotations[certrotation.CertificateRefreshPeriodAnnotation] will guard us. Added a unit test

…n-readable This option converts `time.ParseDuration`-compatible string of duration (i.e. `77h30m00s`) into human-redable string (`3d5h30m`) so that TLS registry markdowns have improved readability

p0lyn0mial · 2025-07-25T10:16:45Z

@wangke19 it looks like we are about to merge this PR. Is there anything else we should take into consideration ?

wangke19 · 2025-07-25T10:19:43Z

@wangke19 it looks like we are about to merge this PR. Is there anything else we should take into consideration ?

lgtm

openshift-ci · 2025-07-25T10:31:04Z

@vrutkovs: all tests passed!

Full PR test history. Your PR dashboard.

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

p0lyn0mial · 2025-07-25T10:32:50Z

/lgtm
/approve

openshift-ci · 2025-07-25T10:33:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: p0lyn0mial, vrutkovs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [p0lyn0mial]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2025-07-25T10:37:40Z

@vrutkovs: Jira Issue OCPBUGS-57049: Some pull requests linked via external trackers have merged:

The following pull requests linked via external trackers have not merged:

openshift/cluster-kube-apiserver-operator#1873 is open

These pull request must merge or be unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with /jira refresh.

Jira Issue OCPBUGS-57049 has not been moved to the MODIFIED state.

In response to this:

This option converts time.Parse Duration-compatible string of duration (i.e. 77h30m00s) into
human-readable string (3d5h30m) so that TLS registry markdowns have improved readability

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.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 24, 2025

openshift-ci-robot added the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Jul 24, 2025

openshift-ci bot requested review from wangke19, hexfusion and p0lyn0mial July 24, 2025 06:56

vrutkovs mentioned this pull request Jul 24, 2025

OCPBUGS-57049: Update cert annotations #1971

Merged

p0lyn0mial reviewed Jul 24, 2025

View reviewed changes

vrutkovs force-pushed the cert-refresh-period-rewrite branch 5 times, most recently from ede4571 to 1920898 Compare July 24, 2025 13:52

vrutkovs mentioned this pull request Jul 25, 2025

OCPBUGS-57049: Bump library-go to vrutkovs:cert-refresh-period-rewrite openshift/origin#30016

Closed

wangke19 reviewed Jul 25, 2025

View reviewed changes

p0lyn0mial reviewed Jul 25, 2025

View reviewed changes

vrutkovs force-pushed the cert-refresh-period-rewrite branch 4 times, most recently from 1aad7e3 to 2bd99a9 Compare July 25, 2025 09:48

p0lyn0mial reviewed Jul 25, 2025

View reviewed changes

vrutkovs force-pushed the cert-refresh-period-rewrite branch from 2bd99a9 to f7d031e Compare July 25, 2025 10:05

cert inspection: add an option to make refresh period annotation huma…

507f6ad

…n-readable This option converts `time.ParseDuration`-compatible string of duration (i.e. `77h30m00s`) into human-redable string (`3d5h30m`) so that TLS registry markdowns have improved readability

vrutkovs force-pushed the cert-refresh-period-rewrite branch from f7d031e to 507f6ad Compare July 25, 2025 10:12

openshift-ci bot assigned p0lyn0mial Jul 25, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 25, 2025

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 25, 2025

openshift-merge-bot bot merged commit 7f9bc3e into openshift:master Jul 25, 2025
4 checks passed

openshift-ci-robot mentioned this pull request Sep 19, 2025

Re-apply "OCPBUGS-57049: certrotation: move test case name outside of AutoRegenerateAfterOfflineExpiry" openshift/cluster-kube-apiserver-operator#1910

Merged

-	year := uint64(time.Hour) * 24 * 365
-	month := uint64(time.Hour) * 24 * 30
-	day := uint64(time.Hour) * 24
-	hour := uint64(time.Hour)
-	minute := uint64(time.Minute)
-	second := uint64(time.Second)
+	totalSeconds := uint64(u.Seconds())
+	// Define time units in seconds
+	const (
+		year   = 365 * 24 * 3600
+		month  = 30 * 24 * 3600
+		day    = 24 * 3600
+		hour   = 3600
+		minute = 60
+		second = 1
+	)

-		{time.Hour * 24 * 365 * 9, "9y"},
-		{time.Hour * 24 * 365, "1y"},
-		{time.Hour * 24 * 30 * 5, "5mo"},
-		{time.Hour * 24 * 30, "1mo"},
-		{time.Hour * 24 * 2, "2d"},
-		{time.Hour * 2, "2h"},
-		{time.Hour*24*3 + time.Hour*2, "3d2h"},
-		{time.Hour*24*5 + time.Hour*4 + time.Minute*25, "5d4h25m"},
+		{0, "0s"},
+		{time.Second, "1s"},
+		{2 * time.Second, "2s"},
+		{time.Minute, "1m"},
+		{time.Minute + 30*time.Second, "1m30s"},
+		{time.Hour, "1h"},
+		{25 * time.Hour, "1d1h"},
+		{30 * 24 * time.Hour, "1mo"},
+		{365 * 24 * time.Hour, "1y"},
+		{400 * 24 * time.Hour, "1y1mo5d"},
+		{-time.Minute, "1m"},                // negative duration
+		{-400 * 24 * time.Hour, "1y1mo5d"}, // negative composite
+		{3*time.Minute + 4*time.Second, "3m4s"},

OCPBUGS-57049: cert inspection: add an option to humanize refresh period annotation #1981

OCPBUGS-57049: cert inspection: add an option to humanize refresh period annotation #1981

Uh oh!

Conversation

vrutkovs commented Jul 24, 2025

Uh oh!

openshift-ci-robot commented Jul 24, 2025

Uh oh!

p0lyn0mial left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p0lyn0mial left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p0lyn0mial commented Jul 25, 2025

Uh oh!

wangke19 commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Jul 25, 2025

Uh oh!

p0lyn0mial commented Jul 25, 2025

Uh oh!

openshift-ci bot commented Jul 25, 2025

Uh oh!

Uh oh!

openshift-ci-robot commented Jul 25, 2025

Uh oh!

Uh oh!

wangke19 commented Jul 25, 2025 •

edited

Loading