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

Docs: add discovery_group to teleport.yaml/discovery_service examples #48362

Merged
merged 3 commits into from
Nov 12, 2024

Conversation

marcoandredinis
Copy link
Contributor

@marcoandredinis marcoandredinis commented Nov 4, 2024

When the discovery_group is not configured, teleport will log a warning message saying that it is recommended.

c.Log.Warn("discovery_service.discovery_group is not set. This field is required for the discovery service to work properly.\n" +

Some configuration examples do not include it, so when users use that example to implement their own variation, it will output a warning message.
After seeing that warning users might wonder if there's anything wrong with their teleport.yaml.
Instead, we add a discovery_group to all examples so that users don't get that message.

Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-48362.d3pp5qlev8mo18.amplifyapp.com

Copy link

github-actions bot commented Nov 4, 2024

🤖 Vercel preview here: https://docs-jceyj4aft-goteleport.vercel.app/docs/ver/preview

@@ -396,6 +396,7 @@ ssh_service:
enabled: off
discovery_service:
enabled: "yes"
discovery_group: "gke-myproject"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have any instructions telling the user to update discovery_group in this and other Teleport configuration examples?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have it in the reference page and some other configurations.
I can add it to everything.
The description we use in the reference page is:

    # discovery_group is used to group discovered resources into different
    # sets. This is required when you have multiple Teleport Discovery services
    # running. It prevents discovered services from colliding in Teleport when
    # managing discovered resources.
    # If two Discovery Services match the same resources, they must be in the
    # same discovery group.
    # If two Discovery Services match different resources, they must be in
    # different discovery groups.
    discovery_group: "disc-group"

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I would just make it explicit how to assign this value. Is this the name of a Google Cloud project where Kubernetes clusters you want to discover are running?

Copy link
Contributor Author

@marcoandredinis marcoandredinis Nov 7, 2024

Choose a reason for hiding this comment

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

It's an opaque value. Can be a random string.
When multiple DiscoveryServices are running, it is used to ensure resources are not deleted when reconciling the list of resources (eg databases).

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I hadn't realized when leaving this comment that there is a partial, docs/pages/includes/discovery/discovery-group.mdx, that provides this information already. I think we can remove the discovery_group comments and use this partial instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the inline comment in the yaml code blocks.
Added the partial where it was missing.

Copy link

github-actions bot commented Nov 7, 2024

🤖 Vercel preview here: https://docs-l8x05mjri-goteleport.vercel.app/docs/ver/preview

Copy link

github-actions bot commented Nov 7, 2024

🤖 Vercel preview here: https://docs-5kprohh6a-goteleport.vercel.app/docs/ver/preview

Copy link

🤖 Vercel preview here: https://docs-o25hbch0m-goteleport.vercel.app/docs/ver/preview

r0mant
r0mant previously approved these changes Nov 11, 2024
Comment on lines -78 to -85
# discovery_group is used to group discovered resources into different
# sets. This is required when you have multiple Teleport Discovery services
# running. It prevents discovered services from colliding in Teleport when
# managing discovered resources.
# If two Discovery Services match the same resources, they must be in the
# same discovery group.
# If two Discovery Services match different resources, they must be in
# different discovery groups.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you remove these explanations from everywhere? They seemed useful, discovery group is not an obvious concept to understand without the comment.

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 removed them but added the (!docs/pages/includes/discovery/discovery-group.mdx!) partial where it was not already present.

I'm fine with either, but I think we should stick to only one form of documentation (either inline with the yaml code block or with the partial).

@r0mant r0mant dismissed their stale review November 11, 2024 16:15

Have a question.

When the discovery_group is not configured, teleport will log a warning
message saying that it is recommended.
Some configuration examples do not include it, so when users use that
example to implement their own variation, it will output a warning
message.
After seeing that warning users might wonder if there's anything wrong
with their `teleport.yaml`.
Instead, we add a discovery_group to all examples so that users don't
get that message.
Copy link

🤖 Vercel preview here: https://docs-f0sxhnkwr-goteleport.vercel.app/docs/ver/preview

@marcoandredinis marcoandredinis added this pull request to the merge queue Nov 12, 2024
github-merge-queue bot pushed a commit that referenced this pull request Nov 12, 2024
…#48362)

* Docs: add discovery_group to teleport.yaml/discovery_service examples

When the discovery_group is not configured, teleport will log a warning
message saying that it is recommended.
Some configuration examples do not include it, so when users use that
example to implement their own variation, it will output a warning
message.
After seeing that warning users might wonder if there's anything wrong
with their `teleport.yaml`.
Instead, we add a discovery_group to all examples so that users don't
get that message.

* explain discovery group

* use partial instead
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 12, 2024
@marcoandredinis marcoandredinis added this pull request to the merge queue Nov 12, 2024
Merged via the queue into master with commit f0a30b3 Nov 12, 2024
41 checks passed
@marcoandredinis marcoandredinis deleted the marco/docs_add_discoverygroup branch November 12, 2024 17:30
@public-teleport-github-review-bot

@marcoandredinis See the table below for backport results.

Branch Result
branch/v15 Failed
branch/v16 Create PR
branch/v17 Create PR

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

Successfully merging this pull request may close these issues.

3 participants