helmrepo: add .spec.certSecretRef for specifying TLS auth data

[aryan9600]

Add .spec.certSecretRef for specifying TLS authentication data using the certFile, keyFile and caFile keys. Deprecate the usage of these keys in the secret specified by .spec.secretRef.
Introduce (and refactor) Helm client builder and auth helpers to reduce duplicated code and increase uniformity and testability.

Fixes: #1131
Prerequisite to: #1097

[souleb]

helmchart_controller also has to be modified has it uses the secret from the helmRepository.Spec. here:

source-controller/internal/controller/helmchart_controller.go

Line 507 in d141456

func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *helmv1.HelmChart,

[souleb]

Well done @aryan9600! LGTM.

[darkowlzz]

This seems to be the first usage of just event in any reconciler.
When it's about deprecation warning, shouldn't it be logged as well using eventLogf()?

But this makes me think of https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/1693-warnings/README.md . This KEP provides ideas about surfacing the deprecation warning to users and cluster admins. I think we can learn from it and think about how to handle such things in flux as we'll have more such needs in the future as we change the APIs.

[aryan9600]

i am not logging it because as far as a i understand, it'll be logged every reconciliation, which will just populate the logs with unnecessary deprecation warnings

[stefanprodan]

Wouldn't we'll be filling etcd instead with warning events? IMO DDOS-ing etcd is way way worse than log spam.

[darkowlzz]

We had this discussion before fluxcd/notification-controller#435 (comment) .

[darkowlzz]

Based on the ideas from the aforementioned KEP, I think we can also provide a flag to disable such warning events and logs.
Also, based on the KEP, I was thinking maybe we should provide some metric about which objects use deprecated configuration. As mentioned in https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/1693-warnings/README.md#risks-and-mitigations , keeping the cardinality in control.

[darkowlzz]

I think this wasn't really resolved. Discussion about a flag to silencing warnings and metrics about deprecated API usage wasn't discussed at all.

[aryan9600]

i don't think we can ever suppress warning events, since that's the event type used for all failed operations.
this is the first usage of a warning log (in this controller, unaware about other controllers). i doubt that there'll be more and even this is a temporary thing. so practically, we'd be introducing a flag to silence one log message, which seems overkill? but then again, i don't have any other ideas on how to silence it and prevent it from filling up the controller's logs so maybe its necessary.

[darkowlzz]

A new feature/flag can pave the way for more usage of it in the future. A lot of kubernetes components have this feature, even if it's for one log line. It's a very basic feature to ask.
Suppressing in this case is about just not checking what it is checking. Just don't check for deprecated usage and there won't be any associated log or event.
I believe this is an opportunity to discuss how do we handle such things every time we deprecate certain APIs without breaking things, and how to convey such things more visibly to the users.

I just noticed that the implementation is different from the conclusion that I understood. It's only logging without any event. I pointed out above that emitting events is not a problem. The event recorder client has the capability to rate limit too many events. So, we could just continue using eventLogf() with EventTypeWarning. Also noticed that it's an info log now. I think this was discussed based on image-reflector's similar log to be an error log.

[aryan9600]

i'm not saying i'm against the flag, if we think we do need such a thing, i'm totally for it. i didn't use eventLogf(), because i thought we decided to log it for now, based on discussions offline as i noted here #1160 (comment). i logged it on an info level instead of warning because this is what the upstream kubernetes folks did: go-logr/logr#151 (comment)

[darkowlzz]

I think it's subjective based on the context. I'm aware of the simplification to just have error and info log. It may depend on the authors of a project if they want user's attention on certain things. I don't know what's better, but it'd be good to have consistency. Update this to be error or update image-reflector's log to be info.
In terms of events, since there are only Normal and Warning event types, we use warnings for error events. But if log is normal, should event also be normal? Then how do we highlight that this is something users need to pay attention to. Based on our usage, I'd incline towards consistency with our current usage of considering warnings as errors. But that's just me.

I don't think we ever discussed about removing events as I pointed above that it's not a problem. May be a misunderstanding.