-
Notifications
You must be signed in to change notification settings - Fork 93
✨ Support upstream hcloud-ccm. ProviderID changes to hrobot://NNNNN
#1703
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: guettli <[email protected]>
|
|
||
| // upstream hcloud-ccm uses the secret key "token", while the old Syself ccm used "hcloud". | ||
| // For compatibilty, we create always the other key, too. | ||
| for _, key := range []string{"token", "hcloud"} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right now we create a duplicate right? Wouldn't it be enough to create the right key (e.g. "token" for the upstream ccm)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The result should work for both CCMs.
By creating both keys we are safe. Users can try to upstream hcloud-ccm. If it fails, they can easily switch back to the old syself-ccm.
Why not create both keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you create both keys in both secrets. But you actually need only one key per secret right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it is less confusing for users if we have only the correct key in the secret. That's why I would implement it in that way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: We deprecate HetznerCluster.Spec.HetznerSecret.Key.HCloudToken. In the docstring we explain that the controller will currently create "token", "hcloud" and the specified value.
In the long run (maybe caph version released at the end of 2027) we will remove that field and will only create "token".
I think it is perfectly fine if that value can't be configured.
What do you think about that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose so. Can you discuss this with @batistein?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janiskemper I talked to Sven. He is fine with creating three keys in the secret. But he does not want to deprecate this setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay can you then update the comments in the function to make that clear that these three keys are created for the hcloud token?
EDIT: And this should be also documented I suppose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janiskemper I added a comment to the function and to the related field in the struct:
ok now?
…d-format-of-upstream-ccm
| type HetznerSecretKeyRef struct { | ||
| // HCloudToken defines the name of the key where the token for the Hetzner Cloud API is stored. | ||
| // We recommend to use "token", because this is the default of upstream hcloud-ccm. | ||
| // hcloudToken defines the name of the key where the token for the Hetzner Cloud API is stored. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
capital "HCloudToken"
| // We recommend to use "token", because this is the default of upstream hcloud-ccm. | ||
| // hcloudToken defines the name of the key where the token for the Hetzner Cloud API is stored. | ||
| // We recommend to use "token", because this is the default of upstream hcloud-ccm, while the | ||
| // legacy Syself ccm uses "hcloud". For maximal compatibility up to three keys get created in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's write "Syself fork" instead of "Syself ccm"
This PR adds support for the upstream hcloud CCM (https://github.com/hetznercloud/hcloud-cloud-controller-manager/).
Until now, CAPH relied on our Syself fork, the "hetzner" CCM (https://github.com/syself/hetzner-cloud-controller-manager/).
The fork was required several years ago because the upstream CCM did not support bare-metal servers. Hetzner has integrated our bare metal support into its CCM (hcloud ccm PR 561), so now is the time to gradually phase out our fork.
The CCM is responsible for setting the ProviderID on Nodes. Our fork used the format
hcloud://bm-NNNNfor bare-metal nodes, while the upstream CCM useshrobot://NNNN.This PR makes both formats work, so you can use either the Syself CCM or the upstream hcloud CCM from hetznercloud.
If you use caph containing this PR, then it is safe to switch to the upstream ccm. Existing nodes will keep their legacy
hcloud://bm-NNNNProviderIDs. Only new nodes will gethrobot://NNNN. The upstream ccm can read the legacy format.HCloud nodes are not affected by this change.
Changes
hrobot://NNNN. Before it washcloud://bm-NNNN.noteto the secret in the wl-cluster. This helps new users to understand that this secret gets reconciled by caph.hcloud://bm-NNNN) and the new ProviderID (hrobot://NNNN)reconcileTargetSecrettoreconcileWorkloadClusterSecretsand rename some internal variables to make differentiation between mgt-cluster and wl-cluster more obvious.TODO Release as caph v1.1.0
This changes a lot, additionally the last caph release was not done from main. This should be released as v1.1.0
This version get used already used in the docs. Please adapt, if a different version will contain that PR.