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

Apply -enable-snippets cli arg to Ingresses #2124

Merged
merged 5 commits into from
Oct 28, 2021

Conversation

pleshakov
Copy link
Contributor

@pleshakov pleshakov commented Oct 25, 2021

Proposed changes

This PR extends the existing -enable-snippets cli argument to apply to Ingress resources. If snippets are not enabled, the Ingress Controller will reject any Ingress resources with snippets annotations.

Previously, the argument only applied to VirtualServer, VirtualServerRoute and TransportServer resources.

Note: this is a breaking change. Because the default value of -enable-snippets is false, If you're using snippets in Ingress resources, make sure to explicitly set the -enable-snippets to true before upgrading the Ingress Controller, so that the new version of the Ingress Controller doesn't reject Ingresses with snippets annotations.

TO-DO:

  • Code
  • Docs
  • Python tests

@pleshakov pleshakov added enhancement Pull requests for new features/feature enhancements change Pull requests that introduce a change labels Oct 25, 2021
@github-actions github-actions bot added the documentation Pull requests/issues for documentation label Oct 25, 2021
@soneillf5 soneillf5 force-pushed the feature/ingress-snippets branch 2 times, most recently from de405e4 to a288b9f Compare October 26, 2021 14:00
@nginx-bot nginx-bot force-pushed the feature/ingress-snippets branch from 17e7349 to c2ad243 Compare October 26, 2021 18:00
indirect=["ingress_controller"])
def test_snippet_annotation_ignored(self, kube_apis, ingress_controller_prerequisites, ingress_controller, test_namespace):
file_name = f"{TEST_DATA}/annotations/standard/annotations-ingress-snippets.yaml"
create_ingress_from_yaml(kube_apis.networking_v1, test_namespace, file_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we not need to save the name of this ingress and then delete it after the test as we did in the previous test?

Copy link
Contributor

Choose a reason for hiding this comment

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

I found that the ingress is not created as it is rejected as invalid ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense - I was mixing up ingress and ingress controller, thanks

@pleshakov pleshakov requested a review from vepatel October 27, 2021 15:55
@nginx-bot nginx-bot force-pushed the feature/ingress-snippets branch from ad5c0fe to c5e1549 Compare October 27, 2021 17:25
@pleshakov pleshakov changed the base branch from master to release-2.0 October 28, 2021 00:03
@pleshakov pleshakov changed the base branch from release-2.0 to master October 28, 2021 00:03
@lucacome lucacome force-pushed the feature/ingress-snippets branch from c5e1549 to 0b00866 Compare October 28, 2021 00:34
@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Oct 28, 2021
@lucacome lucacome changed the base branch from master to release-2.0 October 28, 2021 00:35
@lucacome
Copy link
Member

changed the base branch to release-2.0

@ciarams87 ciarams87 merged commit f4709eb into release-2.0 Oct 28, 2021
@ciarams87 ciarams87 deleted the feature/ingress-snippets branch October 28, 2021 10:46
pleshakov added a commit that referenced this pull request Oct 28, 2021
* Apply -enable-snippets cli arg to Ingresses

* Update docs

* add snippet flag python tests

* removing snippets check as we rely on validation

* removing used param

Co-authored-by: Sean O'Neill <s.oneill@f5.com>
ciarams87 pushed a commit that referenced this pull request Oct 28, 2021
* Apply -enable-snippets cli arg to Ingresses

* Update docs

* add snippet flag python tests

* removing snippets check as we rely on validation

* removing used param

Co-authored-by: Sean O'Neill <s.oneill@f5.com>
@lucacome lucacome removed enhancement Pull requests for new features/feature enhancements documentation Pull requests/issues for documentation change Pull requests that introduce a change dependencies Pull requests that update a dependency file labels Oct 28, 2021
@adamdecaf
Copy link

adamdecaf commented Nov 1, 2021

FYI pushing breaking changes within a bugfix release is a bad practice. We had an outage in development environments due to this.

From https://semver.org/

  1. PATCH version when you make backwards compatible bug fixes.

@adamdecaf
Copy link

Also, can you please update the docs to reflect this CLI flags larger impact in 2.0.3?

https://docs.nginx.com/nginx-ingress-controller/configuration/global-configuration/command-line-arguments#-enable-snippets

lucacome added a commit that referenced this pull request Nov 4, 2021
* fix: Updated theme hugo

* Apply -enable-snippets cli arg to Ingresses (#2124)

* Apply -enable-snippets cli arg to Ingresses

* Update docs

* add snippet flag python tests

* removing snippets check as we rely on validation

* removing used param

Co-authored-by: Sean O'Neill <s.oneill@f5.com>

* Install libcurl on OpenTracing for NGINX Plus (#2132)

* DRAFT: Release v2.0.3

Co-authored-by: Ashutosh Pradhan <a.pradhan@f5.com>
Co-authored-by: Michael Pleshakov <pleshakov@users.noreply.github.com>
Co-authored-by: Sean O'Neill <s.oneill@f5.com>
Co-authored-by: Ciara Stacke <c.stacke@f5.com>
@lucacome lucacome added the enhancement Pull requests for new features/feature enhancements label Dec 21, 2021
@pammecrandall pammecrandall added the change Pull requests that introduce a change label Jul 13, 2022
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.

8 participants