-
Notifications
You must be signed in to change notification settings - Fork 44
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
Embed backend docker command #200
Conversation
deployments/kubehound/embed.go
Outdated
const ( | ||
DefaultComposePath = "docker-compose.embed.yaml" | ||
) | ||
|
||
//go:embed docker-compose.embed.yaml | ||
var F embed.FS |
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.
Just a note, as I'm not sure exactly how you want to handle this but:
you can use var F string
directly, and avoid the need to readfile / do the error checks / convert to byte slice/string.
This also removes the need for the DefaultComposePath
.
(if you want to provide another file, dynamically, you'll also need to change the FS, but that can be done on the caller or here, depends)
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.
Couple question / notes:
- You haven't deleted the kubehound-ingestor directory, is that intended?
- If an env is already running (or something is already listening on a port), it's spitting out the "help", which I don't think is helpful. IMO there should be a prompt asking to recreate the env. (We should probably ask before doing the action, in case there's some data that the user don't want to loose)
- Could you add a print of the context that kubehound without any argument will run in? With enviroment with multiple context, this will get confusing if we are not very explicit about the env that we are pulling the data from by default.
On my machine, the janusgraph is failling on:
✘ Container kubehound-release-kubegraph-1 Error 66.4s
✔ Container kubehound-release-ui-1 Started 0.5s
✔ Container kubehound-release-mongodb-1 Created 0.0s
Error: dependency failed to start: container kubehound-release-kubegraph-1 is unhealthy
waiting for JanusGraph Server...
/etc/opt/janusgraph/janusgraph-server.yaml will be used to start JanusGraph Server in foreground
SLF4J: Class path contains multiple SLF4J bindings.
SLF4J: Found binding in [jar:file:/opt/janusgraph/lib/log4j-slf4j-impl-2.20.0.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: Found binding in [jar:file:/opt/janusgraph/lib/logback-classic-1.2.11.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: See http://www.slf4j.org/codes.html#multiple_bindings for an explanation.
SLF4J: Actual binding is of type [org.apache.logging.slf4j.Log4jLoggerFactory]
17:08:46 INFO org.janusgraph.graphdb.server.JanusGraphServer.printHeader -
mmm mmm #
# mmm m mm m m mmm m" " m mm mmm mmmm # mm
# " # #" # # # # " # mm #" " " # #" "# #" #
# m"""# # # # # """m # # # m"""# # # # #
"mmm" "mm"# # # "mm"# "mmm" "mmm" # "mm"# ##m#" # #
#
"
17:08:46 INFO com.jcabi.log.Logger.infoForced - 0 attributes loaded from 330 stream(s) in 39ms, 104 saved, 5179 ignored: []
17:08:46 INFO org.janusgraph.graphdb.server.JanusGraphServer.printHeader - JanusGraph Version: 1.0.0-rc2
17:08:46 INFO org.janusgraph.graphdb.server.JanusGraphServer.printHeader - TinkerPop Version: 3.6.2
17:08:46 INFO org.janusgraph.graphdb.server.JanusGraphServer.start - Configuring JanusGraph Server from /etc/opt/janusgraph/janusgraph-server.yaml
17:08:46 INFO org.apache.tinkerpop.gremlin.server.util.MetricManager.addJmxReporter - Configured Metrics JmxReporter configured with domain= and agentId=
17:08:46 INFO org.apache.commons.beanutils.FluentPropertyBeanIntrospector.introspect - Error when creating PropertyDescriptor for public final void org.apache.commons.configuration2.AbstractConfiguration.setProperty(java.lang.String,java.lang.Object)! Ignoring this property.
17:08:47 INFO org.janusgraph.graphdb.idmanagement.UniqueInstanceIdRetriever.getOrGenerateUniqueInstanceId - Generated unique-instance-id=ac1113031-b9e1c66ac9961
17:08:47 INFO org.janusgraph.diskstorage.Backend.getIndexes - Configuring index [search]
17:08:47 INFO org.janusgraph.diskstorage.configuration.ExecutorServiceBuilder.buildFixedExecutorService - Initiated fixed thread pool of size 40
17:08:47 INFO org.janusgraph.graphdb.database.StandardJanusGraph.<init> - Gremlin script evaluation is disabled
17:08:47 ERROR org.apache.tinkerpop.gremlin.server.util.ServerGremlinExecutor.<init> - Could not invoke constructor on class org.janusgraph.graphdb.management.JanusGraphManager (defined by the 'graphManager' setting) with one argument of class Settings
17:08:47 ERROR org.janusgraph.graphdb.server.JanusGraphServer.lambda$main$0 - JanusGraph Server was unable to start and will now begin shutdown
java.lang.RuntimeException: java.lang.reflect.InvocationTargetException
at org.apache.tinkerpop.gremlin.server.util.ServerGremlinExecutor.<init>(ServerGremlinExecutor.java:97) ~[gremlin-server-3.6.2.jar:3.6.2]
I have 5GB disk available, could do some cleanup but that seems large?
If not, we should say so somewhere, 5GB in VM and stuff it's a bunch
Will do another pass tomorow but LGTM overall :)
@@ -0,0 +1,23 @@ | |||
package backend | |||
|
|||
func mergeMaps(currentMap map[interface{}]interface{}, newMap map[interface{}]interface{}) map[interface{}]interface{} { |
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.
Could you write a test for this? :D (maybe create a card for later).
It seems simple but the amount of interface{}
... :D
Btw, you can use any
instead, it'll be a tad cleaner I guess.
envCmd = &cobra.Command{ | ||
Use: "dev", | ||
Hidden: true, |
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.
Probably add a line in the readme / how to contribute to document this if we hide that command. (I will forget this in 3month)
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.
adding in the Jira card to track it for later (when new release is official).
cmd/kubehound/dev.go
Outdated
var err error | ||
Backend, err = docker.NewBackend(cobraCmd.Context(), composePath) | ||
|
||
return err |
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.
Backend is a global var right? does it need to be exported? (it's also set in the runEnv function), which i'm not sure on why aren't using as well?
It looks like it's doing that twice: one in the pre run and once in the run.
(I feel like the Backend should be a singleton in another package, rather than in the cmd/kubehound, probably in the pkg/backend directly tbh)
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 the singleton as we discussed
@@ -0,0 +1,8 @@ | |||
package embedconfigdocker |
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.
Nit: I'm always in favor of using the folder as the package name and we can always rename the import.
I find it easier for adding new files and all.
For this "use case" where it's likely to be the only file, i'm fine with it but 🤷
pkg/backend/containers.go
Outdated
type Backend struct { | ||
project *types.Project | ||
composeService api.Service | ||
dockerCli *command.DockerCli | ||
} | ||
|
||
func NewBackend(ctx context.Context, composeFilePaths []string) (*Backend, 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.
I think this package would benefit from using a singleton pattern mentionned in the cmd package earlier.
There's only one backend that should be running at the same time right, and it'll remove the global var there.
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
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.
LFG! ✔️ 🚀
Adding backend docker command to remove
kubehound.sh
bash script.Also, the backend gets automatically spawned when using the basic command
./kubehound
.Also, added an hidden command for all the spawning of the backend for the dev env:
Added also a way to dump the local version :
kubehound version