Add proto validation for ProfileService #4905

rdimitrov · 2024-11-07T15:08:51Z

Summary

Ref https://github.com/stacklok/minder-stories/issues/94

Change Type

Mark the type of change your PR introduces:

Bug fix (resolves an issue without affecting existing features)
Feature (adds new functionality without breaking changes)
Breaking change (may impact existing functionalities or require documentation updates)
Documentation (updates or additions to documentation)
Refactoring or test improvements (no bug fixes or new functionality)

Testing

Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.

Review Checklist:

Reviewed my own code for quality and clarity.
Added comments to complex or tricky code sections.
Updated any affected documentation.
Included tests that validate the fix or feature.
Checked that related changes are merged.

coveralls · 2024-11-07T15:20:58Z

coverage: 54.754% (-0.004%) from 54.758%
when pulling 7dd13bb on validate-profiles
into 8b01055 on main.

rdimitrov · 2024-11-07T23:10:11Z

proto/minder/v1/minder.proto

-    string version = 11;
+    string version = 11 [
+        (buf.validate.field).string = {
+            pattern: "^v\\d+(\\.\\d+)?(\\.\\d+)?(?:-[\\w\\.-]+)?$",


Should we have this for the profiles given we might add some notion of versioning in the hopefully not so distant future?

I think we may want only ^v\d$ (or empty) for now.

Addressed 👍

evankanderson · 2024-11-07T23:20:13Z

proto/minder/v1/minder.proto

-    string label_filter = 2;
+    string label_filter = 2 [
+        (buf.validate.field).string = {
+            pattern: "^(\\*|[a-zA-Z][a-zA-Z0-9_]*)$",


I think I was unable to find a usage of this server-side (though maybe that comment was on another PR?), so I'm not sure if it's in use at the moment. I'd enforce lower-case on this for now.

Addressed it 👍

evankanderson · 2024-11-07T23:27:00Z

proto/minder/v1/minder.proto

+                        pattern: "^([a-zA-Z0-9]([-a-zA-Z0-9]{0,61}[a-zA-Z0-9])?:)?[a-zA-Z0-9]([-a-zA-Z0-9]{0,61}[a-zA-Z0-9])?$"
+                    }


Not sure if you want to keep or remove the preceeding comment. Maybe keep until we can generate these in the OpenAPI doc?

Let's leave it for now 👍

evankanderson · 2024-11-07T23:32:02Z

proto/minder/v1/minder.proto

@@ -2290,33 +2332,78 @@ message Profile {
        // id is optional and use for updates to match upserts as well as read operations. It is ignored for creates.
        string id = 1;
        // entity is the entity to select.
-        string entity = 2;
+        string entity = 2 [


We should have called this entity_type, huh?

Yes, that's one of the few examples I was talking about yesterday 😃

evankanderson · 2024-11-07T23:33:31Z

proto/minder/v1/minder.proto

-        string selector = 4;
+        string selector = 4 [
+            (buf.validate.field).string = {
+                pattern: "^[a-zA-Z_][a-zA-Z0-9_]*(?:\\.[a-zA-Z_][a-zA-Z0-9_]*)*(?:\\[\\d+\\])?$",


I don't understand this. This is a CEL expression, correct?

It feels like this might be a special case where we say "this will be parsed as a CEL expression", because I'm nervous about encoding the allowed CEL characters in a regex. (In particular, I suspect they allow for greater-than and less-than expressions...)

Makes sense 👍 So we can leave it without validation for now I suppose?

evankanderson · 2024-11-07T23:34:20Z

proto/minder/v1/minder.proto

-        string description = 6;
+        string description = 6 [
+            (buf.validate.field).string = {
+                pattern: "[A-Za-z][-/.!?,:;[:word:] ]*",


Why not [[:punct:][:word:] ]*? Are there specific characters we're trying to exclude from the punctuation set?

(Also, why a leading letter?)

Yes, punct seems to include < > which we don't want right?
As for the leading letter, I thought it feels better to establish some form of common formatting for when this value is visualised.

evankanderson · 2024-11-07T23:34:59Z

proto/minder/v1/minder.proto

-    optional string remediate = 8;
+    optional string remediate = 8 [
+        (buf.validate.field).string = {
+            pattern: "^(on|off|dry_run)$",


Use in here?

Addressed 👍

evankanderson · 2024-11-07T23:35:31Z

proto/minder/v1/minder.proto

-    string version = 11;
+    string version = 11 [
+        (buf.validate.field).string = {
+            pattern: "^v\\d+(\\.\\d+)?(\\.\\d+)?(?:-[\\w\\.-]+)?$",


I think we may want only ^v\d$ (or empty) for now.

evankanderson · 2024-11-08T14:50:04Z

proto/minder/v1/minder.proto

@@ -1847,7 +1865,7 @@ message ListEvaluationResultsRequest {
        (buf.validate.field).repeated = {
            items: {
                string: {
-                    pattern: "[-[:word:]]*",
+                    pattern: "^[-[:word:]]*$",


Should this include "/"?

oh, actually this should be ^[A-Za-z][-/[:word:]]*$, you're right 👍

evankanderson · 2024-11-08T14:51:13Z

proto/minder/v1/minder.proto

+        (buf.validate.field).string = {
+            pattern: "^v\\d$",
+        }


This is never empty, correct?

Yeah, I checked all of the profiles I could find and it's set everywhere 👍

evankanderson · 2024-11-08T14:52:39Z

proto/minder/v1/minder.proto

@@ -2915,7 +2999,7 @@ message ListEvaluationHistoryRequest {
        (buf.validate.field).repeated = {
            items: {
                string: {
-                    pattern: "[,[:word:]]*",
+                    pattern: "^[,[:word:]]*$",


Why do we allow "," in a repeated field?

Because of -

minder/internal/controlplane/handlers_evalstatus.go

Lines 283 to 287 in 3872633

// of values. Such elements are either "complex", i.e. they represent

// a comma-separated list of sub-elements, or "simple", they do not

// contain comma characters. If element contains one or more comma

// characters, it is further split into sub-elements before calling

// `f` in them.

evankanderson · 2024-11-08T14:52:59Z

proto/minder/v1/minder.proto

@@ -2927,7 +3011,7 @@ message ListEvaluationHistoryRequest {
        (buf.validate.field).repeated = {
            items: {
                string: {
-                    pattern: "[,-./[:word:]]*",
+                    pattern: "^[,-./[:word:]]*$",


Ditto, why allow , here?

Because of -

minder/internal/controlplane/handlers_evalstatus.go

Lines 283 to 287 in 3872633

// of values. Such elements are either "complex", i.e. they represent

// a comma-separated list of sub-elements, or "simple", they do not

// contain comma characters. If element contains one or more comma

// characters, it is further split into sub-elements before calling

// `f` in them.

evankanderson · 2024-11-08T14:53:33Z

proto/minder/v1/minder.proto

@@ -2951,7 +3035,7 @@ message ListEvaluationHistoryRequest {
        (buf.validate.field).repeated = {
            items: {
                string: {
-                    pattern: "[,[:word:]]*",
+                    pattern: "^[,[:word:]]*$",


Again, , seems like an error converting to a repeated field

Because of -

minder/internal/controlplane/handlers_evalstatus.go

Lines 283 to 287 in 3872633

// of values. Such elements are either "complex", i.e. they represent

// a comma-separated list of sub-elements, or "simple", they do not

// contain comma characters. If element contains one or more comma

// characters, it is further split into sub-elements before calling

// `f` in them.

evankanderson · 2024-11-08T14:53:45Z

proto/minder/v1/minder.proto

@@ -2963,7 +3047,7 @@ message ListEvaluationHistoryRequest {
        (buf.validate.field).repeated = {
            items: {
                string: {
-                    pattern: "[,[:word:]]*",
+                    pattern: "^[,[:word:]]*$",


Because of -

minder/internal/controlplane/handlers_evalstatus.go

Lines 283 to 287 in 3872633

// of values. Such elements are either "complex", i.e. they represent

// a comma-separated list of sub-elements, or "simple", they do not

// contain comma characters. If element contains one or more comma

// characters, it is further split into sub-elements before calling

// `f` in them.

evankanderson · 2024-11-08T14:53:53Z

proto/minder/v1/minder.proto

@@ -2975,7 +3059,7 @@ message ListEvaluationHistoryRequest {
        (buf.validate.field).repeated = {
            items: {
                string: {
-                    pattern: "[,[:word:]]*",
+                    pattern: "^[,[:word:]]*$",


Because of -

minder/internal/controlplane/handlers_evalstatus.go

Lines 283 to 287 in 3872633

// of values. Such elements are either "complex", i.e. they represent

// a comma-separated list of sub-elements, or "simple", they do not

// contain comma characters. If element contains one or more comma

// characters, it is further split into sub-elements before calling

// `f` in them.

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

rdimitrov requested a review from a team as a code owner November 7, 2024 15:08

rdimitrov self-assigned this Nov 7, 2024

rdimitrov force-pushed the validate-profiles branch 2 times, most recently from 7c4398d to bddddb6 Compare November 7, 2024 23:05

rdimitrov commented Nov 7, 2024

View reviewed changes

evankanderson reviewed Nov 7, 2024

View reviewed changes

rdimitrov force-pushed the validate-profiles branch 2 times, most recently from 42e238f to 0f98428 Compare November 8, 2024 14:13

evankanderson reviewed Nov 8, 2024

View reviewed changes

rdimitrov added 5 commits November 8, 2024 17:10

Add proto validation for ProfileService

c55e04d

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

Refactor the name and type validations

b3eaa89

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

Refactor the validations for selectors and labels

7c02bbc

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

Update validations to have a full word match, not partial

e6d5a97

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

Update the validation for display name and rule type name

7dd13bb

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

rdimitrov force-pushed the validate-profiles branch from 899d337 to 7dd13bb Compare November 8, 2024 15:11

evankanderson approved these changes Nov 8, 2024

View reviewed changes

rdimitrov merged commit 4aa7e72 into main Nov 8, 2024
25 checks passed

rdimitrov deleted the validate-profiles branch November 8, 2024 15:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add proto validation for ProfileService #4905

Add proto validation for ProfileService #4905

rdimitrov commented Nov 7, 2024

coveralls commented Nov 7, 2024 •

edited

Loading

rdimitrov Nov 7, 2024

evankanderson Nov 7, 2024

rdimitrov Nov 8, 2024

evankanderson Nov 7, 2024

rdimitrov Nov 8, 2024

evankanderson Nov 7, 2024

rdimitrov Nov 8, 2024

evankanderson Nov 7, 2024

rdimitrov Nov 8, 2024

evankanderson Nov 7, 2024

rdimitrov Nov 8, 2024

evankanderson Nov 7, 2024

rdimitrov Nov 8, 2024

evankanderson Nov 7, 2024

rdimitrov Nov 8, 2024

evankanderson Nov 7, 2024

evankanderson Nov 8, 2024

rdimitrov Nov 8, 2024

evankanderson Nov 8, 2024

rdimitrov Nov 8, 2024

evankanderson Nov 8, 2024

rdimitrov Nov 8, 2024

evankanderson Nov 8, 2024

rdimitrov Nov 8, 2024

evankanderson Nov 8, 2024

rdimitrov Nov 8, 2024

evankanderson Nov 8, 2024

rdimitrov Nov 8, 2024

evankanderson Nov 8, 2024

rdimitrov Nov 8, 2024

		pattern: "^([a-zA-Z0-9]([-a-zA-Z0-9]{0,61}[a-zA-Z0-9])?:)?[a-zA-Z0-9]([-a-zA-Z0-9]{0,61}[a-zA-Z0-9])?$"
		}

	// of values. Such elements are either "complex", i.e. they represent
	// a comma-separated list of sub-elements, or "simple", they do not
	// contain comma characters. If element contains one or more comma
	// characters, it is further split into sub-elements before calling
	// `f` in them.

Add proto validation for ProfileService #4905

Add proto validation for ProfileService #4905

Conversation

rdimitrov commented Nov 7, 2024

Summary

Change Type

Testing

Review Checklist:

coveralls commented Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 7, 2024 •

edited

Loading