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

Bump consul/proto-public and create mocks #406

Merged
merged 5 commits into from
Feb 5, 2024
Merged

Bump consul/proto-public and create mocks #406

merged 5 commits into from
Feb 5, 2024

Conversation

jjti
Copy link
Contributor

@jjti jjti commented Feb 1, 2024

Changes

This pulls in the latest public proto specifications from consul's proto-public package. We need this for a subsequent PR that references the newly created TelemetryState struct: #370

Bumping proto-public breaks the tests in pkg/dns/dns_test.go because the referenced mock is no longer exported. To get around that, while keeping the dns changes largely unchanged, a new mockery mock is created using the newly added make target below:

# This generates mocks against public proto packages in consul. At the time of writing,
# only the dns and resource packages are used in consul-dataplane so only mocks for their
# interfaces are generated here.
.PHONY: mocks
mocks:
	for pkg in pbdns pbresource; do \
		mockery --srcpkg=github.com/hashicorp/consul/proto-public/$$pkg --output ./internal/mocks/$${pkg}mock --outpkg $${pkg}mock --case underscore --all; \
	done

The pbresource mocks are also added here as they are used in the next PR.

Testing

These changes were pulled out of the follow-up PR where they were tested by building + deploying a custom build of consul-dataplane using these changes.

@@ -40,7 +41,7 @@ func genRandomBytes(size int) (blk []byte) {
}

func (s *DNSTestSuite) Test_DisabledServer() {
mockedDNSConsulClient := pbdns.NewMockDNSServiceClient(s.T())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NewMockDNSServiceClient no longer exists in github.com/hashicorp/consul/proto-public/pbdns

@jjti jjti marked this pull request as ready for review February 1, 2024 19:19
@jjti jjti requested a review from a team as a code owner February 1, 2024 19:19
@jjti jjti requested a review from wilkermichael February 1, 2024 19:31
@wilkermichael
Copy link
Contributor

@jjti qq: is this meant to be supported in the next release (Consul 1.18/dataplane 1.14)?

@jjti
Copy link
Contributor Author

jjti commented Feb 1, 2024

@jjti qq: is this meant to be supported in the next release (Consul 1.18/dataplane 1.14)?

@wilkermichael yes that's correct

@wilkermichael
Copy link
Contributor

wilkermichael commented Feb 1, 2024

Ok, we'll need to add a backport label, just waiting for it to be created. The release process is still ongoing for dataplane release/1.4.x

@wilkermichael wilkermichael added the backport/1.4 Changes are backported to 1.4 label Feb 1, 2024
Copy link
Contributor

@DanStough DanStough left a comment

Choose a reason for hiding this comment

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

LGTM and thanks for this fix! See my comment about the changelog.

One rhetorical point that has nothing to do with this PR: it's weird we got rid of the mocks to cut down on dependencies in proto-public, but we ended up pulling in a ton of K8s dependencies. 🤷

.changelog/406.txt Outdated Show resolved Hide resolved
@jjti jjti added the pr/no-changelog This PR does not introduce a user-facing change that should be reflected in the changelog label Feb 2, 2024
@jjti
Copy link
Contributor Author

jjti commented Feb 2, 2024

One rhetorical point that has nothing to do with this PR: it's weird we got rid of the mocks to cut down on dependencies in proto-public, but we ended up pulling in a ton of K8s dependencies. 🤷

I hear you @DanStough and that's something I'll call out here: it appears to increase the consul-dataplane binary size ~50% (19M > 29M):

# josh @ josh-XWGWHJ0JV2 in ~/GitHub/hashicorp/consul-dataplane on git:bump-protos
$ make bin
GOARCH=arm64 GOOS=linux CGO_ENABLED=0 go build -trimpath -buildvcs=false -ldflags="-X github.com/hashicorp/consul-dataplane/pkg/version.GitCommit=6ddd5cc" -o dist/linux/arm64/consul-dataplane ./cmd/consul-dataplane

# josh @ josh-XWGWHJ0JV2 in ~/GitHub/hashicorp/consul-dataplane on git:jjti/bump-protos o [16:26:52] 
$ ll dist/linux/arm64/consul-dataplane                                    
-rwxr-xr-x  1 josh  staff    29M Feb  2 16:22 dist/linux/arm64/consul-dataplane

# josh @ josh-XWGWHJ0JV2 in ~/GitHub/hashicorp/consul-dataplane on git:main o [16:33:17] 
$ make bin
GOARCH=arm64 GOOS=linux CGO_ENABLED=0 go build -trimpath -buildvcs=false -ldflags="-X github.com/hashicorp/consul-dataplane/pkg/version.GitCommit=6ddd5cc" -o dist/linux/arm64/consul-dataplane ./cmd/consul-dataplane

# josh @ josh-XWGWHJ0JV2 in ~/GitHub/hashicorp/consul-dataplane on git:main o [16:33:26] 
$ ll dist/linux/arm64/consul-dataplane
-rwxr-xr-x  1 josh  staff    19M Feb  2 16:33 dist/linux/arm64/consul-dataplane

Similar/same increase image for the image: 139MB to 150MB

@jjti jjti requested a review from DanStough February 2, 2024 21:40
@jjti
Copy link
Contributor Author

jjti commented Feb 5, 2024

One rhetorical point that has nothing to do with this PR: it's weird we got rid of the mocks to cut down on dependencies in proto-public, but we ended up pulling in a ton of K8s dependencies. 🤷

I hear you @DanStough and that's something I'll call out here: it appears to increase the consul-dataplane binary size ~50% (19M > 29M):

# josh @ josh-XWGWHJ0JV2 in ~/GitHub/hashicorp/consul-dataplane on git:bump-protos
$ make bin
GOARCH=arm64 GOOS=linux CGO_ENABLED=0 go build -trimpath -buildvcs=false -ldflags="-X github.com/hashicorp/consul-dataplane/pkg/version.GitCommit=6ddd5cc" -o dist/linux/arm64/consul-dataplane ./cmd/consul-dataplane

# josh @ josh-XWGWHJ0JV2 in ~/GitHub/hashicorp/consul-dataplane on git:jjti/bump-protos o [16:26:52] 
$ ll dist/linux/arm64/consul-dataplane                                    
-rwxr-xr-x  1 josh  staff    29M Feb  2 16:22 dist/linux/arm64/consul-dataplane

# josh @ josh-XWGWHJ0JV2 in ~/GitHub/hashicorp/consul-dataplane on git:main o [16:33:17] 
$ make bin
GOARCH=arm64 GOOS=linux CGO_ENABLED=0 go build -trimpath -buildvcs=false -ldflags="-X github.com/hashicorp/consul-dataplane/pkg/version.GitCommit=6ddd5cc" -o dist/linux/arm64/consul-dataplane ./cmd/consul-dataplane

# josh @ josh-XWGWHJ0JV2 in ~/GitHub/hashicorp/consul-dataplane on git:main o [16:33:26] 
$ ll dist/linux/arm64/consul-dataplane
-rwxr-xr-x  1 josh  staff    19M Feb  2 16:33 dist/linux/arm64/consul-dataplane

Similar/same increase image for the image: 139MB to 150MB

There was a discussion about this in Slack. The gateway team will look at removing the generated code import of k8s to avoid this increase. Ie this increase in size, from this PR, is temporary until the next tagged release of consul/proto-public after which we'll be back to the size we'd been at before

@jjti jjti merged commit 82738d3 into main Feb 5, 2024
34 checks passed
@jjti jjti deleted the jjti/bump-protos branch February 5, 2024 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.4 Changes are backported to 1.4 pr/no-changelog This PR does not introduce a user-facing change that should be reflected in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants