Allow using FIPS variants of ddot-collector and agent -full images#2605
Allow using FIPS variants of ddot-collector and agent -full images#2605
Conversation
fdb5e60 to
ea11e23
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2605 +/- ##
==========================================
+ Coverage 38.94% 39.19% +0.25%
==========================================
Files 313 314 +1
Lines 27134 27403 +269
==========================================
+ Hits 10567 10741 +174
- Misses 15778 15865 +87
- Partials 789 797 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
ea11e23 to
482fefb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 482fefbed0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
f0ea705 to
5c1726e
Compare
5c1726e to
6d35212
Compare
| isFull: true, | ||
| }, | ||
| want: "gcr.io/datadoghq/agent:7.64.0-fips-jmx", | ||
| want: "gcr.io/datadoghq/agent:7.64.0-fips-full", |
There was a problem hiding this comment.
Does fips-full include jmx?
There was a problem hiding this comment.
--build-arg WITH_JMX=true --build-arg WITH_JMX_FIPS=true
| if !i.isFIPS { | ||
| return nil | ||
| } | ||
| if i.isFull || i.name == DefaultDdotCollectorImageName { |
There was a problem hiding this comment.
This is called from applyGlobalSettings which is based on internal default and global registry, so it can't have a full suffix or be any version other than one hardcoded in this file. So it will either always error without user being able to do anything about it or be a dead code once default is 7.78+. We can drop this and wait for 7.78 bump and then merge this change.
There was a problem hiding this comment.
Good catch, I had indeed placed my check at the wrong place, I now placed it after the override, which was my intent.
Preserve useFIPSAgent setting when overriding the agent version tag
Parse tag suffixes right to left (Full -> JMX -> FIPS) in fromImageConfig, matching the existing logic in FromString. Previously, checking FIPS before Full meant that -fips-full tags were misidentified: the FIPS flag was missed and -fips was left in the base tag, leading to double suffixes like -fips-fips-full when combined with a FIPS-enabled current image. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Extract parseTagSuffixes helper and use it in both FromString and fromImageConfig. When the name contains a tag, fromImageConfig now delegates directly to FromString instead of duplicating the logic. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
b5b4eb5 to
df0544d
Compare
pkg/images/images.go
Outdated
| WithFull(overrideImage.isFull) | ||
| // Only override FIPS/Full if they're explicitly present in the override tag | ||
| // Otherwise preserve current settings (from global.useFIPSAgent or existing image) | ||
| if overrideImage.isFIPS { |
There was a problem hiding this comment.
I think user overrides should take precedence over useFips, so this can be:
image.WithFIPS(overrideImage.isFIPS).
image.WithFull(overrideImage.isFull)
IMO client override should take precedence over other settings, if they hardcode something in that use it. Middle ground would be to log an error/ error out when they override to a non fips image tag + add useFips, as it may be risky to run a non fips image if useFips was set
There was a problem hiding this comment.
I'll defere to @levan-m, what do we do in core agent when useFips is true but user overrides image tag to a non fips image ?
There was a problem hiding this comment.
Reverted to previous behavior
Preserve useFIPSAgent setting when overriding the agent version tag
What does this PR do?
useFIPSAgentsetting now applies to agent-fullandddot-collectorimages.Motivation
We will start publishing FIPS variants of those images soon: DataDog/datadog-agent#46053
OTAGENT-845
Additional Notes
Important point to review:
Previously, setting
override.nodeAgent.image.tagto e.g.7.75.3, withglobal.useFIPSAgentset to true, would result in node agents using the non-FIPSagent:7.75.3.I changed the behavior to stop discarding the
useFIPSAgentsetting when overriding the version, in line with the helm chart.Now we can even set the tag to
7.75.3-fulland getagent:7.75.3-fips-fullif desired.Minimum Agent Versions
Are there minimum versions of the Datadog Agent and/or Cluster Agent required?
Both: v7.78.0
can be merged before as the check makes sure the combination is valid
Describe your test plan
I added a few test cases
Checklist
bug,enhancement,refactoring,documentation,tooling, and/ordependenciesqa/skip-qalabelFootnotes
https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits ↩