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

[defaults] Fix cluster agent token autogeneration #447

Merged
merged 3 commits into from
Feb 16, 2022

Conversation

davidor
Copy link
Member

@davidor davidor commented Feb 11, 2022

What does this PR do?

Currently, when a cluster agent token is not specified, it is not autogenerated properly and agents cannot connect to the cluster agent. This PR fixes the issue.

When a token is not specified, this PR autogenerates one and stores it in the "DefaultOverride" attribute of the spec.

Additional Notes

When the secret that contains the token changes, the cluster agent and the agents are not restarted, so they keep using the old token. This issue is not fixed by this PR yet.

Describe your test plan

  • Deploy a datadog agent without specifying a token in the spec (credentials.token). Check the status of the agents (agent status) and verify that they can connect with the cluster agent. Also check that when the datadog agent is deleted, the secret with the token (has the same name as the datadog agent object) is also deleted. You can also check that in the status of the datadog agent, defaultOverride.credentials.token matches the token stored in the secret.
  • Deploy a datadog agent specifying a token in the spec (credentials.token) and check the same except the defaultOverride part, which doesn't apply in this case.

@davidor davidor added bug Something isn't working component/controller labels Feb 11, 2022
@davidor davidor added this to the v0.8.0 milestone Feb 11, 2022
@codecov-commenter
Copy link

codecov-commenter commented Feb 11, 2022

Codecov Report

Merging #447 (71276a4) into main (6b7a141) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #447   +/-   ##
=======================================
  Coverage   60.60%   60.60%           
=======================================
  Files           3        3           
  Lines         132      132           
=======================================
  Hits           80       80           
  Misses         40       40           
  Partials       12       12           
Flag Coverage Δ
unittests 60.60% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b7a141...71276a4. Read the comment docs.

@davidor davidor marked this pull request as ready for review February 14, 2022 16:16
@davidor davidor requested a review from a team as a code owner February 14, 2022 16:16
defaultClusterAgentToken(dda, dso)
}

func defaultClusterAgentToken(dda *DatadogAgent, dso *DatadogAgentStatus) {
Copy link
Collaborator

@clamoriniere clamoriniere Feb 15, 2022

Choose a reason for hiding this comment

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

IMO this change is not fully backward compatible.
what happen if the dso.ClusterAgent.GeneratedToken was already defined?

can we add another step in the function to get the token present in status.ClusterAgent.GeneratedToken if present ? and copy it into dda.Status.DefaultOverride.Credentials.Token

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a new commit with this 👍

} else if isClusterAgentEnabled(dda.Spec.ClusterAgent) && dda.Status.ClusterAgent != nil {
data[datadoghqv1alpha1.DefaultTokenKey] = []byte(dda.Status.ClusterAgent.GeneratedToken)
} else if isClusterAgentEnabled(dda.Spec.ClusterAgent) {
if dda.Status.DefaultOverride != nil && dda.Status.DefaultOverride.Credentials != nil && dda.Status.DefaultOverride.Credentials.Token != "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: create a function the return the token, or check that it is defined.

I saw the same code here: https://github.com/DataDog/datadog-operator/pull/447/files#diff-c46f55565558bd8d50d96ff9d37415ce9a6adbb913178786b3e413ce10440be4R160-R162

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a new commit with this 👍

@davidor davidor force-pushed the davidor/fix-autogenerated-cluster-agent-token branch from bbb4113 to 768d927 Compare February 15, 2022 20:00
@davidor davidor merged commit 6a17bbd into main Feb 16, 2022
@davidor davidor deleted the davidor/fix-autogenerated-cluster-agent-token branch February 16, 2022 17:07
@khewonc khewonc mentioned this pull request May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working component/controller
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants