[feat] validate application name lenth #2631
Conversation
Signed-off-by: Arsen Gumin <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@ChenYi015, Hi! Check my PR pls |
internal/webhook/webhook.go
Outdated
| var allErrs field.ErrorList | ||
| if len(appMeta.Name) > maxAppNameLength { | ||
| allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "name"), appMeta.Name, | ||
| fmt.Sprintf("name must be no more than %d characters to allow for resource suffixes", maxAppNameLength))) |
There was a problem hiding this comment.
| fmt.Sprintf("name must be no more than %d characters to allow for resource suffixes", maxAppNameLength))) | |
| fmt.Sprintf("name length must not exceed %d characters to allow for resource suffixes", maxAppNameLength))) |
A bit easier to read this way imo
| return nil, nil | ||
| } | ||
|
|
||
| if err := v.validateMetadata(ctx, newApp); err != nil { |
There was a problem hiding this comment.
Do we need this for updates?
| "context" | ||
| "fmt" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/util/validation/field" |
There was a problem hiding this comment.
Add a blank line between std and third-party imports to pass the golangci-lint check.
| "context" | |
| "fmt" | |
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | |
| "k8s.io/apimachinery/pkg/util/validation/field" | |
| "context" | |
| "fmt" | |
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | |
| "k8s.io/apimachinery/pkg/util/validation/field" |
| err := validateNameLength(ctx, meta) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Can be simplified as:
| err := validateNameLength(ctx, meta) | |
| if err != nil { | |
| return err | |
| } | |
| return nil | |
| if err := validateNameLength(ctx, meta); err != nil { | |
| return err | |
| } | |
| return nil |
internal/webhook/webhook.go
Outdated
| // and early error message. | ||
| func validateNameLength(_ context.Context, appMeta metav1.ObjectMeta) error { | ||
| var allErrs field.ErrorList | ||
| if len(appMeta.Name) > maxAppNameLength { |
There was a problem hiding this comment.
We only need to make sure the length of name is up to 63, see #2509 (comment).
| if len(appMeta.Name) > maxAppNameLength { | |
| if len(appMeta.Name) > validation.LabelValueMaxLength { |
There was a problem hiding this comment.
We also need to check the web UI service name is a DNS-1035 label name when enabled:
svcName := util.GetDefaultUIServiceName(app)
if !validation.IsDNS1035Label(svcName)) {
...
}
| version: 4.0.0 | ||
| instances: 1 | ||
| cores: 1 | ||
| memory: 512m No newline at end of file |
There was a problem hiding this comment.
Add a new line at the end of file.
Signed-off-by: Arsen Gumin <[email protected]>
aagumin
force-pushed
the
bugfix/issue-2509/sparkapp-name-len-check
branch
from
6082bbc to
344a2c4
Compare
|
Hi! Please look my PR again |
3 participants
Purpose of this PR
Proposed changes:
it's fix for #2509
Change Category
Rationale
Checklist
Additional Notes