Skip to content
This repository was archived by the owner on Jun 24, 2020. It is now read-only.
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
13 changes: 13 additions & 0 deletions Gopkg.lock

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

1 change: 1 addition & 0 deletions Gopkg.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ required = [
"k8s.io/code-generator/cmd/informer-gen",
"k8s.io/kube-openapi/cmd/openapi-gen",
"k8s.io/gengo/args",
"knative.dev/caching/pkg/apis/caching/v1alpha1",
"github.com/knative/test-infra/scripts",
"github.com/knative/test-infra/tools/dep-collector",
"sigs.k8s.io/controller-tools/pkg/crd/generator",
Expand Down
14 changes: 14 additions & 0 deletions config/crds/serving_v1alpha1_knativeserving_crd.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,20 @@ spec:
description: A means to override the corresponding entries in the upstream
configmaps
type: object
registry:
description: A means to override the corresponding deployment images in the upstream.
This affects both apps/v1.Deployment and caching.internal.knative.dev/v1alpha1.Image.
type: object
properties:
default:
description: The default image reference template to use for all knative images.
Takes the form of example-registry.io/custom/path/${NAME}:custom-tag
type: string
override:
description: A map of a container name or image name to the full image location of the individual knative image.
type: object
additionalProperties:
type: string
type: object
status:
description: Status defines the observed state of KnativeServing
Expand Down
24 changes: 23 additions & 1 deletion pkg/apis/serving/v1alpha1/knativeserving_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ limitations under the License.
package v1alpha1

import (
"knative.dev/pkg/apis"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"knative.dev/pkg/apis"
)

// EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN!
Expand All @@ -28,6 +28,23 @@ const (
DeploymentsAvailable apis.ConditionType = "DeploymentsAvailable"
)

// Registry defines image overrides of knative images.
// This affects both apps/v1.Deployment and caching.internal.knative.dev/v1alpha1.Image.
// The default value is used as a default format to override for all knative deployments.
// The override values are specific to each knative deployment.
// +k8s:openapi-gen=true
type Registry struct {
// The default image reference template to use for all knative images.
// It takes the form of example-registry.io/custom/path/${NAME}:custom-tag
// ${NAME} will be replaced by the deployment container name, or caching.internal.knative.dev/v1alpha1/Image name.
// +optional
Default string `json:"default,omitempty"`

// A map of a container name or image name to the full image location of the individual knative image.
// +optional
Override map[string]string `json:"override,omitempty"`
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is the "Default*" prefix meant to imply? Why not just Name and Tag as the only elements of the Registry struct?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Per comment #20 (comment), it demonstrates the that this is the default registry and tag to use. I am starting to prefer the nested default map.


// KnativeServingSpec defines the desired state of KnativeServing
// +k8s:openapi-gen=true
type KnativeServingSpec struct {
Expand All @@ -38,6 +55,11 @@ type KnativeServingSpec struct {
// A means to override the corresponding entries in the upstream configmaps
// +optional
Config map[string]map[string]string `json:"config,omitempty"`

// A means to override the corresponding deployment images in the upstream.
// If no registry is provided, the knative release images will be used.
// +optional
Registry Registry `json:"registry,omitempty"`
}

// KnativeServingStatus defines the observed state of KnativeServing
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import (
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
)

// Set some data in a configmap, only overwriting common keys if they differ
// UpdateConfigMap set some data in a configmap, only overwriting common keys if they differ
func UpdateConfigMap(cm *unstructured.Unstructured, data map[string]string, log logr.Logger) {
for k, v := range data {
message := []interface{}{"map", cm.GetName(), k, v}
Expand Down
48 changes: 46 additions & 2 deletions pkg/controller/knativeserving/common/extensions.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@ package common
import (
mf "github.com/jcrossley3/manifestival"
servingv1alpha1 "github.com/knative/serving-operator/pkg/apis/serving/v1alpha1"
appsv1 "k8s.io/api/apps/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
caching "knative.dev/caching/pkg/apis/caching/v1alpha1"
"sigs.k8s.io/controller-runtime/pkg/client"
logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
)
Expand Down Expand Up @@ -48,16 +50,24 @@ func (platforms Platforms) Extend(c client.Client, scheme *runtime.Scheme) (resu
return
}

func (exts Extensions) Transform(instance *servingv1alpha1.KnativeServing) []mf.Transformer {
func (exts Extensions) Transform(scheme *runtime.Scheme, instance *servingv1alpha1.KnativeServing) []mf.Transformer {
log.V(1).Info("Transforming", "instance", instance)
result := []mf.Transformer{
mf.InjectOwner(instance),
mf.InjectNamespace(instance.GetNamespace()),
}
for _, extension := range exts {
result = append(result, extension.Transformers...)
}
// Let any config in instance override everything else
return append(result, func(u *unstructured.Unstructured) error {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we start adding more conditions in here, it might make more sense to define mf.Transformer functions elsewhere in the file and reference them here, similar to the mf.Inject* functions above (though they wouldn't have to be closures). I'll let you decide whether we do that now or later.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Right, if manifestival could supply something like InjectImageURL, that's will be great.

// Update the deployment with the new registry and tag
if u.GetAPIVersion() == "caching.internal.knative.dev/v1alpha1" && u.GetKind() == "Image" {
updateCachingImage(scheme, instance, u)
}
if u.GetKind() == "Deployment" {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The comment on line 60 should get moved in to this function block now since it is not purely about config overriding

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think it's one challenge here, just consider the image url in deployment.
Actually, there is image url in configmap, event in args in pod definition.
Could we have a common solution to pick them up and do replacement, use regex or something.

Copy link
Copy Markdown
Contributor Author

@trshafer trshafer Jul 2, 2019

Choose a reason for hiding this comment

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

I'm concerned with a common solution that supports replacing all possible values. I think if the configmap values need to be changed, they should use the config key that mutates configmaps.

The Image kind needs to be able to be updated though.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated to support Image

updateDeployment(scheme, instance, u)
}
// Let any config in instance override everything else
if u.GetKind() == "ConfigMap" {
if data, ok := instance.Spec.Config[u.GetName()[len(`config-`):]]; ok {
UpdateConfigMap(u, data, log)
Expand All @@ -67,6 +77,40 @@ func (exts Extensions) Transform(instance *servingv1alpha1.KnativeServing) []mf.
})
}

func updateCachingImage(scheme *runtime.Scheme, instance *servingv1alpha1.KnativeServing, u *unstructured.Unstructured) error {
var image = &caching.Image{}
err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, image)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'll look into why scheme.Convert doesn't work after this is merged.

if err != nil {
log.Error(err, "Error converting Unstructured to Image", "unstructured", u, "image", image)
return err
}

registry := instance.Spec.Registry
log.V(1).Info("Updating Image", "name", u.GetName(), "registry", registry)

UpdateImageSpec(image, &registry, log)
scheme.Convert(image, u, nil)
log.V(1).Info("Finished conversion", "name", u.GetName(), "unstructured", u.Object)
return nil
}

func updateDeployment(scheme *runtime.Scheme, instance *servingv1alpha1.KnativeServing, u *unstructured.Unstructured) error {
var deployment = &appsv1.Deployment{}
err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, deployment)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this better than scheme.Convert(u, deployment)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I couldn't get scheme.Convert(u, deployment) to work. I got error message TypeMeta not present in src which comes from https://github.com/kubernetes/apimachinery/blob/961b39a1baa06f6c52bdd048a809b9f5b47f1337/pkg/conversion/converter.go#L828. The issue I believe is TypeMeta

metav1.TypeMeta `json:",inline"`
is not in the unstructured map. It's unable to take the inline json content and convert it to a Deployment. This way worked.

I'll look more into the scheme tests and see if I can reproduce this issue: https://github.com/kubernetes/apimachinery/blob/961b39a1baa06f6c52bdd048a809b9f5b47f1337/pkg/runtime/scheme_test.go#L1

if err != nil {
log.Error(err, "Error converting Unstructured to Deployment", "unstructured", u, "deployment", deployment)
return err
}

registry := instance.Spec.Registry
log.V(1).Info("Updating Deployment", "name", u.GetName(), "registry", registry)

UpdateDeploymentImage(deployment, &registry, log)
scheme.Convert(deployment, u, nil)
log.V(1).Info("Finished conversion", "name", u.GetName(), "unstructured", u.Object)
return nil
}

func (exts Extensions) PreInstall(instance *servingv1alpha1.KnativeServing) error {
for _, extension := range exts {
for _, f := range extension.PreInstalls {
Expand Down
72 changes: 72 additions & 0 deletions pkg/controller/knativeserving/common/images.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/*
Copyright 2019 The Knative Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
package common

import (
"fmt"
"strings"

"github.com/go-logr/logr"
servingv1alpha1 "github.com/knative/serving-operator/pkg/apis/serving/v1alpha1"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
caching "knative.dev/caching/pkg/apis/caching/v1alpha1"
)

var (
// The string to be replaced by the container name
containerNameVariable = "${NAME}"
)

// UpdateDeploymentImage updates the image of the deployment with a new registry and tag
func UpdateDeploymentImage(deployment *appsv1.Deployment, registry *servingv1alpha1.Registry, log logr.Logger) {
containers := deployment.Spec.Template.Spec.Containers
for index := range containers {
container := &containers[index]
newImage := getNewImage(registry, container.Name)
if newImage != "" {
updateContainer(container, newImage, log)
}
}
log.V(1).Info("Finished updating images", "deployment", deployment.GetName())
}

// UpdateImageSpec updates the image of a with a new registry and tag
func UpdateImageSpec(image *caching.Image, registry *servingv1alpha1.Registry, log logr.Logger) {
newImage := getNewImage(registry, image.Name)
if newImage != "" {
log.V(1).Info(fmt.Sprintf("Updating image from: %v, to: %v", image.Spec.Image, newImage))
image.Spec.Image = newImage
}
log.V(1).Info("Finished updating image", "image", image.GetName())
}

func getNewImage(registry *servingv1alpha1.Registry, containerName string) string {
overrideImage := registry.Override[containerName]
if overrideImage != "" {
return overrideImage
}
return replaceName(registry.Default, containerName)
}

func updateContainer(container *corev1.Container, newImage string, log logr.Logger) {
log.V(1).Info(fmt.Sprintf("Updating container image from: %v, to: %v", container.Image, newImage))
container.Image = newImage
}

func replaceName(imageTemplate string, name string) string {
return strings.ReplaceAll(imageTemplate, containerNameVariable, name)
}
Loading