-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
FIPS Compliance: Refactor Entrypoint, Remove zap Dependency & Update Build Checks #8544
FIPS Compliance: Refactor Entrypoint, Remove zap Dependency & Update Build Checks #8544
Conversation
/kind-cleanup |
b4c40b2
to
d2c70d5
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
/test check-pr-has-kind-label |
@PuneetPunamiya: The specified target(s) for
The following commands are available to trigger optional jobs:
Use In response to this:
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/test-infra repository. |
/retest |
/test check-pr-has-kind-label |
@PuneetPunamiya: The specified target(s) for
The following commands are available to trigger optional jobs:
Use In response to this:
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/test-infra repository. |
/test all |
/kind cleanup |
The following is the coverage report on the affected files.
|
@@ -311,7 +307,8 @@ func (e Entrypointer) Go() error { | |||
resultPath = e.ResultsDirectory | |||
} | |||
if err := e.readResultsFromDisk(ctx, resultPath, result.TaskRunResultType); err != nil { | |||
logger.Fatalf("Error while handling results: %s", err) | |||
slog.Error("Error while substituting step artifacts: ", slog.Any("error", err)) | |||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why log.Fatalf
is replaced with logger.Fatalf
in other places, but why are slog.Error
used in these two instances?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good catch, I did use log.Fatalf
but the issue was it was giving golang lint error as log.Fatalf will exit, and defer cancel() will not run
, hence I did use slog.Error
11834ef
to
d2c70d5
Compare
The following is the coverage report on the affected files.
|
/retest |
The following is the coverage report on the affected files.
|
54b6e02
to
2b76a2d
Compare
The following is the coverage report on the affected files.
|
2b76a2d
to
e6ffdb9
Compare
The following is the coverage report on the affected files.
|
/retest |
e6ffdb9
to
fd1176e
Compare
The following is the coverage report on the affected files.
|
/hold cancel |
go-version-file: "go.mod" | ||
- name: build | ||
run: | | ||
go build -v -tags "disable_spire,disable_tls" ./cmd/entrypoint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maye be we can check the build for tls symbols ?
go tool nm bin/entrypoint | grep -E 'tls'
Or may be we need to check for crypto once Vibhav PR is merged
go tool nm bin/entrypoint | grep -E 'crypto|tls'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkhelil yeah I think we could do that in a follow-up PR.
fd1176e
to
e627e0d
Compare
The following is the coverage report on the affected files.
|
/approve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PuneetPunamiya can you rebase against main
branch to make sure it is not gonna cause issues with code generation ?
.github/workflows/ci.yaml
Outdated
@@ -26,6 +26,18 @@ jobs: | |||
- name: build | |||
run: | | |||
go build -v ./... | |||
buildWithoutTlsAndSpire: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the back and forth. And we could probably do this in a follow-up as well, but I feel we could name this buildFips
😛
Signed-off-by: PuneetPunamiya <[email protected]>
Signed-off-by: PuneetPunamiya <[email protected]>
Signed-off-by: PuneetPunamiya <[email protected]>
This will help to verify the build for entrypoint using `disable_tls` and `disable_spire` flags Signed-off-by: PuneetPunamiya <[email protected]>
e627e0d
to
93d3b76
Compare
The following is the coverage report on the affected files.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jkhelil, vdemeester The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
apis/pipeline/v1/types
disable_tls
flagFixes: #8531 (one part of the issue)
Changes
corev1
API package is now split because it imports crypto-related functionsdisable_tls
to conditionally exclude crypto related dependenciesSubmitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes