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

fix(clouddriver): Throw SpinnakerHttpException when Http status is 403 in DetermineSourceServerGroupTask #4710

Conversation

Pranav-b-7
Copy link
Contributor

@Pranav-b-7 Pranav-b-7 commented Apr 24, 2024

This task earlier returns TaskResult with RUNNING status and exception message in context when Forbidden error 403 has occurred. With the new changes/fix it now throws SpinnakerHttpException in this scenario making it in compliance with the commit: 84a7106

Here is the stack trace that would demonstrate the bug and the fix -

Before Fix:

TaskResult(status=RUNNING, context={lastException=Status: 403, URL: http://localhost:39831/applications/testCluster/clusters/test/testCluster/aws/us-east-1/serverGroups/target/ancestor_asg_dynamic, Message: Forbidden
com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException: Status: 403, URL: http://localhost:39831/applications/testCluster/clusters/test/testCluster/aws/us-east-1/serverGroups/target/ancestor_asg_dynamic, Message: Forbidden
at com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler.handleError(SpinnakerRetrofitErrorHandler.java:55)
at retrofit.RestAdapter$RestHandler.invoke(RestAdapter.java:242)
at jdk.proxy3/jdk.proxy3.$Proxy21.getTargetServerGroup(Unknown Source)
at com.netflix.spinnaker.orca.clouddriver.OortService$getTargetServerGroup.call(Unknown Source)
at com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.support.TargetServerGroupResolver$_resolveByTarget_closure2.doCall(TargetServerGroupResolver.groovy:76)
at com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.support.TargetServerGroupResolver$_resolveByTarget_closure2.doCall(TargetServerGroupResolver.groovy)
, attempt=2, consecutiveNotFound=0}, outputs={})

After Fix:

2024-04-24 16:48:25.900 ERROR --- [ Test worker] .n.s.o.c.p.s.s.TargetServerGroupResolver : [] Unable to locate ancestor_asg_dynamic in test/us-east-1/testCluster
com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException: Status: 403, URL: http://localhost:32773/applications/testCluster/clusters/test/testCluster/aws/us-east-1/serverGroups/target/ancestor_asg_dynamic, Message: Forbidden
at com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler.handleError(SpinnakerRetrofitErrorHandler.java:55)
at retrofit.RestAdapter$RestHandler.invoke(RestAdapter.java:242)
at jdk.proxy3/jdk.proxy3.$Proxy21.getTargetServerGroup(Unknown Source)
at com.netflix.spinnaker.orca.clouddriver.OortService$getTargetServerGroup.call(Unknown Source)
at com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.support.TargetServerGroupResolver$_resolveByTarget_closure2.doCall(TargetServerGroupResolver.groovy:76)
at com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.support.TargetServerGroupResolver$_resolveByTarget_closure2.doCall(TargetServerGroupResolver.groovy)

@Pranav-b-7 Pranav-b-7 changed the title test(clouddriver): Demonstrate bug in DetermineSourceServerGroupTask fix(clouddriver): Handle SpinnakerHttpException in DetermineSourceServerGroupTask Apr 24, 2024
@Pranav-b-7 Pranav-b-7 marked this pull request as draft April 24, 2024 14:16
@Pranav-b-7 Pranav-b-7 force-pushed the remove-retrofitError-determineSourceServerGroupTask branch from aa3e094 to b606baa Compare April 24, 2024 15:23
This task/class is suppose to throw a SpinnakerHttpException when a HTTP forbidden 403 error occurs, but instead it returns TaskResult in which the execution status and exception message is wrapped.

This task/class actually makes a couple of oortService API calls: 1 - oortService.getTargetServerGroup() - API to fetch the server group based on the target. 2 - oortService.getServerGroupsFromClusterTyped() - API to fetch server group based on server group name.

From the commit: 84a7106 , OortService uses SpinnakerRetrofitErrorHandler so, the expectation is to throw SpinnakerHttpException.
@Pranav-b-7 Pranav-b-7 force-pushed the remove-retrofitError-determineSourceServerGroupTask branch from b606baa to e4c19dc Compare April 24, 2024 16:26
…3 in DetermineSourceServerGroupTask

This task earlier returns TaskResult with RUNNING status and exception message in context when Forbidden error 403 has occurred. With the new changes/fix it now throws SpinnakerHttpException in this scenario making it in compliance with the commit: 84a7106

Here is the stack trace that would demonstrate the bug and the fix -

Before Fix:

TaskResult(status=RUNNING, context={lastException=Status: 403, URL: http://localhost:39831/applications/testCluster/clusters/test/testCluster/aws/us-east-1/serverGroups/target/ancestor_asg_dynamic, Message: Forbidden
com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException: Status: 403, URL: http://localhost:39831/applications/testCluster/clusters/test/testCluster/aws/us-east-1/serverGroups/target/ancestor_asg_dynamic, Message: Forbidden
	at com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler.handleError(SpinnakerRetrofitErrorHandler.java:55)
	at retrofit.RestAdapter$RestHandler.invoke(RestAdapter.java:242)
	at jdk.proxy3/jdk.proxy3.$Proxy21.getTargetServerGroup(Unknown Source)
	at com.netflix.spinnaker.orca.clouddriver.OortService$getTargetServerGroup.call(Unknown Source)
	at com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.support.TargetServerGroupResolver$_resolveByTarget_closure2.doCall(TargetServerGroupResolver.groovy:76)
	at com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.support.TargetServerGroupResolver$_resolveByTarget_closure2.doCall(TargetServerGroupResolver.groovy)
, attempt=2, consecutiveNotFound=0}, outputs={})

After Fix:

2024-04-24 16:48:25.900 ERROR   --- [    Test worker] .n.s.o.c.p.s.s.TargetServerGroupResolver : [] Unable to locate ancestor_asg_dynamic in test/us-east-1/testCluster
com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException: Status: 403, URL: http://localhost:32773/applications/testCluster/clusters/test/testCluster/aws/us-east-1/serverGroups/target/ancestor_asg_dynamic, Message: Forbidden
	at com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler.handleError(SpinnakerRetrofitErrorHandler.java:55)
	at retrofit.RestAdapter$RestHandler.invoke(RestAdapter.java:242)
	at jdk.proxy3/jdk.proxy3.$Proxy21.getTargetServerGroup(Unknown Source)
	at com.netflix.spinnaker.orca.clouddriver.OortService$getTargetServerGroup.call(Unknown Source)
	at com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.support.TargetServerGroupResolver$_resolveByTarget_closure2.doCall(TargetServerGroupResolver.groovy:76)
	at com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.support.TargetServerGroupResolver$_resolveByTarget_closure2.doCall(TargetServerGroupResolver.groovy)
@Pranav-b-7 Pranav-b-7 changed the title fix(clouddriver): Handle SpinnakerHttpException in DetermineSourceServerGroupTask fix(clouddriver): Throw SpinnakerHttpException when Http status is 403 in DetermineSourceServerGroupTask Apr 24, 2024
@Pranav-b-7 Pranav-b-7 marked this pull request as ready for review April 24, 2024 16:59
@dbyron-sf dbyron-sf added the ready to merge Approved and ready for merge label Apr 24, 2024
@mergify mergify bot added the auto merged Merged automatically by a bot label Apr 24, 2024
@mergify mergify bot merged commit d2f5024 into spinnaker:master Apr 24, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merged Merged automatically by a bot ready to merge Approved and ready for merge target-release/1.35
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants