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

Simplify Calico IPv6 configuration #11725

Merged
merged 1 commit into from
Jun 15, 2021
Merged

Simplify Calico IPv6 configuration #11725

merged 1 commit into from
Jun 15, 2021

Conversation

johngmyers
Copy link
Member

No description provided.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 9, 2021
@k8s-ci-robot k8s-ci-robot added area/api size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 9, 2021
@hakman
Copy link
Member

hakman commented Jun 9, 2021

If you want to simplify, please automatically set IPv4 and IPv6 support based on NonMasqueradeCIDR.
The 2 flags should stay as they are for now, to allow override.

@johngmyers
Copy link
Member Author

There is no need for override. Dual-stack is out of scope. The CNI needs to operate in the same protocol version as the pod and service networks.

@hakman
Copy link
Member

hakman commented Jun 9, 2021

Dual-stack is out of scope for k8s. CNI still has some tricks that can be done via internal dual-stack.
For example these overrides make it possible to get away without using NAT64.

@johngmyers
Copy link
Member Author

Those "tricks" could be coded up as separate features with their own settings.

@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 Jun 12, 2021
@hakman
Copy link
Member

hakman commented Jun 12, 2021

/hold

@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 Jun 12, 2021
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2021
hakman
hakman requested changes Jun 14, 2021
View reviewed changes
upup/pkg/fi/cloudup/template_functions.go Outdated
@@ -223,6 +223,7 @@ func (tf *TemplateFunctions) AddTo(dest template.FuncMap, secretStore fi.SecretS
return strings.Join(labels, ",")
}

dest["IsIPv6Only"] = tf.IsIPv6Only
Copy link
Member

@hakman hakman Jun 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Would you be ok with something like this? It would make the template more clear.
At this stage, it is irrelevant if cluster is IPv4 or IPv6, but only if they should be enabled or not.
Later, if we decide to add DualStack, we can keep the template and just modify the logic in the function(s).

Suggested change
dest["IsIPv6Only"] = tf.IsIPv6Only
dest["EnableIPv4"] = !tf.IsIPv6Only
dest["EnableIPv6"] = tf.IsIPv6Only

@johngmyers johngmyers changed the title Simplify IPv6 configuration Simplify Calico IPv6 configuration Jun 14, 2021
hakman
hakman approved these changes Jun 15, 2021
View reviewed changes
Copy link
Member

@hakman hakman left a comment

Choose a reason for hiding this comment

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

We can iterate on this later. For now, it makes it possible to run IPv6 setups easier.
/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jun 15, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman

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 15, 2021
@k8s-ci-robot k8s-ci-robot merged commit 7a017af into kubernetes:master Jun 15, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone Jun 15, 2021
@johngmyers johngmyers deleted the is-ipv6 branch June 15, 2021 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hakman hakman hakman approved these changes

@KashifSaadat KashifSaadat Awaiting requested review from KashifSaadat

@mikesplain mikesplain Awaiting requested review from mikesplain

Assignees

@olemarkus olemarkus

@hakman hakman

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

Successfully merging this pull request may close these issues.
4 participants
@johngmyers @hakman @olemarkus @k8s-ci-robot