Add validation for exclusiveMaximum

[veepeeaee]

PR to add functionality requested in #19

In addition to existing checks, makes adding exclusiveMaximum: true to a maximum clause a breaking change

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: veepeeaee
Once this PR has been reviewed and has the lgtm label, please assign everettraven for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[JoelSpeed]

Is the opposite direction also a change we care about? Removing a maximum could also be considered a breaking change, though I guess we would want that behind some sort of strict flag?

@everettraven what's the precedence here?

[everettraven]

Yeah, my understanding is that we would probably want the opposite direction to be enforceable as well.

The precedence here is adding a configuration option to the validation that allows a user to configure whether or not they want to allow/disallow said behavior. For example, in the enum validation we have:

crdify/pkg/validations/property/enum.go

Lines 102 to 110 in 7b37e44

	// additionPolicy is how adding enums to an existing set of
	// enums should be treated.
	// Allowed values are Allow and Disallow.
	// When set to Allow, adding new values to an existing set
	// of enums will not be flagged.
	// When set to Disallow, adding new values to an existing
	// set of enums will be flagged.
	// Defaults to Disallow.
	AdditionPolicy AdditionPolicy `json:"additionPolicy,omitempty"`

We always default to the strictest possible configuration.

[veepeeaee]

Confirming here that we're taking about removing a exclusiveMaximum condition and not the entire max clause?
I see that the existing logic in

crdify/pkg/validations/property/max.go

Lines 35 to 50 in 7b37e44

	// MaxVerification is a generic helper function for comparing
	// two cmp.Ordered values. It returns an error if:
	// - older value is nil and newer value is not nil
	// - older and newer values are not nil, newer is less than older.
	func MaxVerification[T cmp.Ordered](older, newer *T) error {
	var err error
	switch {
	case older == nil && newer != nil:
	err = fmt.Errorf("%w : %v", ErrNetNewMaximumConstraint, *newer)
	case older != nil && newer != nil && *newer < *older:
	err = fmt.Errorf("%w : %v -> %v", ErrMaximumIncreased, *older, *newer)
	}
	return err
	}

doesn't flag the removal of a max clause entirely as a breaking change. Is the removal of exclusiveMaximum (or switch from true to false) fundamentally any different?
Or @everettraven - are you proposing that even this existing check on removal of a max clause should be placed behind a config?

[JoelSpeed]

If you had ExclusiveMaximum set to true, and then removed it, you've widened the available set of values for the field. This means clients that had previously assumed the maximum value was not included, would not be surprised to see that it was included. Generally loosening validation is easier to deal with than tightening validation, so folks sometimes accept this.

At the moment, I don't think the loosening of validation would be checked at all in this code, so we should add it, and ideally add an option so folks can accept the more relaxed validations as not breaking (within their risk acceptance)

[everettraven]

I agree with Joel here that we should attempt to check for loosening of validation here as well and provide a configuration option for the check to allow loosening if they desire.

I also left this comment on the exclusiveMinimum PR, but adapting here as well:

Even though exclusiveMaximum and maximum are fairly coupled, I think that it would make sense to follow the historical pattern of adding an entirely new validation check solely responsible for handling the exclusiveMaximum schema property changes. Doing so decouples the checks and keeps them simple until we need to intentionally introduce complexity to handle more complex use cases.

[everettraven]

doesn't flag the removal of a max clause entirely as a breaking change

This is likely an oversight. The maximum validation here was implemented a while ago before some precedence had evolved. I'll likely spin out a separate issue to go back through the existing set of checks to add more configuration knobs to older validation implementations where it makes sense to.

[veepeeaee]

Thank you both for the review!

• for my broader understanding about checks on loosening of conditions - how is the loosening of the max condition here (which we want to flag by default but provide a config to allow) fundamentally different from other changes that are not flagged (say adding an optional field). Or maybe in other words, what makes crdify different from a simple diff tool
• creating a separate (from max.go) validator for exclusiveMaximum might lead to some noisiness since as you said they are couple, is that fine? For example, if there was a maximum clause with exclusiveMaximum set to true, and it was removed in entirety - it would flag both that the maximum clause was removed as well as that exclusiveMaximum changed from true to false, and the second check doesn't seem to provide any value. exclusiveMaximum seems dependent on maximum, and flagging the change seems to make sense if only that changed but the maximum clause remained the same (I can of course put this dual check in the new validator for exclusiveMaximum as well)

[everettraven]

for my broader understanding about checks on loosening of conditions - how is the loosening of the max condition here (which we want to flag by default but provide a config to allow) fundamentally different from other changes that are not flagged (say adding an optional field). Or maybe in other words, what makes crdify different from a simple diff tool

This is a great question.

My original thinking here is that crdify by nature is a simple diff tool (all API changes are technically breaking changes) but the power comes in the ability to adjust that to match what you feel are the appropriate guardrails for your use case as a user through the configuration knobs that we provide.

Does this maybe make crdify initially difficult to use right out of the box? Probably. But this approach doesn't preclude us from loosening the defaults in the future as we get a better understanding of what the appropriate opinions to codify are (which we may make decisions on a validation-by-validation implementation basis).

I think my main point here is that starting out as the default configuration being akin to a "simple diff tool" gives us the ability to fully evaluate what opinions we think are and are not reasonable to have.

Is this a hill I'm willing to die on? Absolutely not. I'm open to suggestions on how we can do better here without boxing ourselves into a corner.

Maybe @JoelSpeed has some other thoughts here as well. I've been busy with some other stuff recently where I haven't had as much time to think about the forward direction of crdify, but that should be coming to an end soon and I'll have more time + a more clear head to think more about that.

creating a separate (from max.go) validator for exclusiveMaximum might lead to some noisiness since as you said they are couple, is that fine? For example, if there was a maximum clause with exclusiveMaximum set to true, and it was removed in entirety - it would flag both that the maximum clause was removed as well as that exclusiveMaximum changed from true to false, and the second check doesn't seem to provide any value. exclusiveMaximum seems dependent on maximum, and flagging the change seems to make sense if only that changed but the maximum clause remained the same (I can of course put this dual check in the new validator for exclusiveMaximum as well)

I did a bit more digging here and it looks like there was some confusion caused by the way exclusiveMaximum was implemented in the OpenAPI 3.0 specification.

It almost certainly is non-sensical to specify exclusiveMaximum without maximum but the Kubernetes API server doesn't prevent you from doing this. AFAIK it just ignores the exclusiveMaximum setting.

Having done some more investigation here, I'm actually fine with this being built into the existing maximum check. Apologies for any unnecessary confusion here!

[JoelSpeed]

Add no diff with false too?