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

API Key rotation for Grafana Workspace #259

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

veerapratapg
Copy link

What does this PR do?

  • Added a module to enable Grafana API Key rotation --> grafana-key-rotation
  • Made appropriate changes to the eks-monitoring module to retrieve the details of the resources created in the in this module to be used for the above key rotation module.
  • Modified the existing-cluster-with-base-and-infra example to create create the grafana-key-rotation module.

Motivation

More

  • Yes, I have tested the PR using my local account setup (Provide any test evidence report under Additional Notes)
  • Yes, I ran pre-commit run -a with this PR
  • Yes, I have added a new example under examples to support my PR (when applicable)
  • Yes, I have updated the Pages for this feature

Note: Not all the PRs required examples and docs.

For Moderators

  • E2E Test successfully complete before merge?

Additional Notes

…e changes to the eks-monitoring module and examples/existing-cluster-with-base-and-infra directory
@veerapratapg
Copy link
Author

Tagging reviewers --> @bonclay7 @lewinkedrs

Copy link
Contributor

@lewinkedrs lewinkedrs left a comment

Choose a reason for hiding this comment

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

Looks good to me but I did leave some comments with a few small fixes. We also need a doc for this. Can you write a markdown file in the docs folder describing how to use this module? Do not worry about the folder structure or where in the website to place the doc, we can do that part. But in the doc give a brief intro to what this module does, how to enable it or disable it, and describe some of the configuration options available to the users. Thank you for the contribution though it is looking very nice!

modules/grafana-key-rotation/main.tf Outdated Show resolved Hide resolved
modules/grafana-key-rotation/main.tf Outdated Show resolved Hide resolved
modules/grafana-key-rotation/main.tf Outdated Show resolved Hide resolved
modules/grafana-key-rotation/main.tf Outdated Show resolved Hide resolved
modules/grafana-key-rotation/main.tf Outdated Show resolved Hide resolved
…an be found in the comments of the Pull Request.
@veerapratapg
Copy link
Author

veerapratapg commented Jan 31, 2024

Made the following changes to incorporate the suggestions from @lewinkedrs :

  1. Added a README.md doc to grafana-key-rotation module with information regarding :
    • What the module does and the resources it creates
    • How to enable/disable it.
    • Some of the configuration options.
  2. Performed testing with the latest version of python runtime for Lambda, "3.12", and the solution is working as expected.
  3. Removed test from the names of policy attachment resources.
  4. Also removed white spaces from the mentioned files.
  5. Added variables for python runtime, event bridge scheduler expressions to provide additional flexibility for users.

lewinkedrs
lewinkedrs previously approved these changes Jan 31, 2024
Copy link
Contributor

@lewinkedrs lewinkedrs left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@bonclay7 bonclay7 left a comment

Choose a reason for hiding this comment

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

Can you please address this change, re-run pre-commit, and test it once more, as I've fixed some conflicts in your branch.
Thanks again for the work!

modules/eks-monitoring/outputs.tf Outdated Show resolved Hide resolved
… be found in the comments of the Pull Request.
… be found in the comments of the Pull Request.
… be found in the comments of the Pull Request.
@veerapratapg
Copy link
Author

Made the changes suggested by @bonclay7 and removed managed_grafana_workspace_id from the SSM parameter name created through external-secrets to reduce the number of changes made.

Copy link

This PR has been automatically marked as stale because it has been open 60 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added the stale label Apr 23, 2024
@github-actions github-actions bot removed the stale label Apr 24, 2024
@veerapratapg
Copy link
Author

Working on the incorporating unit tests for the Lambda function per my conversation with bonclay7@

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.

None yet

3 participants