ROX-24283: enable strictfipsruntime in the Konflux build #1709
Conversation
BradLugo
changed the title
BradLugo
force-pushed
the
blugo/ROX-24283-strictfipsruntime
branch
from
8126dfc to
8086210
Compare
BradLugo
force-pushed
the
blugo/ROX-24283-strictfipsruntime
branch
from
8086210 to
2c51901
Compare
|
/retest scanner-on-push |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
/retest scanner-slim-on-push |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@davdhacs do you mind double-checking the images listed in the description? |
| ENV GOEXPERIMENT=strictfipsruntime | ||
| ENV GOTAGS=strictfipsruntime |
There was a problem hiding this comment.
I forgot: do these both GOEXPERIMENT=strictfipsruntime env variable and -tags=strictfipsruntime argument need to be provided at the same time?
There was a problem hiding this comment.
I think we do not want the tag set because "The -tag method can technically work on 1.17+, but the environment variable method is preferred." (https://docs.google.com/presentation/d/10O9MIItuPW1fCN4dPaGTxSHAE4KhqISMKAxNGdmPyV4/edit#slide=id.g227165bb50a_0_0)
check-payload accepts the tag for golang <=1.16 and goexperiment var for golang >=1.17 openshift/check-payload#143 (comment)
🤔 The code appears to check for tags on golang < 1.18. So 1.17 may be incorrectly checked, but we're not concerned with 1.17 since we're on 1.20+ (https://github.com/openshift/check-payload/pull/143/files#diff-e8b02dd31a1900ab2922d2408ab4727fdc85e7cafe302133d125ec0a8062efd6R114)
discussion in https://issues.redhat.com/browse/OCPBUGS-24612
and that matches the slides (https://docs.google.com/presentation/d/10O9MIItuPW1fCN4dPaGTxSHAE4KhqISMKAxNGdmPyV4/edit#slide=id.g227165bb50a_0_0):
For Go 1.17+:
As an environment variable: "GOEXPERIMENT=strictfipsruntime"
Setting this environment variable is preferred since it can be done trivially at build time without being concerned with each go build invocation.
For Go 1.16 or prior:
As a -tags parameter "-tags=strictfipsruntime" .
The environment variable method is not supported. The -tag method can technically work on 1.17+, but the environment variable method is preferred.
There was a problem hiding this comment.
From my reading of the info you shared, for us it should be sufficient to only keep GOEXPERIMENT=strictfipsruntime environment variable. Since you achieved the state when check-payload is happy about our images, could you please try removing -tags=strictfipsruntime/GOTAGS=strictfipsruntime and running the tool against the results?
There was a problem hiding this comment.
I tried doing this in stackrox/stackrox#12909, but only setting GOEXPERIMENT didn't move the needle (hence this commit: stackrox/stackrox@0e094ea)
There was a problem hiding this comment.
Perhaps the GOEXPERIMENT variable is not getting carried through and we're relying on the tags?
There was a problem hiding this comment.
Weird. If -tags=strictfipsruntime/GOTAGS=strictfipsruntime has the effect, maybe we don't need GOEXPERIMENT=strictfipsruntime?
There was a problem hiding this comment.
It looks like GOEXPERIMENT is not found on the built binaries. So I think that is why the tags are needed right now.
There was a problem hiding this comment.
I'll try checking in a way separate from check-payload. Maybe check-payload is wrong.
There was a problem hiding this comment.
I'll try checking in a way separate from check-payload. Maybe check-payload is wrong.
This. I'll report the issue to check-payload.
There was a problem hiding this comment.
What's the conclusion of this? Just keeping both?
| ENV GOEXPERIMENT=strictfipsruntime | ||
| ENV GOTAGS=strictfipsruntime |
There was a problem hiding this comment.
+1 to @davdhacs suggestion to add a # TODO(ROX-1234): reminder for us to remove one or the other thing.
| @@ -10,6 +10,9 @@ ARG SCANNER_TAG | |||
| RUN if [[ "$SCANNER_TAG" == "" ]]; then >&2 echo "error: required SCANNER_TAG arg is unset"; exit 6; fi | |||
| ENV RELEASE_TAG="${SCANNER_TAG}" | |||
|
|
|||
| # TODO(ROX-27054): Remove the redundant strictfipsruntime option if one is found to be so | |||
There was a problem hiding this comment.
I don't love the way this comment reads, but it's the best I could come up with.
There was a problem hiding this comment.
lgtm. I'd prefer "remove -tags=strictfipsruntime when GOEXPERIMENT is checked by check-payload" but that may be too much information.
There was a problem hiding this comment.
Honestly I think I prefer the more info in David's suggestion (perhaps even add a link to check-payload so readers can find it)
| @@ -71,7 +71,7 @@ GOPATH_WD_OVERRIDES := -w /src -e GOPATH=/go | |||
| IMAGE_BUILD_FLAGS := -e CGO_ENABLED=$(CGO_ENABLED) -e GOOS=linux -e GOARCH=$(GOARCH) | |||
| IMAGE_BUILD_ARGS = --build-arg LABEL_VERSION=$(TAG) --build-arg LABEL_RELEASE=$(TAG) --build-arg QUAY_TAG_EXPIRATION=$(QUAY_TAG_EXPIRATION) | |||
| BUILD_FLAGS := CGO_ENABLED=$(CGO_ENABLED) GOOS=linux GOARCH=$(GOARCH) | |||
| BUILD_CMD := go build -trimpath -ldflags="-X github.com/stackrox/scanner/pkg/version.Version=$(TAG)" -o image/scanner/bin/scanner ./cmd/clair | |||
| BUILD_CMD := go build -trimpath -ldflags="-X github.com/stackrox/scanner/pkg/version.Version=$(TAG)" -tags=$(GOTAGS) -o image/scanner/bin/scanner ./cmd/clair | |||
| NODESCAN_BUILD_CMD := go build -trimpath -o tools/bin/local-nodescanner ./tools/local-nodescanner | |||
There was a problem hiding this comment.
It does not seem to me that local-nodescanner is included in the final Scanner image. Is this correct?
Is it a development tool? Does anyone still use it?
@Maddosaurus
There was a problem hiding this comment.
It's a development tool. From what I can tell, the (old) RHCOS node scanner runs as a container in the collector pod called node-inventory which runs the usual scanner binary /scanner from within the usual scanner image ._rox.image.scanner.fullRef https://github.com/stackrox/stackrox/blob/master/image/templates/helm/stackrox-secured-cluster/templates/collector.yaml.htpl#L196
Looks like the typical scanner main function was modified to run the scanner in "node inventory mode" - https://github.com/stackrox/scanner/blob/master/cmd/clair/main.go#L279
So I'd say it's accurate to keep this command out
There was a problem hiding this comment.
Yes, this is correct to not be included in the image.
local-nodescanner is a tool that enables local testing and debugging of Scanner v2 Node Scanning code without the need to deploy ACS.
It is similar to the tools for collector and sensor we have over in the main stackrox repo 😄
There was a problem hiding this comment.
Thank you Ross and Matthias, while you answered about FIPS part for the local-nodescanner, it will be good to know whether we need to keep and maintain this tool. Therefore:
Does anyone still use it?
Is there any value to keep it?
There was a problem hiding this comment.
As long as we support Scanner V2 Node Scanning, this tool should stay in place to enable effective debugging and troubleshooting of Node Scanning code.
| @@ -71,7 +71,7 @@ GOPATH_WD_OVERRIDES := -w /src -e GOPATH=/go | |||
| IMAGE_BUILD_FLAGS := -e CGO_ENABLED=$(CGO_ENABLED) -e GOOS=linux -e GOARCH=$(GOARCH) | |||
| IMAGE_BUILD_ARGS = --build-arg LABEL_VERSION=$(TAG) --build-arg LABEL_RELEASE=$(TAG) --build-arg QUAY_TAG_EXPIRATION=$(QUAY_TAG_EXPIRATION) | |||
| BUILD_FLAGS := CGO_ENABLED=$(CGO_ENABLED) GOOS=linux GOARCH=$(GOARCH) | |||
| BUILD_CMD := go build -trimpath -ldflags="-X github.com/stackrox/scanner/pkg/version.Version=$(TAG)" -o image/scanner/bin/scanner ./cmd/clair | |||
| BUILD_CMD := go build -trimpath -ldflags="-X github.com/stackrox/scanner/pkg/version.Version=$(TAG)" -tags=$(GOTAGS) -o image/scanner/bin/scanner ./cmd/clair | |||
| NODESCAN_BUILD_CMD := go build -trimpath -o tools/bin/local-nodescanner ./tools/local-nodescanner | |||
|
|
|||
There was a problem hiding this comment.
(not about this line) Similarly there's updater which does not seem to be included in the image and so doesn't need to be FIPS-built. Is it correct?
Lines 117 to 120 in ff7e0f3
There was a problem hiding this comment.
This updater just runs in CI and isn't shipped to customers, so no need to be FIPS-built
Co-authored-by: Misha Sugakov <[email protected]>
|
@BradLugo: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest scanner-slim-on-push scanner-db-slim-on-push |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
/retest scanner-on-push |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
/retest scanner-on-push |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
/retest scanner-db-on-push |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
/retest scanner-slim-on-push |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
/retest scanner-db-slim-on-push |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@BradLugo Looks like Konflux builds completed fine. Don't worry about |
Description
Enables the strictfipsruntime build flag for the Konflux build.
For more info about the strictfipsruntime flag, see this doc (there might be a better resource but this is the one David and I found), and for our general research regarding the linked ticket, see this doc.
Related stackrox PR: stackrox/stackrox#12909
Verified the build works via Konflux CI and verified the
check-payloadresults of the following images:quay.io/rhacs-eng/scanner:2.35.x-13-g2c519017f0-fastquay.io/rhacs-eng/scanner-slim:2.35.x-13-g2c519017f0-fast