ceph: add opa config options to ObjectStoreSpec #8731

thotz · 2021-09-16T06:02:07Z

Description of your changes:
This PR adds the options in ObjectStore CRD for configuring OPA
authentication with RGW.

Resolves #8737
Signed-off-by: Jiffin Tony Thottan thottanjiffin@gmail.com

Checklist:

This PR adds the options in ObjectStore CRD for configuring OPA authentication with RGW. Signed-off-by: Jiffin Tony Thottan <thottanjiffin@gmail.com>

BlaineEXE · 2021-09-16T14:14:12Z

@thotz Is this related to a BZ or feature request?

travisn · 2021-09-16T17:03:55Z

@thotz Is this related to a BZ or feature request?

Yes it would be helpful just to have more background on requirements for new features, thanks.

thotz · 2021-09-17T06:42:57Z

@thotz Is this related to a BZ or feature request?

Yes it would be helpful just to have more background on requirements for new features, thanks.

The request was from openshift OPA team via mail, So I created Rook Issue #8737 and mentioned it in the Commit message of this PR

leseb · 2021-09-17T09:13:42Z

pkg/apis/ceph.rook.io/v1/types.go

+	// +kubebuilder:validation:MinLength=1
+	// +kubebuilder:validation:Required
+	// +required
+	URL string `json:"url,omitempty"`


Suggested change

URL string `json:"url,omitempty"`

URL string `json:"url"`

Since this is required.

leseb · 2021-09-17T09:13:51Z

pkg/apis/ceph.rook.io/v1/types.go

+	// +kubebuilder:validation:MinLength=1
+	// +kubebuilder:validation:Required
+	// +required
+	Token string `json:"token,omitempty"`


Suggested change

Token string `json:"token,omitempty"`

Token string `json:"token"`

leseb · 2021-09-17T09:15:03Z

pkg/operator/ceph/object/spec_test.go

@@ -210,6 +210,19 @@ func TestSSLPodSpec(t *testing.T) {
 	s, err = c.makeRGWPodSpec(rgwConfig)
 	assert.NoError(t, err)
 	podTemplate = cephtest.NewPodTemplateSpecTester(t, &s)
+	podTemplate.RunFullSuite(cephconfig.RgwType, "default", "rook-ceph-rgw", "mycluster", "quay.io/ceph/ceph:myversion",


What is this actually testing?

leseb · 2021-09-17T09:16:34Z

pkg/operator/ceph/object/spec.go

+		if c.store.Spec.Security.OpenPolicyAgentService.VerifySSL != nil {
+			container.Args = append(container.Args,
+				cephconfig.NewFlag("rgw opa verify ssl",
+					strconv.FormatBool(*c.store.Spec.Security.OpenPolicyAgentService.VerifySSL)))


Is the bool pointer really necessary? If it's not required why not just let the default. false?

BlaineEXE · 2021-09-17T18:26:43Z

@thotz Is this related to a BZ or feature request?

Yes it would be helpful just to have more background on requirements for new features, thanks.

The request was from openshift OPA team via mail, So I created Rook Issue #8737 and mentioned it in the Commit message of this PR

We need to know how to prioritize incoming feature requests. A new Rook issue is good, but let's get a BZ (or Jira?) against OCS/ODF that PM can prioritize. Since this is a cross-team request, we have to have an understanding of who needs it, when they need it, and why they need it. We cannot guarantee that Rook upstream can spend time on under-the-table requests like this.

thotz · 2021-09-20T10:30:27Z

@thotz Is this related to a BZ or feature request?

Yes it would be helpful just to have more background on requirements for new features, thanks.

The request was from openshift OPA team via mail, So I created Rook Issue #8737 and mentioned it in the Commit message of this PR

We need to know how to prioritize incoming feature requests. A new Rook issue is good, but let's get a BZ (or Jira?) against OCS/ODF that PM can prioritize. Since this is a cross-team request, we have to have an understanding of who needs it, when they need it, and why they need it. We cannot guarantee that Rook upstream can spend time on under-the-table requests like this.

I have opened a Jira ticket https://issues.redhat.com/browse/RGW-173 in the RGW kanban board as beginning, try to involve more people to get more understanding about the plan on the ticket

leseb

Also see my comment about having a new entry like IdentityManagement to have OPA and LDAP under it

travisn · 2021-09-20T19:28:01Z

Documentation/ceph-object-store-crd.md

@@ -199,6 +201,15 @@ $ vault write -f transit/keys/<mybucketkey> exportable=true # transit engine

 * TLS authentication with custom certs between Vault and RGW are yet to be supported.

+Here example for OPA config:
+```yaml
+  opa:


Let's avoid non-obvious acronyms in the CR property names

Suggested change

opa:

openPolicyAgent:

travisn · 2021-09-20T19:28:43Z

Documentation/ceph-object-store-crd.md

+    token: eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9
+    verifySSL: true
+```
+The config options include url, token for the OPA service. The TLS certs can be passed with help `caBundleRef`. For more details w.r.t RGW, please refer [Ceph OPA documentation](https://docs.ceph.com/en/latest/radosgw/opa/).


Suggested change

The config options include url, token for the OPA service. The TLS certs can be passed with help `caBundleRef`. For more details w.r.t RGW, please refer [Ceph OPA documentation](https://docs.ceph.com/en/latest/radosgw/opa/).

The config options include url, token for the OPA service. The TLS certs can be passed with help `caBundleRef`. For more details, please refer to the [Ceph OPA documentation](https://docs.ceph.com/en/latest/radosgw/opa/).

travisn · 2021-09-20T19:30:43Z

pkg/apis/ceph.rook.io/v1/types.go

+	// Indicate whether the server certificate is validated by the client or not
+	// +optional
+	// +nullable
+	VerifySSL *bool `json:"verifySSL,omitempty"`


Is the behavior of "not set" and "false" the same? If so, how about removing the pointer for simplicity?

Suggested change

VerifySSL *bool `json:"verifySSL,omitempty"`

VerifySSL bool `json:"verifySSL,omitempty"`

travisn · 2021-09-20T19:31:01Z

pkg/apis/ceph.rook.io/v1/types.go

+	// +optional
+	// +nullable
+	VerifySSL *bool `json:"verifySSL,omitempty"`
+	// Token is the token for the OPA


Suggested change

// Token is the token for the OPA

// Token for the OPA

BlaineEXE · 2021-09-24T17:17:31Z

I think #8579 is the more high priority item that we definitely have in our requirements for ODF 4.10. Let's focus on that PR first.

github-actions · 2021-10-24T20:02:06Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in two weeks if no further activity occurs. Thank you for your contributions.

mergify · 2021-10-28T13:09:04Z

This pull request has merge conflicts that must be resolved before it can be merged. @thotz please rebase it. https://rook.io/docs/rook/latest/development-flow.html#updating-your-fork

thotz · 2022-06-24T05:15:04Z

I am closing this PR since there is no more interest wrt to this feature atm, not planning to work on it anytime soon. I will open once again when priority changes

thotz · 2022-06-24T05:15:40Z

I am closing this PR since there is no more interest wrt to this feature atm, not planning to work on it anytime soon. I will open once again when priority changes

mergify bot added the ceph main ceph tag label Sep 16, 2021

ceph: add opa config options to ObjectStoreSpec

b68c2e6

This PR adds the options in ObjectStore CRD for configuring OPA authentication with RGW. Signed-off-by: Jiffin Tony Thottan <thottanjiffin@gmail.com>

thotz force-pushed the rgwopaconfig branch from 81d4ed1 to b68c2e6 Compare September 16, 2021 06:33

leseb requested changes Sep 17, 2021

View reviewed changes

leseb reviewed Sep 20, 2021

View reviewed changes

travisn requested changes Sep 20, 2021

View reviewed changes

github-actions bot added the stale Labeled by the stale bot label Oct 24, 2021

thotz added keepalive and removed stale Labeled by the stale bot labels Oct 25, 2021

thotz mentioned this pull request Apr 4, 2022

ceph: add ldap configs for rgw #8750

Closed

10 tasks

thotz closed this Jun 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ceph: add opa config options to ObjectStoreSpec #8731

ceph: add opa config options to ObjectStoreSpec #8731

thotz commented Sep 16, 2021 •

edited

BlaineEXE commented Sep 16, 2021

travisn commented Sep 16, 2021

thotz commented Sep 17, 2021

leseb Sep 17, 2021

leseb Sep 17, 2021

leseb Sep 17, 2021

leseb Sep 17, 2021

BlaineEXE commented Sep 17, 2021

thotz commented Sep 20, 2021

leseb left a comment

travisn Sep 20, 2021

travisn Sep 20, 2021

travisn Sep 20, 2021

travisn Sep 20, 2021

BlaineEXE commented Sep 24, 2021

github-actions bot commented Oct 24, 2021

mergify bot commented Oct 28, 2021

thotz commented Jun 24, 2022

thotz commented Jun 24, 2022

	Token string `json:"token,omitempty"`
	Token string `json:"token"`

	The config options include url, token for the OPA service. The TLS certs can be passed with help `caBundleRef`. For more details w.r.t RGW, please refer [Ceph OPA documentation](https://docs.ceph.com/en/latest/radosgw/opa/).
	The config options include url, token for the OPA service. The TLS certs can be passed with help `caBundleRef`. For more details, please refer to the [Ceph OPA documentation](https://docs.ceph.com/en/latest/radosgw/opa/).

	VerifySSL *bool `json:"verifySSL,omitempty"`
	VerifySSL bool `json:"verifySSL,omitempty"`

ceph: add opa config options to ObjectStoreSpec #8731

ceph: add opa config options to ObjectStoreSpec #8731

Conversation

thotz commented Sep 16, 2021 • edited

BlaineEXE commented Sep 16, 2021

travisn commented Sep 16, 2021

thotz commented Sep 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlaineEXE commented Sep 17, 2021

thotz commented Sep 20, 2021

leseb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlaineEXE commented Sep 24, 2021

github-actions bot commented Oct 24, 2021

mergify bot commented Oct 28, 2021

thotz commented Jun 24, 2022

thotz commented Jun 24, 2022

thotz commented Sep 16, 2021 •

edited