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

Update network costs to v16, stub out configuration, disable by default. #1207

Merged
merged 1 commit into from
Jan 26, 2022

Conversation

mbolt35
Copy link
Contributor

@mbolt35 mbolt35 commented Jan 20, 2022

What does this PR change?

  • Adds network costs v16 image and stubs out documented configuration options.

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

  • Adds a service label to all emitted network-costs metrics

How was this PR tested?

  • Against 2 of our live clusters, helm template:
# Source: cost-analyzer/templates/cost-analyzer-network-costs-config-map-template.yaml
apiVersion: v1
kind: ConfigMap
metadata:
  name: network-costs-config
  labels:
    
    app.kubernetes.io/name: cost-analyzer
    helm.sh/chart: cost-analyzer-1.89.1
    app.kubernetes.io/instance: kubecost
    app.kubernetes.io/managed-by: Helm
    app: cost-analyzer
data:
  config.yaml: |
    destinations:
      cross-region: []
      direct-classification: []
      in-region: []
      in-zone:
      - 127.0.0.1
      - 169.254.0.0/16
      - 10.0.0.0/8
      - 172.16.0.0/12
      - 192.168.0.0/16
    services:
      amazon-web-services: false
      azure-cloud-services: false
      google-cloud-services: false

Have you made an update to documentation?

Yes, directly in the values.yaml - fields defaults will likely change at a future date.

@mbolt35 mbolt35 self-assigned this Jan 20, 2022
@@ -512,6 +512,26 @@ networkCosts:
# zone: "us-east1-c"
# ips:
# - "10.0.0.0/24"
services:
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Looks really slick. Did you consider just using one cloud-services flag initially? Just wondering if that would keep things simpler for v1.

Also, what's thinking with defaulting to false? Would that just be initial release or do you see that as long-term?

Copy link
Contributor Author

@mbolt35 mbolt35 Jan 21, 2022

Choose a reason for hiding this comment

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

Disabling them would at least get the service label being set on all the metrics. There are a lot of really straight-forward reasons I did not enable them by default:

  1. They add a bit of overhead to the startup process (which a few people have complained about taking too long even without this feature).
  2. I was going for the most straight-forward approach that gives users the most flexibility with what services they want to be labeled.
  3. The network-costs pods have never reached out to external services for data, so I do not know immediately how this will effect users who have stricter networking policies, Rather than force this option on everyone at once, I've picked the less risky and more flexible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I had no idea these reached out to external services... I thought you had a map of IP ranges that we would update. Do you actually go get this list from the providers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, because apparently it can change - I didn't look into how often changes happen, but I can imagine if all three service providers have dynamically evolving lists, it's significant.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it worth having a fallback local copy in the future (i.e. put a TODO) to handle users with no outbound requests or the format of these files changing?

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 don't know how you could account for service ips shifting with a local copy? If users don't have outbound requests, they wouldn't need this feature anyways right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If users have a defined set of service IPs they use and want to label, you can add them in the configuration. That option is always available, but if you want the network costs pod to maintain a set of services for the 3 providers, it can do that as well. I don't think that attaching a long list of provider specific ranges that change dynamically makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't think these would change that often but I could be mistaken... let's just test/prepare for this file not being available and make sure the product UI/UX doesn't fail in a nasty way.

Also, want to add a section to https://github.com/kubecost/docs/blob/master/network-allocation.md after launch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's chat about this -- For now, let's just get the metrics emission using service, but I think allowing a file source for external service definitions could also be interesting.

Copy link
Contributor

@AjayTripathy AjayTripathy left a comment

Choose a reason for hiding this comment

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

Webb has a few comments and nits but otherwise lgtm, feel free to merge once fixed.

@mbolt35 mbolt35 merged commit 47badcb into develop Jan 26, 2022
@chipzoller chipzoller deleted the bolt/network-costs-16-upgrade branch October 4, 2023 23:09
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