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.

Sign up for GitHub

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

[Elastic-Agent] Use internal port for local fleet server #28993

Merged
merged 7 commits into from
Nov 23, 2021
Merged

[Elastic-Agent] Use internal port for local fleet server #28993

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f223462
use itnernal port for local fleet server
michalpristas Nov 15, 2021
cfcc3f0
Merge branch 'master' of github.com:elastic/beats into use-secondary-…
michalpristas Nov 16, 2021
f9a3e4b
internal on localhost
michalpristas Nov 18, 2021
fa9f592
remove unwanted exports from spec
michalpristas Nov 19, 2021
3607c83
fixed override
michalpristas Nov 19, 2021
27400e8
log message
michalpristas Nov 19, 2021
778c564
log level
michalpristas Nov 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions x-pack/elastic-agent/pkg/agent/cmd/enroll.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package cmd
import (
"context"
"fmt"
"net"
"os"
"os/signal"
"path/filepath"
Expand Down Expand Up @@ -291,6 +292,8 @@ func enroll(streams *cli.IOStreams, cmd *cobra.Command, args []string) error {
fPolicy, _ := cmd.Flags().GetString("fleet-server-policy")
fHost, _ := cmd.Flags().GetString("fleet-server-host")
fPort, _ := cmd.Flags().GetUint16("fleet-server-port")
fInternalHost, _ := cmd.Flags().GetString("fleet-server-internal-host")
fInternalPort, _ := cmd.Flags().GetUint16("fleet-server-internal-port")
fCert, _ := cmd.Flags().GetString("fleet-server-cert")
fCertKey, _ := cmd.Flags().GetString("fleet-server-cert-key")
fInsecure, _ := cmd.Flags().GetBool("fleet-server-insecure-http")
Expand All @@ -308,6 +311,24 @@ func enroll(streams *cli.IOStreams, cmd *cobra.Command, args []string) error {

ctx := handleSignal(context.Background())

if localFleetServer := fServer != ""; localFleetServer {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you move this to a separate function and then have some unit tests on it to validate the behaviour?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that port should be configurable. What if the user is already using that port for any other process running?

Copy link
Contributor

Choose a reason for hiding this comment

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

Further down a random port is selected that is free. So this conflict should never happen.

Copy link

Choose a reason for hiding this comment

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

This line is really hard to understand. Can you break it up somehow?

Copy link
Contributor Author

@michalpristas michalpristas Nov 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

i made it configurable to make the option available for configuring port. if user wants to have it on some managed port. i can take it out if that's something not important atm.

if fInternalHost == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the use case where we would not use localhost? Should we even support something else?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fleet-server and its own Elastic Agent are always gonna be on the same host aren't they?

Copy link
Contributor

Choose a reason for hiding this comment

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

It must be on the same host. Elastic Agent starts fleet-server as a process.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then it will always be a localhost communication. Gotcha.

Copy link

Choose a reason for hiding this comment

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

Should always be on localhost, however making it overridable could be useful for testing.

fInternalHost = "localhost"
}

if fInternalPort <= 0 {
randomPort, err := getRandomPort()
if err != nil {
return err
}
fInternalPort = randomPort
}
} else {
// don't use without local fleet server
fInternalHost = ""
fInternalPort = 0
}

options := enrollCmdOption{
EnrollAPIKey: enrollmentToken,
URL: url,
Expand Down Expand Up @@ -336,6 +357,8 @@ func enroll(streams *cli.IOStreams, cmd *cobra.Command, args []string) error {
SpawnAgent: !fromInstall,
Headers: mapFromEnvList(fHeaders),
Timeout: fTimeout,
InternalHost: fInternalHost,
InternalPort: fInternalPort,
},
}

Expand Down Expand Up @@ -384,3 +407,19 @@ func mapFromEnvList(envList []string) map[string]string {
}
return m
}

func getRandomPort() (uint16, error) {
Copy link

Choose a reason for hiding this comment

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

This works on all os's? Seems risky to me. Assuming that the OS will allow a rebind in a short period of time and/or won't decide to be aggressive and reuse port elsewhere.

Can we just default to 8221 or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i was thinking also about reversal flow, and if we agree that having this configurable is ok.
8221 is ok as well, seems it's not used by a lot of services. initially i wanted to avoid low ports, but i'm ok with this as well

addr, err := net.ResolveTCPAddr("tcp", "localhost:0")
if err != nil {
return 0, err
}

l, err := net.ListenTCP("tcp", addr)
if err != nil {
return 0, err
}
port := l.Addr().(*net.TCPAddr).Port
l.Close()

return uint16(port), nil
}
20 changes: 20 additions & 0 deletions x-pack/elastic-agent/pkg/agent/cmd/enroll_cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ type enrollCmdFleetServerOption struct {
PolicyID string
Host string
Port uint16
InternalHost string
InternalPort uint16
Cert string
CertKey string
Insecure bool
Expand All @@ -91,6 +93,7 @@ type enrollCmdFleetServerOption struct {
// enrollCmdOption define all the supported enrollment option.
type enrollCmdOption struct {
URL string `yaml:"url,omitempty"`
InternalURL string `yaml:"-"`
CAs []string `yaml:"ca,omitempty"`
CASha256 []string `yaml:"ca_sha256,omitempty"`
Insecure bool `yaml:"insecure,omitempty"`
Expand Down Expand Up @@ -307,6 +310,7 @@ func (c *enrollCmd) fleetServerBootstrap(ctx context.Context, persistentConfig m
c.options.FleetServer.ConnStr, c.options.FleetServer.ServiceToken,
c.options.FleetServer.PolicyID,
c.options.FleetServer.Host, c.options.FleetServer.Port,
c.options.FleetServer.InternalHost, c.options.FleetServer.InternalPort,
c.options.FleetServer.Cert, c.options.FleetServer.CertKey, c.options.FleetServer.ElasticsearchCA,
c.options.FleetServer.Headers,
c.options.ProxyURL,
Expand Down Expand Up @@ -401,6 +405,11 @@ func (c *enrollCmd) prepareFleetTLS() error {
if c.options.URL == "" {
return errors.New("url is required when a certificate is provided")
}

if c.options.FleetServer.InternalPort > 0 {
c.options.InternalURL = fmt.Sprintf("%s:%d", c.options.FleetServer.InternalHost, c.options.FleetServer.InternalPort)
}

return nil
}

Expand Down Expand Up @@ -505,6 +514,7 @@ func (c *enrollCmd) enroll(ctx context.Context, persistentConfig map[string]inte
c.options.FleetServer.ConnStr, c.options.FleetServer.ServiceToken,
c.options.FleetServer.PolicyID,
c.options.FleetServer.Host, c.options.FleetServer.Port,
c.options.FleetServer.InternalHost, c.options.FleetServer.InternalPort,
c.options.FleetServer.Cert, c.options.FleetServer.CertKey, c.options.FleetServer.ElasticsearchCA,
c.options.FleetServer.Headers,
c.options.ProxyURL, c.options.ProxyDisabled, c.options.ProxyHeaders,
Expand All @@ -516,6 +526,10 @@ func (c *enrollCmd) enroll(ctx context.Context, persistentConfig map[string]inte
// no longer need bootstrap at this point
serverConfig.Server.Bootstrap = false
fleetConfig.Server = serverConfig.Server
// use internal URL for future requests
if c.options.InternalURL != "" {
fleetConfig.Client.Host = c.options.InternalURL
}
}

configToStore := map[string]interface{}{
Expand Down Expand Up @@ -837,6 +851,7 @@ func storeAgentInfo(s saver, reader io.Reader) error {
func createFleetServerBootstrapConfig(
connStr, serviceToken, policyID, host string,
port uint16,
internalHost string, internalPort uint16,
cert, key, esCA string,
headers map[string]string,
proxyURL string,
Expand Down Expand Up @@ -888,6 +903,11 @@ func createFleetServerBootstrapConfig(
Host: host,
Port: port,
}
if localFleetServer {
cfg.Server.InternalHost = internalHost
cfg.Server.InternalPort = internalPort
}

if policyID != "" {
cfg.Server.Policy = &configuration.FleetServerPolicyConfig{ID: policyID}
}
Expand Down
14 changes: 8 additions & 6 deletions x-pack/elastic-agent/pkg/agent/configuration/fleet_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,14 @@ import (

// FleetServerConfig is the configuration written so Elastic Agent can run Fleet Server.
type FleetServerConfig struct {
Bootstrap bool `config:"bootstrap" yaml:"bootstrap,omitempty"`
Policy *FleetServerPolicyConfig `config:"policy" yaml:"policy,omitempty"`
Output FleetServerOutputConfig `config:"output" yaml:"output,omitempty"`
Host string `config:"host" yaml:"host,omitempty"`
Port uint16 `config:"port" yaml:"port,omitempty"`
TLS *tlscommon.Config `config:"ssl" yaml:"ssl,omitempty"`
Bootstrap bool `config:"bootstrap" yaml:"bootstrap,omitempty"`
Policy *FleetServerPolicyConfig `config:"policy" yaml:"policy,omitempty"`
Output FleetServerOutputConfig `config:"output" yaml:"output,omitempty"`
Host string `config:"host" yaml:"host,omitempty"`
Port uint16 `config:"port" yaml:"port,omitempty"`
InternalHost string `config:"internal_host" yaml:"internal_host,omitempty"`
InternalPort uint16 `config:"internal_port" yaml:"internal_port,omitempty"`
TLS *tlscommon.Config `config:"ssl" yaml:"ssl,omitempty"`
}

// FleetServerPolicyConfig is the configuration for the policy Fleet Server should run on.
Expand Down
2 changes: 1 addition & 1 deletion x-pack/elastic-agent/pkg/agent/program/supported.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions x-pack/elastic-agent/spec/fleet-server.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ rules:
selectors:
- fleet.server.host
- fleet.server.port
- fleet.server.internal_host
Copy link

Choose a reason for hiding this comment

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

still need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, thanks

- fleet.server.internal_port
- fleet.server.ssl
path: inputs.0.server

Expand Down