-
Notifications
You must be signed in to change notification settings - Fork 695
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
Preserve labels and annotations on public cert secrets #1580
Preserve labels and annotations on public cert secrets #1580
Conversation
retest this please |
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 looks good! Maybe lets move the map util stuff someplace else but otherwise I would say it is ready to go 👍
return true | ||
} | ||
if !reflect.DeepEqual(reconciled.Data, expected.Data) { | ||
case !reflect.DeepEqual(expected.Data, reconciled.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.
I wonder if it would make sense to use a hash of the expected service here, similar to what we do for Elasticsearch ssets and Kibana deployments? Not sure myself, so this is not necessary for this to get merged I would say. Wdyt?
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.
Are you suggesting something like comparing the certificate fingerprints? The comparison here is between two Secret objects so in order to use hashes, I think we will have to figure out how to hash the items in the Data
map in a deterministic way.
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.
Maybe I am missing something important but I was thinking of https://github.com/pebrc/cloud-on-k8s/blob/d495796116a956d4d09c773bfd55921b8ddd0702/operators/pkg/controller/common/hash/hash.go#L47 which uses spew. I think that just hexdumps byte arrays and calculates the hash of that. In any case this is not for this PR, your solution is fine IMO
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.
I didn't realise that existed and it's certainly worth investigating. I was initially going to evaluate https://github.com/banzaicloud/k8s-objectmatcher for this issue but it felt like too large a change at this point.
I will create a new issue to explore these options.
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 change allows any external labels and annotations added to public certificate secrets (
*-http-certs-public
and*-transport-certs-public
) to be preserved after reconciliation.Fixes #1472