Skip to content
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

[bug] prevent panic when finding out rollout strategy #210

Merged
merged 2 commits into from
May 2, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions admiral/pkg/clusters/serviceentry.go
Original file line number Diff line number Diff line change
Expand Up @@ -137,11 +137,7 @@ func modifyServiceEntryForNewServiceOrPod(event admiral.EventType, env string, s
localFqdn := serviceInstance.Name + common.Sep + serviceInstance.Namespace + common.DotLocalDomainSuffix
rc := remoteRegistry.RemoteControllers[sourceCluster]
var meshPorts map[string]uint32
isBlueGreenStrategy := false

if len(sourceRollouts) > 0 {
isBlueGreenStrategy = sourceRollouts[sourceCluster].Spec.Strategy.BlueGreen != nil
}
blueGreenStrategy := isBlueGreenStrategy(sourceRollouts[sourceCluster])

if len(sourceDeployments) > 0 {
meshPorts = GetMeshPorts(sourceCluster, serviceInstance, sourceDeployments[sourceCluster])
Expand All @@ -159,7 +155,7 @@ func modifyServiceEntryForNewServiceOrPod(event admiral.EventType, env string, s
//replace istio ingress-gateway address with local fqdn, note that ingress-gateway can be empty (not provisoned, or is not up)
if ep.Address == clusterIngress || ep.Address == "" {
// Update endpoints with locafqdn for active and preview se of bluegreen rollout
if isBlueGreenStrategy {
if blueGreenStrategy {
oldPorts := ep.Ports
updateEndpointsForBlueGreen(sourceRollouts[sourceCluster], sourceWeightedServices[sourceCluster], cnames, ep, sourceCluster, key)

Expand Down Expand Up @@ -761,3 +757,12 @@ func generateServiceEntry(event admiral.EventType, admiralCache *AdmiralCache, m

return tmpSe
}

func isBlueGreenStrategy(rollout *argo.Rollout) bool {
if rollout != nil && &rollout.Spec != (&argo.RolloutSpec{}) && rollout.Spec.Strategy != (argo.RolloutStrategy{}) {
if rollout.Spec.Strategy.BlueGreen != nil {
return true
}
}
return false
}
78 changes: 78 additions & 0 deletions admiral/pkg/clusters/serviceentry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1339,3 +1339,81 @@ func isLower(s string) bool {
}
return true
}

func TestIsBlueGreenStrategy(t *testing.T) {
var (
emptyRollout *argo.Rollout
rolloutWithBlueGreenStrategy = &argo.Rollout{
Spec: argo.RolloutSpec{
Strategy: argo.RolloutStrategy{
BlueGreen: &argo.BlueGreenStrategy{
ActiveService: "active",
},
},
},
}
rolloutWithCanaryStrategy = &argo.Rollout{
Spec: argo.RolloutSpec{
Strategy: argo.RolloutStrategy{
Canary: &argo.CanaryStrategy{
CanaryService: "canaryservice",
},
},
},
}
rolloutWithNoStrategy = &argo.Rollout{
Spec: argo.RolloutSpec{},
}
rolloutWithEmptySpec = &argo.Rollout{}
)
cases := []struct {
name string
rollout *argo.Rollout
expectedResult bool
}{
{
name: "Given argo rollout is configured with blue green rollout strategy" +
"When isBlueGreenStrategy is called" +
"Then it should return true",
rollout: rolloutWithBlueGreenStrategy,
expectedResult: true,
},
{
name: "Given argo rollout is configured with canary rollout strategy" +
"When isBlueGreenStrategy is called" +
"Then it should return false",
rollout: rolloutWithCanaryStrategy,
expectedResult: false,
},
{
name: "Given argo rollout is configured without any rollout strategy" +
"When isBlueGreenStrategy is called" +
"Then it should return false",
rollout: rolloutWithNoStrategy,
expectedResult: false,
},
{
name: "Given argo rollout is nil" +
"When isBlueGreenStrategy is called" +
"Then it should return false",
rollout: emptyRollout,
expectedResult: false,
},
{
name: "Given argo rollout has an empty Spec" +
"When isBlueGreenStrategy is called" +
"Then it should return false",
rollout: rolloutWithEmptySpec,
expectedResult: false,
},
}

for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
result := isBlueGreenStrategy(c.rollout)
if result != c.expectedResult {
t.Errorf("expected: %t, got: %t", c.expectedResult, result)
}
})
}
}