-
Notifications
You must be signed in to change notification settings - Fork 9.2k
HADOOP-19212. Avoid Subject.getSubject method on newer JVMs #7081
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| /** | ||
| * 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.hadoop.util.subject; | ||
|
|
||
| import javax.security.auth.Subject; | ||
| import java.security.AccessControlContext; | ||
| import java.security.AccessController; | ||
|
|
||
| /** | ||
| * Calls Subject methods directly, as this class should not be classloaded on Java 18 and above | ||
| */ | ||
| class ClassicSubjectAdapter implements HiddenSubjectAdapter { | ||
|
|
||
| ClassicSubjectAdapter() {} | ||
|
|
||
| @Override | ||
| public Subject getSubject() { | ||
| final AccessControlContext context = AccessController.getContext(); | ||
| return Subject.getSubject(context); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| /** | ||
| * 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.hadoop.util.subject; | ||
|
|
||
| import javax.security.auth.Subject; | ||
|
|
||
| public interface HiddenSubjectAdapter { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't really like the name, maybe rename to JDKSpecificSubjectAdapter or similar ?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be package-private instead of public? |
||
| Subject getSubject(); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| /** | ||
| * 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.hadoop.util.subject; | ||
|
|
||
| import javax.security.auth.Subject; | ||
|
|
||
| /** | ||
| * javax.security.auth.Subject.getSubject is deprecated for removal. | ||
| * The replacement API exists only in Java 18 and above. | ||
| * This class helps use the newer API if available, without raising the language level. | ||
| */ | ||
| public class SubjectAdapter { | ||
| private static final HiddenSubjectAdapter instance; | ||
| static { | ||
| int version = 0; | ||
| try { | ||
| version = Integer.parseInt(System.getProperty("java.specification.version")); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My comment about dependencies applies to Shell as well. Let me know how you'd like to proceed with this one as well.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO testing trying to load the new classes and failing over to the old classes if they cannot be found would be more robust.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
interesting thought. how about
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @stoty interesting thought about attempting and fallbacks We'd try the java18+ first & fall back to java <= 17 otherwise? or do it in the opposite direction, at least for now? |
||
| } catch (Throwable ignored) {} | ||
| if (version >= 18) { | ||
| instance = new SubjectAdapterJava18AndAbove(); | ||
| } else { | ||
| instance = new ClassicSubjectAdapter(); | ||
| } | ||
| } | ||
|
|
||
| private SubjectAdapter() {} | ||
|
|
||
| public static Subject getSubject() { | ||
| return instance.getSubject(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| /** | ||
| * 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.hadoop.util.subject; | ||
|
|
||
| import javax.security.auth.Subject; | ||
| import java.lang.reflect.InvocationTargetException; | ||
| import java.lang.reflect.Method; | ||
|
|
||
| /** | ||
| * Indirectly calls Subject.current(), which exists in Java 18 and above only | ||
| */ | ||
| class SubjectAdapterJava18AndAbove implements HiddenSubjectAdapter { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use our new DynMethods classes, (which we copied from parquet and which is also found in iceberg). It fails better and as it is so common it's good to get familiar with it
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @steveloughran my comment from here still applies. #7081 (comment) |
||
| private final Method currentMethod; | ||
|
|
||
| SubjectAdapterJava18AndAbove() { | ||
| try { | ||
| currentMethod = Subject.class.getMethod("current"); | ||
| } catch (NoSuchMethodException e) { | ||
| throw new RuntimeException("Unable to find javax.security.auth.Subject.current() method", e); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public Subject getSubject() { | ||
| try { | ||
| return (Subject) currentMethod.invoke(null); | ||
| } catch (IllegalAccessException | InvocationTargetException e) { | ||
| // we would return null, but null has meaning here | ||
| throw new RuntimeException("Unable to call Subject.current()", e); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| /** | ||
| * 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.hadoop.util.subject; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; | ||
|
|
||
| class TestSubjectAdapter { | ||
|
|
||
| @Test | ||
| void getSubject() { | ||
| // how getSubject operates depends on the JVM calling it. | ||
| // asserting that it does not throw is a valid test, especially on Java 18 and above | ||
| // prior to Java 18, this method is just a simple wrapper | ||
| assertDoesNotThrow(() -> SubjectAdapter.getSubject()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use our LambdaTestUtils.intercept() here as it fails better, can validates the type and message
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment in #7081 (comment) applies here as well. |
||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a package-info.java in this new sub-directory and annotate it as private and unstable?
Example: https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/concurrent/package-info.java