Skip to content

feat(http-add-on): Add appProtocol support for interceptor admin and TLS service ports#829

Merged
linkvt merged 2 commits intokedacore:mainfrom
gyanesh-mishra:feat/http-add-on-proxy-port-name
Mar 24, 2026
Merged

feat(http-add-on): Add appProtocol support for interceptor admin and TLS service ports#829
linkvt merged 2 commits intokedacore:mainfrom
gyanesh-mishra:feat/http-add-on-proxy-port-name

Conversation

@gyanesh-mishra
Copy link
Copy Markdown
Contributor

@gyanesh-mishra gyanesh-mishra commented Mar 12, 2026

Add configurable appProtocol for the interceptor's admin and TLS service ports.

Closes #828

Changes

New Helm Values

Value Default Description
interceptor.admin.appProtocol "" appProtocol for the admin service port
interceptor.tls.appProtocol "" appProtocol for the TLS proxy service port

interceptor.proxy.appProtocol already existed — no changes there.

Template Changes

  • templates/interceptor/service-admin.yaml — adds optional appProtocol field
  • templates/interceptor/service-proxy.yaml — adds optional appProtocol field for the TLS port, independent of proxy.appProtocol

Documentation

  • README.md regenerated with helm-docs

Backward Compatibility

Default empty values maintain current behavior. Note: proxy.appProtocol no longer implicitly applies to the TLS port — the two ports are now independently controlled.

Checklist

Fixes #828

@gyanesh-mishra gyanesh-mishra requested a review from a team as a code owner March 12, 2026 22:04
@gyanesh-mishra gyanesh-mishra changed the title feat(http-add-on): Allow overriding interceptor proxy service port names feat(http-add-on): Allow overriding interceptor service port names and add admin appProtocol Mar 14, 2026
@gyanesh-mishra gyanesh-mishra force-pushed the feat/http-add-on-proxy-port-name branch from a9807b9 to ea1d3ee Compare March 14, 2026 19:06
@gyanesh-mishra
Copy link
Copy Markdown
Contributor Author

@JorTurFer @zroubalik — updated this PR to also cover the admin service (port name + appProtocol), regenerated docs with helm-docs, and squashed into a single DCO-signed commit. Would appreciate a review when you get a chance!

@JorTurFer JorTurFer requested review from Fedosin and linkvt March 15, 2026 15:28
@linkvt
Copy link
Copy Markdown
Contributor

linkvt commented Mar 16, 2026

Hi @gyanesh-mishra ,
thanks for the PR, looks good to me in general!

I have two questions/notes:

  1. isn't adding the appProtocol enough? If you set a helm value to change the port name it would be the same effort to just set the appProtocol, right? We could add a tls.appProtocol and drop the portName overrides IMO to reduce the surface area
  2. It would IMO be good if the comment would be more generic and not link to a specific service mesh like istio in this case

@gyanesh-mishra gyanesh-mishra force-pushed the feat/http-add-on-proxy-port-name branch from ea1d3ee to de37dfb Compare March 16, 2026 13:33
@gyanesh-mishra
Copy link
Copy Markdown
Contributor Author

Thanks for the review @linkvt!

On point 1 (appProtocol vs portName): You're right that appProtocol alone is sufficient for Istio. However, other service meshes may rely on port naming conventions rather than (or in addition to) appProtocol for protocol detection. I was being proactive in allowing both to be configured to maximize compatibility across different mesh implementations.

That said, if you'd prefer to keep the surface area smaller, I'm happy to reduce this PR to just appProtocol support (adding tls.appProtocol and admin.appProtocol) and follow up with a separate PR for the portName overrides. Let me know your preference!

On point 2 (generic comments): Agreed! I've updated the comments to remove the Istio-specific links — they now just say "Useful for service mesh protocol selection" with example values. Pushed the update just now.

@linkvt
Copy link
Copy Markdown
Contributor

linkvt commented Mar 17, 2026

Hi @gyanesh-mishra ,

I understand but would prefer to limit this change for now to the appProtocol fields, I'm a big fan of YAGNI 🙂
I did a quick research and Istio, Linkerd or Cilium seem to use appProtocol - from the ones I looked at only istio seems to use the portName which is why I think the appProtocol is fine.

Thanks already for the change of the description!

Comment thread http-add-on/README.md Outdated
@gyanesh-mishra gyanesh-mishra force-pushed the feat/http-add-on-proxy-port-name branch from de37dfb to b072697 Compare March 23, 2026 17:15
@gyanesh-mishra gyanesh-mishra requested a review from a team as a code owner March 23, 2026 17:15
@gyanesh-mishra gyanesh-mishra changed the title feat(http-add-on): Allow overriding interceptor service port names and add admin appProtocol feat(http-add-on): Add appProtocol support for interceptor admin and TLS service ports Mar 23, 2026
@gyanesh-mishra gyanesh-mishra force-pushed the feat/http-add-on-proxy-port-name branch 3 times, most recently from 3f22d7a to c86a6c3 Compare March 23, 2026 17:26
@gyanesh-mishra
Copy link
Copy Markdown
Contributor Author

Thanks @linkvt! Updated the PR to drop all portName overrides and limit the change to appProtocol fields only:

  • interceptor.admin.appProtocol (new)
  • interceptor.tls.appProtocol (new)
  • interceptor.proxy.appProtocol already existed — untouched

Made tls.appProtocol independent of proxy.appProtocol since the two ports would typically take different values (e.g. http vs https). README regenerated with helm-docs.

@gyanesh-mishra gyanesh-mishra requested a review from linkvt March 23, 2026 17:31
gyanesh-mishra and others added 2 commits March 24, 2026 10:40
…TLS service ports

Add interceptor.admin.appProtocol and interceptor.tls.appProtocol Helm values
to allow explicit protocol specification for service meshes. The proxy service
already supported interceptor.proxy.appProtocol; this mirrors that for the
admin and TLS ports.

The proxy and TLS ports are intentionally independent: proxy.appProtocol
applies only to the plain HTTP port, tls.appProtocol only to the TLS port,
as they typically require different values (e.g. "http" vs "https").

Signed-off-by: Gyanesh Mishra <gyanesh.mishra@rubrik.com>
Signed-off-by: Vincent Link <vlink@redhat.com>
@linkvt linkvt force-pushed the feat/http-add-on-proxy-port-name branch from c86a6c3 to fc072fa Compare March 24, 2026 09:47
@linkvt
Copy link
Copy Markdown
Contributor

linkvt commented Mar 24, 2026

Hi @gyanesh-mishra , thanks the changes look good!
I (ab-)used my permissions and force-pushed into this PR to a) rebase on main and b) add a fallback to the tls port appProtocol to use the default one if it is set to not break the existing behavior.
I thought this is easier than us going back and forth for that minor change, hope you don't mind!

@linkvt
Copy link
Copy Markdown
Contributor

linkvt commented Mar 24, 2026

CI failure is unrelated and caused by the merge of kedacore/http-add-on#1532 - this will be handled in #835

@linkvt linkvt merged commit e8a7a58 into kedacore:main Mar 24, 2026
4 of 7 checks passed
@gyanesh-mishra
Copy link
Copy Markdown
Contributor Author

Thank you!

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.

feat(http-add-on): Allow overriding interceptor proxy and admin service port names

2 participants