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

feat(routes): emit warning if cluster cidr is misconfigured #793

Merged
merged 3 commits into from
Nov 19, 2024

Conversation

lukasmetzner
Copy link
Contributor

Before adding a route, verify if the route CIDR is contained within the configured cluster CIDR. If it is not, emit a warning event and log message.

Copy link

codecov bot commented Nov 18, 2024

Codecov Report

Attention: Patch coverage is 90.69767% with 4 lines in your changes missing coverage. Please review.

Project coverage is 70.71%. Comparing base (674696f) to head (0843932).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
hcloud/routes.go 91.17% 2 Missing and 1 partial ⚠️
main.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #793      +/-   ##
==========================================
+ Coverage   70.45%   70.71%   +0.26%     
==========================================
  Files          31       31              
  Lines        3226     3265      +39     
==========================================
+ Hits         2273     2309      +36     
- Misses        779      781       +2     
- Partials      174      175       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Copy link
Member

@apricote apricote left a comment

Choose a reason for hiding this comment

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

Could you split up this PR into one that refactors the init code and the second one that emits the warning?

@lukasmetzner lukasmetzner force-pushed the emit-warning-misconfigured-cluster-cidr branch from dcebddc to 07518c9 Compare November 19, 2024 08:26
@lukasmetzner
Copy link
Contributor Author

@apricote

Could you split up this PR into one that refactors the init code and the second one that emits the warning?

Created #794 for the refactoring. Afterward, we could rebase from main and merge this one, as we already referenced this PR in some answers.

Copy link
Member

@apricote apricote left a comment

Choose a reason for hiding this comment

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

One small change + rebase

hcloud/routes.go Outdated Show resolved Hide resolved
… to be kept

Check if route CIDR is contained in cluster CIDR before adding the route and return an error if it is not contained.
Default value is only used in tests and not in the application code.
@lukasmetzner lukasmetzner force-pushed the emit-warning-misconfigured-cluster-cidr branch from 07518c9 to 0843932 Compare November 19, 2024 11:27
@lukasmetzner lukasmetzner merged commit 81cc8b2 into main Nov 19, 2024
10 checks passed
@lukasmetzner lukasmetzner deleted the emit-warning-misconfigured-cluster-cidr branch November 19, 2024 14:29
This was referenced Nov 19, 2024
lukasmetzner pushed a commit that referenced this pull request Nov 26, 2024
<!-- section-start changelog -->
### Feature Highlights &amp; Upgrade Notes

#### Load Balancer IPs set to Private IPs

If networking support is enabled, the load balancer IPs are now
populated with the private IPs, unless the
`load-balancer.hetzner.cloud/disable-private-ingress` annotation is set
to `true`. Please make sure that you configured the annotation according
to your needs, for example if you are using `external-dns`.

#### Provided-By Label

We introduced a the label `instance.hetzner.cloud/provided-by`, which
will be automatically added to all **new** nodes. This label can have
the values `cloud` or `robot` to distinguish between our products. We
use this label in the csi-driver to ensure the daemonset is only running
on cloud nodes. We recommend to add this label to your existing nodes
with the appropriate value.

- `kubectl label node $CLOUD_NODE_NAME
instance.hetzner.cloud/provided-by=cloud`
- `kubectl label node $ROBOT_NODE_NAME
instance.hetzner.cloud/provided-by=robot`

#### Load Balancer IPMode Proxy

Kubernetes KEP-1860 added a new field to the Load Balancer Service
Status that allows us to mark if the IP address we add should be
considered as a Proxy (always send traffic here) and VIP (allow
optimization by keeping the traffic in the cluster).

Previously Kubernetes considered all IPs as VIP, which caused issues
when when the PROXY protocol was in use. We have previously recommended
to use the annotation `load-balancer.hetzner.cloud/hostname` to
workaround this problem.

We now set the new field to `Proxy` if the PROXY protocol is active so
the issue should no longer appear. If you only added the
`load-balancer.hetzner.cloud/hostname` annotation for this problem, you
can remove it after upgrading.

Further information:

- kubernetes/enhancements#1860
-
#160 (comment)

### Features

- **service**: Specify private ip for loadbalancer (#724)
- add support &amp; tests for Kubernetes 1.31 (#747)
- **helm**: allow setting extra pod volumes via chart values  (#744)
- **instance**: add label to distinguish servers from Cloud and Robot
(#764)
- emit event when robot server name and node name mismatch (#773)
- **load-balancer**: Set IPMode to &#34;Proxy&#34; if load balancer is
configured to use proxy protocol (#727) (#783)
- **routes**: emit warning if cluster cidr is misconfigured (#793)
- **load-balancer**: ignore nodes that don&#39;t use known provider IDs
(#780)
- drop tests for kubernetes v1.27 and v1.28

### Bug Fixes

- populate ingress private ip when disable-private-ingress is false
(#715)
- wrong version logged on startup (#729)
- invalid characters in label instance-type of robot servers (#770)
- no events are emitted as broadcaster has no sink configured (#774)

### Kubernetes Support

This version was tested with Kubernetes 1.29 - 1.31. Furthermore, we
dropped v1.27 and v1.28 support.

<!-- section-end changelog -->

---

<details>
<summary><h4>PR by <a
href="https://github.com/apricote/releaser-pleaser">releaser-pleaser</a>
🤖</h4></summary>

If you want to modify the proposed release, add you overrides here. You
can learn more about the options in the docs.

## Release Notes

### Prefix / Start

This will be added to the start of the release notes.

```rp-prefix
### Feature Highlights & Upgrade Notes

#### Load Balancer IPs set to Private IPs

If networking support is enabled, the load balancer IPs are now populated with the private IPs, unless the `load-balancer.hetzner.cloud/disable-private-ingress` annotation is set to `true`. Please make sure that you configured the annotation according to your needs, for example if you are using `external-dns`.

#### Provided-By Label

We introduced a the label `instance.hetzner.cloud/provided-by`, which will be automatically added to all **new** nodes. This label can have the values `cloud` or `robot` to distinguish between our products. We use this label in the csi-driver to ensure the daemonset is only running on cloud nodes. We recommend to add this label to your existing nodes with the appropriate value.

- `kubectl label node $CLOUD_NODE_NAME instance.hetzner.cloud/provided-by=cloud`
- `kubectl label node $ROBOT_NODE_NAME instance.hetzner.cloud/provided-by=robot`

#### Load Balancer IPMode Proxy

Kubernetes KEP-1860 added a new field to the Load Balancer Service Status that allows us to mark if the IP address we add should be considered as a Proxy (always send traffic here) and VIP (allow optimization by keeping the traffic in the cluster).

Previously Kubernetes considered all IPs as VIP, which caused issues when when the PROXY protocol was in use. We have previously recommended to use the annotation `load-balancer.hetzner.cloud/hostname` to workaround this problem.

We now set the new field to `Proxy` if the PROXY protocol is active so the issue should no longer appear. If you  only added the `load-balancer.hetzner.cloud/hostname` annotation for this problem, you can remove it after upgrading.

Further information:
- kubernetes/enhancements#1860
- #160 (comment)
```

### Suffix / End

This will be added to the end of the release notes.

```rp-suffix
### Kubernetes Support

This version was tested with Kubernetes 1.29 - 1.31. Furthermore, we dropped v1.27 and v1.28 support.
```

</details>

Co-authored-by: releaser-pleaser <>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants