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

salt: Check Service CIDR has a route declared #3076

Merged
merged 3 commits into from
Feb 5, 2021

Conversation

gdemonet
Copy link
Contributor

@gdemonet gdemonet commented Feb 2, 2021

Component: salt, kubernetes, networking

Context:

When deploying some control plane services in the cluster (e.g. calico), we let such services interact with kube-apiserver using its Service IP (10.96.0.1 by default).
However, the resolution of such virtual IPs, emulated using iptables (through kube-proxy), requires at a minimum that a route exists for the Service CIDR (a default route also works).
We add a check to the metalk8s_checks.node helper, to have it run before bootstrap and expansion.

See: kubernetes/kubernetes#57534

Summary:

  • First check implemented using network.get_route Salt execution method
  • Re-worked to handle systems where kernel refuses network ranges (strict netlink validation)
  • Removed initial implementation, simply read through the entire routing table

Acceptance criteria:

@gdemonet gdemonet requested a review from a team as a code owner February 2, 2021 13:03
@bert-e
Copy link
Contributor

bert-e commented Feb 2, 2021

Hello gdemonet,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Status report is not available.

@bert-e
Copy link
Contributor

bert-e commented Feb 2, 2021

Waiting for approval

The following approvals are needed before I can proceed with the merge:

  • the author

  • one peer

Peer approvals must include at least 1 approval from the following list:

@gdemonet gdemonet added kind:enhancement New feature or request topic:deployment Bugs in or enhancements to deployment stages topic:networking Networking-related issues labels Feb 2, 2021
@bert-e
Copy link
Contributor

bert-e commented Feb 3, 2021

Waiting for approval

The following approvals are needed before I can proceed with the merge:

  • the author

  • one peer

Peer approvals must include at least 1 approval from the following list:

When deploying some control plane services in the cluster (e.g. calico),
we let such services interact with kube-apiserver using its Service IP
(10.96.0.1 by default).
However, the resolution of such virtual IPs, emulated using iptables
(through kube-proxy), requires at a minimum that a route exists for the
Service CIDR (a default route also works).
We add a check to the `metalk8s_checks.node` helper, to have it run
before bootstrap and expansion.

See: kubernetes/kubernetes#57534
"ip route get" fails on recent kernel versions (with
NETLINK_GET_STRICT_CHK enabled) if provided with a CIDR larger than /32
(for IPv4).
This prevents our call to `network.get_route` from working if we give it
the full Service IP range.
To work around this limitation, we now:
- cast the `destination` into a `ipaddress.IPv4Network`
- get a route for the network's `network_address`
- if found, check the routing table and verify that the configured
routes include the full network range

See: https://bugzilla.redhat.com/show_bug.cgi?id=1852038
In the previous commit, we introduce a validation of the full Service
CIDR by looking at all routes defined in the system routing tables
(obtained with `network.routes`).
This change makes the previous approach (using `network.get_route`,
similar to an `ip route get` invocation) unneeded, as we are already
comparing networks inclusion-wise.
@gdemonet gdemonet force-pushed the improvement/pre-check-default-route branch from cd79fe2 to 1c347fc Compare February 4, 2021 08:59
@gdemonet
Copy link
Contributor Author

gdemonet commented Feb 5, 2021

/approve

@bert-e
Copy link
Contributor

bert-e commented Feb 5, 2021

Waiting for approval

The following approvals are needed before I can proceed with the merge:

  • the author

  • one peer

Peer approvals must include at least 1 approval from the following list:

The following options are set: approve

Copy link
Collaborator

@TeddyAndrieux TeddyAndrieux left a comment

Choose a reason for hiding this comment

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

LGTM

@bert-e
Copy link
Contributor

bert-e commented Feb 5, 2021

In the queue

The changeset has received all authorizations and has been added to the
relevant queue(s). The queue(s) will be merged in the target development
branch(es) as soon as builds have passed.

The changeset will be merged in:

  • ✔️ development/2.8

The following branches will NOT be impacted:

  • development/1.0
  • development/1.1
  • development/1.2
  • development/1.3
  • development/2.0
  • development/2.1
  • development/2.2
  • development/2.3
  • development/2.4
  • development/2.5
  • development/2.6
  • development/2.7

There is no action required on your side. You will be notified here once
the changeset has been merged. In the unlikely event that the changeset
fails permanently on the queue, a member of the admin team will
contact you to help resolve the matter.

IMPORTANT

Please do not attempt to modify this pull request.

  • Any commit you add on the source branch will trigger a new cycle after the
    current queue is merged.
  • Any commit you add on one of the integration branches will be lost.

If you need this pull request to be removed from the queue, please contact a
member of the admin team now.

The following options are set: approve

@bert-e
Copy link
Contributor

bert-e commented Feb 5, 2021

I have successfully merged the changeset of this pull request
into targetted development branches:

  • ✔️ development/2.8

The following branches have NOT changed:

  • development/1.0
  • development/1.1
  • development/1.2
  • development/1.3
  • development/2.0
  • development/2.1
  • development/2.2
  • development/2.3
  • development/2.4
  • development/2.5
  • development/2.6
  • development/2.7

Please check the status of the associated issue None.

Goodbye gdemonet.

@bert-e bert-e merged commit 7fd39cf into development/2.8 Feb 5, 2021
@bert-e bert-e deleted the improvement/pre-check-default-route branch February 5, 2021 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:enhancement New feature or request topic:deployment Bugs in or enhancements to deployment stages topic:networking Networking-related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants