-
Notifications
You must be signed in to change notification settings - Fork 56
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
Add OpenCost support, make service port and /allocation path configurable #117
Conversation
@jessegoodier sorry to give you the run-around. This is the PR that should give us OpenCost support once merged. |
d89c7c9
to
32e22f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know what you think about the Allocation API endpoint — I may not be grasping something. If that is so, then this is good to go.
cmd.Flags().BoolVar(&options.UseProxy, "use-proxy", false, "Instead of temporarily port-forwarding, proxy a request to Kubecost through the Kubernetes API server.") | ||
cmd.Flags().StringVarP(&options.KubecostNamespace, "kubecost-namespace", "N", "kubecost", "The namespace that kubecost is deployed in. Requests to the API will be directed to this namespace.") | ||
cmd.Flags().StringVar(&options.AllocationPath, "allocation-path", "/model/allocation", "URL path at which Allocation queries can be served from the configured service. If using OpenCost, you may want to set this to '/allocation/compute'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will definitely work, but is (of course) not the nicest UX. Have we exhausted our options for getting these to match between OS and CS?
Ever since we restructured the cmd entry-points, I think we've been able to add endpoints in the OS that only exist there, right? (Before we had to give weird endpoint names to avoid router collisions, which panic.) So can we add /model/allocation to the OS (same behavior as existing /model/allocation/compute) and unify the endpoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could add /allocation
== /allocation/compute
to OpenCost, but there are some wrinkles even then. /allocation
exists in OpenCost and Kubecost, but Kubecost has /model/
in front of everything because of the proxy. We get in sort of a weird situation if we try to fix this:
- pure OSS usage (no Helm chart, no Kubecost
frontend
container) has an API at/allocation/compute
. Adding/model/
in front of all OpenCost API paths works, you end up with/model/allocation/compute
- but then if you deploy OpenCost inside the Kubecost Helm chart, you currently get
/model/allocation/compute
. If you add/model/
in front of all OpenCost API paths, you end up with/model/model/allocation/compute
(The above is also true if you s|/allocation/compute|/allocation/g
)
That is to say, I think unifying the endpoint in OpenCost (/allocation
= /allocation/compute
) is a good idea! But I also think that this new flag may be necessary regardless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we can now attach endpoints in OpenCost (i.e. in pkg/cmd/costmodel/costmodel.go
in func Execute
) I was imagining putting /model/allocation
there. Then in Kubecost, we'd still have /allocation
, which thanks to nginx gets nested into /model/allocation
. And thus, each will just have /model/allocation
, no? And there won't be a /model/model/allocation
because that cmd
doesn't get executed in the closed source. Or am I misunderstanding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed out of band.
This helps add support for OpenCost, which exposes /allocation at a different path and runs on a different port than Kubecost's cost-model does.
This is to help with OpenCost support, because OpenCost may or may not provide the "/getConfigs" endpoint that serves currency code information.
32e22f1
to
65e4201
Compare
What does this PR change?
Adds support for OpenCost by making service name and allocation path configurable and by allowing currency code determination to fail gracefully (defaulting to empty string + a warning log) because OpenCost does not currently and is not guaranteed to provide a
/getConfigs
endpoint.Does this PR rely on any other PRs?
Branched off of Refactor some commands to reduce code duplication #116 (this PR will be rebased after that is merged)How does this PR impact users? (This is the kind of thing that goes in release notes!)
--allocation-path
and--service-port
How was this PR tested?
Locally by using the new flags to cause an expected query failure.
Tested with OpenCost by updating my Kubecost installation to use an OpenCost-only image:
For OpenCost to work in this deployment model, I had to set
--allocation-path "/model/allocation/compute"
. I got a valid response with the expected lack of currency code information.Setting
--log-level
debug surfaces the "failed to get currency code" message, as expected.Have you made an update to documentation?
Yes, see README updates.