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 FIPS flag #3315

Merged
merged 6 commits into from
Oct 2, 2024
Merged

Add FIPS flag #3315

merged 6 commits into from
Oct 2, 2024

Conversation

pavolloffay
Copy link
Member

@pavolloffay pavolloffay commented Sep 30, 2024

@pavolloffay pavolloffay requested a review from a team as a code owner September 30, 2024 15:25
@pavolloffay pavolloffay changed the title Fips Add FIPS flag Sep 30, 2024
internal/fips/check.go Outdated Show resolved Hide resolved
internal/fips/check.go Outdated Show resolved Hide resolved
internal/webhook/podmutation/webhookhandler_suite_test.go Outdated Show resolved Hide resolved
@@ -180,6 +182,7 @@ func main() {
pflag.StringVar(&encodeLevelKey, "zap-level-key", "level", "The level key to be used in the customized Log Encoder")
pflag.StringVar(&encodeTimeKey, "zap-time-key", "timestamp", "The time key to be used in the customized Log Encoder")
pflag.StringVar(&encodeLevelFormat, "zap-level-format", "uppercase", "The level format to be used in the customized Log Encoder")
pflag.StringVar(&fipsDisabledComponents, "fips-disabled-components", "uppercase", "Disabled collector components when operator runs on FIPS enabled platform. Example flag value =receiver.foo,receiver.bar,exporter.baz")
Copy link
Contributor

Choose a reason for hiding this comment

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

should we name this disabled-components? I thought we had discussed wanting this for non-fips users?

Copy link
Member Author

Choose a reason for hiding this comment

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

should we name this disabled-components? I thought we had discussed wanting this for non-fips users?

I don't want to make this feature generic for the follow up reasons:

If we wanted to make this generic we would have to introduce another flag that would conditionally enable the disabled components flag. Such flag could check env var, however users would need to rebuild the operator image and introduce init script that would set the env var based on the content of /proc/sys/crypto/fips_enabled

The FIPS feature on k8s is a well-defined concept. See the links in the PR descriptions. Making its own configuration option on the operator makes sense to me, rather than allowing the use case with generic option that require no trivial configuration.

Copy link
Contributor

Choose a reason for hiding this comment

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

why would need another flag to conditionally enable the disable components flag? wouldn't just not setting it mean that it's not enabled?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, that would not work

Copy link
Member Author

Choose a reason for hiding this comment

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

The operator needs to be shipped with preset fips-disabled-components and the components should be disabled only when running on FIPS platform. Customers don't have option to modify this flag when the operator is installed via OLM.

Copy link
Contributor

Choose a reason for hiding this comment

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

is the worry that if it's always settable that a user could override the disabled components? I'm trying to understand why it should only be disabled on FIPS Platform vs a user trying to disable components on any platform. I don't quite understand the distinction/requirement. And in this case if it's going to be shipped with preset components should this be a build argument?

Copy link
Member Author

Choose a reason for hiding this comment

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

And in this case if it's going to be shipped with preset components should this be a build argument?

It is going to be shipped with preset flag in the bundle/CSV.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah i see. okay. I'm still not sure why this means we can't just have it be disabled-components and have it be required if FIPS is true and optional if its set for the rest of the bundle though. I don't think it's blocking though.

internal/fips/check.go Outdated Show resolved Hide resolved
@pavolloffay
Copy link
Member Author

@jaronoff97 I have updated the PR with the requested changes

internal/fips/fipscheck.go Outdated Show resolved Hide resolved
apis/v1beta1/collector_webhook_test.go Outdated Show resolved Hide resolved
apis/v1beta1/collector_webhook.go Outdated Show resolved Hide resolved
internal/fips/fipscheck.go Outdated Show resolved Hide resolved
internal/fips/fipscheck.go Outdated Show resolved Hide resolved
@pavolloffay
Copy link
Member Author

@jaronoff97 PR updated. I have removed the noopFips and used nil

Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Copy link
Contributor

@jaronoff97 jaronoff97 left a comment

Choose a reason for hiding this comment

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

one comment, otherwise looks good!

internal/fips/fipscheck.go Outdated Show resolved Hide resolved
Signed-off-by: Pavol Loffay <[email protected]>
@pavolloffay pavolloffay merged commit 05c901f into open-telemetry:main Oct 2, 2024
33 checks passed
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