Skip to content
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
54 changes: 54 additions & 0 deletions operator/v1alpha1/types_image_content_source_policy.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
package v1alpha1

import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

// +genclient
// +genclient:nonNamespaced
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

// ImageContentSourcePolicy holds cluster-wide information about how to handle registry mirror rules.
// When multple policies are defined, the outcome of the behavior is defined on each field.
type ImageContentSourcePolicy struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any other possible names we could use? Policy is fairly common. Is content source the best name? I don't want to be specific about "mirroring" or "disconnected", so trying out a name that implies "you can change where we source images" was the initial argument.

Maybe future might be "you have to have signatures from X for Y"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just ImageContentSource or ImageSources?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to ImageContentSource

Copy link
Contributor

Choose a reason for hiding this comment

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

By "policy is fairly common" I meant "It's good to have Policy in there to distinguish this from a more concrete object". I don't think you have to change the name, just want to see some suggestions about what names could be that better describe what this object does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed the name back

Copy link
Contributor

Choose a reason for hiding this comment

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

You’re right that “content source” is not a commonly used term for this. I’d just say one of “image {repositories,locations,(servers,storage?)}”; the “content” word adds nothing.


Per #354 (comment):

Do we want to document this as a general-purpose facility, or specifically restricted to OpenShift product images?

do we want this to be generic for all kinds of images, or actually say something entirely specific to the contemplated use, in the ballpark of OpenShiftReleasedImageLocations?

Copy link
Member

Choose a reason for hiding this comment

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

I think making it generic will allow us to reuse it for other use cases in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

The resource group usually contains the product namespace (operator.openshift.io) so it is redundant in the title.

metav1.TypeMeta `json:",inline"`
// Standard object's metadata.
metav1.ObjectMeta `json:"metadata,omitempty"`

// spec holds user settable values for configuration
// +required
Spec ImageContentSourcePolicySpec `json:"spec"`
}

// ImageContentSourcePolicySpec is the specification of the ImageContentSourcePolicy CRD.
type ImageContentSourcePolicySpec struct {
// repositoryDigestMirrors allows images referenced by image digests in pods to be
// pulled from alternative mirrored repository locations. The image pull specification
// provided to the pod will be compared to the source locations described in RepositoryDigestMirrors
// and the image may be pulled down from any of the repositories in the list instead of the
// specified repository allowing administrators to choose a potentially faster mirror.
// Only image pull specifications that have an image disgest will have this behavior applied
// to them - tags will continue to be pulled from the specified repository in the pull spec.
// When multiple policies are defined, any overlaps found will be merged together when the mirror
// rules are written to `/etc/containers/registries.conf`. For example, if policy A has sources `a, b, c`
// and policy B has sources `c, d, e`. Then the mirror rule written to `registries.conf` will be `a, b, c, d, e`
// where the duplicate `c` is removed.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is fine for now, I'm ok with discussing the policy in a follow up before 4.2 GA but after this feature has been prototyped.

// +optional
RepositoryDigestMirrors []RepositoryDigestMirrors `json:"repositoryDigestMirrors"`
}

// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

// ImageContentSourcePolicyList lists the items in the ImageContentSourcePolicy CRD.
type ImageContentSourcePolicyList struct {
metav1.TypeMeta `json:",inline"`
// Standard object's metadata.
metav1.ListMeta `json:"metadata"`
Items []ImageContentSourcePolicy `json:"items"`
}

// RepositoryDigestMirrors holds cluster-wide information about how to handle mirros in the registries config.
// Note: the mirrors only work when pulling the images that are reference by their digests.
type RepositoryDigestMirrors struct {
// sources are repositories that are mirrors of each other.
// +optional
Sources []string `json:"sources,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be a list of lists, the parent (in ImageContentSource.Spec) should be an array (instead of a nested struct).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, made repositoryDigestMirrors a list as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be a list of lists, the parent (in ImageContentSource.Spec) should be an array (instead of a nested struct).

There should be no canonical name for this - clients may create multiple of these, it is not a singleton. We want individual components to be able to register their policies and manage them independently

(I know very little about k8s API conventions): Isn’t a list of objects that contains a list of entries unnecessarily too much to iterate over? If a client can create multiple instances of ImageContentSourcePolicy, why does it need ImageContentSourcePolicySpec.RepositoryDigestMirrors to be an array?

Is there a semantic difference between a set of specs vs a single spec with an array?

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't force denormalization of ImageContentSourcePolicy-like objects into multiple objects because that puts an extra burden on users/integrators. The atomic unit of transaction is a single object in etcd, and so someone who wishes to add / remove a strict policy will naturally do so via GET/PUT operations on a single resource, where they can ensure the policy matches the desired state.

}
104 changes: 104 additions & 0 deletions operator/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

37 changes: 37 additions & 0 deletions operator/v1alpha1/zz_generated.swagger_doc_generated.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.