Skip to content

Conversation

@joushx
Copy link
Contributor

@joushx joushx commented Oct 31, 2023

Migrate the dnytrace integration to use the LD_PRELOAD method as used within other buildpacks and support FIPS mode.

  • Set LD_PRELOAD environment variable instead of java options
  • Add option to enable FIPS cryptographic mode
  • adapt tests and test fixtures

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 31, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@joushx joushx changed the title Migrate dynatrace integration to LD_PRELOAD WIP: Migrate dynatrace integration to LD_PRELOAD Oct 31, 2023
@joushx joushx force-pushed the main branch 6 times, most recently from 49e08c2 to 955d616 Compare October 31, 2023 14:36
@joushx joushx changed the title WIP: Migrate dynatrace integration to LD_PRELOAD Migrate dynatrace integration to LD_PRELOAD and support FIPS mode Oct 31, 2023
technologies = manifest['technologies']
java_binaries = technologies['java']['linux-x86-64']
loader = java_binaries.find { |bin| bin['binarytype'] == 'loader' }
java_binaries = technologies['process']['linux-x86-64']
Copy link
Contributor

Choose a reason for hiding this comment

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

why change from java to process ?

Copy link
Contributor Author

@joushx joushx Nov 13, 2023

Choose a reason for hiding this comment

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

The use of java is for historical reasons. process is now the default and also used in other buildpacks (e.g. https://github.com/Dynatrace/libbuildpack-dynatrace/blob/master/hook.go#L403)

Copy link
Contributor

Choose a reason for hiding this comment

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

ok!


def skip_errors?
credentials[SKIP_ERRORS].to_b
credentials[SKIP_ERRORS] == "true"
Copy link
Contributor

@anthonydahanne anthonydahanne Nov 10, 2023

Choose a reason for hiding this comment

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

why ignoring credentials[SKIP_ERRORS] ?

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry my mistake ; should be the same - why the change though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We found that the method was not working in the way we expected it (see https://try.ruby-lang.org/playground/#code=class+String%0A++def+to_b%0A++++casecmp+'true'%0A++end%0Aend%0A%0Aif+%22false%22.to_b%0A++puts+%22yes%22%0Aend&engine=cruby-3.2.0).

Also, from my understanding it would match against "t" or other parts of "true". Which contradicts the information in our documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, makes sense!

end

def enable_fips?
credentials[ENABLE_FIPS] == "true"
Copy link
Contributor

@anthonydahanne anthonydahanne Nov 10, 2023

Choose a reason for hiding this comment

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

same, why is it unconditional?

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry my mistake - all is fine!

@meibensteiner
Copy link

@dmikusa @pivotal-david-osullivan Could you please take a look? We have some customers who want to switch to the updated buildpack by end of year.

@dmikusa
Copy link
Contributor

dmikusa commented Nov 28, 2023

@meibensteiner Hi, sorry for the delay. The PR looks OK but it has to go through some manual testing. Testing with the CF JBP is time-consuming, but helps ensure we don't have any regressions. Once that's done we'll move forward and get it merged. Thanks in advance for your patience.

@TimGerlach
Copy link

Hi @dmikusa ,
Have you had a chance to perform the manual testing that you mentioned previously? I'm asking because we are running a critical project that requires this PR to be merged before the end of this year.
We appreciate your support.

@pivotal-david-osullivan
Copy link
Contributor

@TimGerlach
@anthonydahanne is working on this testing, we are aiming to get this out in a release next week!

@anthonydahanne
Copy link
Contributor

@dmikusa , @pivotal-david-osullivan I could test this change, using:

  buildpacks:
    -  https://github.com/joushx/java-buildpack

and using my own Dynatrace trial account - everything still worked fine,see screenshots
Screenshot 2023-12-19 at 14 33 20
Screenshot 2023-12-19 at 14 31 02

You'll need to rebase this branch though before merging; it's pretty far behind now.

Thanks @joushx for your patience!

Migrate the dnytrace integration to use the LD_PRELOAD method as
used within other buildpacks.

* Set LD_PRELOAD environment variable instead of java options
* adapt tests and test fixtures
This adds an option to enable FIPS mode if enablefips is set.

OneAgent uses the FIPS mode to be compliant with the FIPS 140-3
computer security standard.
@joushx
Copy link
Contributor Author

joushx commented Dec 20, 2023

Nice, thank you! I just did the rebase.

joushx and others added 3 commits December 20, 2023 14:32
Co-authored-by: David O'Sullivan <[email protected]>
Co-authored-by: David O'Sullivan <[email protected]>
Co-authored-by: David O'Sullivan <[email protected]>
Co-authored-by: David O'Sullivan <[email protected]>
@joushx
Copy link
Contributor Author

joushx commented Dec 20, 2023

Linter suggestions applied

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.

7 participants