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

Automatically calculate public_addr field for dynamic apps (#10941). #10943

Merged
merged 10 commits into from
Mar 15, 2022

Conversation

Tener
Copy link
Contributor

@Tener Tener commented Mar 8, 2022

For static apps, the public_addr field for app will be discovered automatically if it is missing. For dynamic apps this hasn't been implemented, which breaks some assumptions as well as makes them genuinely less useful.

This fixes that, and also adds some minor validation for the dynamic app name for consistency with static apps.

Fixes #10941.

@github-actions github-actions bot added application-access tctl tctl - Teleport admin tool labels Mar 8, 2022
@github-actions github-actions bot requested review from probakowski and zmb3 March 8, 2022 08:33
@Tener Tener requested a review from smallinsky March 8, 2022 08:34
@Tener Tener requested a review from r0mant March 8, 2022 08:38
@Tener
Copy link
Contributor Author

Tener commented Mar 8, 2022

I've dropped the validation.IsDNS1035Label() call on tctl create ... as it appears to be too strict in practice, breaking tests. Despite uppercase letters being allowed by the spec, the function is unhappy about them. I think this is too strict, so I dropped this check.

We may want to revisit the naming requirements... but there are other options too: for example, we may simply change the logic by which we calculate the public_addr for a given app. So all in all, I think it is best to leave this for now given that this change is not about making the naming more strict.

@Tener Tener changed the title Automatically calculate public_addr field for dynamic apps (#10941). Automatically calculate public_addr field for dynamic apps (#10941). Mar 9, 2022
Copy link
Contributor

@probakowski probakowski left a comment

Choose a reason for hiding this comment

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

LGTM, I left two suggestions both up to you

lib/srv/app/watcher.go Outdated Show resolved Hide resolved
lib/srv/app/watcher.go Outdated Show resolved Hide resolved
@Tener
Copy link
Contributor Author

Tener commented Mar 14, 2022

@r0mant @smallinsky let me know what you think.

lib/srv/app/watcher.go Outdated Show resolved Hide resolved
lib/srv/app/watcher.go Show resolved Hide resolved
@Tener Tener merged commit 3bbd3fc into master Mar 15, 2022
@Tener Tener deleted the tener/10941_public_addr_dynamic_apps branch March 15, 2022 11:51
Tener added a commit that referenced this pull request Mar 15, 2022
#10943)

* Autodiscover public_addr for dynamic apps.
Tener added a commit that referenced this pull request Mar 15, 2022
@webvictim webvictim mentioned this pull request Apr 19, 2022
@webvictim webvictim mentioned this pull request Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
application-access tctl tctl - Teleport admin tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatically calculate public_addr field for dynamic apps
3 participants