Add backend authentication for targetRefs on vmusers by secret

[mohammadkhavari]

This pull request will add backend basic authentication support on vmuser to add them as header on vmauth configuration.
if this approach is accepted I'll add other authentication formats (like bearer) if needed.
#669

[Haleygo]

Hello, we are trying to unify the auth config with HTTPAuth, can you reuse it on here?

operator/api/v1beta1/additional.go

Lines 211 to 223 in f2b8cf7

	type HTTPAuth struct {
	BasicAuth *BasicAuth `json:"basicAuth,omitempty"`
	OAuth2 *OAuth2 `json:"OAuth2,omitempty"`
	TLSConfig *TLSConfig `json:"tlsConfig,omitempty"`
	*BearerAuth `json:",inline,omitempty"`
	// Headers allow configuring custom http headers
	// Must be in form of semicolon separated header with value
	// e.g.
	// headerName:headerValue
	// vmalert supports it since 1.79.0 version
	// +optional
	Headers []string `json:"headers,omitempty"`
	}

[mohammadkhavari]

Sure.

[mohammadkhavari]

I'll fix this.

[mohammadkhavari]

I faced some issues about using HttpAuth in VMuser definition.
It is clear that HttpAuth should place [inline] into TargetRef as the feature request is mentioning it will be transform to the targetRef headers.
And all properties in VMuser's finally should be transformed to vmauth configuration, here goes the challenges I have in writing the logic of this transformation for each field of HttpAuth:

1. Headers: as it is present already in the targetRef struct it will override the outer Header field, the logic could remain the same . but the description will override by the HttpAuth Headers description that is mentioning wrong format of headers format:

HTTPAuth Headers: semicolon separated header with value e.g. headerName:headerValue
TargetRef Headers: ["header_key: value1,value2"]

2. BasicAuth, bearerToken:
   It can be implemented using headers configurations the only drawback are passwordFiles field, they usage is specified for CRDs that are leading to deploy a pod and there's filesystem so we can refer a file. But here there's another operation.

3. About TLSConfig, Oauth: as I have investigated these cannot be satisfy only through headers or current vmauth configuration we have here for each user.

And the logic behind this transformation in something like vmalert is much simpler and straightforward: vmalert crd implements HTTPAuth in remoteWrite section and as we can see theres options(https://docs.victoriametrics.com/vmalert.html ) like -remoteWrite.oauth2.clientID string in vmalert component configuration and operator is reshaping the input configs to component flags.

my recommendation is to first implement HttpAuth features in vmauth component (as I have created issue for basicAuth) and support them in configuration formats then update operator.

What do you think? @Haleygo @f41gh7

And also what is your opinion on passwordFiles? inline basicAuth on CRDS in something like vmalert.notifier.basicAuth.password will finally transform to datasource.basicAuth.passwordFile=/etc/vmalert/remote_secrets/DATASOURCE_BASICAUTHPASSWORD on vmalert component args to hide them in prior info, I think vmauth needs some configuration like this.

[Haleygo]

I faced some issues about using HttpAuth in VMuser definition.

@mohammadkhavari Thank you for all the work! Sorry I didn't look too deep when I propose that.
Now I think it's ok to just use BasicAuth here like you already did, I might do some follow up when I work on this later.

[mohammadkhavari]

Thank you, Please inform me if any refactoring, code completion, or other modifications, such as adding tests or fixing code, are required.
@Haleygo

[mohammadkhavari]

@Haleygo Are there any updates on the feature?

[Haleygo]

Sorry for the long silence!
Please see comment.

[Haleygo]

I think we don't need to reuse BasicAuth here, the only field missing in vmuser now is Username v1.SecretKeySelector and we can add it directly to VMUserSpec.
Password v1.SecretKeySelector in BasicAuth should already be covered by PasswordRef *v1.SecretKeySelector

operator/api/v1beta1/vmuser_types.go

Lines 26 to 27 in 0628def

	// +optional
	PasswordRef *v1.SecretKeySelector `json:"passwordRef,omitempty"`

And PasswordFile field is not used.

[mohammadkhavari]

There seems to be a bit of confusion regarding the nature of this feature. As far as my usage experience goes, a vmuser can encompass multiple path routes, and each of these paths includes various hosts as backends (though typically, we utilize only one host per targetRef entry). Our objective is to implement basic authentication for these backends. The challenge lies in setting distinct basic authentication credentials for each endpoint, but how we can set basic auth on these endpoints with some field that has appeared on top level field like vmuser's spec itself?

While I've built a custom operator to address our requirements, it has served our needs effectively for quite some time. However, we want to align it with the latest version of VictoriaMetrics and utilize the officially released version to ensure ongoing compatibility.

[Haleygo]

Oh, sorry, you're right!
But I still think we can add Username v1.SecretKeySelector and Password v1.SecretKeySelector directly instead of reuse BasicAuth since PasswordFile is not used.

[mohammadkhavari]

so should we add it under vmuserSpec.TargetRef ?
It will be a little confusing and not straightforward to know they are for basicAuth? and also it does not seem to be a good solution to provide other methods of http authentication as you've mentioned you're working on.

[Haleygo]

It will be a little confusing and not straightforward to know they are for basicAuth?

I think with description, it's ok.
I'm more against introducing invalid field, which looks like a bug.
What do you think @f41gh7

[f41gh7]

I agree, it's better to use username and password directly. Or wrap it into new struct TargetRefAuth.

It must have username and password for basic Authorization.

E.g.

type TargetRefAuth struct{
  Username v1.SecretKeySelector
  Password v1.SecretKeySelector
}

I think, it less confusing.

[mohammadkhavari]

I think It's good enough to proceed.
I'll go ahead and update the code with the suggested interface.
Do you Agree?

[Haleygo]

@mohammadkhavari yes, please go ahead!

[mohammadkhavari]

@Haleygo @Amper
Hi, I've updated the implementation due to the suggested interface. A test case has been added, and api.md has been updated as well. I would appreciate it if you could review my changes.

[f41gh7]

@Haleygo @Amper Hi, I've updated the implementation due to the suggested interface. A test case has been added, and api.md has been updated as well. I would appreciate it if you could review my changes.

Thanks! I'll take a look soon and most probably, it'll a part of upcoming release.

[f41gh7]

LGTM

[f41gh7]

Thanks for contribution!

[mohammadkhavari]

@f41gh7 According to the failed pipeline, Ive noticed that I missed the refactor structure commit, I have fixed implementations and adopt new interfaces by a new separated commit.
so, make lint make test are also passing locally.
mohammadkhavari@a63743a

should I make another pull req?

[f41gh7]

@f41gh7 According to the failed pipeline, Ive noticed that I missed the refactor structure commit, I have fixed implementations and adopt new interfaces by a new separated commit. so, make lint make test are also passing locally. mohammadkhavari@a63743a

should I make another pull req?

No worries, I ll make follow-up commit with fix for it.