fix(aws): support aws_ca_bundle

[mwmix]

What does it do ?

Fixes an issue when you have the AWS_CA_BUNDLE set and are using the 'aws' provider. The error is as follows:

instantiating AWS config: unable to add custom RootCAs HTTPClient, has no WithTransportOptions, *http.Client

Motivation

This causes me a ton of headache recently and I was forced to use a Debian based container of the tool as opposed to the scratch based one.

More

• [ x ] Yes, this PR title follows Conventional Commits
• [ x ] Yes, I added unit tests
• [ x ] Yes, I updated end user documentation accordingly

[k8s-ci-robot]

Hi @mwmix. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[ivankatliarchuk]

Configuring metrics per provider not a great idea. Please avoid that.

[mwmix]

I agree with you, I've been struggling to find a better implementation. Any insights would be appreciated.

[ivankatliarchuk]

		config.WithHTTPClient(extdnshttp.NewInstrumentedClient(AWSHTTPCLIENT)),

^ Something simliar

What happens if we simply add WithCustomCABundle, my understanding it could work, even if we wrapping http client

[mwmix]

I tried to come up with a way to do what you suggested before I dived down the middleware rabbit hole. But, I kept hitting road blocks. The closest I could get was this and we get a type error:

func NewInstrumentedAWSClient(next *awshttp.BuildableClient) *awshttp.BuildableClient) {
    next.WithTransportOptions(func(transport *http.Transport) {
			transport = NewInstrumentedTransport(next)
		})
}

The type error is

cannot use next (variable of type *"github.com/aws/aws-sdk-go-v2/aws/transport/http".BuildableClient) as "net/http".RoundTripper value in argument to NewInstrumentedTransport: *"github.com/aws/aws-sdk-go-v2/aws/transport/http".BuildableClient does not implement "net/http".RoundTripper (missing method RoundTrip) 

So the WithCustomCABundle would "work" and so does just updating my systems trusted ca list. However, the moment we set AWS_CA_BUNDLE even with the WithCustomCABundle set then we still hit that error. One workaround would be to use WithCustomCABundle pulling the value from AWS_CA_BUNDLE and then unsetting the environment variable so the SDK doesn't use it. But, I imagine that has it's own issues ...

When looking at other projects such as otel and based on my own reading of the documentation it seemed like this was the expected interface for adding this sort of functionality.

I'm not really married to any particular solution and appreciate your feedback. I've been trying my best to minimize the impact of the changes while preserving the existing instrumentation but haven't found any cleaner way around it.

[ivankatliarchuk]

I do not have access to AWS. If you could reproduce it with kind+localstack or similar, might be able to have a look.

Have you tried WithCustomCABundle?

[ivankatliarchuk]

As well, have you tried for example

config.WithHTTPClient(extdnshttp.NewInstrumentedClient(&http.Client{Transport: transport.NewBuildableClient().GetTransport()})),

[mwmix]

I tried WithCustomCABundle() and the error still appears when I try to run with the AWS_CA_BUNDLE defined.

I just also finished testing the last example you gave me above with the same result :(

[ivankatliarchuk]

Gotcha

[ivankatliarchuk]

Just a note about opentelemetry. It's ok to have a middleware support, but still expected to to support RoundTrippers https://github.com/open-telemetry/opentelemetry-go-contrib/blob/caee80916a50f168c7152967dabaefd0c3cd17c0/instrumentation/net/http/otelhttp/transport.go#L26

Basically to be a consistent across multiple libraries. With aws there is no such option, it's quite aws specific

[ivankatliarchuk]

Can we keep everything within the existing package sigs.k8s.io/external-dns/pkg/http, unless there’s a circular dependency that prevents it or other valid reason?

Please note that packages named utils are not being approved at this time.
For reference, see this example where a similar utils package was proposed but not accepted: #5189 (comment).

[mwmix]

Sure, ty for the insight. I'll update the MR appropriately.

[ivankatliarchuk]

Is it possible to reuse this certs or location https://github.com/kubernetes-sigs/external-dns/tree/master/internal/testresources?

[mwmix]

Yeah absolutely, I didn't realize you already had some stashed away in the repo! :)

[ivankatliarchuk]

There should be a simple reason why certificate bundle is not loaded. Hard to say without how-to-reproduce example

[ivankatliarchuk]

As well, have you tried for example

config.WithHTTPClient(extdnshttp.NewInstrumentedClient(&http.Client{Transport: transport.NewBuildableClient().GetTransport()})),

[ivankatliarchuk]

The WHY is not working is described here https://github.com/aws/aws-sdk-go-v2/blob/f9f7a6bb124a1a7daffc65db40053d97678bd371/config/env_config.go#L174-L189. In principal, AWS sdk instead of Transport should have RoundTripper, as go http library does.

Could we add to our instrumenter an option to wrap and return Transport if config.WithHTTPClient(extdnshttp.NewInstrumentedClient(&http.Client{Transport: transport.NewBuildableClient().GetTransport()})), not going to work?

Middleware is an OK way, but AWS library middleware is just to complex.

[ivankatliarchuk]

Technically, if we passing a Transport (cfg.HTTPClient.(*awshttp.BuildableClient)) then resolveCustomCABundle https://github.com/aws/aws-sdk-go-v2/blob/f9f7a6bb124a1a7daffc65db40053d97678bd371/config/resolve.go#L42 should attach TLS certs.

Just need to find out how to enhance it with our metrics.

[ivankatliarchuk]

I'm unsure actually. I do get why AWS team done it that way, but not simple to extend.

/ok-to-test

[ivankatliarchuk]

/retitle fix(aws): support aws_ca_bundle

[ivankatliarchuk]

Ideally this should stay. The plan is to wrap it like here

external-dns/pkg/metrics/metrics.go

Line 38 in 789494f

RegisterMetric.MustRegister(NewGaugeFuncMetric(prometheus.GaugeOpts{

or here

external-dns/controller/controller.go

Line 38 in 789494f

registryErrorsTotal = metrics.NewCounterWithOpts(

as we have documentation for metrics automated https://github.com/kubernetes-sigs/external-dns/blob/master/docs/monitoring/metrics.md

[ivankatliarchuk]

Changing this, may affect kube monitoring as well, which is not desirable.

[mwmix]

I pushed a new commit which refactors the fix to use the common metrics registry. Does that work? Or do we want to split that into a different PR?

[ivankatliarchuk]

If you have time, would be better to slice PRs.

[mwmix]

Okay I've modified this PR so it has what I hope is the minimum set of changes you were hoping for. I'll have another PR out either later tonight / tomorrow which will be branched off this with the other changes I made.

[mwmix]

Opened PR #5677 for the metrics refactoring.

[mloiseleur]

The code LGTM.
Like for the previous change on metrics, the latest refactor asked by @ivankatliarchuk should go in its own dedicated PR.

=> Would you please extract the code in pkg/http & pkg/metrics in a dedicated refactor PR ?

[mwmix]

The code LGTM. Like for the previous change on metrics, the latest refactor asked by @ivankatliarchuk should go in its own dedicated PR.

=> Would you please extract the code in pkg/http & pkg/metrics in a dedicated refactor PR ?

Yep can do, new PR opened #5717.

[ivankatliarchuk]

I wonder how this works without issues. The package pkg/http depends on pkg/metrics. Interesting, I would expect cyclic dependency

[mloiseleur]

/lgtm
@ivankatliarchuk I'll let you proceed with the final review.

[ivankatliarchuk]

It seems like no issues. so lgtm as well

/approve

[ivankatliarchuk]

Fixes #5666

[ivankatliarchuk]

Looks like on 1CPU machine it will not work, want compile or compile with some flakyness.

[ivankatliarchuk]

/remove-approve

[mwmix]

Ah I see the issue this isn't using the refactored stuff. I've re-based everything and pushed updates. I can build locally and run.

[ivankatliarchuk]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ivankatliarchuk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [ivankatliarchuk]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment