search

ory / oathkeeper-maester

Kuberenetes CRD Controller for Ory Oathkeeper. :warning: Maintained by the community, not an official Ory project!
Apache License 2.0
33 stars 20 forks source link

The field 'upstream' should not be required #43

Closed christian-roggia closed 3 years ago

christian-roggia commented 4 years ago

Currently, it is not possible to apply a Rule without the upstream field being set, this does not reflect what is mentioned in the documentation:

The location of the server where requests matching this rule should be forwarded to. This only needs to be set when using the ORY Oathkeeper Proxy as the Decision API does not forward the request to the upstream.

Error: The Rule "xyz" is invalid: spec.upstream: Required value

https://github.com/ory/oathkeeper-maester/blob/master/config/crd/bases/oathkeeper.ory.sh_rules.yaml#L131

aeneasr commented 4 years ago

Yup, that's a bug! Contribs welcomed!

christian-roggia commented 4 years ago

I would love to contribute, unfortunately, I am not familiar enough with CDRs to feel confident about fixing it.

aeneasr commented 4 years ago

Same holds true for me unfortunately, so I'll tag this as help wanted!

PeteMac88 commented 3 years ago

Hey there, there was a fix on the rules crds merged just a few days ago so that the upstream property of the rule is not required anymore (https://github.com/ory/oathkeeper-maester/pull/45). The maester pod is still showing an error when installing rules resource without this an upstream:

ERROR   controllers.Rule    unable to update Rule status    {"error": "Rule.oathkeeper.ory.sh \"******-rest\" is invalid: spec.upstream: Invalid value: \"null\": spec.upstream in body must be of type object: \"null\""}

I think the problem is that the maester still expects this property to be set for a rule to be valid. To fix this the https://github.com/ory/oathkeeper-maester/tree/master/api/v1alpha1 needs to be adjusted and a new maester image needs to be pushed. Is that right?

aeneasr commented 3 years ago

I released v0.1.1 which should be available soon

PeteMac88 commented 3 years ago

Hey, thanks :). Can you also release a new chart version where you update the default values to oathkeeper maester image to 0.1.1 ?

aeneasr commented 3 years ago

You can do that yourself by overriding the image tag value in the helm values chart

PeteMac88 commented 3 years ago

Yeah i know. But still the default values should represent the latest image. Definitely a best practice since project with automatic chart updates rely on this. Is it possible that you release a new chart?

PeteMac88 commented 3 years ago

Version v0.1.1 did not fix the problem mentioned here. Did you actually fix the problem in the oathkeeper-maester? I think it still validates the rules against a type which requieres upstream to be set.

aeneasr commented 3 years ago

I was under the assumption https://github.com/ory/oathkeeper-maester/commit/ced42147d3bbe22faedc8230c90e2e38547da248 would fix it

PeteMac88 commented 3 years ago

No thats why I extended this Issue

m1pl commented 3 years ago

@aeneasr Maybe a change missing here? https://github.com/ory/oathkeeper-maester/blob/31b6e674e7a285e65f05bfed075ee626e49db065/api/v1alpha1/rule_types.go#L60

aeneasr commented 3 years ago

Upstream is valid, it's just not required.

m1pl commented 3 years ago

@aeneasr Are you sure? The error pretty much says the validation fails because upstream is null... {"error": "Rule.oathkeeper.ory.sh \"******-rest\" is invalid: spec.upstream: Invalid value: \"null\": spec.upstream in body must be of type object: \"null\""}

aeneasr commented 3 years ago

Sorry, I can't work on this. Feel free to debug and fix it appropriately

m1pl commented 3 years ago

Is there any developer that can take a look at this? This issue is open since half a year now and prevents people from using Rule CRDs.

Demonsthere commented 3 years ago

Hey there, I will try to take a look at it ;)

aeneasr commented 3 years ago

You are always free to take a look - it's an open source project and we are very welcoming to new contributors

m1pl commented 3 years ago

@Demonsthere thank you. So the problem seems to be here. If I remove the upstream from your test manifests, the test fails. I'd just remove it from rule_types.go, but I don't know what other implications it might have, hence why I asked for a maintainer to take a look. I'd gladly open a PR if you say it would fix the problem without breaking anything else.

Demonsthere commented 3 years ago

The issue is, that this operator has been build using kubebuilder, and the CRD is generated from code, any manual changes to the CRD are overwritten. In order to make a field optional, you need to create a proper marker and regenerate the CRD

m1pl commented 3 years ago

So this shouldn't have been merged then? Should I open a new PR or are you taking care of it?

Demonsthere commented 3 years ago

Yes, imho https://github.com/ory/oathkeeper-maester/pull/45 shouldn't have been merged, as it is causing confusion, but rather then revering it, I would prefer to fix the root issue, with the markers. If you want, feel free to open a PR, I will be more then happy to help you with it. In the meantime, I will try to add more tests to catch this situation

piotrmsc commented 3 years ago

@m1pl @Demonsthere I see to PRs for the same issue, what's the reason for that? I have checked the PR from @m1pl and the only missing part are tests covering the change. Let's get it shipped, therefore have one PR with everything required (code + tests) ok?:)

Demonsthere commented 3 years ago

https://github.com/ory/oathkeeper-maester/pull/47 is ready for review

christian-roggia commented 3 years ago

Gott sei Dank this issue has been closed, I have been looking forward to being able to use CDRs for oathkeeper rules for a long time. Thanks to everyone for the code changes!

aeneasr commented 3 years ago

😎