-
Notifications
You must be signed in to change notification settings - Fork 23
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
Watch OpenSergo CRD actively in Istio environment #38
base: main
Are you sure you want to change the base?
Conversation
68d711a
to
7cf4db8
Compare
Could you please illustrate the PR in the PR description? |
func Init() error { | ||
home, err := homeDir() | ||
if err != nil { | ||
log.Printf("Find empty home directory, %v", err) | ||
return err | ||
} | ||
kubeConfig := filepath.Join(home, ".kube", "config") | ||
|
||
// uses the current context get restConfig | ||
config, err := clientcmd.BuildConfigFromFlags("", kubeConfig) | ||
if err != nil { | ||
log.Panic(err) | ||
} | ||
dynamicClient, err = dynamic.NewForConfig(config) | ||
if err != nil { | ||
log.Printf("Create dynamicClient for kubernetes failed %v", err) | ||
return err | ||
} | ||
client, err = kubernetes.NewForConfig(config) | ||
if err != nil { | ||
log.Printf("Create client set for kubernetes failed %v", err) | ||
return err | ||
} | ||
|
||
return nil | ||
} |
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.
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.
OK, I will fix it
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.
em... Maybe first need to fix 👇 the following review comment, because I saw that the func Init() also Instance a dynamicClient which will used in ActiveCRDWatcher
.
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.
If k8sclient in ActiveCRDWatcher can be replaced, the k8s_client.go can be deleted. If not, the instance func should be reserved.
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.
And There are some variables, functions, struts named like activeCrdManager
, StartActiveListen
, ActiveCRDWatcher
, I saw they are only used for istio, so may renamed to some word like istio
replace active
will be more clearer.
case TrafficRouterKind: | ||
if crd != nil { | ||
cls := crd.(*traffic.TrafficRouter) | ||
_, err := k8sclient.ApplyCRD(ctx, cls.Namespace, cls.Name, transform.BuildUnstructuredVirtualService(cls), gvr.VirtualServiceGVR) |
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.
k8sclient is a client instanced in Init() of k8s_client.go which depended on k8s.io/client-go, and there is a client in CRDWatcher too, so maybe can try to replace k8sclient by the client in CRDWatcher.
This is only my opinion ~
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 will try to reuse the k8s-client in CRDWatcher! Thanks
Describe what this PR does / why we need it
The PR tends to watch OpenSergo TrafficRouter and VirtualWorkload actively in Istio Environment and convert them into VirtualService and DestinationRule in Istio.
Does this pull request fix one issue?
#37