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

Add Gateway API webhook validation functions to egctl translate #1295

Closed
arkodg opened this issue Apr 13, 2023 · 18 comments · Fixed by #4257
Closed

Add Gateway API webhook validation functions to egctl translate #1295

arkodg opened this issue Apr 13, 2023 · 18 comments · Fixed by #4257
Assignees
Labels
area/egctl kind/enhancement New feature or request
Milestone

Comments

@arkodg
Copy link
Contributor

arkodg commented Apr 13, 2023

Description:

Run the upstream Gateway API validation functions such as https://github.com/kubernetes-sigs/gateway-api/blob/a78e09220feec7947eb1a08b658b45f3b4182d7f/apis/v1beta1/validation/gateway.go#L54 on the inputted resources in egctl translate

gcName, resources, err := kubernetesYAMLToResources(string(inBytes), addMissingResources)
to catch client side validation errors and mimic the same experience the user would have had if they were applying these resources using kubectl apply

@arkodg arkodg added kind/enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Apr 13, 2023
@arkodg arkodg added this to the Backlog milestone Apr 13, 2023
@Xunzhuo
Copy link
Member

Xunzhuo commented Apr 13, 2023

Can we have a egctl validate cmd for validating resources ?

@arkodg
Copy link
Contributor Author

arkodg commented Apr 13, 2023

Can we have a hgctl validate cmd for validating resources ?

we currently solve above case using egctl translate --from gateway-api --to gateway-api but we could also create a validate alias, if users prefer validate

@liangyuanpeng
Copy link

liangyuanpeng commented Apr 13, 2023

I want to working for it.

/assign

@Xunzhuo
Copy link
Member

Xunzhuo commented Apr 13, 2023

Thanks @liangyuanpeng !

@Xunzhuo Xunzhuo removed good first issue Good for newcomers help wanted Extra attention is needed labels Apr 13, 2023
@github-actions
Copy link

This issue has been automatically marked as stale because it has not had activity in the last 30 days.

@github-actions github-actions bot added the stale label May 13, 2023
@Xunzhuo
Copy link
Member

Xunzhuo commented May 18, 2023

@liangyuanpeng hi, what is the status?

@github-actions github-actions bot removed the stale label May 18, 2023
@github-actions
Copy link

This issue has been automatically marked as stale because it has not had activity in the last 30 days.

@github-actions github-actions bot added the stale label Jun 17, 2023
@shawnh2
Copy link
Contributor

shawnh2 commented Apr 17, 2024

This would be very helpful if we bring validations into translate, which will also help the validations in file-resources provider.

For now, some validations are implemented via CEL, maybe what we need to do here is to find a way convert all the CEL rules into validations code.

assign myself this one since I've been doing some works around file-resources provider.

@shawnh2 shawnh2 removed the stale label Apr 17, 2024
@shawnh2 shawnh2 self-assigned this Apr 17, 2024
@zirain
Copy link
Contributor

zirain commented Apr 17, 2024

For now, some validations are implemented via CEL, maybe what we need to do here is to find a way convert all the CEL rules into validations code.

can we run CEL validation locally like what we did in cel validation tests?

@shawnh2
Copy link
Contributor

shawnh2 commented Apr 24, 2024

For now, some validations are implemented via CEL, maybe what we need to do here is to find a way convert all the CEL rules into validations code.

can we run CEL validation locally like what we did in cel validation tests?

Have a discussion about this today in the meeting, it seems we cannot run CEL validation tests locally without kube-api-server support, so may need more investigation.

@shawnh2
Copy link
Contributor

shawnh2 commented Apr 24, 2024

It seems we can take https://github.com/google/cel-go as a example, and input our CEL validation rules like:

ast, issues := env.Compile(`OUR-CEL-RULE`)

and then validate the input object like:

out, details, err := prg.Eval(XXX)

So the only thing left unresolved now is how we retrieve our current CEL rules from comments.

@arkodg
Copy link
Contributor Author

arkodg commented Apr 24, 2024

oh nice @shawnh2 !
we can look into kube-builder converts CEL into x-kubernetes-validations fields

@shawnh2
Copy link
Contributor

shawnh2 commented May 15, 2024

oh nice @shawnh2 ! we can look into kube-builder converts CEL into x-kubernetes-validations fields

sure we can read CEL rules from x-kubernetes-validations fields in all these CRD yaml file. but if we do so, does it mean we need to include all these yaml file ?

maybe we can introduce a knob like --crd-path to indicate all the path for these files.

@arkodg
Copy link
Contributor Author

arkodg commented May 15, 2024

we'll need to codify this, if we need access to the YAML file we can use embed to make it part of the binary

@shawnh2
Copy link
Contributor

shawnh2 commented May 16, 2024

Find a more suitable way to validate k8s CRD object using cel lib provided by apiextensions-apiserver. Here is an example code that works:

import (
	"context"
	"math"
	"testing"
	
	apiextensions "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
	"k8s.io/apiextensions-apiserver/pkg/apiserver/schema"
	celschema "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel"
	"k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/model"
	"k8s.io/apimachinery/pkg/util/validation/field"
	"k8s.io/apimachinery/pkg/util/version"
	celconfig "k8s.io/apiserver/pkg/apis/cel"
	"k8s.io/apiserver/pkg/cel/environment"
)

func TestExprEval(t *testing.T) {
        // Here is going to be our CEL validation schema for each CRD
	objectSchema := schema.Structural{
		Generic: schema.Generic{
			Type: "object",
		},
		Properties: map[string]schema.Structural{
			"nestedObj": {
				Generic: schema.Generic{
					Type: "object",
				},
				Properties: map[string]schema.Structural{
					"val": {
						Generic: schema.Generic{
							Type: "integer",
						},
					},
				},
			},
		},
		Extensions: schema.Extensions{
			XValidations: apiextensions.ValidationRules{
				{
					Rule:    "self.nestedObj.val == 10",
					Message: "val should be equal to 10",
				},
			},
		},
	}

	env, err := environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion()).Extend(
		environment.VersionedOptions{
			IntroducedVersion: version.MajorMinor(1, 999),
		})
	if err != nil {
		t.Fatal(err)
	}
	loader := celschema.NewExpressionsEnvLoader()

	compilationResults, err := celschema.Compile(&objectSchema, model.SchemaDeclType(&objectSchema, false), celconfig.PerCallLimit, env, loader)
	if err != nil {
		t.Fatalf("Expected no error, but got: %v", err)
	}
	t.Log("compile ret", compilationResults)
	
	if len(compilationResults) != len(objectSchema.XValidations) {
		t.Fatalf("complie ret must equal xvalidations size")
	}

	celValidator := celschema.NewValidator(&objectSchema, true, celconfig.PerCallLimit)
	if celValidator == nil {
		t.Fatalf("unexpected nil cel validator")
	}
	
        // Here is going to be the object resource that we'd like to perform validations on
	validateObj := map[string]interface{}{
		"nestedObj": map[string]interface{}{
			"val": 10,
		},
	}

	errs, _ := celValidator.Validate(context.Background(), field.NewPath("root"), &objectSchema, validateObj, nil, math.MaxInt)
	t.Log(errs)
}

// The test pass.

// Changing the 'val' of validateObj to 6, the test fail with reason: "val should be equal to 10" (which is exactly our CEL rule msg)

BTW:

  • The validateObj must be in type map[string]interface{}, so we need convert resource into this type. The easiest approach is to first encode the struct into a JSON string using encoding/json, and then decode the JSON string into a map
  • We need to visit each CRD to build schema.Structural, like objectSchema in this example

FYI: The whole implementation work will based on the idea above, post here first just for better understanding.


Refs:

  1. https://github.com/kubernetes/apiextensions-apiserver/blob/v0.30.1/pkg/apiserver/schema/cel/compilation_test.go
  2. https://github.com/kubernetes/apiextensions-apiserver/blob/v0.30.1/pkg/apiserver/schema/cel/validation_test.go

Copy link

github-actions bot commented Jul 3, 2024

This issue has been automatically marked as stale because it has not had activity in the last 30 days.

@Manoramsharma
Copy link
Contributor

Manoramsharma commented Jul 27, 2024

Hi @arkodg @shawnh2 I was looking forward for help points on this discussion about adding API validation for the functions in egctl translate implementation , under internal/cmd/egctl/translate.go.

I have made some changes in this direction here #3960 , the added resources to the kubernetesYAMLToResources need to have a validity API webhook . Can you guide me more about achieving the same?

@github-actions github-actions bot removed the stale label Jul 27, 2024
Copy link

This issue has been automatically marked as stale because it has not had activity in the last 30 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/egctl kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants