MGMT-15683: Ensure that manifest filename has valid name part #5635
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@paul-maidment: This pull request references MGMT-15683 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.15.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci
bot
added
the
size/S
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: paul-maidment The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #5635 +/- ##
==========================================
+ Coverage 67.71% 67.75% +0.03%
==========================================
Files 233 233
Lines 34211 34219 +8
==========================================
+ Hits 23166 23184 +18
+ Misses 8982 8976 -6
+ Partials 2063 2059 -4
|
|
/lgtm |
Presently when uploading a manifest, it's possible to upload a filename with just an extension, for example a filename '.yaml' is considered to be valid. Clearly this is incorrect and this PR seeks to rectify this issue.
paul-maidment
force-pushed
the
MGMT-15683
branch
from
f3a3c07
to
7da7d2d
Compare
| if len(strings.TrimSpace(fileNameWithoutExtension)) == 0 { | ||
| return m.prepareAndLogError( | ||
| ctx, | ||
| http.StatusUnprocessableEntity, |
There was a problem hiding this comment.
Why StatusUnprocessableEntity? Seems that is not very much used throughout the codebase, any particular reason why we picked this one as opposed to the more used http.StatusBadRequest? I don't disagree with the choice, but I'd prefer consistency with other validation if there isn't a compelling reason to not be consistent
There was a problem hiding this comment.
This is basically in line with bugfixes I was asked to make in https://issues.redhat.com/browse/MGMT-15684 where filename non compliance is requested to be treated as StatusUnprocessableEntity as opposed to BadRequest
Happy to open this up for broader debate but I felt that this was the best way to remain consistent?
There was a problem hiding this comment.
It might be more correct, not sure about consistency since this would be the only validation returning 422.
Sorry for nitpicking on this, I don't mind either way (400 or 422), but I do feel strongly about consistency of the API. AFAIU this would be the only endpoint possibly returning 422. @avishayt thoughts?
There was a problem hiding this comment.
I agree with @rccrdpccl and apologize for not looking more deeply into MGMT-15684. It looks like this is another example of surprising code generation. We have a pattern defined in the swagger for the file name, and I assume that returns 422 based on the JIRA. Our code generally just returns 400 for invalid input. The better fix would be to switch these cases to 400 and see how to fix the code generation so that it also returns 400.
There was a problem hiding this comment.
"and see how to fix the code generation so that it also returns 400" <-
So alter the code generation for the API in such a way that it returns a 400 error code for all cases where it presently returns a 422, can you clarify that this is what you are suggesting @avishayt ?
There was a problem hiding this comment.
I'm actually not sure I agree that this should be changed in the way described.
As evidence, here are the definitions of 422 and 400 error codes as described by Mozilla.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/422
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/400
"The HyperText Transfer Protocol (HTTP) 422 Unprocessable Content response status code indicates that the server understands the content type of the request entity, and the syntax of the request entity is correct, but it was unable to process the contained instructions. "
vs
"The HyperText Transfer Protocol (HTTP) 400 Bad Request response status code indicates that the server cannot or will not process the request due to something that is perceived to be a client error (for example, malformed request syntax, invalid request message framing, or deceptive request routing)."
In all of the cases we are discussing, the syntax is correct but the errors are semantic (bad filename)
I would argue that we should be using 422...
Additionally, it looks like our codegen is based on goswagger and any change to the default behavior would require us to either define a template of our own or to deviate from the "factory" behavior of swagger. Or perhaps upgrade swagger to a new version.
Based on my analysis above, I also believe that the template being used in goswagger is correct and that 422 is the appropriate error code to return in this instance.
To put it another way, I don't think a pull request to the project containing the stratoscale template would be accepted as we are expecting "incorrect" behavior as a default.
I heard some discussion that some clients don't support a 422 error code, though I have not seen any concrete documentation of this problem.
@avishayt What are your thoughts?
There was a problem hiding this comment.
I'm OK with changing the generated code to 400 in a different PR as well.
We own the stratoscale template, so no worries there. 422 might be closer in terms of meaning, but realistically, clients won't differentiate between 400 and 422. Browsers will likely see 422 and fall back to 400 (user error). So I'd rather go with consistency and simplicity over possibly being more "correct" in this case.
There was a problem hiding this comment.
I'm OK with changing the generated code to 400 in a different PR as well. We own the stratoscale template, so no worries there. 422 might be closer in terms of meaning, but realistically, clients won't differentiate between 400 and 422. Browsers will likely see 422 and fall back to 400 (user error). So I'd rather go with consistency and simplicity over possibly being more "correct" in this case.
What do we mean when we say the browser will "fall back" to 400 and what consequences does this have?
There was a problem hiding this comment.
It has no consequences, I meant that clients won't really care if we return 400 or 422. They will see 422 and treat it as 400, meaning user error. So if the client doesn't care, I'd rather have a smaller and more consistent set of return values.
There was a problem hiding this comment.
If we change the goswagger template (actually not sure if this goes as deep as OpenAPI, so might actually be an OpenAPI issue) then this will have consequences for any user who uses that template, I'm not sure we should be promoting incorrect behavior in the name of consistency.
It might be better to make our code comply with the correct definitions of 400/422, might be more work, but feels like the correct course of action.
|
/retest |
|
/lgtm |
|
@paul-maidment: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Presently when uploading a manifest, it's possible to upload a filename with just an extension, for example a filename '.yaml' is considered to be valid.
Clearly this is incorrect and this PR seeks to rectify this issue.
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist