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

Update cloud credentials even for unwanted machine deployments. #3224

Conversation

vpnachev
Copy link
Member

@vpnachev vpnachev commented Nov 25, 2020

How to categorize this PR?

/area robustness
/kind bug
/priority normal

What this PR does / why we need it:
The worker controller will now update the cloud credentials also for machine deployments that are not referenced by the worker resource, e.g. they are in deletion.

Which issue(s) this PR fixes:
Part of #3023

Special notes for your reviewer:

Release note:

The generic worker actuator is now ensuring that all machine class secrets have up-to-date cloud credentials.
The `WorkerDelegate` must implement method `GetMachineControllerManagerCloudCredentials` returning map with cloud credential keys and values just like they are used by the machine-controller-manager.

@vpnachev vpnachev requested a review from a team as a code owner November 25, 2020 12:23
@gardener-robot gardener-robot added area/robustness Robustness, reliability, resilience related kind/bug Bug priority/normal size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 25, 2020
@vpnachev
Copy link
Member Author

A sample implementation in a provider extension looks like gardener/gardener-extension-provider-aws#223

@rfranzke rfranzke self-assigned this Nov 25, 2020
Copy link
Member

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

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

Wouldn't a more "lightweight" and cleaner approach be if we split the DeployMachineClasses function into DeployMachineClassSecrets and DeployMachineClasses. Then DeployMachineClassSecrets can always crupdate all machine class secrets, including those still in the system but meanwhile potentially unwanted, and DeployMachineClasses can handle the machine classes without crupdating the secrets. This is more adaptation effort in the extensions, but probably has a cleaner flow and a cleaner WorkerDelegate interface. What is your opinion?

@vpnachev
Copy link
Member Author

Actually, I wanted to move the cloud credential into dedicated single secret and mount it into the machine-controller-manager deployment, just how it is done for the cloud-controller-manager, however MCM does not support such option at the moment.
At the moment, I do not see the value of splitting DeployMachineClasses into two separate functions.

I changed the PR to patch all machine class secrets, for those that are up-to-date, this should be an empty patch.

@rfranzke
Copy link
Member

In a discussion with @vpnachev and @timuthy we talked about an option for easier handling in the future: If the machine classes would support to read the infrastructure credentials from a different secret than the user-data then we don't need all this special logic. We could just reuse the already existing cloudprovider secret.

The MCM maintainers @hardikdr @prashanth26 @AxiomSamarth wanted to discuss internally.
/hold until the colleagues came back

Assuming that we agree on this, we'd like to merge this PR as is, despite the fact that it "uglyfies" the WorkerDelegate interface a little bit. However, it would be only a temporary step until MCM supports the single secret, and it results in only minimal changes in the actual provider extensions.

/assign @vpnachev @timuthy @hardikdr @prashanth26 @AxiomSamarth

@rfranzke
Copy link
Member

The MCM maintainers accepted the proposal, so we can
/unhold
this PR and proceed with it until the discussed enhancement (gardener/machine-controller-manager#578) is merged and available for us and the provider extensions to use.

@timuthy
Copy link
Member

timuthy commented Nov 30, 2020

Frankly, I didn't expect an implementation in MCM this quick (thanks @rfranzke 🚀). Does it then still even make sense to proceed with this PR instead of waiting for the next MCM release?

@rfranzke
Copy link
Member

For backwards compatibility reasons we should proceed, I think. Some provider extension might want/need to stick to a certain MCM version for whatever reasons, so let's get this fix in or for a couple of releases. WDYT?

@timuthy
Copy link
Member

timuthy commented Nov 30, 2020

For backwards compatibility reasons we should proceed, I think. Some provider extension might want/need to stick to a certain MCM version for whatever reasons, so let's get this fix in or for a couple of releases. WDYT?

Okay, makes sense. Thank you!

Copy link
Member

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

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

/lgtm

@rfranzke rfranzke merged commit 3b9daf1 into gardener:master Nov 30, 2020
@vpnachev vpnachev deleted the worker/update-credentials-for-all-deployments branch November 30, 2020 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/robustness Robustness, reliability, resilience related kind/bug Bug size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants