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

Add support for encryption in Cilium #9154

Merged
merged 1 commit into from
Jun 4, 2020
Merged

Add support for encryption in Cilium #9154

merged 1 commit into from
Jun 4, 2020

Conversation

moshevayner
Copy link
Member

@moshevayner moshevayner commented May 21, 2020 •
edited
Loading

Fixes #9031
Summary of the PR:

  1. Update the networking modules and CRDs (including auto generated) to support enableEncryption flag, along with creating an encryption key, along with relevant documentation.

  2. Add a subcommand ciliumpassword for kops create secret. This will generate the encryption key according to the provided instructions in the official cilium docs.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 21, 2020
@k8s-ci-robot k8s-ci-robot added area/api area/documentation size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 21, 2020
@johngmyers
Copy link
Member

Weave added a create secret weavepassword subcommand. I think I'd prefer a new subcommand of create secret than an option on create secret encryptionconfig.

Alternatively we could rename create secret weavepassword to create secret network and use that secret for any networking provider that supports encryption.

@moshevayner
Copy link
Member Author

Weave added a create secret weavepassword subcommand. I think I'd prefer a new subcommand of create secret than an option on create secret encryptionconfig.

Alternatively we could rename create secret weavepassword to create secret network and use that secret for any networking provider that supports encryption.

Yeah, my initial thought was to create a separate subcommand for cilium, but then I figured it will end up being too much code replication, just for the sake of having a different command.
As for renaming the weave script, I wasn't sure if that would be a good practice, since doing that would break backwards compatibility - how do we usually feel about this kind of this?

Which eventually brought me to think that adding on top of the existing one without breaking any existing compatibility would work best, for future cases (and weave would be the single exception).

WDYT?

@johngmyers
Copy link
Member

I think the user interface of an option on a subcommand is worse than the code duplication. The code duplication could probably be addressed with a refactor.

Cilium should either use the secret named "weavepassword" or there should be auto-migration code. For the Cobra subcommand we could make "weavepassword" an alias for "network" or whatever we choose.

olemarkus
olemarkus reviewed May 21, 2020
View reviewed changes
Copy link
Member

@olemarkus olemarkus left a comment

Choose a reason for hiding this comment

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

Besides the few comments I added, this looks good from cilium side. The use of encryptionConfig is more uncertain.

docs/networking/cilium.md Outdated Show resolved Hide resolved
pkg/apis/kops/networking.go Outdated Show resolved Hide resolved
@olemarkus
Copy link
Member

/hold until after we have branched off 1.18

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 21, 2020
@moshevayner
Copy link
Member Author

I think the user interface of an option on a subcommand is worse than the code duplication. The code duplication could probably be addressed with a refactor.

Cilium should either use the secret named "weavepassword" or there should be auto-migration code. For the Cobra subcommand we could make "weavepassword" an alias for "network" or whatever we choose.

Gotcha, @johngmyers. Point taken about the user interface. 😄
So since weave seems less strict about the key requirements than Cilium, I think it'll make most since to separate it to a new subcommand (kops create secret ciliumpassword). Uniting both of them under the same file would still mean they need separate functions, so at this point might as well just create a new subcommand.
I'll get to work on it

@moshevayner moshevayner force-pushed the issue-9031 branch 2 times, most recently from 0f066fb to cb9ff4b Compare May 24, 2020 05:37
@moshevayner
Copy link
Member Author

@johngmyers @olemarkus
I've addressed all comments and suggestions, and squashed my commits to a single one so it'll be cleaner.
Let me know your thoughts.
Thanks! 😄

@olemarkus
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2020
Adding encryption support for Cilium
316a0e2 
Adding support for 'secret-name' flag

Adding instructions to enable encryption

Updating docs for cli

Addressing comments

Adding ciliumpassword subcommand to 'kops create secret'

Updating command to generate ciliumpassword secret
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2020
@olemarkus
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2020
@moshevayner
Copy link
Member Author

@olemarkus now that 1.18 was branched-off, are we clear to remove the hold?

@olemarkus
Copy link
Member

Yep.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 2, 2020
@moshevayner
Copy link
Member Author

/assign @KashifSaadat

@moshevayner
Copy link
Member Author

/milestone 1.19

@k8s-ci-robot
Copy link
Contributor

@MoShitrit: You must be a member of the kubernetes/kops-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your Kops Maintainers and have them propose you as an additional delegate for this responsibility.

In response to this:

/milestone 1.19

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.

@rifelpet rifelpet added this to the v1.19 milestone Jun 3, 2020
@KashifSaadat
Copy link
Contributor

Looks great, thanks @MoShitrit for the contribution!

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: KashifSaadat, MoShitrit

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 Jun 4, 2020
@k8s-ci-robot k8s-ci-robot merged commit c6dcaa8 into kubernetes:master Jun 4, 2020
@k8s-ci-robot k8s-ci-robot modified the milestones: v1.19, v1.18 Jun 4, 2020
@moshevayner moshevayner deleted the issue-9031 branch June 4, 2020 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olemarkus olemarkus olemarkus left review comments

@joshbranham joshbranham Awaiting requested review from joshbranham

@robinpercy robinpercy Awaiting requested review from robinpercy

Assignees

@olemarkus olemarkus

@KashifSaadat KashifSaadat

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api area/documentation cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.18
Development

Successfully merging this pull request may close these issues.

Add ability to set encryption flags for Cilium
6 participants
@moshevayner @johngmyers @olemarkus @k8s-ci-robot @KashifSaadat @rifelpet