Add decorator to remove namespace from ClusterRole and ClusterRoleBin…

[mcruzdev]

• Fixes: quarkus.kubernetes.namespace should not be applied to clustered resources #43464

[quarkus-bot]

Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

• title should not end up with ellipsis (make sure the title is complete)

_{This message is automatically generated by a bot.}

[metacosm]

While this solution probably works, I would rather we don't add the namespace to clustered resources in the first place instead of adding it and then removing it.

[mcruzdev]

I think adding the AddClusterRole... after AddNamespaceDecorator, works well.

Thank you for this comment @metacosm, I solved it with before method!

[metacosm]

Sorry to be a pain but I think the AddNamespaceDecorator should actually be rewritten completely to deal with this instead of trying to order things in such a way that things work. Maybe the decorator should only target resources that implement Namespaced so that we know for sure that processed resources need a namespace?

[mcruzdev]

No worry, I was trying to solve it without big changes, but I agree with you, to deal with order is a problem. I will try to solve in a better way, thank you for the help @metacosm!

[mcruzdev]

Hi @metacosm, I tried a lot to solve this one without to use after() method... I think that the previous way of configuring order is still better. Do you know another way to do the samething?

[mcruzdev]

Hi @metacosm, I didn't want to have to remove and then add. Do you know, another way?

[metacosm]

I would do something like:

final var updated = list.buildItems().stream()
                .filter(Namespaced.class::isInstance)
                .peek(o -> o.getMetadata().edit().withNamespace(namespace).build())
                .toList();
list.withItems(updated);

but I haven't checked if this actually works. This has the advantage to not require maintaining a list of clustered resources. Ideally, we wouldn't need to build the items in the first place but I'm not sure this is avoidable. withItems resets the list completely so it should be faster than first removing then adding the items again. Maybe @iocanel has a better solution?

[mcruzdev]

With this aproach:

                .filter(Namespaced.class::isInstance)

does not work 😢

[mcruzdev]

I don't like using .set. I prefer the object to manage its own state, but it's not working 😞

[metacosm]

I'm surprised that it doesn't work. I'll check tomorrow.

[metacosm]

At the very least, you don't need this: all the items in the list should be HasMetadata instances.

[mcruzdev]

Yeah!

[metacosm]

This should work:

public void visit(KubernetesListBuilder list) {
        final var updated = list.buildItems().stream()
                .peek(o -> {
                    if (o instanceof Namespaced) {
                        final ObjectMeta metadata = o.getMetadata();
                        // only set the namespace if it hasn't already been otherwise explicitly set
                        if (metadata.getNamespace() == null) {
                            metadata.setNamespace(namespace);
                            o.setMetadata(metadata);
                        }
                    }
                })
                .toList();
        list.withItems(updated);
    }

The previous version was filtering so the updated list didn't contain any of the cluster-scoped resources.

[metacosm]

The PR should be renamed more appropriately as well, since the PR is not adding a new decorator, simply fixing the existing one.

[iocanel]

I really don't like it when a visitor operates on levels lower than its target, as it makes it really hard to order things.

I am wondering if it would be possible to crete a new kind of decroator based on sundrio's PathAwareTypedVisitor that allow devs to look up the parent node and / or the parent type.

If this is not possible, then we shall proceed with this approach.

[metacosm]

I really don't like it when a visitor operates on levels lower than its target, as it makes it really hard to order things.

I'm not sure I understand what you mean here.

I am wondering if it would be possible to crete a new kind of decroator based on sundrio's PathAwareTypedVisitor that allow devs to look up the parent node and / or the parent type.

That would require a new Dekorate release, though. And I'm not sure what benefits that would bring. How looking up the parent would help in this case? Do you mean to look up the resource object from its ObjectMeta?
The current approach seems simpler with the downside of requiring building the objects instead of working directly on builders.

[mcruzdev]

I tried to do this one and it requires a new implementation/release at Dekorate side.

[mcruzdev]

Hi @iocanel, @metacosm what is the decision?

[iocanel]

The suggested change not only requires a decorate release, but it requires a major release that is also not trivial.

So, given that it will require time, I am going to accept this.

[mcruzdev]

Hi @iocanel @metacosm, sorry for disturb you, but this one can be merged?

[metacosm]

@mcruzdev I agree but this isn't my call to make 🙍🏼

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 34bb99d.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.