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

Encryption docs update #14940

Merged
merged 1 commit into from May 7, 2021
Merged

Encryption docs update #14940

merged 1 commit into from May 7, 2021

Conversation

aditighag
Copy link
Member

@aditighag aditighag commented Feb 11, 2021 •
edited

This is a follow-up to #14924.

  • Adds more troubleshooting steps.
  • Add steps for IPAM EMI mode.
  • Match installation and validation steps.

Fixes: b6ec84c

@aditighag aditighag requested review from a team February 11, 2021 18:43
@aditighag aditighag requested review from a team as code owners February 11, 2021 18:43
@aditighag aditighag requested a review from a team February 11, 2021 18:43
@aditighag aditighag requested a review from a team as a code owner February 11, 2021 18:43
@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 Feb 11, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Feb 11, 2021
@aditighag aditighag marked this pull request as draft February 11, 2021 18:43
joestringer
joestringer requested changes Feb 11, 2021
View reviewed changes
Documentation/gettingstarted/encryption.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/encryption.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/encryption.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/encryption.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/encryption.rst Outdated
--set ipam.mode=eni \\
--set tunnel=disabled \\
--set encryption.enabled=true \\
--set encryption.nodeEncryption=false
Copy link
Member

Choose a reason for hiding this comment

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

I'll defer to @jrfastab here but from what I understand, the user must also specify --ipv4-pod-subnets to cover all subnets that AWS may allocate IPs from within the cluster. Note this is not currently exposed in the helm charts, so users would need to either use extraArgs or we'd need to introduce dedicated helm options for this setting.

qmonnet
qmonnet approved these changes Feb 12, 2021
View reviewed changes
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.

Few suggestions on minor nitpicks, looks good to me besides Joe's concerns.

pkg/datapath/linux/routing/routing.go Outdated Show resolved Hide resolved
pkg/datapath/linux/routing/routing.go Outdated Show resolved Hide resolved
Documentation/gettingstarted/encryption.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/encryption.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/encryption.rst Outdated Show resolved Hide resolved
@qmonnet qmonnet removed their assignment Feb 12, 2021
@pchaigno pchaigno marked this pull request as draft April 6, 2021 13:50
@aditighag
Copy link
Member Author

aditighag commented May 3, 2021 •
edited

I'll resurrect this PR to submit this to 1.10. Marking it as a release-blocker, see the note - https://github.com/isovalent/roadmap/issues/64#issuecomment-831341265.

@aditighag aditighag force-pushed the encryption-docs-update branch 2 times, most recently from 6688875 to 10f25df Compare May 3, 2021 23:09
@aditighag
Copy link
Member Author

@jrfastab @joestringer I rebased and cleaned up the guide based on the recent encryption fixes/regressions. PTAL.

@aditighag aditighag marked this pull request as ready for review May 3, 2021 23:11
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.0-rc2 May 3, 2021
@joestringer joestringer mentioned this pull request May 3, 2021
Merged
joestringer
joestringer requested changes May 6, 2021
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Looks fine to me, just a couple of nits.

I'd still like for there to be a proper way to enforce encryption and validate that it's working correctly without executing into the cilium pods and installing software since that's generally not a recommended production workflow, but in the absence of such features and for a getting started guide I think this is probably OK.

Documentation/gettingstarted/encryption-ipsec.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/encryption-ipsec.rst Outdated Show resolved Hide resolved
@aditighag
docs: Improve ipsec encryption guide
05b0208 
- Adds more troubleshooting steps.
- Add steps for IPAM EMI mode.
- Match installation and validation steps. (Fixes: b6ec84c)

Signed-off-by: Aditi Ghag <aditi@cilium.io>
@aditighag aditighag requested a review from joestringer May 6, 2021 02:24
joestringer
joestringer approved these changes May 6, 2021
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@aditighag aditighag added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 6, 2021
@ti-mo ti-mo merged commit 7b27a99 into cilium:master May 7, 2021
1.10.0 automation moved this from In progress to Done May 7, 2021
@brb brb mentioned this pull request May 7, 2021
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.0-rc2 May 7, 2021
@aanm aanm moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.0-rc2 May 17, 2021
@aanm aanm mentioned this pull request May 17, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer joestringer approved these changes

@qmonnet qmonnet qmonnet approved these changes

@jrfastab jrfastab Awaiting requested review from jrfastab

Assignees

@jrfastab jrfastab

Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.10.0
Done
1.10.0-rc2
Backport done to v1.10
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@aditighag @joestringer @jrfastab @qmonnet @twpayne @brb @borkmann @ti-mo @jrajahalme