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

NGINX: Remove inline Lua from template. #11806

Merged
merged 8 commits into from
Sep 8, 2024

Conversation

rikatz
Copy link
Contributor

@rikatz rikatz commented Aug 14, 2024

What this PR does / why we need it:

We need to simplify our lifes and the nginx.conf file. This PR is the beginning of the work to remove any lua inline script from nginx template, calling the lua script directly.

The end goal is to remove any by_lua_block directive, even if this means we add more lua scripts temporarily

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

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have added unit and/or e2e tests to cover my changes.
  • All new and existing tests passed.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 14, 2024
@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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. area/lua Issues or PRs related to lua code needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 14, 2024
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 14, 2024
Copy link

netlify bot commented Aug 14, 2024

Deploy Preview for kubernetes-ingress-nginx canceled.

Name Link
🔨 Latest commit aa5c3ec
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/66ddc92ab3da2600082f6aba

@rikatz rikatz force-pushed the move-lua-to-files branch from 938bcd1 to fbf87e4 Compare August 14, 2024 22:37
@Gacko
Copy link
Member

Gacko commented Aug 15, 2024

Do you want to merge this to main or the NGINX crossplane branch?

@rikatz
Copy link
Contributor Author

rikatz commented Aug 15, 2024

@Gacko to main :)

@Gacko
Copy link
Member

Gacko commented Aug 15, 2024

Then please wait until after the release. 😅

@rikatz rikatz force-pushed the move-lua-to-files branch 2 times, most recently from 97fd4bb to dd0412a Compare August 15, 2024 13:35
@rikatz
Copy link
Contributor Author

rikatz commented Aug 15, 2024

Then please wait until after the release. 😅

I'm not on a rush :P

@Gacko Gacko changed the title Remove inline lua script from template NGINX: Remove inline Lua from template. Aug 16, 2024
@Gacko Gacko force-pushed the move-lua-to-files branch from dd0412a to 4032717 Compare August 16, 2024 07:34
@Gacko
Copy link
Member

Gacko commented Aug 16, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 16, 2024
@Gacko
Copy link
Member

Gacko commented Aug 16, 2024

/unhold

@Gacko
Copy link
Member

Gacko commented Aug 16, 2024

@rikatz I rebased your branch. Unhold whenever ready. :)

@rikatz
Copy link
Contributor Author

rikatz commented Aug 16, 2024

/hold

I will still finish the other migrations! :D Will let you know once it is ready

@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 Aug 16, 2024
@rikatz rikatz force-pushed the move-lua-to-files branch from 4032717 to 0201a89 Compare August 17, 2024 00:41
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 17, 2024
@rikatz rikatz force-pushed the move-lua-to-files branch 2 times, most recently from 459c320 to c63788b Compare August 25, 2024 22:03
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 25, 2024
@rikatz rikatz force-pushed the move-lua-to-files branch 4 times, most recently from 43bc72f to e41b743 Compare August 25, 2024 23:24
@rikatz rikatz force-pushed the move-lua-to-files branch from e41b743 to e327abd Compare August 25, 2024 23:55
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
rootfs/etc/nginx/lua/nginx/ngx_srv_redirect.lua Outdated Show resolved Hide resolved
@rikatz rikatz force-pushed the move-lua-to-files branch from 912f17b to 3b12461 Compare August 31, 2024 17:27
@rikatz rikatz force-pushed the move-lua-to-files branch from 8ca819d to d049b2e Compare August 31, 2024 20:23
Copy link
Member

@tao12345666333 tao12345666333 left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 1, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Gacko, rikatz, tao12345666333

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:
  • OWNERS [Gacko,rikatz,tao12345666333]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Comment on lines +80 to +81
if: |
(needs.changes.outputs.lua == 'true') || ${{ github.event.workflow_dispatch.run_e2e == 'true' }}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if: |
(needs.changes.outputs.lua == 'true') || ${{ github.event.workflow_dispatch.run_e2e == 'true' }}
if: fromJSON(needs.changes.outputs.lua) || fromJSON(github.event.workflow_dispatch.run_e2e)

Have you tried this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

never tried. but then wouldn't it be better to change all of the ifs for this approach on a follow up PR?

hack/verify-lualint.sh Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@@ -0,0 +1,2 @@
local balancer = require("balancer")
balancer.balance()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
balancer.balance()
balancer.balance()

Copy link
Member

Choose a reason for hiding this comment

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

Can we have the newline at EOF added in snippets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry I am not following. What would be the goal of adding a new line here?

rootfs/etc/nginx/lua/nginx/ngx_srv_redirect.lua Outdated Show resolved Hide resolved
rootfs/etc/nginx/lua/nginx/ngx_srv_redirect.lua Outdated Show resolved Hide resolved
rootfs/etc/nginx/lua/nginx/ngx_srv_redirect.lua Outdated Show resolved Hide resolved
rootfs/etc/nginx/lua/util.lua Outdated Show resolved Hide resolved
rootfs/etc/nginx/template/nginx.tmpl Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 8, 2024
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Sep 8, 2024

@rikatz: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-ingress-nginx-lualint b65dae6b80609c53e1255ffafec0d86032a601a4 link true /test pull-ingress-nginx-lualint

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

@rikatz
Copy link
Contributor Author

rikatz commented Sep 8, 2024

/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 Sep 8, 2024
@rikatz rikatz merged commit 6510535 into kubernetes:main Sep 8, 2024
36 of 37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/lua Issues or PRs related to lua code cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants