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

Fix k8s podCIDRs for vagrant deployment #22786

Merged
merged 1 commit into from Feb 20, 2023
Merged

Fix k8s podCIDRs for vagrant deployment #22786

merged 1 commit into from Feb 20, 2023

Conversation

romanspb80
Copy link
Contributor

@romanspb80 romanspb80 commented Dec 18, 2022 •
edited by christarazi

This patch makes podCIDRs of k8s Node and CiliumNode consistent: vagrant@k8s1:~/go/src/github.com/cilium/cilium$ kubectl get nodes k8s1 -o yaml apiVersion: v1
kind: Node
metadata:
...
spec:
podCIDR: 10.11.0.0/24
podCIDRs:

  • 10.11.0.0/24
  • fd04::/112 ...

vagrant@k8s1:~/go/src/github.com/cilium/cilium$ kubectl get ciliumnodes.cilium.io -o yaml apiVersion: v1
items:

  • apiVersion: cilium.io/v2 kind: CiliumNode ... spec: ... ipam: podCIDRs: - 10.11.0.0/24 - fd04::/112 ...

Fixes: #19594

Signed-off-by: Roman Ptitcyn romanspb@yahoo.com

@romanspb80 romanspb80 requested review from a team as code owners December 18, 2022 23:14
@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 Dec 18, 2022
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Dec 18, 2022
christarazi
christarazi requested changes Dec 19, 2022
View reviewed changes
Copy link
Member

@christarazi christarazi 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 PR. I think the changes need only be scoped to contrib/vagrant.

pkg/ip/ip_test.go Outdated Show resolved Hide resolved
pkg/labels/cidr/cidr_test.go Outdated Show resolved Hide resolved
pkg/option/config.go Outdated Show resolved Hide resolved
pkg/policy/cidr_test.go Outdated Show resolved Hide resolved
@christarazi christarazi added area/CI Continuous Integration testing issue or flake area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. area/CI-improvement Topic or proposal to improve the Continuous Integration workflow release-note/ci This PR makes changes to the CI. labels Dec 19, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Dec 19, 2022
@romanspb80 romanspb80 force-pushed the make_CIDRs_consistent_k8s_&_Cilium branch 2 times, most recently from 8a37f02 to 8745e2e Compare December 19, 2022 22:39
@maintainer-s-little-helper

This comment was marked as resolved.

Sign in to view
@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 Dec 24, 2022
@romanspb80 romanspb80 force-pushed the make_CIDRs_consistent_k8s_&_Cilium branch from cdd2eff to 826fc94 Compare December 25, 2022 21:27
@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 Dec 25, 2022
@aditighag
Copy link
Member

@julianwiedmann Could you re-review to check if your comments have been addressed? Thanks!

julianwiedmann
julianwiedmann requested changes Jan 9, 2023
View reviewed changes
Copy link
Member

@julianwiedmann julianwiedmann left a comment •
edited

Choose a reason for hiding this comment

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

Hi @romanspb80 , thanks! Besides the IPv6 inconsistency in the change (as noted below), a more general remark - we want these CIDRs to end up looking identical to what gets passed to

cilium/contrib/vagrant/start.sh

Lines 332 to 333 in 80af06e

cilium_operator_options+=" --cluster-pool-ipv4-cidr=10.${master_ipv4_suffix}.0.0/16"
cilium_operator_options+=" --cluster-pool-ipv6-cidr=fd04::/104"

Right now the per-node CIDR size gets auto-sized there, but it can be configured with --cluster-pool-ipv4-mask-size.

contrib/vagrant/scripts/helpers.bash Outdated Show resolved Hide resolved
@github-actions

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Feb 9, 2023
@maintainer-s-little-helper

This comment was marked as resolved.

Sign in to view
@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 Feb 11, 2023
@github-actions github-actions bot removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Feb 12, 2023
@romanspb80 romanspb80 force-pushed the make_CIDRs_consistent_k8s_&_Cilium branch from 7ffaf03 to befac67 Compare February 12, 2023 09:48
@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 Feb 12, 2023
@romanspb80
Fix k8s podCIDRs for vagrant deployment
b85a883 
This patch makes podCIDRs of k8s Node and CiliumNode consistent:
vagrant@k8s1:~/go/src/github.com/cilium/cilium$ kubectl get nodes k8s1 -o yaml
apiVersion: v1
kind: Node
metadata:
...
spec:
  podCIDR: 10.11.0.0/24
  podCIDRs:
  - 10.11.0.0/24
  - fd04::/112
...

vagrant@k8s1:~/go/src/github.com/cilium/cilium$ kubectl get ciliumnodes.cilium.io -o yaml
apiVersion: v1
items:
- apiVersion: cilium.io/v2
  kind: CiliumNode
  ...
  spec:
    ...
    ipam:
      podCIDRs:
      - 10.11.0.0/24
      - fd04::/112
...

Fixes: cilium#19594

Signed-off-by: Roman Ptitcyn <romanspb@yahoo.com>
@romanspb80 romanspb80 force-pushed the make_CIDRs_consistent_k8s_&_Cilium branch from befac67 to b85a883 Compare February 20, 2023 11:48
julianwiedmann
julianwiedmann approved these changes Feb 20, 2023
View reviewed changes
Copy link
Member

@julianwiedmann julianwiedmann 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 @romanspb80 !

@julianwiedmann
Copy link
Member

Think we're good to go - this is devVM-only, so the CI won't do us any good.

@julianwiedmann julianwiedmann added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 20, 2023
pchaigno
pchaigno approved these changes Feb 20, 2023
View reviewed changes
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've tested this locally.

@pchaigno pchaigno merged commit 2c3b814 into cilium:master Feb 20, 2023
@romanspb80
Copy link
Contributor Author

Thanks guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pchaigno pchaigno pchaigno approved these changes

@christarazi christarazi christarazi approved these changes

@julianwiedmann julianwiedmann julianwiedmann approved these changes

Assignees
No one assigned
Labels
area/CI Continuous Integration testing issue or flake area/CI-improvement Topic or proposal to improve the Continuous Integration workflow area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[dev-VM] make CIDRs & IPs consistent (k8s vs Cilium)
5 participants
@romanspb80 @aditighag @julianwiedmann @pchaigno @christarazi