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

Improve secrets api errors #362

Open
LucasRoesler opened this Issue Jan 25, 2019 · 13 comments

Comments

@LucasRoesler
Copy link
Member

LucasRoesler commented Jan 25, 2019

The Secrets API current returns all errors from the k8s cluster as 500 Server Error. This obfuscates validation errors and can be confusing.

For example @rgee0 shared this in Slack

$ echo 'burt' | faas secret create I_LOVE_UNDERSCORES
Creating secret: I_LOVE_UNDERSCORES
WARNING! Communication is not secure, please consider using HTTPS. Letsencrypt.org offers free SSL/TLS certificates.
server returned unexpected status code: 500 - 

$ echo 'burt' | faas secret create i-prefer-lowercase
Creating secret: i-prefer-lowercase
WARNING! Communication is not secure, please consider using HTTPS. Letsencrypt.org offers free SSL/TLS certificates.
Created: 202 Accepted

Expected Behaviour

If the error is a known error, for example a validation error about the secret name, then we should return the appropriate http status code

$ echo 'burt' | faas secret create I_LOVE_UNDERSCORES
Creating secret: I_LOVE_UNDERSCORES
ERROR 422: The Secret "I_LOVE_UNDERSCORES" is invalid: metadata.name: Invalid value: "I_LOVE_UNDERSCORES": 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])?)*')

Current Behaviour

All errors are returned at 500

$ echo 'burt' | faas secret create I_LOVE_UNDERSCORES
Creating secret: I_LOVE_UNDERSCORES
WARNING! Communication is not secure, please consider using HTTPS. Letsencrypt.org offers free SSL/TLS certificates.
server returned unexpected status code: 500 - 

Possible Solution

The is comes from this block of code

_, err = kube.Create(secret)
if err != nil {
w.WriteHeader(http.StatusInternalServerError)
log.Printf("Secret create error: %v\n", err)
return
}

Instead of returning the plain error, we should use the ReasonForError helper in the k8s.io/apimachinery/pkg/api/errors package defined here https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/api/errors/errors.go#L575

to capture the reason and decide the appropriate API response. For example

 _, err = kube.Create(secret)
switch {
case k8serrors.ReasonForError(err) == metav1.StatusReasonNotAcceptable:
    w.WriteHeader(http.StatusUnprocessableEntity)
// and other cases ...
case err == nil:
// do nothing
default:
    w.WriteHeader(http.StatusInternalServerError)
    log.Printf("Secret create error: %v\n", err)
    return
}

This should be done for each endpoint, not just the creation endpoint because there are various errors that could arise due to cluster configuration.

Steps to Reproduce (for bugs)

  1. install the latest openfaas on k8s
  2. install the latest CLI
  3. execute echo 'burt' | faas secret create I_LOVE_UNDERSCORES

Context

Managing secrets via the openfaas CLI simplifies the cognitive overhead and improves the scriptability of OpenFaaS. The current error messages are not informative enough to fix the underlying issue

@LucasRoesler

This comment has been minimized.

Copy link
Member Author

LucasRoesler commented Jan 25, 2019

https://httpstatuses.com/ is a good reference for the different standard http status codes and there meanings. We should be thoughtful to map to those where possible

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Jan 30, 2019

Derek add label: good first issue

@derek derek bot added the good first issue label Jan 30, 2019

@rajatjindal

This comment has been minimized.

Copy link

rajatjindal commented Jan 31, 2019

I want to give this a shot this weekend. let me see if Derek listens to me.

Derek assign :me

@LucasRoesler

This comment has been minimized.

Copy link
Member Author

LucasRoesler commented Jan 31, 2019

@rajatjindal if you have any questions feel free to ask me. Also, ping me when you have a pull request ready

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Mar 2, 2019

@rajatjindal it's been about a month since you picked up this task. During that time nobody else has been able to work on it. What's your current update? If you've not made any progress would you like to let us know so we can find another contributor? Likewise if you're in need to technical help please ask here.

@rajatjindal

This comment has been minimized.

Copy link

rajatjindal commented Mar 2, 2019

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Mar 2, 2019

It's no problem.

Lucas please could you look for someone else and if you don't find someone by mid-next week pick it up or throw it over to me if necessary.

@acornies

This comment has been minimized.

Copy link
Member

acornies commented Mar 3, 2019

Derek assign :me

@rgee0

This comment has been minimized.

Copy link
Member

rgee0 commented Mar 3, 2019

/assign: acornies

@derek derek bot assigned acornies Mar 3, 2019

@acornies

This comment has been minimized.

Copy link
Member

acornies commented Mar 7, 2019

@LucasRoesler Looks like we need to update the k8s deps to have access to the exported ReasonForError method. I'm bumping to 1.9. Should we go higher?

@LucasRoesler

This comment has been minimized.

Copy link
Member Author

LucasRoesler commented Mar 8, 2019

@acornies good question. I think there is a compatibility matrix somewhere, just found it https://github.com/kubernetes/client-go#compatibility-matrix I am not sure what versions of k8s we have promised to support. @alexellis perhaps we should make that more explicit in our Readme

@acornies acornies referenced this issue Mar 8, 2019

Open

Adding ReasonForError usage: #384

7 of 11 tasks complete

@acornies acornies moved this from Help wanted to Review in Triage March 2019 Mar 8, 2019

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Mar 10, 2019

I think @stefanprodan has a better idea of that. The CRDs and operator mandate a minimum version, but faas-netes can go back to older K8s API versions.

If this change is pinning us to a minimum level of 1.9x I think that's OK.

@stefanprodan

This comment has been minimized.

Copy link
Member

stefanprodan commented Mar 11, 2019

Yes 1.9 would be the best choice since we want to support as many K8s version as possible. I think is a good idea to post in the readme that the minimum version supported by faas-netes is 1.9 after the upgrade.

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.