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

Spring Boot Centraldogma Loader #1079

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

flex-gyeongil
Copy link

Disclaimer
This PR contains multiple conceptual things which is on-going development under flex-team.

We affected huge conflict between upstream. so want to resolve some discussion point about our current approach.

Motivation 1
To make centraldogma k8s native

boot-loader, and jib is related

Motivation 2
To support SAML group authorization

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Comment on lines -81 to +88
if (project.name.startsWith("java-spring-boot3")) {
implementation libs.slf4j2.api
testImplementation libs.slf4j2.jul.to.slf4j
testImplementation libs.slf4j2.jcl.over.slf4j
testImplementation libs.slf4j2.log4j.over.slf4j
testRuntimeOnly libs.logback15
configurations.configureEach {
resolutionStrategy {
force libs.slf4j2.api.get()
force libs.slf4j2.jul.to.slf4j.get()
force libs.slf4j2.jcl.over.slf4j.get()
force libs.slf4j2.log4j.over.slf4j.get()
force libs.logback15.get()
}
}
} else {
implementation libs.slf4j1.api
testImplementation libs.slf4j1.jul.to.slf4j
testImplementation libs.slf4j1.jcl.over.slf4j
testImplementation libs.slf4j1.log4j.over.slf4j
testRuntimeOnly libs.logback12
}
implementation libs.slf4j1.api
testImplementation libs.slf4j1.jul.to.slf4j
testImplementation libs.slf4j1.jcl.over.slf4j
testImplementation libs.slf4j1.log4j.over.slf4j
testRuntimeOnly libs.logback12
Copy link
Author

Choose a reason for hiding this comment

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

To make separated concern for other configurations.
I had some dependency issues by this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure which problem you've gone through but make sure not to include the slf4j1 for spring boot3 module.

Comment on lines +115 to +123
// FIXME Should be applied to armeria upstream!!
final int port = uri.getPort() > 0 ? uri.getPort() : defaultPort;
if (!(port == 80 && "http".equals(scheme) || port == 443 && "https".equals(scheme))) {
builder.port(uri.getPort() > 0 ? uri.getPort() : defaultPort);
}
return builder
.path(uri.getPath())
.toUriString();
}
Copy link
Author

Choose a reason for hiding this comment

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

Temporal handling due to known port erasing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops what's the bug btw?

Comment on lines +38 to +62
@Validated
data class ZooKeeperServerConfig(
private val host: String,
private val quorumPort: Int,
private val electionPort: Int,
private val clientPort: Int,
private val groupId: Int?,
private val weight: Int = 1,
) : ZooKeeperServerConfigSpec {
init {
check(weight >= 0) { "weight: $weight (expected: >= 0)" }
}

override fun host(): String = host

override fun quorumPort(): Int = ZooKeeperServerConfigSpec.validatePort(quorumPort, "quorumPort")

override fun electionPort(): Int = ZooKeeperServerConfigSpec.validatePort(electionPort, "electionPort")

override fun clientPort(): Int = clientPort

override fun groupId(): Int? = groupId

override fun weight(): Int = weight
}
Copy link
Author

Choose a reason for hiding this comment

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

Discussion point 1.

Due to hard coupling between configs and jackson deserialization, I wanna suggest to split configuration spec and it's implementation.

This approach can separate dependencies between jackson and others.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good suggestion but I prefer dererring the separation until we use a configuration format that JAckson does not support, as Jackson already supports the YAML format:

public static CentralDogmaConfig load(File configFile) throws JsonMappingException, JsonParseException {
    requireNonNull(configFile, "configFile");
    try {
        if (configFile.getPath().endsWith(".yaml")) {
            return yamlMapper.readValue(configFile, CentralDogmaConfig.class);        
        } else {
            return objectMapper.readValue(configFile, CentralDogmaConfig.class);
        }
    } catch (JsonParseException | JsonMappingException e) {
        throw e;
    } catch (IOException e) {
        throw new IOError(e);
    }
}

I also tried a simple PoC to see if it works:

@Test
void foo() throws JsonProcessingException {
    ObjectMapper jsonMapper = new ObjectMapper();
    ObjectMapper yamlMapper = new ObjectMapper(new YAMLFactory());
    final Foo foo = jsonMapper.readValue("{\"a\":\"foo\",\"bar\":{\"b\":\"c\"}}", Foo.class);
    System.err.println(foo);
    final Foo fooYaml = yamlMapper.readValue(
            "a: foo\n" +
            "bar:\n" +
            "  b: c", Foo.class);
    System.err.println(fooYaml);
}

public static class Foo {

    private final String a;
    private final Bar bar;

    @JsonCreator
    public Foo(@JsonProperty("a") String a, @JsonProperty("bar") Bar bar) {
        this.a = a;
        this.bar = bar;
    }

    @Override
    public String toString() {
        return MoreObjects.toStringHelper(this)
                          .add("a", a)
                          .add("bar", bar)
                          .toString();
    }
}

public static class Bar {

    private final String b;

    @JsonCreator
    public Bar(@JsonProperty("b") String b) {
        this.b = b;
    }

    @Override
    public String toString() {
        return MoreObjects.toStringHelper(this)
                          .add("b", b)
                          .toString();
    }
}

Comment on lines +68 to +102
val tokenAuthorizerCreator =
TokenAuthorizerCreator { mds, sessionManager ->
ApplicationTokenAuthorizer(mds::findTokenBySecret)
.orElse(SessionGroupTokenAuthorizer(sessionManager, adminProperties.adminGroupNames))
}
val authProviderCreator =
if (authConfig == null) {
null
} else {
CentralDogma.AuthProviderCreator { commandExecutor, sessionManager, mds ->
checkNotNull(sessionManager) { "SessionManager is null" }

val parameters =
AuthProviderParameters(
tokenAuthorizerCreator.create(mds, sessionManager),
properties,
sessionManager::generateSessionId,
{ commandExecutor.execute(Command.createSession(it)) },
{ commandExecutor.execute(Command.removeSession(it)) },
)

authConfig.factory().create(parameters)
}
}

val centralDogmaConfig =
object : CentralDogmaConfigSpec by properties {
override fun toString(): String = "Contains sensitive values. Please use actuator instead."
}

return CentralDogmaManager(
CentralDogma.forConfig(centralDogmaConfig, meterRegistry)
.authProviderCreator(authProviderCreator)
.tokenAuthorizerCreator(tokenAuthorizerCreator),
)
Copy link
Author

Choose a reason for hiding this comment

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

Discussion Point 2.

configurable token authorizer and auth provider creator.

current implementation has strong coupling between json config file.

to resolve this coupling, I'd extract it

Comment on lines +40 to +92
samlMetadata: |-
Bag Attributes
friendlyName: encryption
localKeyID: 54 69 6D 65 20 31 37 32 37 32 33 31 35 36 33 35 34 34
subject=C=Unknown, ST=Unknown, L=Unknown, O=Unknown, OU=Unknown, CN=Unknown
issuer=C=Unknown, ST=Unknown, L=Unknown, O=Unknown, OU=Unknown, CN=Unknown
-----BEGIN CERTIFICATE-----
MIIDdzCCAl+gAwIBAgIEbiT6NTANBgkqhkiG9w0BAQUFADBsMRAwDgYDVQQGEwdV
bmtub3duMRAwDgYDVQQIEwdVbmtub3duMRAwDgYDVQQHEwdVbmtub3duMRAwDgYD
VQQKEwdVbmtub3duMRAwDgYDVQQLEwdVbmtub3duMRAwDgYDVQQDEwdVbmtub3du
MB4XDTE4MTAwNTA2MjcyNVoXDTE5MDEwMzA2MjcyNVowbDEQMA4GA1UEBhMHVW5r
bm93bjEQMA4GA1UECBMHVW5rbm93bjEQMA4GA1UEBxMHVW5rbm93bjEQMA4GA1UE
ChMHVW5rbm93bjEQMA4GA1UECxMHVW5rbm93bjEQMA4GA1UEAxMHVW5rbm93bjCC
ASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAK9V1vYacLGyBk3owicRYamd
ttk/VJ0Y79BhjJobpMCeP8oyqiLNoeQXh3q5MI3YRSrSewtJKweun1E2rNHWveuk
6+wIEtJ4HChAorNLkI4R0OG99Qe/kltRsr3Q1GDycuAFi9jWTN221S40Zj9kIadn
3dtwXUrg2gSIdFW7+mhTuODKRzBaZAO/5OZN7600ZL9j7IjtldpIZCum3FIIXTQD
6z88Ymfbgs8eMQzNmXvC04ULWjgD5KzbJX5W2df1Yg3a4QJBcTIyD2NBJKL1410R
t4/tHd7bhcJ+MXU23qShWkxThP8n4akFZ38uslNQeI86F2wWT5uxyrsFmVXtAqMC
AwEAAaMhMB8wHQYDVR0OBBYEFLKyum/cqOwnnMFJdc9SRa9j5H8oMA0GCSqGSIb3
DQEBBQUAA4IBAQBYLxmsaljw9qLFxWBjaQBKv1xSzQIwL01zaXntkS7zhffhdElS
fn9ril18N32M6K/l4wkjY57tUXNyDr6WsYouP+Xv8FilwVM3GKW6Jj3ED5rtkRzr
jWU1t4Si8rLSVClIRw/4UTy7BLonhE47DI/3jGIC60jxrE8fomtHeusioXn1NYfK
Qvrfjd0ZldhBz1ZEU7Dlx6+vQ4YfonFUWDByUJUobF6NIWP+kmdRVV6fX4fP6+Yq
ciPzN59IELyxvnvCSBvuE8ihIzT6zg5bundOU+ATOvTIe+94tXmFwU3+0ifHdmas
fAzZ+eEXN6iz7yZLHLQg5FuRUjvJRWQmSkfe
-----END CERTIFICATE-----
Bag Attributes
friendlyName: signing
localKeyID: 54 69 6D 65 20 31 37 32 37 32 33 31 35 36 33 36 33 33
subject=C=Unknown, ST=Unknown, L=Unknown, O=Unknown, OU=Unknown, CN=Unknown
issuer=C=Unknown, ST=Unknown, L=Unknown, O=Unknown, OU=Unknown, CN=Unknown
-----BEGIN CERTIFICATE-----
MIIDdzCCAl+gAwIBAgIEIXeHCDANBgkqhkiG9w0BAQUFADBsMRAwDgYDVQQGEwdV
bmtub3duMRAwDgYDVQQIEwdVbmtub3duMRAwDgYDVQQHEwdVbmtub3duMRAwDgYD
VQQKEwdVbmtub3duMRAwDgYDVQQLEwdVbmtub3duMRAwDgYDVQQDEwdVbmtub3du
MB4XDTE4MTAwNTA2Mjc0M1oXDTE5MDEwMzA2Mjc0M1owbDEQMA4GA1UEBhMHVW5r
bm93bjEQMA4GA1UECBMHVW5rbm93bjEQMA4GA1UEBxMHVW5rbm93bjEQMA4GA1UE
ChMHVW5rbm93bjEQMA4GA1UECxMHVW5rbm93bjEQMA4GA1UEAxMHVW5rbm93bjCC
ASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAKubyc0G4ekrNBd131G1+WCS
QIMLYhunLfUjz+46PngDWWt7nS21RbHvV8eDirpc3wEuEfTYP4FpwF6etTHB92u6
X+1oSboiMfx3EiEDA+3ziBinzhBP9lL6Xd8x0v7hPCTtlWrwBY5RMSkvBT5N4khX
APQwCgYmwK2UBoTEPcr3O/s2ysbnpu4fa5SAX1mxQhUAeZMRgG7j05jP9Np8KoIH
W0zPsF4W+n+09pbG5JYOgVFkM1gVc56PmTUwFa+4D6Wqq/LyRowkXvjyWPoAE6JI
jbFhu2UTMKpL6kA9j/48b/ixULOblfV6ahB+kA9Pw9kjMAgcXUTm6hRepmu+gTcC
AwEAAaMhMB8wHQYDVR0OBBYEFDXaz1aatlHvzZi355hb+PHMtlK4MA0GCSqGSIb3
DQEBBQUAA4IBAQCXcCcwTpj2bWMzrxJOv1YScPT/KzRNlj3Bu6bKh4UW3//wwfO4
1uUhiYL9ont+EFCX6qtxbrWz16fahklaHKIc4AlKvwplwAqw2FUlNKhztmDDC0UM
as9o/TUs9rIthOofrfho8FeLULsiKQbZjwSp2o81xooIElBxilkXin+5GjyTQIlL
COMJngk/mFdOP5oCvAXVI3YsISgUsDFmR1A1vO0oiUKZY2evruMb7O+Pk2JieyJZ
AYI29mmfoDtaBH9pb1OpqEs1dpkTT1EkM51qOQv0wDGV7cksPOHqBNnJHzc9oyuW
tCaFCVVxUeL48EUfHU3efqCMw9ba++1LlYAp
-----END CERTIFICATE-----
Copy link
Author

Choose a reason for hiding this comment

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

In k8s, or container environment, certificates can injected by environment variable and spring boot can handle it as configuration properties

Copy link
Contributor

Choose a reason for hiding this comment

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

ConfigValueConverter also supports that:


But it's added recently so it might not be available when you started this implementation. 😓

Comment on lines 82 to +84
registerModules(new SimpleModule().addSerializer(Instant.class, InstantSerializer.INSTANCE)
.addDeserializer(Instant.class, InstantDeserializer.INSTANT));
.addDeserializer(Instant.class, InstantDeserializer.INSTANT),
new KotlinModule.Builder().build());
Copy link
Author

Choose a reason for hiding this comment

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

for... some kotlin stuff.

Comment on lines -46 to +50
public Class<?> configType() {
public Class<? extends PluginConfig> configType() {
// Return the plugin class itself because it does not have a configuration.
return TestAllReplicasPlugin.class;
return NoopPluginConfig.class;
Copy link
Author

Choose a reason for hiding this comment

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

Discussion point 3.

From some reason. Plugins have specification for duplication check in plugin group.

It seems weird to check duplication by plugin config class because for that check, some plugins which has no configurations returns itself.

Plugin config may be resuable, then duplication check key should be plugin class, not plugin config.

HDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. 👍

Comment on lines -89 to +110
final ImmutableMap.Builder<Class<?>, Plugin> allPlugins = new ImmutableMap.Builder<>();
for (Plugin plugin : Iterables.concat(plugins, loader)) {
if (plugin.isEnabled(config)) {
allPlugins.put(plugin.configType(), plugin);
}

final List<Plugin> allPlugins = StreamSupport.stream(Iterables.concat(plugins, loader).spliterator(),
false)
.filter(plugin -> plugin.isEnabled(config))
.collect(toImmutableList());

final long uniquePluginCounts = allPlugins.stream()
.map(Plugin::getClass)
.distinct()
.count();

if (allPlugins.size() != uniquePluginCounts) {
throw new IllegalArgumentException("Found duplicated plugins");
}

// IllegalArgumentException is thrown if there are duplicate keys.
final Map<Class<?>, Plugin> pluginMap = allPlugins.build();
if (pluginMap.isEmpty()) {
if (allPlugins.isEmpty()) {
return ImmutableMap.of();
}

final Map<PluginTarget, PluginGroup> pluginGroups =
pluginMap.values()
allPlugins
Copy link
Author

Choose a reason for hiding this comment

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

related with Discution point 3.

duplication check by plugin class count.

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

I haven't discussed this with the other maintainers, but let me leave my thoughts for now:

Due to hard coupling between configs and jackson deserialization, I wanna suggest to split configuration spec and it's implementation.

I'm neutral about this proposition.

I do think it's possible to add ways to create a CentralDogmaConfig (e.g. CentralDogmaConfig.fromYaml()).

Having said this, in terms of 1) keeping the config in-sync with the cloud-native module 2) deduplication I think the proposal makes sense

configurable token authorizer and auth provider creator.

Ideally, I think I prefer that the CentralDogma.java class provides ways to be configured and other applications cloud-native can consume this.

What do you think of exposing an SPI provider which is analogous to AuthProviderCreator, TokenAuthorizerCreator instead?

From some reason. Plugins have specification for duplication check in plugin group.

Sounds like a bug to me, but it's probably better to check with the original authors @minwoox @ikhoon

includeWithFlags ':client:java-spring-boot3-starter', 'java17', 'publish', 'relocate'
includeWithFlags ':client:java-spring-boot3-autoconfigure', 'java17', 'spring-boot3', 'publish', 'relocate'
includeWithFlags ':client:java-spring-boot3-starter', 'java17', 'spring-boot3', 'publish', 'relocate'
includeWithFlags ':cloud-native:boot-loader', 'kotlin', 'spring-boot3'
Copy link
Contributor

Choose a reason for hiding this comment

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

Question) Have you tried specifying java17? I think it could help avoid manually setting the supported minimum java version in gradle scripts

Suggested change
includeWithFlags ':cloud-native:boot-loader', 'kotlin', 'spring-boot3'
includeWithFlags ':cloud-native:boot-loader', 'java17', 'kotlin', 'spring-boot3'

@minwoox
Copy link
Contributor

minwoox commented Jan 2, 2025

What do you think of exposing an SPI provider which is analogous to AuthProviderCreator, TokenAuthorizerCreator instead?

I like this approach. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants