-
Notifications
You must be signed in to change notification settings - Fork 83
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
Generate VirtualServices based on DependencyProxy #282
Generate VirtualServices based on DependencyProxy #282
Conversation
877a3b2
to
37470d7
Compare
addUpdateVirtualService(ctx, obj, exist, syncNamespace, rc) | ||
} | ||
} | ||
} | ||
return nil | ||
} | ||
|
||
func addUpdateVirtualService(ctx context.Context, obj *v1alpha3.VirtualService, exist *v1alpha3.VirtualService, namespace string, rc *RemoteController) { | ||
func addUpdateVirtualService(ctx context.Context, obj *v1alpha3.VirtualService, exist *v1alpha3.VirtualService, namespace string, rc *RemoteController) error { |
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 we add a function to evaluate if the update is destructive or not?
And do we need knowledge from multiple clusters to build this virtual service? Just like admiral requires knowledge from multiple clusters to build service entries, hence we skip destructive updates during start up time.
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.
Add some logging on diff
admiral/pkg/clusters/serviceentry.go
Outdated
} | ||
for _, dependency := range dependencies { | ||
vs := remoteRegistry.AdmiralCache.DependencyProxyVirtualServiceCache.get(dependency) | ||
if vs == 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.
nitpick:
if vs == nil || len(vs) == 0 {
continue
}
if err != nil { | ||
return fmt.Errorf("failed generating proxy VirtualService %s due to error: %w", v.Name, err) | ||
} | ||
log.Infof("successfully generated proxy VirtualService %s", v.Name) |
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.
nitpick suggestion: addUpdateVirtualService
is adding success/failure logs.
expectedVS *v1alpha3.VirtualService | ||
}{ | ||
{ | ||
name: "Given dependency proxy to generate VS, when dependencylookipCache is nil, then the func should return an error", |
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.
nitpick, typo: dependencylookipCache
-> dependencylookupCache
expectedError: fmt.Errorf("remoteRegistry.AdmiralCache.DependencyProxyVirtualServiceCache is nil"), | ||
}, | ||
{ | ||
name: "Given dependency proxy to generate VS, when the sourceIdentity is not in dependencylookipCache, then the func should not return an error", |
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 we also assert that we are calling the client to create the VS ?
@@ -124,6 +135,32 @@ func NewRemoteRegistry(ctx context.Context, params common.AdmiralParams) *Remote | |||
} | |||
} | |||
|
|||
type dependencyLookupCache struct { | |||
sourceDestinations map[string][]string |
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.
should we call this destinationBySource
, or sourceToDestinations
?
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.
sourceToDestinations
dnsSuffix := dependencyProxyObj.Spec.Destination.DnsSuffix | ||
proxyEnv := dependencyProxyObj.ObjectMeta.Annotations[common.GetEnvKey()] | ||
|
||
defaultVSHostName := strings.ToLower(common.GetCnameVal([]string{proxyEnv, destinationServiceIdentity, dnsSuffix})) |
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.
just put this in a interface
…sources Signed-off-by: Shriram Sharma <[email protected]>
Signed-off-by: Shriram Sharma <[email protected]>
e1f59db
to
06ee9a5
Compare
Signed-off-by: Shriram Sharma <[email protected]>
@@ -1,8 +1,6 @@ | |||
apiversion: kustomize.config.k8s.io/v1beta1 | |||
kind: Kustomization | |||
|
|||
namespace: sample |
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.
not using namespace anymore?
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.
adding it back, I'm not sure how this got removed
@@ -0,0 +1,24 @@ | |||
#!/bin/bash -e | |||
|
|||
## This test verifies if the DependencyProxy created for the httpbin service |
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 add the flow of request what we discussed yesterday as well.
Signed-off-by: Shriram Sharma <[email protected]>
fixes #278