Skip to content

Add Opt-In KEP-1623 support for existing Conditions #565

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.

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

mallardduck
Copy link
Member

@mallardduck mallardduck commented Apr 16, 2025

Why?

Since Wrangler's creation around 2019 (when the existing Cond was created) a lot has changed. One of those significant changes was KEP-1623 - essentially a standard for Conditions in k8s.

The existing Wrangler/Rancher style of Conditons is very similar to the new standard. This allows potential for adding the KEP support in an opt-in manner; when comparing Wrangler and KEP/k8s flavored Condition the primary differences are:

  • Slightly different, but compatible Status field type (api/core/v1 vs apimachinery's meta/v1)
  • Wrangler's Condition has a LastUpdatedTime
  • k8s Condition has strict behavior around updating LastTransitionTime
  • k8s Condition LastTransitionTime field type is metav1.Time
  • Wrangler allows Reason and Message to be empty and omitted; k8s requires a non-empty Reason value, and requires Message but allows empty ("") values.
  • k8s Condition includes an optional ObservedGeneration field to track resource generation.
  • k8s Condition also includes protobuf annotations

How to use?

Just like the existing implementation you will create a Conditions field on your status. However the first difference will be how you define that fields type and annotations. It should use:

	// +listType=map
	// +listMapKey=type
	// +patchStrategy=merge
	// +patchMergeKey=type
	// +optional
	Conditions                 []metav1.Condition      `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"`

Then, when you're in your controller and want to use a condition you do it exactly the same as today - for the most part - but with the addition of one extra call .ToK8sCondition(). The main caveat of "for the most part" is that the new implementation doesn't use the exact same interface as the wrangler flavor condition.

Specifically:

  • Message(obj interface{}, msg string) changed to SetMessage(obj interface{}, msg string)
  • Reason(obj interface{}, reason string) changed to SetReason(obj interface{}, reason string)
  • Original style condition added HasCondition(obj interface{}) bool
  • (Optional, as in can be removed) Fluent API that's much nicer than the backward compatible API [parity of unit coverage for fluent API is a WIP]

Here is a more complete example of using a condition:

    ResourceConditionReady Cond = "Ready"

// Via Fluent API
    ResourceConditionReady.ToK8sFluentCondition(&myCrdObj).False().SetMessage("Hello world").Apply(&myCrdObj)

// Or via Wrangler compatible KEP1623 updated API
    ResourceConditionReady.ToK8sCondition().False(&myCrdObj)
    ResourceConditionReady.ToK8sCondition().SetMessage(&myCrdObj, "Hello world")

// Reference: Wrangler API
    ResourceConditionReady.False(&myCrdObj)
    ResourceConditionReady.SetMessage(&myCrdObj, "Hello world")

If accepted, I also have backport branch ready for V2; and could likely make a backport for V1 as well.


Breaking Changes?

Highly Unlikely. I've come to this conclusion via using golang experimental apidiff tool. Note that when I checked this aspect for both main and V2 the results were the same. So I will only show the work for the main branch checks I did. Here's the steps I did to get this result:

± |kep1623 → origin {1} ?:1 ✗| → apidiff -w ./new ./pkg/condition/
± |kep1623 → origin {1} ?:2 ✗| → git checkout main
± |main → upstream {1} ?:2 ✗| → apidiff -w ./main ./pkg/condition/
± |main → upstream {1} ?:3 ✗| → apidiff main new
Compatible changes:
- Cond.HasCondition: added
- Cond.Name: added
- Cond.SetMessage: added
- Cond.SetReason: added
- Cond.ToFluentBuilder: added
- Cond.ToK8sCondition: added
- MetaV1ConditionFluentBuilder: added
- MetaV1ConditionHandler: added

Doing the same for genericcondition pkg:

Compatible changes:
- GenericCondition.ToK8sCondition: added

What if an existing CRD adopts the new KEP based Conditions though?

I have tested with BRO here: rancher/backup-restore-operator#763

The results of my tests are that:

  • Any existing resource with old style conditions will parse into new conditions fine,
  • The LastUpdatedAt field is lost as expected - this would be a breaking change likely workaround is just using LastTransitionedAt that still introduces behavior changes though
  • When the resource is first updated after this change, any conditions will be updated to include the new required fields.

So to be more succinct, the main conditions where you must use caution are:

  • Your current resource controller logic tightly depends on the LastUpdatedAt time of conditions.
  • Your current Condition sets non-CamelCase Reason values
    • Maybe not deal breaker; the Reason defaults to Created - so this will be filled in when Wrangler flavor updated

@mallardduck mallardduck changed the title Don't mind me Add Opt-In KEP-1623 support for existing Conditions Apr 16, 2025
@mallardduck mallardduck requested a review from tomleb April 16, 2025 06:21
@mallardduck mallardduck marked this pull request as ready for review April 16, 2025 14:25
@mallardduck mallardduck requested a review from a team as a code owner April 16, 2025 14:25
@mallardduck
Copy link
Member Author

Experimental BRO PR: rancher/backup-restore-operator#763

Per feedback from Platform Design meeting I'm using BRO to test what it's like when a CRD definition is updated to use the new KEP based ones in place. I suspect it will drop the LastUpdatedAt and all other fields will translate into KEP values fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants