Uploaded image for project: 'Apache Twill'
  1. Apache Twill
  2. TWILL-189

SecureStoreUpdater should support updating secure store as other users

    Details

    • Type: New Feature
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 0.7.0-incubating
    • Fix Version/s: 0.11.0
    • Component/s: api
    • Labels:
      None

      Description

      SecureStoreUpdater always creates secure stores for the current user and writes these as the current user.
      It should allow writing the secure store as a different user.

      For an example use case, see: https://github.com/caskdata/cdap/pull/6380

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/twill/pull/48

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/twill/pull/48
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on a diff in the pull request:

          https://github.com/apache/twill/pull/48#discussion_r109237595

          — Diff: twill-api/src/main/java/org/apache/twill/api/security/SecureStoreRenewer.java —
          @@ -0,0 +1,40 @@
          +/*
          + * 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.twill.api.security;
          +
          +import org.apache.twill.api.RunId;
          +import org.apache.twill.api.SecureStore;
          +import org.apache.twill.filesystem.Location;
          +
          +import java.io.IOException;
          +
          +/**
          + * This class is responsible for renewing the secure store used by application.
          + */
          +public abstract class SecureStoreRenewer {
          +
          + /**
          + * Renew the secure store for an application run. It must use the

          {@link Location}

          provided by the
          — End diff –

          updated

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/48#discussion_r109237595 — Diff: twill-api/src/main/java/org/apache/twill/api/security/SecureStoreRenewer.java — @@ -0,0 +1,40 @@ +/* + * 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.twill.api.security; + +import org.apache.twill.api.RunId; +import org.apache.twill.api.SecureStore; +import org.apache.twill.filesystem.Location; + +import java.io.IOException; + +/** + * This class is responsible for renewing the secure store used by application. + */ +public abstract class SecureStoreRenewer { + + /** + * Renew the secure store for an application run. It must use the {@link Location} provided by the — End diff – updated
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on a diff in the pull request:

          https://github.com/apache/twill/pull/48#discussion_r109236503

          — Diff: twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillRunnerService.java —
          @@ -265,6 +254,44 @@ public void cancel() {
          }

          @Override
          + public Cancellable setSecureStoreRenewer(SecureStoreRenewer renewer, long initialDelay,
          + long delay, long retryDelay, TimeUnit unit) {
          + synchronized (this) {
          + if (secureStoreScheduler != null) {
          + // Shutdown and block until the schedule is stopped
          + stopScheduler(secureStoreScheduler);
          +
          + Futures.getUnchecked(secureStoreScheduler.submit(new Runnable() {
          — End diff –

          Strange, I thought I removed it. Looks like some changes are lost. Let me double check

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/48#discussion_r109236503 — Diff: twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillRunnerService.java — @@ -265,6 +254,44 @@ public void cancel() { } @Override + public Cancellable setSecureStoreRenewer(SecureStoreRenewer renewer, long initialDelay, + long delay, long retryDelay, TimeUnit unit) { + synchronized (this) { + if (secureStoreScheduler != null) { + // Shutdown and block until the schedule is stopped + stopScheduler(secureStoreScheduler); + + Futures.getUnchecked(secureStoreScheduler.submit(new Runnable() { — End diff – Strange, I thought I removed it. Looks like some changes are lost. Let me double check
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user albertshau commented on the issue:

          https://github.com/apache/twill/pull/48

          just a couple comments

          Show
          githubbot ASF GitHub Bot added a comment - Github user albertshau commented on the issue: https://github.com/apache/twill/pull/48 just a couple comments
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user albertshau commented on a diff in the pull request:

          https://github.com/apache/twill/pull/48#discussion_r109223872

          — Diff: twill-api/src/main/java/org/apache/twill/api/security/SecureStoreRenewer.java —
          @@ -0,0 +1,40 @@
          +/*
          + * 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.twill.api.security;
          +
          +import org.apache.twill.api.RunId;
          +import org.apache.twill.api.SecureStore;
          +import org.apache.twill.filesystem.Location;
          +
          +import java.io.IOException;
          +
          +/**
          + * This class is responsible for renewing the secure store used by application.
          + */
          +public abstract class SecureStoreRenewer {
          +
          + /**
          + * Renew the secure store for an application run. It must use the

          {@link Location}

          provided by the
          — End diff –

          incomplete javadoc

          Show
          githubbot ASF GitHub Bot added a comment - Github user albertshau commented on a diff in the pull request: https://github.com/apache/twill/pull/48#discussion_r109223872 — Diff: twill-api/src/main/java/org/apache/twill/api/security/SecureStoreRenewer.java — @@ -0,0 +1,40 @@ +/* + * 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.twill.api.security; + +import org.apache.twill.api.RunId; +import org.apache.twill.api.SecureStore; +import org.apache.twill.filesystem.Location; + +import java.io.IOException; + +/** + * This class is responsible for renewing the secure store used by application. + */ +public abstract class SecureStoreRenewer { + + /** + * Renew the secure store for an application run. It must use the {@link Location} provided by the — End diff – incomplete javadoc
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user albertshau commented on a diff in the pull request:

          https://github.com/apache/twill/pull/48#discussion_r109225715

          — Diff: twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillRunnerService.java —
          @@ -265,6 +254,44 @@ public void cancel() {
          }

          @Override
          + public Cancellable setSecureStoreRenewer(SecureStoreRenewer renewer, long initialDelay,
          + long delay, long retryDelay, TimeUnit unit) {
          + synchronized (this) {
          + if (secureStoreScheduler != null) {
          + // Shutdown and block until the schedule is stopped
          + stopScheduler(secureStoreScheduler);
          +
          + Futures.getUnchecked(secureStoreScheduler.submit(new Runnable() {
          — End diff –

          what is this doing? isn't it shutdown by stopScheduler()?

          Show
          githubbot ASF GitHub Bot added a comment - Github user albertshau commented on a diff in the pull request: https://github.com/apache/twill/pull/48#discussion_r109225715 — Diff: twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillRunnerService.java — @@ -265,6 +254,44 @@ public void cancel() { } @Override + public Cancellable setSecureStoreRenewer(SecureStoreRenewer renewer, long initialDelay, + long delay, long retryDelay, TimeUnit unit) { + synchronized (this) { + if (secureStoreScheduler != null) { + // Shutdown and block until the schedule is stopped + stopScheduler(secureStoreScheduler); + + Futures.getUnchecked(secureStoreScheduler.submit(new Runnable() { — End diff – what is this doing? isn't it shutdown by stopScheduler()?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on the issue:

          https://github.com/apache/twill/pull/48

          I tried to create a unit test using MiniKdc, but seems like I hit this bug https://issues.apache.org/jira/browse/HDFS-9213

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on the issue: https://github.com/apache/twill/pull/48 I tried to create a unit test using MiniKdc, but seems like I hit this bug https://issues.apache.org/jira/browse/HDFS-9213
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user chtyim opened a pull request:

          https://github.com/apache/twill/pull/48

          (TWILL-189) Allows secure store update with different UGI

          • Deprecated the old TwillRunner.scheduleSecureStoreUpdate
          • Added new TwillRunner.setSecureStoreRenewer method
          • Takes SecureStoreRenewer that writes SecureStore via
            SecureStoreWriter
          • The renewer implementation can use appropriate UGI.doAs to make
            call to SecureStoreWriter

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/chtyim/twill feature/TWILL-189

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/twill/pull/48.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #48


          commit 292008e903866b828e32d5a93aa2a7b6775e46c7
          Author: Terence Yim <chtyim@apache.org>
          Date: 2017-03-28T21:31:06Z

          (TWILL-189) Allows secure store update with different UGI

          • Deprecated the old TwillRunner.scheduleSecureStoreUpdate
          • Added new TwillRunner.setSecureStoreRenewer method
          • Takes SecureStoreRenewer that writes SecureStore via
            SecureStoreWriter
          • The renewer implementation can use appropriate UGI.doAs to make
            call to SecureStoreWriter

          Show
          githubbot ASF GitHub Bot added a comment - GitHub user chtyim opened a pull request: https://github.com/apache/twill/pull/48 ( TWILL-189 ) Allows secure store update with different UGI Deprecated the old TwillRunner.scheduleSecureStoreUpdate Added new TwillRunner.setSecureStoreRenewer method Takes SecureStoreRenewer that writes SecureStore via SecureStoreWriter The renewer implementation can use appropriate UGI.doAs to make call to SecureStoreWriter You can merge this pull request into a Git repository by running: $ git pull https://github.com/chtyim/twill feature/ TWILL-189 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/48.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #48 commit 292008e903866b828e32d5a93aa2a7b6775e46c7 Author: Terence Yim <chtyim@apache.org> Date: 2017-03-28T21:31:06Z ( TWILL-189 ) Allows secure store update with different UGI Deprecated the old TwillRunner.scheduleSecureStoreUpdate Added new TwillRunner.setSecureStoreRenewer method Takes SecureStoreRenewer that writes SecureStore via SecureStoreWriter The renewer implementation can use appropriate UGI.doAs to make call to SecureStoreWriter
          Hide
          anwar1 Ali Anwar added a comment -

          I like the second option, which provides a LocationProvider. That way, user can have more logic instead of being restricted to writing the credentials.
          And agreed - we can remove the getStore() method then.

          Show
          anwar1 Ali Anwar added a comment - I like the second option, which provides a LocationProvider. That way, user can have more logic instead of being restricted to writing the credentials. And agreed - we can remove the getStore() method then.
          Hide
          chtyim Terence Yim added a comment -

          Or to give more flexibility, the method can be void persist(LocationProvider locationProvider), with the LocationProvider has a Location get() method.

          Show
          chtyim Terence Yim added a comment - Or to give more flexibility, the method can be void persist(LocationProvider locationProvider) , with the LocationProvider has a Location get() method.
          Hide
          chtyim Terence Yim added a comment -

          Shall we remove the <T> T getStore() method then? Since now's it's the SecureStore responsibility to write to a given location? By the way, instead of giving a URI, I think it's better to have the method as:

          void persist(OutputProvider<OutputStream> osProvider)

          with OutputProvider has a single method OutputStream open().

          With that, the user doesn't need to handle the URI by himself. With this API, I believe we also need to fix the LocationFactory implementations (HDFS and FileContext) to have the create method always use the current UGI to open the stream, instead of the cached one.

          Show
          chtyim Terence Yim added a comment - Shall we remove the <T> T getStore() method then? Since now's it's the SecureStore responsibility to write to a given location? By the way, instead of giving a URI , I think it's better to have the method as: void persist(OutputProvider<OutputStream> osProvider) with OutputProvider has a single method OutputStream open() . With that, the user doesn't need to handle the URI by himself. With this API, I believe we also need to fix the LocationFactory implementations (HDFS and FileContext) to have the create method always use the current UGI to open the stream, instead of the cached one.
          Hide
          anwar1 Ali Anwar added a comment - - edited

          Proposed change is to add a method to the SecureStore interface as follows:

          // Responsible for persisting the credentials to the specified URI.
          void persist(URI uri);

          Additionally, TwillRunner's scheduleSecureStoreUpdate method's behavior will be changed to only support one secure store updater at any given time.
          Calling this method will replace any previously scheduled updater.

          Show
          anwar1 Ali Anwar added a comment - - edited Proposed change is to add a method to the SecureStore interface as follows: // Responsible for persisting the credentials to the specified URI. void persist(URI uri); Additionally, TwillRunner's scheduleSecureStoreUpdate method's behavior will be changed to only support one secure store updater at any given time. Calling this method will replace any previously scheduled updater.

            People

            • Assignee:
              Unassigned
              Reporter:
              anwar1 Ali Anwar
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development