Skip to content

Conversation

@fivetran-arunsuri
Copy link
Contributor

@fivetran-arunsuri fivetran-arunsuri commented Oct 30, 2025

Fixes #2785

This change adds a Server response header to all HTTP responses, formatted as:

Server: Polaris/1.1

Details

PR to use the Quarkus http.header mechanism for enabling Polaris version

When enabled, the header will be included in all outgoing HTTP responses.

The primary purpose of this feature is observability — allowing clients or monitoring tools to identify the responding service version.

Testing:

  • Added unit tests to verify:
    Header inclusion when the flag is enabled.
    Absence of the header when the flag is disable
  • Verified using Postman
image

@adutra
Copy link
Contributor

adutra commented Oct 30, 2025

Hi @fivetran-arunsuri Quarkus already exposes a similar feature:

https://quarkus.io/guides/http-reference#additional-http-headers

Couldn't you leverage that?

For example:

quarkus.http.header."Server".value=Polaris/${quarkus.application.version}

@dimas-b
Copy link
Contributor

dimas-b commented Oct 30, 2025

The quarkus.http.header approach looks interesting and promising from my POV 🙂

It will also allow downstream builds to control whether the version header is exposed or not with per-deployment granularity (which is sufficient IMHO).

However, users of ASF Polaris binaries may not be able to "unset" the default value of this property 🤔 Those users will only be able to override it to something like quarkus.http.header."Server".value=NotAvailable. Alternatively, playing with the path and/or method sub-properties may be needed to exclude this header from responses... All in all, this might still be fine for end users, but we'll probably need to document it well in comments.

@fivetran-arunsuri
Copy link
Contributor Author

Thanks @adutra @dimas-b — I verified the Quarkus option works:

quarkus.http.header."Server".value=Polaris/${quarkus.application.version}

I kept the ServerHeaderFilter behind polaris.http.version-header.enabled (default false) based on the previous dev email discussion to have a true off switch (fully removes the header, not just overrides). This addresses the “can’t unset” concern you mentioned. I’ll also document the Quarkus property as a downstream-friendly alternative for per-deployment control.

Unless there are objections, I’ll merge this as-is and add docs:

  • how to enable via polaris.http.version-header.enabled=true
  • how downstreams can set the Quarkus property instead if they prefer config-only.

If you’d rather we switch to config-only now, happy to do that, but I think this strikes a good balance (explicit toggle + simple override path).

@dimas-b
Copy link
Contributor

dimas-b commented Oct 30, 2025

If we can avoid adding java code to support the new header, it would be preferable from my POV.

@fivetran-arunsuri : could you check whether adding quarkus.http.header."Server".path=/irrelevant actually removes the Server header from responses?

If that works, it should be sufficient. I do not expect suppressing this header to be a common use case and advanced uses can figure it out (provided we add comments in the default config).

@fivetran-arunsuri
Copy link
Contributor Author

Thanks @dimas-b, confirmed that adding a non-matching path (/disabled) prevents the header from being added, while keeping .value defined avoids the config-validation error.

simplified the PR to use the Quarkus http.header mechanism only, drop the custom filter, and document:

  • how to enable via %prod.quarkus.http.header."Server".path=/*
  • how to keep it disabled by default with path=/disabled.

dimas-b
dimas-b previously approved these changes Oct 30, 2025
Copy link
Contributor

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

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

Thanks, @fivetran-arunsuri !

@github-project-automation github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Oct 30, 2025
@snazy
Copy link
Member

snazy commented Oct 31, 2025

If we can avoid adding java code to support the new header, it would be preferable from my POV.

I'd prefer this "no additional code" approach as well and document it.


# --- Polaris Server header (Quarkus-managed) ---
# Keep .value present (required by Quarkus), but make the path never match so it's effectively OFF by default.
quarkus.http.header."Server".value=Polaris/${quarkus.application.version}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to put anything in this file? The property is not marked as a build-time property in the configuration reference.

I think users could just add this setting in their own configuration. There should be no need to rebuild Polaris, just add it in one of the supported configuration sources.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. Doc changes alone should be sufficient for users.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on doc only, we don't have to make it default here. Thanks @adutra for the suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, thanks everyone for confirming. I’ll proceed with docs-only changes since users can add this configuration themselves.

adutra
adutra previously approved these changes Nov 3, 2025
Copy link
Contributor

@adutra adutra left a comment

Choose a reason for hiding this comment

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

LGTM thanks @fivetran-arunsuri !

Comment on lines 138 to 139
If you prefer to scope the header to specific environments, only set the property for the desired
profile (for example, `%prod`).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If you prefer to scope the header to specific environments, only set the property for the desired
profile (for example, `%prod`).

I could imagine that "specific environment" is confusing for non-Quarkus affine users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing that out, @snazy — that’s a fair observation. I do think the current phrasing (“specific environment”) works fine here, though, since the following example (%prod) already makes it clear that we’re referring to Quarkus profiles.
The intent is mainly to help readers understand where to apply the property, and the term “environment” keeps it a bit more approachable for those who might not be deeply familiar with Quarkus terminology. I’d prefer to keep it as is, but happy to revisit if others find it confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

From my POV having this phrase in the doc is ok. People who go to this level of configuration / customization should be aware of the Quarkus runtime and its configuration features.

This will cause Polaris to also return a `404 Not Found` response if the realm header is not present
in the request.

### Polaris Server Header
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I think it's better to move this section down to the end of the doc.
It's (at least IMHO) an advanced option for very specific use cases, while the other items mentioned here are all mandatory settings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point, moved to the end of doc

flyrain
flyrain previously approved these changes Nov 3, 2025
Copy link
Contributor

@flyrain flyrain left a comment

Choose a reason for hiding this comment

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

LGTM. Feel free to merge when other reviewers' comments are addressed.

@fivetran-arunsuri fivetran-arunsuri dismissed stale reviews from flyrain and adutra via 00ed2e6 November 4, 2025 03:13
@fivetran-arunsuri
Copy link
Contributor Author

If everything looks good with the PR, would someone with write access be able to merge it? I don’t currently have the permissions to do so. Thank you

Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

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

LGTM as well!
I'll merge it.
Thanks for the contribution!

@snazy snazy merged commit 74d4198 into apache:main Nov 4, 2025
15 checks passed
@github-project-automation github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Nov 4, 2025
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.

Add Server header to Polaris API responses to indicate Polaris version

5 participants