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

helm: move IPSec options under encryption.ipsec #15846

Merged
merged 2 commits into from
Apr 27, 2021
Merged

helm: move IPSec options under encryption.ipsec #15846

merged 2 commits into from
Apr 27, 2021

Conversation

jibi
Copy link
Member

@jibi jibi commented Apr 23, 2021 •
edited

This commit moves the keyFile, mountPath, secretName and interface Helm
options (IPSec specific) from encryption to encryption.ipsec, while
marking the old options as deprecated.

To avoid breaking existing configurations the new options have
precedence over the deprecated ones, but they don't have default values.

In this way:

  • if set, the encryption.ipsec.* options take precedence
  • otherwise we fallback to the deprecated encryption.* values (either
    the user specified values or the default ones)

@jibi jibi added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. area/helm Impacts helm charts and user deployment experience labels Apr 23, 2021
@jibi jibi requested review from gandro, brb, nbusseneau and a team April 23, 2021 12:52
@jibi jibi requested review from a team as code owners April 23, 2021 12:52
@jibi jibi requested review from a team and qmonnet April 23, 2021 12:52
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Apr 23, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Apr 23, 2021
@jibi jibi added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Apr 23, 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 23, 2021
qmonnet
qmonnet approved these changes Apr 23, 2021
View reviewed changes
install/kubernetes/cilium/values.yaml Outdated Show resolved Hide resolved
@qmonnet qmonnet removed their assignment Apr 23, 2021
@jibi jibi force-pushed the pr/jibi/reorg-ipsec-helm-opts branch from 8129376 to 6365049 Compare April 23, 2021 13:11
@jibi
Copy link
Member Author

jibi commented Apr 23, 2021

test-me-please

gandro
gandro approved these changes Apr 23, 2021
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Thanks!

brb
brb approved these changes Apr 23, 2021
View reviewed changes
Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

Thanks!

@jibi
Copy link
Member Author

jibi commented Apr 23, 2021 •
edited

Sorted the documentation by switching from:

  # -- Deprecated in favor of encryption.ipsec.interface.
  # The interface to use for encrypted traffic.
  # This option is only effective when encryption.type is set to ipsec.
  # interface: eth0

to:

  # -- Deprecated in favor of encryption.ipsec.interface.
  # The interface to use for encrypted traffic.
  # This option is only effective when encryption.type is set to ipsec.
  interface: ""

(to my understanding they are equivalent) and reordered the fields as suggested ✔️

@jibi jibi requested a review from nbusseneau April 23, 2021 17:04
nbusseneau
nbusseneau reviewed Apr 23, 2021
View reviewed changes
Copy link
Member

@nbusseneau nbusseneau left a comment

Choose a reason for hiding this comment

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

Not sure if that's only me but on my end the values in README are still ordered as they were before 😅

@jibi
Copy link
Member Author

jibi commented Apr 24, 2021

test-me-please

@jibi
Copy link
Member Author

jibi commented Apr 25, 2021

retest-gke

2 similar comments
@jibi
Copy link
Member Author

jibi commented Apr 25, 2021

retest-gke

@jibi
Copy link
Member Author

jibi commented Apr 26, 2021

retest-gke

@gandro gandro removed their assignment Apr 26, 2021
@jibi jibi force-pushed the pr/jibi/reorg-ipsec-helm-opts branch from 2e504a3 to a31c55b Compare April 26, 2021 12:54
@jibi
helm: move IPSec options under encryption.ipsec
fe724b2 
This commit moves the keyFile, mountPath, secretName and interface Helm
options (IPSec specific) from encryption to encryption.ipset, while
marking the old options as deprecated.

To avoid breaking existing configurations the new options have
precedence over the deprecated ones, but they don't have default values.

In this way:
* if set, the encryption.ipsec.* options take precedence
* otherwise we fallback to the deprecated encryption.* values (either
  the user specified values or the default ones)

Signed-off-by: Gilberto Bertin <gilberto@isovalent.com>
@jibi
docs: update IPSec documentation to reflect new Helm options
ba7eb65 
Signed-off-by: Gilberto Bertin <gilberto@isovalent.com>
@jibi jibi force-pushed the pr/jibi/reorg-ipsec-helm-opts branch from a31c55b to ba7eb65 Compare April 26, 2021 13:07
@jibi
Copy link
Member Author

jibi commented Apr 26, 2021

test-me-please

@jibi
Copy link
Member Author

jibi commented Apr 26, 2021

retest-4.19

@jibi
Copy link
Member Author

jibi commented Apr 26, 2021

retest-netnext

@jibi
Copy link
Member Author

jibi commented Apr 27, 2021

retest-gke

@jibi
Copy link
Member Author

jibi commented Apr 27, 2021

GKE tests are failing due to known issues (#15863), marking it as ready to be merged

@jibi jibi added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 27, 2021
@gandro gandro merged commit 709cf70 into master Apr 27, 2021
1.10.0 automation moved this from In progress to Done Apr 27, 2021
@gandro gandro deleted the pr/jibi/reorg-ipsec-helm-opts branch April 27, 2021 08:18
This was referenced Apr 28, 2021
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

@brb brb brb approved these changes

@nbusseneau nbusseneau nbusseneau approved these changes

@qmonnet qmonnet qmonnet approved these changes

Assignees
No one assigned
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. area/helm Impacts helm charts and user deployment experience ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
No open projects
1.10.0
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jibi @gandro @brb @nbusseneau @qmonnet @aanm