-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Scala based admin tooling #3722
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3722 +/- ##
==========================================
+ Coverage 75.13% 75.16% +0.03%
==========================================
Files 132 136 +4
Lines 6140 6378 +238
Branches 376 392 +16
==========================================
+ Hits 4613 4794 +181
- Misses 1527 1584 +57
Continue to review full report at Codecov.
|
banner("OpenWhisk admin command line tool") | ||
|
||
val verbose = tally() | ||
val configFile = opt[File](descr = "application.conf path") |
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.
Hi @chetanmeh,
I'm wondering if there can be a default path of application.conf
that we can automatically generate this during ansible deployment? i.e. step on setup couchdb.
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.
That can be done. Another way I was thinking to have better support for Couch by reading whisk.properties
as being done by current wskadmin
. So scala tooling would also read that and seed in required typesafe properties such that for couch you need not pass in application.conf.
For others like Cosmos or later Mongo user can pass application.conf
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.
Reading whisk.properties
also works fine, but is there any specific reason that we can't provide a consistent way? (not only supporting couch)
i.e. once user deploy/re-deploy/change the database, update the config for them.
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.
No specific reason. So. far I was thinking that we "publish" the tool jar to maven/github release and then admins can just download the jar and need not checkout whole of openwhisk repo to run the admin commands. In such a case they would need to provide a custom conf
Thinking about it it now it would be fine to support that. This may require following changes
- Capture
ArtifactStore
settings as part of group vars - Have a
application.conf.j2
with some templating logic to render different config based on store type - Have it copied to std place say
${OPENWHISK_HOME}/whisk-cli.conf
or put it under${OPENWHISK_HOME}/bin/whisk-cli.conf
- Have the tooling look there by default
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.
Cool, thanks! Just some comments, I think these can be done later. Let me know if you need more hands on these.
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.
Makes sense. I would try to get a base setup working with some parity with current wskadmin and get that merged. Then we can collaborate on such enhancements
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.
Good discussion. Had similar thoughts to @tz70s and look forward to future enhancements to make it easier to use without -c.
e1884e2
to
5c49e67
Compare
@rabbah @markusthoemmes This PR is now ready for review. Currently it supports all of Dumping db content efficiently across stores would require some more work and would be done in a later PR. |
case _ => | ||
} | ||
e.printStackTrace() | ||
3 |
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.
2 for syntax/usage error?
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. It tries to map to existing wskadmin
error
class CommandError(val message: String, val code: Int)
case class IllegalState(override val message: String) extends CommandError(message, 1)
case class IllegalArg(override val message: String) extends CommandError(message, 2)
def isBlocked: Boolean = blocked.getOrElse(false) | ||
} | ||
|
||
private object ExtendedAuthFormat extends RootJsonFormat[WhiskAuth] { |
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.
should this be moved to WhiskAuth?
@markusthoemmes opinion?
logging, | ||
materializer) | ||
|
||
class ExtendedAuth(subject: Subject, namespaces: Set[WhiskNamespace], blocked: Option[Boolean]) |
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.
should this be moved to WhiskAuth?
@markusthoemmes opinion?
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.
Intention here is to add blocked: Option[Boolean]
attribute to WhiskAuth
itself?
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.
Can the existing wskadmin tests pass against wskadmin-next?
|
||
And pass that to command via `-c` option. | ||
|
||
$./wskadmin-next -c application-cli.conf user get guest |
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.
as a future enhancement, making this simpler to pass a canonical conf file (ie dont have to pass it on every command) would be nice. Wether the values are defined in the environment, or there's a canonical conf file one can use.
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.
examples below don't show a -c
file - is one required?
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.
Added them for now. Would be removed once we support a file at specific location
tools/admin/README-NEXT.md
Outdated
### Setup | ||
|
||
Build task would have created an executable ay `bin/wskadmin-next`. This script would require config related to `ArtifactStore` | ||
for accessing database. For e.g. to access user details from default CouchDB setup create a file `application-cli.conf`. |
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.
e.g. -> example.
tools/admin/README-NEXT.md
Outdated
|
||
$./gradlew :tools:admin:build | ||
|
||
This would create a jar at `tools/admin/build/libs/openwhisk-admin-tools-1.0.0-SNAPSHOT-cli.jar` and install it as an executable script at |
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'd replace "would create/have" etc with present tense. e.g., This creates a jar and installs it...
here and elsewhere.
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.
Done ✅
banner("OpenWhisk admin command line tool") | ||
|
||
val verbose = tally() | ||
val configFile = opt[File](descr = "application.conf path") |
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.
as a future enhancement, should we permit environment based overrides or using the command line to specify a configuration inline?
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.
That should work as of now also as the config loading is done in the standard way like being done when Controller/Invoker run in container
dba9b39
to
cb1d6cd
Compare
May be not all as in some cases the message format may have changed. Would give that a try |
- Set command name
TODO is removed as changing log level has same effect
-- Use present tense -- avoid abbreviations
2a26b5e
to
5747090
Compare
|
||
To build the tool run | ||
|
||
$./gradlew :tools:admin:build |
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.
we should add a redo target to build all tools (of which there is only one component now).
Build task creates an executable at `bin/wskadmin-next`. This script requires config related to `ArtifactStore` | ||
for accessing database. For example to access user details from default CouchDB setup create a file `application-cli.conf`. | ||
|
||
include classpath("application.conf") |
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.
we should have wskadmin-next generate a template conf file as a convenience.
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 thats the plan. Opened #3807 to track this
Suggestion:
should instead print help. perhaps also when given bogus arguments:
for contrast:
i realize it may be hard to match python's argparse. |
Should we suppress the stack trace - in this case the error message is useful enough. Perhaps a
|
|
We should probably prune these error logs.
|
will we be able to add tab completion to wskadmin-next? (the wskadmin version supports tab completion if you configure the bash shell properly.) |
Makes sense. Now only the error message would be logged. Stacktrace would be logged with |
This was happening because the Spring Boot Launch Script changes the base working directory to. one where the script file/jar is present. Fixed that now with with 6a3fc82 by looking for This is done on a best-effort basis. So if |
Those logs were coming because Http connection pool was not getting closed. So closing them now |
Currently Scallop does not support that. Would be an interesting feature to add ... would look into this later |
Opened #3808 wrt error messages in case of missing sub commands. This would need some changes in Scallop library used for CLI parsing. Currently some adhoc checks are done (see |
Looks like a PG should have been ran on this one. |
@dubee Missed on requesting the PG as changes were not impacting the runtime and local build of cli project went fine. However looks like it would impact downstream project which build main repo and not using Maven dependency |
Introduced wskadmin-next, an implementation of wskadmin in Scala (packaged as a fat executable) to support datastores other than CouchDB (which is the limitation of wskadmin) by sharing the implementation of the ArtifactStore. See the documentation for how to use the new CLI. Briefly > wskadmin-cli -c application-cli.conf user get guest where wskadmin-cli is the executable fat jar (Gradle build would copy this to ${OPENWHISK_HOME}/bin folder) and application-cli.conf contains configuration details. include classpath("application.conf") whisk { couchdb { protocol = "http" host = "172.17.0.1" port = "5984" username = "whisk_admin" password = "some_passw0rd" provider = "CouchDB" databases { WhiskAuth = "whisk_local_subjects" WhiskEntity = "whisk_local_whisks" WhiskActivation = "whisk_local_activations" } } }
This PR implements some part of wskadmin in scala. See this thread for related discussion.
Description
Currently some of the admin toolings like
wskadmin
connect directly to CouchDB and hence would not work for setups where some otherArtifactStore
is configured (likeCosmosDBArtifactStore
). To support duch deployments this PR implement some of the wskadmin features in a Scala based tooling which makes use of theArtifactStore
abstraction.Usage
The tooling is packaged as an executable fat jar which packages all the requiored dependencies in a single jar and can be executed like
Here
wskadmin-cli
- Executable fat jar. Gradle build would copy this to${OPENWHISK_HOME}/bin
folder-c application-cli.conf
- Configuration details. For now the db related configuration is specified hereSee readme for details
Commands Supported
User
manage users commands
create
- create a user and show authorization keydelete
- delete a userget
- get authorization key for userwhois
- identify user from an authorization keyblock
- block one or more usersunblock
- unblock one or more userslist
- list authorization keys associated with a namespacelimits
manage namespace-specific limits
set
- set limits for a given namespaceget
- get limits for a given namespace (if none exist, system defaults apply)delete
- delete limits for a given namespace (system defaults apply)db
work with dbs
get
- get contents of databaseKey aspects
application.conf
Implementation Details
Packaging
The tooling is packaged as an executable jar via Spring Boot Gradle Plugin. The plugin is used just for packaging and there is no dependency on Spring framework. Spring boot plugin creates a jar with following structure
This allows having multiple files with same name like
reference.conf
by ensuring dependency jars remain separate.Further it alse allows packaging the jar as an executable script. This enable launching jar like
./wskadmin-cli
instead of usingjava -jar wskadmin-cli.jar
approach for Linux and Mac setups. For windows users can still use thejava -jar
approach to launch the command.CLI Option Parsing
It uses scallop library to parse the command line options. It also supports sub commands similar to Python argparse library
Open Points
wskadmin-cli
core/admin
Related issue and scope
My changes affect the following components
Types of changes
Checklist: