Creation of service-keys #842

joergdw · 2025-11-01T15:14:27Z

This contains the "app-autoscaler"-changes for cloudfoundry/app-autoscaler-release#3652.

Current top-commit-id of that branch: "9ca68503e2f08bd8123df39b5722decb0fcde1bb"

This PR comes with the following changes:

Service-key-support
- Introduce the necessary handling into the Bind-function of the broker.
- Unit-tests
- Divide the schema into three logical units for three different scenarios, where the "meta.schema.json" brings all of them together:
  - Creation of a service-key using the default-policy while enforcing the usage of the field schema-version (only allowed value: "0.9").
  - Creation of a service-key with explicitly set policy while enforcing the usage of the field schema-version.
  - All the other use-cases that have been supported until now allowing to skip the schema-version. This is for backwards-compatibility.
Multi-schema support for binding-requests:
- Separate logic between request-handling and request-parsing. Introduce the interface BindRequestParser for request-parsers. This is an architectural improvement to simplify maintenance even for a growing variance and seconds the seperation of the internal representation of scaling-policies from their json-representation.
- Adapt tests to the slighly changed error-messages. This is necessary to support the old schema as well. For acceptance-tests these changes are minimal.
Makefiles:
- Fix usage of shell-styles
- (Re-)Generate fakes and gorouterproxy only if relevant input-files have changed.
- Fix dependencies (e.g. for autoscaler.test) and recipes.
- Use programs provided by devbox instead of go run ….
- Fix target clean.
Efficiency-improvement: Avoid parsing of the default credential-type for the communication with the metricsforwarder on each request.

Current top-commit-id of that branch: "9ca68503e2f08bd8123df39b5722decb0fcde1bb"

…the error-type;

Context: <https://github.com/cloudfoundry/app-autoscaler/actions/runs/19294343932/job/55191830755#step:6:1228>

This commit does adaptions to a slightly modified output due to switching to a multi-version-schema.

joergdw · 2025-11-19T10:41:58Z

Makefile

