From 925ec100d691662d129723072c0c3a7cae1862bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20=22Pisco=22=20Fernandes?= Date: Fri, 9 Jun 2023 18:17:04 +0100 Subject: [PATCH] TUN-7463: Add default ingress rule if no ingress rules are provided when updating the configuration --- cmd/cloudflared/tunnel/cmd.go | 6 ++--- ingress/ingress.go | 29 +++++++++++++++-------- orchestration/orchestrator.go | 17 ++++++++----- orchestration/orchestrator_test.go | 38 +++++++++++++++++++++++------- 4 files changed, 63 insertions(+), 27 deletions(-) diff --git a/cmd/cloudflared/tunnel/cmd.go b/cmd/cloudflared/tunnel/cmd.go index be11f24e..263283ef 100644 --- a/cmd/cloudflared/tunnel/cmd.go +++ b/cmd/cloudflared/tunnel/cmd.go @@ -399,7 +399,7 @@ func StartServer( } } - localRules := []ingress.Rule{} + internalRules := []ingress.Rule{} if features.Contains(features.FeatureManagementLogs) { serviceIP := c.String("service-op-ip") if edgeAddrs, err := edgediscovery.ResolveEdge(log, tunnelConfig.Region, tunnelConfig.EdgeIPVersion); err == nil { @@ -416,9 +416,9 @@ func StartServer( logger.ManagementLogger.Log, logger.ManagementLogger, ) - localRules = []ingress.Rule{ingress.NewManagementRule(mgmt)} + internalRules = []ingress.Rule{ingress.NewManagementRule(mgmt)} } - orchestrator, err := orchestration.NewOrchestrator(ctx, orchestratorConfig, tunnelConfig.Tags, localRules, tunnelConfig.Log) + orchestrator, err := orchestration.NewOrchestrator(ctx, orchestratorConfig, tunnelConfig.Tags, internalRules, tunnelConfig.Log) if err != nil { return err } diff --git a/ingress/ingress.go b/ingress/ingress.go index 56b3b1e7..e2498842 100644 --- a/ingress/ingress.go +++ b/ingress/ingress.go @@ -44,7 +44,7 @@ func (ing Ingress) FindMatchingRule(hostname, path string) (*Rule, int) { if err == nil { hostname = host } - for i, rule := range ing.LocalRules { + for i, rule := range ing.InternalRules { if rule.Matches(hostname, path) { // Local rule matches return a negative rule index to distiguish local rules from user-defined rules in logs // Full range would be [-1 .. ) @@ -77,7 +77,7 @@ func matchHost(ruleHost, reqHost string) bool { // Ingress maps eyeball requests to origins. type Ingress struct { // Set of ingress rules that are not added to remote config, e.g. management - LocalRules []Rule + InternalRules []Rule // Rules that are provided by the user from remote or local configuration Rules []Rule `json:"ingress"` Defaults OriginRequestConfig `json:"originRequest"` @@ -110,16 +110,18 @@ func ParseIngressFromConfigAndCLI(conf *config.Configuration, c *cli.Context, lo // --bastion for ssh bastion service ingressRules, err = parseCLIIngress(c, false) if errors.Is(err, ErrNoIngressRulesCLI) { - // Only log a warning if the tunnel is not a remotely managed tunnel and the config - // will be loaded after connecting. + // If no token is provided, the probability of NOT being a remotely managed tunnel is higher. + // So, we should warn the user that no ingress rules were found, because remote configuration will most likely not exist. if !c.IsSet("token") { log.Warn().Msgf(ErrNoIngressRulesCLI.Error()) } return newDefaultOrigin(c, log), nil } + if err != nil { return Ingress{}, err } + return ingressRules, nil } @@ -148,14 +150,10 @@ func parseCLIIngress(c *cli.Context, allowURLFromArgs bool) (Ingress, error) { // newDefaultOrigin always returns a 503 response code to help indicate that there are no ingress // rules setup, but the tunnel is reachable. func newDefaultOrigin(c *cli.Context, log *zerolog.Logger) Ingress { - noRulesService := newDefaultStatusCode(log) + defaultRule := GetDefaultIngressRules(log) defaults := originRequestFromSingleRule(c) ingress := Ingress{ - Rules: []Rule{ - { - Service: &noRulesService, - }, - }, + Rules: defaultRule, Defaults: defaults, } return ingress @@ -219,6 +217,17 @@ func (ing Ingress) CatchAll() *Rule { return &ing.Rules[len(ing.Rules)-1] } +// Gets the default ingress rule that will be return 503 status +// code for all incoming requests. +func GetDefaultIngressRules(log *zerolog.Logger) []Rule { + noRulesService := newDefaultStatusCode(log) + return []Rule{ + { + Service: &noRulesService, + }, + } +} + func validateAccessConfiguration(cfg *config.AccessConfig) error { if !cfg.Required { return nil diff --git a/orchestration/orchestrator.go b/orchestration/orchestrator.go index a45dbfc4..0d43e59d 100644 --- a/orchestration/orchestrator.go +++ b/orchestration/orchestrator.go @@ -28,8 +28,8 @@ type Orchestrator struct { lock sync.RWMutex // Underlying value is proxy.Proxy, can be read without the lock, but still needs the lock to update proxy atomic.Value - // Set of local ingress rules defined at cloudflared startup (separate from user-defined ingress rules) - localRules []ingress.Rule + // Set of internal ingress rules defined at cloudflared startup (separate from user-defined ingress rules) + internalRules []ingress.Rule warpRoutingEnabled atomic.Bool config *Config tags []tunnelpogs.Tag @@ -41,13 +41,13 @@ type Orchestrator struct { proxyShutdownC chan<- struct{} } -func NewOrchestrator(ctx context.Context, config *Config, tags []tunnelpogs.Tag, localRules []ingress.Rule, log *zerolog.Logger) (*Orchestrator, error) { +func NewOrchestrator(ctx context.Context, config *Config, tags []tunnelpogs.Tag, internalRules []ingress.Rule, log *zerolog.Logger) (*Orchestrator, error) { o := &Orchestrator{ // Lowest possible version, any remote configuration will have version higher than this // Starting at -1 allows a configuration migration (local to remote) to override the current configuration as it // will start at version 0. currentVersion: -1, - localRules: localRules, + internalRules: internalRules, config: config, tags: tags, log: log, @@ -116,8 +116,13 @@ func (o *Orchestrator) updateIngress(ingressRules ingress.Ingress, warpRouting i default: } - // Assign the local ingress rules to the parsed ingress - ingressRules.LocalRules = o.localRules + // Assign the internal ingress rules to the parsed ingress + ingressRules.InternalRules = o.internalRules + + // Check if ingress rules are empty, and add the default route if so. + if ingressRules.IsEmpty() { + ingressRules.Rules = ingress.GetDefaultIngressRules(o.log) + } // Start new proxy before closing the ones from last version. // The upside is we don't need to restart proxy from last version, which can fail diff --git a/orchestration/orchestrator_test.go b/orchestration/orchestrator_test.go index b9bac16a..b07b67bd 100644 --- a/orchestration/orchestrator_test.go +++ b/orchestration/orchestrator_test.go @@ -91,15 +91,15 @@ func TestUpdateConfiguration(t *testing.T) { "enabled": true, "connectTimeout": 10 } -} +} `) updateWithValidation(t, orchestrator, 2, configJSONV2) configV2 := orchestrator.config - // Validate local ingress rules - require.Equal(t, "management.argotunnel.com", configV2.Ingress.LocalRules[0].Hostname) - require.True(t, configV2.Ingress.LocalRules[0].Matches("management.argotunnel.com", "/ping")) - require.Equal(t, "management", configV2.Ingress.LocalRules[0].Service.String()) + // Validate internal ingress rules + require.Equal(t, "management.argotunnel.com", configV2.Ingress.InternalRules[0].Hostname) + require.True(t, configV2.Ingress.InternalRules[0].Matches("management.argotunnel.com", "/ping")) + require.Equal(t, "management", configV2.Ingress.InternalRules[0].Service.String()) // Validate ingress rule 0 require.Equal(t, "jira.tunnel.org", configV2.Ingress.Rules[0].Hostname) require.True(t, configV2.Ingress.Rules[0].Matches("jira.tunnel.org", "/login")) @@ -145,7 +145,7 @@ func TestUpdateConfiguration(t *testing.T) { { "originRequest": } - + `) resp = orchestrator.UpdateConfig(3, invalidJSON) @@ -165,7 +165,7 @@ func TestUpdateConfiguration(t *testing.T) { "warp-routing": { "enabled": false } -} +} `) updateWithValidation(t, orchestrator, 10, configJSONV10) configV10 := orchestrator.config @@ -204,12 +204,34 @@ func TestUpdateConfiguration_FromMigration(t *testing.T) { "warp-routing": { "enabled": true } -} +} `) updateWithValidation(t, orchestrator, 0, configJSONV2) require.Len(t, orchestrator.config.Ingress.Rules, 1) } +// Validates that the default ingress rule will be set if there is no rule provided from the remote. +func TestUpdateConfiguration_WithoutIngressRule(t *testing.T) { + initConfig := &Config{ + Ingress: &ingress.Ingress{}, + } + orchestrator, err := NewOrchestrator(context.Background(), initConfig, testTags, []ingress.Rule{}, &testLogger) + require.NoError(t, err) + initOriginProxy, err := orchestrator.GetOriginProxy() + require.NoError(t, err) + require.Implements(t, (*connection.OriginProxy)(nil), initOriginProxy) + + // We need to create an empty RemoteConfigJSON because that will get unmarshalled to a RemoteConfig + emptyConfig := &ingress.RemoteConfigJSON{} + configBytes, err := json.Marshal(emptyConfig) + if err != nil { + require.FailNow(t, "The RemoteConfigJSON shouldn't fail while being marshalled") + } + + updateWithValidation(t, orchestrator, 0, configBytes) + require.Len(t, orchestrator.config.Ingress.Rules, 1) +} + // TestConcurrentUpdateAndRead makes sure orchestrator can receive updates and return origin proxy concurrently func TestConcurrentUpdateAndRead(t *testing.T) { const (