-
Notifications
You must be signed in to change notification settings - Fork 78
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
skipped updating istio resources which were not created by admiral #271
skipped updating istio resources which were not created by admiral #271
Conversation
Signed-off-by: Shriram Sharma <shriram_sharma@intuit.com>
9ebf3f5
to
7b30fba
Compare
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.
The implementation looks good, couple of refactor suggestions.
admiral/pkg/clusters/serviceentry.go
Outdated
@@ -35,6 +35,11 @@ type SeDrTuple struct { | |||
DestinationRule *networking.DestinationRule | |||
} | |||
|
|||
const ( | |||
istioCustomResourceCreatedByAnnotationLabel = "app.kubernetes.io/created-by" |
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.
Probably rename these as resourceCreatedByAnnotationLabel
as we can use these for other resources.
admiral/pkg/clusters/serviceentry.go
Outdated
// if it was, then admiral will not take any action on this SE | ||
skipSEUpdate := false | ||
if oldServiceEntry != nil { | ||
if !isGeneratedByAdmiral(oldServiceEntry.Annotations) { |
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.
Can be made a single if statement instead of nesting?
newServiceEntry := createServiceEntrySkeletion(*seDr.ServiceEntry, seDr.SeName, syncNamespace) | ||
if newServiceEntry != nil { | ||
newServiceEntry.Labels = map[string]string{common.GetWorkloadIdentifier(): fmt.Sprintf("%v", identityId)} | ||
addUpdateServiceEntry(ctx, newServiceEntry, oldServiceEntry, syncNamespace, rc) |
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.
Is it possible to move the logic to skip updating into addUpdateServiceEntry
and addUpdateDestinationRule
? That might make it cleaner.
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.
hey @aattuluri , I agree it will make it cleaner but I would need to do the same for the deleteSE
and deleteDR
funcs. Plus, in order to avoid adding it to the SEClusterCache, I added the logic here.
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.
Got it, I am good then.
Signed-off-by: Shriram Sharma <shriram_sharma@intuit.com>
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.
lgtm
Fixes #270