THREESCALE-8508 - /admin/api/account/proxy_configs endpoint for configuration loading #1352

samugi · 2022-06-06T15:55:19Z

This PR uses /admin/api/account/proxy_configs for the standard configuration loading process, as the default endpoint that APIcast will attempt to fetch the configuration from when the portal endpoint is provided without a path (admin portal endpoint).

With this proposed implementation, APICAST_LOAD_SERVICES_WHEN_NEEDED is deprecated and the configuration is fetched "when needed" by default whenever possible, with the following logic:

THREESCALE_PORTAL_ENDPOINT has no path
- APICAST_CONFIGURATION_LOADER=boot
  - http://${THREESCALE_PORTAL_ENDPOINT}/admin/api/account/proxy_configs/<env>.json?version=version (i.e. all services from proxy_configs endpoint). Using the new endpoint now one request is enough, no need to loop through all services.
- APICAST_CONFIGURATION_LOADER=lazy
  - http://${THREESCALE_PORTAL_ENDPOINT}/admin/api/account/proxy_configs/<env>.json?host=host&version=version (i.e. services that match host from proxy_configs endpoint). This matches the current behaviour when APICAST_LOAD_SERVICES_WHEN_NEEDED is enabled.
THREESCALE_PORTAL_ENDPOINT has a path
- APICAST_CONFIGURATION_LOADER=boot
  - http://${THREESCALE_PORTAL_ENDPOINT}/<env>.json (i.e. all services from master endpoint). Same as the current behaviour.
- APICAST_CONFIGURATION_LOADER=lazy
  - http://${THREESCALE_PORTAL_ENDPOINT}/<env>.json?host=host (i.e. services that match host from master endpoint). Same as the current behaviour.

i.e. the path appended to ${THREESCALE_PORTAL_ENDPOINT} is:

	APICAST_CONFIGURATION_LOADER=boot	APICAST_CONFIGURATION_LOADER=lazy
endpoint has no path	/admin/api/account/proxy_configs/${env}.json?version=version	/admin/api/account/proxy_configs/${env}.json?host=host&version=version
endpoint has a path	/${env}.json	/${env}.json?host=host

Update : When any of the following environment variables exist, the gateway will load services one by one.

APICAST_SERVICE_%s_CONFIGURATION_VERSION
~~APICAST_SERVICES_LIST~~
~~APICAST_SERVICES_FILTER_BY_URL~~

These environment variables are not compatible when pointing to master (when a custom path is provided). Added a check to log error when that incompatible scenario happens.

update: APICAST_SERVICES_FILTER_BY_URL and APICAST_SERVICES_LIST were added as incompatible with loading config with the new endpoint. Actually, those env vars are compatible with all loading modes. This issue will be fixed in #1397

eguzki · 2022-06-27T10:00:46Z

gateway/src/apicast/configuration_loader/remote_v2.lua

+-- http://${THREESCALE_PORTAL_ENDPOINT}/admin/api/account/proxy_configs/<env>.json?host=host&version=version
+local function configuration_endpoint_params(env, host, portal_endpoint_path)
+  local version = resty_env.get(
+      format('APICAST_SERVICE_%s_CONFIGURATION_VERSION', service_id)


where is this service_id coming from?

you are right @eguzki we don't have that info there, I missed it, and even if we did, it wouldn't be the correct logic to apply because the version of a single service would apply to all services returned by the API endpoint. This means when the APICAST_SERVICE_%s_CONFIGURATION_VERSION env var is set we should use the old logic and endpoints because we need to retrieve each service's configuration individually.

apparently, that environment variable (for configuring a specific service version) only works when specific conditions are met:

THREESCALE_PORTAL_ENDPOINT points to an admin portal, and

APICAST_LOAD_SERVICES_WHEN_NEEDED is not configured

Because in order to fetch a specific version it's necessary to load services individually (so that the version can be specified for each service configuration URL). And this is not possible via master or the new services endpoint. This is currently the behavior we have also in master and even in previous stable versions.

For now, I updated the logic to fall back to the "old" endpoint and load services one by one when APICAST_SERVICE_%s_CONFIGURATION_VERSION is configured (and THREESCALE_PORTAL_ENDPOINT is an admin portal), and I added logs to notify when services will be loaded this way, individually. This means we would preserve the current behaviour when the version env var is set.

I added 2 tests too.

We need to verify and confirm that this is the best way to handle things.

I added the same "exceptions" for APICAST_SERVICES_LIST and APICAST_SERVICES_FILTER_BY_URL env vars.

eguzki · 2022-06-27T10:31:42Z

Code looks ok. Also tests.

Just to verify: APIcast no longer will request proxy configuration for a given service ID. When "lazy" configuration, the endpoint will have a query param for the given "host". Right?

Question: what if two services have the same host?

eguzki · 2022-06-27T10:53:50Z

For managed APIcast deployment (on-prem releases), the THREESCALE_PORTAL_ENDPOINT env var has path configured to /master/api/proxy/configs
https://github.com/3scale/3scale-operator/blob/69c1e2c41b2b9d308278e513d07d405b5d8196e6/pkg/3scale/amp/component/system_options.go#L114

This is still valid or should be changed?

samugi · 2022-06-27T20:42:15Z

Code looks ok. Also tests.

Just to verify: APIcast no longer will request proxy configuration for a given service ID. When "lazy" configuration, the endpoint will have a query param for the given "host". Right?

Question: what if two services have the same host?

That's right. If multiple services have the same host they will all be loaded in APIcast at once and treated as usual.

samugi · 2022-06-27T20:45:28Z

Code looks ok. Also tests.

Just to verify: APIcast no longer will request proxy configuration for a given service ID. When "lazy" configuration, the endpoint will have a query param for the given "host". Right?

Question: what if two services have the same host?

That's fine @eguzki, since it points to master, this PR should have no effect on gateways configured that way.

secretsurf · 2022-07-20T11:00:48Z

Is this lost in the ether?

kevprice83 · 2022-07-22T17:00:12Z

Is this lost in the ether?

No @secretsurf sorry I am supposed to be reviewing and testing this but too many other burning issues I didn't manage to get to this yet.

eguzki · 2022-09-20T08:43:22Z

One thing I concern about this is the paginated endpoint. From the description in the issue

Open Points:  

    Currently the endpoint /admin/api/account/proxy_configs/${ENV}

 is paginated, and APIcast does not handle pages, so we would need to 
make the pagination optional or to implement pagination on APIcast too before we can proceed with this

and I do not remember to see any pagination implementation. Maybe it was done after my initial review

kevprice83

I think this PR is missing documentation because of how it behaves with different env vars. Without that documentation it will result in users run into issues as they won't understand what is the expected behaviour. Also a decision needs to be made whether we want to split the implementation into 2 iterations as this PR will not support >500 services but it does introduce support for >500 versions of a service's proxy config object.

eguzki · 2023-03-15T09:48:33Z

@pehala @thomasmaas this PR is ready for review

eguzki · 2023-03-15T09:49:24Z

leaving pagination implementation for a (soon) coming PR

dfennessy

LGTM!

pehala

This seems like a breaking change, can you enable the old behaviour? Does enabling the old behaviour even make sense?

eguzki · 2023-03-15T13:18:40Z

This seems like a breaking change, can you enable the old behaviour? Does enabling the old behaviour even make sense?

Basically, we are making an opt-in behavior the default one. Opt-in is no longer needed, hence removal of env var.

pehala · 2023-03-16T11:07:04Z

This seems like a breaking change, can you enable the old behaviour? Does enabling the old behaviour even make sense?

Basically, we are making an opt-in behavior the default one. Opt-in is no longer needed, hence removal of env var.

Right, and is there a option to opt-out? Does it even make sense to opt-out or is the new behaviour simply superior?

eguzki · 2023-03-16T11:47:35Z

This seems like a breaking change, can you enable the old behaviour? Does enabling the old behaviour even make sense?

Basically, we are making an opt-in behavior the default one. Opt-in is no longer needed, hence removal of env var.

Right, and is there a option to opt-out? Does it even make sense to opt-out or is the new behaviour simply superior?

For me it does not make sense to opt-out. But if for whatever reason there is a need to opt-out from the current default behavior and get all services and ask for proxy config for each service, there are ways.

Use APICAST_SERVICE_${SERVICE_ID}_CONFIGURATION_VERSION env var with a not existing ${SERVICE_ID}
~~Use APICAST_SERVICES_FILTER_BY_URL with a filter that always match.~~

pehala

I do not have enough knowledge to do a full-on technical review. However, the proposed changes make sense to me.

samugi requested a review from a team as a code owner June 6, 2022 15:55

samugi requested review from a team and removed request for a team June 6, 2022 15:57

samugi changed the title ~~THREESCALE-6448 | THREESCALE-8373 | THREESCALE-7945 - account/proxy_configs endpoint for configuration loading~~ [WIP] THREESCALE-6448 | THREESCALE-8373 | THREESCALE-7945 - account/proxy_configs endpoint for configuration loading Jun 16, 2022

samugi force-pushed the new-account-proxyconfigs-endpoint branch 3 times, most recently from a76258a to bf8b6e8 Compare June 23, 2022 20:23

samugi changed the title ~~[WIP] THREESCALE-6448 | THREESCALE-8373 | THREESCALE-7945 - account/proxy_configs endpoint for configuration loading~~ THREESCALE-6448 | THREESCALE-8373 | THREESCALE-7945 - account/proxy_configs endpoint for configuration loading Jun 24, 2022

eguzki reviewed Jun 27, 2022

View reviewed changes

samugi changed the title ~~THREESCALE-6448 | THREESCALE-8373 | THREESCALE-7945 - account/proxy_configs endpoint for configuration loading~~ [WIP] THREESCALE-6448 | THREESCALE-8373 | THREESCALE-7945 - account/proxy_configs endpoint for configuration loading Jun 27, 2022

samugi force-pushed the new-account-proxyconfigs-endpoint branch from 9add291 to 851a1d6 Compare June 27, 2022 20:47

samugi requested a review from kevprice83 June 27, 2022 20:49

samugi changed the title ~~[WIP] THREESCALE-6448 | THREESCALE-8373 | THREESCALE-7945 - account/proxy_configs endpoint for configuration loading~~ [WIP] THREESCALE-8508 - account/proxy_configs endpoint for configuration loading Jun 28, 2022

samugi changed the title ~~[WIP] THREESCALE-8508 - account/proxy_configs endpoint for configuration loading~~ [WIP] THREESCALE-8508 - /admin/api/account/proxy_configs endpoint for configuration loading Jun 28, 2022

samugi changed the title ~~[WIP] THREESCALE-8508 - /admin/api/account/proxy_configs endpoint for configuration loading~~ THREESCALE-8508 - /admin/api/account/proxy_configs endpoint for configuration loading Jun 28, 2022

samugi force-pushed the new-account-proxyconfigs-endpoint branch 3 times, most recently from 45d16f1 to eb006ce Compare June 28, 2022 17:38

kevprice83 requested changes Nov 22, 2022

View reviewed changes

eguzki force-pushed the new-account-proxyconfigs-endpoint branch 2 times, most recently from 33fab9f to 7edeb7d Compare February 21, 2023 14:40

samugi added 2 commits March 15, 2023 10:29

added new endpoint

8cd13aa

removed deprecated code

a6a1608

samugi and others added 4 commits March 15, 2023 10:29

fixed service version configuration via environment variable

a213122

remote_v2 refactor

af18e7f

refactor tests

fd15331

update doc

a09c671

eguzki force-pushed the new-account-proxyconfigs-endpoint branch from 7edeb7d to 2002f9f Compare March 15, 2023 09:30

eguzki requested a review from a team as a code owner March 15, 2023 09:30

update changelog

7c3c460

eguzki force-pushed the new-account-proxyconfigs-endpoint branch from 2002f9f to 7c3c460 Compare March 15, 2023 09:43

eguzki requested review from pehala and thomasmaas March 15, 2023 09:48

dfennessy approved these changes Mar 15, 2023

View reviewed changes

pehala reviewed Mar 15, 2023

View reviewed changes

eguzki added 2 commits March 16, 2023 11:43

fix t/configuration-loading-with-oidc.t

427312e

Makefile: clean e2e test leftovers

35f8111

pehala approved these changes Mar 16, 2023

View reviewed changes

eguzki merged commit 443b84c into master Mar 16, 2023

eguzki deleted the new-account-proxyconfigs-endpoint branch March 16, 2023 12:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

THREESCALE-8508 - /admin/api/account/proxy_configs endpoint for configuration loading #1352

THREESCALE-8508 - /admin/api/account/proxy_configs endpoint for configuration loading #1352

samugi commented Jun 6, 2022 •

edited by eguzki

Loading

eguzki Jun 27, 2022

samugi Jun 27, 2022 •

edited

Loading

samugi Jun 27, 2022 •

edited

Loading

eguzki Mar 15, 2023

eguzki commented Jun 27, 2022

eguzki commented Jun 27, 2022

samugi commented Jun 27, 2022

samugi commented Jun 27, 2022

secretsurf commented Jul 20, 2022

kevprice83 commented Jul 22, 2022

eguzki commented Sep 20, 2022

kevprice83 left a comment

eguzki commented Mar 15, 2023

eguzki commented Mar 15, 2023

dfennessy left a comment

pehala left a comment

eguzki commented Mar 15, 2023

pehala commented Mar 16, 2023

eguzki commented Mar 16, 2023 •

edited

Loading

pehala left a comment

THREESCALE-8508 - /admin/api/account/proxy_configs endpoint for configuration loading #1352

THREESCALE-8508 - /admin/api/account/proxy_configs endpoint for configuration loading #1352

Conversation

samugi commented Jun 6, 2022 • edited by eguzki Loading

eguzki Jun 27, 2022

Choose a reason for hiding this comment

samugi Jun 27, 2022 • edited Loading

Choose a reason for hiding this comment

samugi Jun 27, 2022 • edited Loading

Choose a reason for hiding this comment

eguzki Mar 15, 2023

Choose a reason for hiding this comment

eguzki commented Jun 27, 2022

eguzki commented Jun 27, 2022

samugi commented Jun 27, 2022

samugi commented Jun 27, 2022

secretsurf commented Jul 20, 2022

kevprice83 commented Jul 22, 2022

eguzki commented Sep 20, 2022

kevprice83 left a comment

Choose a reason for hiding this comment

eguzki commented Mar 15, 2023

eguzki commented Mar 15, 2023

dfennessy left a comment

Choose a reason for hiding this comment

pehala left a comment

Choose a reason for hiding this comment

eguzki commented Mar 15, 2023

pehala commented Mar 16, 2023

eguzki commented Mar 16, 2023 • edited Loading

pehala left a comment

Choose a reason for hiding this comment

samugi commented Jun 6, 2022 •

edited by eguzki

Loading

samugi Jun 27, 2022 •

edited

Loading

samugi Jun 27, 2022 •

edited

Loading

eguzki commented Mar 16, 2023 •

edited

Loading