Skip to content
Closed
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
1 change: 1 addition & 0 deletions app/actions/app_apply_manifest.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ def apply(app_guid, message)

app_update_message = message.app_update_message
lifecycle = AppLifecycleProvider.provide_for_update(app_update_message, app)

AppUpdate.new(@user_audit_info, manifest_triggered: true).update(app, app_update_message, lifecycle)

update_routes(app, message)
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/v3/buildpacks_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ def upload

unauthorized! unless permission_queryer.can_write_globally?

message = BuildpackUploadMessage.create_from_params(hashed_params[:body])
message = BuildpackUploadMessage.create_from_params(hashed_params[:body], buildpack.lifecycle)
combine_messages(message.errors.full_messages) unless message.valid?

unprocessable!('Buildpack is locked') if buildpack.locked
Expand Down
1 change: 1 addition & 0 deletions app/controllers/v3/space_manifests_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ def apply_manifest
can_write_space(space)

messages = parsed_app_manifests.map { |app_manifest| AppManifestMessage.create_from_yml(app_manifest) }

errors = messages.each_with_index.flat_map { |message, i| errors_for_message(message, i) }
compound_error!(errors) unless errors.empty?

Expand Down
3 changes: 2 additions & 1 deletion app/fetchers/buildpack_list_fetcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ def filter(message, dataset)
dataset = dataset.where(name: message.names) if message.requested?(:names)

dataset = NullFilterQueryGenerator.add_filter(dataset, :stack, message.stacks) if message.requested?(:stacks)
dataset = dataset.where(lifecycle: message.requested?(:lifecycle) ? message.lifecycle : Config.config.get(:default_app_lifecycle))

dataset = dataset.where(lifecycle: message.lifecycle) if message.requested?(:lifecycle)
Comment on lines +22 to +23
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how the list will look when called with an older version of CLI (by doing cf buildpacks)?

Copy link
Copy Markdown
Author

@tomkennedy513 tomkennedy513 Apr 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the cli explicitly sends an order_by of position, the list will be mixed, but we felt that it was better to have that behavior because the default filter was breaking the update-buildpack and delete-buildpack commands if the buildpack was using a non default lifecycle because the buildpack could not be found without the lifecycle query in the api call.

Copy link
Copy Markdown

@pbusko pbusko Apr 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But since all buildpacks endpoints now have the lifecycle parameter, the mutating commands have to specify it. We'll introduce these flags to the CLI in the following PR after this one is merged. I believe that filtering is a must for backward compatibility.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then old clis would break if the default lifecycle is changed. Or if someone uploads a build pack with a non default lifecycle and older cli can’t delete it. In my opinion the buildpack list being jumbled is not a breaking change as much as commands breaking.

Copy link
Copy Markdown

@sethboyles sethboyles Apr 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pbusko can you explain what this will break? It is just confusing? We already have precendence for intermingled positions with stacks:

Screenshot 2025-04-30 at 9 54 50 AM

It's not ideal for lifecycles, since that's not in the table yet, but at least stack will be blank, so there is a clue that these buildpacks are different.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm creating a new app I could use either lifecycle and I may not know that I need to look for multiple lifecycles. We could make multiple calls but that seems unnecessary when we could just make the filtering configurable and leave it up to the operator whether they should be returned in a single table or not. I could also see an issue if another lifecycle is added one day and the cli would need to be updated to add another call for filtering.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently the cnb lifecycle works only with custom buildpacks, so it is not an issue. Apps can be created with either but the cf buildpacks is still reliably returns buildpacks for the buildpack lifecycle. I do see filtering as a necessity.

perhaps the default filtering should be a configuration option?

This sounds like a valid approach, but with the filtering enabled by default.

Copy link
Copy Markdown

@sethboyles sethboyles May 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now we don't have default filtering anywhere in the API and I don't think we should include it. It's inherently confusing.

Additionally, developers can still push with CNB buildpacks on the old CLI if they are using manifests. It will be confusing if they can't see those buildpacks at all.

Copy link
Copy Markdown

@Gerg Gerg May 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, hiding resources on the API by default is too cautious; buildpacks of all lifecycles should be returned by default.

Some thoughts:

  1. Returning additional resources from an endpoint or adding fields to a resource is not a breaking change.
  2. The CLI is not the only client of the API, so we don't want to over-fit the API design to the CLI.
  3. I'm not aware of any prior art for hiding resources by default on v3 CAPI. It would be confusing for API users & client authors to have an endpoint that behaves inconsistently with other endpoints.
  4. I would not consider showing somewhat confusing output in old versions of the CLI to be a CLI breaking change either, especially since it would only affect environments that have CNBs installed.
  5. If CLI users run into issues, they can always upgrade their CLI version.

Some mitigations:

  1. The CNB buildpack names could reflect their lifecycle (e.g. ruby_cnb), so it would be clear to users what they are, even if their CLI version doesn't display the "lifecycle".
  2. Operators can configure min CLI versions in their environments to prompt users to upgrade: https://github.com/cloudfoundry/capi-release/blob/a36693b72164c93a57f01957f278bc35a124a0cd/jobs/cloud_controller_ng/spec#L416-L424

I personally wouldn't add a new manifest feature flag to add opt-in filtering behavior, but I don't oppose it, if you feel strongly that it's important to do.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pbusko We are planning to merge in your PR + this PR tomorrow. When releasing, we'll be sure to include a release note indicating the behavior of the buildpacks endpoint if CNBs are added. If you want to include a configuration flag to add default filtering, that's something we can add at a later date.


if message.requested?(:label_selector)
dataset = LabelSelectorQueryGenerator.add_selector_queries(
Expand Down
53 changes: 22 additions & 31 deletions app/messages/app_manifest_message.rb
Original file line number Diff line number Diff line change
Expand Up @@ -123,16 +123,32 @@ def audit_hash
end

def app_lifecycle_hash
lifecycle_data = if requested?(:lifecycle) && @lifecycle == 'cnb'
cnb_lifecycle_data
lifecycle_type = if requested?(:lifecycle) && @lifecycle == 'cnb'
Lifecycles::CNB
elsif requested?(:lifecycle) && @lifecycle == 'buildpack'
Lifecycles::BUILDPACK
elsif requested?(:docker)
docker_lifecycle_data
elsif requested?(:buildpacks) || requested?(:buildpack) || requested?(:stack)
buildpacks_lifecycle_data
Lifecycles::DOCKER
end

data = {
buildpacks: requested_buildpacks,
stack: @stack,
credentials: @cnb_credentials
}.compact

if lifecycle_type == Lifecycles::DOCKER
lifecycle = {
type: Lifecycles::DOCKER
}
elsif lifecycle_type || data.present?
lifecycle = {}
lifecycle[:type] = lifecycle_type if lifecycle_type.present?
lifecycle[:data] = data if data.present?
end

{
lifecycle: lifecycle_data,
lifecycle: lifecycle,
metadata: requested?(:metadata) ? metadata : nil
}.compact
end
Expand Down Expand Up @@ -279,31 +295,6 @@ def service_bindings_attribute_mapping
mapping
end

def docker_lifecycle_data
{ type: Lifecycles::DOCKER }
end

def cnb_lifecycle_data
{
type: Lifecycles::CNB,
data: {
buildpacks: requested_buildpacks,
stack: @stack,
credentials: @cnb_credentials
}.compact
}
end

def buildpacks_lifecycle_data
{
type: Lifecycles::BUILDPACK,
data: {
buildpacks: requested_buildpacks,
stack: @stack
}.compact
}
end

def should_autodetect?(buildpack)
buildpack == 'default' || buildpack == 'null' || buildpack.nil?
end
Expand Down
25 changes: 21 additions & 4 deletions app/messages/buildpack_upload_message.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
module VCAP::CloudController
GZIP_MIME = Regexp.new("\x1F\x8B\x08".force_encoding('binary'))
ZIP_MIME = Regexp.new("PK\x03\x04".force_encoding('binary'))
CNB_MIME = Regexp.new("\x75\x73\x74\x61\x72\x00\x30\x30".force_encoding('binary'))

class BuildpackUploadMessage < BaseMessage
class MissingFilePathError < StandardError; end
Expand All @@ -16,8 +17,15 @@ class MissingFilePathError < StandardError; end
validate :is_not_empty
validate :missing_file_path

def self.create_from_params(params)
BuildpackUploadMessage.new(params.dup.symbolize_keys)
attr_reader :lifecycle

def initialize(params, lifecycle)
@lifecycle = lifecycle
super(params)
end

def self.create_from_params(params, lifecycle)
BuildpackUploadMessage.new(params.dup.symbolize_keys, lifecycle)
end

def nginx_fields
Expand Down Expand Up @@ -51,9 +59,18 @@ def is_archive

mime_bits = File.read(bits_path, 4)

return if mime_bits =~ /^#{VCAP::CloudController::GZIP_MIME}/ || mime_bits =~ /^#{VCAP::CloudController::ZIP_MIME}/
if lifecycle == VCAP::CloudController::Lifecycles::BUILDPACK
return if mime_bits =~ /^#{VCAP::CloudController::ZIP_MIME}/

errors.add(:base, "#{bits_name} is not a zip file. Buildpacks of lifecycle \"#{lifecycle}\" must be valid zip files.")
elsif lifecycle == VCAP::CloudController::Lifecycles::CNB
return if mime_bits =~ /^#{VCAP::CloudController::GZIP_MIME}/

mime_bits_at_offset = File.read(bits_path, 8, 257)
return if mime_bits_at_offset =~ /^#{VCAP::CloudController::CNB_MIME}/

errors.add(:base, "#{bits_name} is not a zip or gzip archive")
errors.add(:base, "#{bits_name} is not a gzip archive or cnb file. Buildpacks of lifecycle \"#{lifecycle}\" must be valid gzip archives or cnb files.")
end
end

def missing_file_path
Expand Down
5 changes: 3 additions & 2 deletions app/messages/buildpacks_list_message.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ class BuildpacksListMessage < MetadataListMessage

def initialize(params={})
super
pagination_options.default_order_by = :position
pagination_options.default_order_by = :lifecycle
pagination_options.secondary_default_order_by = :position
end

def self.from_params(params)
Expand All @@ -33,7 +34,7 @@ def to_param_hash
end

def valid_order_by_values
super << :position
super + %i[position lifecycle]
end
end

Expand Down
2 changes: 2 additions & 0 deletions docs/v3/source/includes/api_resources/_buildpacks.erb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"filename": null,
"stack": "windows64",
"position": 42,
"lifecycle": "buildpack",
"enabled": true,
"locked": false,
"metadata": {
Expand Down Expand Up @@ -52,6 +53,7 @@
"filename": null,
"stack": "my-stack",
"position": 1,
"lifecycle": "cnb",
"enabled": true,
"locked": false,
"metadata": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ Name | Type | Description
----------- | -------- | ----------------------------------------------------------------------------- | -------
**stack** | _string_ | The name of the stack that the buildpack will use | null
**position** | _integer_ | The order in which the buildpacks are checked during buildpack auto-detection | 1
**lifecycle**| _string_ | The version of buildpack the buildpack will use. `buildpack` indicates [Classic Buildpacks](https://docs.cloudfoundry.org/buildpacks/classic.html). `cnb` indicates [Cloud Native Buildpacks](https://docs.cloudfoundry.org/buildpacks/cnb/) | buildpack
**enabled** | _boolean_ | Whether or not the buildpack will be used for staging | true
**locked** | _boolean_ | Whether or not the buildpack is locked to prevent updating the bits | false
**metadata.labels** | [_label object_](#labels) | Labels applied to the buildpack
Expand Down
3 changes: 2 additions & 1 deletion docs/v3/source/includes/resources/buildpacks/_list.md.erb
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ Name | Type | Description
**per_page** | _integer_ | Number of results per page; <br>valid values are 1 through 5000
**names** | _list of strings_ | Comma-delimited list of buildpack names to filter by
**stacks**| _list of strings_ | Comma-delimited list of stack names to filter by
**order_by** | _string_ | Value to sort by; defaults to ascending. Prepend with `-` to sort descending. <br>Valid values are `created_at`, `updated_at`, and `position`
**lifecycle**| _string_ | Type of buildpack. Valid values are `buildpack` and `cnb`
**order_by** | _string_ | Value to sort by; defaults to ascending. Prepend with `-` to sort descending. <br>Valid values are `created_at`, `updated_at`, `lifecycle`, and `position`
**label_selector** | _string_ | A query string containing a list of [label selector](#labels-and-selectors) requirements
**created_ats** | _[timestamp](#timestamps)_ | Timestamp to filter by. When filtering on equality, several comma-delimited timestamps may be passed. Also supports filtering with [relational operators](#relational-operators)
**updated_ats** | _[timestamp](#timestamps)_ | Timestamp to filter by. When filtering on equality, several comma-delimited timestamps may be passed. Also supports filtering with [relational operators](#relational-operators)
Expand Down
3 changes: 2 additions & 1 deletion docs/v3/source/includes/resources/buildpacks/_object.md.erb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ Name | Type | Description
**updated_at** | _[timestamp](#timestamps)_ | The time with zone when the object was last updated
**name** | _string_ | The name of the buildpack; to be used by app buildpack field (only alphanumeric characters)
**state** | _string_ | The state of the buildpack; valid states are: `AWAITING_UPLOAD`, `READY`
**stack** | _string_ | The name of the stack that the buildpack will use
**stack** | _string_ | The name of the stack that the buildpack uses
**lifecycle** | _string_ | The version of buildpacks the buildpack uses. `buildpack` indicates [Classic Buildpacks](https://docs.cloudfoundry.org/buildpacks/classic.html). `cnb` indicates [Cloud Native Buildpacks](https://docs.cloudfoundry.org/buildpacks/cnb/)
**filename** | _string_ | The filename of the buildpack
**position** | _integer_ | The order in which the buildpacks are checked during buildpack auto-detection
**enabled** | _boolean_ | Whether or not the buildpack can be used for staging
Expand Down
2 changes: 1 addition & 1 deletion lib/cloud_controller/install_buildpacks.rb
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ def detected_stack(file, opts)
def buildpack_zip(package, zipfile)
return zipfile if zipfile

job_dir = File.join('/var/vcap/packages', package, '*.zip')
job_dir = File.join('/var/vcap/packages', package, '*[.zip|.cnb|.tgz|.tar.gz]')
Dir[job_dir].first
end

Expand Down
8 changes: 7 additions & 1 deletion lib/cloud_controller/paging/pagination_options.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ class PaginationOptions
DIRECTION_DEFAULT = 'asc'.freeze
VALID_DIRECTIONS = %w[asc desc].freeze

attr_writer :order_by, :order_direction, :default_order_by
attr_writer :order_by, :order_direction, :default_order_by, :secondary_default_order_by
attr_accessor :page, :per_page

def initialize(params)
Expand All @@ -38,6 +38,12 @@ def order_direction
@order_direction || DIRECTION_DEFAULT
end

def secondary_order_by
return if @order_by && @order_by.to_s != default_order_by.to_s

@secondary_default_order_by
end

def keys
%i[page per_page order_by order_direction]
end
Expand Down
2 changes: 2 additions & 0 deletions lib/cloud_controller/paging/sequel_paginator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ def get_page(dataset, pagination_options)
order_type = Sequel.send(order_direction, Sequel.qualify(table_name, order_by))
dataset = dataset.order(order_type)

secondary_order_by = pagination_options.secondary_order_by
dataset = dataset.order_append(Sequel.send(order_direction, Sequel.qualify(table_name, secondary_order_by))) if secondary_order_by
dataset = dataset.order_append(Sequel.send(order_direction, Sequel.qualify(table_name, :guid))) if order_by != 'id' && has_guid_column

distinct_opt = dataset.opts[:distinct]
Expand Down
Loading