Skip to content
This repository was archived by the owner on Oct 9, 2023. It is now read-only.

Always register the metadata endpoint#392

Merged
EngHabu merged 6 commits intomasterfrom
service-http-endpoint
Apr 11, 2022
Merged

Always register the metadata endpoint#392
EngHabu merged 6 commits intomasterfrom
service-http-endpoint

Conversation

@EngHabu
Copy link
Contributor

@EngHabu EngHabu commented Apr 7, 2022

Signed-off-by: Haytham Abuelfutuh haytham@afutuh.com

TL;DR

Expose a new property in Public Client metadata and always registers the metadata endpoint (even when auth is not configured)

Type

  • Bug Fix
  • Feature
  • Plugin

Tracking Issue

fixes https://github.com/flyteorg/flyte/issues/

Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com>

DataProxy DataProxyConfig `json:"dataProxy" pflag:",Defines data proxy configuration."`

ExportedDNS config.URL `json:"exportedDNS" pflag:",Defines the http endpoint the service is accessible at."`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What should I call this 😬, @wild-endeavor ?

Copy link
Contributor

Choose a reason for hiding this comment

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

serviceEndpoint?

Copy link
Contributor

Choose a reason for hiding this comment

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

HTTP_URL? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a comment to explain under what circumstance this should be used?

@codecov
Copy link

codecov bot commented Apr 7, 2022

Codecov Report

Merging #392 (90dd77c) into master (1199b48) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head 90dd77c differs from pull request most recent head 0ec309b. Consider uploading reports for the commit 0ec309b to get more accurate results

@@            Coverage Diff             @@
##           master     #392      +/-   ##
==========================================
- Coverage   61.11%   61.08%   -0.04%     
==========================================
  Files         154      154              
  Lines       11107    11113       +6     
==========================================
  Hits         6788     6788              
- Misses       3611     3617       +6     
  Partials      708      708              
Flag Coverage Δ
unittests ?

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

Impacted Files Coverage Δ
auth/handlers.go 31.14% <ø> (ø)
pkg/config/config.go 20.00% <ø> (ø)
auth/authzserver/metadata_provider.go 80.35% <100.00%> (+0.72%) ⬆️
pkg/config/serverconfig_flags.go 58.69% <100.00%> (+0.91%) ⬆️
pkg/manager/impl/execution_manager.go 74.03% <100.00%> (+0.07%) ⬆️
...implementations/workflow_execution_event_writer.go 40.00% <0.00%> (-40.00%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1199b48...0ec309b. Read the comment docs.

EngHabu added 3 commits April 7, 2022 15:31
Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com>
Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com>
@EngHabu EngHabu marked this pull request as ready for review April 9, 2022 00:18
@EngHabu EngHabu requested review from kumare3 and wild-endeavor April 9, 2022 00:18

configuration := runtime2.NewConfigurationProvider()
service.RegisterAdminServiceServer(grpcServer, adminservice.NewAdminServer(ctx, pluginRegistry, configuration, cfg.KubeConfig, cfg.Master, dataStorageClient, scope.NewSubScope("admin")))
service.RegisterAuthMetadataServiceServer(grpcServer, authCtx.AuthMetadataService())
Copy link
Contributor

Choose a reason for hiding this comment

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

how does this work if UseAuth isn't true?

katrogan
katrogan previously approved these changes Apr 9, 2022
Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com>
kumare3
kumare3 previously approved these changes Apr 10, 2022
Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com>
@EngHabu EngHabu merged commit f04414e into master Apr 11, 2022
@EngHabu EngHabu deleted the service-http-endpoint branch April 11, 2022 19:52
katrogan pushed a commit that referenced this pull request Apr 12, 2022
katrogan pushed a commit that referenced this pull request Apr 12, 2022
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
* Always register the metadata endpoint

Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com>

* Rename

Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com>

* Using released flyteidl

Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com>

* lint

Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com>

* Initialize authz metadata server

Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com>
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants