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 error pages for VS/VSR #847

Merged
merged 1 commit into from
Feb 11, 2020
Merged

Add support for error pages for VS/VSR #847

merged 1 commit into from
Feb 11, 2020

Conversation

Rulox
Copy link
Contributor

@Rulox Rulox commented Feb 10, 2020

Proposed changes

This PR introduces the error_page directive to use with VS/VSR for circuit breaking.

Changes

This PR introduces changes in how the NGINX configuration is generated using the templates. Prior to this PR, the internal redirections were done by using the error_page directive. However, this would prevent the usage of error_pages by the user, this has been changed to use the rewrite directive:

    location {{ $l.Path }} {
        rewrite ^ {{ $l.Destination }} last;
    }

instead of

    location {{ $l.Path }} {
        error_page 418 = {{ $l.Destination }};
        return 418;
    }

This change affects to the locations used for internal redirect. Before, they were named locations. But because rewrite directive won't work with named locations, regular locations need to be used. However, to prevent these locations to be accessible, the internal directive has been added.

Additionally, in order to avoid conflicts with locations defined by the user, a prefix internal_location_ will be used for these locations. This will make the locations look like this:

location /internal_location_my_location {
   internal;
   // location config
}

Instead of:

location @my_location {
   // location config
}

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 master
  • I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

@Rulox Rulox added enhancement Pull requests for new features/feature enhancements change Pull requests that introduce a change labels Feb 10, 2020
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

@Rulox 👍

before merging, please fix a small problem in the docs

Copy link
Member

@lucacome lucacome left a comment

Choose a reason for hiding this comment

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

Looks good @Rulox 👍

@Rulox Rulox merged commit c2bccae into master Feb 11, 2020
@Rulox Rulox deleted the error-pages branch February 11, 2020 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change Pull requests that introduce a change enhancement Pull requests for new features/feature enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants