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.

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

Enable configuration of the calico IP_AUTODETECTION_METHOD and IP6_AUTODETECTION_METHOD #9175

Merged

Conversation

mtl-wgtwo
Copy link
Contributor

@mtl-wgtwo mtl-wgtwo commented May 25, 2020

By default, Calico auto-detects the IP address to bind to from the IP addresses available on the host using a "first-found" policy. This works, except when it doesn't. This will sometimes fail on hosts that have multiple interfaces. To work around this, Calico allows setting the IP_AUTODETECTION_METHOD environment variable and customizing how it selects the right IP address.

This PR adds the environment variables IP_AUTODETECTION_METHOD and IP6_AUTODETECTION_METHOD to the DaemonSet config and adds some api variables for this so they can be customized in the cluster yaml.

I'm not a kops expert by any means, so if there is a better way to do this please let me know.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 25, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @mtl-wgtwo. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 25, 2020
@k8s-ci-robot k8s-ci-robot requested review from hakman and johngmyers May 25, 2020 11:16
@mtl-wgtwo
Copy link
Contributor Author

/assign @geojaz

The bot tells me I should ping you!

@mtl-wgtwo mtl-wgtwo changed the title Enable configuration of the calico IP_AUTODETECTION_METHOD WIP: Enable configuration of the calico IP_AUTODETECTION_METHOD May 25, 2020
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 25, 2020
@olemarkus
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 25, 2020
@mtl-wgtwo
Copy link
Contributor Author

I see this in the build errors:

E0525 11:21:48.330642   17985 conversion.go:755] Warning: could not find nor generate a final Conversion function for k8s.io/kops/pkg/apis/kops.CalicoNetworkingSpec -> k8s.io/kops/pkg/apis/kops/v1alpha2.CalicoNetworkingSpec
E0525 11:21:48.330685   17985 conversion.go:756]   the following fields need manual conversion:
E0525 11:21:48.330690   17985 conversion.go:758]       - IPAutoDetectionMethod
E0525 11:21:48.330693   17985 conversion.go:758]       - IP6AutoDetectionMethod
F0525 11:21:49.567530   17985 main.go:122] Error: Failed executing generator: some packages had errors:
errors in package "k8s.io/kops/pkg/apis/kops/v1alpha2":
output for "v1alpha2/zz_generated.conversion.go" differs; first existing/expected diff: 
  "out.IptablesBackend = in.IptablesBackend\n\tout.IPIPMode = in.IPIPMode\n\tout.TyphaPrometheusMetricsEnab"
  "// WARNING: in.IPAutoDetectionMethod requires manual conversion: does not exist in peer-type\n\t// WAR"

Can someone please point me in the direction of how I would handle "manual conversion" of this?

@hakman
Copy link
Member

hakman commented May 25, 2020

@mtl-wgtwo you should run make apimachinery and maybe also make crds.
The result should be committed together with the PR.

@hakman
Copy link
Member

hakman commented May 25, 2020

One more thing that may help: c36470f.
Your final commit should look similar to that one.

@johngmyers
Copy link
Member

johngmyers commented May 26, 2020

@mtl-wgtwo You need to also add the fields to the v1alpha2 api. See https://github.com/kubernetes/kops/blob/master/docs/development/api_updates.md

Copy link
Member

@johngmyers johngmyers left a comment

Choose a reason for hiding this comment

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

What are the

pkg/apis/kops/networking.go Outdated Show resolved Hide resolved
pkg/apis/kops/networking.go Show resolved Hide resolved
@johngmyers
Copy link
Member

Since you are changing the templates, please bump the version in bootstrapchannelbuilder:

"k8s-1.12": "3.9.5-kops.3",
"k8s-1.16": "3.13.3-kops.2",

@mtl-wgtwo
Copy link
Contributor Author

@johngmyers Thanks for the pointers to the bits I wasn't aware of. I'll get this turned around.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 27, 2020
@mtl-wgtwo mtl-wgtwo changed the title WIP: Enable configuration of the calico IP_AUTODETECTION_METHOD Enable configuration of the calico IP_AUTODETECTION_METHOD and IP6_AUTODETECTION_METHOD May 27, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 27, 2020
pkg/apis/kops/validation/validation.go Outdated Show resolved Hide resolved
pkg/apis/kops/validation/validation.go Show resolved Hide resolved
pkg/apis/kops/validation/validation.go Outdated Show resolved Hide resolved
pkg/apis/kops/validation/validation.go Outdated Show resolved Hide resolved
pkg/apis/kops/validation/validation.go Outdated Show resolved Hide resolved
pkg/apis/kops/validation/validation.go Outdated Show resolved Hide resolved
pkg/apis/kops/validation/validation.go Outdated Show resolved Hide resolved
pkg/apis/kops/validation/validation.go Outdated Show resolved Hide resolved
mtl-wgtwo and others added 2 commits May 28, 2020 08:47
Co-authored-by: John Gardiner Myers <jgmyers@proofpoint.com>
@mtl-wgtwo
Copy link
Contributor Author

/test pull-kops-e2e-kubernetes-aws

@mtl-wgtwo
Copy link
Contributor Author

I'm not sure if the failing tests have anything to do with this change. I can't see why this change would affect just these two things and not anything else.

[k8s.io] PrivilegedPod [NodeConformance] should enable privileged commands [LinuxOnly]
[sig-storage] In-tree Volumes [Driver: local][LocalVolumeType: dir] [Testpattern: Pre-provisioned PV (default fs)] volumes should allow exec of files on the volume

@rifelpet
Copy link
Member

the test failure is a flake. retrying the test should (eventually) succeed

/retest

@mtl-wgtwo
Copy link
Contributor Author

/retest

Copy link
Member

@johngmyers johngmyers left a comment

Choose a reason for hiding this comment

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

I think the field.InternalError nit is the only potential blocker.

pkg/apis/kops/validation/validation.go Outdated Show resolved Hide resolved
pkg/apis/kops/validation/validation.go Show resolved Hide resolved
@johngmyers
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 29, 2020
@mtl-wgtwo
Copy link
Contributor Author

@hakman Is there anything you need from me to finalize this one?

@hakman
Copy link
Member

hakman commented Jun 3, 2020

It's ok also from my point of view. There is quite a long queue of PRs so it may take some time to get the approval to merge. Thanks @mtl-wgtwo.
/lgtm

@@ -100,6 +100,7 @@ require (
github.com/zclconf/go-cty v1.3.1
go.uber.org/zap v1.10.0
golang.org/x/crypto v0.0.0-20200220183623-bac4c82f6975
golang.org/x/net v0.0.0-20200202094626-16171245cfb2
Copy link
Member

Choose a reason for hiding this comment

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

i looked this up and this is already a transitive dependency so it is already present in go.sum and vendor. Thats why making it a direct dependency only results in a change to go.mod 👍

@rifelpet
Copy link
Member

rifelpet commented Jun 3, 2020

👍
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mtl-wgtwo, rifelpet

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 3, 2020
@k8s-ci-robot k8s-ci-robot merged commit 4ef6bbe into kubernetes:master Jun 3, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Jun 3, 2020
@mtl-wgtwo mtl-wgtwo deleted the calico-ip-detection-method branch February 3, 2021 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api 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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants