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

cilium: encryption, use default route interface if encrypt-interface is not specified #8867

Merged
merged 1 commit into from
Aug 13, 2019
Merged

cilium: encryption, use default route interface if encrypt-interface is not specified #8867

merged 1 commit into from
Aug 13, 2019

Conversation

lbernail
Copy link
Contributor

@lbernail lbernail commented Aug 10, 2019 •
edited by tgraf

This PR completes #8711 "use default route interface if encrypt-interface is not specified"

  • removes the test in initEnv
  • add the logic from WriteNodeConfig in compileBase to inject the variable in init.sh
  • add the logic from WriteNodeConfig in runDaemon to inject the variable in datapathConfig

@jrfastab I used the logic from your initial PR but we now have the same exact code in 3 locations. I wonder if it wouldn't be clearer to put this in initEnv instead. What do you think? I can update the PR

(I tested this change on a test cluster and things look fine)

This change is Reviewable

@lbernail lbernail requested a review from a team August 10, 2019 18:31
houndci-bot
houndci-bot reviewed Aug 10, 2019
View reviewed changes
daemon/daemon_main.go Outdated
@@ -1323,9 +1319,25 @@ func (d *Daemon) initKVStore() {
}

func runDaemon() {
var eif string = ""
Copy link

Choose a reason for hiding this comment

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

should drop = "" from declaration of var eif; it is the zero value

@coveralls
Copy link

coveralls commented Aug 10, 2019 •
edited

Coverage Status

Coverage increased (+0.03%) to 44.18% when pulling 3ac26d88bb5f19be94b41dcb79cd50741d888a56 on DataDog:lbernail/default-encryptif into a96d7f4 on cilium:master.

@jrfastab jrfastab added area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. kind/bug This is a bug in the Cilium logic. priority/high This is considered vital to an upcoming release. backport-done/1.6 and removed backport-done/1.6 labels Aug 12, 2019
@jrfastab
Copy link
Contributor

@lbernail Agreed it would probably be a bit cleaner if its all moved into initEnv.

@lbernail
Copy link
Contributor Author

OK, let me update the PR

@ianvernon ianvernon added this to To Merge To Master in 1.6.0-rc6 Aug 12, 2019
@lbernail lbernail requested a review from a team August 12, 2019 18:43
@lbernail
Copy link
Contributor Author

@jrfastab I updated the PR to move the logic to initEnv only
I kept the initial commit in case we want to revert but if we agree on this I'll squash everything into a single commit before merging

@jrfastab
Copy link
Contributor

test-me-please

@jrfastab
Copy link
Contributor

@lbernail makes sense to me. Squash it all together and I'll kick of testing again.

@lbernail
Centralize automatic interface detection in initEnv
fe44538 
Signed-off-by: Laurent Bernaille <laurent.bernaille@datadoghq.com>
@lbernail
Copy link
Contributor Author

@jrfastab Great. I just rebased

@jrfastab
Copy link
Contributor

test-me-please

@ianvernon
Copy link
Member

Build failed: https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Validated/14259/execution/node/7/log/

Jenkins slave ran out of disk space. I took the offending Jenkins slave offline; triggering build again.

@ianvernon
Copy link
Member

test-me-please

@tgraf tgraf merged commit aa20fd3 into cilium:master Aug 13, 2019
@tgraf
Copy link
Member

tgraf commented Aug 13, 2019

Nice @lbernail!

@lbernail lbernail deleted the lbernail/default-encryptif branch August 13, 2019 14:02
@ianvernon ianvernon moved this from To Merge To Master to To Backport in 1.6.0-rc6 Aug 13, 2019
@brb brb mentioned this pull request Aug 14, 2019
Merged
@ianvernon ianvernon moved this from To Backport to Backport Pending in 1.6.0-rc6 Aug 14, 2019
@ianvernon ianvernon moved this from Backport Pending to Done in 1.6.0-rc6 Aug 15, 2019
@aanm aanm added the release-note/misc This PR makes changes that have no direct user impact. label Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@jrfastab jrfastab jrfastab approved these changes

@tgraf tgraf tgraf approved these changes

Assignees
No one assigned
Labels
area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. kind/bug This is a bug in the Cilium logic. priority/high This is considered vital to an upcoming release. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.6.0-rc6
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@lbernail @coveralls @jrfastab @ianvernon @tgraf @houndci-bot @brb @aanm