Create init image for lib-injection #2606

TonyCTHsu · 2023-02-08T10:42:42Z

What

Create init container image for k8s admission controller injection.

The init container image leverage RUBYOPT for ruby intepreter and the injection script would make an attempt to include ddtrace within the bundle. The install attempt could failed due to Bundler's deployment mode or unable to resolve compatible versions from bundler, but it would not blocked application process.

For a Rails/Hanami app, the application would be auto instrumented.

Public release workflow would be triggered during a release with a tag push v*.*.* . Otherwise, the workflow would create images for internal testing purchase.

lib-injection/auto_inject.rb

ivoanjo · 2023-02-21T17:20:31Z

Any thoughts on how to test this? There's quite a bit of non-trivial logic in auto_inject.rb, so it would be great to have some tests that give us confidence that we don't accidentally break it as we change things.

codecov-commenter · 2023-02-21T17:31:56Z

Codecov Report

Merging #2606 (befe4f7) into master (d76de94) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2606   +/-   ##
=======================================
  Coverage   98.08%   98.08%           
=======================================
  Files        1160     1160           
  Lines       63676    63678    +2     
  Branches     2849     2849           
=======================================
+ Hits        62457    62459    +2     
  Misses       1219     1219

Impacted Files	Coverage Δ
spec/ddtrace/release_gem_spec.rb	`100.00% <ø> (ø)`
...ec/datadog/tracing/contrib/sidekiq/patcher_spec.rb	`96.00% <0.00%> (-4.00%)`	⬇️
lib/datadog/core/workers/polling.rb	`96.55% <0.00%> (-3.45%)`	⬇️
spec/datadog/profiling/ext/forking_spec.rb	`100.00% <0.00%> (+0.72%)`	⬆️
lib/datadog/core/diagnostics/environment_logger.rb	`98.48% <0.00%> (+0.79%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

marcotc · 2023-02-22T22:23:52Z

lib-injection/Dockerfile

+# and auto instrument Ruby applications in containerized environments.
+FROM busybox
+
+ARG UID=10000


Why is the user id set to 10000? Any reason for this specific value?

Found it: http://www.linfo.org/uid.html

lib-injection/auto_inject.rb

marcotc

Looks solid! Let's get this out and hear the feedback!

lib-injection/auto_inject.rb

GustavoCaso · 2023-02-28T10:08:05Z

lib-injection/auto_inject.rb

+      puts "[DATADOG LIB INJECTION] #{output}"
+    end
+  rescue => e
+    puts "[DATADOG LIB INJECTION] trial error: #{e}"


Trial error? What does it mean?

The ruby script here is conducting a trial for adding ddtrace into the bundle. The trial could be success or not and then cleanup the filesystem. I added this to be safe if there is an exception thrown during the trial but I don't see it coming.

I think customers may have the same question, and I suggest that the answer is part of the error message, not just a comment here :)

GustavoCaso

Great work

ivoanjo

Left a few notes. I wouldn't say my feedback is blocking, but please do consider looking into it before we release this to customers.

Especially the current log messages I think need a bit of love.

ivoanjo · 2023-02-28T12:01:55Z

.github/workflows/lib-injection.yml

+    runs-on: ubuntu-latest
+    permissions:
+      contents: read
+      packages: write


I'm curious about this -- does the current test action still require write permissions? Is this perhaps a left-over from an example/another action we started from?

lib-injection/auto_inject.rb

ivoanjo · 2023-02-28T12:29:01Z

lib-injection/auto_inject.rb

+  require 'fileutils'
+
+  begin
+    # Copies for trial


Consider explaining here in a bit more detail why we're doing this

lib-injection/auto_inject.rb

ivoanjo · 2023-02-28T12:30:24Z

lib-injection/auto_inject.rb

+      puts "[DATADOG LIB INJECTION] #{output}"
+    end
+  rescue => e
+    puts "[DATADOG LIB INJECTION] trial error: #{e}"


I think customers may have the same question, and I suggest that the answer is part of the error message, not just a comment here :)

ivoanjo · 2023-02-28T12:32:16Z

lib-injection/auto_inject.rb

+  puts "[DATADOG LIB INJECTION] LoadError: #{e}"
+rescue Exception => e
+  puts "[DATADOG LIB INJECTION] Exception: #{e}"
+end


I had left an earlier comment on this PR, that perhaps you missed but -- I think we're missing some specs for this. We do have some coverage via the system tests, but consider testing some or all of the behaviors here using RSpec as well, to give us more confidence when changing this script.

marcotc reviewed Feb 13, 2023

View reviewed changes

lib-injection/auto_inject.rb Outdated Show resolved Hide resolved

marcotc reviewed Feb 14, 2023

View reviewed changes

lib-injection/auto_inject.rb Outdated Show resolved Hide resolved

TonyCTHsu added 3 commits February 15, 2023 11:21

Create init image for lib-injection

dba83ca

Add version and remove DDTRACE_AUTOINJECT

76458d7

Add build publish workflow

d2f48b4

TonyCTHsu force-pushed the tonycthsu/lib-injection branch 2 times, most recently from db3ca3e to d2f48b4 Compare February 16, 2023 13:09

Add deployment to gitlab ci

d559785

TonyCTHsu force-pushed the tonycthsu/lib-injection branch from 55ee18f to 1d48dad Compare February 16, 2023 19:31

WIP

159657b

TonyCTHsu force-pushed the tonycthsu/lib-injection branch from 1d48dad to 159657b Compare February 16, 2023 19:42

TonyCTHsu added 4 commits February 20, 2023 16:24

Cleanup workflow

009f521

Handle failure

953ad81

Change test for lib-injection to not included for gem packaging

eba7706

Refactor workflow

17ec104

TonyCTHsu force-pushed the tonycthsu/lib-injection branch 2 times, most recently from 4554856 to 764028b Compare February 21, 2023 11:02

Release workflow

4b648ee

TonyCTHsu marked this pull request as ready for review February 21, 2023 11:54

TonyCTHsu requested a review from a team February 21, 2023 11:54

TonyCTHsu force-pushed the tonycthsu/lib-injection branch from 764028b to 4b648ee Compare February 21, 2023 11:54

Dynamic gemfile

75c3fa9

marcotc reviewed Feb 22, 2023

View reviewed changes

lib-injection/auto_inject.rb Outdated Show resolved Hide resolved

marcotc reviewed Feb 22, 2023

View reviewed changes

lib-injection/auto_inject.rb Outdated Show resolved Hide resolved

marcotc reviewed Feb 22, 2023

View reviewed changes

lib-injection/auto_inject.rb Outdated Show resolved Hide resolved

marcotc reviewed Feb 22, 2023

View reviewed changes

lib-injection/auto_inject.rb Show resolved Hide resolved

marcotc reviewed Feb 22, 2023

View reviewed changes

lib-injection/auto_inject.rb Outdated Show resolved Hide resolved

TonyCTHsu added 2 commits February 24, 2023 22:33

All variant

322deda

Fix gems.rb check

e6baa62

TonyCTHsu force-pushed the tonycthsu/lib-injection branch 5 times, most recently from 3901487 to 8d3e847 Compare February 27, 2023 15:30

Refactor

f656a00

TonyCTHsu force-pushed the tonycthsu/lib-injection branch from 8d3e847 to f656a00 Compare February 27, 2023 15:32

TonyCTHsu added 2 commits February 27, 2023 17:36

Shell escape

90bf0b6

Change with open3

a9bfad6

TonyCTHsu force-pushed the tonycthsu/lib-injection branch from 84f7128 to a9bfad6 Compare February 27, 2023 17:36

Merge branch 'master' into tonycthsu/lib-injection

a0b1a04

marcotc reviewed Feb 27, 2023

View reviewed changes

lib-injection/auto_inject.rb Outdated Show resolved Hide resolved

Update Dockerfile

d6388c5

marcotc approved these changes Feb 27, 2023

View reviewed changes

TonyCTHsu added 2 commits February 27, 2023 21:24

Split puts statement for different rescue

db8301c

Rescue all

c2e7434

GustavoCaso reviewed Feb 28, 2023

View reviewed changes

lib-injection/auto_inject.rb Outdated Show resolved Hide resolved

GustavoCaso reviewed Feb 28, 2023

View reviewed changes

lib-injection/auto_inject.rb Outdated Show resolved Hide resolved

GustavoCaso reviewed Feb 28, 2023

View reviewed changes

Improve scripting by extrace the add cmd

befe4f7

GustavoCaso approved these changes Feb 28, 2023

View reviewed changes

ivoanjo reviewed Feb 28, 2023

View reviewed changes

Improve logging

0499917

TonyCTHsu force-pushed the tonycthsu/lib-injection branch from 4dcf76c to 0499917 Compare February 28, 2023 15:30

TonyCTHsu merged commit f39b81b into master Feb 28, 2023

TonyCTHsu deleted the tonycthsu/lib-injection branch February 28, 2023 16:51

github-actions bot added this to the 1.10.0 milestone Feb 28, 2023

TonyCTHsu mentioned this pull request Mar 6, 2023

Bump to version 1.10.0 #2667

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create init image for lib-injection #2606

Create init image for lib-injection #2606

TonyCTHsu commented Feb 8, 2023 •

edited

ivoanjo commented Feb 21, 2023

codecov-commenter commented Feb 21, 2023 •

edited

marcotc Feb 22, 2023

marcotc Feb 27, 2023

marcotc left a comment

GustavoCaso Feb 28, 2023

TonyCTHsu Feb 28, 2023 •

edited

ivoanjo Feb 28, 2023

GustavoCaso left a comment

ivoanjo left a comment

ivoanjo Feb 28, 2023

ivoanjo Feb 28, 2023

ivoanjo Feb 28, 2023

ivoanjo Feb 28, 2023

Create init image for lib-injection #2606

Create init image for lib-injection #2606

Conversation

TonyCTHsu commented Feb 8, 2023 • edited

ivoanjo commented Feb 21, 2023

codecov-commenter commented Feb 21, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcotc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TonyCTHsu Feb 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GustavoCaso left a comment

Choose a reason for hiding this comment

ivoanjo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TonyCTHsu commented Feb 8, 2023 •

edited

codecov-commenter commented Feb 21, 2023 •

edited

TonyCTHsu Feb 28, 2023 •

edited