+aes_terminal_font_yellow := \033[38;2;255;255;0m
+aes_terminal_reset := \033[0m


For unknown reasons \e has stopped working on the shell. Similar adaptions that make these shell-styles work, follow.

joergdw · 2025-11-19T16:25:06Z

acceptance/helpers/apps.go

+	appResponse := cfh.CurlAppWithTimeout(
+		cfg, appName, fmt.Sprintf("/disk/%d/%d", spaceInMB, minutes), 10*time.Second)
+	expectedResponse := fmt.Sprintf("{\"minutes\":%d,\"utilization\":%d}", minutes, spaceInMB)
+	Expect(appResponse).Should(MatchJSON(expectedResponse))


This change has been done to be able to track an error there using a debugger.

joergdw · 2025-11-19T16:37:11Z

api/broker/broker.go

+	defaultCustomMetricsCredentialType := &models.X509Certificate
+	if len(conf.DefaultCustomMetricsCredentialType) > 0 {
+		var err error
+		defaultCustomMetricsCredentialType, err = models.ParseCustomMetricsBindingAuthScheme(
+			conf.DefaultCustomMetricsCredentialType)
+		if err != nil {
+			logger.Fatal("parse-default-credential-type", err, lager.Data{
+				"default-credential-type": conf.DefaultCustomMetricsCredentialType,
+			})
+		}
+	}


Its best to set this default at this level. Before we validated the configuration's default-value on each request where we needed it.

joergdw · 2025-11-19T16:51:52Z

api/policyvalidator/policy_validator.go

+var _ error = &ValidationErrors{}

-func (v ValidationErrors) Error() string {
+func (v *ValidationErrors) Error() string {
 	var errs []string
-	for _, failure := range v {
+	for _, failure := range *v {


This has been done to support the error-handling in "broker.go:550-573" with harmonised error-types (i.e. they are now all pointers).

joergdw · 2025-11-19T16:52:25Z

api/policyvalidator/policy_validator.go

 // 🚧 To-do: When making the type `models.PolicyDefinition` safe, then this validation should go
 // into a functional constructor.
 func (pv *PolicyValidator) validateAttributes(policy *models.PolicyDefinition, result *gojsonschema.Result) {
+	if policy == nil {


Needed for a simple service-key-creation using the default-policy.

joergdw · 2025-11-19T16:54:10Z

api/policyvalidator/policy_validator_test.go

Due to the more diverse json-schema for "bind"-requests, the validation against the schema can now return more errors. We need to adapt in the sense that we want to see if the expected one is in a list of potentially several ones that may occur.

joergdw · 2025-11-19T16:56:34Z

models/policy.go

+	if noPolicyData := len(data) <= 0 ||
+		// In case we have policy-data we can rely on the presence of "instance_min_count" due to
+		// the json-schema.
+		!strings.Contains(string(data), `instance_min_count`); noPolicyData {


Again to capture the case where one just wants to create a service-key using the default-policy.

joergdw · 2025-11-19T16:57:12Z

.../java/org/cloudfoundry/autoscaler/scheduler/conf/CloudFoundryConfigurationProcessorTest.java

Unrelated but to satisfy our Java-linter.

joergdw · 2025-11-20T08:25:03Z

All tests are green at the moment.

geigerj0

this is just an intermediate review

geigerj0 · 2025-11-21T14:42:18Z

api/broker/broker.go

-	if appGUID == "" {
-		err := errors.New("error: service must be bound to an application - service key creation is not supported")
-		logger.Error("check-required-app-guid", err)
+	appScalingConfig, err := b.bindingReqParser.Parse(details)


which mechanism ensures that a user does not create a service-key with an app-guid from a completely different org/space?

the app-guid, passed in the scaling policy, must always exist on CF already, right? this is kind of a pre-requisite to check, if the app belongs to the users org.

if this is not the case, we will never be able to solve the issue of "does the app belong to the users org"

This check is part of the task but has been forgotten. A fix will follow soon. Good catch! 👍🏻

geigerj0 · 2025-11-21T14:54:02Z

api/broker/broker.go

+		}
+		apiErr := apiresponses.NewFailureResponseBuilder(
+			fmt.Errorf("invalid policy provided: %s", resultsJson),
+			http.StatusBadRequest, "failed-to-validate-policy").


if it is no valid JSON -> 400
if it does not stick to schema -> 422

geigerj0 · 2025-11-21T14:58:10Z

api/policyvalidator/meta.schema.json

+  "anyOf": [
+    {"$ref": "./scaling-policy.schema.json"},
+    {"$ref": "./legacy.schema.json"},
+    {"$ref": "./service-key_only.schema.json"}
+  ]


anyOf allows everything, or? shouldn't this be oneOf

https://json-schema.org/understanding-json-schema/reference/combining#anyOf

the following test fails even though it shouldn't

FIt("foo", func() { var bindingParams = []byte(` { "configuration": { "app_guid": "12345678-abcd-1234-5678-123456789abc", "custom_metrics": { "metric_submission_strategy": { "allow_from": "bound_app" } } }, "instance_min_count": 1, "instance_max_count": 5, "scaling_rules": [ { "metric_type": "memoryused", "threshold": 100, "operator": "<", "adjustment": "+1" } ] }`) details = domain.BindDetails{ AppGUID: "", // No deprecated app GUID PlanID: "some_plan-id", ServiceID: "some_service-id", BindResource: nil, // No BindResource for service keys RawParameters: bindingParams, } // Execution _, err := aBroker.Bind(ctx, instanceID, bindingID, details, false) Expect(err).NotTo(BeNil()) })

I thought a bit about this: We require backwards-compatibility. When you define anyOf in combination with the legacy-schema, then it is (trivially) guaranteed that you have backwards compatibility because the legacy-schema is one of the entries in the anyOf-construction. When you use oneOf it is not so trivial. And it has the potential to be wrong: Our legacy-schema is quite open to extension (due to missing "additionalProperties": false all over the file) – and therefore may overlap with newer schemas.

This is indeed a problem. As this test-case shows we currently can not distinguish in some cases between legacy-schema and the newer schema. I think I have found a different solution: I restrict the usage of schema-version in the legacy-schema to the single value null. By that it will be always distinguishable on this single field from other schema-versions.

…in a service-key-request;

…ter; Background: Distinguishability from the newer schemas which all should have that field filled out with sensible values.

…ing-details;

sonarqubecloud · 2025-11-25T08:39:15Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Fetch state from aas-release-branch "service-key-creation"

e9e1f87

Current top-commit-id of that branch: "9ca68503e2f08bd8123df39b5722decb0fcde1bb"

joergdw changed the title ~~Fetch state from aas-release-branch "service-key-creation"~~ Creation of service-keys Nov 1, 2025

joergdw mentioned this pull request Nov 1, 2025

Creation of service-keys cloudfoundry/app-autoscaler-release#3652

Draft

joergdw added the enhancement label Nov 1, 2025

joergdw added 15 commits November 3, 2025 10:10

Adapt gitignore and fix linting-message regarding java-indentation;

e7390d6

Merge remote-tracking branch 'os-origin/main' into service-key-creation

7292d3f

🔧 Fix make clean;

d9b73f8

🔧 Fix certificate-location in tests;

7051f69

🔧 Fix ginkgo-argument-order and use devbox-gingko!

593a083

🔧 Fix more test-cert-paths;

7b069f5

🔧 Fix output-style;

4961121

🔧 Fix error-matching-situation due to an erroneous implementation of …

10429c2

…the error-type;

Merge remote-tracking branch 'os-origin/main' into service-key-creation

c7d46b9

Make linter happy;

a90a8f7

Generate fakes only if not already present!

7ffc3bf

🔧 Fix tests;

1aecd22

🚧 Work in progress: Debug disk-usage-scenario;

08e7749

Context: <https://github.com/cloudfoundry/app-autoscaler/actions/runs/19294343932/job/55191830755#step:6:1228>

Add test-case for service-key-creation;

4c8fabf

Build gorouter-proxy if and only if necessary to speedup tests;

d02e286

joergdw force-pushed the service-key-creation branch from 3e0f6c3 to d02e286 Compare November 17, 2025 15:11

joergdw added 4 commits November 17, 2025 16:24

Preparation for multiple schema-variants;

7319fe2

Require schema-version when creating a service-key;

f6b8e28

Expect a schema-version in pure service-key-requests;

1cef624

Fix test-case;

d968d6b

joergdw marked this pull request as ready for review November 19, 2025 08:35

joergdw added 3 commits November 19, 2025 10:22

🔧 Fix acceptance-tests

98173de

This commit does adaptions to a slightly modified output due to switching to a multi-version-schema.

Merge remote-tracking branch 'os-origin/main' into service-key-creation

aceb65e

🔧 Fix acceptance-tests;

829e565

joergdw commented Nov 19, 2025

View reviewed changes

joergdw added 2 commits November 21, 2025 15:51

Merge remote-tracking branch 'os-origin/main' into service-key-creation

6bb6716

🧹 Clean-up;

431fbf2

geigerj0 requested changes Nov 21, 2025

View reviewed changes

joergdw added 4 commits November 21, 2025 18:06

Introduce test for misuse of the quite open legacy-schema to smuggle …

7b5af98

…in a service-key-request;

Enforce that old schema does not know the app_guid;

631a19e

Enforce legacy-schema to not offer a sensible ~schema-version~-Parame…

3e4c0d5

…ter; Background: Distinguishability from the newer schemas which all should have that field filled out with sensible values.

🔧 Fix test;

e2b2214

joergdw force-pushed the service-key-creation branch from 0f30adc to e2b2214 Compare November 21, 2025 17:36

joergdw added 5 commits November 24, 2025 14:48

Re-ordering of test-cases;

e035f2f

🔧 Fix error in changed json-schema.

d261c3f

🛠️ Preparation for the client-communication within the broker;

ee60444

More concurrency-robustness for two tests;

74c34aa

Add test-case for app-space-miss-alignment;

cb6674d

joergdw force-pushed the service-key-creation branch from e1dd497 to cb6674d Compare November 24, 2025 16:11

joergdw added 2 commits November 24, 2025 21:33

🔧 Fix new test for ensuring that the app is in the space.

9cf8835

Introduce handling in case the cloud-controller does not provide bind…

e46b8dc

…ing-details;

		aes_terminal_font_yellow := \033[38;2;255;255;0m
		aes_terminal_reset := \033[0m

Creation of service-keys #842

Are you sure you want to change the base?

Creation of service-keys #842

Conversation

joergdw commented Nov 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joergdw commented Nov 20, 2025

Uh oh!

geigerj0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joergdw Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Nov 25, 2025

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

joergdw commented Nov 1, 2025 •

edited

Loading

joergdw Nov 21, 2025 •

edited

Loading