Skip to content

fix: cloud_controller_ng#4971#5026

Draft
b1tamara wants to merge 1 commit intocloudfoundry:mainfrom
sap-contributions:fix-hash
Draft

fix: cloud_controller_ng#4971#5026
b1tamara wants to merge 1 commit intocloudfoundry:mainfrom
sap-contributions:fix-hash

Conversation

@b1tamara
Copy link
Copy Markdown
Contributor

@b1tamara b1tamara commented Apr 14, 2026

Thanks for contributing to cloud_controller_ng. To speed up the process of reviewing your pull request please provide us with:

Output during cf push with my change:


    buildpacks:
      nodejs_buildpack
+   routes:
+   - options:
+       loadbalancing: hash
+     route: hash-invalid.cfapps.aws-cfn07.aws.cfn.sapcloud.io
+   - options:
+       hash_balance: 1.25
+       loadbalancing: hash
+     route: hash-no-header.cfapps.aws-cfn07.aws.cfn.sapcloud.io
+   - options:
+       hash_header: hash-header
+       loadbalancing: hash
+     route: hash-no-balance.cfapps.aws-cfn07.aws.cfn.sapcloud.io
  version: 1
For application 'retry-test': For route 'hash-invalid.cfapps.aws-cfn07.aws.cfn.sapcloud.io': Hash header must be present when loadbalancing is set to hash.
  • Links to any other associated PRs: related to hash-based routing implementation in PR#4746

  • I have reviewed the contributing guide

  • I have viewed, signed, and submitted the Contributor License Agreement

  • I have made this pull request to the main branch

  • I have run all the unit tests using bundle exec rake

  • I have run CF Acceptance Tests

@philippthun
Copy link
Copy Markdown
Member

Your change handles errors raised during route creation/update in the first loop. However, the second loop (route mapping) can also raise UpdateRouteDestinations::Error (e.g., protocol mismatch errors), which currently bubbles up without route context.

I've prepared an extended version that also handles these errors:

           message.manifest_route_mappings.each do |manifest_route_mapping|
             current_route = manifest_route_mapping[:route].to_s
             route = {
               model: find_or_create_valid_route(app, manifest_route_mapping[:route].to_hash, user_audit_info),
  -            protocol: manifest_route_mapping[:protocol]
  +            protocol: manifest_route_mapping[:protocol],
  +            route_string: current_route
             }

             raise InvalidRoute.new("No domains exist for route #{current_route}") if route[:model].blank?

             routes_to_map << route
           end

           # map route to app, but do this only if the full message contains valid routes
           routes_to_map.
             each do |route|
  +            current_route = route[:route_string]
               route_mapping = RouteMappingModel.find(app: app, route: route[:model])
               ...
             end
         rescue Sequel::ValidationFailed, RouteCreate::Error, RouteUpdate::Error => e
           route_info = current_route ? "For route '#{current_route}': " : ''
           raise InvalidRoute.new("#{route_info}#{e.message}")
  +      rescue UpdateRouteDestinations::Error => e
  +        route_info = current_route ? "For route '#{current_route}': " : ''
  +        raise UpdateRouteDestinations::Error.new("#{route_info}#{e.message}")
         end

This stores the route string in routes_to_map and updates current_route in the second loop, so errors from both loops include the correct route info.

Please also update the tests (spec/unit/actions/manifest_route_update_spec.rb) to check for this route info being present in the error message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants