You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by ygy <gi...@git.apache.org> on 2017/03/13 15:13:11 UTC
[GitHub] brooklyn-server pull request #591: Fix ssh login when provider's image requi...
GitHub user ygy opened a pull request:
https://github.com/apache/brooklyn-server/pull/591
Fix ssh login when provider's image requires TTY
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ygy/brooklyn-server fix/ssh-login
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/591.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #591
----
commit 855fe8930f28accf416df0b434cc82a32b7f1eb2
Author: Yavor Yanchev <ya...@yanchev.com>
Date: 2017-03-13T15:11:16Z
Fix ssh login when provider's image requires TTY
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #591: Fix ssh login when provider's image requires TTY
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on the issue:
https://github.com/apache/brooklyn-server/pull/591
I wanted to check whether it is tested against CentOS 6.
Thank you for inspecting that!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #591: Fix ssh login when provider's image requires TTY
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/591
LGTM.
In jclouds, it always sets allocatePTY when using sshj: https://github.com/jclouds/jclouds/blob/rel/jclouds-2.0.0/drivers/sshj/src/main/java/org/jclouds/sshj/SshjSshClient.java#L438
In contrast, brooklyn only sets it if configured to do so: https://github.com/apache/brooklyn-server/blob/rel/apache-brooklyn-0.10.0/core/src/main/java/org/apache/brooklyn/util/core/internal/ssh/sshj/SshjTool.java#L837-L839
Brooklyn defaults this to not allocatePTY: https://github.com/apache/brooklyn-server/blob/rel/apache-brooklyn-0.10.0/core/src/main/java/org/apache/brooklyn/util/core/internal/ssh/SshTool.java#L75
Brooklyn will then by default execute a command to disable requiretty: https://github.com/apache/brooklyn-server/blob/rel/apache-brooklyn-0.10.0/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java#L904-L915
That behaviour is configurable via https://github.com/apache/brooklyn-server/blob/rel/apache-brooklyn-0.10.0/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocationConfig.java#L179-L188
Making this change just means we do the same as jclouds already does. This change will only affect us if we've set `useJcloudsSshInit: false`, and then only for the initial createUser command execution.
We have much more thoroughly tested the jclouds behaviour across many clouds, because `useJcloudsSshInit` defaults to true.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #591: Fix ssh login when provider's image requires TTY
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on the issue:
https://github.com/apache/brooklyn-server/pull/591
Is it possible that `PROP_ALLOCATE_PTY` breaks for Centos 6 images?
Is it tested on other clouds. I thing it should be optional.
I have to suggestions for implementing it:
- Move DONT_REQUIRE_TTY_FOR_SUDO check above. Unfortunatelly this may require rearanging of wait for reachable as well.
- Make `SshMachineLocation.SSH_CONFIG_GIVEN_TO_PROPS` configurable or make that parameter configurable.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #591: Fix ssh login when provider's image requires TTY
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/591
@bostko I've tested this on aws and softlayer, on centos 6 & 7, and ubuntu 16, 14, 12 and 10. I'll add some more live tests for those in a separate PR later.
I don't think we should rely on `DONT_REQUIRE_TTY_FOR_SUDO`: that controls whether we execute an extra command so that we disable requiretty in the `/etc/sudoers` file! It might be set to false. Note that the extra command also sets `SshTool.PROP_ALLOCATE_PTY` to true (as @ygy is doing here).
`SshMachineLocation.SSH_CONFIG_GIVEN_TO_PROPS` feels overkill here. This is an unusual use-case: it only affects the arg passed to the single createUser command, and then only if one has set `useJcloudsSshInit: false`. I think that a user would sensibly expect a config option like `SSH_CONFIG_GIVEN_TO_PROPS` to be used more widely. It's unclear to me where else we'd want to use it, given one can pass through other config for the SshMachineLocation to use already.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #591: Fix ssh login when provider's image requires TTY
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/591
@bostko is there a specific reason you think it would break CentOS 6 deployments? Or just that you wanted to ensure it had been tested before this was merged?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #591: Fix ssh login when provider's image requires TTY
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on the issue:
https://github.com/apache/brooklyn-server/pull/591
I would move tty disable flag after the createUser statement.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #591: Fix ssh login when provider's image requi...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/591
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---