fix: same-name conflict check specified by multus.spidernet.io/cr-name #4168
Conversation
ty-dc
added
release/bug
cherrypick-release-v0.8
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4168 +/- ##
==========================================
- Coverage 80.85% 80.39% -0.47%
==========================================
Files 51 51
Lines 4519 5637 +1118
==========================================
+ Hits 3654 4532 +878
- Misses 699 939 +240
Partials 166 166
Flags with carried forward coverage won't be shown. Click here to find out more. |
ty-dc
force-pushed
the
fix/multus-informer
branch
from
5f7fe89
to
0429e59
Compare
| if ok && customMultusName == "" { | ||
| func (mcw *MultusConfigWebhook) validateAnnotation(ctx context.Context, multusConfig *spiderpoolv2beta1.SpiderMultusConfig) *field.Error { | ||
| // Validate the annotation 'multus.spidernet.io/cr-name' to customize the net-attach-def resource name. | ||
| customMultusName, hasCustomMultusName := multusConfig.Annotations[constant.AnnoNetAttachConfName] |
There was a problem hiding this comment.
为 nil 的场景在后面判断了
case !hasCustomMultusName &&
There was a problem hiding this comment.
我的意思是 multus.Annotations 可能是 nil，L266 可能panic
| return field.Invalid(annotationField, multusConfig.Annotations, "invalid custom net-attach-def resource empty name") | ||
| } | ||
| if len(customMultusName) > k8svalidation.DNS1123SubdomainMaxLength { | ||
| return field.Invalid(annotationField, multusConfig.Annotations, | ||
| fmt.Sprintf("the custom net-attach-def resource name must be no more than %d characters", k8svalidation.DNS1123SubdomainMaxLength)) | ||
| } | ||
|
|
||
| // validate the custom net-attach-def CNI version | ||
| var spiderMultusConfigList spiderpoolv2beta1.SpiderMultusConfigList | ||
| if err := mcw.APIReader.List(ctx, &spiderMultusConfigList); err != nil { |
There was a problem hiding this comment.
这里感觉需要有一个 listOptions 之类的过滤 mulutsname ？如果list 不为空说明当前已经有同名冲突的 multusConfig 了
There was a problem hiding this comment.
是通过 multus.spidernet.io/cr-name 指定的别名，出现冲突。listOptions 并不能去过滤注解？
There was a problem hiding this comment.
是通过 multus.spidernet.io/cr-name 指定的别名，
我知道，我的意思就是直接去 Get 通过 multus.spidernet.io/cr-name 指定的别名，如果成功则有冲突，避免每次创建 webhhok 都需要向 apiServer 全量 List
There was a problem hiding this comment.
默认 spiderMultusConfig 同步 nad 需要一分钟，如果 nad 删除了，这需要一分钟才会同步出来。此时有同名创建， 通过指定 multus.spidernet.io/cr-name 的别名，去查可能就查不到，仍会导致同名 nad 创建成功。
对于 spidernet，或则开始使用 multus 并未使用 spidermultusConfig 的场景。开始存在 nad，然后安装了 spidermultusConfig ，此时需要通过 spidermultusConfig 去纳管 nad ，如果 list multus.spidernet.io/cr-name 指定的别名，有可能纳管失败。
所以全部通过比对 spidermultusconfig 自身不存在冲突，如上是出于这样的目。cc
There was a problem hiding this comment.
默认 spiderMultusConfig 同步 nad 需要一分钟，如果 nad 删除了，这需要一分钟才会同步出来。此时有同名创建， 通过指定 multus.spidernet.io/cr-name 的别名，去查可能就查不到，仍会导致同名 nad 创建成功。
所以这里直接去Get Multus NAD？我感觉这里只需要去检查 当前创建的 smc 指定的 annotationCR name 是否存在同名的 multus Nad就可以了，理论上每一个 spiderMultusConfig 都会得到webhook 验证
There was a problem hiding this comment.
场景是如果直接去 Get Multus NAD，此时 Multus NAD 被删除，但 spiderMultusConfig 还没有完成同步 Multus NAD（同步时间很长），新建的 spiderMultusConfig 指定了被删除的别名，将能够创建出来。仍会导致同名冲突。故就对 spiderMultusConfig 做检查。
There was a problem hiding this comment.
应该 Multus NAD 事件触发 spiderMultusConfig 同步
ty-dc
force-pushed
the
fix/multus-informer
branch
7 times, most recently
from
ee6c7c7
to
d368f01
Compare
Signed-off-by: tao.yang <tao.yang@daocloud.io>
ty-dc
force-pushed
the
fix/multus-informer
branch
from
d368f01
to
ae327b9
Compare
Thanks for contributing!
Notice:
"release/none"
"release/bug"
"release/feature"
What issue(s) does this PR fix:
Fixes #4167
Fixed #4183
Special notes for your reviewer: