Support setting CA validity duration via unsupportedConfigOverrides #85

marun · 2019-11-15T09:36:23Z

Setting the duration of CA validity allows setting a short duration to test operator compatibility with CA rotation.

This PR is required by the proposed periodic rotation compatibility job: openshift/release#5930

marun · 2019-11-22T03:46:34Z

Rebased and ready for review

stlaz

nobody should need expiry in seconds, if they do, we'll change lib-go instead

stlaz · 2019-11-22T11:50:02Z

pkg/operator/cert.go

+		return caConfig, nil
+	}
+	// operator-go's cert generation only accepts an expiry in days. To
+	// ensure support for all validity durations, even those less than a


I don't see value in durations < 1 day, just stick to what library-go does and pass the duration in days directly to it. The API ValidityDuration: certificate validity in seconds does not sound very user friendly, either.

Having a flexible initial expiry is required to support validating that cert expiry is handled correctly in the context of a periodic job that takes less than a day to execute (see the description of the proposed rotation job). Exploratory testing has a similar requirement. If qa or someone else wants to see what happens when a cert expires, I think it makes sense to allow the specification of the appropriate interval (likely in minutes or hours) rather than forcing them to wait a whole day or depend on clock manipulation.

So, there is a requirement. And if this repo is the only one that requires it, I'm not sure why that suggests the effort of updating library-go (especially for an unsupported api).

To your point, though, it is not great UX for a general-purpose option to set the CA expiry in at most hours (since that's the maximum granularity that Duration supports). I think that suggests changing the name to ValidityDurationForTesting and maybe limiting the scope of the option to the initial cert (or manual rotation, since that's the same code path). If we are required to support custom cert duration for production use in the future, we could add a separate option specified in days. WDYT?

stlaz · 2019-11-22T12:02:05Z

pkg/operator/rotate.go

@@ -16,7 +16,7 @@ import (
 )

 const (
-	signingCertificateLifetimeInDays = 365 // 1 year
+	SigningCertificateLifetimeInDays = 365 // 1 year


If someone in the future changes this to var, I will eat them

stlaz · 2019-11-22T12:05:25Z

/test e2e-aws-operator

sttts · 2019-11-22T17:05:52Z

pkg/operator/cert.go

+// provided duration is greater than zero. If an error occurs or the duration is
+// zero, the original CA is returned.
+func maybeUpdateExpiry(caConfig *crypto.TLSCertificateConfig, duration time.Duration) (*crypto.TLSCertificateConfig, error) {
+	if duration.Nanoseconds() == 0 {


isn’t there duration.IsZero?

~~Doh. Done.~~

Time.IsZero is a thing. Duration.IsZero is not.

sttts · 2019-11-22T17:18:16Z

pkg/operator/config.go

+	// reason indicates why a rotation of the signing CA should be forced. If the
+	// reason is not empty and has not been recorded as an annotation on the signing
+	// secret, the rotation of the signing CA will be triggered at most once.
+	// +optional
 	Reason string `json:"reason"`


So you do not share @stlaz's concerns regarding ValidityDuration being specified in at most hours?

Probably >= 1h makes sense. So either document the minimum or switch to hours as integer.

What would be the value in switching to an integer representing hours given that metav1.Duration supports providing a time in hours (e.g. 1h) and supporting a duration in minutes (e.g. 20m) is potentially useful for testing?

See @liggitt's talk at contributor summit: metav1.Duration is considered a failure.

I don't see the video up yet, but I'm assuming you're suggesting that metav1.Duration not be used? That reinforces my suggestion for ValidityDurationForTesting and denominating it in minutes. If ValidityDuration becomes a thing to set the CA expiry for production use, it would likely be in days.

ValidityDurationForTesting is fine with me as well. Let's decide and move forward.

I've update the name to ValidityDurationForTesting and left it as metav1.Duration, hopefully that makes sense for you.

sttts · 2019-11-22T17:18:56Z

pkg/operator/rotate.go

+	// Set a custom expiry if one was provided
+	signingCA.config, err = maybeUpdateExpiry(signingCA.config, serviceCAConfig.CAConfig.ValidityDuration)
+	if err != nil {
+		return "", fmt.Errorf("Error renewing ca for custom duration: %v", err)


lower case errors

stlaz · 2019-11-22T17:30:08Z

/approve

marun · 2019-11-22T17:36:31Z

/hold

marun · 2019-11-22T17:44:19Z

/hold cancel

Updated a couple of error messages in latest push, nothing functional.

sttts · 2019-11-22T20:02:00Z

Sgtm overall (would switch to hours).

marun · 2019-11-27T15:01:02Z

/retest

marun · 2019-11-28T12:38:39Z

/retest

sttts · 2019-11-28T12:44:03Z

/lgtm

openshift-bot · 2019-11-28T14:05:24Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-11-28T15:22:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-11-28T16:01:47Z

/retest

Please review the full test history for this PR and help us cut down flakes.

This is intended to support testing of rotation compatibility.

marun · 2019-11-28T16:34:50Z

Rebased

marun · 2019-11-29T00:15:50Z

/retest

marun · 2019-11-29T02:52:48Z

/retest

stlaz · 2019-12-03T12:42:13Z

/lgtm

openshift-ci-robot · 2019-12-03T12:42:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marun, stlaz, sttts

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~~ [stlaz,sttts]

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 added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 15, 2019

openshift-ci-robot requested review from stlaz and sttts November 15, 2019 09:36

marun force-pushed the ca-validity-duration branch from 0022f25 to c9a9bae Compare November 22, 2019 03:46

marun changed the title ~~WIP Support setting CA validity duration via unsupportedConfigOverrides~~ Support setting CA validity duration via unsupportedConfigOverrides Nov 22, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 22, 2019

marun force-pushed the ca-validity-duration branch 4 times, most recently from e009dae to 747b4d3 Compare November 22, 2019 07:56

stlaz requested changes Nov 22, 2019

View reviewed changes

sttts reviewed Nov 22, 2019

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 22, 2019

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 22, 2019

marun force-pushed the ca-validity-duration branch from 747b4d3 to 98eae94 Compare November 22, 2019 17:43

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 22, 2019

marun force-pushed the ca-validity-duration branch from 98eae94 to 3cd3ae3 Compare November 27, 2019 12:23

openshift-ci-robot assigned sttts Nov 28, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 28, 2019

Support setting CA validity duration via unsupportedConfigOverrides

0852211

This is intended to support testing of rotation compatibility.

marun force-pushed the ca-validity-duration branch from 3cd3ae3 to 0852211 Compare November 28, 2019 16:34

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 28, 2019

openshift-ci-robot assigned stlaz Dec 3, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2019

openshift-merge-robot merged commit 39ba3e6 into openshift:master Dec 3, 2019

marun deleted the ca-validity-duration branch December 3, 2019 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support setting CA validity duration via unsupportedConfigOverrides #85

Support setting CA validity duration via unsupportedConfigOverrides #85

marun commented Nov 15, 2019 •

edited

marun commented Nov 22, 2019

stlaz left a comment

stlaz Nov 22, 2019

marun Nov 22, 2019 •

edited

stlaz Nov 22, 2019

stlaz commented Nov 22, 2019

sttts Nov 22, 2019

marun Nov 22, 2019 •

edited

sttts Nov 22, 2019

marun Nov 22, 2019

sttts Nov 22, 2019

marun Nov 22, 2019

sttts Nov 25, 2019

marun Nov 26, 2019

sttts Nov 26, 2019

marun Nov 27, 2019

sttts Nov 22, 2019

marun Nov 22, 2019

stlaz commented Nov 22, 2019

marun commented Nov 22, 2019

marun commented Nov 22, 2019

sttts commented Nov 22, 2019

marun commented Nov 27, 2019

marun commented Nov 28, 2019

sttts commented Nov 28, 2019

openshift-bot commented Nov 28, 2019

openshift-bot commented Nov 28, 2019

openshift-bot commented Nov 28, 2019

marun commented Nov 28, 2019

marun commented Nov 29, 2019

marun commented Nov 29, 2019

stlaz commented Dec 3, 2019

openshift-ci-robot commented Dec 3, 2019

Support setting CA validity duration via unsupportedConfigOverrides #85

Support setting CA validity duration via unsupportedConfigOverrides #85

Conversation

marun commented Nov 15, 2019 • edited

marun commented Nov 22, 2019

stlaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marun Nov 22, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stlaz commented Nov 22, 2019

Choose a reason for hiding this comment

marun Nov 22, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stlaz commented Nov 22, 2019

marun commented Nov 22, 2019

marun commented Nov 22, 2019

sttts commented Nov 22, 2019

marun commented Nov 27, 2019

marun commented Nov 28, 2019

sttts commented Nov 28, 2019

openshift-bot commented Nov 28, 2019

openshift-bot commented Nov 28, 2019

openshift-bot commented Nov 28, 2019

marun commented Nov 28, 2019

marun commented Nov 29, 2019

marun commented Nov 29, 2019

stlaz commented Dec 3, 2019

openshift-ci-robot commented Dec 3, 2019

marun commented Nov 15, 2019 •

edited

marun Nov 22, 2019 •

edited

marun Nov 22, 2019 •

edited