wmedvede commented on code in PR #3013:
URL:
https://github.com/apache/incubator-kie-tools/pull/3013#discussion_r2023256325
##########
packages/sonataflow-operator/internal/controller/sonataflow_controller.go:
##########
@@ -118,6 +122,11 @@ func (r *SonataFlowReconciler) Reconcile(ctx
context.Context, req ctrl.Request)
return ctrl.Result{}, err
}
+ if err := r.Validate(ctx, workflow, req); err != nil {
+ klog.V(log.E).ErrorS(err, "Failed to validate SonataFlow")
Review Comment:
I think the error message should include the name / namespace of the failing
workflow. That information will be very helpful when we need to see the logs in
case of failues.
##########
packages/sonataflow-operator/internal/controller/validation/ImageValidator.go:
##########
@@ -0,0 +1,128 @@
+// Copyright 2024 Apache Software Foundation (ASF)
+//
+// 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 validation
+
+import (
+ "archive/tar"
+ "context"
+ "fmt"
+ "io"
+ "regexp"
+
+ "github.com/google/go-cmp/cmp"
+ "github.com/google/go-containerregistry/pkg/name"
+ v1 "github.com/google/go-containerregistry/pkg/v1"
+ "github.com/google/go-containerregistry/pkg/v1/remote"
+ "k8s.io/apimachinery/pkg/util/yaml"
+ ctrl "sigs.k8s.io/controller-runtime"
+ "sigs.k8s.io/controller-runtime/pkg/client"
+
+ operatorapi
"github.com/apache/incubator-kie-tools/packages/sonataflow-operator/api/v1alpha08"
+)
+
+type ImageValidator struct{}
+
+var pattern = `^.*\.sw\.(yaml|yml|json)$`
+
+var readablePattern = `*.sw.(yaml|yml|json)`
+var compiled = regexp.MustCompile(pattern)
+
+func (v ImageValidator) Validate(ctx context.Context, client client.Client,
sonataflow *operatorapi.SonataFlow, req ctrl.Request) error {
+ if sonataflow.HasContainerSpecImage() {
+ err := client.Get(ctx, req.NamespacedName, sonataflow)
Review Comment:
If we are validating a `workflow' that was just read the instant before
calling the Validators sequence, I don't believe we need to re-read it again.
But if you do, I don't believe we should read it and storing the value in
the parameter sonataflow *operatorapi.SonataFlow paramter, I think this could
make the validator has side effects on that variable value. When multiple
validators are executed I think this code wont be clear.
Another nitpick, when we do reading of the resource as part of the
reconciliation, we normally guard the code with something like this:
```
err := r.Client.Get(ctx, req.NamespacedName, workflow)
if err != nil {
if errors.IsNotFound(err) {
not good, return the error or whatever, but we know
exactly that the given resource don't exist at this time.
}
not good, return the error or whatever, something went wrong,
we don't know what.
}
```
##########
packages/sonataflow-operator/internal/controller/validation/Validator.go:
##########
@@ -0,0 +1,41 @@
+// Copyright 2024 Apache Software Foundation (ASF)
+//
+// 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 validation
+
+import (
+ "context"
+
+ ctrl "sigs.k8s.io/controller-runtime"
+ "sigs.k8s.io/controller-runtime/pkg/client"
+
+ operatorapi
"github.com/apache/incubator-kie-tools/packages/sonataflow-operator/api/v1alpha08"
+)
+
+var validators = []Validator{ImageValidator{}}
+
+type Validator interface {
Review Comment:
Nice approach!
##########
packages/sonataflow-operator/internal/controller/validation/Validator.go:
##########
@@ -0,0 +1,41 @@
+// Copyright 2024 Apache Software Foundation (ASF)
+//
+// 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 validation
+
+import (
+ "context"
+
+ ctrl "sigs.k8s.io/controller-runtime"
+ "sigs.k8s.io/controller-runtime/pkg/client"
+
+ operatorapi
"github.com/apache/incubator-kie-tools/packages/sonataflow-operator/api/v1alpha08"
+)
+
+var validators = []Validator{ImageValidator{}}
+
+type Validator interface {
+ Validate(ctx context.Context, client client.Client, sonataflow
*operatorapi.SonataFlow, req ctrl.Request) error
+}
+
+// validate the SonataFlow object, right now it's only check if workflow is in
deployment has image declared as that image
Review Comment:
nitpick, validate here, goes with uppercase Validate instead.
##########
packages/sonataflow-operator/internal/controller/validation/ImageValidator.go:
##########
@@ -0,0 +1,128 @@
+// Copyright 2024 Apache Software Foundation (ASF)
Review Comment:
wrong licence format, copyright not needed
##########
packages/sonataflow-operator/internal/controller/validation/ImageValidator.go:
##########
@@ -0,0 +1,128 @@
+// Copyright 2024 Apache Software Foundation (ASF)
+//
+// 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 validation
+
+import (
+ "archive/tar"
+ "context"
+ "fmt"
+ "io"
+ "regexp"
+
+ "github.com/google/go-cmp/cmp"
+ "github.com/google/go-containerregistry/pkg/name"
+ v1 "github.com/google/go-containerregistry/pkg/v1"
+ "github.com/google/go-containerregistry/pkg/v1/remote"
+ "k8s.io/apimachinery/pkg/util/yaml"
+ ctrl "sigs.k8s.io/controller-runtime"
+ "sigs.k8s.io/controller-runtime/pkg/client"
+
+ operatorapi
"github.com/apache/incubator-kie-tools/packages/sonataflow-operator/api/v1alpha08"
+)
+
+type ImageValidator struct{}
+
+var pattern = `^.*\.sw\.(yaml|yml|json)$`
+
+var readablePattern = `*.sw.(yaml|yml|json)`
+var compiled = regexp.MustCompile(pattern)
+
+func (v ImageValidator) Validate(ctx context.Context, client client.Client,
sonataflow *operatorapi.SonataFlow, req ctrl.Request) error {
+ if sonataflow.HasContainerSpecImage() {
+ err := client.Get(ctx, req.NamespacedName, sonataflow)
+ equals, err := validateImage(sonataflow,
sonataflow.Spec.PodTemplate.Container.Image)
+ if err != nil {
+ return err
+ }
+ if !equals {
+ return fmt.Errorf("Workflow, defined in the image %s
doesn't match deployment workflow", sonataflow.Spec.PodTemplate.Container.Image)
+ }
+ }
+ return nil
+}
+
+func validateImage(sonataflow *operatorapi.SonataFlow, image string) (bool,
error) {
+ imageRef, err := name.ParseReference(image)
+ if err != nil {
+ return false, err
+ }
+
+ ref, err := remote.Image(imageRef)
+ if err != nil {
+ return false, err
+ }
+
+ reader, err := readLayers(ref)
+ if err != nil {
+ return false, err
+ }
+
+ workflowDockerImage, err := jsonFromDockerImage(reader)
+ if err != nil {
+ return false, err
+ }
+
+ return cmp.Equal(workflowDockerImage, sonataflow.Spec.Flow), nil
+}
+
+func readLayers(image v1.Image) (*tar.Reader, error) {
+ layers, err := image.Layers()
+ if err != nil {
+ return nil, err
+ }
+
+ for i := len(layers) - 1; i >= 0; i-- {
+ if reader, err := readLayer(layers[i]); err == nil && reader !=
nil {
+ return reader, nil
+ } else if err != nil {
+ return nil, err
+ }
+ }
+ return nil, fmt.Errorf("file matching the pattern %s, not found in
container image", readablePattern)
+}
+
+func readLayer(layer v1.Layer) (*tar.Reader, error) {
+ uncompressedLayer, err := layer.Uncompressed()
+ if err != nil {
+ return nil, fmt.Errorf("failed to get uncompressed layer: %v",
err)
+ }
+ defer uncompressedLayer.Close()
+
+ tarReader := tar.NewReader(uncompressedLayer)
+ for {
+ header, err := tarReader.Next()
+ if err != nil {
+ if err.Error() == "EOF" {
+ break
+ }
+ return nil, fmt.Errorf("error reading tar: %v", err)
+ }
+
+ if header.Typeflag == '0' && compiled.MatchString(header.Name) {
+ return tarReader, nil
+ }
+ }
+
+ return nil, nil
+}
+
+func jsonFromDockerImage(reader io.Reader) (operatorapi.Flow, error) {
+ data, err := io.ReadAll(reader)
+ workflow := &operatorapi.Flow{}
+ if err = yaml.Unmarshal(data, workflow); err != nil {
Review Comment:
Excuse me if I'm missing something, but do this Unmarshalling work both when
the workflow inside the image is in json and yaml format? If so, maybe this
function can be called workflowFromDockerImage.
##########
packages/sonataflow-operator/internal/controller/sonataflow_controller.go:
##########
@@ -118,6 +122,11 @@ func (r *SonataFlowReconciler) Reconcile(ctx
context.Context, req ctrl.Request)
return ctrl.Result{}, err
}
+ if err := r.Validate(ctx, workflow, req); err != nil {
+ klog.V(log.E).ErrorS(err, "Failed to validate SonataFlow")
Review Comment:
This invocation to the validator should go after the code below.
That `if` indicates that the WF being deleted right now, so, we don't need
to validate it.
if workflow.DeletionTimestamp != nil {
return r.applyFinalizers(ctx, workflow)
}
##########
packages/sonataflow-operator/internal/controller/validation/ImageValidator.go:
##########
@@ -0,0 +1,128 @@
+// Copyright 2024 Apache Software Foundation (ASF)
+//
+// 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 validation
+
+import (
+ "archive/tar"
+ "context"
+ "fmt"
+ "io"
+ "regexp"
+
+ "github.com/google/go-cmp/cmp"
+ "github.com/google/go-containerregistry/pkg/name"
+ v1 "github.com/google/go-containerregistry/pkg/v1"
+ "github.com/google/go-containerregistry/pkg/v1/remote"
+ "k8s.io/apimachinery/pkg/util/yaml"
+ ctrl "sigs.k8s.io/controller-runtime"
+ "sigs.k8s.io/controller-runtime/pkg/client"
+
+ operatorapi
"github.com/apache/incubator-kie-tools/packages/sonataflow-operator/api/v1alpha08"
+)
+
+type ImageValidator struct{}
+
+var pattern = `^.*\.sw\.(yaml|yml|json)$`
+
+var readablePattern = `*.sw.(yaml|yml|json)`
+var compiled = regexp.MustCompile(pattern)
+
+func (v ImageValidator) Validate(ctx context.Context, client client.Client,
sonataflow *operatorapi.SonataFlow, req ctrl.Request) error {
+ if sonataflow.HasContainerSpecImage() {
+ err := client.Get(ctx, req.NamespacedName, sonataflow)
+ equals, err := validateImage(sonataflow,
sonataflow.Spec.PodTemplate.Container.Image)
Review Comment:
if we don't do the consideration above, in case of error the sonataflow
value is uncertain.
##########
packages/sonataflow-operator/internal/controller/validation/Validator.go:
##########
@@ -0,0 +1,41 @@
+// Copyright 2024 Apache Software Foundation (ASF)
Review Comment:
same here, copyright we don't need
##########
packages/sonataflow-operator/internal/controller/validation/ImageValidator.go:
##########
@@ -0,0 +1,128 @@
+// Copyright 2024 Apache Software Foundation (ASF)
+//
+// 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 validation
+
+import (
+ "archive/tar"
+ "context"
+ "fmt"
+ "io"
+ "regexp"
+
+ "github.com/google/go-cmp/cmp"
+ "github.com/google/go-containerregistry/pkg/name"
+ v1 "github.com/google/go-containerregistry/pkg/v1"
+ "github.com/google/go-containerregistry/pkg/v1/remote"
+ "k8s.io/apimachinery/pkg/util/yaml"
+ ctrl "sigs.k8s.io/controller-runtime"
+ "sigs.k8s.io/controller-runtime/pkg/client"
+
+ operatorapi
"github.com/apache/incubator-kie-tools/packages/sonataflow-operator/api/v1alpha08"
+)
+
+type ImageValidator struct{}
+
+var pattern = `^.*\.sw\.(yaml|yml|json)$`
+
+var readablePattern = `*.sw.(yaml|yml|json)`
+var compiled = regexp.MustCompile(pattern)
+
+func (v ImageValidator) Validate(ctx context.Context, client client.Client,
sonataflow *operatorapi.SonataFlow, req ctrl.Request) error {
+ if sonataflow.HasContainerSpecImage() {
+ err := client.Get(ctx, req.NamespacedName, sonataflow)
+ equals, err := validateImage(sonataflow,
sonataflow.Spec.PodTemplate.Container.Image)
+ if err != nil {
+ return err
+ }
+ if !equals {
+ return fmt.Errorf("Workflow, defined in the image %s
doesn't match deployment workflow", sonataflow.Spec.PodTemplate.Container.Image)
Review Comment:
workflow name/namepace would be nice to include in the message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
