From b140a5af591b9a3d4fdfd75bd8f124d0013b2c25 Mon Sep 17 00:00:00 2001 From: Mike McNeil Date: Mon, 4 Dec 2023 16:13:39 -0600 Subject: [PATCH 1/6] 2023-12-01 Handbook: Github labels (#15399) @lukeheath Could you have a look at the todos in the note? Your call on when to prioritize these changes, but wanted to get the exceptions tracked ASAP to stop the sprawl (i.e. 2 more labels were created this week that don't match the convention-- addressed separately and not listed here) --- handbook/company/communications.md | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/handbook/company/communications.md b/handbook/company/communications.md index aba6a89129..3a9a335bc1 100644 --- a/handbook/company/communications.md +++ b/handbook/company/communications.md @@ -206,16 +206,33 @@ We use these prefixes to organize the Fleet Slack: Fleet uses Github as the [source of truth](https://fleetdm.com/handbook/company/why-this-way#why-do-we-use-one-repo) for our product and documentation; a platform to allow community members to interact with Fleet, [contribute and provide feedback](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/Committing-Changes.md#committing-changes). ### GitHub labels -We use special characters to define different types of GitHub labels. By combining labels, we -organize and categorize GitHub issues. This reduces the total number of labels required while -maintaining an expressive labeling system. For example, instead of a label called -`platform-dev-backend`, we use `#platform :dev ~backend`. +Fleet prefixes all GitHub labels with special characters or words to organize and categorize GitHub issues. -| Special character | Label type | Examples | +| Prefix | Label type | Examples | |:------------------|:------------|:------------------------------------| | `#` | Noun | `#g-marketing`, `#g-ceo`, `#agent` | `:` | Verb | `:dev`, `:research`, `:design` | `~` | Adjective | `~blocked`, `~frontend`, `~backend` +| `customer-` | [Customer request](TODO link to handbook section) | `customer-leo`, `customer-sagittarius` +| `#g-` | Group isssue | _An issue requesting something from a group at Fleet, such that it will be seen and procesed on their kanban board within 1 business day._ + +Opionated conventions help people work faster and spend less time figuring out what to name things, or misunderstanding why they're named what they are. This also reduces the total number of labels required while maintaining an expressive labeling system. For example, instead of a label called `platform-dev-backend`, we use `#platform :dev ~backend`. + +> _**Note:** There are only a few "special" labels that are exceptions to this rule: +> - `bug` +> - `story` +> - `prospect-` _(TODO: This makes sense, but I noticed on Nov 30, 2023 that `customer-` was being used in at least one place `prospect-` was supposed to be. Let's decide what we'll be doing. Easiest thing is probably to switch to `customer-` for everything and then clarify who is a customer in the mapping between labels and actual organization names. Up to Luke and Zay.)_ +> - `customer request` _(FUTURE: is this needed? if so, could use special symbol prefix in front of it)_ +> - `needs prioritization` _(FUTURE: is this needed? could we use a symbol prefix in front of it?)_ +> - `github_actions` _(TODO: why is this here? Instead we can use prefixes, like `~`)_ +> - `docker` _(TODO: why is this here? Instead we can use prefixes, like `~`)_ +> - `go` _(TODO: why is this here? Instead we can use prefixes, like `~`)_ +> - `javascript` _(TODO: why is this here? Instead we can use prefixes, like `~`)_ +> - `Epic` _(TODO: Find a way to remove this. It is an artifact from Zenhub and not something we actually want to exist or use, as it is confusing.)_ +> - `p4` _(TODO: why is this here? Instead we can use prefixes, like `~`. Or better yet, delete it. What is a P4 anyway?)_ +> - `p5` _(TODO: why is this here? Instead we can use prefixes, like `~`. Same as `p4`.)_ +> - `story to demo` _(FUTURE: is this needed? if so, could use special symbol prefix in front of it)_ +> - `bug to demo` _(FUTURE: is this needed? if so, could use special symbol prefix in front of it)_ ### Process new requests Team members [process their department's kanban boards](https://fleetdm.com/handbook/company/why-this-way#why-lean-software-development) daily, prioritizing all new requests including issues and PRs within one business day. From 5cb37d9c62dbe35376e56cdf616188be83ce92bf Mon Sep 17 00:00:00 2001 From: Andrew Baker <89049099+DrewBakerfdm@users.noreply.github.com> Date: Tue, 5 Dec 2023 10:00:27 -0500 Subject: [PATCH 2/6] Drew bakerfdm patch 1 (#15434) --- ...tion-checks-during-software-development.md | 45 ++++++++++++++++++ ...during-software-development-720x179@2x.jpg | Bin 0 -> 14950 bytes 2 files changed, 45 insertions(+) create mode 100644 articles/catch-missed-authorization-checks-during-software-development.md create mode 100644 website/assets/images/articles/catch-missed-authorization-checks-during-software-development-720x179@2x.jpg diff --git a/articles/catch-missed-authorization-checks-during-software-development.md b/articles/catch-missed-authorization-checks-during-software-development.md new file mode 100644 index 0000000000..60700c7e3b --- /dev/null +++ b/articles/catch-missed-authorization-checks-during-software-development.md @@ -0,0 +1,45 @@ +# Catch missed authorization checks during software development + +
+ +
+ +Authorization is giving permission to a user to do an action on the server. As developers, we must ensure that users are only allowed to do what they are authorized. + +One way to ensure that authorization has happened is to loudly flag when it hasn’t. This is how we do it at [Fleet Device Management](https://www.linkedin.com/company/fleetdm/?lipi=urn%3Ali%3Apage%3Ad_flagship3_pulse_read%3BCaXkx0wxSNeQ8WfF5SZ17g%3D%3D). + +In our code base, we use the [go-kit library](https://github.com/go-kit/kit). Most of the general endpoints are created in the handler.go file. For example: +``` +// user-authenticated endpoints +ue := newUserAuthenticatedEndpointer(svc, opts, r, apiVersions...) + +ue.POST("/api/_version_/fleet/trigger", triggerEndpoint, triggerRequest{}) +``` + +Every endpoint calls **kithttp.NewServer** and wraps the endpoint with our **AuthzCheck**. From [handler.go](https://github.com/fleetdm/fleet/blob/36421bd5055d37a4c39a04e0f9bd96ad47951131/server/service/handler.go#L729): +``` +e = authzcheck.NewMiddleware().AuthzCheck()(e) +return kithttp.NewServer(e, decodeFn, encodeResponse, opts...) +``` +![Example check](../website/assets/images/articles/catch-missed-authorization-checks-during-software-development-720x179@2x.jpg +"Example check") + +This means that after the business logic is processed, the AuthzCheck is called. This check ensures that authorization was checked. Otherwise, an error is returned. From [authzcheck.go](https://github.com/fleetdm/fleet/blob/36421bd5055d37a4c39a04e0f9bd96ad47951131/server/service/middleware/authzcheck/authzcheck.go#L51): +``` +// If authorization was not checked, return a response that will +// marshal to a generic error and log that the check was missed. +if !authzctx.Checked() { + // Getting to here means there is an authorization-related bug in our code. + return nil, authz.CheckMissingWithResponse(response) +} +``` + +This additional check is useful during our development and QA process, to ensure that authorization always happens in our business logic. + + + + + + + + diff --git a/website/assets/images/articles/catch-missed-authorization-checks-during-software-development-720x179@2x.jpg b/website/assets/images/articles/catch-missed-authorization-checks-during-software-development-720x179@2x.jpg new file mode 100644 index 0000000000000000000000000000000000000000..a48096345376766d8b9e0efdb3f422e32dc2a5c0 GIT binary patch literal 14950 zcmd6O1ymf{vi1z_3?AGe5Zv80KyY^_xP%ZOxI0XcKnNP#A%lhB4uRkVcPF?L_=n`2 zob&#B@B80BSJ$rUU2{KuzXSk1m64YL0D%Ai5PAUaX91D`SQwax z6Pn2pt0p0~Hm755mI1#lyox!z6fwkNXH4 z7Z3NL6Cf$X77!K-c;5*id4LKFgeJhB z2>}is5eN%|1T7_kvSDEVDuPylfdj(ABizpb(4c%65G)9KqjyDrw*4>Wz%}6>!Q48t zY5tdI`~Fk4utwA9-X`4t>SuRp@Ta}8Q{Blwx`)-XPsr&bV)~1`y#kPNt0%;?lZLdD z7*KZoCR@j|$O=OqBm9euT}UVIK>*JK(Qjn_8{R$%5Et~dqw#xy&@Kky4tWuoHIql3 z$YqZ(!C8+(f0eNb0GQtt)O2RhBE7FnB^2!l+G7E&EMxphR=?>L$#K9c<3X(;VfNLt=Bcmi`>vKuK=92XxKV|X5ja!}FiBH}5h)Tb2 zmHgpr{4fH5@X-~#&Yxqdxp~lM*VGm9$M*9zN!#T{&1D#0Wq8F`6xrd~eXVmANV(ng z!FhjNsuIUP^8EaE(Z@?_W|$>`hXKdz9|+4y4)czCht@pz8tG>7`ZcTVlIJDiogaA0 z-sC7|9d3N6@5M*i6~4yq(4J&3^`sYkJr>-OycNe7EwejbOy6P1caN{b-`NDp9DA$j zzg~VB;xY*kd7&GZ zEJ;qSRXe5)MV?mT&f2!3HstyfxtrsgFJ&sIM)trjn=J8B$7&G8VNz1|tH?aeR9Z8u zG%Tb}^rVkU5M}MHGBd3xL(iQC*T(VyPH%16Z@cGYe&CFrzJcY_Tr-XqVgy=-!)i-f zh4^R*_Ak4)K^5oM8Bv1%Vb{0KSpe=&6aWAR`!PzTr1XsR$>V}Tmo3c=a--CO#kx$# zl2gK=OpIBESih4k-^`c|1Roy-of7>?M?|%IK>9(BTeD^))}{c9!>A{u7-;t;&9K(; zG`!A|wW#i9NEOG>4U<1`=o87JpY%mH9Ye3wrOzseM{9&FMP9yoekdZ)|=77(%RFB)LeEXG>XWsS8!N! z;M8aWbA31=T3|U?U`etZ?hm4%>AClT@B!Fgi3~arrmW*ch3ozv(NU#ccUuK9LX-W! z`WfdzulucFtETj?9{#7lplX&C1OURq!ob79zyRTY_6HAug@ZxB2ZFFD*tmFb@i@3~ z*u_*Rv1#a>{h_)S396TY@G$p)+U*9$m!%gx#=5Axls*=$|1K-oZDQ!| zko@^=|LfeFR-a65!&lFxy|k4N{HC79J-(CbJL=(1iqkcKPipQN*-m+VDNV{m9J-aqEJ)3-cuu>E~95#h%>2dLMm1VW? zZ|b5Guxym;jW6_A<}Jk=Hk6C!aIMwV1aLiN&gNH%-8k`U*zNuk5;mvuRDPXKBEi|L zb?W%+$vq&h;b!6FnhcV9Dj2kPwOIG~@yilzxe%Yku~n;}^tFcN#WyHIIQB_zxq>Vx zI_cT+d0F8oo%qGm9KJJG(O(c-1z8O@_gR7?q$;fW=|X*&tb5pO;vyZGcv6=3Cx(k;BQ!(~ zlU+KD>VJpiD8&eW-a3w1d0m?(!?!~{2b=e~_cV0%IOsT5Kc#+G&zBjpc`E2%@HEra z(Vw=5>%?`nQ7)J3Ghh0~ph8N1H93ZSP(|&=x%JoYm^E{8U_b0{KMrIQop9D6!|I8m z7>b(~Ue!)*XQL4VFU9Ffz-Dr`z1`_!6PYh1b7^-45L$0@lh?oVH024S%(LxNnaB%E z3A@_(&JwLzN&RqqJOlgAnH%5GgG>3T?M_i8R>m2cgZ*62qdJb(C?=#>!?q!WMX?&4 z`1$6ZYzu>y0Ui3H(X8?+YO?(B{ea23R4kVF9t%JZs2 zCJzsIN>QWNS3mTW-Yf>8@lF(#=?bZrmi3pfdxv@1Gmh82({nb`D7JOp+(SFXh)P+! zTJzy058^sW9jgmu8?Uc+d%!h(JU&i8^mEZYK=Q*q;Ke7qNvqo6dOUGqoqj#+Q#G~y zUa+u%5A@MZ&o^=5eg3$uCmwbHJ~d;Cq?28b|G}LcBh%| z*oM*QYnX*%WjXRh*SXI<+~zaRVZsF~vqpZBkhW8GVg_7xgzcgWNIVa2LIOgH?Po3s zX?EY*XU>V3iF}(-Y(baK{hxC869ftkn{TSJLiWfpcWv641T91=!ApUFW;9_Z`cqb) z=(ZO%B(sjeGePJVh*z1kx!4Q!OMJUt5WF1(a7dZ!wiUZu>_myi1Xeb^4cQoegdCn6 z&FaD~$AUxecM4zC0oMysJLRrgsJvTG=O}R1NHg1F;8#&cM&YHEP3FdXEs)(*OG%{^ z)JUAFc1lGlK4ywFx8vOMK>YSywvWeo5Wyg)(Jc0JgN_SiW255Y$*1wm{hV@ziS40OOhiA*S+kO{Nv1v^N6;2J>yXX- ze6E*n#V3tyUuwUo?$1|n+}k{~t53%@l>zF4eusft$@1(O=gKO>{# zMDHAv3+MdLQKUN%nAr3>;7k8CF(VTWaQQX;0=N7QnG2^-6*>b;;)aG_*=F~jkg(ZG z>At%{a_a`2(Npf$z$I~Kih7U%5(^vw3~B)Uzhr2wSx@IeMI5@E6r3;|#~m=#?%0-k zp2dk&ZB7bZ_hjmv?2`GI5C6NYV9e2H_kd16=(@>>1%Ls-!9%^O-|h<#1{Mn&4us30 zhJ(jWLCMA`ChlUQ`g6&IRuTo?1Ar{>#JOCDX|I%o=rVAWkqd&y+Mh4~2SzL&nuc+d zMr2N5G2;XAl$+mjjLeBpY5H^;RkYSXz&JL>>)roHDPyyNY|?JW)k7`YW(6!;W@hPd zh*opzrv=sTz76^Wec{K|5K#-ADR8&WJwVs=w&{ao|LdVd0@Ai4!Jrin#Ln-S5p(1? z^&<4nub(YlzW>Qqh`iJ=zF>Ih#DcLr1aheN7!g(X>2$NAY|`mK2IA`O zAIf~1x^~Pjm@P|fV^f#!3jI=*7LMcGu8F6TFcIzk#x``Q>P1ngJ<=tGA`K!AGKzict6 zLkokY3J1c$?r(ojLV)q%K$5u zuS{Jt@NYkf)ezX=o9)948fN{oA{-0?eXPtCFd`ni=I=uTNr>eL9c!w0j@U}BT0CB6 z$db)F@vB9@bzti(j{k^>EJIEoXy4P`3v@BG9a;3Zqg*7fA|<1*bAE-0x&n+eNH)%l zb)qxkcyJoE;Rfy&`UJiQP%+384CW0Cn12mx_7FS}etqu4zN2?#7>cpng&)3B8QCsW zUhTsBDl~l8O$BM_EO@5f!IY=+osLo|-6W13GUS25kJ0H$pmR!j@P6oKSQC&6?ns@h z>_^uq%wj`+S9)ze4mlQ6k}?P}ld4_ScX?NuuCzd%BQS4$TMuJL0Jixz{XI=jo&@A-sqO;DhAu+(I91-yqoz) zM^IE(d(a!AAY+t>{C$K2_zaAxQxm$mr{U+p_IQv_A2H*N9t_9X@k3w7-vsLU_3Ez8H5ATuG! zpH!-+eiykxP+4APvweGB1-u)83as*jwGV_rLP3N>K!!nt$}IE>4uFLXk3-4M{UUJ` zmx78zToqhhGX`Qab$OFCLai21g~ur-kFnko7_@wN^oKWqAPV&c zK3Q}wH58a59U0q(U}=_Xm3!oDKK)vAV{&3AF}xr2r3_WH7L(^A)7`Yt%>O@ZdkOJ3 zpmO5K2_9xuRXi-ua^t4V#eyl?R|h|8@e8P%F%QOe!A&05yIN(oxo^sZUyweNeNFyx zl_uTgTiU2Ve_#9jzCAUg$CG!w>&LwRO`5D0AaWYMhT|8NmKaPtviDNMO>5kU`a_>< zrILoUhV3VT=~NsR_wpBI!8b`6TJ1jl!%s{_)~%B&GhJ)?r5GTS(|}R>wtL48{N8R729tVqB=Q*Q^|_UZdjBuPjtvbq)|=# z>k$m7iXIg&C^ax`n#^j;lfY)z-& zfO~k;YtOW>F+kC6e6|E!ZTd2&-Mkg~k(V9HBZF%^5}(e>w;SIZEvh4`CoCd@g0$Zc z5zp}&glp5T=@T&WG>rbgXY2nlt5WZk6)os zDHRqOr__inarL*+bv^A?lU#{6ghy-WP3fO8q)Lf{dV6z8u#sKLs2ZL|Yi!lfJ?-eF z3u$c^Yn&?Cb0drXl-A$c?l3!ofsH+}WoudeVVH@pdgx9pX;ia5`^UIKW_L&kB?X^) zVBj0vSpACa-d)Z-3`dZb6iBa(L=4N&wnAYlzeP$<1kd%mud>KY5}%F;K90o)hvoj> zEk|qsD z(z99$v;5yPvtQ}XPkQJ=X9s?5eZ6FBeGnEC;NEqJx_c;+PqyM^SSow;tJC@L|C*U?yAP?^cFCQZ`QaSh z5`vuSkwWGPZ+i?JK0oO~$oIW`HBDe(3}|kF-7)=6pEuL#N1Cq4Y)gOM<14S@(rRWl z-Ijp(HO?Rji_g4((kPl@lB0UPr76mKQznT_U1FR%jiPWw)n1=pAtMh-4V##Hx~sm^ ztKnR);hK?)ry)D}1VqEZGp=alE_H_&97mDP&=EexKIJ&uZ*UKYk@i4)0ihnrLqyP` zcG#A#$(`*~GLwzHqde;~5`=mlViyB9EO?Xm05A*9gN`JTPg~0*yi3Q;!RJe-rr7`F zO*N+X`q6a9JphMnO7~I=^!8f}whW~sChkO2%x;!H!cxrAK0jHEEQ`Sx1zQ?KT!(n; zmg@oio0#pI^8D}vdE4Gw3T3V7hP^8L^GOt4j!{W1eO6xl2zPsfh5qD?A)U9?PZ03C zwR1~%^+g%s!Z`6aYbTu4uEl@K8~hYOD{H>Em?aRaL($wVufkTQS4v1ASxo|)n#W~Ad%naW90Q$>`Ue_+QeaS?{(n>Z+5Po@SyICpjG-8#)0 z#hA#s2hIn#jA&B2+jT-oBBiY;Nt5#c6`qUq4AI|REA9cf@Cm^Nsv!%AoECB)Ed}+~ZmdtWe!|nULKq(| zqYzb))-E=JAlrHC7WPNkGBt=m1;YLB0s*5C|F}a3OccvG65()oHu?79=Kg3+fGVTS zXe4cS!22r6k+ES z9R?;PgvZ&rXz5!U=_Rz$JDa@kzhIa3>S6IN8BxUMAFfYv5_Zr$4N@0gjuTa;OEG*s+0nn#g)8`2|E^1(I~ zy|NhIv}Umi-JC?p`Y9YY!b#Y@J+YnP8nSYvT`#pxnNeZyqEp&COYNrcdaKByY+03; zb)$~?IC_h6S@0e45T8qXmd-H0mTxy+lwnnh#TA5Pq7`^xgyXt!zUFSF7 z6S=Ono3iY;cCRcH`noAI2Z=Rzzdt3^cbI~ZBwwrRB&@cK&QaKuGS#v_Y8Pg146j|g zLK7cq%pcB7UEVi}?w`4qM2j6&`KC*vkQio%_zpLF6@YsN(H0r2;LB)Ae@ zLLB@#J1N}fj|K#uz)Qr^B%oqf*rY3zF(>P$6qHxy61q|}5AVs??GS5q8CyMVBO$CW z8Fnv|-aIB?y$5*hqRceBrxcyH!I~$m?&OR17L%BBG+sC#FJx5!Sy4psM6%rH~D3Zk*{9IVjmtf>qXVNO;lJsfw*Uv~2Im1xJDRrFNF?I{T{Fd@zji! z@kxPIT{vIJ@?UaB!mY0>9oT=y^N(6TnxW0|gQX10Y)6ZubSa}JrrTksb3?6e->3I= zK1oC-t)TagqG2Lh+QnyU+dD8mAkWIiCq-q+bLxh9A(z@5J0x9=$1WPQ_07!do}ZximV7^xTG-- zT<5H5^*cJ^&&bmm(v#iFf$zOZJb1V_AbQXqN% zT#iP$M(UHtH_4cY8_Kc`+I2|t`mhwJJq2~Rj`<$o)35`~LD`jBZ!SL5$KKhf4ZrA- zR&xU%PnS$t+1bd6`xY^or>)D$vHzn?l&1{i$W1i!A=?kj@+<^6#O!u9X&*#?pxIxcQdM|6|-6!lSUg+-SMT9#c8tm;B2vJq- zq-YMgHcYzfeT=U%o_KTjzT}_)H)p+p$rZ9QyKuw#HS{oV#J7BBUh5|52?5Eg+TsPH zciLak6@MHaRmzLHzu~x}qW0rfHjofZ>uiDqcJ9SmsB(KVY`?4`@3F4FV z&f#^u2@y=ObG1-RwH?vnaGs5WDcErme5Yy-!O{^==+79h4vtMFMcdTTktT{6ykLDg zUIz@2@Qv@Jr|Z|HQ2@fA35Q-^0< z3q+Dr6)t-Dqr8T7RYj#6IgFy0cUq9eVrxM9kq%vY%_3=(2RL)XsPWZW0vOh^j8os; zVW(y+#Jy!LD&nVif^F46-|biaQx?0T*R_l`K66&}C6(Xe*VA;0JyYD6QM0>m_UE(Gimt93ADR~S zGWR9oH(iP7BXq62?eit=H)^Sws`f2IKTSB)svfCHZEXzk!?w8n;I|c|>{kfFXOO0> zg)DAdjD`7-Z6?U=7MVL3eb^~-&FKe?N)PCX>!&j97WU{;%a2Uhdtt^Gwlo$wD8C6N zsYVhv(1;yi;pRB@BY^}?&r$SURZ654g3npC8lq+Rc^L z2ca;c3pd{bp8cchsV7RkSzcXP^S8G0Mw+#wM|G;IQH)qDf3f&h?!QZQq9{f7BH*6* z1*LzCHdl>yl(JK&av>#B&|GoNDW*i@~$KT$gSvq7EOflhwpK7Enn3N=6X%%5J9 zWGEI*#8LM^hc|W)b>>BSrsor>r^&SM>oW4zG;6n2x(~l7=rfTS4AL=sL)wA_8 zA=7+b^uym%Gr!;*xUQ9!IgOa~{|{R!S$@Z)&^UG+uMMCRf2TdASs!lnD>iLiFrY_U8aN7*%pAqbh<%aQco^ zCSV7SN6A&2&EZg&nwq;AUHtZ!$ysG$h`4M~t?ccZcgQoP{EWv;EUvMga0) zW&MQ3gAmXjv6-|%v&SZMsl{?Y(6^~U5ukhxPU8hEz$&WV_5dw288sto$in-r*(inC zS2Zc^&`Bc~?jN(^C_HNz6)YZO8*wpm4mzHFu4&%zOa5Zc%!}i=3b5Yjap$?4@w8N5 zQQOA_Iw?rvO<_tS4-E{S=p~oR#D8R@qw+_A1j6B%Z<2M*SL*HwX|s~~s!6Uan7}@U zi;01z?7aS+ui^tm=x*iV;$Gy@b$}<0fbO_rx2{mg;#F!pkDe z?-G8bM6nGv(p(TL3Z)F*l7^>x)!Im;gCOQV!aZc;3BEX1=&4m7v6<^cfl2!oxF?$y zP&tjj%o`pNp7(}UoI;L}8dx4chmAxJzWoq37HPUqMb74DcXSfeCMO(0@@yvzVtpl+ zr~E-Q2Dj}V;Qy()sQ4(IN9oP6m+@ntk;kG18%d%Dp!^S6F{+DoqjL?e>jNn)2EVh_ zG%k*jB-(EkeorOG87BY&MppbC$(oy^mCi!JD#EAt0GNkvKPIemx(2dT9P%|9*D4C< zoJxJ|r+)_Xn#7^%8yU^TmknPETxAp@`19UbY3g{}x+Ih(o#5aX{u9#$x*seoUYH4b zavdG_XZog9?6SQ&r4g10$#x=g(UuXy=5p5i+WZZG2C6dmXyfMl)XLL>zgr3Wv}(wW z>b+#0WHt|>$P#Om$r=~Bfo7I>2%=ETRLsT-Ui*4Y@;0dRP@wae_~I&>JncIQAh>zJ zf4V>U#gkW4B+Frvm=@sx_c}*B1!1Lt3x*1BE4!E|?0L{^VH1y;mU&dPyESzay#lMY z3EmpYjVY_MStu4l=ya`u*2mBOp7EV1SiKDBIGTO;acWZz+VA&_-ipd`M3RN^|G!Ls77~=a0fKt{*#z zC3QFU$aS{CA6tvu%(qfIVVZ6oOI&QZ)qOC-?ZT&m;_+46Q^xU8JUl zR15cjiMf{aBZwKZP57Q{^AQ$ZELK2+322A7Q&nM_Io|h@TonXj`Q5@|$k)(EfS z!jjlVjRki@;~;{ALB?BqjG+SrY7RRTB(CRq($M1?Tkwh++OkBHS!y77x72flYt$Y7<#8K0y=(!#q3=x71UO%j`P>L4Md zNH4hUSz87~37+0Y)RnO8}o9O#ABR1M5LmYO~XO4$UTGHbL z3>>tHY(PO!ZmyBIwM2o@;3G+1&YpI^Kq58RJT4r}vdWOf{J7JD+7UEs;MJlU<{KQ= zYLBfWC!MQqdC>-UsluBj&gd${kI|Tjmx#sEoE>yLRKb7k*AVsYF^U)KOFXC_n@+-XTq{sv&&|~KZMHgX0``*APqR1E?5s5rvC`YrD3i^n5cNbiAnZwGED82W>;&mvc1$x}8*{DXn_xZU2AKCrI zbHri}o{1HI>`$@MZSd&E{~9Xz4VNYTC=zBZVXo-;f!@$OeT@=^(Pp!@!Xmhz>{Xkj znN3EU`_2y;(raw81OpUXFGn?{dw{2rgP4C7Og{7cm-6WsC`Mhh0?prw!?#w&zozec zFZjX}#%F+dJQo~pu&akD#=hkvSRstqoX#74cZ^B#m-GR<1gW8>egBYBry) z97dJ0!K+FgixNl-P^LWTs4=<+5Fzzt0x*Ah?AaL!#!SZ*g$%*_X7z#Sn)psJ?A2ULG%t+fJ7%PPDeh7> zH+jUyyVa5N?ACdc|t7h=YrfqWW|iwaf|STWCg%A&}QdK4nEF7oR@H z^~L6$=u6yZY3&+0#Q{y9Pj9!xbrO1>;ieZnSJYGz^r(0}c~A+|h*k03>xHRLt1Y^T z>p6Smu_fojmyew+n+y0|Et3Ty@Pz6tO z&!}49iMdSw7k}=7Qu$~@2|kwI+#Wt|9d-v(y{6OGem`HzaTL==>kqJ_M&xc-d@ZMk zm?==B}QS@_HBcv##+(BQ_nb)^{vy|07L zBy`|sg+)tGBQPU8{S`mJ;AzYH@%_zCv8zWjqXFWr9Nl`Jd`m9eid>ES!BAuMCoc;`9 z2RuX#fKG;m{;e|xjjiL4lgI6kwtNpg#0VWcT=E{0`<%1285x~jEcz7m{Lwk*LlN3> zd>|4dTgpYycGP!1Ap(=QCU{bDAqL1>u3v*=%pla3jiBDQC6Agz;O zJ`*XXu^>uhI6ycEZTKq?jw1At1Q1?X(Bwl_BwU+5bcg|=lYXHOPZ3$E0Egsaa9Jpr zWLY$LLiv%%2Ke6@oP;_aU_5$&0eFA`Af&vC?|`e+IEKOi`~~A=;1)Xk5j14x6d(=< z5(5InIA>muJv9EmPbSIV^Fj7cEBs@V{|X0ccava2x5pp;q3|oV<~B{0GNDUI%&s!1T#?>tM|pKM#-vqA4M7_f@@4( zSV>76{)T*2;|5g~j*T=Xg0-moPKN0)@{!I_AW4GvWW8#o&mS&?VuGWVO%=} z0I2%?A%Xk>QUO$ONFWNa02pXS@|hWm_3`g`q~Bnn5xmQX6qM|yRcr~_&{(84Rg+&^ z_74##pdUa(^{>Vn%|$+H*Li0O&~G}d#l{+qe_R%4EHBh!%Tox+Vj_RPbE$kO*Nba> z4~Q08h?J*^D9_b>N7q+v|Bi0;Ru&V%8lkRT#y0^R?}4(>Iyh$W4Qh5;ENbqC#MmM! zJtFUPUfTMR-*F&mdXZgu^L5&1%U$eta@@Dy5SM+#of0e)fWR< z@IGOK+^d<7fmiapc|I2w2FX1>nvs^t;1{75v3HMx?Rn~yZ%z;#sph*}1FYmYrH zc0x2YmT5tCXqQkfM*b=3@|l4~XreE@C6%CdnaR9@59 zQt|fyqrG}DUJN0i_# zB7hD!65L$X{p>9@A)u)Wqt9MvyMN1!q_z1FmOBIN+O5IqU(xC;knRCUEa~3xDa5y6 zROaCu1!SpuhGZuRgf-9GX~43_Sak%DGhfhUDi#_*|t+3F1p|Qm~ z=M|GecbnPKA?s{bt8tFE63XJ*5EB)J&l2-1WXj)pl}V5@M6JB9^@$b>Hl!=7ATL(7 zAnxY^yZl-wQK8SWWiPMhhUga?o}ay)ufOJ;ZcdA|8fS?(rR)T$69@Vt4?(kST(Gmi`%AknnP z_Ic+d;7BU*7Dh3CkK*Hyyl!4`bgL!tl1rS(b6F(t0q$F!N@*1!THie&Bq|(EvnY~Y zv3Y42GdBg<=JN0m|MNOp_mGN!Qm~ihocbL6m4gtH)WorDK!$#na%BNKaaN;?z0rJ? zs~As+=DX#u_EI;M?S#!3QB54r&PQ50O_B@CM7P?S*`^HgHpXcr>zs-FphH!JOZ6SH zUa^)~6w#{T z_jt<|jk=~;lzAWwc*xcy+Gk7911>Jn)t2J;b-Li;>O#%$LNNE zdpt&YE)H|j^`oECbAxBYa6*OyFORxiPVefk>uB(vjipgS!l=QQE)n@lSsFvBOY&c6 zD9mkh?czE3$f^#7{N?BnlwnB6BBavLE}cq-YMdHS?*V+~ZR^C)ckR&|cJr2saWogf7J~F zG|`{#SE}VJWRkt1s;GoDp@MX6(UQ>3@g2f9x-j%!bWVuopMT`_?zG;&@C*1gVhS&o zdNUGNAIpw|38u5_Aq`_lQCx6(RmXSH;ZlsCbl*V0GgK<0o0{uXBHmiTKH;d9cfr~{ zQ`m?mm{qm}IrJnzr1RjW#c9OGAxCj!J*9e=B~%Y}wpF|HdRUt!#Gbnam_JQ|B1(B`1cvMAtBZ8y~w} jYuoSw@70pbnSWuiTJ(nWZur$deIKlF4+!?V|Mq_Xo?Qkr literal 0 HcmV?d00001 From 86febbfdd2b35d84a7842b7e3abe44e240983461 Mon Sep 17 00:00:00 2001 From: Noah Talerman <47070608+noahtalerman@users.noreply.github.com> Date: Tue, 5 Dec 2023 10:40:06 -0500 Subject: [PATCH 3/6] Update Windows MDM setup guide (#15448) - Make reuses SCEP certificate and key obvious --- articles/windows-mdm-setup.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/articles/windows-mdm-setup.md b/articles/windows-mdm-setup.md index 1a24cf17f3..fdf5257d8b 100644 --- a/articles/windows-mdm-setup.md +++ b/articles/windows-mdm-setup.md @@ -28,7 +28,7 @@ This section will show you how to: ### Step 1: generate your certificate and key -If you're already using Fleet's macOS MDM features, you already have a certificate and key. These are your SCEP certificate and SCEP private key you used when turning on macOS MDM. +> If you're already using Fleet's macOS MDM features, you already have a SCEP certificate and key. Skip to step 2 and reuse the SCEP certificate and key as your WSTEP certificate and key. If you're not using macOS MDM features, run the following command to download three files and send an email to you with an attached CSR file. From a59b609f6f24e3026659ecc83783dfa98b820d09 Mon Sep 17 00:00:00 2001 From: George Karr Date: Tue, 5 Dec 2023 11:54:36 -0600 Subject: [PATCH 4/6] Fix copy for OS settings (#15453) remedy #15412 --- frontend/pages/ManageControlsPage/OSSettings/OSSettings.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/pages/ManageControlsPage/OSSettings/OSSettings.tsx b/frontend/pages/ManageControlsPage/OSSettings/OSSettings.tsx index 20974fd842..65caa724e8 100644 --- a/frontend/pages/ManageControlsPage/OSSettings/OSSettings.tsx +++ b/frontend/pages/ManageControlsPage/OSSettings/OSSettings.tsx @@ -69,7 +69,7 @@ const OSSettings = ({ return (

- Remotely enforce settings on macOS hosts assigned to this team. + Remotely enforce OS settings on hosts assigned to this team.

Date: Tue, 5 Dec 2023 18:24:58 -0300 Subject: [PATCH 5/6] Fix deadlock when replacing (upserting) `host_batteries` (#15447) #14779 This PR fixes the deadlock when upserting to `host_batteries`. Which probably happens because InnoDB uses row-locking. I was able to reproduce in main with the new test `TestHosts/ReplaceHostBatteriesDeadlock`. I refactored `ds.ReplaceHostBatteries` to use the same upsert pattern as `ds.ReplaceHostDeviceMapping` (given `battery` is assumed to return just a few rows per host). With such pattern the tests does not fail with deadlock errors anymore. Here are some of the techniques MySQL recommends: https://dev.mysql.com/doc/refman/5.7/en/innodb-deadlocks-handling.html Basically by changing the upsert pattern the deadlock goes away (It's hard to know exactly why the original code deadlocks). Here's the deadlock trace from load test performed in October: ``` 2023-10-26T17:19:17.244707Z 0 [Note] [MY-012468] [InnoDB] Transactions deadlock detected, dumping detailed information. (lock0lock.cc:6482) 2023-10-26T17:19:17.244756Z 0 [Note] [MY-012469] [InnoDB] *** (1) TRANSACTION: (lock0lock.cc:6496) TRANSACTION 3069771944, ACTIVE 0 sec inserting mysql tables in use 1, locked 1 LOCK WAIT 7 lock struct(s), heap size 1136, 5 row lock(s), undo log entries 1 MySQL thread id 75, OS thread handle 70369297350384, query id 658 10.12.3.201 fleet update INSERT INTO host_batteries ( host_id, serial_number, cycle_count, health ) VALUES (27472, '0000', 505, 'Good'),(27472, '0001', 730, 'Good') ON DUPLICATE KEY UPDATE cycle_count = VALUES(cycle_count), health = VALUES(health), updated_at = CURRENT_TIMESTAMP 2023-10-26T17:19:17.244800Z 0 [Note] [MY-012469] [InnoDB] *** (1) HOLDS THE LOCK(S): (lock0lock.cc:6496) RECORD LOCKS space id 867 page no 320 n bits 280 index PRIMARY of table `fleet`.`host_batteries` trx id 3069771944 lock_mode X locks gap before rec Record lock, heap no 205 PHYSICAL RECORD: n_fields 9; compact format; info bits 0 0: len 4; hex 00526996; asc Ri ;; 1: len 6; hex 0000b6f900d0; asc ;; 2: len 7; hex 82000033370110; asc 37 ;; 3: len 4; hex 0000d829; asc );; 4: len 4; hex 30303030; asc 0000;; 5: len 4; hex 8000065b; asc [;; 6: len 4; hex 506f6f72; asc Poor;; 7: len 4; hex 653a9f95; asc e: ;; 8: len 4; hex 653a9f95; asc e: ;; 2023-10-26T17:19:17.245027Z 0 [Note] [MY-012469] [InnoDB] *** (1) WAITING FOR THIS LOCK TO BE GRANTED: (lock0lock.cc:6496) RECORD LOCKS space id 867 page no 320 n bits 280 index PRIMARY of table `fleet`.`host_batteries` trx id 3069771944 lock_mode X locks gap before rec insert intention waiting Record lock, heap no 205 PHYSICAL RECORD: n_fields 9; compact format; info bits 0 0: len 4; hex 00526996; asc Ri ;; 1: len 6; hex 0000b6f900d0; asc ;; 2: len 7; hex 82000033370110; asc 37 ;; 3: len 4; hex 0000d829; asc );; 4: len 4; hex 30303030; asc 0000;; 5: len 4; hex 8000065b; asc [;; 6: len 4; hex 506f6f72; asc Poor;; 7: len 4; hex 653a9f95; asc e: ;; 2023-10-26T17:19:17.245239Z 0 [Note] [MY-012469] [InnoDB] *** (2) TRANSACTION: (lock0lock.cc:6496) TRANSACTION 3069771958, ACTIVE 0 sec inserting mysql tables in use 1, locked 1 LOCK WAIT 7 lock struct(s), heap size 1136, 5 row lock(s), undo log entries 1 MySQL thread id 9, OS thread handle 70369296809712, query id 708 10.12.2.156 fleet update INSERT INTO host_batteries ( host_id, serial_number, cycle_count, health ) VALUES (59161, '0000', 1384, 'Fair'),(59161, '0001', 396, 'Good') ON DUPLICATE KEY UPDATE cycle_count = VALUES(cycle_count), health = VALUES(health), updated_at = CURRENT_TIMESTAMP 2023-10-26T17:19:17.245272Z 0 [Note] [MY-012469] [InnoDB] *** (2) HOLDS THE LOCK(S): (lock0lock.cc:6496) RECORD LOCKS space id 867 page no 320 n bits 280 index PRIMARY of table `fleet`.`host_batteries` trx id 3069771958 lock_mode X locks gap before rec Record lock, heap no 205 PHYSICAL RECORD: n_fields 9; compact format; info bits 0 0: len 4; hex 00526996; asc Ri ;; 1: len 6; hex 0000b6f900d0; asc ;; 2: len 7; hex 82000033370110; asc 37 ;; 3: len 4; hex 0000d829; asc );; 4: len 4; hex 30303030; asc 0000;; 5: len 4; hex 8000065b; asc [;; 6: len 4; hex 506f6f72; asc Poor;; 7: len 4; hex 653a9f95; asc e: ;; 8: len 4; hex 653a9f95; asc e: ;; 2023-10-26T17:19:17.245504Z 0 [Note] [MY-012469] [InnoDB] *** (2) WAITING FOR THIS LOCK TO BE GRANTED: (lock0lock.cc:6496) RECORD LOCKS space id 867 page no 320 n bits 280 index PRIMARY of table `fleet`.`host_batteries` trx id 3069771958 lock_mode X locks gap before rec insert intention waiting Record lock, heap no 205 PHYSICAL RECORD: n_fields 9; compact format; info bits 0 0: len 4; hex 00526996; asc Ri ;; 1: len 6; hex 0000b6f900d0; asc ;; 2: len 7; hex 82000033370110; asc 37 ;; 3: len 4; hex 0000d829; asc );; 4: len 4; hex 30303030; asc 0000;; 5: len 4; hex 8000065b; asc [;; 6: len 4; hex 506f6f72; asc Poor;; 7: len 4; hex 653a9f95; asc e: ;; 8: len 4; hex 653a9f95; asc e: ;; 2023-10-26T17:19:17.245730Z 0 [Note] [MY-012469] [InnoDB] *** WE ROLL BACK TRANSACTION (2) (lock0lock.cc:6496) ``` - [X] Changes file added for user-visible changes in `changes/` or `orbit/changes/`. See [Changes files](https://fleetdm.com/docs/contributing/committing-changes#changes-files) for more information. - [X] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements) - [X] Added/updated tests - [X] Manual QA for all new/changed functionality --- changes/14779-fix-host_batteries-deadlock | 1 + server/datastore/mysql/hosts.go | 116 ++++++++++++++-------- server/datastore/mysql/hosts_test.go | 67 +++++++++++++ server/fleet/hosts.go | 1 + 4 files changed, 142 insertions(+), 43 deletions(-) create mode 100644 changes/14779-fix-host_batteries-deadlock diff --git a/changes/14779-fix-host_batteries-deadlock b/changes/14779-fix-host_batteries-deadlock new file mode 100644 index 0000000000..d70a3eedbc --- /dev/null +++ b/changes/14779-fix-host_batteries-deadlock @@ -0,0 +1 @@ +* Fix possible deadlocks when upserting to `host_batteries` (found during load test). diff --git a/server/datastore/mysql/hosts.go b/server/datastore/mysql/hosts.go index 71a4ac1144..df82354692 100644 --- a/server/datastore/mysql/hosts.go +++ b/server/datastore/mysql/hosts.go @@ -2912,65 +2912,95 @@ func (ds *Datastore) ReplaceHostDeviceMapping(ctx context.Context, hid uint, map } func (ds *Datastore) ReplaceHostBatteries(ctx context.Context, hid uint, mappings []*fleet.HostBattery) error { + for _, m := range mappings { + if hid != m.HostID { + return ctxerr.Errorf(ctx, "host batteries mapping are not all for the provided host id %d, found %d", hid, m.HostID) + } + } + + // The following SQL statements assume a small number of batteries reported per host. + // This is using the same pattern as ReplaceHostDeviceMapping. const ( - replaceStmt = ` - INSERT INTO - host_batteries ( + selStmt = ` + SELECT + id, host_id, serial_number, - cycle_count, - health - ) - VALUES - %s - ON DUPLICATE KEY UPDATE - cycle_count = VALUES(cycle_count), - health = VALUES(health), - updated_at = CURRENT_TIMESTAMP -` - valuesPart = `(?, ?, ?, ?),` + cycle_count, + health + FROM + host_batteries + WHERE + host_id = ?` - deleteExceptStmt = ` - DELETE FROM - host_batteries - WHERE - host_id = ? AND - serial_number NOT IN (?) -` - deleteAllStmt = ` - DELETE FROM - host_batteries - WHERE - host_id = ? -` + delStmt = ` + DELETE FROM + host_batteries + WHERE + id IN (?)` + + insStmt = ` + INSERT INTO + host_batteries (host_id, serial_number, cycle_count, health) + VALUES` + insPart = ` (?, ?, ?, ?),` ) - replaceArgs := make([]interface{}, 0, len(mappings)*4) - deleteNotIn := make([]string, 0, len(mappings)) - for _, hb := range mappings { - deleteNotIn = append(deleteNotIn, hb.SerialNumber) - replaceArgs = append(replaceArgs, hid, hb.SerialNumber, hb.CycleCount, hb.Health) + keyFn := func(b *fleet.HostBattery) string { + return b.SerialNumber + ":" + fmt.Sprint(b.CycleCount) + ":" + b.Health } return ds.withRetryTxx(ctx, func(tx sqlx.ExtContext) error { - // first, insert the new batteries or update the existing ones - if len(replaceArgs) > 0 { - if _, err := tx.ExecContext(ctx, fmt.Sprintf(replaceStmt, strings.TrimSuffix(strings.Repeat(valuesPart, len(mappings)), ",")), replaceArgs...); err != nil { - return ctxerr.Wrap(ctx, err, "upsert host batteries") + // Index the mappings by serial_number, to quickly check which ones + // need to be deleted and inserted. + toIns := make(map[string]*fleet.HostBattery) + serials := make(map[string]struct{}) + for _, m := range mappings { + if _, ok := serials[m.SerialNumber]; ok { + // Ignore multiple rows with the same serial number + // (e.g. in case of bugs in results reported by osquery). + continue + } + toIns[keyFn(m)] = m + serials[m.SerialNumber] = struct{}{} + } + + var prevMappings []*fleet.HostBattery + if err := sqlx.SelectContext(ctx, tx, &prevMappings, selStmt, hid); err != nil { + return ctxerr.Wrap(ctx, err, "select previous host batteries") + } + + var delIDs []uint + for _, pm := range prevMappings { + key := keyFn(pm) + if _, ok := toIns[key]; ok { + // already exists, no need to insert + delete(toIns, key) + } else { + // does not exist anymore, must be deleted + delIDs = append(delIDs, pm.ID) } } - // then, delete the old ones - if len(deleteNotIn) > 0 { - delStmt, args, err := sqlx.In(deleteExceptStmt, hid, deleteNotIn) + if len(delIDs) > 0 { + stmt, args, err := sqlx.In(delStmt, delIDs) if err != nil { - return ctxerr.Wrap(ctx, err, "generating host batteries delete NOT IN statement") + return ctxerr.Wrap(ctx, err, "prepare delete statement") } - if _, err := tx.ExecContext(ctx, delStmt, args...); err != nil { + if _, err := tx.ExecContext(ctx, stmt, args...); err != nil { return ctxerr.Wrap(ctx, err, "delete host batteries") } - } else if _, err := tx.ExecContext(ctx, deleteAllStmt, hid); err != nil { - return ctxerr.Wrap(ctx, err, "delete all host batteries") + } + + if len(toIns) > 0 { + var args []interface{} + for _, m := range toIns { + args = append(args, hid, m.SerialNumber, m.CycleCount, m.Health) + } + stmt := insStmt + strings.TrimSuffix(strings.Repeat(insPart, len(toIns)), ",") + if _, err := tx.ExecContext(ctx, stmt, args...); err != nil { + return ctxerr.Wrap(ctx, err, "insert host batteries") + } } return nil }) diff --git a/server/datastore/mysql/hosts_test.go b/server/datastore/mysql/hosts_test.go index 36ebae62b4..710ffbe802 100644 --- a/server/datastore/mysql/hosts_test.go +++ b/server/datastore/mysql/hosts_test.go @@ -29,6 +29,7 @@ import ( "github.com/micromdm/nanodep/godep" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/sync/errgroup" ) var expLastExec = func() time.Time { @@ -134,6 +135,7 @@ func TestHosts(t *testing.T) { {"DeleteHosts", testHostsDeleteHosts}, {"HostIDsByOSVersion", testHostIDsByOSVersion}, {"ReplaceHostBatteries", testHostsReplaceHostBatteries}, + {"ReplaceHostBatteriesDeadlock", testHostsReplaceHostBatteriesDeadlock}, {"CountHostsNotResponding", testCountHostsNotResponding}, {"FailingPoliciesCount", testFailingPoliciesCount}, {"HostRecordNoPolicies", testHostsRecordNoPolicies}, @@ -6000,6 +6002,31 @@ func testHostsReplaceHostBatteries(t *testing.T, ds *Datastore) { require.NoError(t, err) require.ElementsMatch(t, h1Bat, bat1) + type timestamp struct { + CreatedAt time.Time `db:"created_at"` + UpdatedAt time.Time `db:"updated_at"` + } + var timestamps1 []timestamp + ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + return sqlx.SelectContext(ctx, q, ×tamps1, `SELECT created_at, updated_at FROM host_batteries WHERE host_id = ?`, h1.ID) + }) + + // Insert the same battery data again. + err = ds.ReplaceHostBatteries(ctx, h1.ID, h1Bat) + require.NoError(t, err) + + var timestamps2 []timestamp + ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + return sqlx.SelectContext(ctx, q, ×tamps2, `SELECT created_at, updated_at FROM host_batteries WHERE host_id = ?`, h1.ID) + }) + + // Verify that there were no inserts/updates (because reported data hasn't changed). + require.ElementsMatch(t, timestamps1, timestamps2) + + bat1, err = ds.ListHostBatteries(ctx, h1.ID) + require.NoError(t, err) + require.ElementsMatch(t, h1Bat, bat1) + bat2, err := ds.ListHostBatteries(ctx, h2.ID) require.NoError(t, err) require.Len(t, bat2, 0) @@ -6045,6 +6072,46 @@ func testHostsReplaceHostBatteries(t *testing.T, ds *Datastore) { require.ElementsMatch(t, h2Bat, bat2) } +func testHostsReplaceHostBatteriesDeadlock(t *testing.T, ds *Datastore) { + ctx := context.Background() + var hosts []*fleet.Host + for i := 1; i <= 100; i++ { + h, err := ds.NewHost(ctx, &fleet.Host{ + ID: uint(i), + OsqueryHostID: ptr.String(fmt.Sprintf("id-%d", i)), + NodeKey: ptr.String(fmt.Sprintf("key-%d", i)), + Platform: "linux", + Hostname: fmt.Sprintf("host-%d", i), + DetailUpdatedAt: time.Now(), + LabelUpdatedAt: time.Now(), + PolicyUpdatedAt: time.Now(), + SeenTime: time.Now(), + }) + require.NoError(t, err) + hosts = append(hosts, h) + } + + var g errgroup.Group + for _, h := range hosts { + hostID := h.ID + g.Go(func() error { + for i := 0; i < 100; i++ { + if err := ds.ReplaceHostBatteries(ctx, hostID, []*fleet.HostBattery{ + {HostID: hostID, SerialNumber: fmt.Sprintf("%d-0000", hostID), CycleCount: 1, Health: "Good"}, + {HostID: hostID, SerialNumber: fmt.Sprintf("%d-0000", hostID), CycleCount: 2, Health: "Fair"}, + }); err != nil { + return err + } + time.Sleep(10 * time.Millisecond) + } + return nil + }) + } + + err := g.Wait() + require.NoError(t, err) +} + func testCountHostsNotResponding(t *testing.T, ds *Datastore) { ctx := context.Background() config := config.FleetConfig{Osquery: config.OsqueryConfig{DetailUpdateInterval: 1 * time.Hour}} diff --git a/server/fleet/hosts.go b/server/fleet/hosts.go index 6de340ebe8..9610f720cd 100644 --- a/server/fleet/hosts.go +++ b/server/fleet/hosts.go @@ -982,6 +982,7 @@ func (h *HostMDM) UnmarshalJSON(b []byte) error { // HostBattery represents a host's battery, as reported by the osquery battery // table. type HostBattery struct { + ID uint `json:"-" db:"id"` HostID uint `json:"-" db:"host_id"` SerialNumber string `json:"-" db:"serial_number"` CycleCount int `json:"cycle_count" db:"cycle_count"` From 33ceb0ab9fb59f77b167962f2563287906af40df Mon Sep 17 00:00:00 2001 From: Lucas Manuel Rodriguez Date: Tue, 5 Dec 2023 18:25:11 -0300 Subject: [PATCH 6/6] Update edge case bug in `ds.UpdateHostDeviceMapping` (#15454) Found by @mna while working on #14779. Sort of an edge case but the change is simple enough to fix it. --- server/datastore/mysql/hosts.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/server/datastore/mysql/hosts.go b/server/datastore/mysql/hosts.go index df82354692..44f73728a2 100644 --- a/server/datastore/mysql/hosts.go +++ b/server/datastore/mysql/hosts.go @@ -2862,14 +2862,14 @@ func (ds *Datastore) ReplaceHostDeviceMapping(ctx context.Context, hid uint, map insPart = ` (?, ?, ?),` ) - // index the mappings by email and source, to quickly check which ones - // need to be deleted and inserted - toIns := make(map[string]*fleet.HostDeviceMapping) - for _, m := range mappings { - toIns[m.Email+"\n"+m.Source] = m - } - return ds.withRetryTxx(ctx, func(tx sqlx.ExtContext) error { + // index the mappings by email and source, to quickly check which ones + // need to be deleted and inserted + toIns := make(map[string]*fleet.HostDeviceMapping) + for _, m := range mappings { + toIns[m.Email+"\n"+m.Source] = m + } + var prevMappings []*fleet.HostDeviceMapping if err := sqlx.SelectContext(ctx, tx, &prevMappings, selStmt, hid); err != nil { return ctxerr.Wrap(ctx, err, "select previous host emails")