API Review of v1alpha1

[adambkaplan]

As this project progresses, several outdated/obsolete aspects of the Build APIs have been uncovered. This will serve as a master tracking issue of API fields that should be removed in v1alpha1. We will consider this issue "done" when we have reviewed the current API and agreed that all removals from our initial implementation are complete. This does not necessarily mean that we consider Shipwright's API stable enough for a beta version.

The description of this field will be updated with accepted API changes.

• Deprecate .status.succeeded and .status.reason #674 Deprecate BuildRun status.succeeded and status.reason.
• Rename build.dev -> shipwright.io #609 Rename build.dev -> shipwright.io
• Remove unused fields from GitSource #683 Remove unused fields from GitSource
• Unify Go struct field names with YAML/JSON field names #687 Unify Go struct field names with YAML/JSON field names

[sbose78]

👍

[imjasonh]

As discussed today in the Slack, we might also want to take this opportunity to audit the permissions assigned to the controller's Service Account.

When we did this in Tekton, we discovered some permissions we didn't actually need, and some which we only needed in Tekton's own namespace, instead of cluster-wide.

[qu1queee]

I´m adding an initial list of API changes from a brief recollection of issues. This is categorized by Breaking, Addition or Removal API changes. Looking forward for more contributions.

This is a living table, changes are expected to happen

API field	Resource	Change Type	Backwards Compatible	Description
CRD Scheme Group	All CRDs	Breaking	No. We can only have one single gvk per Golang struct. Existing CRDs will not longer work.	Move the scheme group from build.dev to shipwright.io, more info in issue 563
CRD Scheme Version	All CRDs	Breaking	TBD. Tekton Community should have done this before, e.g. migration approach	Move the scheme version from v1alpha1 to v1beta1
spec.sources	Build	Breaking	Yes. We will keep the spec.source as a deprecated field for a while. Users can continue using the deprecated one until the new one is adopted.	This comes as part of the implementation of EP remote artifacts
status.conditions	BuildRun	Addition	Yes. We kept the old Status.Reason and Status.Succeeded for a while as deprecated fields. Workflows relying on the deprecated fields should adopt the new Condition.	This was already done as part of adopting Conditions in BuildRun.
status.XYZ	BuildRun	Addition	N/A	There are new fields we can inherit from TaskRuns to report more data of the state of a build container image. Similar to issue 618
spec.parameters	Build	Addition	N/A	Based on issue 184, will this make it to the API?.. tbd.
spec.output.tag	Build	Addition	N/A	Based on issue 171, will this make it to the API?.. tbd.
spec.env	Build	Addition	N/A	Based on issue 224, will this make it to the API?.. tbd.

[gabemontero]

On going from alpha to beta, I'll put a cookie crumb link to https://kubernetes.slack.com/archives/C019ZRGUEJC/p1614018974009600 in slack around some of the pain upstream tekton has run into wrt supporting alpha clients once their API went beta and more fields were added.

As we discussed in the Mar 1 2021 community meeting today, we should review the k8s conventions / requirements around that scenario when deciding to move from alpha to beta.

[imjasonh]

I noticed that BuildStrategy's .spec.buildSteps specifies a type that wraps the K8s core v1.Container. This means that BuildStrategy authors can't take advantage of Tekton's script support. This might be fine; it also means the Shipwright API isn't tied too closely to Tekton's.

Aside from exposing script, it might be useful to fully specify the BuildStep type in Shipwright code, instead of relying on fields in v1.Container -- I wish Tekton had done this, and might try to make it happen some time before Tekton's v1.

The reason you'd want to specify the fields yourself is twofold:

1. Not all fields in v1.Container make sense in a BuildStrategy -- things like livenessCheck or stdin, or even things that Tekton itself blocks like terminationMessagePath.
2. If v1.Container adds a new field in a future release, it would become part of the Shipwright API at the next release. Whether or not that field is useful in Shipwright, it's nice to make that decision ourselves.

[imjasonh]

I also noticed that a number of fields in the API have a different name in the Go struct than in the JSON/YAML, such as:

type BuildSpec struct {
	Source        GitSource   `json:"source"`
	StrategyRef  *StrategyRef `json:"strategy"`
	BuilderImage *Image       `json:"builder,omitempty"`
...
}

(ref)

In this case, it can be hard to read the Go struct and understand what the corresponding YAML should look like. Is it image: or builderImage: or builder:? source: or gitSource:? strategy: or strategyRef:?

This can become painful when you're trying to write Go code against the API, since you have to keep both names in mind all the time. Maybe your IDE helps you, but we shouldn't require an IDE to have a good time.

This could be aligned in a backward-compatible* change, by renaming the Go fields/structs to align with the JSON/YAML struct tags. This could be a good-first-issue even on the pre-beta timeline.

*Tekton has defined backward compatibility to mean "YAML/JSON that used to work still works", not necessarily that Go code still compiles.

edit: also a couple examples of SecretRef tagged as credentials, which could just have the field name Credentials.

[sbose78]

it might be useful to fully specify the BuildStep type in Shipwright code

+1
Yes, there's an open ticket somewhere on this.

[imjasonh]

+1
Yes, there's an open ticket somewhere on this.

Possibly #224 ?

[sbose78]

In this case, it can be hard to read the Go struct and understand what the corresponding YAML should look like. Is it image: or builderImage: or builder:? source: or gitSource:? strategy: or strategyRef:?

We were going back and forth on this in the initial days. Fixing this sounds like a good thing to do.

This could be aligned in a backward-compatible* change, by renaming the Go fields/structs to align with the JSON/YAML struct tags. This could be a good-first-issue even on the pre-beta timeline.
*Tekton has defined backward compatibility to mean "YAML/JSON that used to work still works", not necessarily that Go code still compiles.

Agreed.
Yeah, I wouldn't bother about go code compatibility here. Our project is too young to make that promise.

[sbose78]

Possibly #224 ?

Actually a different one :(
@gabemontero had brought this up and there was great agreement on this.

[qu1queee]

@adambkaplan wondering if this one should not belong to the v0.4.0 milestone, it feels like this is a "living" issue that should continue open for more weeks?

[gabemontero]

@adambkaplan wondering if this one should not belong to the v0.4.0 milestone, it feels like this is a "living" issue that should continue open for more weeks?

I agree @qu1queee . I'm moving it out while Adam is on PTO. If he feels strongly about it when he gets back, we can go from there.

[imjasonh]

@gabemontero also points out that ObjectReference is discouraged

// ObjectReference contains enough information to let you inspect or modify the referred object.
// ---
// New uses of this type are discouraged because of difficulty describing its usage when embedded in APIs.
//  1. Ignored fields.  It includes many fields which are not generally honored.  For instance, ResourceVersion and FieldPath are both very rarely valid in actual usage.
//  2. Invalid usage help.  It is impossible to add specific help for individual usage.  In most embedded usages, there are particular
//     restrictions like, "must refer only to types A and B" or "UID not honored" or "name must be restricted".
//     Those cannot be well described when embedded.
//  3. Inconsistent validation.  Because the usages are different, the validation rules are different by usage, which makes it hard for users to predict what will happen.
//  4. The fields are both imprecise and overly precise.  Kind is not a precise mapping to a URL. This can produce ambiguity
//     during interpretation and require a REST mapping.  In most cases, the dependency is on the group,resource tuple
//     and the version of the actual struct is irrelevant.
//  5. We cannot easily change it.  Because this type is embedded in many locations, updates to this type
//     will affect numerous schemas.  Don't make new APIs embed an underspecified API type they do not control.
// Instead of using this type, create a locally provided and used type that is well-focused on your reference.
// For example, ServiceReferences for admission registration: https://github.com/kubernetes/api/blob/release-1.17/admissionregistration/v1/types.go#L533 .

We use LocalObjectReference to reference secrets in a few places, for example here

[gabemontero]

@gabemontero also points out that ObjectReference is discouraged

// ObjectReference contains enough information to let you inspect or modify the referred object.
// ---
// New uses of this type are discouraged because of difficulty describing its usage when embedded in APIs.
//  1. Ignored fields.  It includes many fields which are not generally honored.  For instance, ResourceVersion and FieldPath are both very rarely valid in actual usage.
//  2. Invalid usage help.  It is impossible to add specific help for individual usage.  In most embedded usages, there are particular
//     restrictions like, "must refer only to types A and B" or "UID not honored" or "name must be restricted".
//     Those cannot be well described when embedded.
//  3. Inconsistent validation.  Because the usages are different, the validation rules are different by usage, which makes it hard for users to predict what will happen.
//  4. The fields are both imprecise and overly precise.  Kind is not a precise mapping to a URL. This can produce ambiguity
//     during interpretation and require a REST mapping.  In most cases, the dependency is on the group,resource tuple
//     and the version of the actual struct is irrelevant.
//  5. We cannot easily change it.  Because this type is embedded in many locations, updates to this type
//     will affect numerous schemas.  Don't make new APIs embed an underspecified API type they do not control.
// Instead of using this type, create a locally provided and used type that is well-focused on your reference.
// For example, ServiceReferences for admission registration: https://github.com/kubernetes/api/blob/release-1.17/admissionregistration/v1/types.go#L533 .

We use LocalObjectReference to reference secrets in a few places, for example here

Small clarification ... I believeLocalObjectReference is OK, given it is vary narrowly scoped, and that you have to explicitly opt into things like api version, kind in the name for it to resolve, like /api/v1/pods/some-name:

// LocalObjectReference contains enough information to let you locate the
// referenced object inside the same namespace.
type LocalObjectReference struct {
	// Name of the referent.
	// More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
	// TODO: Add other useful fields. apiVersion, kind, uid?
	// +optional
	Name string `json:"name,omitempty" protobuf:"bytes,1,opt,name=name"`
}

It is only ObjectReference with its explicit fields for each element (api, kind, etc.) where I think the issue arises.

[imjasonh]

I think the guidance is still reasonable, especially:

// Instead of using this type, create a locally provided and used type that is well-focused on your reference.

paired with

// TODO: Add other useful fields. apiVersion, kind, uid?

That TODO makes me worried that LocalObjectReference might grow to have all the problematic fields of ObjectReference.

This is basically just another instance of the problem outlined in #516 (comment) -- we shouldn't use k8s built-in types we don't control. It's easier to make this switch sooner rather than later.

[qu1queee]

@imjasonh from your comment, what does script support in Tekton means? Regarding the usage of corev1.Container, we can certainly move to a slimmer struct, however I dont see this as a critical todo item. What we can do is to layout first which fields under corev1.Container we need and which ones not, and then add this to our list of changes before GA.

[imjasonh]

@imjasonh from your comment, what does script support in Tekton means? Regarding the usage of corev1.Container, we can certainly move to a slimmer struct, however I dont see this as a critical todo item. What we can do is to layout first which fields under corev1.Container we need and which ones not, and then add this to our list of changes before GA.

Tekton supports a script field in the Step type, that is a bit nicer to use than command/args provided by v1.Container by default.

https://tekton.dev/docs/pipelines/tasks/#running-scripts-within-steps

I agree about moving off v1.Container not being high priority, but I do think it should be relatively non-distruptive to users, so I don't think we need to hold it for GA/v1 necessarily.

[qu1queee]

From grooming:

• We need to define the whole API for spec.sources
• Consider removing the service account support . We still have a blocker here, @imjasonh to provide more context.
• On spec.dockerfile, consider removing it in favor of params or should it remain there?
• Status Results for BuildRuns proposal SHIP-0023: Add SHIP for BuildRun Status Results community#13

[imjasonh]

• Consider removing the service account support . We still have a blocker here, @imjasonh to provide more context.

This started in #662 and resulted in #679

Generating service accounts is an unusual feature, which requires users grant Shipwright massive permissions inside the cluster, and should be deprecated before we consider the v1alpha1 API to be stable and close this issue.

[SaschaSchwarze0]

Done with https://hackmd.io/BsPjvGFwQgmVeJRe6FJfMA