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 support for headers in action return #5204

Merged
merged 7 commits into from
May 2, 2024

Conversation

andrew-s
Copy link
Contributor

@andrew-s andrew-s commented Mar 6, 2024

Proposed changes

This PR adds support for headers in the action return object. This closes/fixes #2420

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

Notes

I've added in unit tests, but it looks like there are other tests within the repo - I have also validated by compiling and running on a local cluster.

@andrew-s andrew-s requested review from a team as code owners March 6, 2024 07:59
Copy link

netlify bot commented Mar 6, 2024

👷 Deploy request for nginx-kubernetes-ingress pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 8d91886

@github-actions github-actions bot added documentation Pull requests/issues for documentation enhancement Pull requests for new features/feature enhancements labels Mar 6, 2024
Copy link
Contributor

@ADubhlaoich ADubhlaoich left a comment

Choose a reason for hiding this comment

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

LGTM docs-wise but the majority of the PR is code, so I will not approve.

@andrew-s
Copy link
Contributor Author

andrew-s commented Mar 6, 2024

I think to be self critical of this PR;

What is done;

  • Code for the controller and chart are implemented (I tested this, loaded the built alpine image into a kind cluster, with the chart and was able to test different variances on the action.return type, including without the headers)
  • Docs match for action return

What is not done;

  • Tests are mostly missing, the unit tests can't check if the headers have actually been set - I've noticed this for the errorpage action return headers too. Perhaps the other tests that run from the data folder would be better suited?

@andrew-s andrew-s force-pushed the feature/action-return-headers branch from 3769030 to 9175d98 Compare March 10, 2024 04:46
@andrew-s
Copy link
Contributor Author

I've updated the PR to passing unit tests

@andrew-s andrew-s force-pushed the feature/action-return-headers branch from 9175d98 to b5076da Compare March 15, 2024 07:56
@andrew-s andrew-s force-pushed the feature/action-return-headers branch from b5076da to 977ee4b Compare March 28, 2024 22:02
@andrew-s
Copy link
Contributor Author

@ADubhlaoich @haywoodsh

I think this needs someone from nginx to weigh in. With the PR as it is now, the generated/expected spec isn't correct - it would generate errorPages as;

                              headers:
                                allOf:
                                - items:
                                    description: Header defines an HTTP Header.
                                    properties:
                                      name:
                                        type: string
                                      value:
                                        type: string
                                    type: object
                                - items:
                                    description: Header defines an HTTP Header.
                                    properties:
                                      name:
                                        type: string
                                      value:
                                        type: string
                                    type: object
                                type: array

This is because ErrorPageReturn uses ActionReturn which now has headers, so this applies headers twice. There's two solutions;

  • ErrorPageReturn exclusively uses ActionReturn which now has headers, this would need all of the tests updating and anywhere that uses headers on ErrorPageReturn directly to then implement headers onto ActionReturn within the struct
  • ErrorPageReturn no longer uses ActionReturn and implements Code, Type and Body directly - all of the tests and other logic which uses ActionReturn on ErrorPageReturn to then use these fields.

After some initial testing, these struct changes to produce the expected results.

I think these changes could break any existing PRs but vice versa, if we decide one of these routes then it would make sense to try and merge quickly to prevent conflicts.

@haywoodsh
Copy link
Contributor

haywoodsh commented Apr 25, 2024

Just trying to make it clearer, in the current API,ActionReturn has 3 fields, Code, Type and Body, and ErrorPage has 4 fields:Code, Type, Body and Headers, as it uses ActionReturn and Headers. After this PR, both Action.Return and ErrorPage.Return will have the same items.

I like the first solution more, because I don't think Action.Return and ErrorPage.Return will divert again in the future, like something should only exist for Action but not ErrorPage, or the other way around.

Also, I saw ErrorPageRidirect also has only one item ActionRedirect.

type ErrorPageRedirect struct {
	ActionRedirect `json:",inline"`
}

Therefore, I feel we should be consistent and remove Headers from ErrorPageReturn in this line, so that it becomes:

type ErrorPageReturn struct {
	ActionReturn `json:",inline"`
}

As we are not changing the API for ErrorPage.Return, we might only need some minor refactorings to the unit tests from what I can when I tested it locally.

@andrew-s andrew-s force-pushed the feature/action-return-headers branch from 977ee4b to 4598c26 Compare April 27, 2024 02:54
@github-actions github-actions bot added the go Pull requests that update Go code label Apr 27, 2024
@andrew-s
Copy link
Contributor Author

That makes the most sense, I've made the change, updated tests and generated the new config.

@github-actions github-actions bot added python Pull requests that update Python code tests Pull requests that update tests labels Apr 30, 2024
@andrew-s
Copy link
Contributor Author

@haywoodsh @vepatel Added the suite tests, this should cover the three scenarios for both virtualserver and virtualserverroute;

  • Invalid OpenAPI test
  • Valid header and contents
  • Update of header and contents

@andrew-s andrew-s force-pushed the feature/action-return-headers branch from c7df110 to 3ba4f27 Compare April 30, 2024 23:12
@haywoodsh haywoodsh requested a review from a team May 1, 2024 12:05
Copy link
Contributor

@jjngx jjngx left a comment

Choose a reason for hiding this comment

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

🚀

@haywoodsh haywoodsh merged commit 04ca913 into nginxinc:main May 2, 2024
109 of 126 checks passed
@haywoodsh
Copy link
Contributor

Thank you for your contribution @andrew-s!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Pull requests/issues for documentation enhancement Pull requests for new features/feature enhancements go Pull requests that update Go code python Pull requests that update Python code tests Pull requests that update tests
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

VirtualServerRoute: action.return headers
6 participants