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

doc: Add Egress Gateway Getting Started Guide #15661

Merged
merged 1 commit into from
Apr 27, 2021
Merged

Conversation

blzhao-0
Copy link
Contributor

@blzhao-0 blzhao-0 commented Apr 12, 2021

This is a documentation of the detailed instructions on configuring
cilium components to start using the egress nat gateway feature.

RFE: #13575

Signed-off-by: Yongkun Gui ygui@google.com
Signed-off-by: Bolun Zhao blzhao@google.com

@blzhao-0 blzhao-0 requested a review from a team as a code owner April 12, 2021 20:33
@blzhao-0 blzhao-0 requested a review from qmonnet April 12, 2021 20:33
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 12, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Apr 12, 2021
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Looks good overall, I find the steps very clear and well-explained. I have a few comments on the formatting below.

Please also address the issues reporting from the CI:

  • A few typo (also highlighted in my review)
  • You need to reference your documents from an index. From the list of the Getting Started Guides for example, likely in the Advanced Networking section.

Please also make sure that you use consistent levels of indents for all the directives in your document (block-content should ideally be left-aligned on the first letter of the directive name, as is the case for your first parsed-literal block for example).

Documentation/gettingstarted/egress-gateway.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/egress-gateway.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/egress-gateway.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/egress-gateway.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/egress-gateway.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/egress-gateway.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/egress-gateway.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/egress-gateway.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/egress-gateway.rst Outdated Show resolved Hide resolved
@qmonnet qmonnet requested review from a team and jrajahalme and removed request for a team April 12, 2021 21:48
@qmonnet qmonnet requested review from a team and joestringer April 12, 2021 21:49
@qmonnet qmonnet added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/misc This PR makes changes that have no direct user impact. labels Apr 12, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 12, 2021
@qmonnet qmonnet removed the request for review from a team April 12, 2021 21:51
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 14, 2021
@maintainer-s-little-helper

This comment has been minimized.

@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 14, 2021
@cilium cilium deleted a comment from maintainer-s-little-helper bot Apr 14, 2021
@cilium cilium deleted a comment from maintainer-s-little-helper bot Apr 14, 2021
@cilium cilium deleted a comment from maintainer-s-little-helper bot Apr 14, 2021
@cilium cilium deleted a comment from maintainer-s-little-helper bot Apr 14, 2021
@cilium cilium deleted a comment from maintainer-s-little-helper bot Apr 14, 2021
@cilium cilium deleted a comment from maintainer-s-little-helper bot Apr 14, 2021
@cilium cilium deleted a comment from maintainer-s-little-helper bot Apr 14, 2021
@cilium cilium deleted a comment from maintainer-s-little-helper bot Apr 14, 2021
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Thanks for the update! There are still a few items remaining, please see the un-resolved items above. In addition, you still need to link the document somewhere, and to add busybox to the list of correct words as suggested by the failing GitHub action:

 Please fix the following documentation warnings:
/github/workspace/Documentation/gettingstarted/egress-gateway.rst: WARNING: document isn't included in any toctree

Please fix the following spelling mistakes:
* Documentation/gettingstarted/egress-gateway.rst:127: (busybox) 

If the words are not misspelled, run:
Documentation/update-spelling_wordlist.sh busybox 

Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

I fixed the couple build issues mentioned by Quentin to be able to render locally and review. I haven't tested yet, but I think it needs a couple changes if we want folks to be able to follow the guide (without needing to deploy our development VM).

Documentation/gettingstarted/egress-gateway.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/egress-gateway.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/egress-gateway.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/egress-gateway.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/egress-gateway.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/egress-gateway.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/egress-gateway.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/egress-gateway.rst Outdated Show resolved Hide resolved
- name: EGRESS_IPS
value: "192.168.33.100/24 192.168.33.101/24"
args:
- "for i in $EGRESS_IPS; do ip address add $i dev enp0s8; done; sleep 10000000"
Copy link
Member

Choose a reason for hiding this comment

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

Is this going to be enough for users running in e.g. GKE, EKS, AKS?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the guide for onprem environment. In cloud, the way to assign IP addresses will be very different in different cloud providers. We haven't covered those environments yet. Maybe we could do it as next step.

Copy link
Member

@pchaigno pchaigno Apr 19, 2021

Choose a reason for hiding this comment

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

Maybe it would be enough to use the existing node IP for managed Kubernetes? It won't demonstrate the SNAT part of the egress gateway, but at least users would be able to observe the redirect. I guess it's better than nothing.

If that's not an option, we should at least clarify at the top that the guide doesn't currently support managed Kubernetes. I would be very careful how we word this though; we don't want to give the impression the feature doesn't support managed Kubernetes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Make sense. I'll try in on GKE first as an example. But I won't be able to cover other platforms.

Copy link
Member

Choose a reason for hiding this comment

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

But I won't be able to cover other platforms.

I think that's fine as long as we don't make the guide GKE-specific in some way. If there are small issues when running it on other managed K8s, we can always fix them up later.

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Looks good from my side. Thanks for the updates.

@pchaigno pchaigno added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Apr 20, 2021
Copy link
Member

@jrajahalme jrajahalme left a comment

Choose a reason for hiding this comment

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

One question for potential simplification only.

examples/kubernetes-egress-gateway/egress-nat-policy.yaml Outdated Show resolved Hide resolved
This is a documentation of the detailed instructions on configuring
cilium components to start using the egress nat gateway feature.

RFE: cilium#13575

Signed-off-by: Yongkun Gui <ygui@google.com>
Signed-off-by: Bolun Zhao <blzhao@google.com>
@anfernee
Copy link
Contributor

test-me-please

1 similar comment
@anfernee
Copy link
Contributor

test-me-please

@pchaigno
Copy link
Member

test-me-please

@anfernee This isn't needed for documentation-only changes. There's already Documentation Updates / Validate & Build HTML (pull_request), which runs automatically, to test such changes.

@aanm aanm merged commit d89d81e into cilium:master Apr 27, 2021
1.10.0 automation moved this from In progress to Done Apr 27, 2021
@anfernee anfernee deleted the egress_doc branch April 27, 2021 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants