-
Notifications
You must be signed in to change notification settings - Fork 40
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
🐛 allow arbitrary config fields when calculating config hash #243
🐛 allow arbitrary config fields when calculating config hash #243
Conversation
13125cf
to
1c0fb7a
Compare
/assign @qiujian16 |
pkg/utils/helpers.go
Outdated
func GetSpecHash(obj *unstructured.Unstructured) (string, error) { | ||
if obj == nil { | ||
return "", fmt.Errorf("object is nil") | ||
} | ||
spec, ok := obj.Object["spec"] | ||
if !ok { | ||
return "", fmt.Errorf("object has no spec field") | ||
// handle cases like secret and configmaps | ||
spec = obj.Object["data"] |
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.
what if an API that does not have data field and spec field?
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.
var spec will be nil and return a hash like case .For example, if a configmap removed data field, hash will update and trigger an upgrade.
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.
how about we remove status and metadata part and hash?
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.
Yes, sounds better. Have modified the code.
Signed-off-by: haoqing0110 <qhao@redhat.com>
e6cd3e1
to
1dceac9
Compare
if err != nil { | ||
return "", err | ||
} | ||
|
||
hash := sha256.Sum256(specBytes) | ||
hash := sha256.Sum256(configBytes) |
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.
May be a fast non-cryptographic is good enough here?
https://github.com/zeebo/xxh3
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.
This seems to have better performance, but might not be suitable for the addon case, for example, a cluster-scoped config hash will be exposed to cma, mca and work, and might be cracked by an unexpected namespace-level user.
pkg/utils/helpers.go
Outdated
|
||
specBytes, err := json.Marshal(spec) | ||
// remove non config related fields, the remaining filed should be spec, data, other use defined fields or even empty | ||
delete(obj.Object, "apiVersion") |
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.
This will modify the input object. Is that going to cause any unintended issue? May be a better choice is to perform a shallow copy into a new map and use that calculate hash?
cp := map[string]any{}
for k, v := range obj.Object {
switch k: {
case "apiVersion", "kind", "metadata", "status":
default:
cp[k] = v
}
}
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.
Thanks for raising this, have modified the code.
Signed-off-by: haoqing0110 <qhao@redhat.com>
1dceac9
to
1eb68cd
Compare
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: haoqing0110, qiujian16 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
974d337
into
open-cluster-management-io:main
…uster-management-io#243) * allow data field and empty field when calculating config hash Signed-off-by: haoqing0110 <qhao@redhat.com> * hash config fields Signed-off-by: haoqing0110 <qhao@redhat.com> --------- Signed-off-by: haoqing0110 <qhao@redhat.com>
…uster-management-io#243) * allow data field and empty field when calculating config hash Signed-off-by: haoqing0110 <qhao@redhat.com> * hash config fields Signed-off-by: haoqing0110 <qhao@redhat.com> --------- Signed-off-by: haoqing0110 <qhao@redhat.com> Signed-off-by: Jian Qiu <jqiu@redhat.com>
…uster-management-io#243) * allow data field and empty field when calculating config hash Signed-off-by: haoqing0110 <qhao@redhat.com> * hash config fields Signed-off-by: haoqing0110 <qhao@redhat.com> --------- Signed-off-by: haoqing0110 <qhao@redhat.com> Signed-off-by: Jian Qiu <jqiu@redhat.com>
…uster-management-io#243) * allow data field and empty field when calculating config hash Signed-off-by: haoqing0110 <qhao@redhat.com> * hash config fields Signed-off-by: haoqing0110 <qhao@redhat.com> --------- Signed-off-by: haoqing0110 <qhao@redhat.com> Signed-off-by: Jian Qiu <jqiu@redhat.com>
…uster-management-io#243) * allow data field and empty field when calculating config hash Signed-off-by: haoqing0110 <qhao@redhat.com> * hash config fields Signed-off-by: haoqing0110 <qhao@redhat.com> --------- Signed-off-by: haoqing0110 <qhao@redhat.com> Signed-off-by: Jian Qiu <jqiu@redhat.com>
…uster-management-io#243) * allow data field and empty field when calculating config hash Signed-off-by: haoqing0110 <qhao@redhat.com> * hash config fields Signed-off-by: haoqing0110 <qhao@redhat.com> --------- Signed-off-by: haoqing0110 <qhao@redhat.com> Signed-off-by: Jian Qiu <jqiu@redhat.com>
…259) * allow data field and empty field when calculating config hash * hash config fields --------- Signed-off-by: haoqing0110 <qhao@redhat.com> Signed-off-by: Jian Qiu <jqiu@redhat.com> Co-authored-by: Qing Hao <qhao@redhat.com>
Fix open-cluster-management-io/ocm#364 #237