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

add ingress for model access #2536

Merged
merged 5 commits into from
Sep 19, 2023
Merged

Conversation

leodag
Copy link
Contributor

@leodag leodag commented Aug 24, 2023

What does this PR change?

This adds an optional kubecostModel.ingress option so that it can be configured separately from the frontend. This comes from a need in my company to expose the model through a basic auth ingress so that it can be accessed programatically, while the frontend is handled differently.

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!)

Adds an additional option (defaulting off) to have an ingress that points directly to the model container.

Links to Issues or tickets this PR addresses or fixes

I haven't created any issues for this, should I create one?

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

Should be a pretty simple change; one deployment with it enabled and disabled should be enough IMO.

How was this PR tested?

Tested locally and backported with no changes o the version currently in use by my company (1.102.2), which is deployed in my company's test cluster.

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

Added kubecostModel.ingress to the README

This is my first contribution to the project, if there's any additional information or changes needed, do comment!

@thomasvn
Copy link
Member

thomasvn commented Sep 5, 2023

Thanks for the PR @leodag! This seems reasonable to me! Could you test the PR using helm template and a variety of configurations for .Values.kubecostModel.ingress, so we know the template renders correctly?

cc @jessegoodier When you return, thoughts on the changes in this PR?

cost-analyzer/values.yaml Outdated Show resolved Hide resolved
@leodag
Copy link
Contributor Author

leodag commented Sep 6, 2023

I've tested the template with ingress: {}, ingress: { enabled: true }, ingress: { enabled: false }, and some combinations of annotations and/or paths specified and it behaves as expected.

This is also basically just a copy of cost-analyzer-ingress-template.yaml, so it should behave the same. At first I tried to make the same template generate both, but it ended up being a bunch of ifs in the middle of the template, and I thought it'd be better to have them separate.

This made me look at cost-analyzer-ingress template again and I noticed I was missing a small part of it (handling labels), I'll add that too.

Copy link
Collaborator

@jessegoodier jessegoodier left a comment

Choose a reason for hiding this comment

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

Works for me. Thanks!

@jessegoodier jessegoodier merged commit ead28a9 into kubecost:develop Sep 19, 2023
8 checks passed
ameijer added a commit that referenced this pull request Oct 6, 2023
…l/main

* commit '16f7bafcfdfb74aab2c2b41a0046a4e476f7114b': (85 commits)
  fix description link (#2626)
  Issue templating system (#2595)
  reduce issue stale period to 230 days (#2625)
  add stale bot (#2597)
  Update Network Costs to 0.17.0
  revised comment
  default to false, rename env var to match kcm
  Add note about supporting IDR currency to values.yaml (#2055)
  Bump actions/checkout from 4.0.0 to 4.1.0 (#2604)
  updated grafana dashboard to match queries used by kubecost (#2590)
  skip online validation helm value
  Add support for busybox image path (#2387)
  Revise comments for FederatedETL configs for clarity/readability (#2598)
  copy README (mostly) from root (#2591)
  add ingress for model access (#2536)
  fix(cost-analyzer): Set remoreWrite env variables only when enabled (#2578)
  remove unused check for Nodes (#2586)
  add and/or fix templating of global.podAnnotations (#2587)
  Re-add the `useMultiClusterDB` helm config (#2585)
  Update AMP endpoint's path in config (#2575)
  ...

# Conflicts:
#	cost-analyzer/templates/cost-analyzer-deployment-template.yaml
#	cost-analyzer/values.yaml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants