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

Remove ExportData token permission if host-requests are disabled #670

Conversation

ernstvonoelsen
Copy link
Contributor

@ernstvonoelsen ernstvonoelsen commented Mar 26, 2022

…s-requests is true.

Description

summary of the change:
This PR validates the APITokens' scopes in case that a Dynakube instance is annotated with the alpha.operator.dynatrace.com/feature-disable-hosts-requests: "true" even if there is no ExportData permission (since this permission is never used in this case).

relevant motivation and context
Dynatrace Operator will be used in our enterprise landscape by several hundred teams to monitor their K8s clusters in a single-tenant Dynatrace Environment. Due to strict data privacy rules, we cannot allow that regular users get such broad read access on v1 API of Dynatrace as the ExportData permission scope would provide.
From my analysis of the code, the ExportData permission of the provided APIToken is never really required if
automatic kubernetes api monitoring is not enabled and the annotation alpha.operator.dynatrace.com/feature-disable-hosts-requests: "true" on the dynakube instance is present.

Since such a least-privilege approach is feasible without losing Dynatrace's monitoring capabilities (one only looses some comfort imho), I suggest to enable it also in code, although not making it the default.

How can this be tested?

What environment is necessary for the change to be noticeable ?
What needs to be enabled/applied for the change to be noticeable ?

  • a dynakube secret with an APIToken that grants nothing else than InstallerDownload permissions
  • the annotation feature.dynatrace.com/disable-hosts-requests: "true" on the dynakube instance
  • instead of getting a missing scopes error on startup, the dynakube instance is spawned without any complaint

Checklist

  • Unit tests have been updated/added
  • PR is labeled accordingly

@ernstvonoelsen ernstvonoelsen requested a review from a team as a code owner March 26, 2022 20:24
meik99
meik99 previously approved these changes Mar 29, 2022
@meik99 meik99 dismissed their stale review March 29, 2022 07:56

I was a bit too fast with my approval

@ernstvonoelsen
Copy link
Contributor Author

Hi @meik99,
what are we missing?

@chrismuellner chrismuellner added the core Changes to core functionality of the Operator label Mar 29, 2022
@luhi-DT luhi-DT changed the title Abstain from ExportData permission if annotation feature-disable-host… Remove ExportData token permission if host-requests are disabled Mar 29, 2022
meik99
meik99 previously approved these changes Mar 29, 2022
Copy link
Contributor

@meik99 meik99 left a comment

Choose a reason for hiding this comment

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

I was just confused about an internal process, you're good

@0sewa0 0sewa0 enabled auto-merge (squash) March 29, 2022 13:10
@ernstvonoelsen
Copy link
Contributor Author

@meik99 any update on a merge date?

@ernstvonoelsen
Copy link
Contributor Author

Hi @chrismuellner thank you for merging the latest master branch.

On which end do we have to fix the failing tests? Locally they are green, don't know where the difference stems from?

@chrismuellner
Copy link
Collaborator

  1. You can fix the linting issues by running make lint
  2. Tests seem to be failing for dtclient_reconciler_test which you modified. I'm assuming your logic changed the behavior for other tests. How are you running your tests? Running go test ./... should show what tests are failing.

auto-merge was automatically disabled April 26, 2022 07:20

Head branch was pushed to by a user without write access

@ernstvonoelsen
Copy link
Contributor Author

Failing unittests updated (removed DeprecatedFeatureFlagPrefix to annotate the dk instance properly).

@chrismuellner
Copy link
Collaborator

Thanks! Just making sure the Actions run through before merging.

@chrismuellner chrismuellner merged commit ac89da5 into Dynatrace:master Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Changes to core functionality of the Operator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants