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

MDM proxy for seamless migrations #19779

Merged
merged 12 commits into from
Jun 25, 2024
Merged

MDM proxy for seamless migrations #19779

merged 12 commits into from
Jun 25, 2024

Conversation

zwass
Copy link
Member

@zwass zwass commented Jun 15, 2024

Implementation for the proxy described in #19387.

@zwass zwass marked this pull request as ready for review June 21, 2024 22:19
@zwass zwass requested a review from a team as a code owner June 21, 2024 22:19
@zwass zwass requested review from roperzh and dherder June 21, 2024 22:19
@zwass
Copy link
Member Author

zwass commented Jun 21, 2024

@dherder @roperzh I marked this ready for review and requested your review as we are now requesting @rfairburn to write some Terraform for it and I'm thinking it will be easier if we have it merged into the repo. It has been tested working but there will inevitably be further changes in the future.

@dherder
Copy link
Contributor

dherder commented Jun 23, 2024

@zwass did you want to take a few devices from our internal micromdm server as a test of this?

@rfairburn
Copy link
Contributor

I added a basic Dockerfile/entrypoint to allow this to run easily in ECS/FARGATE and tested it locally. I'll be building some terraform around it shortly.

-migrate-percentage int
Percentage of clients to migrate from existing MDM to Fleet
-migrate-udids string
Comma-delimited list of UDIDs to migrate always
Copy link
Contributor

@rfairburn rfairburn Jun 24, 2024

Choose a reason for hiding this comment

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

Is comma delimited true here?

robert@Roberts-MacBook-Pro mdmproxy % ./mdmproxy --existing-hostname micromdm.example.com --fleet-url https://micromdm.micromdm.com --migrate-percentage 50  --existing-url https://micromdm.micromdm.com --migrate-udids "a,b,c,d"
2024/06/23 19:07:52 --migrate-udids set: map[a,b,c,d:{}]
2024/06/23 19:07:52 --migrate-percentage set: 50
2024/06/23 19:07:52 --existing-url set: https://micromdm.micromdm.com
2024/06/23 19:07:52 --existing-hostname set: micromdm.example.com
2024/06/23 19:07:52 --fleet-url set: https://micromdm.micromdm.com
2024/06/23 19:07:52 Starting server on :8080
^C
robert@Roberts-MacBook-Pro mdmproxy % ./mdmproxy --existing-hostname micromdm.example.com --fleet-url https://micromdm.micromdm.com --migrate-percentage 50  --existing-url https://micromdm.micromdm.com --migrate-udids "a b c d" 
2024/06/23 19:08:02 --migrate-udids set: map[a:{} b:{} c:{} d:{}]
2024/06/23 19:08:02 --migrate-percentage set: 50
2024/06/23 19:08:02 --existing-url set: https://micromdm.micromdm.com
2024/06/23 19:08:02 --existing-hostname set: micromdm.example.com
2024/06/23 19:08:02 --fleet-url set: https://micromdm.micromdm.com
2024/06/23 19:08:02 Starting server on :8080

The map doesn't appear to do map things unless split by spaces.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, typo! I thought it would be a bit easier to work with if it were newline/space delimited.


func processUDIDs(in io.Reader) (map[string]struct{}, error) {
scanner := bufio.NewScanner(in)
scanner.Split(bufio.ScanWords)
Copy link
Contributor

Choose a reason for hiding this comment

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

ScanWords appears to operate space-delimited: https://pkg.go.dev/bufio#ScanWords

roperzh
roperzh previously approved these changes Jun 24, 2024
Copy link
Member

@roperzh roperzh left a comment

Choose a reason for hiding this comment

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

LGTM! (bearing the issue that Robert brought up)

Comment on lines +45 to +49
if strings.Contains(r.URL.Path, "scep") {
log.Printf("%s %s -> Existing (SCEP)", r.Method, r.URL.String())
m.existingProxy.ServeHTTP(w, r)
return
}
Copy link
Member

Choose a reason for hiding this comment

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

a completely separate discussion, but just so it's in your radar, Ideally all SCEP requests go through Fleet, I think we'll soon know more as part of #19800

Copy link
Member Author

Choose a reason for hiding this comment

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

This will be an easy change, I just didn't know how to test it so left it as-is for now.

Comment on lines 93 to 99
token := r.Header.Get("Authorization")
if token == "" {
http.Error(w, "Authorization header must be provided", http.StatusUnauthorized)
return

}
if token != m.token {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is super important, but might be unexpected for consumers so just throwing the heads up: the expected header here is Authorization: <token>, it doesn't contain the type (eg: Authorization: Bearer <token>)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I think it makes sense to be consistent with Fleet server conventions.


func udidIncludedByPercentage(udid string, percentage int) bool {
index := hashUDID(udid) % 100
return int(index) < percentage
Copy link
Member

Choose a reason for hiding this comment

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

love this because we don't have to keep any important state in the server ✨

@zwass zwass merged commit e4c9712 into main Jun 25, 2024
13 checks passed
@zwass zwass deleted the mdm-proxy branch June 25, 2024 16:31
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.

None yet

4 participants