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

CI testing improvements #2734

Merged
merged 12 commits into from
Nov 27, 2023
Merged

Conversation

chipzoller
Copy link
Collaborator

@chipzoller chipzoller commented Nov 3, 2023

What does this PR change?

Closes #2708
Closes #2515

This PR dramatically expands and improves chart testing in this repository by setting up an extensible framework, documenting it, and adding 3 files which cover most of the common Kubecost deployment configurations. For further details on this PR, see the DEVELOPMENT.md guide contained herein.

Does this PR rely on any other PRs?

No

How does this PR impact users? (This is the kind of thing that goes in release notes!)

No user impact. This is internal only.

Links to Issues or tickets this PR addresses or fixes

What risks are associated with merging this PR? What is required to fully test this PR?

CI could break.

How was this PR tested?

Extensive testing in fork.

Have you made an update to documentation? If so, please provide the corresponding PR.

Developer docs updated in DEVELOPMENT.md

Signed-off-by: chipzoller <[email protected]>
Signed-off-by: chipzoller <[email protected]>
Signed-off-by: chipzoller <[email protected]>
Signed-off-by: chipzoller <[email protected]>
@thomasvn
Copy link
Member

thomasvn commented Nov 3, 2023

@chipzoller Really awesome! Glad that we can now test deployments of these common configurations upon each PR. Only a few minor comments here, mostly questions for clarity.

Also, is there any reason the aggregator-values.yaml file is so large? Could we remove all the excess and just include what's pertinent to aggregator?

@chipzoller
Copy link
Collaborator Author

Great questions, as always.

Also, is there any reason the aggregator-values.yaml file is so large? Could we remove all the excess and just include what's pertinent to aggregator?

No particular reason, I suppose. I used an existing config in the PoC repo plus your suggestions (plus a couple of my own, ex., enabling network costs) to come up with this manifest. If we can slim it down that'd be ideal. Do you have any recommendations?

@thomasvn
Copy link
Member

thomasvn commented Nov 7, 2023

How about using these values for the Aggregator? (aggregator-minimal.yaml internal repo)

@chipzoller
Copy link
Collaborator Author

Can I opt out of using an existing ServiceAccount? This will reduce the dependencies by one. It doesn't impact the ability to determine if aggregator works properly, correct?

@thomasvn
Copy link
Member

thomasvn commented Nov 8, 2023

Is this the ServiceAccount you're referring to? Aggregator shouldn't need it!

# when using managed identity/irsa, set the service account accordingly:
serviceAccount:
  create: false
  name: kubecost-irsa-sa

@chipzoller
Copy link
Collaborator Author

Can kubecostProductConfigs.productKey.key just remain as YOUR_KEY or does it have to be something valid?

@chipzoller
Copy link
Collaborator Author

Hey @thomasvn, can you help me refine this so we can get it in?

@thomasvn
Copy link
Member

@chipzoller Thoughts on omitting kubecostProductConfigs.productKey.key from any CI tests?

Even if we put a dummy string here, it wouldn't be testing much functionality. And we also wouldn't want to be putting a valid key in these CI tests unless it's possible to store this secret outside git.

@chipzoller
Copy link
Collaborator Author

Omitting that sounds good, I just don't know what happens if the value isn't provided. Can you summarize the changes you'd like to see in this PR?

@jessegoodier
Copy link
Collaborator

by default, no value is needed. The majority of installs are free.

@chipzoller
Copy link
Collaborator Author

But can it be null or does it have to be ""?

@thomasvn
Copy link
Member

@chipzoller Happy to get this PR in ASAP! No major concerns with it. I think as a minor improvement we can use a much more minimal aggregator-values.yaml.

When I configured a minimal aggregator, these were the only configs I needed:

# kubectl create secret generic federated-store --from-file=federated-store.yaml
# kubectl create secret generic cloud-integration --from-file=cloud-integration.json
kubecostAggregator:
  enabled: true
  cloudCost:
    enabled: true
  aggregatorStorage:
    storageRequest: 5Gi
  aggregatorDbStorage:
    storageRequest: 10Gi
kubecostModel:
  federatedStorageConfigSecret: federated-store
kubecostProductConfigs:
  cloudIntegrationSecret: cloud-integration

Signed-off-by: chipzoller <[email protected]>
Copy link

gitguardian bot commented Nov 27, 2023

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request
GitGuardian id Secret Commit Filename
- Google API Key de86fff cost-analyzer/templates/aggregator-statefulset.yaml View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secret safely. Learn here the best practices.
  3. Revoke and rotate this secret.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

@chipzoller chipzoller requested a review from thomasvn November 27, 2023 12:41
@chipzoller
Copy link
Collaborator Author

@thomasvn, made the changes. Please review.

@chipzoller chipzoller merged commit 2564343 into kubecost:develop Nov 27, 2023
11 checks passed
@chipzoller chipzoller deleted the ci-improvements branch November 27, 2023 18:37
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.

Helm tests for aggregator [CI] Use official Helm chart-testing
3 participants