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 launch policy for cmd and env vars #195

Merged
merged 3 commits into from
May 26, 2022
Merged

Conversation

jkl73
Copy link
Contributor

@jkl73 jkl73 commented May 3, 2022

Add the launch policy implementation for env vars and cmd

Update logger so it output everything from launcher to stdout and cloud logging (for better debug)

Update service file so it will sleep for 60 seconds before shutting down the machine.

Update cloudbuild.yaml with the new image name.

@jkl73 jkl73 force-pushed the launchpolicy branch 2 times, most recently from 0f599c2 to f2b6847 Compare May 3, 2022 05:08
@jkl73 jkl73 requested review from josephlr and alexmwu May 3, 2022 05:40
@jkl73 jkl73 marked this pull request as ready for review May 3, 2022 05:40
@jkl73 jkl73 force-pushed the launchpolicy branch 3 times, most recently from fc7a3e4 to 11da8cc Compare May 25, 2022 00:30
@jkl73 jkl73 changed the title Add launch policy and stdout/stderr redirect Add launch policy for cmd and env vars May 25, 2022
Copy link
Contributor

@alexmwu alexmwu left a comment

Choose a reason for hiding this comment

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

Looks good. Mostly nits.

cloudbuild.yaml Show resolved Hide resolved
@@ -6,7 +6,8 @@ After=network-online.target gcr-online.target containerd.service
[Service]
ExecStart=/var/lib/google/cc_container_launcher --addr=${ATTEST_ENDPOINT}
# Shutdown the host after the launcher exits
ExecStopPost=systemctl poweroff
ExecStopPost=/bin/sleep 60
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried looking through the documentation (https://www.freedesktop.org/software/systemd/man/systemd.service.html) but couldn't find anything about ordering. Does systemd guarantee execution in order or declaration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I can't find documentation either. From my testing, it is in order. I think this fine for now, later we should create an exit script.

@@ -345,10 +345,8 @@ func (r *ContainerRunner) Run(ctx context.Context) error {
return fmt.Errorf("failed to fetch and write OIDC token: %v", err)
}

loggerAndStdout := io.MultiWriter(os.Stdout, r.logger.Writer())
loggerAndStderr := io.MultiWriter(os.Stderr, r.logger.Writer())
Copy link
Contributor

Choose a reason for hiding this comment

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

do we no longer want the ability to distinguish b/w stdout/err?

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 moved this step to the beginning, so all log generated by the launcher will have these two destination, better debug.

launcher/spec/launch_policy.go Outdated Show resolved Hide resolved
}

if v, ok := imageLabels[cmdOverride]; ok {
if launchPolicy.AllowedCmdOverride, err = strconv.ParseBool(v); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

because ParseBool (https://pkg.go.dev/strconv#ParseBool) accepts so many different bool-like values, we should clearly document that this is the current interface. Alternatively, we can consider only allowing a string "true" and a string "false" and maybe different capitalizations

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 agree we should emphasis this in the codelab/doc for image authors, recommend using (true/false), but other value are also accept but may not work later.
I just don't want/too lazy to implement our own logic for checking bool val...

launcher/spec/launch_policy.go Outdated Show resolved Hide resolved
launcher/spec/launch_policy_test.go Outdated Show resolved Hide resolved
launcher/spec/launch_policy_test.go Outdated Show resolved Hide resolved
jkl73 added 3 commits May 25, 2022 20:51
Signed-off-by: Jiankun Lu <jiankun@google.com>
Launcher logger will be published to stdout and Cloud Logging

Signed-off-by: Jiankun Lu <jiankun@google.com>
Signed-off-by: Jiankun Lu <jiankun@google.com>
@jkl73 jkl73 merged commit ebbee63 into google:master May 26, 2022
@JoshuaKrstic JoshuaKrstic mentioned this pull request Aug 19, 2022
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