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

Embed backend docker command #200

Merged
merged 36 commits into from
Jun 11, 2024
Merged

Embed backend docker command #200

merged 36 commits into from
Jun 11, 2024

Conversation

jt-dd
Copy link
Contributor

@jt-dd jt-dd commented Jun 6, 2024

Adding backend docker command to remove kubehound.sh bash script.

./bin/build/kubehound backend --help
Handle the kubehound stack - docker compose based stack for kubehound services (mongodb, graphdb and UI)

Usage:
  kubehound backend [command]

Available Commands:
  down        Tear down the kubehound stack
  reset       Restart the kubehound stack
  up          Spawn the kubehound stack
  wipe        Wipe the persisted backend data

Flags:
  -f, --file strings   Compose configuration files
  -h, --help           help for backend

Global Flags:
      --debug   Enable debug logs

Use "kubehound backend [command] --help" for more information about a command.

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:

./bin/build/kubehound dev --help
[devOnly] Spawn the kubehound dev stack for the system-tests (build from dockerfile)

Usage:
  kubehound dev [flags]
  kubehound dev [command]

Available Commands:
  system-tests [devOnly] Spawn the kubehound testing stack for the system-tests

Flags:
      --down   Tearing down the kubehound dev stack and deleting the data associated with it
  -h, --help   help for dev
      --ui     Tearing down the kubehound dev stack and deleting the data associated with it

Global Flags:
      --debug   Enable debug logs

Use "kubehound dev [command] --help" for more information about a command.

Added also a way to dump the local version : kubehound version

Comment on lines 7 to 12
const (
DefaultComposePath = "docker-compose.embed.yaml"
)

//go:embed docker-compose.embed.yaml
var F embed.FS
Copy link
Contributor

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)

Copy link
Contributor

@edznux-dd edznux-dd left a 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{} {
Copy link
Contributor

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.

cmd/kubehound/dev.go Outdated Show resolved Hide resolved
Comment on lines +19 to +21
envCmd = &cobra.Command{
Use: "dev",
Hidden: true,
Copy link
Contributor

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)

Copy link
Contributor Author

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).

Comment on lines 25 to 28
var err error
Backend, err = docker.NewBackend(cobraCmd.Context(), composePath)

return err
Copy link
Contributor

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)

Copy link
Contributor Author

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

cmd/kubehound/version.go Outdated Show resolved Hide resolved
@@ -0,0 +1,8 @@
package embedconfigdocker
Copy link
Contributor

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 🤷

Comment on lines 17 to 23
type Backend struct {
project *types.Project
composeService api.Service
dockerCli *command.DockerCli
}

func NewBackend(ctx context.Context, composeFilePaths []string) (*Backend, error) {
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

pkg/backend/containers.go Show resolved Hide resolved
pkg/backend/project.go Show resolved Hide resolved
pkg/backend/containers.go Outdated Show resolved Hide resolved
@jt-dd jt-dd marked this pull request as ready for review June 11, 2024 08:17
@jt-dd jt-dd requested a review from a team as a code owner June 11, 2024 08:17
Copy link
Contributor

@edznux-dd edznux-dd left a comment

Choose a reason for hiding this comment

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

LFG! ✔️ 🚀

@jt-dd jt-dd merged commit 8b7951b into main Jun 11, 2024
7 checks passed
@jt-dd jt-dd deleted the jt-dd/backend-commands-embedded branch June 11, 2024 08:43
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.

2 participants