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 validation for secret name #596

Merged
merged 3 commits into from Mar 9, 2019

Conversation

4 participants
@viveksyngh
Copy link
Member

viveksyngh commented Jan 28, 2019

This commit add validation (subdomain in RFC-1123) for secret name while
creating the secret name.

Fixes #590

Signed-off-by: Vivek Singh vivekkmr45@yahoo.in

Description

Motivation and Context

  • I have raised an issue to propose this change (required)
    #590

How Has This Been Tested?

./faas-cli secret create my-secret --from-literal=value
Creating secret: my-secret
WARNING! Communication is not secure, please consider using HTTPS. Letsencrypt.org offers free SSL/TLS certificates.
Created: 201 Created
➜  faas-cli git:(add_secrent_name_validation) ./faas-cli secret create my_secret --from-literal=value
ERROR: invalid secret name: my_secret, secret name must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (regex used for validation is ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.
Add validation for secret name
This commit add validation (subdomain in RFC-1123) for secret name while
creating the secret name.

Fixes #590

Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>
}

func Test_validateSecretName_Invalid(t *testing.T) {
secretName := "api_key_@secret"

This comment has been minimized.

@LucasRoesler

LucasRoesler Jan 28, 2019

Member

Are there any other test cases we want to add here, this would be a reasonable place for a table test

This comment has been minimized.

@alexellis

alexellis Jan 29, 2019

Member

Good point

We should have positive/negative tests here.

@@ -104,3 +107,23 @@ func readSecretFromFile(secretFile string) (string, error) {
fileData, err := ioutil.ReadFile(secretFile)
return string(fileData), err
}

//kubectl create secret generic my_secret --from-literal=my_secret=my_secret

This comment has been minimized.

@alexellis

alexellis Jan 29, 2019

Member

Can we remove this comment?

//kubectl create secret generic my_secret --from-literal=my_secret=my_secret
//The Secret "my_secret" is invalid: metadata.name: Invalid value: "my_secret": a DNS-1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')

// Kubernetes DNS-1123 Subdomain Regex

This comment has been minimized.

@alexellis

alexellis Jan 29, 2019

Member

Add the comment to the regex itself

@viveksyngh viveksyngh force-pushed the viveksyngh:add_secrent_name_validation branch from d1b15ab to 3fedbe0 Feb 8, 2019

Add multiple tests for secret name validation
Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>

@viveksyngh viveksyngh force-pushed the viveksyngh:add_secrent_name_validation branch from 3fedbe0 to 008f37c Feb 8, 2019

@alexellis alexellis added this to Review in Triage March 2019 Mar 2, 2019

@alexellis alexellis requested a review from rgee0 Mar 2, 2019

const (
dns1123LabelFmt string = "[a-z0-9]([-a-z0-9]*[a-z0-9])?"
dns1123SubdomainFmt string = dns1123LabelFmt + "(\\." + dns1123LabelFmt + ")*"
invalidSecretNameMessage string = `ERROR: invalid secret name: %s, secret name must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (regex used for validation is %s)`

This comment has been minimized.

@burtonr

burtonr Mar 9, 2019

Member

I'd prefer to see this error message spread out one multiple lines. Having to scroll, or wrapped text makes it difficult to read.

Also, perhaps it would be better to describe the regex rather than printing it out in the raw
Something like:

Error: Invalid secret name.
Secret name must start and end with alphanumeric character
and can only contain lower-case alphanumeric characters, '.', or '-'
@burtonr

This comment has been minimized.

Copy link
Member

burtonr commented Mar 9, 2019

@viveksyngh It looks like this PR is very nearly there. Just some minor comments to address before this can be merged.
Any chance we could get an update?

Remove regex from error message and re-format it
Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>
@alexellis
Copy link
Member

alexellis left a comment

LGTM

@alexellis alexellis merged commit 8dfe6ac into openfaas:master Mar 9, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Mar 9, 2019

Looks great. Thank you for all your work on this PR Vivek.

@burtonr burtonr moved this from Review to Merge in Triage March 2019 Mar 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.