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

[FLINK-32373][sql-client] Support passing headers with SQL Client gateway requests #22816

Merged
merged 2 commits into from Jul 6, 2023

Conversation

afedulov
Copy link
Contributor

@afedulov afedulov commented Jun 17, 2023

What is the purpose of the change

FLINK-32030 and FLINK-32035 enable communication from the SQL Client to the SQL Gateway placed behind a proxy (i.e. a K8S ingress). Such use cases typically require authentication. This PR enables authentication by adding the ability to supply custom headers to the underlying RestClient.

Verifying this change

  • Added a test that verifies that custom headers are read and parsed correctly

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
  • The serializers: (yes / no / don't know)
  • The runtime per-record code paths (performance sensitive): (yes / no / don't know)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes / no / don't know)
  • The S3 file system connector: (yes / no / don't know)

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • A follow up docs PR will be created to document URLs support in SQL Client gateway mode, including the custom headers feature

@flinkbot
Copy link
Collaborator

flinkbot commented Jun 17, 2023

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

String rawHeaders = System.getenv(envVarName);

if (rawHeaders != null) {
String[] lines = rawHeaders.split("\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it will be not so easy to specify a single env variable value that is separated by newlines? What about considering a different separator for distinguishing a list of header key-value pairs in a single line?

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: instead of \n, should we consider platform independent System.lineSeparator()?

Copy link
Contributor Author

@afedulov afedulov Jul 5, 2023

Choose a reason for hiding this comment

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

I believe this is actually the best option because, as RFC 7230 states, CRLF within header values is not expected anywhere apart from "obsolete folding", which is not supported anymore.

     header-field   = field-name ":" OWS field-value OWS

     field-name     = token
     field-value    = *( field-content / obs-fold )
     field-content  = field-vchar [ 1*( SP / HTAB ) field-vchar ]
     field-vchar    = VCHAR / obs-text

     obs-fold       = CRLF 1*( SP / HTAB )
                    ; obsolete line folding
                    ; see [Section 3.2.4](https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.4)

field-value, on the other hand, rules out everything from VCHAR. Expecting a delimiter and an escaping strategy for a character that can naturally occur in a cookie is not user-friendly.

Copy link
Contributor

Choose a reason for hiding this comment

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

HTTP header-wise it seems ok, I thought it will be challenging for specifying the env variable with multi-line values (each separated with a new line. this is just a nit comment.

@tweise tweise merged commit 1354d2f into apache:master Jul 6, 2023
Hebella pushed a commit to Hebella/flink that referenced this pull request Jul 11, 2023
mas-chen pushed a commit to mas-chen/flink that referenced this pull request Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants