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

Remove fastcgi feature #9864

Merged
merged 1 commit into from
Jun 11, 2023
Merged

Remove fastcgi feature #9864

merged 1 commit into from
Jun 11, 2023

Conversation

rikatz
Copy link
Contributor

@rikatz rikatz commented Apr 17, 2023

What this PR does / why we need it:

fastcgi is a feature that can be served on another ways, like a backend with nginx + fastcgi. We should remove a bunch of features from Ingress NGINX and keep it as simpler proxy.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • CVE Report (Scanner found CVE and adding report)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation only

/kind cleanup

Does my pull request need a release note?

Any user-visible or operator-visible change qualifies for a release note. This could be a:

  • CLI change
  • API change
  • UI change
  • configuration schema change
  • behavioral change
  • change in non-functional attributes such as efficiency or availability, availability of a new platform
  • a warning about a deprecation
  • fix of a previous Known Issue
  • fix of a vulnerability (CVE)

No release notes are required for changes to the following:

  • Tests
  • Build infrastructure
  • Fixes for unreleased bugs

For more tips on writing good release notes, check out the Release Notes Handbook

Remove fast-cgi as a backend feature

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 17, 2023
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. area/docs labels Apr 17, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rikatz

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 17, 2023
@rikatz
Copy link
Contributor Author

rikatz commented Apr 17, 2023

/hold

for discussion

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2023
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2023
@tao12345666333
Copy link
Member

need rebase.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 11, 2023
@rikatz rikatz added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 11, 2023
@rikatz
Copy link
Contributor Author

rikatz commented Jun 11, 2023

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 11, 2023
@k8s-ci-robot k8s-ci-robot merged commit 90ed0cc into kubernetes:main Jun 11, 2023
@cdemers
Copy link
Contributor

cdemers commented Jun 12, 2023

oh, I missed the discussion about this, are you sure about this decision? I dont have exact numbers, and maybe I'm wrong, but I feel that this feature is in use and removing it will take people by surprise. I also only speak for myself but in our case, as we run many hundreds of PHP pods with FCGI, and running that many sidecars will rise our operations costs significantly.
Also, I do understand the need for simplification, I'm a big proponent of keeping things simple and I'm always pushing for that, but removing FCGI doesn't seem to remove that much code. And why, for example, chose to remove FCGI over AJP?
I apologize for not having cough on this sooner.

@tao12345666333
Copy link
Member

This will indeed increase the complexity of operations for scenarios that run a large number of PHP applications.

In fact, you don't need to use sidecar to run the FastCGI proxy.

The overall architecture will undergo the following changes.

From:

2023-06-13 10-29-55屏幕截图

To:

2023-06-13 10-29-21屏幕截图

It is necessary to add a group of proxies to perform protocol conversion.

@rikatz
Copy link
Contributor Author

rikatz commented Jun 13, 2023

@cdemers thanks for the feedback.

We have decided to rollback this removal, so I will work on it during the week and before the next release :)

Thanks for the headsup

@adoy
Copy link
Contributor

adoy commented Jun 13, 2023 •
edited
Loading

Thanks a lot. If I can be of any help on this feel free to let me know.

tao12345666333 added a commit that referenced this pull request Jun 13, 2023
@tao12345666333
Revert "Remove fastcgi feature (#9864)"
e7dc5ca 
This reverts commit 90ed0cc.
@tao12345666333 tao12345666333 mentioned this pull request Jun 13, 2023
Merged
@cdemers
Copy link
Contributor

cdemers commented Jun 13, 2023

Thank you so much @rikatz ! I'm very sorry to have you work on the weekend, and have to revert all the work you've done already, I wish I had noticed this change sooner.

@rikatz
Copy link
Contributor Author

rikatz commented Jun 13, 2023

nah no worries, part of the "job" :D

k8s-ci-robot pushed a commit that referenced this pull request Jun 13, 2023
@tao12345666333
Revert "Remove fastcgi feature" (#10081)
cccba35 
* Revert "Remove fastcgi feature (#9864)"

This reverts commit 90ed0cc.

* revert fastcgi* annotations warning

Signed-off-by: Jintao Zhang <zhangjintao9020@gmail.com>

---------

Signed-off-by: Jintao Zhang <zhangjintao9020@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ElvinEfendi ElvinEfendi Awaiting requested review from ElvinEfendi

@tao12345666333 tao12345666333 Awaiting requested review from tao12345666333

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/docs cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rikatz @k8s-ci-robot @tao12345666333 @cdemers @adoy