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

HDDS-4704. Add permission check in OMDBCheckpointServlet #1801

Merged
merged 12 commits into from
Jan 30, 2021

Conversation

smengcl
Copy link
Contributor

@smengcl smengcl commented Jan 14, 2021

What changes were proposed in this pull request?

Add permission check in OMDBCheckpointServlet

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-4704

How was this patch tested?

  1. Add new UT in TestOMDbCheckpointServlet to test with different "login" users.
  2. Pending manual testing on a cluster.

Change-Id: I2745bff95c9b0448a64921f5659b49d3f2c3eb59
@smengcl smengcl self-assigned this Jan 14, 2021
@smengcl smengcl added the om label Jan 14, 2021
@smengcl smengcl added this to the 1.1.0 milestone Jan 14, 2021
@swagle swagle requested review from elek and xiaoyuyao January 20, 2021 19:19
…(even when using non Ozone admin principal and the login user is hadoop).

Using request.getUserPrincipal() instead.

Change-Id: I5b028388f632b96ec1f54f9949f3958b7c85324f
Change-Id: I04c3489be51631913a6568c8a57b23b5c1e233fb
@smengcl smengcl marked this pull request as ready for review January 27, 2021 10:26
Copy link
Member

@elek elek left a comment

Choose a reason for hiding this comment

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

Thanks the patch @smengcl. Overall it looks good to me, but I have a few questions.

And one more: Can you please add at least two lines to the documentation to make it clear which users should be added to the admins?

I think it's a very tricky requirement, and we need to make it clear from day 1 what are the requirements...

Change-Id: Id718a55f763f01b52b078bb2e48786dd6cc5ccd6
Change-Id: I54ec7fcd7eda263289c962f6c4c98731ce586f3f
Change-Id: I57b49b4c4a5993524e315b1a5d22c96712edb1ad
Change-Id: I6ce51eba9b039c2b726413695a4d8b30f00a5e81
Copy link
Contributor

@xiaoyuyao xiaoyuyao left a comment

Choose a reason for hiding this comment

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

Thanks @smengcl for working on this. The change LGTM, just few minor comments inline.

Change-Id: I806784349250547ecd2e31a52ac866c416c503a0
Change-Id: Ifca3404f6c990c717d5f9aea13e9e05fb93a9d14
Change-Id: Ib6ca8e1ed41baed46d1a6031be92f6219713e653
Copy link
Member

@elek elek left a comment

Choose a reason for hiding this comment

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

Thank you very much the update. @smengcl. Looks good to me.

One of my comments still not addressed:

And one more: Can you please add at least two lines to the documentation to make it clear which users should be added to the admins?

If you wouldn't like to add it to the markdown docs, can you please explain how can it be fixed in the error log?

Like this:

LOG.error("Permission denied: User principal '{}' does not have"
          + " access to /dbCheckpoint. In case the Ozone services started with different users, the principal of other services should be added to the Ozone administrators. Please add {} user to the 'ozone.administrators' configuration value.", userPrincipalName, userPrincipalName);```

Change-Id: I77ef3b4e942a154f50bdffb6fc28057104eab786
@smengcl
Copy link
Contributor Author

smengcl commented Jan 29, 2021

@elek Done.

Change-Id: Ic7e12559262cea60710768e844a061aea8a8fc21
Copy link
Member

@elek elek left a comment

Choose a reason for hiding this comment

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

Thanks the update @smengcl (and the review @xiaoyuyao).

@elek elek merged commit d964cc9 into apache:master Jan 30, 2021
errose28 added a commit to errose28/ozone that referenced this pull request Feb 1, 2021
* master: (176 commits)
  HDDS-4760. Intermittent failure in ozone-ha acceptance test (apache#1853)
  HDDS-4770. Upgrade Ratis Thirdparty to 0.6.0 (apache#1868)
  HDDS-4765. Update close-pending workflow for new repo (apache#1856)
  HDDS-4737. Add ModifierOrder to checkstyle rules (apache#1839)
  HDDS-4704. Add permission check in OMDBCheckpointServlet (apache#1801)
  HDDS-4757. Unnecessary WARNING to set OZONE_CONF_DIR (apache#1849)
  HDDS-4751. TestOzoneFileSystem#testTrash failed when enabledFileSystemPaths and omRatisDisabled (apache#1851)
  HDDS-4736. Intermittent failure in testExpiredCertificate (apache#1838)
  HDDS-4758. Adjust classpath of ozone version to include log4j (apache#1850)
  HDDS-4518. Add metrics around Trash Operations. (apache#1832)
  HDDS-4708. Optimization: update RetryCount less frequently (update once per ~100) (apache#1805)
  HDDS-4748. sonarqube issue fix - "static" members should be accessed statically (apache#1748)
  HDDS-2402. Adapt hadolint check to improved CI framework (apache#1778)
  HDDS-4698. Upgrade Java for Sonar check (apache#1800)
  HDDS-4739. Upgrade Ratis to 1.1.0-eb66796d-SNAPSHOT (apache#1842)
  HDDS-4735. Fix typo in hdds.proto (apache#1837)
  HDDS-4430. OM failover timeout is too short (apache#1807)
  HDDS-4477. Delete txnId in SCMMetadataStoreImpl may drop to 0 after SCM restart. (apache#1828)
  HDDS-4688. Update Hadoop version to 3.2.2 (apache#1795)
  HDDS-4725. Change metrics unit from nanosecond to millisecond (apache#1823)
  ...
jojochuang pushed a commit to jojochuang/ozone that referenced this pull request Mar 11, 2021
Ref.: CDPD-20488

(cherry picked from commit d964cc9)
Change-Id: I6482923ec6f51cb016c81f80afc70fc8b4030436
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants