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

bootstrapper: only err if no control plane IPs available #3496

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

msanft
Copy link
Contributor

@msanft msanft commented Nov 26, 2024

Context

Previously we errored out of the entire join if retrieval of either LB IP or control plane public IP failed. This resulted in the entire "use either IP" logic not working as intended.

This also fixes the problem encountered by STACKIT folks, where public IP retrieval fails and thus no nodes can join the cluster.

Proposed change(s)

  • Log a warning only if the IP retrievals fail, and only errors out of the join if no IP can be found at all.

Checklist

  • Add labels (e.g., for changelog category)
  • Is PR title adequate for changelog?
  • Link to Milestone

Previously we errored out of the entire join if retrieval
of either LB IP or control plane public IP failed. This resulted
in the entire "use either IP" logic not working as intended. This now
makes it log a warning only if the IP retrievals fail, and only errors
out of the join if no IP can be found at all.
@msanft msanft added the bug fix Fixing a bug label Nov 26, 2024
@msanft msanft requested a review from 3u13r as a code owner November 26, 2024 08:59
Copy link

netlify bot commented Nov 26, 2024

Deploy Preview for constellation-docs canceled.

Name Link
🔨 Latest commit c7d88d8
🔍 Latest deploy log https://app.netlify.com/sites/constellation-docs/deploys/67458e077f544e0008067055

return nil, nil, fmt.Errorf("failed to get load balancer endpoint: %w", err)
c.log.Warn("Failed to get load balancer endpoint", "err", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I understand we could leave this block unchanged, because joining via node IP does not work anyway due to node-to-node strict mode. Otoh, this way it's nicely symmetric.

Copy link
Contributor

Coverage report

Package Old New Trend
bootstrapper/internal/joinclient 87.00% 87.00% ↔️

@msanft msanft merged commit 52372ae into main Nov 26, 2024
10 checks passed
@msanft msanft deleted the msant/bootstrapper-join-logic branch November 26, 2024 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix Fixing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants