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

Try to improve the installation guide #7757

Merged
merged 1 commit into from
Nov 5, 2021

Conversation

jpetazzo
Copy link
Contributor

@jpetazzo jpetazzo commented Oct 5, 2021

Important: this is a work in progress that should be checked for correctness by someone familiar with the code base. When installing the ingress controller, I found that the documentation was missing, so I opened issue #7701 and this is an attempt to address that, by adding all the information that I wish I had found in the doc in the first place. But I don't know the code base nor the philosophy of the ingress controller, so it is possible that I got a few things wrong here, and since this is the install guide, this is probably the first document that users will read, so I think it should be correct. Thank you!

Thanks to @longwuyuan's feedback, this should now hopefully be in a good state :)

What this PR does / why we need it:

Improve installation guide:

  • move generic instructions to the beginning of the file
  • clarify how to install the ingress controller in the generic case
  • add an example of ingress resource creation
  • simplify a few commands to make them shorter and simpler
  • add short paragraphs about PROXY protocol and traffic policy

This tries to address the concerns I expressed in #7701.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation only

Which issue/s this PR fixes

Fixes #7701.

How Has This Been Tested?

I tested the install instructions on various clusters (minikube, Docker Desktop, Scaleway, and a few others)

Checklist:

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

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 5, 2021
@k8s-ci-robot
Copy link
Contributor

Welcome @jpetazzo!

It looks like this is your first PR to kubernetes/ingress-nginx 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/ingress-nginx has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

Hi @jpetazzo. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 5, 2021
@longwuyuan
Copy link
Contributor

Just want to state that personally its an honor and a privilege just to be in conversation with you on a issue/PR. Big fan.

On this PR, I looked at https://github.com/jpetazzo/ingress-nginx/blob/rework-installation-guide/docs/deploy/index.md to get a feel of what the end-result would look like and (no surprises, its after all from you), its the kind of improvement that one desires. My first feedback is while it will help to dive into details, I want to help as much as possible, with the context and the uniqueness associated with the project (and hence the implying documentation). Am going to try listing these aspects below ;

(1) Because of the deprecation of older ingress api(s) in K8s v1.22, there was a massive + breaking change in code. In short, we now maintain and release 2 editions of the controller. One for pre-K8s-v1.22 specs and one for v1.22 specs and upwards. You can see some reference to that here https://kubernetes.github.io/ingress-nginx/#faq-migration-to-apiversion-networkingk8siov1 . So this effort at improvement of the install guide must make this aspect front & centre.

After you incorporate this aspect, I can look at your fork again and comment further.

Once again thanks tons for everything from you over the years and absolute joy to be just sending you a message directly :-), (never thought there would be such a day).

@strongjz
Copy link
Member

strongjz commented Oct 8, 2021

/kind documentation

thanks for cleaning this up.

@k8s-ci-robot k8s-ci-robot added kind/documentation Categorizes issue or PR as related to documentation. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 8, 2021
* move generic instructions to the beginning of the file
* add an example of ingress resource creation
* simplify a few commands to make them shorter and simpler
* add short paragraphs about PROXY protocol and traffic policy

This tries to address the concerns I expressed in kubernetes#7701.
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 3, 2021
@jpetazzo
Copy link
Contributor Author

jpetazzo commented Nov 3, 2021

@longwuyuan Hi! I've added a note at the end of the document explaining the compatibility challenges associated with Ingress v1beta1 vs v1, and a link to that note in the beginning of the install instructions. That way, if someone tries to install on Kubernetes 1.18, they should get the compatibility warning and the note will tell them what to do.

Let me know if that works!

@longwuyuan
Copy link
Contributor

I love this kind of improvement so I think we should merge this for now, and continue with follow up PRs as needed.

However, I am of the opinion that the section "Running on Kubernetes versions older than 1.19" is better off, visible at the top of the page, instead of at the bottom.

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 3, 2021
@longwuyuan
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 3, 2021
@longwuyuan
Copy link
Contributor

Also, while you are at it, would you want to take a shot at adding a section for a "kind" cluster. Otherwise, hopefully, we can do it in a new PR. I think it will help users a lot to get a step-by-step proecedure to run kind + metallb + ingress-nginx .

@jpetazzo
Copy link
Contributor Author

jpetazzo commented Nov 4, 2021

I have some ideas for KinD but I'll do it in a separate PR.
However I don't have experience with KinD + MetalLB. Were you thinking about something like this, which seems Linux-specific?

@longwuyuan
Copy link
Contributor

Hi @jpetazzo,
Yes, basically give users some idea on the docker-network subnet being useful on linux at least.

Different PR is ok. Just an idea I floated.

Also, I already put a ok-to-test on this PR and also a lgtm, so we wait for approval. Maybe the title of the PR needs edit to remove the "WIP", if you feel you are done.

Also, look forward to your comments on putting that paragraph about older versions on top, instead of at the bottom. Either way is fine I guess but I thought putting it on top gives critical info to new users.

@jpetazzo jpetazzo changed the title [WIP] Try to improve the installation guide Try to improve the installation guide Nov 4, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 4, 2021
@jpetazzo
Copy link
Contributor Author

jpetazzo commented Nov 4, 2021

Thanks for the reminder - I had forgotten to remove the WIP!

About "let's put the version warning on top" - I'm on the fence on this; I'm hesitating. Here is my thought process:

  • the NGINX Ingress Controller version 1.0 is the stable version and it supports Kubernetes 1.19+
  • Kubernetes 1.19 is no longer supported upstream (the last 3 stable releases are 1.20, 1.21, 1.22 as I write these lines)
  • therefore, NGINX Ingress Controller version 1.0 supports all stable releases of Kubernetes
  • therefore, there shouldn't be any reason to use NGINX Ingress Controller 0.47 for new installs (it's only going to cause extra work when upgrading k8s, without providing any gain)
  • therefore, in a guide that explains how to install the NGINX Ingress Controller, we should focus on version 1.0

I agree that there is a lot of confusion and issues out there with folks trying to install version 0.47 on Kubernetes 1.22; but in my humble opinion, the problem comes from old YAML / old installation guides that are still using version 0.47. In a modern guide, we should focus on version 1.0. Of course we should mention that compatibility issue but I think it should be a small footnote. Within 6-9 months, Kubernetes 1.20 and 1.21 will be deprecated and version 0.47 will only be needed on legacy clusters.

That's my thought process; but if you think the note must be put forward more visibly, feel free to amend the PR of course.

Thank you for your consideration!

@longwuyuan
Copy link
Contributor

That is exactly my thought process.
I was only thinking of folks who have migration problems.
But after seeing the whole thing so rt of elucidated in writing from you, I now feel some kind of decision was required and now I concur with your decision.

As always, a big fan of all your work over the years. Honored to be talking to you :-) .
This settles it I guess. We wait for comments and reviews and final approval now.

@strongjz
Copy link
Member

strongjz commented Nov 5, 2021

/triage accepted
/priority important-soon
/lgtm

Looks great, thanks for that.

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority labels Nov 5, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpetazzo, longwuyuan, strongjz

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2021
@k8s-ci-robot k8s-ci-robot merged commit ce165f9 into kubernetes:main Nov 5, 2021
@longwuyuan
Copy link
Contributor

longwuyuan commented Nov 7, 2021

Hi @jpetazzo ,
I had a follow up topic to talk to you about.
The URLs in the docs are generated during the (less common) release process we follow. Please look at https://github.com/kubernetes/ingress-nginx/blob/legacy/RELEASE.md . This script https://github.com/kubernetes/ingress-nginx/blob/main/hack/generate-deploy-scripts.sh is related .

I wanted to double-check that our release process (and likely that script in particular) can still find the text/url in the installation guide and edit accordingly. The version is embedded in the URL to manifests. Just want to be sure we covered that.

I ought to check this myself but owing to some unavoidable reasons, I am not up to it just yet, hence asked. Most likely I am being paranoid because we also have to manually change version in docs in a follow up step of the release process. Apologies if this message is not helpful.

Thanks,
; Long

@jpetazzo jpetazzo deleted the rework-installation-guide branch November 8, 2021 06:41
@jpetazzo
Copy link
Contributor Author

jpetazzo commented Nov 8, 2021

Hi,

It looks like generate-deploy-scripts.sh generates a bunch of YAML manifests; it should keep working the same way since I didn't change anything about these YAML manifests in the documentation.

However, you seem to indicate that there might be another script that perhaps updates the URLs automatically ("our release process (and likely that script in particular) can still find the text/url in the installation guide and edit accordingly") but I couldn't find that script. I did a quick grep -r deploy in the hack directory but it didn't turn up anything else than the YAML generation.

rchshld pushed a commit to joomcode/ingress-nginx that referenced this pull request May 19, 2023
* move generic instructions to the beginning of the file
* add an example of ingress resource creation
* simplify a few commands to make them shorter and simpler
* add short paragraphs about PROXY protocol and traffic policy

This tries to address the concerns I expressed in kubernetes#7701.
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/docs cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting started documentation doesn't say how to get started in the general case
4 participants