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.

Sign up for GitHub

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

Jump to bottom

Escape CEL Reserved Keywords when Generating Validation Rules #255

Merged
merged 3 commits into from
Aug 21, 2023
Merged

Escape CEL Reserved Keywords when Generating Validation Rules #255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bd187bd
escapes cel reserved keywords when generating validation rules
Aug 14, 2023
1825930
updated test case name to be more consistent with existing names
dalton-hill-0 Aug 14, 2023
322c5eb
removes unnecessary calls to test.EquateErrors() in test comparison
dalton-hill-0 Aug 17, 2023
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
24 changes: 22 additions & 2 deletions pkg/types/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,16 @@ const (
wildcard = "*"

emptyStruct = "struct{}"

// ref: https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#validation-rules
celEscapeSequence = "__%s__"
)

var (
// ref: https://github.com/google/cel-spec/blob/v0.6.0/doc/langdef.md#syntax
celReservedKeywords = []string{"true", "false", "null", "in", "as", "break", "const", "continue",
Copy link
Collaborator

@ulucinar ulucinar Aug 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

nit: We may use a map[string]struct{} (or the higher-level "k8s.io/apimachinery/pkg/util/sets.Set ) for O(1) lookups. Also the list we search has a relatively static small size.

"else", "for", "function", "if", "import", "let", "loop", "package", "namespace", "return", "var",
"void", "while"}
)

// Generated is a struct that holds generated types
Expand Down Expand Up @@ -139,10 +149,11 @@ func (g *Builder) AddToBuilder(typeNames *TypeNames, r *resource) (*types.Named,

for _, p := range r.topLevelRequiredParams {
g.validationRules += "\n"
sp := sanitizePath(p.path)
if p.includeInit {
g.validationRules += fmt.Sprintf(`// +kubebuilder:validation:XValidation:rule="!('*' in self.managementPolicies || 'Create' in self.managementPolicies || 'Update' in self.managementPolicies) || has(self.forProvider.%s) || has(self.initProvider.%s)",message="%s is a required parameter"`, p.path, p.path, p.path)
g.validationRules += fmt.Sprintf(`// +kubebuilder:validation:XValidation:rule="!('*' in self.managementPolicies || 'Create' in self.managementPolicies || 'Update' in self.managementPolicies) || has(self.forProvider.%s) || has(self.initProvider.%s)",message="%s is a required parameter"`, sp, sp, p.path)
} else {
g.validationRules += fmt.Sprintf(`// +kubebuilder:validation:XValidation:rule="!('*' in self.managementPolicies || 'Create' in self.managementPolicies || 'Update' in self.managementPolicies) || has(self.forProvider.%s)",message="%s is a required parameter"`, p.path, p.path)
g.validationRules += fmt.Sprintf(`// +kubebuilder:validation:XValidation:rule="!('*' in self.managementPolicies || 'Create' in self.managementPolicies || 'Update' in self.managementPolicies) || has(self.forProvider.%s)",message="%s is a required parameter"`, sp, p.path)
}
}

Expand Down Expand Up @@ -444,3 +455,12 @@ func fieldPathWithWildcard(parts []string) string {
}
return seg.String()
}

func sanitizePath(p string) string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit:

Suggested change
func sanitizePath(p string) string {
func escapeName(p string) string {

for _, reserved := range celReservedKeywords {
if p == reserved {
return fmt.Sprintf(celEscapeSequence, p)
}
}
return p
}
52 changes: 47 additions & 5 deletions pkg/types/builder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,9 +208,10 @@ func TestBuild(t *testing.T) {
cfg *config.Resource
}
type want struct {
forProvider string
atProvider string
err error
forProvider string
atProvider string
validationRules string
err error
}
cases := map[string]struct {
args
Expand Down Expand Up @@ -251,6 +252,9 @@ func TestBuild(t *testing.T) {
want: want{
forProvider: `type example.Parameters struct{Enable *bool "json:\"enable,omitempty\" tf:\"enable,omitempty\""; ID *int64 "json:\"id,omitempty\" tf:\"id,omitempty\""; Name *string "json:\"name,omitempty\" tf:\"name,omitempty\""}`,
atProvider: `type example.Observation struct{Config *string "json:\"config,omitempty\" tf:\"config,omitempty\""; Enable *bool "json:\"enable,omitempty\" tf:\"enable,omitempty\""; ID *int64 "json:\"id,omitempty\" tf:\"id,omitempty\""; Name *string "json:\"name,omitempty\" tf:\"name,omitempty\""; Value *float64 "json:\"value,omitempty\" tf:\"value,omitempty\""}`,
validationRules: `
// +kubebuilder:validation:XValidation:rule="!('*' in self.managementPolicies || 'Create' in self.managementPolicies || 'Update' in self.managementPolicies) || has(self.forProvider.id) || has(self.initProvider.id)",message="id is a required parameter"
// +kubebuilder:validation:XValidation:rule="!('*' in self.managementPolicies || 'Create' in self.managementPolicies || 'Update' in self.managementPolicies) || has(self.forProvider.name) || has(self.initProvider.name)",message="name is a required parameter"`,
},
},
"Resource_Types": {
Expand Down Expand Up @@ -284,6 +288,9 @@ func TestBuild(t *testing.T) {
want: want{
forProvider: `type example.Parameters struct{List []*string "json:\"list,omitempty\" tf:\"list,omitempty\""; ResourceIn map[string]example.ResourceInParameters "json:\"resourceIn,omitempty\" tf:\"resource_in,omitempty\""}`,
atProvider: `type example.Observation struct{List []*string "json:\"list,omitempty\" tf:\"list,omitempty\""; ResourceIn map[string]example.ResourceInParameters "json:\"resourceIn,omitempty\" tf:\"resource_in,omitempty\""; ResourceOut map[string]example.ResourceOutObservation "json:\"resourceOut,omitempty\" tf:\"resource_out,omitempty\""}`,
validationRules: `
// +kubebuilder:validation:XValidation:rule="!('*' in self.managementPolicies || 'Create' in self.managementPolicies || 'Update' in self.managementPolicies) || has(self.forProvider.list) || has(self.initProvider.list)",message="list is a required parameter"
// +kubebuilder:validation:XValidation:rule="!('*' in self.managementPolicies || 'Create' in self.managementPolicies || 'Update' in self.managementPolicies) || has(self.forProvider.resourceIn) || has(self.initProvider.resourceIn)",message="resourceIn is a required parameter"`,
},
},
"Sensitive_Fields": {
Expand Down Expand Up @@ -311,6 +318,9 @@ func TestBuild(t *testing.T) {
want: want{
forProvider: `type example.Parameters struct{Key1SecretRef *github.com/crossplane/crossplane-runtime/apis/common/v1.SecretKeySelector "json:\"key1SecretRef,omitempty\" tf:\"-\""; Key2SecretRef github.com/crossplane/crossplane-runtime/apis/common/v1.SecretKeySelector "json:\"key2SecretRef\" tf:\"-\""; Key3SecretRef []github.com/crossplane/crossplane-runtime/apis/common/v1.SecretKeySelector "json:\"key3SecretRef\" tf:\"-\""}`,
atProvider: `type example.Observation struct{}`,
validationRules: `
// +kubebuilder:validation:XValidation:rule="!('*' in self.managementPolicies || 'Create' in self.managementPolicies || 'Update' in self.managementPolicies) || has(self.forProvider.key2SecretRef)",message="key2SecretRef is a required parameter"
// +kubebuilder:validation:XValidation:rule="!('*' in self.managementPolicies || 'Create' in self.managementPolicies || 'Update' in self.managementPolicies) || has(self.forProvider.key3SecretRef)",message="key3SecretRef is a required parameter"`,
},
},
"Invalid_Sensitive_Fields": {
Expand Down Expand Up @@ -356,6 +366,8 @@ func TestBuild(t *testing.T) {
want: want{
forProvider: `type example.Parameters struct{Name *string "json:\"name,omitempty\" tf:\"name,omitempty\""; ReferenceID *string "json:\"referenceId,omitempty\" tf:\"reference_id,omitempty\""; ExternalResourceID *github.com/crossplane/crossplane-runtime/apis/common/v1.Reference "json:\"externalResourceId,omitempty\" tf:\"-\""; ReferenceIDSelector *github.com/crossplane/crossplane-runtime/apis/common/v1.Selector "json:\"referenceIdSelector,omitempty\" tf:\"-\""}`,
atProvider: `type example.Observation struct{Name *string "json:\"name,omitempty\" tf:\"name,omitempty\""; ReferenceID *string "json:\"referenceId,omitempty\" tf:\"reference_id,omitempty\""}`,
validationRules: `
// +kubebuilder:validation:XValidation:rule="!('*' in self.managementPolicies || 'Create' in self.managementPolicies || 'Update' in self.managementPolicies) || has(self.forProvider.name) || has(self.initProvider.name)",message="name is a required parameter"`,
},
},
"Invalid_Schema_Type": {
Expand All @@ -375,6 +387,33 @@ func TestBuild(t *testing.T) {
err: errors.Wrapf(errors.Wrapf(errors.Errorf("invalid schema type %s", "TypeInvalid"), "cannot infer type from schema of field %s", "name"), "cannot build the Types"),
},
},
"Validation_Rules_With_Keywords": {
args: args{
cfg: &config.Resource{
TerraformResource: &schema.Resource{
Schema: map[string]*schema.Schema{
"name": {
Type: schema.TypeString,
Required: true,
},
// "namespace" is a cel reserved value and should be wrapped when used in
// validation rules (i.e., __namespace__)
"namespace": {
Type: schema.TypeString,
Required: true,
},
},
},
},
},
want: want{
forProvider: `type example.Parameters struct{Name *string "json:\"name,omitempty\" tf:\"name,omitempty\""; Namespace *string "json:\"namespace,omitempty\" tf:\"namespace,omitempty\""}`,
atProvider: `type example.Observation struct{Name *string "json:\"name,omitempty\" tf:\"name,omitempty\""; Namespace *string "json:\"namespace,omitempty\" tf:\"namespace,omitempty\""}`,
validationRules: `
// +kubebuilder:validation:XValidation:rule="!('*' in self.managementPolicies || 'Create' in self.managementPolicies || 'Update' in self.managementPolicies) || has(self.forProvider.name) || has(self.initProvider.name)",message="name is a required parameter"
// +kubebuilder:validation:XValidation:rule="!('*' in self.managementPolicies || 'Create' in self.managementPolicies || 'Update' in self.managementPolicies) || has(self.forProvider.__namespace__) || has(self.initProvider.__namespace__)",message="namespace is a required parameter"`,
},
},
}
for n, tc := range cases {
t.Run(n, func(t *testing.T) {
Expand All @@ -385,15 +424,18 @@ func TestBuild(t *testing.T) {
t.Fatalf("Build(...): -want error, +got error: %s", diff)
}
if g.ForProviderType != nil {
if diff := cmp.Diff(tc.want.forProvider, g.ForProviderType.Obj().String(), test.EquateErrors()); diff != "" {
if diff := cmp.Diff(tc.want.forProvider, g.ForProviderType.Obj().String()); diff != "" {
t.Fatalf("Build(...): -want forProvider, +got forProvider: %s", diff)
}
}
if g.AtProviderType != nil {
if diff := cmp.Diff(tc.want.atProvider, g.AtProviderType.Obj().String(), test.EquateErrors()); diff != "" {
if diff := cmp.Diff(tc.want.atProvider, g.AtProviderType.Obj().String()); diff != "" {
t.Fatalf("Build(...): -want atProvider, +got atProvider: %s", diff)
}
}
if diff := cmp.Diff(tc.want.validationRules, g.ValidationRules); diff != "" {
t.Fatalf("Build(...): -want validationRules, +got validationRules: %s", diff)
}
})
}
}
Loading