madrob commented on a change in pull request #2010:
URL: https://github.com/apache/lucene-solr/pull/2010#discussion_r508842858
##########
File path: solr/core/src/test/org/apache/solr/cloud/SSLMigrationTest.java
##########
@@ -63,7 +61,7 @@ public void test() throws Exception {
testMigrateSSL(new SSLTestConfig(false, false));
}
- public void testMigrateSSL(SSLTestConfig sslConfig) throws Exception {
+ private void testMigrateSSL(SSLTestConfig sslConfig) throws Exception {
Review comment:
?
##########
File path: solr/core/src/java/org/apache/solr/core/ZkContainer.java
##########
@@ -263,4 +275,27 @@ public void close() {
public ExecutorService getCoreZkRegisterExecutorService() {
return coreZkRegister;
}
+
+ private void setGlobalUrlScheme(final ZkController zkCtrl) {
+ // Set the global urlScheme from cluster prop or if that is not set, look
at the urlScheme sys prop
+ final String urlScheme =
zkCtrl.getZkStateReader().getClusterProperty(ZkStateReader.URL_SCHEME, null);
+ if (StringUtils.isNotEmpty(urlScheme)) {
+ // track the urlScheme in a global so we can use it during ZK read /
write operations for cluster state objects
+ GlobalStateVars.singleton().setUrlScheme(urlScheme);
+ } else {
+ final String urlSchemeFromSysProp = System.getProperty(URL_SCHEME, HTTP);
+ if (HTTPS.equals(urlSchemeFromSysProp)) {
+ // it's OK to set the cluster prop to https b/c we're making the
cluster more secure, not less
+ // wouldn't do this for http though
+ log.warn("Cluster property 'urlScheme' not set but system property is
set to 'https'. Updating the cluster property to match.");
Review comment:
If we have an initially HTTP cluster, and then one node comes up with
HTTPS, then that will bring down the rest of the cluster, right? I don't think
we can do this rolling, however.
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/GlobalStateVars.java
##########
@@ -0,0 +1,79 @@
+/*
+ * 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
+ *
+ * http://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 org.apache.solr.common.cloud;
+
+import static org.apache.solr.common.cloud.ZkStateReader.URL_SCHEME;
+
+/**
+ * Singleton access to global vars in persisted state, such as the urlScheme,
which although is stored in ZK as a cluster property
+ * really should be treated like a static global that is set at initialization
and not altered after.
+ */
+public class GlobalStateVars {
+
+ public static final String HTTP = "http";
+ public static final String HTTPS = "https";
+ public static final String SCHEME_VAR = "${scheme}://";
+ public static final String HTTPS_PORT_PROP = "solr.jetty.https.port";
+
+ private static final GlobalStateVars _singleton = new GlobalStateVars();
Review comment:
I prefer the enum singleton pattern
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/GlobalStateVars.java
##########
@@ -0,0 +1,79 @@
+/*
+ * 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
+ *
+ * http://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 org.apache.solr.common.cloud;
+
+import static org.apache.solr.common.cloud.ZkStateReader.URL_SCHEME;
+
+/**
+ * Singleton access to global vars in persisted state, such as the urlScheme,
which although is stored in ZK as a cluster property
+ * really should be treated like a static global that is set at initialization
and not altered after.
+ */
+public class GlobalStateVars {
+
+ public static final String HTTP = "http";
+ public static final String HTTPS = "https";
+ public static final String SCHEME_VAR = "${scheme}://";
+ public static final String HTTPS_PORT_PROP = "solr.jetty.https.port";
+
+ private static final GlobalStateVars _singleton = new GlobalStateVars();
+
+ public static GlobalStateVars singleton() {
+ return _singleton;
+ }
+
+ private String urlScheme = System.getProperty(URL_SCHEME, HTTP);
+
+ // no new! you have to use the static singleton!
+ private GlobalStateVars() {
+ }
+
+ /**
+ * Set the global urlScheme variable; ideally this should be immutable once
set, but some tests rely on changing
+ * the value on-the-fly.
+ * @param urlScheme The new URL scheme, either http or https.
+ */
+ public void setUrlScheme(final String urlScheme) {
+ if (HTTP.equals(urlScheme) || HTTPS.equals(urlScheme)) {
+ this.urlScheme = urlScheme;
+ } else {
+ throw new IllegalArgumentException("Invalid urlScheme: "+urlScheme);
+ }
+ }
+
+ /**
+ * Given a URL with a replaceable parameter for the scheme, return a new URL
with the correct scheme applied.
+ * @param url A URL to change the scheme (http|https)
+ * @return A new URL with the correct scheme or null if the supplied url
remains unchanged.
+ */
+ public String applyUrlSchemeIfChanged(final String url) {
Review comment:
null on unchanged seems like a trap to me
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/ZkCoreNodeProps.java
##########
@@ -20,6 +20,9 @@
private ZkNodeProps nodeProps;
public ZkCoreNodeProps(ZkNodeProps nodeProps) {
+ if (nodeProps == null) {
Review comment:
Use Objects.requireNonNull
##########
File path: solr/core/src/java/org/apache/solr/core/ZkContainer.java
##########
@@ -31,6 +31,7 @@
import java.util.function.Supplier;
import org.apache.commons.lang3.StringUtils;
+import org.apache.solr.common.cloud.GlobalStateVars;
Review comment:
nit: alphabetize imports
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/ZkNodeProps.java
##########
@@ -105,6 +111,17 @@ public static ZkNodeProps load(byte[] bytes) {
@Override
public void write(JSONWriter jsonWriter) {
+ final String baseUrl = (String)propMap.get(ZkStateReader.BASE_URL_PROP);
+ if (baseUrl != null && !baseUrl.startsWith(SCHEME_VAR)) {
+ final int at = baseUrl.indexOf("://");
+ if (at != -1) {
+ final String updatedUrl = SCHEME_VAR + baseUrl.substring(at+3);
+ Map<String,Object> modMap = new LinkedHashMap<>(propMap);
+ modMap.put(ZkStateReader.BASE_URL_PROP, updatedUrl);
+ jsonWriter.write(modMap);
Review comment:
I think it's clearer here if we only have a single return from the
method, so we should fall through or have an else.
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/GlobalStateVars.java
##########
@@ -0,0 +1,79 @@
+/*
+ * 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
+ *
+ * http://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 org.apache.solr.common.cloud;
+
+import static org.apache.solr.common.cloud.ZkStateReader.URL_SCHEME;
+
+/**
+ * Singleton access to global vars in persisted state, such as the urlScheme,
which although is stored in ZK as a cluster property
+ * really should be treated like a static global that is set at initialization
and not altered after.
+ */
+public class GlobalStateVars {
Review comment:
I don't think we want to encourage further use of this pattern. Let's
call it something more specific like `UrlSchemeState`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
