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

Handle CORS in secure connections #5855

Merged
merged 1 commit into from
Oct 18, 2024

Conversation

eapolinario
Copy link
Contributor

Tracking issue

#5790

Why are the changes needed?

CORS options were never enabled in the case of tls servers.

What changes were proposed in this pull request?

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

Signed-off-by: Eduardo Apolinario <[email protected]>
Copy link

codecov bot commented Oct 17, 2024

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Project coverage is 36.71%. Comparing base (66391ff) to head (3d4e7cc).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
flyteadmin/pkg/server/service.go 0.00% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5855      +/-   ##
==========================================
+ Coverage   34.50%   36.71%   +2.21%     
==========================================
  Files        1138     1304     +166     
  Lines      102739   130081   +27342     
==========================================
+ Hits        35448    47758   +12310     
- Misses      63615    78153   +14538     
- Partials     3676     4170     +494     
Flag Coverage Δ
unittests-datacatalog 51.58% <ø> (+0.21%) ⬆️
unittests-flyteadmin 54.39% <0.00%> (-1.22%) ⬇️
unittests-flytecopilot 11.73% <ø> (-0.45%) ⬇️
unittests-flytectl 62.40% <ø> (?)
unittests-flyteidl 6.89% <ø> (-0.26%) ⬇️
unittests-flyteplugins 53.62% <ø> (+0.17%) ⬆️
unittests-flytepropeller 42.84% <ø> (+0.80%) ⬆️
unittests-flytestdlib 54.78% <ø> (-0.59%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@eapolinario eapolinario merged commit eda7521 into master Oct 18, 2024
49 of 50 checks passed
@eapolinario eapolinario deleted the add-support-for-CORS-if-tls-is-enabled branch October 18, 2024 15:07
eapolinario added a commit that referenced this pull request Oct 22, 2024
eapolinario added a commit that referenced this pull request Oct 23, 2024
* Add finalizer to avoid premature CRD Deletion (#5788)

Signed-off-by: Rafael Raposo <[email protected]>

* Handle CORS in secure connections (#5855)

* Upstream contributions from Union.ai (#5769)

* Overlap create execution blob store reads/writes

This change modifies launch paths stemming from `launchExecutionAndPrepareModel` to overlap blob store write and read calls, which dominate end-to-end latency (as seen in the traces below).

Signed-off-by: Andrew Dye <[email protected]>

* Overlap FutureFileReader blob store writes/reads

This change updates `FutureFileReader.Cache` and `FutureFileReader.RetrieveCache` to use overlapped write and reads, respectively, to reduce end-to-end latency. The read path is a common operation on each iteration of the propeller `Handle` loop for dynamic nodes.

Signed-off-by: Andrew Dye <[email protected]>

* Fix async notifications tests

I didn't chase down why assumptions changed here and why these tests broke, but fixing them with more explicit checks.

Signed-off-by: Andrew Dye <[email protected]>

* Overlap fetching input and output data

This change updates `GetExecutionData`, `GetNodeExecutionData`, and `GetTaskExecutionData` to use overlapped reads when fetching input and output data.

Signed-off-by: Andrew Dye <[email protected]>

* Add configuration for launchplan cache resync duration

Currently, the launchplan cache resync duration uses the DownstreamEval duration configuration which is also used for the sync period on the k8s client. This means if we want to configure a more aggressive launchplan cache resync, we would also incur overhead in syncing all k8s resources (ex. Pods from `PodPlugin`). By adding a separate configuration value we can update these independently.

Signed-off-by: Andrew Dye <[email protected]>

* Enqueue owner on launchplan terminal state

This PR enqueues the owner workflow for evaluation when the launchplan auto refresh cache detects a launchplan in a terminal state.

Signed-off-by: Andrew Dye <[email protected]>

* Add client-go metrics

Register a few metric callbacks with the client-go metrics interface so that we can monitor request latencies and rate limiting of kubeclient.

```
❯ curl http://localhost:10254/metrics | rg k8s_client
k8s_client_rate_limiter_latency_bucket{verb="GET",le="0.005"} 84
k8s_client_rate_limiter_latency_bucket{verb="GET",le="0.01"} 87
k8s_client_rate_limiter_latency_bucket{verb="GET",le="0.025"} 89
k8s_client_rate_limiter_latency_bucket{verb="GET",le="0.05"} 99
k8s_client_rate_limiter_latency_bucket{verb="GET",le="0.1"} 114
k8s_client_rate_limiter_latency_bucket{verb="GET",le="0.25"} 117
k8s_client_rate_limiter_latency_bucket{verb="GET",le="0.5"} 117
k8s_client_rate_limiter_latency_bucket{verb="GET",le="1"} 117
k8s_client_rate_limiter_latency_bucket{verb="GET",le="2.5"} 117
k8s_client_rate_limiter_latency_bucket{verb="GET",le="5"} 117
k8s_client_rate_limiter_latency_bucket{verb="GET",le="10"} 117
k8s_client_rate_limiter_latency_bucket{verb="GET",le="+Inf"} 117
k8s_client_rate_limiter_latency_sum{verb="GET"} 1.9358371670000003
k8s_client_rate_limiter_latency_count{verb="GET"} 117
k8s_client_rate_limiter_latency_bucket{verb="POST",le="0.005"} 6
k8s_client_rate_limiter_latency_bucket{verb="POST",le="0.01"} 6
k8s_client_rate_limiter_latency_bucket{verb="POST",le="0.025"} 6
k8s_client_rate_limiter_latency_bucket{verb="POST",le="0.05"} 6
k8s_client_rate_limiter_latency_bucket{verb="POST",le="0.1"} 6
k8s_client_rate_limiter_latency_bucket{verb="POST",le="0.25"} 6
k8s_client_rate_limiter_latency_bucket{verb="POST",le="0.5"} 6
k8s_client_rate_limiter_latency_bucket{verb="POST",le="1"} 6
k8s_client_rate_limiter_latency_bucket{verb="POST",le="2.5"} 6
k8s_client_rate_limiter_latency_bucket{verb="POST",le="5"} 6
k8s_client_rate_limiter_latency_bucket{verb="POST",le="10"} 6
k8s_client_rate_limiter_latency_bucket{verb="POST",le="+Inf"} 6
k8s_client_rate_limiter_latency_sum{verb="POST"} 1.0542e-05
k8s_client_rate_limiter_latency_count{verb="POST"} 6
k8s_client_rate_limiter_latency_bucket{verb="PUT",le="0.005"} 1
k8s_client_rate_limiter_latency_bucket{verb="PUT",le="0.01"} 1
k8s_client_rate_limiter_latency_bucket{verb="PUT",le="0.025"} 1
k8s_client_rate_limiter_latency_bucket{verb="PUT",le="0.05"} 1
k8s_client_rate_limiter_latency_bucket{verb="PUT",le="0.1"} 1
k8s_client_rate_limiter_latency_bucket{verb="PUT",le="0.25"} 1
k8s_client_rate_limiter_latency_bucket{verb="PUT",le="0.5"} 1
k8s_client_rate_limiter_latency_bucket{verb="PUT",le="1"} 1
k8s_client_rate_limiter_latency_bucket{verb="PUT",le="2.5"} 1
k8s_client_rate_limiter_latency_bucket{verb="PUT",le="5"} 1
k8s_client_rate_limiter_latency_bucket{verb="PUT",le="10"} 1
k8s_client_rate_limiter_latency_bucket{verb="PUT",le="+Inf"} 1
k8s_client_rate_limiter_latency_sum{verb="PUT"} 5e-07
k8s_client_rate_limiter_latency_count{verb="PUT"} 1
k8s_client_request_latency_bucket{verb="GET",le="0.005"} 84
k8s_client_request_latency_bucket{verb="GET",le="0.01"} 86
k8s_client_request_latency_bucket{verb="GET",le="0.025"} 89
k8s_client_request_latency_bucket{verb="GET",le="0.05"} 99
k8s_client_request_latency_bucket{verb="GET",le="0.1"} 112
k8s_client_request_latency_bucket{verb="GET",le="0.25"} 117
k8s_client_request_latency_bucket{verb="GET",le="0.5"} 117
k8s_client_request_latency_bucket{verb="GET",le="1"} 117
k8s_client_request_latency_bucket{verb="GET",le="2.5"} 117
k8s_client_request_latency_bucket{verb="GET",le="5"} 117
k8s_client_request_latency_bucket{verb="GET",le="10"} 117
k8s_client_request_latency_bucket{verb="GET",le="+Inf"} 117
k8s_client_request_latency_sum{verb="GET"} 2.1254330859999997
k8s_client_request_latency_count{verb="GET"} 117
k8s_client_request_latency_bucket{verb="POST",le="0.005"} 5
k8s_client_request_latency_bucket{verb="POST",le="0.01"} 5
k8s_client_request_latency_bucket{verb="POST",le="0.025"} 5
k8s_client_request_latency_bucket{verb="POST",le="0.05"} 6
k8s_client_request_latency_bucket{verb="POST",le="0.1"} 6
k8s_client_request_latency_bucket{verb="POST",le="0.25"} 6
k8s_client_request_latency_bucket{verb="POST",le="0.5"} 6
k8s_client_request_latency_bucket{verb="POST",le="1"} 6
k8s_client_request_latency_bucket{verb="POST",le="2.5"} 6
k8s_client_request_latency_bucket{verb="POST",le="5"} 6
k8s_client_request_latency_bucket{verb="POST",le="10"} 6
k8s_client_request_latency_bucket{verb="POST",le="+Inf"} 6
k8s_client_request_latency_sum{verb="POST"} 0.048558582
k8s_client_request_latency_count{verb="POST"} 6
k8s_client_request_latency_bucket{verb="PUT",le="0.005"} 1
k8s_client_request_latency_bucket{verb="PUT",le="0.01"} 1
k8s_client_request_latency_bucket{verb="PUT",le="0.025"} 1
k8s_client_request_latency_bucket{verb="PUT",le="0.05"} 1
k8s_client_request_latency_bucket{verb="PUT",le="0.1"} 1
k8s_client_request_latency_bucket{verb="PUT",le="0.25"} 1
k8s_client_request_latency_bucket{verb="PUT",le="0.5"} 1
k8s_client_request_latency_bucket{verb="PUT",le="1"} 1
k8s_client_request_latency_bucket{verb="PUT",le="2.5"} 1
k8s_client_request_latency_bucket{verb="PUT",le="5"} 1
k8s_client_request_latency_bucket{verb="PUT",le="10"} 1
k8s_client_request_latency_bucket{verb="PUT",le="+Inf"} 1
k8s_client_request_latency_sum{verb="PUT"} 0.002381375
k8s_client_request_latency_count{verb="PUT"} 1
k8s_client_request_total{code="200",method="GET"} 120
k8s_client_request_total{code="200",method="PUT"} 1
k8s_client_request_total{code="409",method="POST"} 6
```

Signed-off-by: Andrew Dye <[email protected]>

* Histogram Bucket Options

Add abstraction to be able to pass buckets custom defined to histogram vectors.

Signed-off-by: Andrew Dye <[email protected]>

* Add org to CreateUploadLocation

Signed-off-by: Andrew Dye <[email protected]>

* Add config for grpc MaxMessageSizeBytes

We need to make the grpc max recv message size in propeller's admin client configurable to match the server-side configuration we support in admin.

Signed-off-by: Andrew Dye <[email protected]>

* Move storage cache settings to correct location

Signed-off-by: Andrew Dye <[email protected]>

* added lock to memstore make threadsafe

Signed-off-by: Andrew Dye <[email protected]>

* Add read replica host config and connection

- Add a new field to the postgres db config struct, `readReplicaHost`.
- Add a new endpoint in the `database` package to enable establishing a connection with a db without creating it if it doesn't exist

Signed-off-by: Andrew Dye <[email protected]>

* Fix type assertion when an event is missed while connection to apiser…

…ver was severed

Signed-off-by: Andrew Dye <[email protected]>

* Log and monitor failures to validate access tokens

Signed-off-by: Andrew Dye <[email protected]>

* Dask dashboard should have a separate log config

Signed-off-by: Andrew Dye <[email protected]>

* adjust Dask LogName to (Dask Runner Logs)

Signed-off-by: Andrew Dye <[email protected]>

* Fix k3d local setup prefix

I was trying to use `setup_local_dev.sh`, and it wasn't working out of the box. Looks like it expects `k3d-` prefix for the kubecontext

Ran `setup_local_dev.sh`

Signed-off-by: Andrew Dye <[email protected]>

* Override ArrayNode log links with map plugin

This PR adds a configuration option to override ArrayNode log links with those defined in the map plugin. The map plugin contains it's own configuration for log links, which may differ from those defined on the PodPlugin. ArrayNode, executing subNodes as regular tasks (ie. using the PodPlugin) means that it uses the default PodPlugin log templates.

Signed-off-by: Andrew Dye <[email protected]>

* Add histogram stopwatch to stow storage

This change
* Adds a new `HistogramStopWatch` to promutils. This [allows for aggregating latencies](https://prometheus.io/docs/practices/histograms/#quantiles) across pods and computing quantiles at query time
* Adds `HistogramStopWatch` latency metrics for stow so that we can reason about storage latencies in aggregate. Existing latency metrics remain.

- [x] Added unittests

Signed-off-by: Andrew Dye <[email protected]>

* Fix metrics scale division in timer

* Fix metrics scale division in timer

Signed-off-by: Iaroslav Ciupin <[email protected]>

Signed-off-by: Andrew Dye <[email protected]>

* CreateDownloadLink: Head before signing

Signed-off-by: Andrew Dye <[email protected]>

* Unexpectedly deleted pod metrics

* Count when we see unexpectedly terminated pods

Signed-off-by: Andrew Dye <[email protected]>

* Don't send inputURI for start-node

* send empty `inputUri` for `start-node` in node execution event to flyteadmin and therefore, GetNodeExecutionData will not attempt to download non-existing inputUri as it was doing before this change.
* add DB migration to clear `input_uri` in existing `node_executions` table for start nodes.

Signed-off-by: Andrew Dye <[email protected]>

* Fix cluster pool assignment validation

Signed-off-by: Andrew Dye <[email protected]>

---------

Signed-off-by: Andrew Dye <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Dan Rammer <[email protected]>
Co-authored-by: Joe Eschen <[email protected]>
Co-authored-by: Katrina Rogan <[email protected]>
Co-authored-by: Michael Barrientos <[email protected]>
Co-authored-by: Haytham Abuelfutuh <[email protected]>
Co-authored-by: Jan Fiedler <[email protected]>
Co-authored-by: Iaroslav Ciupin <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>

---------

Signed-off-by: Rafael Raposo <[email protected]>
Signed-off-by: Andrew Dye <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Rafael Raposo <[email protected]>
Co-authored-by: Andrew Dye <[email protected]>
Co-authored-by: Dan Rammer <[email protected]>
Co-authored-by: Joe Eschen <[email protected]>
Co-authored-by: Katrina Rogan <[email protected]>
Co-authored-by: Michael Barrientos <[email protected]>
Co-authored-by: Haytham Abuelfutuh <[email protected]>
Co-authored-by: Jan Fiedler <[email protected]>
Co-authored-by: Iaroslav Ciupin <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
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