-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
837 new policies #868
837 new policies #868
Conversation
Signed-off-by: Mohamed Bana <mohamed@bana.io>
Signed-off-by: jasmingacic <jasmin.gacic@gmail.com>
✅ Deploy Preview for kusk-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Signed-off-by: jasmingacic <jasmin.gacic@gmail.com>
Signed-off-by: jasmingacic <jasmin.gacic@gmail.com>
Signed-off-by: jasmingacic <jasmin.gacic@gmail.com>
While at this change we should start designing how JWT should fit into policies. I already have a proposal in this issue -> #402 |
Signed-off-by: jasmingacic <jasmin.gacic@gmail.com>
…ateway into 837_new_policies
Note: In this PR i have disabled check-cache as it was failing too often for integration tests to be practical |
Good choice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Needs a few changes though.
Signed-off-by: jasmingacic <jasmin.gacic@gmail.com>
Co-authored-by: Kyle Hodgetts <kyle@kubeshop.io>
Co-authored-by: Kyle Hodgetts <kyle@kubeshop.io>
Co-authored-by: Kyle Hodgetts <kyle@kubeshop.io>
Co-authored-by: Kyle Hodgetts <kyle@kubeshop.io>
Co-authored-by: Kyle Hodgetts <kyle@kubeshop.io>
Co-authored-by: Kyle Hodgetts <kyle@kubeshop.io>
Co-authored-by: Kyle Hodgetts <kyle@kubeshop.io>
…ateway into 837_new_policies
Co-authored-by: Kyle Hodgetts <kyle@kubeshop.io>
Co-authored-by: Kyle Hodgetts <kyle@kubeshop.io>
Co-authored-by: Kyle Hodgetts <kyle@kubeshop.io>
Co-authored-by: Kyle Hodgetts <kyle@kubeshop.io>
Co-authored-by: Kyle Hodgetts <kyle@kubeshop.io>
Co-authored-by: Kyle Hodgetts <kyle@kubeshop.io>
Signed-off-by: jasmingacic <jasmin.gacic@gmail.com>
…ateway into 837_new_policies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved. Feel free to merge once the E2E tests have passed
@aabedraba do you need more input for the docs update? I tried to fix where I could |
if o.OAuth2 != nil && o.Custom != nil { | ||
return fmt.Errorf("custom auth and OAuth2 cannot be enabled at the same time") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need a generic check:
if: more than 1 auth mechanism is defined
return: Only 1 authentication mechanism allowed
or something like that? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's already in place
Co-authored-by: Abdallah Abedraba <aabedraba@gmail.com>
fixes #837
This PR has breaking changes to the policies (formerly known as extensions) schema.
I decided to go with host instead of authorizer-hostname and authorizer-port. It makes it easier to copy past shorter property names
scheme: Basic
is nowcustom
This one is ugly but I opted for underscores instead of hyphen because it is easier to select words with underscore in it. I think we should take this property to the "drawing board"
disabled
is nowhidden
. Hidden is more descriptive than disabled as the endpoint isn't really disabled but just hidden from the users.All tests have passed locally and on are remote cluster. We only need to wait on the CI to agree with that.