-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Validate autoupdate_config and autoupdate_agent_rollout #50181
base: master
Are you sure you want to change the base?
Conversation
} | ||
|
||
var maxGroups int | ||
isCloud := modules.GetModules().Features().Cloud |
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.
We may want to relax these restrictions for some Cloud customers on a case-by-case basis. Would it be tricky to add an "unrestricted" feature? (Could be a separate PR)
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.
We will likely want to rely on entitlements. I'm not familiar with this mechanism and will have to ask cloud devs how to do this. If it's not easy, I'll just glue an ugly environment variable so we are not blocked.
f7451c1
to
3a91f1b
Compare
3a91f1b
to
2966633
Compare
return trace.BadParameter("max groups (%d) exceeded for strategy %s, %s schedule contains %d groups", maxGroups, agentsSpec.GetStrategy(), update.AgentsScheduleRegular, len(agentsSpec.GetSchedules().GetRegular())) | ||
} | ||
|
||
if isCloud { |
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.
if isCloud { | |
if !isCloud { | |
return nil | |
} |
nit: cleans up indent a bit
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.
LGTM, seems like you need to adjust unit tests (nil struct in map)
@@ -47,7 +47,8 @@ const ( | |||
) | |||
|
|||
var ( | |||
defaultUpdateDays = []string{"Mon", "Tue", "Wed", "Thu"} | |||
// DefaultUpdateDays is the default list of days when groups can be updated. | |||
DefaultUpdateDays = []string{"Mon", "Tue", "Wed", "Thu"} |
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.
hope its not going to be modified externally
Part of: RFD-184
Goal (internal): https://github.com/gravitational/cloud/issues/10289
This PR removes the restrictions of the
autoupdate_agent_rollout
andautoupdate_config
schedules but adds groups validation.It also adds some optional server-side validation that should not be enforced at the resource level.