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.

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

is it required to validate object #135

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
41 changes: 34 additions & 7 deletions controllers/configurationpolicy_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
gocmp "github.com/google/go-cmp/cmp"
"github.com/prometheus/client_golang/prometheus"
templates "github.com/stolostron/go-template-utils/v3/pkg/templates"
"golang.org/x/mod/semver"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
extensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
Expand Down Expand Up @@ -95,6 +96,7 @@ type cachedEncryptionKey struct {
type discoveryInfo struct {
apiResourceList []*metav1.APIResourceList
apiGroups []*restmapper.APIGroupResources
serverVersion string
discoveryLastRefreshed time.Time
}

Expand Down Expand Up @@ -297,6 +299,13 @@ func (r *ConfigurationPolicyReconciler) refreshDiscoveryInfo() error {

dd := r.TargetK8sClient.Discovery()

serverVersion, err := dd.ServerVersion()
if err != nil {
log.Error(err, "Could not get the server version")
}

r.serverVersion = serverVersion.String()
Copy link
Member

Choose a reason for hiding this comment

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

Will this work if err != nil?

Copy link
Member

@JustinKuli JustinKuli May 31, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


_, apiResourceList, resourceErr := dd.ServerGroupsAndResources()
if resourceErr != nil {
log.Error(resourceErr, "Could not get the full API resource list")
Expand Down Expand Up @@ -2187,11 +2196,17 @@ func (r *ConfigurationPolicyReconciler) createObject(
objLog := log.WithValues("name", unstruct.GetName(), "namespace", unstruct.GetNamespace())
objLog.V(2).Info("Entered createObject", "unstruct", unstruct)

if err := r.validateObject(&unstruct); err != nil {
return nil, err
// FieldValidation is supported in k8s 1.25 as beta release
// so if the version is below 1.25, we need to use client side validation to validate the object
if semver.Compare(r.serverVersion, "v1.25.0") < 0 {
if err := r.validateObject(&unstruct); err != nil {
return nil, err
}
}

object, err = res.Create(context.TODO(), &unstruct, metav1.CreateOptions{})
object, err = res.Create(context.TODO(), &unstruct, metav1.CreateOptions{
FieldValidation: metav1.FieldValidationStrict,
})
if err != nil {
if k8serrors.IsAlreadyExists(err) {
objLog.V(2).Info("Got 'Already Exists' response for object")
Expand Down Expand Up @@ -2669,20 +2684,32 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
if updateNeeded {
log.V(2).Info("Updating the object based on the template definition")

if err := r.validateObject(obj.object); err != nil {
message := fmt.Sprintf("Error validating the object %s, the error is `%v`", obj.name, err)
// FieldValidation is supported in k8s 1.25 as beta release
// so if the version is below 1.25, we need to use client side validation to validate the object
if semver.Compare(r.serverVersion, "v1.25.0") < 0 {
if err := r.validateObject(obj.object); err != nil {
message := fmt.Sprintf("Error validating the object %s, the error is `%v`", obj.name, err)

return false, message, true, updateNeeded, false
return false, message, true, updateNeeded, false
}
}

_, err = res.Update(context.TODO(), obj.object, metav1.UpdateOptions{})
_, err = res.Update(context.TODO(), obj.object, metav1.UpdateOptions{
FieldValidation: metav1.FieldValidationStrict,
})
if k8serrors.IsNotFound(err) {
message := fmt.Sprintf("`%v` is not present and must be created", obj.object.GetKind())

return false, message, true, updateNeeded, false
}

if err != nil {
if strings.Contains(err.Error(), "strict decoding error:") {
message := fmt.Sprintf("Error validating the object %s, the error is `%v`", obj.name, err)

return false, message, true, updateNeeded, false
}

message := fmt.Sprintf("Error updating the object `%v`, the error is `%v`", obj.name, err)

return false, message, true, updateNeeded, false
Expand Down
5 changes: 3 additions & 2 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,15 @@ require (
github.com/stolostron/go-log-utils v0.1.2
github.com/stolostron/go-template-utils/v3 v3.2.1
github.com/stretchr/testify v1.8.1
golang.org/x/mod v0.10.0
k8s.io/api v0.27.1
k8s.io/apiextensions-apiserver v0.27.1
k8s.io/apimachinery v0.27.1
k8s.io/client-go v12.0.0+incompatible
k8s.io/klog v1.0.0
k8s.io/klog/v2 v2.100.1
k8s.io/kube-openapi v0.0.0-20230501164219-8b0f38b5fd1f
k8s.io/kubectl v0.27.1
k8s.io/kubectl v0.0.0-00010101000000-000000000000
open-cluster-management.io/addon-framework v0.6.1
sigs.k8s.io/controller-runtime v0.14.6
sigs.k8s.io/yaml v1.3.0
Expand All @@ -36,7 +37,7 @@ require (
github.com/emicklei/go-restful/v3 v3.10.2 // indirect
github.com/evanphx/json-patch v5.6.0+incompatible // indirect
github.com/evanphx/json-patch/v5 v5.6.0 // indirect
github.com/exponent-io/jsonpath v0.0.0-20210407135951-1de76d718b3f // indirect
github.com/exponent-io/jsonpath v0.0.0-20151013193312-d6023ce2651d // indirect
github.com/fsnotify/fsnotify v1.6.0 // indirect
github.com/go-errors/errors v1.0.1 // indirect
github.com/go-logr/logr v1.2.4 // indirect
Expand Down
6 changes: 4 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ github.com/evanphx/json-patch v5.6.0+incompatible h1:jBYDEEiFBPxA0v50tFdvOzQQTCv
github.com/evanphx/json-patch v5.6.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk=
github.com/evanphx/json-patch/v5 v5.6.0 h1:b91NhWfaz02IuVxO9faSllyAtNXHMPkC5J8sJCLunww=
github.com/evanphx/json-patch/v5 v5.6.0/go.mod h1:G79N1coSVB93tBe7j6PhzjmR3/2VvlbKOFpnXhI9Bw4=
github.com/exponent-io/jsonpath v0.0.0-20210407135951-1de76d718b3f h1:Wl78ApPPB2Wvf/TIe2xdyJxTlb6obmF18d8QdkxNDu4=
github.com/exponent-io/jsonpath v0.0.0-20210407135951-1de76d718b3f/go.mod h1:OSYXu++VVOHnXeitef/D8n/6y4QV8uLHSFXX4NeXMGc=
github.com/exponent-io/jsonpath v0.0.0-20151013193312-d6023ce2651d h1:105gxyaGwCFad8crR9dcMQWvV9Hvulu6hwUh4tWPJnM=
github.com/exponent-io/jsonpath v0.0.0-20151013193312-d6023ce2651d/go.mod h1:ZZMPRZwes7CROmyNKgQzC3XPs6L/G2EJLHddWejkmf4=
github.com/flowstack/go-jsonschema v0.1.1/go.mod h1:yL7fNggx1o8rm9RlgXv7hTBWxdBM0rVwpMwimd3F3N0=
github.com/frankban/quicktest v1.14.3 h1:FJKSZTDHjyhriyC81FLQ0LY93eSai0ZyR/ZIkd3ZUKE=
github.com/fsnotify/fsnotify v1.6.0 h1:n+5WquG0fcWoWp6xPWfHdbskMCQaFnG6PfBrh1Ky4HY=
Expand Down Expand Up @@ -240,6 +240,8 @@ golang.org/x/lint v0.0.0-20190930215403-16217165b5de/go.mod h1:6SW0HCj/g11FgYtHl
golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4=
golang.org/x/mod v0.10.0 h1:lFO9qtOdlre5W1jxS3r/4szv2/6iXxScdzjoBMXNhYk=
golang.org/x/mod v0.10.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs=
golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
golang.org/x/net v0.0.0-20180826012351-8a410e7b638d/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
golang.org/x/net v0.0.0-20190108225652-1e06a53dbb7e/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
Expand Down
37 changes: 32 additions & 5 deletions test/e2e/case23_invalid_field_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"golang.org/x/mod/semver"
corev1 "k8s.io/api/core/v1"
k8serrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -23,12 +24,24 @@ var _ = Describe("Test an objectDefinition with an invalid field", Ordered, func
policyYAML = "../resources/case23_invalid_field/policy.yaml"
)

var serverVersion string
BeforeAll(func() {
serverVersion = utils.GetServerVersion(clientManaged)
})

It("Fails when an invalid field is provided", func() {
By("Creating the " + policyName + " policy")
utils.Kubectl("apply", "-f", policyYAML, "-n", testNamespace)

expectedMsg := "configmaps [case23] in namespace default is missing, and cannot be created, reason: " +
"`ValidationError(ConfigMap): unknown field \"invalid\" in io.k8s.api.core.v1.ConfigMap`"
"`ConfigMap in version \"v1\" cannot be handled as a ConfigMap: strict decoding error: unknown " +
"field \"invalid\"`"

// if the server version is less than 1.25.0, we use client side validation
if semver.Compare(serverVersion, "v1.25.0") < 0 {
expectedMsg = "configmaps [case23] in namespace default is missing, and cannot be created, reason: " +
"`ValidationError(ConfigMap): unknown field \"invalid\" in io.k8s.api.core.v1.ConfigMap`"
}

By("Verifying that the " + policyName + " policy is noncompliant")
Eventually(func(g Gomega) {
Expand All @@ -43,11 +56,16 @@ var _ = Describe("Test an objectDefinition with an invalid field", Ordered, func
By("Verifying events do not continue to be created after the first violation for created objects")
startTime := metav1.NewTime(time.Now())

msg := "ConfigMap in version \"v1\" cannot be handled as a ConfigMap: " +
"strict decoding error: unknown field \"invalid\""
if semver.Compare(serverVersion, "v1.25.0") < 0 {
msg = "unknown field \"invalid\" in io.k8s.api.core.v1.ConfigMap"
}
Consistently(func() interface{} {
compPlcEvents := utils.GetMatchingEvents(clientManaged, testNamespace,
policyName,
"",
"unknown field \"invalid\" in io.k8s.api.core.v1.ConfigMap",
msg,
defaultTimeoutSeconds)

if len(compPlcEvents) == 0 {
Expand All @@ -66,8 +84,13 @@ var _ = Describe("Test an objectDefinition with an invalid field", Ordered, func
_, err := clientManaged.CoreV1().ConfigMaps("default").Create(context.TODO(), configmap, metav1.CreateOptions{})
Expect(err).ToNot(HaveOccurred())

expectedMsg = "Error validating the object case23, the error is `ValidationError(ConfigMap): unknown " +
"field \"invalid\" in io.k8s.api.core.v1.ConfigMap`"
expectedMsg = "Error validating the object case23, the error is `ConfigMap in version \"v1\"" +
" cannot be handled as a ConfigMap: strict decoding error: unknown field \"invalid\"`"
// if the server version is less than 1.25.0, we use client side validation
if semver.Compare(serverVersion, "v1.25.0") < 0 {
expectedMsg = "Error validating the object case23, the error is `ValidationError(ConfigMap): unknown " +
"field \"invalid\" in io.k8s.api.core.v1.ConfigMap`"
}
Eventually(func() interface{} {
managedPlc := utils.GetWithTimeout(
clientManagedDynamic, gvrConfigPolicy, policyName, testNamespace, true, defaultTimeoutSeconds,
Expand All @@ -79,11 +102,15 @@ var _ = Describe("Test an objectDefinition with an invalid field", Ordered, func
By("Verifying events do not continue to be created after the first violation for existing objects")
alreadyExistsStartTime := metav1.NewTime(time.Now())

msg = "strict decoding error: unknown field \"invalid\""
if semver.Compare(serverVersion, "v1.25.0") < 0 {
msg = "unknown field \"invalid\" in io.k8s.api.core.v1.ConfigMap"
}
Consistently(func() interface{} {
compPlcEvents := utils.GetMatchingEvents(clientManaged, testNamespace,
policyName,
"",
"unknown field \"invalid\" in io.k8s.api.core.v1.ConfigMap",
msg,
defaultTimeoutSeconds)

if len(compPlcEvents) == 0 {
Expand Down
7 changes: 7 additions & 0 deletions test/utils/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -335,3 +335,10 @@ func DoConfigPolicyMessageTest(clientHubDynamic dynamic.Interface,
return message
}, timeout, 1).Should(Equal(expectedMsg))
}

func GetServerVersion(clientManaged kubernetes.Interface) string {
serverVersion, err := clientManaged.Discovery().ServerVersion()
Expect(err).ToNot(HaveOccurred())

return serverVersion.String()
}