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.

Sign up for GitHub

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

Jump to bottom

MGMT-15683: Ensure that manifest filename has valid name part #5635

Merged
merged 1 commit into from Oct 31, 2023
Merged

MGMT-15683: Ensure that manifest filename has valid name part #5635

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions internal/manifests/manifests.go
View file
Open in desktop
Expand Up @@ -451,6 +451,15 @@ func (m *Manifests) fetchManifestContent(ctx context.Context, clusterID strfmt.U

func (m *Manifests) validateManifestFileNames(ctx context.Context, clusterID strfmt.UUID, fileNames []string) error {
for _, fileName := range fileNames {
fileNameWithoutExtension := strings.TrimSuffix(fileName, filepath.Ext(fileName))
if len(strings.TrimSpace(fileNameWithoutExtension)) == 0 {
return m.prepareAndLogError(
ctx,
http.StatusUnprocessableEntity,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@rccrdpccl

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@avishayt

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.

errors.Errorf("Cluster manifest %s for cluster %s has an invalid filename.",
fileName,
clusterID))
}
if strings.Contains(fileName, " ") {
return m.prepareAndLogError(
ctx,
Expand Down
17 changes: 17 additions & 0 deletions internal/manifests/manifests_test.go
View file
Open in desktop
Expand Up @@ -258,6 +258,23 @@ var _ = Describe("ClusterManifestTests", func() {
return fmt.Sprintf("{\"data\":\"%s\"}", largeElementBuilder.String())
}

It("Does not accept a filename that does not contain a name before the extension", func() {
clusterID := registerCluster().ID
content := "{}"
fileName := ".yaml"
response := manifestsAPI.V2CreateClusterManifest(ctx, operations.V2CreateClusterManifestParams{
ClusterID: *clusterID,
CreateManifestParams: &models.CreateManifestParams{
Content: &content,
FileName: &fileName,
},
})
err := response.(*common.ApiErrorResponse)
expectedErrorMessage := fmt.Sprintf("Cluster manifest %s for cluster %s has an invalid filename.", fileName, clusterID)
Expect(err.StatusCode()).To(Equal(int32(http.StatusUnprocessableEntity)))
Expect(err.Error()).To(Equal(expectedErrorMessage))
})

It("Does not accept a filename that contains spaces", func() {
clusterID := registerCluster().ID
content := "{}"
Expand Down