Make sure Calico can talk IP-in-IP by sfzylad · Pull Request #701 · kubernetes-sigs/cluster-api-provider-aws

sfzylad · 2019-04-04T16:37:00Z

What this PR does / why we need it:
Allow Calico to talk IP-in-IP protocol

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #554

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

sethp-nr · 2019-04-04T16:44:11Z

This looks like it overlaps a bit with #697 in handling the From/To port gunk. I guess we're racing to see who gets the conflict? :)

Also, I think you'll want to add this rule to the ControlPlane security group as well.

sfzylad · 2019-04-04T16:51:16Z

Let's see who'll be the first to get into master. :)

I only added it to the node SG because now it's getting attached to the control plane nodes too, so I thought it's redundant. However I'm happy to add it.

vincepri · 2019-04-04T16:53:01Z

It might be worthwhile to touch base on both PR at the next community meeting, wdyt?

sfzylad · 2019-04-04T17:02:27Z

@vincepri Great idea. Let's discuss it Monday. :)

sethp-nr · 2019-04-04T17:04:18Z

That's Monday the 8th, right? That'd work for me.

randomvariable · 2019-04-10T16:46:23Z

/lgtm

detiber · 2019-04-10T16:48:34Z

/approve

vincepri

/approve

k8s-ci-robot · 2019-04-10T16:49:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: detiber, sfzylad, vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [detiber,vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* MAke sure Calico can talk IP-in-IP * Add IP in IP protocol to the control plane security group * Add IPv4 protocol definition and make sure it's handled properly. * Make port ranges AWS complient and security groups more restrictive. * Fix security groups

* Update the releasing docs (#689) * Add error reason to output if fail to checkout an account from boskos (#698) * Temporary workaround a data issue in boskos service (#699) * Update checkout_account.py to not reuse connections (#700) * Fix checkout_account.py (#702) * Make hack/checkin_account.py executable (#703) * Fix: all traffic ingress rule triggers fatal nil dereference (#697) * fix: respect all traffic security group rules (and others) For anything besides tcp, udp, icmp, and icmpv6 there is no applicable notion of "port range." AWS omits FromPort and ToPort in its responses, causing a fatal nil dereference when attempting to read any security groups with e.g. an "all traffic" rule. * fix: omit description when empty string * fix: handle more security groups without crashing This commit cleans up and clarifies a few of the less obvious components of the previous work. * fix: handle more security groups without crashing Address linter failures. * fix: handle more security groups without crashing Usage needs to match declaration. Computers are sticklers about that sort of thing. * fix: handle more security groups without crashing Add clarifying comment to serializer function. * Fixes a bug and adds tests for kubeadm defaults (#707) The pointers were not working as expected so the API is changing to be more functional and leverage kubernetes' DeepCopy function. * Update listed v1.14 AMIs to v1.14.1 (#708) * Update listed v1.14 AMIs to v1.14.1 * Update README with list of published AMIs/Kubernetes versions * GZIP user-data (#710) Signed-off-by: Vince Prignano <vincepri@vmware.com> * Make sure Calico can talk IP-in-IP (#701) * MAke sure Calico can talk IP-in-IP * Add IP in IP protocol to the control plane security group * Add IPv4 protocol definition and make sure it's handled properly. * Make port ranges AWS complient and security groups more restrictive. * Fix security groups * Adds tests to kubeadm defaults (#709) Attempt at documenting the assumptions made in the kubeadm defaults code. Signed-off-by: Chuck Ha <chuckh@vmware.com> * Logging (#713) * Adds logr as dependency Signed-off-by: Chuck Ha <chuckh@vmware.com> * Use logr in the cluster actuator This only creates the logger. Does not yet swap out actual klog calls. Signed-off-by: Chuck Ha <chuckh@vmware.com> * update bazel Signed-off-by: Chuck Ha <chuckh@vmware.com> * update Signed-off-by: Chuck Ha <chuckh@vmware.com> * Switch dep to use release-0.1 branch instead of version (#715) * Adds logr as dependency (#714) Adds context for logs and removes excessive logging Signed-off-by: Chuck Ha <chuckh@vmware.com> * Ensure `make manifests` generates machines file for HA control plane too. (#720) * Add HA machines template * Introduce HA machines file in `make manifests` target * Add clusterawsadm as make dependency to manifests make target. (#721) Ensures manifests are generated from the current state of the source. Assuming $GOPATH/bin is in the $PATH * Update to Go 1.12 (#719) Signed-off-by: Vince Prignano <vincepri@vmware.com> * Add ability to override Organization ID for image lookups (#723) * Add ability to override Organization ID for image lookups * Update pkg/cloud/aws/services/ec2/ami.go Co-Authored-By: detiber <detiberusj@vmware.com> * Add updated generated crd * feat: support customizing root device size (#718) * feat: support customizing root device size * chore: re-generate CRDs * fix: update formatting * chore: add comment describing Service.sdkToInstance * chore: make service.SDKToInstance public * Rename BUILD -> BUILD.bazel for consistency (#724) find . -type file -name BUILD -not -path "./vendor/*" | xargs -n1 -I{} -- git mv {} {}.bazel Preferred build name changed in 3788fb1 Fixes #722 * Adds retry-on-conflict during updates (#725) * Adds retry-on-conflict during updates Signed-off-by: Chuck Ha <chuckh@vmware.com> * adds note about status update caveat Signed-off-by: Chuck Ha <chuckh@vmware.com> * clarify errors/comments Signed-off-by: Chuck Ha <chuckh@vmware.com> * Add the HA machines configuration to bazel (#733) Signed-off-by: Chuck Ha <chuckh@vmware.com> * Ensure bazel is the correct version (#731) Signed-off-by: Chuck Ha <chuckh@vmware.com> * Update OWNERS_ALIASES and SECURITY_CONTACTS (#712) * Fix the prow jobs (#735) Signed-off-by: Chuck Ha <chuckh@vmware.com> * Fix markdown formatting (#736) * extract fmt from release tool (#738) Signed-off-by: Chuck Ha <chuckh@vmware.com> * Use DEFAULT_REGION as the default and REGION as the supplied (#739) Signed-off-by: Chuck Ha <chuckh@vmware.com> * e2e testing improvement (#743) * Bump kind version * Remove docker load in favor of kind load for e2e cluster Signed-off-by: Chuck Ha <chuckh@vmware.com> * fix: Don't try to update root size when it's unset (#726) * fix: Don't try to update root size when it's unset This commit looks for empty RootDeviceSize in the spec and ignores it. Otherwise, none of our control plane machines were updating with this error: ``` E0418 23:07:48.250925 1 controller.go:214] Error updating machine "ns/controlplane-2": found attempt to change immutable state for machine "controlplane-2": ["Root volume size cannot be mutated from 8 to 0"] ``` * fix: updates without specifying a root volume size Add unit test. * fix: updates without specifying a root volume size Fix gofmt. * Scope nodeRef to workload cluster (#744) Signed-off-by: Vince Prignano <vincepri@vmware.com> * Fix NPE on delete bastion host (#746) Signed-off-by: Vince Prignano <vincepri@vmware.com> * Documentation for creating a new cluster on a different AWS account (#728) * Initial draft of documentation for Cluster creation using cross account role assumption * Update roleassumption.md Complete the document. * cleanup the documentation for roleassumption * Resolved the comments: role assumption documentation. * Fix minor issues - roleassumption.md * resolve more comments to roleassumption.md * Resolve more comments - roleassumption.md * include machines-ha.yaml.template in release artifacts (#741) * Update AWS sdk, improve log in machine actuator delete (#747) Signed-off-by: Vince Prignano <vincepri@vmware.com> * Fixes the infinite reconcile loop (#748) * Uses patch for updating the cluster and machine specs - patch does not cause a re-reconcile in the capi controller * Uses update for updating the cluster and machine status - update for status is ok since it does not update any of the metadata no re-reconcile is necessary for the capi controller Signed-off-by: Chuck Ha <chuckh@vmware.com> * Update Gopkg.lock and cleanup Makefile (#751) * Update cluster-api release-0.1 vendor (#750) Signed-off-by: Vince Prignano <vincepri@vmware.com> * Reduce the number of re-reconciles (#752) Signed-off-by: Chuck Ha <chuckh@vmware.com>

* Update the releasing docs (kubernetes-sigs#689) * Add error reason to output if fail to checkout an account from boskos (kubernetes-sigs#698) * Temporary workaround a data issue in boskos service (kubernetes-sigs#699) * Update checkout_account.py to not reuse connections (kubernetes-sigs#700) * Fix checkout_account.py (kubernetes-sigs#702) * Make hack/checkin_account.py executable (kubernetes-sigs#703) * Fix: all traffic ingress rule triggers fatal nil dereference (kubernetes-sigs#697) * fix: respect all traffic security group rules (and others) For anything besides tcp, udp, icmp, and icmpv6 there is no applicable notion of "port range." AWS omits FromPort and ToPort in its responses, causing a fatal nil dereference when attempting to read any security groups with e.g. an "all traffic" rule. * fix: omit description when empty string * fix: handle more security groups without crashing This commit cleans up and clarifies a few of the less obvious components of the previous work. * fix: handle more security groups without crashing Address linter failures. * fix: handle more security groups without crashing Usage needs to match declaration. Computers are sticklers about that sort of thing. * fix: handle more security groups without crashing Add clarifying comment to serializer function. * Fixes a bug and adds tests for kubeadm defaults (kubernetes-sigs#707) The pointers were not working as expected so the API is changing to be more functional and leverage kubernetes' DeepCopy function. * Update listed v1.14 AMIs to v1.14.1 (kubernetes-sigs#708) * Update listed v1.14 AMIs to v1.14.1 * Update README with list of published AMIs/Kubernetes versions * GZIP user-data (kubernetes-sigs#710) Signed-off-by: Vince Prignano <vincepri@vmware.com> * Make sure Calico can talk IP-in-IP (kubernetes-sigs#701) * MAke sure Calico can talk IP-in-IP * Add IP in IP protocol to the control plane security group * Add IPv4 protocol definition and make sure it's handled properly. * Make port ranges AWS complient and security groups more restrictive. * Fix security groups * Adds tests to kubeadm defaults (kubernetes-sigs#709) Attempt at documenting the assumptions made in the kubeadm defaults code. Signed-off-by: Chuck Ha <chuckh@vmware.com> * Logging (kubernetes-sigs#713) * Adds logr as dependency Signed-off-by: Chuck Ha <chuckh@vmware.com> * Use logr in the cluster actuator This only creates the logger. Does not yet swap out actual klog calls. Signed-off-by: Chuck Ha <chuckh@vmware.com> * update bazel Signed-off-by: Chuck Ha <chuckh@vmware.com> * update Signed-off-by: Chuck Ha <chuckh@vmware.com> * Switch dep to use release-0.1 branch instead of version (kubernetes-sigs#715) * Adds logr as dependency (kubernetes-sigs#714) Adds context for logs and removes excessive logging Signed-off-by: Chuck Ha <chuckh@vmware.com> * Ensure `make manifests` generates machines file for HA control plane too. (kubernetes-sigs#720) * Add HA machines template * Introduce HA machines file in `make manifests` target * Add clusterawsadm as make dependency to manifests make target. (kubernetes-sigs#721) Ensures manifests are generated from the current state of the source. Assuming $GOPATH/bin is in the $PATH * Update to Go 1.12 (kubernetes-sigs#719) Signed-off-by: Vince Prignano <vincepri@vmware.com> * Add ability to override Organization ID for image lookups (kubernetes-sigs#723) * Add ability to override Organization ID for image lookups * Update pkg/cloud/aws/services/ec2/ami.go Co-Authored-By: detiber <detiberusj@vmware.com> * Add updated generated crd * feat: support customizing root device size (kubernetes-sigs#718) * feat: support customizing root device size * chore: re-generate CRDs * fix: update formatting * chore: add comment describing Service.sdkToInstance * chore: make service.SDKToInstance public * Rename BUILD -> BUILD.bazel for consistency (kubernetes-sigs#724) find . -type file -name BUILD -not -path "./vendor/*" | xargs -n1 -I{} -- git mv {} {}.bazel Preferred build name changed in 3788fb1 Fixes kubernetes-sigs#722 * Adds retry-on-conflict during updates (kubernetes-sigs#725) * Adds retry-on-conflict during updates Signed-off-by: Chuck Ha <chuckh@vmware.com> * adds note about status update caveat Signed-off-by: Chuck Ha <chuckh@vmware.com> * clarify errors/comments Signed-off-by: Chuck Ha <chuckh@vmware.com> * Add the HA machines configuration to bazel (kubernetes-sigs#733) Signed-off-by: Chuck Ha <chuckh@vmware.com> * Ensure bazel is the correct version (kubernetes-sigs#731) Signed-off-by: Chuck Ha <chuckh@vmware.com> * Update OWNERS_ALIASES and SECURITY_CONTACTS (kubernetes-sigs#712) * Fix the prow jobs (kubernetes-sigs#735) Signed-off-by: Chuck Ha <chuckh@vmware.com> * Fix markdown formatting (kubernetes-sigs#736) * extract fmt from release tool (kubernetes-sigs#738) Signed-off-by: Chuck Ha <chuckh@vmware.com> * Use DEFAULT_REGION as the default and REGION as the supplied (kubernetes-sigs#739) Signed-off-by: Chuck Ha <chuckh@vmware.com> * e2e testing improvement (kubernetes-sigs#743) * Bump kind version * Remove docker load in favor of kind load for e2e cluster Signed-off-by: Chuck Ha <chuckh@vmware.com> * fix: Don't try to update root size when it's unset (kubernetes-sigs#726) * fix: Don't try to update root size when it's unset This commit looks for empty RootDeviceSize in the spec and ignores it. Otherwise, none of our control plane machines were updating with this error: ``` E0418 23:07:48.250925 1 controller.go:214] Error updating machine "ns/controlplane-2": found attempt to change immutable state for machine "controlplane-2": ["Root volume size cannot be mutated from 8 to 0"] ``` * fix: updates without specifying a root volume size Add unit test. * fix: updates without specifying a root volume size Fix gofmt. * Scope nodeRef to workload cluster (kubernetes-sigs#744) Signed-off-by: Vince Prignano <vincepri@vmware.com> * Fix NPE on delete bastion host (kubernetes-sigs#746) Signed-off-by: Vince Prignano <vincepri@vmware.com> * Documentation for creating a new cluster on a different AWS account (kubernetes-sigs#728) * Initial draft of documentation for Cluster creation using cross account role assumption * Update roleassumption.md Complete the document. * cleanup the documentation for roleassumption * Resolved the comments: role assumption documentation. * Fix minor issues - roleassumption.md * resolve more comments to roleassumption.md * Resolve more comments - roleassumption.md * include machines-ha.yaml.template in release artifacts (kubernetes-sigs#741) * Update AWS sdk, improve log in machine actuator delete (kubernetes-sigs#747) Signed-off-by: Vince Prignano <vincepri@vmware.com> * Fixes the infinite reconcile loop (kubernetes-sigs#748) * Uses patch for updating the cluster and machine specs - patch does not cause a re-reconcile in the capi controller * Uses update for updating the cluster and machine status - update for status is ok since it does not update any of the metadata no re-reconcile is necessary for the capi controller Signed-off-by: Chuck Ha <chuckh@vmware.com> * Update Gopkg.lock and cleanup Makefile (kubernetes-sigs#751) * Update cluster-api release-0.1 vendor (kubernetes-sigs#750) Signed-off-by: Vince Prignano <vincepri@vmware.com> * Reduce the number of re-reconciles (kubernetes-sigs#752) Signed-off-by: Chuck Ha <chuckh@vmware.com>

k8s-ci-robot requested review from ingvagabund and vincepri April 4, 2019 16:37

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 4, 2019

randomvariable reviewed Apr 4, 2019

View reviewed changes

Comment thread pkg/cloud/aws/services/ec2/securitygroups.go Outdated

randomvariable reviewed Apr 4, 2019

View reviewed changes

Comment thread pkg/cloud/aws/services/ec2/securitygroups.go Outdated

randomvariable reviewed Apr 4, 2019

View reviewed changes

Comment thread pkg/cloud/aws/services/ec2/securitygroups.go Outdated

This was referenced Apr 5, 2019

Is Cluster API Provider AWS clashing with cloud provider code? #704

Closed

refactor: separate CAPA resources from cluster #706

Merged

sfzylad added 5 commits April 10, 2019 12:56

MAke sure Calico can talk IP-in-IP

2dee686

Add IP in IP protocol to the control plane security group

c331591

Add IPv4 protocol definition and make sure it's handled properly.

97f806c

Make port ranges AWS complient and security groups more restrictive.

a4688d5

Fix security groups

0bc2dfb

sfzylad force-pushed the ip-in-ip-net branch from a3f81e0 to 0bc2dfb Compare April 10, 2019 16:41

k8s-ci-robot assigned randomvariable Apr 10, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 10, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 10, 2019

vincepri approved these changes Apr 10, 2019

View reviewed changes

k8s-ci-robot merged commit c694a55 into kubernetes-sigs:master Apr 10, 2019

sfzylad deleted the ip-in-ip-net branch April 10, 2019 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure Calico can talk IP-in-IP#701

Make sure Calico can talk IP-in-IP#701
k8s-ci-robot merged 5 commits intokubernetes-sigs:masterfrom
sfzylad:ip-in-ip-net

sfzylad commented Apr 4, 2019

Uh oh!

sethp-nr commented Apr 4, 2019

Uh oh!

sfzylad commented Apr 4, 2019

Uh oh!

vincepri commented Apr 4, 2019

Uh oh!

sfzylad commented Apr 4, 2019

Uh oh!

sethp-nr commented Apr 4, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

randomvariable commented Apr 10, 2019

Uh oh!

detiber commented Apr 10, 2019

Uh oh!

vincepri left a comment

Uh oh!

k8s-ci-robot commented Apr 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

sfzylad commented Apr 4, 2019

Uh oh!

sethp-nr commented Apr 4, 2019

Uh oh!

sfzylad commented Apr 4, 2019

Uh oh!

vincepri commented Apr 4, 2019

Uh oh!

sfzylad commented Apr 4, 2019

Uh oh!

sethp-nr commented Apr 4, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

randomvariable commented Apr 10, 2019

Uh oh!

detiber commented Apr 10, 2019

Uh oh!

vincepri left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Apr 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants