Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate ArmeriaSettings.ip and add ArmeriaSettings.address #4598

Merged
merged 10 commits into from
Jan 12, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@ armeria:
ports:
- port: 0
protocol: HTTP
- ip: 127.0.0.1
- address: 127.0.0.1
port: 0
protocol: HTTP
- ip: 0.0.0.0
- address: 0.0.0.0
port: 0
protocol: HTTP

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@ armeria:
ports:
- port: 0
protocol: HTTP
- ip: 127.0.0.1
- address: 127.0.0.1
port: 0
protocol: HTTP
- ip: 0.0.0.0
- address: 0.0.0.0
port: 0
protocol: HTTP
metrics-path: ''
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,12 @@ public static void configurePorts(ServerBuilder server, List<Port> ports) {
requireNonNull(server, "server");
requireNonNull(ports, "ports");
ports.forEach(p -> {
final String ip = p.getIp();
final String iface = p.getIface();
final int port = p.getPort();
final List<SessionProtocol> protocols = firstNonNull(p.getProtocols(),
ImmutableList.of(SessionProtocol.HTTP));

if (ip == null) {
final InetSocketAddress socketAddress = createSocketAddress(p);
if (socketAddress == null) {
if (iface == null) {
server.port(new ServerPort(port, protocols));
} else {
Expand All @@ -71,24 +70,42 @@ public static void configurePorts(ServerBuilder server, List<Port> ports) {
}
}
} else if (iface == null) {
if (NetUtil.isValidIpV4Address(ip) || NetUtil.isValidIpV6Address(ip)) {
final byte[] bytes = NetUtil.createByteArrayFromIpAddressString(ip);
try {
server.port(new ServerPort(new InetSocketAddress(
InetAddress.getByAddress(bytes), port), protocols));
} catch (UnknownHostException e) {
// Should never happen.
throw new Error(e);
}
} else {
throw new IllegalStateException("invalid IP address: " + ip);
}
server.port(new ServerPort(socketAddress, protocols));
} else {
throw new IllegalStateException("A port cannot have both IP and iface: " + p);
}
});
}

@Nullable
private static InetSocketAddress createSocketAddress(Port p) {
final InetAddress address = p.getAddress();
final String ip = p.getIp();
final int port = p.getPort();
if (ip != null && address != null) {
throw new IllegalStateException("A port cannot have both IP and address: " + p);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we override Port.toString() to properly create the message?

}
final InetSocketAddress targetAddress;
if (ip != null) {
if (NetUtil.isValidIpV4Address(ip) || NetUtil.isValidIpV6Address(ip)) {
final byte[] bytes = NetUtil.createByteArrayFromIpAddressString(ip);
try {
targetAddress = new InetSocketAddress(InetAddress.getByAddress(bytes), port);
} catch (UnknownHostException e) {
// Should never happen.
throw new Error(e);
}
} else {
throw new IllegalStateException("invalid IP address: " + ip);
}
} else if (address != null) {
targetAddress = new InetSocketAddress(address, port);
} else {
targetAddress = null;
}
return targetAddress;
}

/**
* Returns a newly created {@link Port}.
* {@code null} if the specified {@code code} is either {@code null} or a negative number.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package com.linecorp.armeria.spring;

import java.net.InetAddress;
import java.time.Duration;
import java.util.ArrayList;
import java.util.List;
Expand Down Expand Up @@ -48,7 +49,7 @@
* ports:
* - port: 8080
* protocol: HTTP
* - ip: 127.0.0.1
* - address: 127.0.0.1
* port: 8081
* protocol:HTTP
* - port: 8443
Expand Down Expand Up @@ -82,10 +83,18 @@ public class ArmeriaSettings {
public static class Port {
/**
* IP address to bind to. If not set, will bind to all addresses, e.g. {@code 0.0.0.0}.
* @deprecated Use {@link #address} instead.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a blank line between the doc and @deprecated. 😉

*/
@Deprecated
@Nullable
private String ip;

/**
* Network address to bind to. If not set, will bind to all addresses, e.g. {@code 0.0.0.0}.
*/
@Nullable
private InetAddress address;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit; do you mind also updating the example?


/**
* Network interface to bind to. If not set, will bind to the first detected network interface.
*/
Expand All @@ -105,20 +114,40 @@ public static class Port {

/**
* Returns the IP address that the {@link Server} uses.
* @deprecated Use {@link #getAddress()} instead.
*/
@Deprecated
@Nullable
public String getIp() {
return ip;
}

/**
* Registers an IP address that the {@link Server} uses.
* @deprecated Use {@link #setAddress(InetAddress)} instead.
*/
@Deprecated
public Port setIp(String ip) {
this.ip = ip;
return this;
}

/**
* Returns the Network address that the {@link Server} uses.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I guess we don't need to capitalize the word

Suggested change
* Returns the Network address that the {@link Server} uses.
* Returns the network address that the {@link Server} uses.

*/
@Nullable
public InetAddress getAddress() {
return address;
}

/**
* Registers a Network address that the {@link Server} uses.
*/
public Port setAddress(InetAddress address) {
this.address = address;
return this;
}

/**
* Returns the network interface that the {@link Server} uses.
*/
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
* Copyright 2023 LINE Corporation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy new year. 🎉 🎉 🎉

*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/
package com.linecorp.armeria.spring;

import static org.assertj.core.api.Assertions.assertThat;

import java.net.InetAddress;
import java.util.Collection;

import javax.inject.Inject;

import org.junit.Test;
import org.junit.runner.RunWith;
import org.springframework.boot.autoconfigure.SpringBootApplication;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.test.annotation.DirtiesContext;
import org.springframework.test.context.ActiveProfiles;
import org.springframework.test.context.junit4.SpringRunner;

import com.linecorp.armeria.server.Server;
import com.linecorp.armeria.server.ServerPort;
import com.linecorp.armeria.spring.ArmeriaSettings.Port;
import com.linecorp.armeria.spring.DeprecatedIpTest.TestConfiguration;

/**
* Tests for keeping the behavior of {@link Port#getIp()}.
*/
@RunWith(SpringRunner.class)
@SpringBootTest(classes = TestConfiguration.class)
@ActiveProfiles({ "local", "deprecatedIpTest" })
@DirtiesContext
public class DeprecatedIpTest {

@SpringBootApplication
static class TestConfiguration {}

@Inject
private Server server;

@Test
public void testIpCanBeUsed() {
final Collection<ServerPort> serverPorts = server.activePorts().values();
for (ServerPort sp : serverPorts) {
final InetAddress address = sp.localAddress().getAddress();
assertThat(address.isAnyLocalAddress() || address.isLoopbackAddress()).isTrue();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: are these values different depending on the OS?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure, I just put 127.0.0.1 & 0.0.0.0 at yml, so keep test both

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ha, if so how about branching it so that we can be sure what the test actually means?

if ("127.0.0.1".equals(address.getHostAddress())) {
    assertThat(...).isTrue();
} else {
    // 0.0.0.0
    assertThat(...).isTrue();
}

}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/*
* Copyright 2023 LINE Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/
package com.linecorp.armeria.spring;

import static org.assertj.core.api.Assertions.assertThat;

import java.net.InetAddress;
import java.util.Collection;

import javax.inject.Inject;

import org.junit.Test;
import org.junit.runner.RunWith;
import org.springframework.boot.autoconfigure.SpringBootApplication;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.test.annotation.DirtiesContext;
import org.springframework.test.context.ActiveProfiles;
import org.springframework.test.context.junit4.SpringRunner;

import com.linecorp.armeria.server.Server;
import com.linecorp.armeria.server.ServerPort;
import com.linecorp.armeria.spring.ArmeriaSettings.Port;
import com.linecorp.armeria.spring.LocalhostAddressTest.TestConfiguration;

/**
* Tests for keeping the behavior of {@link Port#getIp()}.
*/
@RunWith(SpringRunner.class)
@SpringBootTest(classes = TestConfiguration.class)
@ActiveProfiles({ "local", "localhostAddressTest" })
@DirtiesContext
public class LocalhostAddressTest {

@SpringBootApplication
static class TestConfiguration {}

@Inject
private Server server;

@Test
public void testLocalhostAddressCanBeUsed() {
final Collection<ServerPort> serverPorts = server.activePorts().values();
assertThat(serverPorts).hasSize(1);
for (ServerPort sp : serverPorts) {
final InetAddress address = sp.localAddress().getAddress();
assertThat(address.isLoopbackAddress()).isTrue();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ armeria:
ports:
- port: 0
protocol: HTTP
- ip: 127.0.0.1
- address: 127.0.0.1
port: 0
protocol: HTTP
- ip: 0.0.0.0
- address: 0.0.0.0
port: 0
protocol: HTTP
enable-auto-injection: true
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
armeria:
ports:
- ip: 127.0.0.1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional; I'm unsure if this would also work in windows, but I think it would be cool if we can check that non-ip addresses are also accepted

Suggested change
- ip: 127.0.0.1
- address: localhost

port: 0
protocol: HTTP
- ip: 0.0.0.0
port: 0
protocol: HTTP
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
armeria:
ports:
- address: localhost
port: 0
protocol: HTTP