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

Upgrade to ingress-nginx v1.2.1 #109

Closed
wants to merge 80 commits into from
Closed

Upgrade to ingress-nginx v1.2.1 #109

wants to merge 80 commits into from

Conversation

samkulkarni20
Copy link

@samkulkarni20 samkulkarni20 commented Jun 8, 2022

Based off of upstream tag controller-v1.2.1

Disclaimer: This PR is based off of the self exploration/learning & some documentation resources shared by @snasovich. A detailed review is advised.

Steps followed:

  1. Fork rancher/ingress-nginx repo to personal github account
  2. Clone the fork & checkout branch nginx-1.2.x-fix
  3. Add rancher fork & upstream kubernetes/ingress-nginx remotes. Also fetch updates from these remote locations
git remote add upstream https://github.com/kubernetes/ingress-nginx
git remote add rancher https://github.com/rancher/ingress-nginx.git
git fetch --all
  1. Rebase onto upstream tag controller-v1.2.1
git rebase --onto controller-v1.2.1 upstream/main
  1. Resolve conflicts & push
git rebase --continue
git push --force-with-lease

Why rebase & not merge?

Since this branch is based off of upstream repo, rebase is always a better strategy instead of merging. This way we always have all the customizations handy & they become very easily manageable. With continued merging the git history gets clunky over a period of time. This makes managing customizations during merge conflicts very tedious.

rikatz and others added 30 commits May 1, 2022 17:13
Bumps [k8s.io/component-base](https://github.com/kubernetes/component-base) from 0.23.5 to 0.23.6.
- [Release notes](https://github.com/kubernetes/component-base/releases)
- [Commits](kubernetes/component-base@v0.23.5...v0.23.6)

---
updated-dependencies:
- dependency-name: k8s.io/component-base
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.44.0 to 1.46.0.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.44.0...v1.46.0)

---
updated-dependencies:
- dependency-name: google.golang.org/grpc
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…s#8510)

Bumps [github.com/mitchellh/mapstructure](https://github.com/mitchellh/mapstructure) from 1.4.3 to 1.5.0.
- [Release notes](https://github.com/mitchellh/mapstructure/releases)
- [Changelog](https://github.com/mitchellh/mapstructure/blob/main/CHANGELOG.md)
- [Commits](mitchellh/mapstructure@v1.4.3...v1.5.0)

---
updated-dependencies:
- dependency-name: github.com/mitchellh/mapstructure
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [github.com/fsnotify/fsnotify](https://github.com/fsnotify/fsnotify) from 1.5.1 to 1.5.3.
- [Release notes](https://github.com/fsnotify/fsnotify/releases)
- [Changelog](https://github.com/fsnotify/fsnotify/blob/main/CHANGELOG.md)
- [Commits](fsnotify/fsnotify@v1.5.1...v1.5.3)

---
updated-dependencies:
- dependency-name: github.com/fsnotify/fsnotify
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
)

Bumps [github.com/prometheus/common](https://github.com/prometheus/common) from 0.33.0 to 0.34.0.
- [Release notes](https://github.com/prometheus/common/releases)
- [Commits](prometheus/common@v0.33.0...v0.34.0)

---
updated-dependencies:
- dependency-name: github.com/prometheus/common
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…ubernetes#8532)

* Document nginx 1.19.7 deprecations pulled in by ingress-nginx 1.1.3

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* Document ingress-nginx 1.1.3/1.3.0 removals

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

Co-authored-by: Josh Soref <jsoref@users.noreply.github.com>
Bumps [github.com/fsnotify/fsnotify](https://github.com/fsnotify/fsnotify) from 1.5.3 to 1.5.4.
- [Release notes](https://github.com/fsnotify/fsnotify/releases)
- [Changelog](https://github.com/fsnotify/fsnotify/blob/main/CHANGELOG.md)
- [Commits](fsnotify/fsnotify@v1.5.3...v1.5.4)

---
updated-dependencies:
- dependency-name: github.com/fsnotify/fsnotify
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
X-CustomHeader looks more like an example than a header we would want to
accept in production. Added Range as a useful header that enables
operations on resources that can be fetched in chunks.
* disable modsecurity on error page

* fix modsecurity error pages test

* fix variable in nginx template

* disable modsecurity on all internal locations

* fix pipeline checks for gofmt

Signed-off-by: Florian Michel <florianmichel@hotmail.de>
Allow user to set custom preffix for TCP and UDP ports
* nginx opentelemetry modules

* revert sha check
Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.46.0 to 1.46.2.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.46.0...v1.46.2)

---
updated-dependencies:
- dependency-name: google.golang.org/grpc
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
It adds description to the `Ingress Percentile Response Times and Transfer Rates` view, so that user knows that this latency data is independant of dashboard time range.

This PR also adds two new panel about the latency, where one shows latency as a timeseries graph and other shows heatmap of the latency distribution.
…netes#8591)

Bumps [github.com/prometheus/client_golang](https://github.com/prometheus/client_golang) from 1.12.1 to 1.12.2.
- [Release notes](https://github.com/prometheus/client_golang/releases)
- [Changelog](https://github.com/prometheus/client_golang/blob/main/CHANGELOG.md)
- [Commits](prometheus/client_golang@v1.12.1...v1.12.2)

---
updated-dependencies:
- dependency-name: github.com/prometheus/client_golang
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Mac Chaffee <me@macchaffee.com>
rmweir and others added 7 commits June 8, 2022 12:05
Now, apks-tools package is installed first. Apk-tools needs
to finish installing  before busybox can succesfully install
on arm. Prior, apk-tools would start installing first but
frequently busybox would start installing before apk-tools
installation finished in drone. Specifically, the trigger
script for busybox would fail while building arm image in
drone.
Remove test step from our CI because it runs the upstream's e2e Go tests
incorrectly and fails. Since we don't have any tests of our own, and
upstream's CI already runs these tests, this step can be removed.
add drone's build and validate job in github actions workflow

add release workflow which will run only while tagging

added git in workflow steps for build and release

remove G109 check till gosec resolves issues
@snasovich
Copy link
Collaborator

@samkulkarni20 , build is failing, could you please look into this?

@samkulkarni20
Copy link
Author

@snasovich I've resolved the build issue, but I'm not sure if the root cause of the build failure will have have any implications while using the ingress.
CC: @kinarashah @superseb

Copy link

@superseb superseb left a comment

Choose a reason for hiding this comment

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

@samkulkarni20 I expect to see only upstream commits that need to be added because we already made a 1.2.0 and released it. So I don't expect all of our commits to show in the list of commits as they are already in.

Regarding the build failure, if something changed upstream that causes the build to fail, we need to know why its broken and the root cause should be in this PR as we probably need to cherry-pick this commit for the next version (or if possible, see if there is a better solution if the root cause is known)

@samkulkarni20
Copy link
Author

Hi @superseb, You see our commits because I rebased this branch with upstream tag. You can see more details & reason I chose to rebase in the issue description.

Regarding the build failure, I'm looking into getting more information. It did happen due to an upstream update.

@superseb
Copy link

superseb commented Jun 9, 2022

@samkulkarni20 I used the current nginx-1.2.x-fix branch and cleanly cherry-picked everything starting from v1.2.0 from upstream, what conflicts did you run into?

@samkulkarni20
Copy link
Author

I meant that as a general experience, rather than an occurrence in this case.
JFYI, I'm mostly unaware of the process to follow for updating this repo, like I mentioned in the disclaimer in the description. I'm just trying to get this going, while Kinara is away, to the best of my understanding from a confluence page that @snasovich had shared. If you're aware of the complete procedure & details around updating this repo, I would love to learn it from you.

For the build failure, looks like the upstream change was only to fix the tests run using github actions. Upstream PRs: kubernetes#8566
kubernetes#8569

@kinarashah
Copy link
Member

@superseb I could be wrong but thought we wanted rancher changes cherry-picked on top, so we start with upstream tag and then cherry-pick rancher commits so even if the branch already existed, we have them separated right away on a git log.
We don't need to look at anything below the tag v1.2.1 in this case but if we just rebased then we'd have v1.2.0 followed by rancher changes and then upstream stuff for v1.2.1. Did something similar for v1.1.1 as well, #82. Would like to know your thoughts on if it makes sense or we should do this only for the first time we have a branch and then rebase? The net change remains the same, it's just how commits history show up for us.

@samkulkarni20 I looked at upstream changes, since they add install gingko in build/run-in-docker.sh, the change you made looks good to me. Wondering why they didn't run into the same issue but our build process is very different than upstream so I'd say this should suffice.

@superseb
Copy link

superseb commented Jun 9, 2022

I'm fine either way, cherry picking the commits between 1.2.0 and 1.2.1 works as well but you can also get latest and then cherry pick ours on top, I don't see a difference in either. I don't like the rebase approach where everything gets a new commit.

@kinarashah
Copy link
Member

@samkulkarni20 Could we start with controller-v1.2.1 upstream and cherry-pick rancher changes on top? That way everything won't get a new commit. Example PR: nginx-1.2.x-fix...kinarashah:v121, the commit hashes are same as when we first committed them.

@samkulkarni20
Copy link
Author

I'll close this PR & create a new one, with a new branch.

@kinarashah
Copy link
Member

@superseb Looks like even if we cherry-pick changes,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.