is it required to validate object

[clyang82]

This is not a final PR need to be reviewed.

I just post this PR to discuss:

1. is it really need to validate the object since the unstructured object is just retrieved from the runtime environment. refer to

   config-policy-controller/controllers/configurationpolicy_controller.go

   Lines 1504 to 1506 in 0499944

   	object, _ = getObject(
   	objDetails.isNamespaced, namespace, objDetails.name, mapping.Resource, r.TargetK8sDynamicClient,
   	)

2. config-policy-controller/controllers/configurationpolicy_controller.go

   Line 2544 in 0499944

   openAPIResources, err := r.openAPIParser.Parse()

   need to consume around 10M memory based on my tests. And it cannot be released. Is it possible to reduce the cost?
   Here is my tests code

	var m1, m2 goruntime.MemStats

	goruntime.ReadMemStats(&m1)
	fmt.Println("Alloc in validateObject: m1", m1.Alloc/(1<<10))

	// Parse() handles caching of the OpenAPI data.
	r.lock.RLock()
	openAPIResources, err := r.openAPIParser.Parse()
	r.lock.RUnlock()
	goruntime.ReadMemStats(&m2)
	fmt.Println("Alloc in validateObject: m2", m2.Alloc/(1<<10))
	fmt.Println("memory usage Alloc:", (m2.Alloc-m1.Alloc)/(1<<10))

FYI @gparvin @mprahl

[mprahl]

@clyang82 interesting find! So the reason it's needed is that during object creation, it validates to ensure all required fields and no unknown fields are set in the policy. On updates, it takes the object returned from the Kubernetes API and merges in any updates from the policy. The validation ensures that no unknown fields were set.

The reason it is important that no unknown fields are set is that Kubernetes silently ignores unknown fields and then when ConfigurationPolicy is evaluated again, it'll think that it's non-compliant because the unknown field is missing and tries to update it again.

I think we could consider setting FieldValidation to Strict on the create and update requests so that the requests fail when unknown fields are set. This would only handle the case for enforce though since in inform, it wouldn't know that there is an unknown field without client side validation.

What do you think @JustinKuli?

[clyang82]

Thanks @mprahl for your comments. I gave a try to set FieldValidation to Strict on the create and update. Please take a look at if you are OK with the changes.

[JustinKuli]

The changes sound right to me. I'm happy to see the kind tests cover both the newer k8s with the validation, and the older version where it wasn't possible server-side.

@clyang82 what did you mean in the original description "And it cannot be released." ?

[mprahl]

Will this work if err != nil?

[JustinKuli]

Assuming that in that case it is just set to an empty string, then the semver library would treat it as invalid and do this:

An invalid semantic version string is considered less than a valid one. All invalid semantic version strings compare equal to each other.
(https://pkg.go.dev/golang.org/x/mod/semver#Compare)

So I think it would fallback to the "old" validation gracefully.

[clyang82]

Yes. @JustinKuli is correct. in that case it would fallback to the original validation.

[clyang82]

The changes sound right to me. I'm happy to see the kind tests cover both the newer k8s with the validation, and the older version where it wasn't possible server-side.

@clyang82 what did you mean in the original description "And it cannot be released." ?

I mean the memory is allocated by r.openAPIParser.Parse() won't be released.

[mprahl]

Nice job!

[mprahl]

/hold for squashing of the commits

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: clyang82, mprahl

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 [mprahl]

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

[mprahl]

/unhold