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

feat: add possibility to route requests to sidecar container #270

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

edorizzardi
Copy link

Description

This PR changes the Service such that it allows to route requests to an arbitrary port. This is not a breaking change.

@dmotte
Copy link

dmotte commented Dec 16, 2024

@pierluigilenoci since you are the most active committer of this repo, may we kindly ask you if you could please take a look? Although quite simple, this fix would be very beneficial for us 🙂 thank you so much in advance!

@pierluigilenoci
Copy link
Contributor

@edorizzardi Please update the message for ArtifactHub and rebase the PR from the main branch. 🙏🏻 👍🏻

@edorizzardi
Copy link
Author

@pierluigilenoci thank you. Done

@pierluigilenoci
Copy link
Contributor

pierluigilenoci commented Dec 23, 2024

@edorizzardi The chart bump is now missing (this is a new feature, so it must be a "minor"). ➕
I also noticed that the option must be documented in the readme. 😬
https://github.com/oauth2-proxy/manifests/blob/main/helm/oauth2-proxy/README.md

@edorizzardi
Copy link
Author

@pierluigilenoci Updated minor version. Plus I added the parameter in the list, and added a small paragraph on how to use my changes.

@@ -1,5 +1,5 @@
name: oauth2-proxy
version: 7.8.3
version: 7.9.3
Copy link
Contributor

Choose a reason for hiding this comment

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

Please 7.9.0 🙏🏻

Copy link
Contributor

@pierluigilenoci pierluigilenoci left a comment

Choose a reason for hiding this comment

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

There are still some details to iron out, sorry if I'm being pedantic. 🙏🏻

@@ -196,6 +196,7 @@ The following table lists the configurable parameters of the oauth2-proxy chart
| `service.loadBalancerIP` | ip of load balancer | `nil` |
| `service.loadBalancerSourceRanges` | allowed source ranges in load balancer | `nil` |
| `service.nodePort` | external port number for the service when service.type is `NodePort` | `nil` |
| `service.targetPort` | name for the service's `targetPort` | `""`
Copy link
Contributor

Choose a reason for hiding this comment

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

"A numeric port number (e.g., 80) or a port name defined in the pod's container(s) (e.g., http)."

@@ -357,3 +358,6 @@ config:
whitelist_domains = [ ".domain.com", ".example.io"]
provider = "google"
```

## Route requests to sidecar container
You can route requests to a sidecar container first by setting the `service.targetPort` variable; by default, the service's `targetPort` value equals to the `httpSchema`'s.
Copy link
Contributor

Choose a reason for hiding this comment

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

The possible values for the targetPort field of a Kubernetes Service can be either a port number or the name of a port defined in the pod.

@@ -153,6 +153,9 @@ service:
externalTrafficPolicy: ""
# configure internalTrafficPolicy
internalTrafficPolicy: ""
# configure service target port
# useful when there's need to route requests to sidecar containers first
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove this comment.

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.

3 participants