hsheinblatt commented on code in PR #1361: URL: https://github.com/apache/knox/pull/1361#discussion_r3900519912
########## gateway-server/src/main/java/org/apache/knox/gateway/services/factory/DelegationPolicyServiceFactory.java: ########## @@ -0,0 +1,84 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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 org.apache.knox.gateway.services.factory; + +import org.apache.knox.gateway.GatewayMessages; +import org.apache.knox.gateway.config.GatewayConfig; +import org.apache.knox.gateway.i18n.messages.MessagesFactory; +import org.apache.knox.gateway.services.GatewayServices; +import org.apache.knox.gateway.services.Service; +import org.apache.knox.gateway.services.ServiceLifecycleException; +import org.apache.knox.gateway.services.ServiceType; +import org.apache.knox.gateway.services.knoxidf.delegation.EmptyDelegationPolicyService; +import org.apache.knox.gateway.services.knoxidf.delegation.JdbcDelegationPolicyService; + +import java.util.Collection; +import java.util.List; +import java.util.Map; + +public class DelegationPolicyServiceFactory extends AbstractServiceFactory { + + private static final GatewayMessages LOG = MessagesFactory.get(GatewayMessages.class); + private static final String DEFAULT_IMPLEMENTATION = EmptyDelegationPolicyService.class.getName(); + + @Override + protected Service createService(GatewayServices gatewayServices, ServiceType serviceType, + GatewayConfig gatewayConfig, Map<String, String> options, String implementation) + throws ServiceLifecycleException { + + String implementationToUse = implementation; Review Comment: For the empty implementation, I was following the pattern from FederatedIdentityService, so Sandor might have a better understanding of the intent. My understanding is that the empty service is a soft config failure. On a config failure, you could say, that's not allowed, fail knox startup, everything stops with a clear config exception. Other services do that, like DefaultTokenAuthorityService. The pattern in the FederatedIdentityService, also used in TrustedOidcIssuer, is that if there is some failure, it doesn't block startup, instead it loads the empty service and that will cause runtime errors if those paths are hit. One example failure case is you've configured the delegation service enabled flag to allow policy-driven delegation token exchanges, but you haven't configured the delegation service. Arguably this should just fail startup -- you want to use the delegation service but it won't work. But possibly there are other topologies that you'd want to run and not block for this. Depending on your deployment model, failing startup is cleaner -- for example in k8s, you'd ideally get a crash loop backoff and not deploy the new version until you fixed the config. But in other deployment models, it may cause a real problem. With this implementation, knox starts up, but then if you try a delegation token exchange, they'll all fail with a configuration error. It also fails over to an empty config on other init failures, so if the config is incorrect or there's a runtime problem initializing, knox will still start up but with the empty service. We could make a distinction on what causes a full failure and what fails over to empty based on the type of configuration problem or runtime issue. The FederatedIdentity service is different: we may not need it at init time. Since the delegation service is to have an explicit enablement flag, we can fail startup deterministically if we're comfortable failing startup. For now I suggest we keep this failure pattern, and follow up to further justify this pattern or rethink it across the three services. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
