scylladb / alternator-load-balancing

Various tricks, scripts, and libraries, for load balancing multiple Alternator nodes
Apache License 2.0
18 stars 11 forks source link

Client-side LB code region reporting change request acknowledgment #30

Closed maesta2 closed 2 months ago

maesta2 commented 3 months ago

Is it possible to get this changed so client side LB code reports the actual region: https://github.com/scylladb/alternator-load-balancing/blob/master/go/v2/alternator_lb.go#L53?

mykaul commented 3 months ago

Is it possible to get this changed so client side LB code reports the actual region: https://github.com/scylladb/alternator-load-balancing/blob/master/go/v2/alternator_lb.go#L53?

Reports to whom? @maesta2 - what's the bug that you are concerned with?

maesta2 commented 3 months ago

@mykaul if you look at the source of above link, it seems that below function does not return Region info and that is what the user was asking.

func (n *AlternatorNodes) Config(fake_domain string, key string, secret_key string) aws.Config {
    fake_url := fmt.Sprintf("%s://%s:%d", n.scheme, fake_domain, n.port)

    return aws.Config{
        EndpointResolverWithOptions: staticEndpointResolver(fake_url),
        // Region is used in the signature algorithm so prevent request sent
        // to one region to be forward by an attacker to a different region.
        // But Alternator doesn't check it. It can be anything.
        Region: "whatever",
        // The third credential below, the session token, is only used for
        // temporary credentials, and is not supported by Alternator anyway.
        Credentials: credentials.NewStaticCredentialsProvider(key, secret_key, ""),

        APIOptions: []func(*middleware.Stack) error{
            func(m *middleware.Stack) error {
                return m.Finalize.Add(n.loadBalancerMiddleware(fake_domain), middleware.Before)
            },
        },
    }
}
mykaul commented 3 months ago

@mykaul if you look at the source of above link, it seems that below function does not return Region info and that is what the user was asking.

Yes, I've looked at the source code and the comment there. If that's an enterprise customer, please open a separate enterprise issue. It's not clear to me what the request is. The data center name? As you can see, there's not yet a check around it in Alternator. See also https://github.com/scylladb/scylladb/issues/12147

dkropachev commented 3 months ago

@maesta2 , couple cents to add to @mykaul, alternator is runnign on top of scylla, in scylla there is no Region, only Datacenter and Rack. And technically both can represent Cloud Region, you can have nodes of one Rack be running on one AWS/GCP Region, while nodes from other Rack on another, and you can do the same for Datacenter. Beside that there are many other possible configurations, so it is up to deployment and configuration what represents what. On Scylla Cloud, for example, Datacenter represents Cloud Region, even though naming is bit different.

In the code you have provided client creates aws.Config which is used by dynamodb.NewFromConfig to forge requests to Alternator, it does not go anywhere else. Alternator ignores this field, that is why you see random hardcoded string there, now question is, why do you want to change it?

maesta2 commented 3 months ago

@dkropachev the answer to your question:

basically the problem we are having is we have instrumented our AWS SDK with open telemetry (https://pkg.go.dev/go.opentelemetry.io/contrib/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws), and since swapping to use this client side load balancing lib we get the region "whatever" propagated through our traces where we used to get an AWS region name
dkropachev commented 3 months ago

@dkropachev the answer to your question:

basically the problem we are having is we have instrumented our AWS SDK with open telemetry (https://pkg.go.dev/go.opentelemetry.io/contrib/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws), and since swapping to use this client side load balancing lib we get the region "whatever" propagated through our traces where we used to get an AWS region name

Now it makes sense, we can make it configurable to allow you to see whatever you want there, is it good enough?

maesta2 commented 3 months ago

@dkropachev Yes, it would be enough.

dkropachev commented 3 months ago

@dkropachev Yes, it would be enough.

Then following should be enough:

    cfg := alternatorNodes.Config("dog.scylladb.com", "alternator", "secret_pass")
        cfg.Region = "eu-west-1"
maesta2 commented 2 months ago

@dkropachev when will it be merged?

dkropachev commented 2 months ago

@dkropachev when will it be merged?

What I meant is that you can do that in your app to see the region.

maesta2 commented 2 months ago

@dkropachev Would it be nice to port this to the code so that I don't have ask the customer to write their own code?

dkropachev commented 2 months ago

@dkropachev Would it be nice to port this to the code so that I don't have ask the customer to write their own code?

I am pretty sure there is no other way to use code from this repo but write your own code.

maesta2 commented 2 months ago

Thanks. The customer agreed to change it by themselves.