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 support for oauth-proxy sidecar #84

Merged
merged 2 commits into from
Oct 19, 2023
Merged

Conversation

TheRealNoob
Copy link
Contributor

Add the ability to add sidecars with the specific use-case in mind of oauth-proxy for the UI.

closes #82

@TheRealNoob
Copy link
Contributor Author

one thing I think worth noting is that I nested extraContainers at a different level than extraVolumes. This is obviously not ideal, but I did it this way because I believe extraVolumes is located at the wrong level. Let me explain.

If I could be so forward, what I see implemented now a mixed presumption that this helm chart will only ever have one Pod and ServiceMonitor (because a SM can only point to a single pod). Some of the config says that statement is true and some says it's false. On the root of the values file are several items:

  • podAnnotations
  • podLabels
  • podSecurityContext
  • extraVolumes
  • etc
    All of these objects can only be tied to one podSpec. If you add a second pod down the road, now these objects being located at the root is confusing. I think you lock yourself into a corner by having them there - I think the correct place is under the opencost object. An example of a second pod that you might add down the road could be subcharting Prometheus, in which case that would be controlled under the root prometheus object. I'm tempted to say that everything written now should be moved under opencost. Even things like ServiceAccount, which isn't technically 1:1, but if you're going to create another pod, it probably won't need the same k8s permissions as opencost, so having a different serviceAccount for it makes sense IMO.

This is all just my internal thoughts as I was drafting it. If you want me to move things around let me know, I won't object.

@github-actions
Copy link

This pull request has been marked as stale because it has been open for 60 days with no activity. Please remove the stale label or comment or this pull request will be closed in 5 days.

@github-actions github-actions bot added the Stale label Aug 23, 2023
@mattray mattray removed the Stale label Aug 24, 2023
@mattray
Copy link
Collaborator

mattray commented Aug 28, 2023

I don't have a means to test this, but if someone could validate this works for them we'll get it merged.

Copy link

@debMan debMan left a comment

Choose a reason for hiding this comment

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

Works fine. I just tested and confirmed the functionality. Except for the chart version, everything works fine. I appreciate it if you @mattray merge this, need these sidecars.
Thanks in advanced.

@mattray mattray merged commit 9e8288e into opencost:main Oct 19, 2023
1 check passed
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.

ui authentication
3 participants