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

Add linux container support #131

Merged
merged 3 commits into from
Sep 13, 2017
Merged

Add linux container support #131

merged 3 commits into from
Sep 13, 2017

Conversation

zwass
Copy link
Contributor

@zwass zwass commented Sep 12, 2017

  • Add Makefile commands for container generation and pushing
  • Refactor code for finding extension path
  • Refactor tests for changed extension path (tests now run ~50% faster)

@zwass
Copy link
Contributor Author

zwass commented Sep 12, 2017

@zwass zwass requested review from marpaia and groob September 12, 2017 17:35
Makefile Outdated

containers: xp-launcher xp-extension $(CONTAINERS)

$(CONTAINERS):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is clever, I like it!


COPY build/linux/ /usr/local/bin/

ENTRYPOINT ["/usr/local/bin/launcher", "-osqueryd_path", "/usr/bin/osqueryd", "-debug"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to define osqueryd_path? I would imagine that /usr/bin is in the default PATH?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's necessary, but I'm just being explicit here.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's definitely a nit, but I think if it works without defining the flag, we should leave it out as it illustrates the simplicity of the launcher command-line experience. That being said, however, isn't hostname required now? Maybe we should just not have an entrypoint in these containers?

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I think it should be a CMD not an ENTRYPOINT.

The entrypoints are annoying to override in a pinch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am okay with deleting the ENTRYPOINT, but I don't see the purpose of having a CMD when there are still other arguments (enroll secret, host:port) that must be specified.

Copy link
Contributor

Choose a reason for hiding this comment

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

Those could be passed as env vars?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does launcher support passing options as env vars? If so, it doesn't indicate in launcher --help.

Copy link
Contributor

Choose a reason for hiding this comment

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

yep, check out the flag parsing code:

var (
flDebug = flag.Bool(
"debug",
false,
"enable debug logging",
)
flVersion = flag.Bool(
"version",
false,
"print launcher version and exit",
)
flInsecureTLS = flag.Bool(
"insecure",
false,
"do not verify TLS certs for outgoing connections",
)
flInsecureGRPC = flag.Bool(
"insecure_grpc",
false,
"dial GRPC without a TLS config",
)
flOsquerydPath = flag.String(
"osqueryd_path",
env.String("KOLIDE_LAUNCHER_OSQUERYD_PATH", ""),
"path to osqueryd binary",
)
flRootDirectory = flag.String(
"root_directory",
env.String("KOLIDE_LAUNCHER_ROOT_DIRECTORY", os.TempDir()),
"path to the working directory where file artifacts can be stored",
)
flNotaryServerURL = flag.String(
"notary_url",
env.String("KOLIDE_LAUNCHER_NOTARY_SERVER_URL", ""),
"The URL of the notary update server",
)
flKolideServerURL = flag.String(
"hostname",
env.String("KOLIDE_LAUNCHER_HOSTNAME", ""),
"Hostname of the remote server to communicate with",
)
flEnrollSecret = flag.String(
"enroll_secret",
env.String("KOLIDE_LAUNCHER_ENROLL_SECRET", ""),
"The enrollment secret used to authenticate with the server",
)
flEnrollSecretPath = flag.String(
"enroll_secret_path",
env.String("KOLIDE_LAUNCHER_ENROLL_SECRET_PATH", ""),
"Path to a file containing the enrollment secret",
)
flMirrorURL = flag.String(
"mirror_url",
env.String("KOLIDE_LAUNCHER_MIRROR_SERVER_URL", ""),
"The URL of the mirror server for autoupdates",
)
flAutoupdateInterval = flag.Duration(
"autoupdate_interval",
duration("KOLIDE_LAUNCHER_AUTOUPDATE_INTERVAL", 1*time.Hour),
"The interval when launcher checks for new updates. Only enabled if notary_url is set.",
)
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like there is some dependency on specifying the full path of the launcher in order to find the extension binary. I think I should be able to fix this in the Go launcher code, but if not, it would probably warrant leaving the ENTRYPOINT.

@zwass
Copy link
Contributor Author

zwass commented Sep 13, 2017

Hmm, my fix for getting the correct binary path seems to have broken some assumptions in tests.

@zwass zwass changed the title Add Makefile commands for building and pushing Linux containers Add linux container support Sep 13, 2017
Copy link
Contributor

@marpaia marpaia left a comment

Choose a reason for hiding this comment

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

This looks great


CONTAINERS = ubuntu14 ubuntu16 centos6 centos7

.PHONY: push-containers containers $(CONTAINERS)
Copy link
Contributor

Choose a reason for hiding this comment

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

there is already a phony on line 3; do you think it's worth adding these up there and/or adding phonies for all of the targets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a bunch of refactoring that could be done to this Makefile to avoid redundant builds (see generate, for example... It should be a .PHONY, and should list the files it generates as dependencies. Then those files should have make rules. That would enable them to only be built when needed), but I'd like to merge as-is now and come back to that when it's a priority.

@zwass zwass merged commit 5ee0878 into kolide:master Sep 13, 2017
@zwass zwass deleted the make_containers branch September 13, 2017 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants