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

CUSTCOM-12 Fixed node configuration in admin UI #4548

Merged
merged 7 commits into from
Mar 24, 2020
Merged

CUSTCOM-12 Fixed node configuration in admin UI #4548

merged 7 commits into from
Mar 24, 2020

Conversation

dmatej
Copy link
Contributor

@dmatej dmatej commented Mar 4, 2020

Description

This is a bug fix for incorrect GUI behavior when configuring Nodes, especially SSH.

  • fixed logging of errors (did not log anything in some types of failures)

  • fixed distribution of error messages to the user (I still don't like it's formatting, but that's not a thing of this issue)

  • fixed updating SSH Node connector when moving from password to keyfile and vice-versa

  • validation before the update ends with verification that the keyfile exists. It does not test it's content. Originally it merged defaults, existing configuration and new configuration, so finally it ended with several kinds of exceptions (some of them were swallowed, so GUI showed only "See server.log", but there was nothing).

Important Info

Testing

New tests

  • no new automatic tests

Testing Performed

Manual

  • manual testing GUI, looking on changes in server.log and domain.xml
  • create SSH Node, update SSH configuration, ping
  • create DCOM node, update it's configuration, ping
asadmin create-node-dcom --force=true --nodehost=nodehost123 --passwordfile=./passwordfile-windows.txt  cmd-dcom001
Warning: some parameters appear to be invalid.
Unknown host nodehost123
Continuing with node creation due to use of --force.
Command create-node-dcom executed successfully.

asadmin create-node-ssh --force=true --nodehost=localhost1 cmd-ssh001
Warning: some parameters appear to be invalid.
Unknown host localhost1
Continuing with node creation due to use of --force.
Command create-node-ssh executed successfully.

asadmin create-node-ssh --force=true --nodehost=localhost --sshkeyfile=./sss cmd-ssh002
Warning: some parameters appear to be invalid.
Key file path ./sss must be an absolute path.
Continuing with node creation due to use of --force.
Command create-node-ssh executed successfully.

asadmin create-node-ssh --force=true --nodehost=localhost --sshkeyfile=/app/appservers/passwords/payara-test-id_rsa cmd-ssh002
remote failure: Configuration not added. A Node instance with a "cmd-ssh002" name already exists in the configuration
Command create-node-ssh failed.

asadmin create-node-ssh --force=true --nodehost=localhost --sshkeyfile=/app/appservers/passwords/payara-test-id_rsa cmd-ssh003
Command create-node-ssh executed successfully.

asadmin update-node-ssh --force=true --nodehost=localhost --sshport=32000 cmd-ssh003
Command update-node-ssh executed successfully.

asadmin update-node-ssh --force=true --nodehost=localhost --sshport=22 --sshkeyfile=/app/appservers/passwords/payara-test-id_rsad cmd-ssh003
Warning: some parameters appear to be invalid.
Key file /app/appservers/passwords/payara-test-id_rsad not found. The key file path must be reachable by the DAS.
Continuing with node update due to use of --force.
Command update-node-ssh executed successfully.

asadmin update-node-ssh --force=true --nodehost=localhost --sshport=22 --sshkeyfile=/app/appservers/passwords/payara-test-id_rsa cmd-ssh003
Command update-node-ssh executed successfully.

sadmin update-node-ssh --force=true --nodehost=localhost --sshport=22 --sshkeyfile=/app/appservers/passwords/payara-test-id_rsa --sshuser=payara-test cmd-ssh003
Command update-node-ssh executed successfully.

Test suites executed

  • Quicklook - NO
  • Payara Samples - OK
  • Java EE7 Samples - NO
  • Java EE8 Samples - NO
  • Payara Private Tests - NO
  • Payara Microprofile TCKs Runner - NO
  • Jakarta TCKs - NO
  • Mojarra - NO
  • Cargo Tracker - NO

@dmatej
Copy link
Contributor Author

dmatej commented Mar 4, 2020

Jenkins test please

@dmatej dmatej requested review from Pandrex247 and MattGill98 March 4, 2020 12:06
@dmatej dmatej self-assigned this Mar 4, 2020
@dmatej dmatej requested a review from pdudits March 4, 2020 14:22
@dmatej dmatej changed the title CUSTCOM-12 Fixed node gui CUSTCOM-12 Fixed node configuration in admin UI Mar 4, 2020
Copy link
Member

@Pandrex247 Pandrex247 left a comment

Choose a reason for hiding this comment

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

fixed updating SSH Node connector when moving from password to keyfile and vice-versa

I'm not finding this to be true.
If I enter a password and then try to change it to key file and hit save, when I next go back to the node config it's still showing password.

@dmatej
Copy link
Contributor Author

dmatej commented Mar 18, 2020

I deleted my last comment, because when I was testing now, I forgot to press Save button :D
On the seccond attempt, both in this and second branch where these commits are, it works.
Can you record a video to see what you did? Or try it again, at least?
Btw I tested it even with existing but invalid file (public key)

@dmatej
Copy link
Contributor Author

dmatej commented Mar 18, 2020

But I was able to break it again with know exception with unknown cause ... I think it should be in separate issue and at this moment I don't understand what exactly causes it. (unresolved log message is targeted under CUSTCOM-55).
It broke when I changed user from payara-test to dmatej and keyfile from to ${user.home}/.ssh/id_rsa . Restart helped. It seems any error while saving breaks the domain. Why?

[#|2020-03-18T13:49:34.717+0100|WARNING|Payara 5.202|javax.enterprise.system.core|_ThreadID=185;_ThreadName=admin-thread-pool::admin-listener(2);_TimeMillis=1584535774717;_LevelValue=900;|
  failed.to.update.node ssh003
org.jvnet.hk2.config.TransactionFailure: This service has been unbound: SystemDescriptor(
        implementation=org.glassfish.config.support.GlassFishConfigBean
        name=com.sun.enterprise.config.serverbeans.SshAuth
        contracts={com.sun.enterprise.config.serverbeans.SshAuth,org.jvnet.hk2.config.ConfigBean,org.jvnet.hk2.config.ConfigBeanProxy}
        scope=javax.inject.Singleton
        qualifiers={javax.inject.Named}
        descriptorType=CLASS
        descriptorVisibility=NORMAL
        metadata=
        rank=0
        loader=org.jvnet.hk2.config.ConfigModel$SafeHk2Loader@2d658712
        proxiable=null
        proxyForSameScope=null
        analysisName=null
        id=1700
        locatorId=0
        identityHashCode=1554410178
        reified=true)
        at org.jvnet.hk2.config.WriteableView.commit(WriteableView.java:408)
        at org.jvnet.hk2.config.Transaction.commit(Transaction.java:114)
        at org.jvnet.hk2.config.ConfigSupport._apply(ConfigSupport.java:182)
        at org.jvnet.hk2.config.ConfigSupport.apply(ConfigSupport.java:139)
        at org.jvnet.hk2.config.ConfigSupport.apply(ConfigSupport.java:118)
        at com.sun.enterprise.v3.admin.cluster.UpdateNodeCommand.updateNodeElement(UpdateNodeCommand.java:222)

@dmatej
Copy link
Contributor Author

dmatej commented Mar 18, 2020

I cannot reproduce it any more after the rebase to current master

@dmatej
Copy link
Contributor Author

dmatej commented Mar 18, 2020

Jenkins test please

@dmatej dmatej requested a review from Pandrex247 March 18, 2020 14:27
@dmatej
Copy link
Contributor Author

dmatej commented Mar 18, 2020

Jenkins test please

Copy link
Member

@Pandrex247 Pandrex247 left a comment

Choose a reason for hiding this comment

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

Seems to build now, but I'm still finding that changing from using password to key file doesn't seem to take effect?

Moment I hit save it switches back to password. I'm unfortunately not in a position to test if that's just a visual thing or not atm.

From a clean domain:

  • Open admin console
  • New node:
    • name = Noddy1
    • host = localhost
    • auth = Password
    • password = passy
  • Save
  • Edit Noddy1
    • auth = Key File
  • Save

@dmatej
Copy link
Contributor Author

dmatej commented Mar 18, 2020

Aha, that's because you did not provide the key file. The problem is that empty value cannot be replaced by default, because it would go against asadmin command which does not change what is not set (reason why it originally did not work at all).
Maybe if it would be resolved in UI layer somehow ...

@dmatej
Copy link
Contributor Author

dmatej commented Mar 19, 2020

Jenkins test please

@dmatej dmatej requested a review from Pandrex247 March 19, 2020 17:20
@Pandrex247
Copy link
Member

Admin GUI seems to work nicely now.

It might be worth hiding the new --sshauthtype parameter for the asadmin commands, since it's not really usable from the command line - you aren't allowed to specify a password or alias from the command line so essentially all the parameter does is wipe your config 😂

Speaking of which, I see that it clears the key and password config when you switch between them.
Just double-checking - this isn't going to trip anyone up is it? To me it makes sense but I don't know if there's any sort of "fallback" behaviour typically expected from SSH.

@dmatej
Copy link
Contributor Author

dmatej commented Mar 20, 2020

Admin GUI seems to work nicely now.

It might be worth hiding the new --sshauthtype parameter for the asadmin commands, since it's not really usable from the command line - you aren't allowed to specify a password or alias from the command line so essentially all the parameter does is wipe your config joy

I experimented with it, and I even found one use case where it was useful - again - to reconfigure authentication to use defaults keyfile. Otherwise you cannot send null via command line, it is an empty string and it will be saved to domain.xml (and the behavior is same then).
So it might be useful.

Same with password, you can set password, but you have to provide passwordfile. If not, it will use user's private key as a default.

Seems it would be possible to use even passphrase again, but that goes behind borders of this issue.

Speaking of which, I see that it clears the key and password config when you switch between them.
Just double-checking - this isn't going to trip anyone up is it? To me it makes sense but I don't know if there's any sort of "fallback" behaviour typically expected from SSH.

By default those values are clean => it uses system defaults (or trilead defaults? I'm not sure, but it tried to use my private key and in domain.xml was nothing).

Example (created node in description of this PR, I updated only the hostname):

    <node name="cmd-ssh001" install-dir="${com.sun.aas.productRoot}" type="SSH" node-host="localhost">
      <ssh-connector>
        <ssh-auth></ssh-auth>
      </ssh-connector>
    </node>

And "ping" result:
Failed to validate SSH connection to node cmd-ssh001 (localhost) Could not connect to host localhost using SSH. Could not authenticate. SSH authentication with key file /home/dmatej/.ssh/id_rsa failed: PEM problem: it is of unknown type

  • the reason is that as expected, my private key has passphrase, so Trilead cannot detect even type of the key.

@dmatej dmatej requested a review from Pandrex247 March 20, 2020 13:09
@dmatej
Copy link
Contributor Author

dmatej commented Mar 20, 2020

Jenkins test please

@Pandrex247
Copy link
Member

So it might be useful.
Ok, but we could still probably do with some improvements here - it provides no command-line help on what the acceptable values are and they're case-sensitive.

Param.acceptableValues should help with the former if you didn't know about it.

David Matejcek added 6 commits March 21, 2020 20:38
- removed "random" settings of psSelected/winPsSelected
- removed configuration of psSelect from convertNodePswd
- created presetNodeAuthSelectBox
- enabled autocomplete, without it was configuration hostile except password,
  which were still remembered by Firefox
- fixed nodeButtons.inc - password/keystore gui
- fixed UpdateNodeCommand
  - password/keystore gui, old version used previous version
  - reimplemented validation
- renamed PARAM_REMOTEPASSWORD to PARAM_SSHPASSWORD
- ParameterMap - implemented additional methods for better readibility
- report classes have toString now (useful for debugging and logging)
- GuiUtil - reduced copy and paste and using sane filtering
- RestUtil2 + RestApiHandlers
  - fixed processing error messages using new method in GuiUtil
  - expecting JSON, but it will not fail if the response would be unparseable,
    it would only print error to log.
- created enum SshAuthType (KEY/PASSWORD) to distinguish which variant should
  be saved if we don't set neither password nor keyfile
- new parameter sshauthtype to allow switching with defaults
@dmatej
Copy link
Contributor Author

dmatej commented Mar 23, 2020

Jenkins test please

@dmatej dmatej added the PR: DO NOT MERGE Don't merge PR until further notice label Mar 23, 2020
- seems that when we are updating sshauth element, setSshConnector must be
  already called
@dmatej dmatej removed the PR: DO NOT MERGE Don't merge PR until further notice label Mar 23, 2020
@dmatej
Copy link
Contributor Author

dmatej commented Mar 23, 2020

Jenkins test please

Copy link
Member

@Pandrex247 Pandrex247 left a comment

Choose a reason for hiding this comment

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

👌🥳

@dmatej dmatej merged commit 52d9c93 into payara:master Mar 24, 2020
@dmatej dmatej deleted the CUSTCOM-12-fixed-node-gui branch March 24, 2020 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants