Skip to content

Introduce config version v3, allow proxy_addresses to be given in the config#14054

Closed
ryanclark wants to merge 10 commits intomasterfrom
ryan/add-proxy-addrs
Closed

Introduce config version v3, allow proxy_addresses to be given in the config#14054
ryanclark wants to merge 10 commits intomasterfrom
ryan/add-proxy-addrs

Conversation

@ryanclark
Copy link
Copy Markdown
Member

@ryanclark ryanclark commented Jul 1, 2022

This introduces a new version of the config, v3, as we're adding a new item to the schema - proxy_addresses.

This allows us to go straight to direct dialling the proxy server if specified via proxy_addresses, as we know it's not an auth server and we don't need to attempt to direct dial it.

This keeps backwards compatibility with v2, where if a proxy server is specified in auth_addresses it'll still connect. It will put a warning in the console telling the user to upgrade their config version and specify proxy_addresses instead, like so:

image

(Do we want to add an item to a documentation page about this change? I've left the URL as an example of what we can display to the user)

For the tests, I've duplicated a few v2 tests and changed the version to v3 - just sanity checking nothing strange happens when we upgrade the config version.

This also updates the CLI reference docs to set the version to v3, and specify proxy_addresses. I'm not sure if there's more documentation I need to update, but I'll take a look through.

I took the chance to refactor the config validation code, splitting it into its own file instead of adding more code to the already long service.go.

This would close #11524, as we'll not need dial_mode as we know whether the specified URI is a proxy server or auth server.

@ryanclark ryanclark force-pushed the ryan/add-proxy-addrs branch from c002fca to ba9c0cd Compare July 1, 2022 19:35
Comment thread lib/config/configuration.go Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: for all these checks, instead of a == or !=, I would find a switch easier to reason about.

switch fs.Version {
case defaults.TeleportConfigVersionV1:
    // v1 logic
case defaults.TeleportConfigVersionV2, defaults.TeleportConfigVersionV3:
    // v2/v3 logic
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I had chose to do it this way so we have less places to update every time we do a new config version, happy to change it to a switch statement though

Comment thread lib/service/cfg.go Outdated
Comment thread lib/service/connect.go Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If you can determine that the config version is v3, you can skip dialing via tunnel here. V3 configs will be opt-in, so we can make proxy_servers required for tunneling.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah that TODO is an old comment that can be removed. This tunnelling is needed as it's the fallback after trying to direct dial the value from AuthServers.

I've made it so v3 configs are default - is there a reason we'd make it opt-in? The way I saw it was there's no backwards compatibility issues, so we can skip straight to v3 being the default.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We don't know the config version in this function to know to make v3 only direct dial an auth server or tunnel a proxy server. We can guess, which is what I'm doing now - if proxy addresses are specified, tunnel to them, if not, direct dial to the auth server value and fall back to tunnelling if it doesn't work.

Maybe it would be nicer to pass the configuration version to here, so we can restrict the fallback of tunnelling the auth addresses value to v2 and below.

Comment thread lib/service/validateconfig.go Outdated
Comment thread tool/teleport/common/teleport.go Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Don't use the auth connect addr in the help message, as the presence of auth port 3025 will confuse users (prefer 3080 or 443).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I missed this and have set it to [none] like the others. Shall I add an example URL there on port 3080 or 443, or leave it as [none]?

@ryanclark ryanclark force-pushed the ryan/add-proxy-addrs branch from 2a9ba83 to eb31962 Compare July 4, 2022 13:31
@rosstimothy
Copy link
Copy Markdown
Contributor

rosstimothy commented Jul 5, 2022

I realize this is still a draft, but I stumbled across this and while everything here looks good I have a question:

Should proxy_addresses actually be proxy_address?

Perhaps a follow up PR to this now that there is a config v3 would be to add auth_server and deprecate auth_servers?

@zmb3 thoughts?

@nklaassen
Copy link
Copy Markdown
Contributor

Came across this because I am interested in https://github.com/gravitational/teleport-private/issues/135

If it is possible to do this in a backward-compatible way without bumping the config version to v3, I think we should do that.

As Tim mentioned, I think we should also take this opportunity to de-pluralize the fields, because we don't actually support more than 1 auth/proxy address.

We could add a proxy_server field, which if used, will only try to connect to a proxy without fallback. Similarly, we could add a auth_server field (not auth_servers), which if used will only try to direct dial to auth, without fallback. The feature would remain opt-in because you would need to explicitly use these new fields. Over time we could deprecate the current auth_servers field.

I don't see a need to bump the config version to v3. New resource versions can be confusing to users, and if it's not absolutely necessary I think we should avoid it.

@zmb3
Copy link
Copy Markdown
Collaborator

zmb3 commented Aug 5, 2022

That makes sense to me, thanks for the suggestion @nklaassen. We decided on config v3 before we talked about making auth_server singular.

I'll get confirmation in next product sync.

@ryanclark ryanclark force-pushed the ryan/add-proxy-addrs branch from eb31962 to e9e9984 Compare August 22, 2022 12:28
@ryanclark
Copy link
Copy Markdown
Member Author

Closing as I restarted this work in #15761

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow users to specify dial mode "hint"

4 participants