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

ssh when sshAccess disable #331

Closed
wants to merge 1 commit into from
Closed

Conversation

tedteng
Copy link
Contributor

@tedteng tedteng commented Aug 17, 2023

What this PR does / why we need it:
for issue, #325, however, check with MCM colleagues, don't find the solution to Enable/disable sshd service on a specific node. it seems the only way is enabling sshaccess then generate ssh key and enable sshd service to affect all the worker nodes.

the solution here is

  1. to patch shoot workersSettings.sshAccess when shoot workersSettings.sshAccess is false,
  2. waiting for ssh key to generate
  3. then create a bastion with the annotation ["WorkerSSHD"] = false , to keep the original shoot workersSettings.sshAccess value , usually value is false
    will add extra logic in delete phase in the bastion extension repo individually and verify it. in delete phase will handle revert back code to patch shoot workersSettings.sshAccess back to original value false when detecting annotation ["WorkerSSHD"] = false in case any issue gardenctl break or keep bastion case

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

gardenctl ssh worker node when shoot disable sshAccess 

@gardener-robot gardener-robot added needs/review Needs review size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Aug 17, 2023
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Aug 18, 2023
@petersutter
Copy link
Member

Hi @tedteng, I understand that the PR is in draft, however I think this is heading in the wrong direction.

You suggested patching the shoot setting with gardenctl and then reverting it back to its previous value using gardenctl once a signal is received. However, if the ssh command is run multiple times for the same shoot, an incorrect value could be restored.

You also proposed setting an annotation on the bastion to store the "initial" enabled state. This would potentially allow the extension to patch the shoot to restore this value if it was not restored by gardenctl for any reason. However, this approach has the same issue: it's unclear what the real initial value is if the command was run multiple times.

Additionally, this approach would require another controller (the bastion extension controller) to patch the shoot resource, but does the user even have the permission to change this setting on the shoot?

I do not think it is a good idea to try to solve / workaround above mention issues. Lets first agree upon a design in #325 before investing any further time in this PR.

Also, if ssh access is disabled, do not silently enable it. The user/operator should be notified and asked if it should be enabled and there should be a flag to bypass the need for confirmation, similar to the confirm-access-restriction flag.

@tedteng
Copy link
Contributor Author

tedteng commented Aug 20, 2023

@petersutter welcome any feedback for the solution discussion. I'm also verifying and looking for a solution.

You suggested patching the shoot setting with gardenctl and then reverting it back to its previous value using gardenctl once a signal is received. However, if the ssh command is run multiple times for the same shoot, an incorrect value could be restored.
You also proposed setting an annotation on the bastion to store the "initial" enabled state. This would potentially allow the extension to patch the shoot to restore this value if it was not restored by gardenctl for any reason. However, this approach has the same issue: it's unclear what the real initial value is if the command was run multiple times.

it should only have a value stored once. user A executed the gardenctl , then shoot patch to true. start from this pointer, the next user will ssh directly due to workersSettings.sshAccess being true already, there is no difference like before using ssh.

From the bastion object pointer, it should only have one bastion object with annotation to store the initial value (["WorkerSSHD"] = "false") at the beginning when user A executed the gardenctl and shoot ``workersSettings.sshAccessisfalse`.

The next/new user will create a new bastion object same as the normal ssh process (["WorkerSSHD"] = "true") because workersSettings.sshAccess is true already. then only the first bastion with ["WorkerSSHD"] = "false" annotation, will process the shoot reverting process when bastion object delete phase triggered by gardenctl or keep-life end

The purpose for reverting it back to its previous value using gardenctl once a signal is received is only in case the user cancel(Ctrl+C) the session during the shoot workersSettings.SSHAccess.Enabled patch before bastion created at that time. just for double safety, I involved another controller (the bastion extension controller) to revert the value back during the bastion delete phase. in this way always make sure the value is revetted back the initial value.

Additionally, this approach would require another controller (the bastion extension controller) to patch the shoot resource, but does the user even have permission to change this setting on the shoot?

I have the same concern too and will experiment with it.

Also, if ssh access is disabled, do not silently enable it. The user/operator should be notified and asked if it should be enabled and there should be a flag to bypass the need for confirmation, similar to the confirm-access-restriction flag

sure, I also want to have a similar flag or reminder when executed gardenctl

Lets first agree upon a design in #325 before investing any further time in this PR.

agree

@tedteng
Copy link
Contributor Author

tedteng commented Sep 13, 2023

close this PR based on the discussion the new feature might be implemented in g/g side instead of triggering from gardenctl

@tedteng tedteng closed this Sep 13, 2023
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/review Needs review size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants