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

Enable oci hooks through API settings #1872

Merged
merged 3 commits into from Dec 17, 2021

Conversation

arnaldo2792
Copy link
Contributor

@arnaldo2792 arnaldo2792 commented Dec 16, 2021

Issue number:
N / A

Description of changes:

api: add oci-hooks setting to enable OCI hooks

The oci-hooks setting allows a user to enable OCI hooks provided by the OS. For the time being, the only OCI hook provided is the log4j2-hotpatch, which applies the hotpatch for Apache Log4j2 to containers running JVMs.

Remaining work:

  • Documentation for the new setting
  • Add API setting to vmware variants
  • Add migrations
  • Upgrade/Downgrade migration test

Testing done:
In aws-ecs/aws-k8s-1.21:

Run workloads with java services, with and without enabling the log4j-hotpatch hook. I saw the containers being patched, and the logs generated in /dev/shm/hotdog.log.

  • Run testing pods in aws k8s variants
  • Run testing pods in vmware k8s variants

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

@arnaldo2792 arnaldo2792 changed the title Enable oci hooks though API Enable oci hooks through API settings Dec 17, 2021
Copy link
Contributor

@samuelkarp samuelkarp left a comment

Choose a reason for hiding this comment

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

This provisionally looks good to me.

@cbgbt
Copy link
Contributor

cbgbt commented Dec 17, 2021

We need a migration to remove this setting on downgrade, as discussed out of github. But this otherwise LGTM.

Copy link
Member

@webern webern left a comment

Choose a reason for hiding this comment

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

Don't we need a migration?

@@ -1,7 +1,6 @@
%global _cross_first_party 1
%global _is_k8s_variant %(if echo %{_cross_variant} | grep -Fqw "k8s"; then echo 1; else echo 0; fi)
%global _is_aws_variant %(if echo %{_cross_variant} | grep -Fqw "aws"; then echo 1; else echo 0; fi)
%global _is_vendor_variant %(if echo %{_cross_variant} | grep -Fqw "nvidia"; then echo 1; else echo 0; fi)
Copy link
Member

Choose a reason for hiding this comment

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

Is this on develop?

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 was, yeah, the line was added with shimpei, because we didn't want to use it as the default runtime for all variants. But now we do 👍

@webern
Copy link
Member

webern commented Dec 17, 2021

We need a migration to remove this setting on downgrade, as discussed out of github. But this otherwise LGTM.

Oh, things are happening to fast, I can't keep up 😅

@arnaldo2792
Copy link
Contributor Author

Forced push includes:

  • Missing settings for vmware variants
  • Removed restart services configurations

@arnaldo2792
Copy link
Contributor Author

Forced push includes rebase to upstream.

@cbgbt cbgbt mentioned this pull request Dec 17, 2021
1 task
With shimpei as the default runtime for container runtimes, we can run
OCI hooks provided through API settings. This commit needs the OCI
settings to properly work, since shimpei reads a hooks file generated by
changes in the settings.

Signed-off-by: Arnaldo Garcia Rincon <agarrcia@amazon.com>
@arnaldo2792
Copy link
Contributor Author

Forced push includes:

  • Simplified configuration files, based on offline conversation with @bcressey
  • Migrations for the new API
  • Documentation for the new API
  • Missing dependency in release for log4j-hotpatch

Copy link
Contributor

@cbgbt cbgbt left a comment

Choose a reason for hiding this comment

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

LGTM, pending testing! 🚢

README.md Outdated
#### OCI Hooks settings

Bottlerocket allows you to opt-in to use additional [OCI hooks](https://github.com/opencontainers/runtime-spec/blob/main/runtime.md#lifecycle) for your orchestrated containers.
Once you opt-in to use additional OCI hooks, the new orchestrated containers will be configured with them, but the existing containers won't be changed.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
Once you opt-in to use additional OCI hooks, the new orchestrated containers will be configured with them, but the existing containers won't be changed.
Once you opt-in to using additional OCI hooks, newly-orchestrated containers will be configured with them. Existing containers won't be changed.

README.md Outdated Show resolved Hide resolved
packages/release/Cargo.toml Outdated Show resolved Hide resolved
packages/release/release.spec Outdated Show resolved Hide resolved
sources/models/shared-defaults/defaults.toml Outdated Show resolved Hide resolved
We always include log4j-hotpatch in all variants

Signed-off-by: Arnaldo Garcia Rincon <agarrcia@amazon.com>
The oci-hooks setting allows a user to enable OCI hooks provided by the
OS. For the time being, the only OCI hook provided is the
`log4j2-hotpatch`, which applies the hotpatch for Apache Log4j2 to
containers running JVMs.

Signed-off-by: Arnaldo Garcia Rincon <agarrcia@amazon.com>
@arnaldo2792
Copy link
Contributor Author

Forced push includes:

  • Fixed documentation
  • Moved default settings to oci-hooks.toml
  • Add log4j-hotpatch as a dependency of hotdog
  • Changed commit's order

Copy link
Contributor

@cbgbt cbgbt left a comment

Choose a reason for hiding this comment

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

:shipit:

@arnaldo2792 arnaldo2792 merged commit 58f95d2 into bottlerocket-os:develop Dec 17, 2021
@arnaldo2792 arnaldo2792 deleted the log4j-hotpatch branch January 14, 2022 20:23
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.

None yet

5 participants