Update go docker #309
Update go docker #309
Conversation
nathanlaceyraft
force-pushed
the
update-go-docker
branch
2 times, most recently
from
09c725c to
18f3bcc
Compare
fix issues with -race sonarqube fixes remove dups set docker go version explicit
nathanlaceyraft
force-pushed
the
update-go-docker
branch
from
18f3bcc to
fac5d9f
Compare
|
There was a problem hiding this comment.
I appreciate the intent behind the -race-related portions of this PR, but I think the scope has become cluttered, and the original intent (dropping the digest-specific reference to the golang base image) is inconsistent with the security posture of this project. I'm particularly grateful that you drew my attention to -race, since a recent PR from Dependabot had actually failed due to concurrent read and write of the metrics map.
I've opened a new issue to track the data race, and I'll add -race to the test targets as part of addressing that issue. Assuming I understand the collective intents of this PR, that plus Dependabot's existing configuration should cover everything. If I'm wrong, please let me know.
| @@ -1,4 +1,4 @@ | |||
| FROM --platform=$BUILDPLATFORM golang:1.23.4@sha256:574185e5c6b9d09873f455a7c205ea0514bfd99738c5dc7750196403a44ed4b7 AS build | |||
| FROM --platform=$BUILDPLATFORM golang:1.23.5 AS build | |||
There was a problem hiding this comment.
This is an intentional supply-chain integrity feature with updates managed by Dependabot.
There was a problem hiding this comment.
So why is Dependabot not requiring you to update to 1.23.5?
1.23.4 has security vulnerabilities resolved with 1.23.5
| @@ -19,7 +19,7 @@ generate: | |||
| .PHONY: test | |||
| test: generate vet envtest | |||
| KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(BIN) -p path)" \ | |||
| go test -v ./... -ginkgo.label-filter="!e2e && !broken" | |||
| go test -race -v ./... -ginkgo.label-filter="!e2e && !broken" | |||
There was a problem hiding this comment.
The original intent of this PR is to update the base Docker image, so this feels like substantial scope creep.
| @@ -37,6 +37,11 @@ vet: helm-lint | |||
| build: generate | |||
| go build -o bin/konfirm main.go | |||
|
|
|||
| # Build service with data race detector see https://go.dev/doc/articles/race_detector | |||
| .PHONY: build-race | |||
| build-race: generate | |||
There was a problem hiding this comment.
What is the use case for this? When would we want to build with -race?
There was a problem hiding this comment.
In development, (on your local machine).
You should be running binaries with -race.
Because we probably don't have 100% unit test coverage.
Exercising more of the code with -race (running the binary) increases your changes of detecting a data race condition.
| @@ -42,9 +44,21 @@ func (metrics Metrics) GetGaugeWithLabels(name string, labels prometheus.Labels) | |||
| return nil | |||
| } | |||
|
|
|||
| func (metrics Metrics) GetGaugeWithLabelsMutex(name string, labels prometheus.Labels, mux *sync.RWMutex) *dto.Gauge { | |||
There was a problem hiding this comment.
I understand the need to resolve the data race, but extending/modifying a dependency's API seems like a code smell/questionable practice.
There was a problem hiding this comment.
I didn't originally do it this way.
But putting mutex.RLock(). mutex.RUnlock(). all over the place within the unit test, one increased the chance of missing something. Also SonarQube complained about the code duplication.
Creating the functions to wrap the mutex decreased the code duplication from 55% to 14%
And personally I think it made the code more readable.
I could have made the functions within the unit test, but that didn't seem right
|
Agree about the scope creep in the PR. Started with wanting to fix the security vulnerabilities in Docker container. And saw you weren't using -race within your unit tests. Than saw your unit tests had a data race condition ... I can remove the docker update, and turn PR into just a -race fix. If the base implemenation doesn't need mutex support, I don't know that creating a struct to add a mutex (just for a unit test) is better than simply adding 2 functions to support the mutex needed for the unit test. |
Update Dockerfile for floating current go 1.23
this trades a repeatable build for automatically keeping up with current security patch releases
create make target to build using -race
Add -race to unit tests to automatically detect data race conditions.
Fix unit test to pass with -race