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.

Sign up for GitHub

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

zuul-core: Add overload for OriginName trust #1066

Merged
merged 1 commit into from
May 20, 2021
Merged

zuul-core: Add overload for OriginName trust #1066

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
45 changes: 22 additions & 23 deletions zuul-core/src/main/java/com/netflix/zuul/origins/OriginName.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@
import com.netflix.zuul.util.VipUtils;
import java.util.Locale;
import java.util.Objects;
import java.util.Optional;
import javax.annotation.CheckReturnValue;

public final class OriginName {
/**
Expand All @@ -44,30 +42,36 @@ public final class OriginName {
* used for establishing a secure connection, as well as logging.
*/
private final String authority;

/**
* Indicates if the authority comes from a trusted source.
* @deprecated use {@link #fromVipAndApp(String, String)}
*/
private final boolean authorityTrusted;

@Deprecated
public static OriginName fromVip(String vip) {
return fromVip(vip, vip);
return fromVipAndApp(vip, VipUtils.extractUntrustedAppNameFromVIP(vip));
}

/**
* @deprecated use {@link #fromVipAndApp(String, String, String)}
*/
@Deprecated
public static OriginName fromVip(String vip, String niwsClientName) {
return new OriginName(niwsClientName, vip, VipUtils.extractUntrustedAppNameFromVIP(vip), false);
return fromVipAndApp(vip, VipUtils.extractUntrustedAppNameFromVIP(vip), niwsClientName);
}

@CheckReturnValue
public OriginName withTrustedAuthority(String authority) {
return new OriginName(niwsClientName, target, authority, true);
public static OriginName fromVipAndApp(String vip, String appName) {
return fromVipAndApp(vip, appName, vip);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't obvious without a comment explaining why the vip and niws name may or may not be the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Followed up here: #1067

}

private OriginName(String niwsClientName, String target, String authority, boolean authorityTrusted) {
this.niwsClientName = Objects.requireNonNull(niwsClientName, "niwsClientName");
this.metricId = niwsClientName.toLowerCase(Locale.ROOT);
public static OriginName fromVipAndApp(String vip, String appName, String niwsClientName) {
return new OriginName(vip, appName, niwsClientName);
}

private OriginName(String target, String authority, String niwsClientName) {
this.target = Objects.requireNonNull(target, "target");
this.authority = Objects.requireNonNull(authority, "authority");
this.authorityTrusted = authorityTrusted;
this.niwsClientName = Objects.requireNonNull(niwsClientName, "niwsClientName");
this.metricId = niwsClientName.toLowerCase(Locale.ROOT);
}

/**
Expand Down Expand Up @@ -97,11 +101,8 @@ public String getMetricId() {
* Returns the Authority of this origin. This is used for establishing secure connections. May be absent
* if the authority is not trusted.
*/
public Optional<String> getTrustedAuthority() {
if (authorityTrusted) {
return Optional.of(authority);
}
return Optional.empty();
public String getAuthority() {
return authority;
}

@Override
Expand All @@ -110,15 +111,14 @@ public boolean equals(Object o) {
return false;
}
OriginName that = (OriginName) o;
return authorityTrusted == that.authorityTrusted
&& Objects.equals(niwsClientName, that.niwsClientName)
return Objects.equals(niwsClientName, that.niwsClientName)
&& Objects.equals(target, that.target)
&& Objects.equals(authority, that.authority);
}

@Override
public int hashCode() {
return Objects.hash(authorityTrusted, niwsClientName, target, authority);
return Objects.hash(niwsClientName, target, authority);
}

@Override
Expand All @@ -127,7 +127,6 @@ public String toString() {
"niwsClientName='" + niwsClientName + '\'' +
", target='" + target + '\'' +
", authority='" + authority + '\'' +
", authorityTrusted=" + authorityTrusted +
'}';
}
}
11 changes: 3 additions & 8 deletions zuul-core/src/test/java/com/netflix/zuul/origins/OriginNameTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
package com.netflix.zuul.origins;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;

import org.junit.Test;
import org.junit.runner.RunWith;
Expand All @@ -27,13 +26,9 @@
@RunWith(JUnit4.class)
public class OriginNameTest {
Copy link
Collaborator

@argha-c argha-c May 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This needs more unit tests, at least:

  • verification of equality check
  • Tests for when niws name and vip are different

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above PR.

@Test
public void getTrustedAuthority() {
OriginName originName = OriginName.fromVip("woodly-doodly");
public void getAuthority() {
OriginName trusted = OriginName.fromVipAndApp("woodly-doodly", "westerndigital");

assertFalse(originName.getTrustedAuthority().isPresent());

OriginName trusted = originName.withTrustedAuthority("westerndigital");

assertEquals("westerndigital", trusted.getTrustedAuthority().get());
assertEquals("westerndigital", trusted.getAuthority());
}
}