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.

Sign up for GitHub

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

Jump to bottom

Add policies for "No team" #21972

Merged
merged 13 commits into from
Sep 12, 2024
Merged

Add policies for "No team" #21972

merged 13 commits into from
Sep 12, 2024

Conversation

lucasmrod
Copy link
Member

#21467

  • Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.
    See Changes files for more information.
  • Input data is properly validated, SELECT * is avoided, SQL injection is prevented (using placeholders for values in statements)
  • Added/updated tests
  • If database migrations are included, checked table schema to confirm autoupdate
  • For database migrations:
    • Checked schema for all modified table for columns that will auto-update timestamps during migration.
    • Confirmed that updating the timestamps is acceptable, and will not cause unwanted side effects.
    • Ensured the correct collation is explicitly set for character columns (COLLATE utf8mb4_unicode_ci).
  • Manual QA for all new/changed functionality

@lucasmrod lucasmrod requested a review from a team as a code owner September 10, 2024 23:06
@codecov Codecov
Copy link

codecov bot commented Sep 11, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 87.13826% with 40 lines in your changes missing coverage. Please review.

Project coverage is 65.40%. Comparing base (8e5d056) to head (bac19cf).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
server/datastore/mysql/policies.go 78.57% 10 Missing and 5 partials ⚠️
...tions/tables/20240905200001_AddPoliciesToNoTeam.go 63.63% 7 Missing and 5 partials ⚠️
server/service/client.go 93.38% 5 Missing and 3 partials ⚠️
server/datastore/mysql/vpp.go 60.00% 1 Missing and 1 partial ⚠️
server/service/team_policies.go 77.77% 1 Missing and 1 partial ⚠️
pkg/spec/gitops.go 97.72% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #21972      +/-   ##
==========================================
+ Coverage   64.85%   65.40%   +0.54%     
==========================================
  Files        1493     1493              
  Lines      116579   116332     -247     
  Branches     3537     3487      -50     
==========================================
+ Hits        75611    76085     +474     
+ Misses      33924    33152     -772     
- Partials     7044     7095      +51
Flag Coverage Δ
backend 66.67% <87.13%> (+0.54%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

getvictor
getvictor reviewed Sep 11, 2024
View reviewed changes
Copy link
Member

@getvictor getvictor left a comment

Choose a reason for hiding this comment

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

Looks good. Left a few comments/questions.

cmd/fleetctl/gitops.go Outdated Show resolved Hide resolved
pkg/spec/gitops.go
@@ -366,12 +411,14 @@ func parseAgentOptions(top map[string]json.RawMessage, result *GitOps, baseDir s
func parseControls(top map[string]json.RawMessage, result *GitOps, baseDir string, multiError *multierror.Error) *multierror.Error {
controlsRaw, ok := top["controls"]
if !ok {
return multierror.Append(multiError, errors.New("'controls' is required"))
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we need this error here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because controls can be defined on global file or in no-team.yml, so we allow omitting it. (In cmd/fleetctl we check that it has to be defined in one of these at least.)

server/datastore/mysql/migrations/tables/20240905140515_AddPoliciesToNoTeam.go Outdated
return err
}
if len(constraints) != 1 {
return errors.New("policies foreign key to teams not found")
Copy link
Member

Choose a reason for hiding this comment

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

Why the error here? If there are no constraints, then we don't need to drop the FK, I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

We are assuming the fk is there on all deployments.

server/datastore/mysql/migrations/tables/20240905140515_AddPoliciesToNoTeam.go Outdated
DROP INDEX policy_team_unique,
MODIFY inherited_team_id INT UNSIGNED NULL,
ADD COLUMN inherited_team_id_char char(10) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci
GENERATED ALWAYS AS (IF(inherited_team_id IS NULL, 'global', CONVERT(inherited_team_id, CHAR))),
Copy link
Member

Choose a reason for hiding this comment

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

Why VIRTUAL and not GENERATED ALWAYS AS (...) STORED?

Copy link
Member Author

Choose a reason for hiding this comment

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

Given policies is usually a small table (in the order of ~1k entries?) it doesn't make a difference, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, just wondering if this was intentional. I recommend explicitly specifying VIRTUAL in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's add it to the patterns.md (when time allows).

server/datastore/mysql/policies.go Outdated Show resolved Hide resolved
server/datastore/mysql/policies.go
@@ -12,9 +12,9 @@ import (

"golang.org/x/text/unicode/norm"

"github.com/doug-martin/goqu/v9"
Copy link
Member

Choose a reason for hiding this comment

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

🎉

server/service/integration_mdm_test.go Outdated Show resolved Hide resolved
server/datastore/mysql/migrations/tables/20240905140515_AddPoliciesToNoTeam.go Outdated Show resolved Hide resolved
@lucasmrod lucasmrod requested a review from getvictor September 12, 2024 13:52
getvictor
getvictor previously approved these changes Sep 12, 2024
View reviewed changes
Copy link
Member

@getvictor getvictor left a comment

Choose a reason for hiding this comment

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

LGTM

@lucasmrod lucasmrod requested a review from getvictor September 12, 2024 16:48
getvictor
getvictor approved these changes Sep 12, 2024
View reviewed changes
Copy link
Member

@getvictor getvictor left a comment

Choose a reason for hiding this comment

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

LGTM

@lucasmrod lucasmrod merged commit 4c24729 into main Sep 12, 2024
24 of 25 checks passed
@lucasmrod lucasmrod deleted the 21467-policies-for-no-team branch September 12, 2024 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@getvictor getvictor getvictor approved these changes

Assignees

@getvictor getvictor

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lucasmrod @getvictor