restore default port 8500 for CONSUL_HTTP_ADDR (#394) #395

larrytamnjong · 2024-11-12T19:06:18Z

Restore default port 8500 for CONSUL_HTTP_ADDR when no port is specif…ied (#394)

larrytamnjong · 2024-11-12T19:20:40Z

@marcin-krystianc I made a tiny adjustment to default the port to 8500 if it isn’t specified in CONSUL_HTTP_ADDR. Please let me know if this makes sense.

marcin-krystianc

Hi @larrytamnjong,

implementation seems legit, please add tests for following cases:

CONSUL_HTTP_ADDR=127.0.0.1 => protocol: http, host: 127.0.0.1, port: 8500
CONSUL_HTTP_ADDR=http://127.0.0.1/ => protocol: http, host: 127.0.0.1, port: 8500 
CONSUL_HTTP_ADDR=http://127.0.0.1:8500/ => protocol: http, host: 127.0.0.1, port: 8500
CONSUL_HTTP_ADDR=http://127.0.0.1:80/ => protocol: http, host: 127.0.0.1, port: 80

Please also verify the test results are the same for your implementation and the version before the ef310c2.

larrytamnjong · 2024-11-13T17:05:03Z

Hi @marcin-krystianc
I create a test method and for my implementation

 public void ConsulClient_BaseUri_Parses_ConsulHttpAddr_Correctly()
 {
     Environment.SetEnvironmentVariable("CONSUL_HTTP_ADDR", "127.0.0.1");
     var client1 = new ConsulClient();
     var config1 = client1.Config;


     Environment.SetEnvironmentVariable("CONSUL_HTTP_ADDR", "http://127.0.0.1/");
     var client2 = new ConsulClient();
     var config2 = client2.Config;


     Environment.SetEnvironmentVariable("CONSUL_HTTP_ADDR", "http://127.0.0.1:8500/");
     var client3 = new ConsulClient();
     var config3 = client3.Config;


     Environment.SetEnvironmentVariable("CONSUL_HTTP_ADDR", "http://127.0.0.1:80/");
     var client4 = new ConsulClient();
     var config4 = client4.Config;

     Assert.Equal("127.0.0.1", config1.Address.Host);  //Passes
     Assert.Equal("127.0.0.1", config2.Address.Host);  //Passes
     Assert.Equal("127.0.0.1", config4.Address.Host);  //Passes
     Assert.Equal("127.0.0.1", config3.Address.Host); //Passes
     Assert.Equal(8500, config1.Address.Port);  //Passes
     Assert.Equal(8500, config2.Address.Port); //Passes
     Assert.Equal(8500, config3.Address.Port);  //Passes
     Assert.Equal(80, config4.Address.Port);  //It fails here because the port returns 8500
 }

However, when I run this test on commit f6a8038, I encounter exceptions with certain values of CONSUL_HTTP_ADDR:

 public void ConsulClient_BaseUri_Parses_ConsulHttpAddr_Correctly()
 {
     Environment.SetEnvironmentVariable("CONSUL_HTTP_ADDR", "127.0.0.1");
     var client1 = new ConsulClient();
     var config1 = client1.Config;


     Environment.SetEnvironmentVariable("CONSUL_HTTP_ADDR", "http://127.0.0.1/");  //An exception is thrown here "Failed parsing port from environment variable CONSUL_HTTP_ADDR"
     var client2 = new ConsulClient();
     var config2 = client2.Config;


     Environment.SetEnvironmentVariable("CONSUL_HTTP_ADDR", "http://127.0.0.1:8500/"); //An exception is thrown here "Failed parsing port from environment variable CONSUL_HTTP_ADDR"
     var client3 = new ConsulClient();
     var config3 = client3.Config;


     Environment.SetEnvironmentVariable("CONSUL_HTTP_ADDR", "http://127.0.0.1:80/"); //An exception is thrown here "Failed parsing port from environment variable CONSUL_HTTP_ADDR"
     var client4 = new ConsulClient();
     var config4 = client4.Config;

     Assert.Equal("127.0.0.1", config1.Address.Host);  
     Assert.Equal("127.0.0.1", config2.Address.Host);  
     Assert.Equal("127.0.0.1", config4.Address.Host);  
     Assert.Equal("127.0.0.1", config3.Address.Host); 
     Assert.Equal(8500, config1.Address.Port);  
     Assert.Equal(8500, config2.Address.Port); 
     Assert.Equal(8500, config3.Address.Port);  
     Assert.Equal(80, config4.Address.Port);  
 }

I'm not sure if this is the expected way to test it. Could you provide some guidance on how to proceed?

orange-puff · 2024-11-13T22:00:22Z

This seems to not be quite right. Uri.IsDefaultPort will return true if the specified port matches the scheme. So,
new Uri("http://mysite.com").IsDefaultPort and new Uri("http://mysite.com:80").IsDefaultPort will both resolve to true.
It seems dumb, but what if the consumer has their consul agent listening on port 80? This logic would overwrite their port with 8500

For determining if the original string contains a port, I'd suggest either

envAddr.Contains($"{uri.Host}:{uri.Port}");

or
Regex.IsMatch(envAddr, ":[0-9]+");

orange-puff · 2024-11-13T22:18:26Z

Unit tests before ef310c2 never included scheme, which was not supported properly previously.

It looks like f6a8038 breaks if the client specifies a scheme http or https, or any other for that matter. There is an environment variable driven way of determining scheme

It will split the address into 3 parts for http://127.0.0.1:80
which would be ["http", "//127.0.0.1", "80"]
which means

consulAddress.Port = ushort.Parse(addrParts[1]);
will try to parse "//127.0.0.1" as an int causing this to be thrown
throw new ConsulConfigurationException("Failed parsing port from environment variable CONSUL_HTTP_ADDR", ex);

if (!string.IsNullOrEmpty(envAddr))
            {
                var addrParts = envAddr.Split(':');
                for (int i = 0; i < addrParts.Length; i++)
                {
                    addrParts[i] = addrParts[i].Trim();
                }
                if (!string.IsNullOrEmpty(addrParts[0]))
                {
                    consulAddress.Host = addrParts[0];
                }
                if (addrParts.Length > 1 && !string.IsNullOrEmpty(addrParts[1]))
                {
                    try
                    {
                        consulAddress.Port = ushort.Parse(addrParts[1]);
                    }
                    catch (Exception ex)
                    {
                        throw new ConsulConfigurationException("Failed parsing port from environment variable CONSUL_HTTP_ADDR", ex);
                    }
                }
            }

…rsing (G-Research#394)

marcin-krystianc · 2024-11-14T09:01:24Z

Hi @marcin-krystianc I create a test method and for my implementation

 public void ConsulClient_BaseUri_Parses_ConsulHttpAddr_Correctly()
 {
     Environment.SetEnvironmentVariable("CONSUL_HTTP_ADDR", "127.0.0.1");
     var client1 = new ConsulClient();
     var config1 = client1.Config;


     Environment.SetEnvironmentVariable("CONSUL_HTTP_ADDR", "http://127.0.0.1/");
     var client2 = new ConsulClient();
     var config2 = client2.Config;


     Environment.SetEnvironmentVariable("CONSUL_HTTP_ADDR", "http://127.0.0.1:8500/");
     var client3 = new ConsulClient();
     var config3 = client3.Config;


     Environment.SetEnvironmentVariable("CONSUL_HTTP_ADDR", "http://127.0.0.1:80/");
     var client4 = new ConsulClient();
     var config4 = client4.Config;

     Assert.Equal("127.0.0.1", config1.Address.Host);  //Passes
     Assert.Equal("127.0.0.1", config2.Address.Host);  //Passes
     Assert.Equal("127.0.0.1", config4.Address.Host);  //Passes
     Assert.Equal("127.0.0.1", config3.Address.Host); //Passes
     Assert.Equal(8500, config1.Address.Port);  //Passes
     Assert.Equal(8500, config2.Address.Port); //Passes
     Assert.Equal(8500, config3.Address.Port);  //Passes
     Assert.Equal(80, config4.Address.Port);  //It fails here because the port returns 8500
 }

However, when I run this test on commit f6a8038, I encounter exceptions with certain values of CONSUL_HTTP_ADDR:

 public void ConsulClient_BaseUri_Parses_ConsulHttpAddr_Correctly()
 {
     Environment.SetEnvironmentVariable("CONSUL_HTTP_ADDR", "127.0.0.1");
     var client1 = new ConsulClient();
     var config1 = client1.Config;


     Environment.SetEnvironmentVariable("CONSUL_HTTP_ADDR", "http://127.0.0.1/");  //An exception is thrown here "Failed parsing port from environment variable CONSUL_HTTP_ADDR"
     var client2 = new ConsulClient();
     var config2 = client2.Config;


     Environment.SetEnvironmentVariable("CONSUL_HTTP_ADDR", "http://127.0.0.1:8500/"); //An exception is thrown here "Failed parsing port from environment variable CONSUL_HTTP_ADDR"
     var client3 = new ConsulClient();
     var config3 = client3.Config;


     Environment.SetEnvironmentVariable("CONSUL_HTTP_ADDR", "http://127.0.0.1:80/"); //An exception is thrown here "Failed parsing port from environment variable CONSUL_HTTP_ADDR"
     var client4 = new ConsulClient();
     var config4 = client4.Config;

     Assert.Equal("127.0.0.1", config1.Address.Host);  
     Assert.Equal("127.0.0.1", config2.Address.Host);  
     Assert.Equal("127.0.0.1", config4.Address.Host);  
     Assert.Equal("127.0.0.1", config3.Address.Host); 
     Assert.Equal(8500, config1.Address.Port);  
     Assert.Equal(8500, config2.Address.Port); 
     Assert.Equal(8500, config3.Address.Port);  
     Assert.Equal(80, config4.Address.Port);  
 }

I'm not sure if this is the expected way to test it. Could you provide some guidance on how to proceed?

@larrytamnjong , we have already a parametrised test Client_DefaultConfig_env, just add more cases to that test instead of implementing a new test method.
As per @orange-puff comment, the old version didn't support parsing of all possible combination of scheme, address and port so in such cases we do not have to worry about backwards compatibility. What we care about is that if an CONSUL_HTTP_ADDR value was valid in the old version it should be also valid in the new version and it should produce the same consul URL.

larrytamnjong · 2024-11-14T09:03:14Z

Hi @marcin-krystianc,

I've added tests for the scenarios you mentioned and adjusted the check for the default port. The tests pass in both my implementation and commit f6a8038. The main difference is that in previous versions, we can't specify an HTTP scheme when adding the address, as noted by @orange-puff.

I believe it’s ready now. Let me know your thoughts!

larrytamnjong · 2024-11-14T14:13:57Z

Hi @marcin-krystianc I create a test method and for my implementation

 public void ConsulClient_BaseUri_Parses_ConsulHttpAddr_Correctly()
 {
     Environment.SetEnvironmentVariable("CONSUL_HTTP_ADDR", "127.0.0.1");
     var client1 = new ConsulClient();
     var config1 = client1.Config;


     Environment.SetEnvironmentVariable("CONSUL_HTTP_ADDR", "http://127.0.0.1/");
     var client2 = new ConsulClient();
     var config2 = client2.Config;


     Environment.SetEnvironmentVariable("CONSUL_HTTP_ADDR", "http://127.0.0.1:8500/");
     var client3 = new ConsulClient();
     var config3 = client3.Config;


     Environment.SetEnvironmentVariable("CONSUL_HTTP_ADDR", "http://127.0.0.1:80/");
     var client4 = new ConsulClient();
     var config4 = client4.Config;

     Assert.Equal("127.0.0.1", config1.Address.Host);  //Passes
     Assert.Equal("127.0.0.1", config2.Address.Host);  //Passes
     Assert.Equal("127.0.0.1", config4.Address.Host);  //Passes
     Assert.Equal("127.0.0.1", config3.Address.Host); //Passes
     Assert.Equal(8500, config1.Address.Port);  //Passes
     Assert.Equal(8500, config2.Address.Port); //Passes
     Assert.Equal(8500, config3.Address.Port);  //Passes
     Assert.Equal(80, config4.Address.Port);  //It fails here because the port returns 8500
 }

However, when I run this test on commit f6a8038, I encounter exceptions with certain values of CONSUL_HTTP_ADDR:

 public void ConsulClient_BaseUri_Parses_ConsulHttpAddr_Correctly()
 {
     Environment.SetEnvironmentVariable("CONSUL_HTTP_ADDR", "127.0.0.1");
     var client1 = new ConsulClient();
     var config1 = client1.Config;


     Environment.SetEnvironmentVariable("CONSUL_HTTP_ADDR", "http://127.0.0.1/");  //An exception is thrown here "Failed parsing port from environment variable CONSUL_HTTP_ADDR"
     var client2 = new ConsulClient();
     var config2 = client2.Config;


     Environment.SetEnvironmentVariable("CONSUL_HTTP_ADDR", "http://127.0.0.1:8500/"); //An exception is thrown here "Failed parsing port from environment variable CONSUL_HTTP_ADDR"
     var client3 = new ConsulClient();
     var config3 = client3.Config;


     Environment.SetEnvironmentVariable("CONSUL_HTTP_ADDR", "http://127.0.0.1:80/"); //An exception is thrown here "Failed parsing port from environment variable CONSUL_HTTP_ADDR"
     var client4 = new ConsulClient();
     var config4 = client4.Config;

     Assert.Equal("127.0.0.1", config1.Address.Host);  
     Assert.Equal("127.0.0.1", config2.Address.Host);  
     Assert.Equal("127.0.0.1", config4.Address.Host);  
     Assert.Equal("127.0.0.1", config3.Address.Host); 
     Assert.Equal(8500, config1.Address.Port);  
     Assert.Equal(8500, config2.Address.Port); 
     Assert.Equal(8500, config3.Address.Port);  
     Assert.Equal(80, config4.Address.Port);  
 }

I'm not sure if this is the expected way to test it. Could you provide some guidance on how to proceed?

@larrytamnjong , we have already a parametrised test Client_DefaultConfig_env, just add more cases to that test instead of implementing a new test method. As per @orange-puff comment, the old version didn't support parsing of all possible combination of scheme, address and port so in such cases we do not have to worry about backwards compatibility. What we care about is that if an CONSUL_HTTP_ADDR value was valid in the old version it should be also valid in the new version and it should produce the same consul URL.

@marcin-krystianc thanks.. please have a look and let me know if it's okay.

marcin-krystianc · 2024-11-15T09:08:26Z

Consul/Client.cs

+                else
+                {
+                    consulAddress.Port = 8500;
+                }


Assigning the default 8500 here is redundant as the consulAddress variable already has the default port pre-assigned. You can remove the else block entirely.

@marcin-krystianc thanks. Done ✅

marcin-krystianc · 2024-11-15T09:10:30Z

Consul/Client.cs

@@ -174,8 +174,14 @@ private void ConfigureFromEnvironment(UriBuilder consulAddress)
                {
                    consulAddress.Host = uri.Host;
                }
-
-                consulAddress.Port = uri.Port;
+                if (envAddr.Contains($"{uri.Host}:{uri.Port}"))


👍 I'm not a fan of this, but it does the job and I don't see how we can do it differently.

restore default port 8500 for CONSUL_HTTP_ADDR when no port is specif…

8cf99e1

…ied (G-Research#394)

marcin-krystianc reviewed Nov 13, 2024

View reviewed changes

update check for default port 80 and add test for CONSUL_HTTP_ADDR pa…

b90470e

…rsing (G-Research#394)

larrytamnjong added 4 commits November 14, 2024 14:24

add more cases to Client_DefaultConfig_env (G-Research#394)

98e6456

update test

0b4bfed

Update test (G-Research#394)

3fc7620

Update HTTP scheme to HTTPS

00c7f4f

marcin-krystianc reviewed Nov 15, 2024

View reviewed changes

remove redundant condition

b1d654d

marcin-krystianc approved these changes Nov 15, 2024

View reviewed changes

marcin-krystianc merged commit 39a598d into G-Research:master Nov 15, 2024
242 of 243 checks passed

larrytamnjong deleted the bugfix/consul-http-addr-default-port-8500 branch November 15, 2024 12:53

marcin-krystianc mentioned this pull request Nov 26, 2024

[Bug]: Loading consul configuration from environment variables fails in 1.7.14.4 #359

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

restore default port 8500 for CONSUL_HTTP_ADDR (#394) #395

restore default port 8500 for CONSUL_HTTP_ADDR (#394) #395

larrytamnjong commented Nov 12, 2024

larrytamnjong commented Nov 12, 2024

marcin-krystianc left a comment

larrytamnjong commented Nov 13, 2024

orange-puff commented Nov 13, 2024

orange-puff commented Nov 13, 2024 •

edited

Loading

marcin-krystianc commented Nov 14, 2024

larrytamnjong commented Nov 14, 2024

larrytamnjong commented Nov 14, 2024

marcin-krystianc Nov 15, 2024

larrytamnjong Nov 15, 2024

marcin-krystianc Nov 15, 2024

larrytamnjong Nov 15, 2024

restore default port 8500 for CONSUL_HTTP_ADDR (#394) #395

restore default port 8500 for CONSUL_HTTP_ADDR (#394) #395

Conversation

larrytamnjong commented Nov 12, 2024

larrytamnjong commented Nov 12, 2024

marcin-krystianc left a comment

Choose a reason for hiding this comment

larrytamnjong commented Nov 13, 2024

orange-puff commented Nov 13, 2024

orange-puff commented Nov 13, 2024 • edited Loading

marcin-krystianc commented Nov 14, 2024

larrytamnjong commented Nov 14, 2024

larrytamnjong commented Nov 14, 2024

marcin-krystianc Nov 15, 2024

Choose a reason for hiding this comment

larrytamnjong Nov 15, 2024

Choose a reason for hiding this comment

marcin-krystianc Nov 15, 2024

Choose a reason for hiding this comment

larrytamnjong Nov 15, 2024

Choose a reason for hiding this comment

orange-puff commented Nov 13, 2024 •

edited

Loading