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

Allow setting of server default values for member expiry days #2840

Merged

Conversation

TakuyaMatsu
Copy link
Contributor

Description

This PR adds parameters to configure the maximum expiry duration for Members in Athenz ZMS.
The maximum expiry duration can be set individually for Users, Services, and Groups using the following properties:

  • athenz.zms.default_user_expiry_days : Applies the maximum expiry duration for all user members.
  • athenz.zms.default_service_expiry_days : Applies the maximum expiry duration for all service members.
  • athenz.zms.default_group_expiry_days : Applies the maximum expiry duration for all group members.

The default value is 0, which means no expiry is enforced unless explicitly configured.

Contribution Checklist:

  • The pull request does not introduce any breaking changes
  • I have read the contribution guidelines.
  • Create an issue and link to the pull request.

Attach Screenshots (Optional)

@TakuyaMatsu TakuyaMatsu changed the title Feat/server default expiry days Allow setting of server default values for member expiry days Dec 26, 2024
}

public MemberDueDays(Domain domain, Group group) {

// for groups we only have user and service members
// groups cannot include other groups

Integer domainUserDays = domain.getMemberExpiryDays();
Integer domainServiceDays = domain.getServiceExpiryDays();
Integer domainUserDays = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did we remove the domainUserDays and domainServiceDays value?

Copy link
Contributor Author

@TakuyaMatsu TakuyaMatsu Dec 27, 2024

Choose a reason for hiding this comment

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

Sorry, this is a mistake. I have restored it.

# This property specifies the maximum expiry duration in days for user/service/group.
# The value must be an integer, and the default value is 0.
# If set to 0, it indicates that there is no expiry limit.
#athenz.zms.default_user_expiry_days=0
Copy link
Collaborator

@havetisyan havetisyan Dec 30, 2024

Choose a reason for hiding this comment

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

let's rename these three variables to be athenz.zms.default_max_... (add the max_ component). default value indicates that we'll set this if no value is specified but here we're doing 2 things: first if no value is set, we default to this value and second, if a value is specified then we do not allow the value to be bigger than this (which is max part and not necessarily default).


public class MemberDueDays {

private static final int DEFAULT_USER_EXPIRY = Integer.parseInt(System.getProperty(ZMSConsts.ZMS_PROP_DEFAULT_USER_EXPIRY, "0"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here. let's name these DEFAULT_MAX_USER_EXPIRY, etc to correctly indicate the max aspect of the value.

@@ -449,7 +449,7 @@ public static boolean metaValueChanged(Object domainValue, Object metaValue) {
return metaValue != null && !metaValue.equals(domainValue);
}

public static long configuredDueDateMillis(Integer domainDueDateDays, Integer roleDueDateDays) {
public static long configuredDueDateMillis(Integer serverDefaultDueDateDays, Integer domainDueDateDays, Integer roleDueDateDays) {
Copy link
Collaborator

@havetisyan havetisyan Dec 30, 2024

Choose a reason for hiding this comment

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

why do we have serverDefaultDueDateDays (should also be called serverDefaultMaxDueDateDays) set as Integer? It's int so we should define the function as such and it also eliminates the boxing and unnecessary null check later.

@@ -459,6 +459,13 @@ public static long configuredDueDateMillis(Integer domainDueDateDays, Integer ro
} else if (domainDueDateDays != null && domainDueDateDays > 0) {
expiryDays = domainDueDateDays;
}

if (serverDefaultDueDateDays != null && serverDefaultDueDateDays > 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

once serverDefaultMaxDueDateDays is defined as int, the null check is not necessary here

@@ -4820,6 +4820,9 @@ public DomainRoleMembers getOverdueReview(ResourceContext ctx, String domainName
}

Timestamp getMemberDueDate(long cfgDueDateMillis, Timestamp memberDueDate) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

please update the testGetMemberDueDate unit test case to include the cfgDueDateMillis == 0 case.

@TakuyaMatsu TakuyaMatsu force-pushed the feat/server-default-expiry-days branch from 22b0e52 to 5796feb Compare January 6, 2025 06:01
Signed-off-by: Takuya Matsumoto <[email protected]>
@TakuyaMatsu TakuyaMatsu force-pushed the feat/server-default-expiry-days branch from 5796feb to e0f8fa7 Compare January 6, 2025 06:04
@TakuyaMatsu
Copy link
Contributor Author

thank you. Addressed your feedback.

@havetisyan
Copy link
Collaborator

Thanks @TakuyaMatsu

@havetisyan havetisyan merged commit e88a3d5 into AthenZ:master Jan 7, 2025
3 checks passed
@TakuyaMatsu TakuyaMatsu deleted the feat/server-default-expiry-days branch January 7, 2025 02:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants