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

Update beats import to v7.17.20 #3473

Merged
merged 3 commits into from
Apr 23, 2024
Merged

Conversation

michel-laterman
Copy link
Contributor

What is the problem this PR solves?

beats v7.12.0 introduces major changes to how TLS is handled and upgrading causes bootstapping to fail

How does this PR solve the problem?

Update beats import to v7.17.20 and fix how the api tls server is initiated to prevent #3435.

How to test this PR locally

bootstrap fleet-server built from this pr with an elastic-agent + cluster running v7.17.21-SNAPSHOT

Related issues

Update beats import to v7.17.20 and fix how the api tls server is
initiated to prevent issue/3435.
@michel-laterman michel-laterman added bug Something isn't working backport Team:Fleet Label for the Fleet team labels Apr 17, 2024
@michel-laterman michel-laterman requested review from a team as code owners April 17, 2024 23:49
@cmacknz
Copy link
Member

cmacknz commented Apr 23, 2024

What was the explanation for why this broke self-signed certificates? Why this fixes it still isn't clear to me even though the change is small.

Copy link
Contributor

@juliaElastic juliaElastic left a comment

Choose a reason for hiding this comment

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

LGTM, could the changes be tested locally?

@michel-laterman
Copy link
Contributor Author

@juliaElastic, I've tested by running fleet-server under agent in a VM against a QA snapshot deployment, do you think that is sufficient or should I run with a locally managed instance of ES as well?

@juliaElastic
Copy link
Contributor

@michel-laterman I think we should test that fleet-server can bootstrap with self-signed certificates, did you test cover that? If so, that should be good.

@michel-laterman
Copy link
Contributor Author

Yes,I tested and also got @kaanyalti to test locally as well

@michel-laterman michel-laterman merged commit 901659b into elastic:7.17 Apr 23, 2024
5 of 6 checks passed
@michel-laterman michel-laterman deleted the fix-3435 branch April 23, 2024 17:13
if err != nil {
return err
}
server.TLSConfig = commonTlsCfg.ToConfig()
server.TLSConfig = commonTLSCfg.BuildServerConfig(cfg.Host)
Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/elastic/fleet-server/blob/4e3c8b0e571e96921ab7c69c30ab8a857a4690d3/cmd/fleet/server.go#L106-L118

This is now the same as main so 👍

			commonTLSCfg, err := tlscommon.LoadTLSServerConfig(cfg.TLS)
			if err != nil {
				return err
			}
			server.TLSConfig = commonTLSCfg.BuildServerConfig(cfg.Host)

			// Must enable http/2 in the configuration explicitly.
			// (see https://golang.org/pkg/net/http/#Server.Serve)
			server.TLSConfig.NextProtos = []string{"h2", "http/1.1"}

			ln = tls.NewListener(ln, server.TLSConfig)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BuildServerConfig function was introduced in beats 7.12.x when we updated the required version of go to 1.15.x
It specifies a custom VerifyConnection attribute that is need for the changes with certificate verification introduced in that version of go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport bug Something isn't working Team:Fleet Label for the Fleet team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants