Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
3 changes: 3 additions & 0 deletions cmd/fetcher/kodata/config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,19 @@ ingress:
- s3:
bucket: "gs-noauth://knative-releases"
prefix: "net-istio/previous"
eventingService: istio

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 there a reason why this is called eventingService? I was first confused with Eventing as we are in the knative-serving part of the yaml.

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.

IngressService or just ingress?

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.

@ReToCode @skonto I changed the name of the field.

include:
- "net-istio.yaml"
- s3:
bucket: "gs-noauth://knative-releases"
prefix: "net-contour/previous"
eventingService: contour
include:
- "net-contour.yaml"
- s3:
bucket: "gs-noauth://knative-releases"
prefix: "net-kourier/previous"
eventingService: kourier
include:
- "kourier.yaml"
knative-eventing:
Expand Down
6 changes: 3 additions & 3 deletions pkg/reconciler/common/releases_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -246,11 +246,11 @@ func TestTargetVersion(t *testing.T) {
component: &v1beta1.KnativeServing{
Spec: v1beta1.KnativeServingSpec{
CommonSpec: base.CommonSpec{
Version: "0.22.0",
Version: "1.9.0",
},
},
},
expected: "0.22.0",
expected: "1.9.0",
}, {
name: "eventing CR with major.minor.patch version not available",
component: &v1beta1.KnativeEventing{
Expand Down Expand Up @@ -1135,7 +1135,7 @@ func TestInstalledManifest(t *testing.T) {
},
},
Status: v1beta1.KnativeServingStatus{
Version: "0.22.0",
Version: "1.9.0",
},
},
expectedManifestsPath: "testdata/kodata/empty/empty-resource.yaml",
Expand Down
2 changes: 0 additions & 2 deletions pkg/reconciler/knativeserving/ingress/contour.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ import (
"knative.dev/operator/pkg/apis/operator/v1beta1"
)

var contourFilter = ingressFilter("contour")

func contourTransformers(ctx context.Context, instance *v1beta1.KnativeServing) []mf.Transformer {
return nil
}
117 changes: 54 additions & 63 deletions pkg/reconciler/knativeserving/ingress/ingress.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,51 +24,13 @@ import (

mf "github.com/manifestival/manifestival"
"golang.org/x/mod/semver"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"knative.dev/operator/pkg/apis/operator/base"
"knative.dev/operator/pkg/apis/operator/v1beta1"
"knative.dev/operator/pkg/reconciler/common"
)

const providerLabel = "networking.knative.dev/ingress-provider"

func ingressFilter(name string) mf.Predicate {
return func(u *unstructured.Unstructured) bool {
provider, hasLabel := u.GetLabels()[providerLabel]
if !hasLabel {
return true
}
return provider == name
}
}

// noneFilter drops all ingresses but allows everything else.
func noneFilter(u *unstructured.Unstructured) bool {
_, hasLabel := u.GetLabels()[providerLabel]
return !hasLabel
}

// Filters makes sure the disabled ingress resources are removed from the manifest.
func Filters(ks *v1beta1.KnativeServing) mf.Predicate {
var filters []mf.Predicate
if ks.Spec.Ingress == nil {
return istioFilter
}
if ks.Spec.Ingress.Istio.Enabled {
filters = append(filters, istioFilter)
}
if ks.Spec.Ingress.Kourier.Enabled {
filters = append(filters, kourierFilter)
}
if ks.Spec.Ingress.Contour.Enabled {
filters = append(filters, contourFilter)
}
if len(filters) == 0 {
return noneFilter
}
return mf.Any(filters...)
}

// Transformers returns a list of transformers based on the enabled ingresses
func Transformers(ctx context.Context, ks *v1beta1.KnativeServing) []mf.Transformer {
if ks.Spec.Ingress == nil {
Expand All @@ -87,61 +49,90 @@ func Transformers(ctx context.Context, ks *v1beta1.KnativeServing) []mf.Transfor
return transformers
}

func getIngress(version string) (mf.Manifest, error) {
// If we can not determine the version, append no ingress manifest.
if version == "" {
func getIngress(path string) (mf.Manifest, error) {
if path == "" {
return mf.Manifest{}, nil
}
return common.FetchManifest(path)
}

func getIngressPath(version string, ks *v1beta1.KnativeServing) string {
var urls []string
koDataDir := os.Getenv(common.KoEnvKey)
// Ingresses are saved in the directory named major.minor. We remove the patch number.
ingressVersion := common.LATEST_VERSION
sourceVersion := common.LATEST_VERSION
if !strings.EqualFold(version, common.LATEST_VERSION) {
ingressVersion = semver.MajorMinor(common.SanitizeSemver(version))[1:]
sourceVersion = semver.MajorMinor(common.SanitizeSemver(version))[1:]
}

// This line can make sure a valid available source version is returned.
ingressPath := filepath.Join(koDataDir, "ingress", sourceVersion)
if ks.Spec.Ingress == nil {
url := filepath.Join(ingressPath, "istio")
urls = append(urls, url)
return strings.Join(urls, common.COMMA)
}

if ks.Spec.Ingress.Istio.Enabled {
url := filepath.Join(ingressPath, "istio")
urls = append(urls, url)
}
if ks.Spec.Ingress.Contour.Enabled {
url := filepath.Join(ingressPath, "contour")
urls = append(urls, url)
}
if ks.Spec.Ingress.Kourier.Enabled {
url := filepath.Join(ingressPath, "kourier")
urls = append(urls, url)
}

// This line can make sure a valid available ingress version is returned.
ingressVersion = common.GetLatestIngressRelease(ingressVersion)
ingressPath := filepath.Join(koDataDir, "ingress", ingressVersion)
return common.FetchManifest(ingressPath)
if len(urls) == 0 {
url := filepath.Join(ingressPath, "istio")

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.

We have the default behavior around L#67 so it should be fine, I think. But currently (without this PR) nonFilter was returned and no ingress was created. So, it should be same behavior instead of installing Istio?

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.

Right. Let me change it. I can add some more test cases to see if the edge cases are covered.

urls = append(urls, url)
}

return strings.Join(urls, common.COMMA)
}

// AppendTargetIngresses appends the manifests of ingresses to be installed
func AppendTargetIngresses(ctx context.Context, manifest *mf.Manifest, instance base.KComponent) error {
m, err := getIngress(common.TargetVersion(instance))
// AppendTargetIngress appends the manifests of the ingress to be installed
func AppendTargetIngress(ctx context.Context, manifest *mf.Manifest, instance base.KComponent) error {
version := common.TargetVersion(instance)
ingressPath := getIngressPath(version, convertToKS(instance))
m, err := getIngress(ingressPath)
if err == nil {
*manifest = manifest.Append(m)
}

if len(instance.GetSpec().GetManifests()) != 0 {
// If spec.manifests is not empty, it is possible that the ingress is not available with the specified version.
// The user can specify the ingress link in the spec.manifests.
// If spec.manifests is not empty, it is possible that the eventing source is not available with the
// specified version. The user can specify the eventing source link in the spec.manifests.
return nil
}
return err
}

// AppendInstalledIngresses appends the installed manifests of ingresses
// AppendInstalledIngresses appends all the manifests of the ingresses
func AppendInstalledIngresses(ctx context.Context, manifest *mf.Manifest, instance base.KComponent) error {
version := instance.GetStatus().GetVersion()
if version == "" {
version = common.TargetVersion(instance)
}

m, err := getIngress(version)
ingressPath := getIngressPath(version, convertToKS(instance))
m, err := getIngress(ingressPath)
if err == nil {
*manifest = manifest.Append(m)
}

// It is possible that the ingress is not available with the specified version.
// If the user specified a version with a minor version, which is not supported by the current operator, as long as
// spec.manifests contains all the manifest links, the operator can still work. This function can always return nil,
// If the user specified a version with a minor version, which is not supported by the current operator, the operator
// can still work, as long as spec.manifests contains all the manifest links. This function can always return nil,
// even if the ingress is not available.
return nil
}

func hasProviderLabel(u *unstructured.Unstructured) bool {
if _, hasLabel := u.GetLabels()[providerLabel]; hasLabel {
return true
func convertToKS(instance base.KComponent) *v1beta1.KnativeServing {

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.

Exact same function exists in ./pkg/reconciler/knativeserving/security/security.go can we change it a common func and use it?

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.

good point.

ks := &v1beta1.KnativeServing{}
switch instance := instance.(type) {
case *v1beta1.KnativeServing:
ks = instance
}
return false
return ks
}
Loading