Skip to content
This repository has been archived by the owner on Nov 8, 2022. It is now read-only.

make security requirements optional #56

Merged
merged 2 commits into from
Oct 14, 2021
Merged

make security requirements optional #56

merged 2 commits into from
Oct 14, 2021

Conversation

eguzki
Copy link
Contributor

@eguzki eguzki commented Oct 7, 2021

Support for APIProduct obejcts without security (AuthN) configuration

Reconcile adding and removing security from apiproduct

This PR is based on #55, it is ready for review, but should be merged after it

@eguzki eguzki requested a review from jmprusi as a October 7, 2021 17:18
@eguzki eguzki force-pushed the refactor-api-selectors branch from d56fcca to d0a6d59 Compare October 7, 2021 19:23
@eguzki eguzki force-pushed the make-security-optional branch from 0cfb94b to f7f1904 Compare October 7, 2021 19:24
@eguzki eguzki force-pushed the refactor-api-selectors branch from d0a6d59 to a2f4946 Compare October 7, 2021 20:05
@eguzki eguzki force-pushed the make-security-optional branch from f7f1904 to dce049f Compare October 7, 2021 20:06
@eguzki eguzki force-pushed the refactor-api-selectors branch from a2f4946 to 51a8900 Compare October 7, 2021 20:38
@eguzki eguzki force-pushed the make-security-optional branch 2 times, most recently from 6ac3f69 to 735805a Compare October 7, 2021 21:00
@eguzki eguzki requested review from a team and removed request for jmprusi October 7, 2021 21:07
Name: common.KuadrantAuthorizationProvider,
},
},
Action: v1beta12.AuthorizationPolicy_CUSTOM,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of scope here, but there's a request for discussing the conventions across components at Kuadrant/authorino#164

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

istio constant :)

Hosts []string `json:"hosts"`

// Configure authentication mechanisms
// +optional
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed for controller-gen to make it optional? I usually rely only on omitempty and it does the trick, but I'd love to learn about any other extra advantage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have experienced that too. That's confusing as the kubebulder doc says it should be the '+optional marker.

To make it explicit and easy to read, I always add it for optional fields

Copy link
Contributor

@guicassolato guicassolato Oct 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps +kubebuilder:validation:Optional then, so there's no doubt it's for kubebuilder?

Copy link
Contributor

@guicassolato guicassolato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left a couple comments kicking off conversation aside, but nothing that disapproves the change of course. LGTM

@eguzki eguzki force-pushed the refactor-api-selectors branch 2 times, most recently from 23026d6 to dedf108 Compare October 8, 2021 12:57
@eguzki eguzki force-pushed the make-security-optional branch from 735805a to ad3c5da Compare October 8, 2021 12:58
@eguzki
Copy link
Contributor Author

eguzki commented Oct 8, 2021

Added fix for the apiproduct status subresource computing logic when resources are optional.

@eguzki eguzki force-pushed the refactor-api-selectors branch from dedf108 to 7bc3213 Compare October 14, 2021 08:42
Base automatically changed from refactor-api-selectors to main October 14, 2021 09:17
@eguzki eguzki force-pushed the make-security-optional branch from 88623eb to e54d4eb Compare October 14, 2021 09:20
@eguzki eguzki merged commit 0c927b9 into main Oct 14, 2021
@eguzki eguzki deleted the make-security-optional branch October 14, 2021 09:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants