Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve logic for setting LastTransitionTime #16

Merged
merged 5 commits into from
May 30, 2023
Merged

Improve logic for setting LastTransitionTime #16

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
aa4880c
improve setting conditions & fix wrong LastTransitionTime
inteon May 30, 2023
6c8d01e
improve fake clock testing
inteon May 30, 2023
44d57d4
fix FailureTime value
inteon May 30, 2023
a6fa208
update fake creationtime logic
inteon May 30, 2023
642ff17
use randomTime
inteon May 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 43 additions & 12 deletions conditions/certificaterequest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,30 +17,61 @@ limitations under the License.
package conditions

import (
cmutil "github.com/cert-manager/cert-manager/pkg/api/util"
cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/utils/clock"
)

// Update the status with the provided condition details & return
// the added condition.
// NOTE: this code is just a workaround for cmutil only accepting the certificaterequest object
func SetCertificateRequestStatusCondition(
Comment on lines -27 to 28
Copy link
Member

Choose a reason for hiding this comment

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

comment: It's a shame to have this code here - functions which act on cert-manager resources such as CertificateRequest seem like a natural fit for the cert-manager codebase. Better to have one source of truth for "how do I update a CR status condition".

Don't think it's a dealbreaker - but I wonder if we could add something to the cert-manager codebase instead? Doesn't really matter though

conditions *[]cmapi.CertificateRequestCondition,
clock clock.PassiveClock,
existingConditions []cmapi.CertificateRequestCondition,
patchConditions *[]cmapi.CertificateRequestCondition,
conditionType cmapi.CertificateRequestConditionType,
status cmmeta.ConditionStatus,
reason, message string,
) *cmapi.CertificateRequestCondition {
cr := cmapi.CertificateRequest{
Status: cmapi.CertificateRequestStatus{
Conditions: *conditions,
},
) (*cmapi.CertificateRequestCondition, *metav1.Time) {
newCondition := cmapi.CertificateRequestCondition{
Type: conditionType,
Status: status,
Reason: reason,
Message: message,
}

cmutil.SetCertificateRequestCondition(&cr, conditionType, status, reason, message)
condition := cmutil.GetCertificateRequestCondition(&cr, conditionType)
nowTime := metav1.NewTime(clock.Now())
newCondition.LastTransitionTime = &nowTime

*conditions = cr.Status.Conditions
// Reset the LastTransitionTime if the status hasn't changed
for _, cond := range existingConditions {
if cond.Type != conditionType {
continue
}

return condition
// If this update doesn't contain a state transition, we don't update
// the conditions LastTransitionTime to Now()
if cond.Status == status {
newCondition.LastTransitionTime = cond.LastTransitionTime
}
}

// Search through existing conditions
for idx, cond := range *patchConditions {
// Skip unrelated conditions
if cond.Type != conditionType {
continue
}

// Overwrite the existing condition
(*patchConditions)[idx] = newCondition

return &newCondition, &nowTime
}

// If we've not found an existing condition of this type, we simply insert
// the new condition into the slice.
*patchConditions = append(*patchConditions, newCondition)

return &newCondition, &nowTime
}
80 changes: 43 additions & 37 deletions conditions/issuer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,59 +17,65 @@ limitations under the License.
package conditions

import (
cmutil "github.com/cert-manager/cert-manager/pkg/api/util"
cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/utils/clock"
)

// private struct; only used to implement the GenericIssuer interface
// for use with the cmutil.SetIssuerCondition function
type genericIssuer struct {
metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata,omitempty"`

Status cmapi.IssuerStatus `json:"status"`
}

func (g *genericIssuer) DeepCopyObject() runtime.Object {
panic("[HACK]: this function should not get called")
}

func (g *genericIssuer) GetSpec() *cmapi.IssuerSpec {
panic("[HACK]: this function should not get called")
}

func (g *genericIssuer) GetObjectMeta() *metav1.ObjectMeta {
return &g.ObjectMeta
}

func (g *genericIssuer) GetStatus() *cmapi.IssuerStatus {
return &g.Status
}

// Update the status with the provided condition details & return
// the added condition.
// NOTE: this code is just a workaround for cmutil only accepting a GenericIssuer interface
Copy link
Member Author

@inteon inteon May 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Instead of calling the cert-manager code, we now just host this logic in this repo. This makes it much clearer what is going on & allowed me to customize the logic for our needs (SSA).

Copy link
Member

Choose a reason for hiding this comment

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

Definitely makes sense for his repo's issuer type to have these kinds of functions be more specific!

func SetIssuerStatusCondition(
conditions *[]cmapi.IssuerCondition,
clock clock.PassiveClock,
existingConditions []cmapi.IssuerCondition,
patchConditions *[]cmapi.IssuerCondition,
observedGeneration int64,
conditionType cmapi.IssuerConditionType,
status cmmeta.ConditionStatus,
reason, message string,
) *cmapi.IssuerCondition {
gi := genericIssuer{
Status: cmapi.IssuerStatus{
Conditions: *conditions,
},
) (*cmapi.IssuerCondition, *metav1.Time) {
newCondition := cmapi.IssuerCondition{
Type: conditionType,
Status: status,
Reason: reason,
Message: message,
ObservedGeneration: observedGeneration,
}

cmutil.SetIssuerCondition(&gi, observedGeneration, conditionType, status, reason, message)
nowTime := metav1.NewTime(clock.Now())
newCondition.LastTransitionTime = &nowTime

// Reset the LastTransitionTime if the status hasn't changed
for _, cond := range existingConditions {
if cond.Type != conditionType {
continue
}

// If this update doesn't contain a state transition, we don't update
// the conditions LastTransitionTime to Now()
if cond.Status == status {
newCondition.LastTransitionTime = cond.LastTransitionTime
}
}

// Search through existing conditions
for idx, cond := range *patchConditions {
// Skip unrelated conditions
if cond.Type != conditionType {
continue
}

// Overwrite the existing condition
(*patchConditions)[idx] = newCondition

return &newCondition, &nowTime
}

*conditions = gi.Status.Conditions
// If we've not found an existing condition of this type, we simply insert
// the new condition into the slice.
*patchConditions = append(*patchConditions, newCondition)

return GetIssuerStatusCondition(*conditions, conditionType)
return &newCondition, &nowTime
}

func GetIssuerStatusCondition(
Expand Down
Loading