feat: Move http scaled object from single host to multi host system

[jocelynthode]

Initial tests, Moving http scaled object from single host association to multi host.

Recreate #585 as the rebasing moved the host field to triggers which only accepts map[string]string in metadata and thus we could not continue using a list of hosts.

The approach chosen is that when creating the ScaledObject we concatenate the HTTPScaledObject host list to a comma separated string.

e2e-tests have been ran locally as well as testing that both hosts scale the correct container.

Here is an excerpt of the configmap generated:

apiVersion: v1
data:
  routing-table: |
    {"myhost.com":{"Service":"xkcd","Port":8080,"Deployment":"xkcd","Namespace":"app","TargetPendingRequests":200},"myhost2.com":{"Service":"xkcd","Port":8080,"Deployment":"xkcd","Namespace":"app","TargetPendingRequests":200}}
kind: ConfigMap
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"v1","data":{"routing-table":"{}"},"kind":"ConfigMap","metadata":{"annotations":{},"labels":{"app.kubernetes.io/component":"add-on","app.kubernetes.io/instance":"operator","app.kubernetes.io/managed-by":"kustomize","app.kubernetes.io/name":"http","app.kubernetes.io/part-of":"keda","app.kubernetes.io/version":"main"},"name":"keda-http-add-on-routing-table","namespace":"keda"}}
  creationTimestamp: "2023-05-10T12:34:09Z"
  labels:
    app.kubernetes.io/component: add-on
    app.kubernetes.io/instance: operator
    app.kubernetes.io/managed-by: kustomize
    app.kubernetes.io/name: http
    app.kubernetes.io/part-of: keda
    app.kubernetes.io/version: main
  name: keda-http-add-on-routing-table
  namespace: keda
  resourceVersion: "1495"
  uid: be5766b9-6d5c-4355-b382-b8ec0343271d

Checklist

• Commits are signed with Developer Certificate of Origin (DCO)
• Changelog has been updated and is aligned with our changelog requirements
• Any necessary documentation is added, such as:
  • README.md
  • The docs/ directory
  • The docs repo

cc @someshkoli

Fixes #552

[jocelynthode]

@JorTurFer Hey, as stated in #585 here is the PR continuing the work on multi hosts after the rebase :)

[JorTurFer]

Looking good but I have one question about the expected behavior.
This approach will create a ScaledObject with one external trigger, but in the HPA side we will have multiple metrics (one per host), so we won't scale based on the sum of all the hosts, we will scale based on max(hosts).
If we want to scale based on the sum of all of them, we should return just a single metric spec in GetMetricSpec and sum all the host values in a single metric result from GetMetrics (in scaler/handlers.go).
IMO, scaling based on the sum of all of them makes sense but I don't have any strong opinion about this, maybe we can choose one option and iterate over it in the future

[jocelynthode]

Looking good but I have one question about the expected behavior. This approach will create a ScaledObject with one external trigger, but in the HPA side we will have multiple metrics (one per host), so we won't scale based on the sum of all the hosts, we will scale based on max(hosts). If we want to scale based on the sum of all of them, we should return just a single metric spec in GetMetricSpec and sum all the host values in a single metric result from GetMetrics (in scaler/handlers.go). IMO, scaling based on the sum of all of them makes sense but I don't have any strong opinion about this, maybe we can choose one option and iterate over it in the future

@JorTurFer Good idea! I agree with you I think it makes more sense to scale based on the sum of all requests. I've updated my PR to integrate this change and added a unit test for this scenario.

I've added this change in a separate commit to make it easier to review this new change on your end but feel free to squash it all together when merging.

I've also ran make test and make e2e-test locally using kind and it worked on my end :)

[jocelynthode]

Hopefully this one is the one :)

[JorTurFer]

The last nits about the code to avoid some variables 😄

Apart from them, could you update the changelog to add these changes? (In the breaking change version)

Once these things are done, we can merge the PR 🥳 🥳 🥳

[jocelynthode]

Changelog has been updated :)

[JorTurFer]

LGTM !! 👍 👍 👍
thanks for this awesome improvement

@tomkerkhove , could you take a look?

[t0rr3sp3dr0]

Are we allowing breaking changes without bumping the API Version? This will break every HTTPScaledObject on the cluster when you update the add-on.

I was about to propose a new v1alpha1 schema so we can support this and other features like:

• Scaling any object that implements the scale sub-resource, not just Deployments
• Perform scale-up and scale-down operations on multiple resources at once
• Routing multiple hosts/paths to a single service
• Allow different path types when routing, similar to an Ingress
• Have a ScaledObject template instead of reimplementing fields on our side

[JorTurFer]

I thought that we are fine with the breaking changes until v1.0: #585 (comment)

[t0rr3sp3dr0]

I see that a bit differently. I agree we can remove features, change behaviours, and don't commit to anything before v1.0.0, but I think we should always have an upgrade path available for the current users of the service. Otherwise, we are telling people not to use this in any way before we have a stable version.

People using non-stable software expect that there may be bugs and that everything can change anytime. But if we don't give them an upgrade option, even users comfortable with the nuances of beta software would stop using the add-on other than to play around with it. Fewer people using the add-on also means fewer contributors and less feedback, making it harder to continue development.

For example, Argo Rollouts and Terraform are two softwares we used at Microsoft even before their 1.0.0 versions. There were problems, and things changed abruptly, but we could rely on the fact that there would always be a way to move forward. If there wasn’t, we wouldn't have used them.

[JorTurFer]

I prefer to not introduce breaking changes neither, but based on the comment I understood that it wasn't a problem.

Maybe we can support both (host and hosts):

• If host is set, we can generate the hosts slice and pass to the internal code at controller level (and print a deprecation warning)
• if hosts is set, we will use it
• if both are set, we will raise an error clarifying that both aren't compatible
  WDYT?

[t0rr3sp3dr0]

Yeah, that sounds perfect. I totally agree with that implementation.

[JorTurFer]

Lets address the proposal without the breaking change as @t0rr3sp3dr0 suggested

[jocelynthode]

Hey @JorTurFer and @t0rr3sp3dr0, I've added a commit to instead deprecate the host field and convert it to hosts as well as print a log message for the user to prompt them to switch as you suggested.

Unfortunately as there are currently no differentiation between Info logs that I could see I could not make the deprecation warning stand out more. Let me know if I missed something.

I've added a few unit tests to make sure it's doing what we want and I've tested my feature with the e2e Suite. Here are the results:

Specifying both hosts and host

It errors out as expected and the routing table stays empty.

2023-05-22T11:33:37Z	ERROR	Reconciler error	{"controller": "httpscaledobject", "controllerGroup": "http.keda.sh", "controllerKind": "HTTPScaledObject", "HTTPScaledObject": {"name":"xkcd","namespace":"app"}, "namespace": "app", "name": "xkcd", "reconcileID": "0c0781a6-5bb4-42ba-9120-aba3b57fcc69", "error": "Mutually exclusive fields Error"}
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler
	/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.14.5/pkg/internal/controller/controller.go:329
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem
	/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.14.5/pkg/internal/controller/controller.go:274
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2
	/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.14.5/pkg/internal/controller/controller.go:235

apiVersion: v1
data:
  routing-table: |
    {}
kind: ConfigMap
metadata:
  name: keda-http-add-on-routing-table
  namespace: keda

Specifying only host

The host field is converted and the routing table is updated. A log message is printed warning the user about the deprecation.

2023-05-22T11:40:37Z	INFO	Using the 'host' field is deprecated. Please consider switching to the 'hosts' field	{"controller": "httpscaledobject", "controllerGroup": "http.keda.sh", "controllerKind": "HTTPScaledObject", "HTTPScaledObject": {"name":"xkcd","namespace":"app"}, "namespace": "app", "name": "xkcd", "reconcileID": "05f0437e-144d-44d1-8a1e-bcb25a382809", "httpscaledobject": "app/xkcd"}

apiVersion: v1
data:
  routing-table: |
    {"myhost.com":{"Service":"xkcd","Port":8080,"Deployment":"xkcd","Namespace":"app","TargetPendingRequests":200}}
kind: ConfigMap
metadata:
  name: keda-http-add-on-routing-table
  namespace: keda

Specifying only hosts

Works as before this last change.

Let me know if that's ok :)

[jocelynthode]

Used https://github.com/kedacore/keda/blob/main/CHANGELOG.md as the inspiration to fix the Changelog for the deprecation :)

[JorTurFer]

I think now it better, WDTY @t0rr3sp3dr0 ?

[t0rr3sp3dr0]

Overall, the patch LGTM. I suggested just a tiny change.

[JorTurFer]

Thanks a lot ❤️

[jocelynthode]

@JorTurFer @t0rr3sp3dr0: The static checks are failing due to my If-Else chain in the sanitizeHosts function. However I'm not sure rewriting it as a switch statement would make sense nor make the code more readable.

Could you please advise me on how I could disable this check for this function or if you want me to rewrite it as a switch statement how would you like it to be rewritten?

Feel free to directly commit a fix to the branch if you have one, so that we can finally merge this PR :)

[t0rr3sp3dr0]

The problem is that you have an else block after an if statement that always returns. Remove all the redundant else’s from the code and the check should pass.

You should hardly use else blocks in Go if you follow the patterns of the language. If you are using them, it’s likely that some refactoring is able to get rid of these blocks.

[jocelynthode]

@t0rr3sp3dr0: Thanks for the pointers I think I've fixed the issue :)

[jocelynthode]

@JorTurFer: Could you restart the process again please to see if this time it goes through?

[JorTurFer]

thanks for the contribution! ❤️

[jocelynthode]

@JorTurFer Yay finally! All tests are green. Happy to have contributed :) This is a feature that we need in my company as we're currently trying to rollout a scale to zero on all our dev/staging environments :)

Thank you both for your patience!

[someshkoli]

Happy to see this wrap 🚀 thanks @jocelynthode for helping with rebase and updates