Add webapi v2 endpoints for creating discovery token and enrolling eks with labels #50472

kimlisa · 2024-12-20T07:17:19Z

The endpoint for creating a token for discovery flow now allows user defined labels to be set as part of suggested_labels (this PR)
The endpoint for enrolling eks clusters recently added ability to add user defined labels as extraLabels (Discover EKS: allow custom labels for Kube Server #49420)

Both scenarios if a user tries to create token/resource and provides labels, if requests goes to an older proxy, it'll look like the request succeeded but the labels will not have been set. So this PR defines v2 endpoints, so that if request goes to an older proxy, a 404 error will return, which we will assume it's because of version mismatch.

This PR also returns the version number of proxy when a route wasn't matched.

Rendered example of route not matched 404 error:

if proxyVersion is returned:

if proxyVersion is not returned:

kimlisa · 2024-12-20T07:24:51Z

we do set version with app hash as a etag on the response headers for app.js, but I wasn't sure how to actually extract that, so I opted for a simpler meta tag that just stores the version instead

tigrato · 2024-12-20T09:19:07Z

lib/web/apiserver.go

 	// used for creating tokens used during guided discover flows
 	h.POST("/webapi/token", h.WithAuth(h.createTokenForDiscoveryHandle))
+	// used for creating tokens used during guided discover flows
+	h.POST("/v2/webapi/token", h.WithAuth(h.createTokenForDiscoveryHandle))


Not sure this will align well with our weird v1 auto prefix:

teleport/lib/web/apiserver.go

Lines 618 to 622 in c222030

// request is going to the API?

if strings.HasPrefix(r.URL.Path, apiPrefix) {

http.StripPrefix(apiPrefix, h).ServeHTTP(w, r)

return

}

This /v2/ will make /v1/v2/webapi/token to work as well 😆

We need to fix the weird prefixing issue first

One possible way is to inverse the handler.

Append the /v1/ to every single route h.*("/v1...".

if strings.HasPrefix(r.URL.Path,"/webapi") { addPrefix(apiPrefix, h).ServeHTTP(w, r) return } func addPrefix(prefix string, h Handler) Handler { if prefix == "" { return h } return http.HandlerFunc(func(w ResponseWriter, r *Request) { p := path.Join(prefix,r.URL.Path) rp := path.Join(prefix,r.URL.RawPath) r2 := new(Request) *r2 = *r r2.URL = new(url.URL) *r2.URL = *r.URL r2.URL.Path = p r2.URL.RawPath = rp h.ServeHTTP(w, r2) }) }

thanks for catching that, i completely overlooked this part

looks like @avatus already thought of that though in his RFD.

I'll apply his logic and test things out

web/packages/teleport/src/services/webUiVersion/webUiVersion.ts

zmb3 · 2024-12-23T16:37:09Z

I'm not sure pulling the version from the HTML document makes sense here.

If a user runs into this error, it's because they have a newer web UI that knows how to use the new V2 endpoint with labels, but they ended up hitting an older proxy that doesn't know how to handle the request.

If you show them the version from the HTML document, you're going to show them the newer web UI version, which is actually new enough.

If we want this error message to make sense, we need to show the version of the proxy that is too old. (This means we probably need the backend to surface this information)

kimlisa · 2025-01-02T18:45:11Z

lib/web/apiserver.go

+// apiPrefixRegex matches pathnames starting with /v<version num>/<any characters>
+var apiPrefixRegex = regexp.MustCompile(`^/v(\d+)/(.+)`)
+


I ended up going with this approach to keep the old behavior the same. I could've done something stricter where regex is like ^/v(\d+)/(webapi|enterprise|scripts|.well-known|workload-identity) but once you introduce another non-prefix leading path, you have to remember to update this regex.

For cases where route is double prefixed like this /v1/v2/webapi, I do another regex match on the path after the first prefix to prevent that.

I couldn't see other issues with this approach but let me know.

kimlisa · 2025-01-02T18:50:37Z

lib/client/https_client.go

@@ -62,7 +61,7 @@ func httpTransport(insecure bool, pool *x509.CertPool) *http.Transport {

 func NewWebClient(url string, opts ...roundtrip.ClientParam) (*WebClient, error) {
 	opts = append(opts, roundtrip.SanitizerEnabled(true))
-	clt, err := roundtrip.NewClient(url, teleport.WebAPIVersion, opts...)
+	clt, err := roundtrip.NewClient(url, "", opts...)


The version parameter refers to this field, and from what i can tell, its only purpose is to just append the version prefix when calling this function (which we use a lot in our tests, and in auth here)

I didn't think it was necessary to define the version, since we are just going to strip it off anyways, and it wouldn't work with v2 endpoints

gzdunek · 2025-01-03T12:11:26Z

web/packages/teleport/src/services/api/parseError.ts

@@ -29,6 +52,12 @@ export default function parseError(json) {
  return msg;
 }

+export function parseProxyVersion(json): ProxyVersion | null {


This won't return null but undefined.

Suggested change

export function parseProxyVersion(json): ProxyVersion | null {

export function parseProxyVersion(json): ProxyVersion | undefined {

gzdunek · 2025-01-03T12:22:46Z

web/packages/build/vite/html.ts

@@ -117,6 +117,7 @@ export function htmlPlugin(target: string): Plugin {

            replaceMetaTag('grv_csrf_token', source, result);
            replaceMetaTag('grv_bearer_token', source, result);
+            replaceMetaTag('teleport_version', source, result);


This is a leftover, right?

gzdunek · 2025-01-03T12:30:54Z

web/packages/teleport/src/services/version/unsupported.ts

+  err: unknown
+): never {
+  if (err instanceof ApiError && err.response.status === 404) {
+    if (err.proxyVersion && err.proxyVersion.string) {


Having proxyVersion will be probably useful in the future, but for this particular feature we won't be able to show it. Returning proxy version will land in the same version as the V2 endpoint, so we either won't show the error or show the more generic version from below.

yeah that's the plan, to make it fallback to the generic messaging for the time being

So we can remove the more specific version as we will never show it.

oh that's true! i'll remove it

gzdunek · 2025-01-03T12:32:19Z

web/packages/teleport/src/services/integrations/integrations.test.ts

+  );
+});
+
+test('enrollEksClusters with labbels calls v2', async () => {


Suggested change

test('enrollEksClusters with labbels calls v2', async () => {

test('enrollEksClusters with labels calls v2', async () => {

gzdunek · 2025-01-03T12:33:33Z

web/packages/teleport/src/services/api/parseError.ts

+ * Currently, the proxy version field is only returned
+ * with api routes "not found" error.
+ *
+ * Used to determine out dated proxies.


Suggested change

* Used to determine out dated proxies.

* Used to determine outdated proxies.

gzdunek · 2025-01-03T12:40:05Z

web/packages/teleport/src/Integrations/Enroll/AwsOidc/AwsOidc.test.tsx

@@ -125,7 +125,7 @@ test('generate command', async () => {

  // Test create is still called with 404 ping error.
  jest.clearAllMocks();
-  let error = new ApiError('', { status: 404 } as Response);
+  let error = new ApiError('', { status: 404 } as Response, null);


I think the proxyVersion in ApiError could be optional, so we wouldn't have to pass null everywhere.

i made it into an object param

gzdunek · 2025-01-03T12:41:10Z

web/packages/teleport/src/services/api/parseError.ts

@@ -29,6 +52,12 @@ export default function parseError(json) {
  return msg;
 }

+export function parseProxyVersion(json): ProxyVersion | null {
+  if (json && json.fields && json.fields.proxyVersion) {


Optional chaining?

avatus · 2025-01-03T15:17:36Z

web/packages/teleport/src/config.ts

+  getJoinTokenUrlV2() {
+    return cfg.api.joinTokenPathV2;
+  },
+


This might get a bit unruly now that I think about it. We have the same "path" like /webapi/tokens but now each of the different http methods might go to some separate version. so this could theoretically have as many different versions as we have http methods.

I don't suggest any change here, but we should think about how to handle this in the future. perhaps we could maybe pass a version number as argument or something. or maybe this isnt a problem at al and im just overthinking it

i did mark old endpoints for deletion eg // TODO(kimlisa): DELETE IN 19.0 - replaced by /v2/webapi/token, so with clean up it wouldn't be so bad?

oh that reminds me, perhaps i can also mark it deprecated in case anyone tries to use these endpoints outside of the service/* directory

but we wont delete the old one in v19. because other services use this path, just not the v2. for example. GET doesnt use v2.

oh i see what you mean now. though with these specific endpoints marked for deletion, there is only one HTTP verb attached to it

but you raise a really good question... how do we want to handle same paths but with differing HTTP verbs and versions?

i think we have to start being specific about the api endpoints, something like this maybe?

// before: usersPath: '/v1/webapi/users' <--- used with multiple HTTP verbs // after: user: { create: '/v1/webapi/users', update: '/v1/webapi/users/:username', delete: '/v1/webapi/users/:username', <-- kept for backward compat deleteV2: '/v2/webapi/users/:username', <-- new feature }

we start doing something like it here

would that be a good solution? for example webapi/token, it'll look like this:

discoveryToken: { // TODO(kimlisa): DELETE IN 19.0 - replaced by /v2/webapi/token create: '/v1/webapi/token' createV2: '/v2/webapi/token' }

yeah i think an object is best. i suggested that to ryan when he added those paths in and i think it'd be a good solution to versioning in the future as well. at least, i dont have any better suggestions yet.

ok moving forward with this, i also updated RFD #50743 with it

Thanks for the update!

web/packages/teleport/src/services/webUiVersion/webUiVersion.ts

avatus · 2025-01-03T15:32:12Z

lib/web/apiserver.go

+		if matches := apiPrefixRegex.FindStringSubmatch(r.URL.Path); matches != nil && len(matches) > 1 {
+			versionNum := matches[1]
+			if versionNum == "1" {
+				// Try path matching again with the stripped prefix.


i can't seem to follow this. wouldn't this catch any version prefixed api path, and if its not v1, then it replies with not found? where does it send the other version number requests to? how does it break out of this

so this entire block of code is only ran if the first attempt at routing match failed. if the routing match failed, and the path is prefixed with v1 we strip it and serve it again (so essentially each path gets a second chance)

i also added more comments to this block on the newest changes that may explain it more?

that makes sense, thats why its called notFound handler now. ok i like it

…d err Include proxy version with the path not found error

github-actions bot added size/md ui labels Dec 20, 2024

github-actions bot requested review from avatus and gzdunek December 20, 2024 07:17

kimlisa requested review from flyinghermit and removed request for avatus December 20, 2024 07:17

kimlisa changed the title ~~Lisa/add v2 endpoint~~ Add webapi v2 endpoints for creating discovery token and enrolling eks with labels Dec 20, 2024

kimlisa added the backport/branch/v17 label Dec 20, 2024

kimlisa force-pushed the lisa/add-v2-endpoint branch from b51dded to 0a6aebc Compare December 20, 2024 07:21

kimlisa requested a review from marcoandredinis December 20, 2024 07:26

tigrato reviewed Dec 20, 2024

View reviewed changes

gzdunek reviewed Dec 23, 2024

View reviewed changes

zmb3 reviewed Dec 23, 2024

View reviewed changes

web/packages/teleport/src/services/webUiVersion/webUiVersion.ts Outdated Show resolved Hide resolved

kimlisa force-pushed the lisa/add-v2-endpoint branch from 4db33f1 to 15a7e75 Compare December 30, 2024 07:57

kimlisa mentioned this pull request Dec 30, 2024

WebDiscover: allow custom labels for single resource enroll (server, kube, eks, rds) #50606

Open

5 tasks

kimlisa force-pushed the lisa/add-v2-endpoint branch from 15a7e75 to 0729ad9 Compare December 31, 2024 22:57

kimlisa added backport/branch/v16 no-changelog Indicates that a PR does not require a changelog entry labels Dec 31, 2024

kimlisa marked this pull request as draft December 31, 2024 23:33

kimlisa force-pushed the lisa/add-v2-endpoint branch 4 times, most recently from 410b710 to 538b042 Compare January 2, 2025 18:41

kimlisa commented Jan 2, 2025

View reviewed changes

kimlisa force-pushed the lisa/add-v2-endpoint branch from 538b042 to c6d7694 Compare January 2, 2025 18:58

kimlisa marked this pull request as ready for review January 2, 2025 19:20

github-actions bot requested a review from ryanclark January 2, 2025 19:21

kimlisa force-pushed the lisa/add-v2-endpoint branch from c6d7694 to 3ed0168 Compare January 2, 2025 20:31

kimlisa requested review from gzdunek, tigrato, zmb3 and avatus January 2, 2025 20:31

kimlisa force-pushed the lisa/add-v2-endpoint branch from 3ed0168 to 67c5614 Compare January 3, 2025 06:15

gzdunek reviewed Jan 3, 2025

View reviewed changes

avatus reviewed Jan 3, 2025

View reviewed changes

kimlisa mentioned this pull request Jan 3, 2025

Add how to define new endpoints in web for api-versioning.md #50743

Open

kimlisa added 7 commits January 3, 2025 13:05

Create v2 web api endpoint adding user defined labels

015f137

Web: add version two endpoint

b903d0f

Swap wording

8513cec

Add strict api path check and return JSON response with path not foun…

dcbad3c

…d err Include proxy version with the path not found error

Web: parse the proxy version from error if any

36fb415

Remove meta tag

449913c

Lax regex and remove prefix versioning on new web clients

03d5df0

avatus self-requested a review January 3, 2025 21:06

kimlisa requested a review from gzdunek January 3, 2025 22:12

Address CRs and fix vite proxy paths config

fb39fee

kimlisa force-pushed the lisa/add-v2-endpoint branch from 67c5614 to fb39fee Compare January 3, 2025 22:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add webapi v2 endpoints for creating discovery token and enrolling eks with labels #50472

Add webapi v2 endpoints for creating discovery token and enrolling eks with labels #50472

kimlisa commented Dec 20, 2024 •

edited

Loading

kimlisa commented Dec 20, 2024

tigrato Dec 20, 2024

tigrato Dec 20, 2024

kimlisa Dec 20, 2024

zmb3 commented Dec 23, 2024 •

edited

Loading

kimlisa Jan 2, 2025

kimlisa Jan 2, 2025 •

edited

Loading

gzdunek Jan 3, 2025

gzdunek Jan 3, 2025

gzdunek Jan 3, 2025

kimlisa Jan 3, 2025

gzdunek Jan 3, 2025

kimlisa Jan 3, 2025

gzdunek Jan 3, 2025

gzdunek Jan 3, 2025

gzdunek Jan 3, 2025

kimlisa Jan 3, 2025

gzdunek Jan 3, 2025

avatus Jan 3, 2025

kimlisa Jan 3, 2025

kimlisa Jan 3, 2025

avatus Jan 3, 2025

kimlisa Jan 3, 2025 •

edited

Loading

avatus Jan 3, 2025 •

edited

Loading

kimlisa Jan 3, 2025

avatus Jan 3, 2025

avatus Jan 3, 2025

kimlisa Jan 3, 2025 •

edited

Loading

avatus Jan 3, 2025

	// request is going to the API?
	if strings.HasPrefix(r.URL.Path, apiPrefix) {
	http.StripPrefix(apiPrefix, h).ServeHTTP(w, r)
	return
	}

		// apiPrefixRegex matches pathnames starting with /v<version num>/<any characters>
		var apiPrefixRegex = regexp.MustCompile(`^/v(\d+)/(.+)`)

	export function parseProxyVersion(json): ProxyVersion \| null {
	export function parseProxyVersion(json): ProxyVersion \| undefined {

	test('enrollEksClusters with labbels calls v2', async () => {
	test('enrollEksClusters with labels calls v2', async () => {

	* Used to determine out dated proxies.
	* Used to determine outdated proxies.

Add webapi v2 endpoints for creating discovery token and enrolling eks with labels #50472

Are you sure you want to change the base?

Add webapi v2 endpoints for creating discovery token and enrolling eks with labels #50472

Conversation

kimlisa commented Dec 20, 2024 • edited Loading

kimlisa commented Dec 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmb3 commented Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

kimlisa Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimlisa Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

avatus Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimlisa Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimlisa commented Dec 20, 2024 •

edited

Loading

zmb3 commented Dec 23, 2024 •

edited

Loading

kimlisa Jan 2, 2025 •

edited

Loading

kimlisa Jan 3, 2025 •

edited

Loading

avatus Jan 3, 2025 •

edited

Loading

kimlisa Jan 3, 2025 •

edited

Loading