Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In the API types, optional structs should be pointers. #1170

Closed
vaikas opened this issue Sep 17, 2020 · 2 comments · Fixed by #1174
Closed

In the API types, optional structs should be pointers. #1170

vaikas opened this issue Sep 17, 2020 · 2 comments · Fixed by #1174
Labels
bug Something isn't working

Comments

@vaikas
Copy link
Contributor

vaikas commented Sep 17, 2020

Some API fields are optional, but they should then be pointers as per:
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#optional-vs-required

Expected Behavior

There's couple of structs in the APIs that are marked as optional, but they are not pointers. This means that they get filled in as empties and that causes validation errors.
https://github.com/kedacore/keda/blob/v2/api/v1alpha1/triggerauthentication_types.go#L25
https://github.com/kedacore/keda/blob/v2/api/v1alpha1/triggerauthentication_types.go#L34

And that I could then define (simplified example):

		Spec: kedav1alpha1.TriggerAuthenticationSpec{
			SecretTargetRef: []kedav1alpha1.AuthSecretTargetRef{
				{
					Parameter: "host",
					Name:      secretName,
					Key:       secretKey,
				},
			},

Actual Behavior

However, unless I add this here, it will not pass because the webhook will fail it.

		Spec: kedav1alpha1.TriggerAuthenticationSpec{
			SecretTargetRef: []kedav1alpha1.AuthSecretTargetRef{
				{
					Parameter: "host",
					Name:      secretName,
					Key:       secretKey,
				},
			},
			// HACK. Without this doesn't work.
			HashiCorpVault: kedav1alpha1.HashiCorpVault{Secrets: []kedav1alpha1.VaultSecret{}},

Steps to Reproduce the Problem

  1. Call the API with that object, webhook will fail it saying that VaultSecret is null
  2. But, now I'm then left with these kinds of log messages:
2020-09-17T18:50:07.737Z	ERROR	scalehandler	Error authenticate to Vault	{"type": "ScaledObject", "namespace": "default", "name": "ping-trigger", "triggerAuthRef.Name": "default-trigger-auth", "error": "Vault auth method  is not supported"}
github.com/go-logr/zapr.(*zapLogger).Error
	/go/pkg/mod/github.com/go-logr/zapr@v0.1.1/zapr.go:128
github.com/kedacore/keda/pkg/scaling/resolver.ResolveAuthRef
	/workspace/pkg/scaling/resolver/scale_resolvers.go:74
github.com/kedacore/keda/pkg/scaling.(*scaleHandler).buildScalers
	/workspace/pkg/scaling/scale_handler.go:306
github.com/kedacore/keda/pkg/scaling.(*scaleHandler).GetScalers
	/workspace/pkg/scaling/scale_handler.go:67
github.com/kedacore/keda/pkg/scaling.(*scaleHandler).checkScalers
	/workspace/pkg/scaling/scale_handler.go:182
github.com/kedacore/keda/pkg/scaling.(*scaleHandler).startScaleLoop
	/workspace/pkg/scaling/scale_handler.go:134

But at least I can now create the resource :)
3.

Specifications

  • KEDA Version: V2 Head
  • Platform & Version: Please elaborate
  • Kubernetes Version: Please elaborate
  • Scaler(s): Please elaborate
@vaikas vaikas added the bug Something isn't working label Sep 17, 2020
@zroubalik
Copy link
Member

@vaikas good catch, we should convert those structs to the pointers. We have changed the API for v2 anyway, so it won't be a problem.

@vaikas
Copy link
Contributor Author

vaikas commented Sep 18, 2020

/assign
I can do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants