Validate space name with a regexp in the controller (OSIO#3580) #2100
Conversation
xcoulon
force-pushed
the
IssueOSIO3580_delete_space
branch
from
553c482
to
f595e14
Compare
This will prevent creating spaces with a name that cannot be used as a value in pod labels (matching max length and pattern). The regexp and length checks in the controller replace the validation function that was previously used at the design level, which allows for returning proper JSON-API errors to the clients. Fixes openshiftio/openshift.io#3580 Signed-off-by: Xavier Coulon <[email protected]>
xcoulon
force-pushed
the
IssueOSIO3580_delete_space
branch
from
f595e14
to
87bf371
Compare
controller/space.go
Outdated
| @@ -546,6 +547,18 @@ func (c *SpaceController) Update(ctx *app.UpdateSpaceContext) error { | |||
| return ctx.OK(&response) | |||
| } | |||
|
|
|||
| const ( | |||
| // see https://github.com/kubernetes/community/blob/master/contributors/design-proposals/architecture/identifiers.md | |||
| spaceNameMaxLength int = 63 | |||
There was a problem hiding this comment.
yes, but I remove the use of the nameValidationFunction at the design level to validate the space name, actually. I'm not sure why the same length rule applies to area, etc., though...
Signed-off-by: Xavier Coulon <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #2100 +/- ##
==========================================
- Coverage 69.05% 69.03% -0.02%
==========================================
Files 162 162
Lines 15115 15105 -10
==========================================
- Hits 10437 10428 -9
- Misses 3730 3734 +4
+ Partials 948 943 -5
Continue to review full report at Codecov.
|
|
@xcoulon Your image is available in the registry. Run |
|
The PR looks good to me. |
Also, enforce name requirement in the creatio operation, which requires its own type in the design also, rename the legacy `system.space` to `system-space` to avoid test failures, and replace all ` ` with `-` in other tests as well. Signed-off-by: Xavier Coulon <[email protected]>
@xcoulon I find it highly interesting in the first place that we let ourselves dictated space name rules by some architecture. Shouldn't this architecture below, namely k8, just not care about space names? |
|
@kwk right, the space name rule is bound to the namespace rule, indeed. That's for now the simplest way to create resources in relation with a given space on k8s, without having to deal with conflicting names if we decided to have a transformation from space name to namespace. |
But if we need a name in k8 for something, can't we use the space's ID instead? That's unique. |
| var spacenameValidationFunction = func() { | ||
| a.MaxLength(63) // maximum name length is 63 characters | ||
| a.MinLength(1) // minimum name length is 1 characters | ||
| a.Pattern("^([A-Za-z0-9][-A-Za-z0-9]*)?[A-Za-z0-9]$") |
There was a problem hiding this comment.
So Kubernetes follows the RFC 1123 with respect to naming the namespaces. This RFC talks about the naming of hosts, since the namespace name then becomes the route/FQDN.
So the regex for the validation seems wrong, here is a reference from upstream
https://github.com/kubernetes/apimachinery/blob/241e268dc8e07223510442ab7d7902695c585d80/pkg/util/validation/validation.go#L106-L107
There was a problem hiding this comment.
@surajssd if RFC 1123 can also handle UUIDs, then I'd says we use a space's ID in pods. @aslakknutsen is that possible? I don't see a good reason for why the pod naming conventions should block us from having more relaxed space naming conventions.
There was a problem hiding this comment.
@kwk we don't name pods directly, I think we name things like the namespaces that are created in the user's account viz. run, stage, jenkins, etc. These are appended with their usernames, so they are unique across the cluster username-nstype as combination.
And about the pod names, pod get their names from the deployment or deploymentconfig we create, for example if you create a deployment with name foo then the pods get names like foo-xxx-xxxx. (here x are random strings)
And sorry my bad about the comment on the regex being wrong the rightful regex for validating the label value is
This will prevent creating spaces with a name that cannot be used
as a value in pod labels.
The regexp used in the controller replaces the validation function
that was previously used at the design level, which allows for returning
proper JSON-API errors to the clients.
Fixes openshiftio/openshift.io#3580
Signed-off-by: Xavier Coulon [email protected]