THREESCALE-8002 - feat: allow reading policy configuration from secret

[KevFan]

Description

Allow reading policy configuration from secret

Verification

• Checkout this branch
• Run 3scale

make install
oc new-project 3scale
make run

• Create s3 secret

oc apply -f - <<EOF                                                             
---             
apiVersion: v1                      
kind: Secret    
metadata:
  creationTimestamp: null
  name: aws-auth
stringData:                                               
  AWS_ACCESS_KEY_ID: something      
  AWS_SECRET_ACCESS_KEY: something
  AWS_BUCKET: something
  AWS_REGION: us-east-1    
type: Opaque                   
EOF

• Create APIMananger

oc apply -f - <<EOF                                                             
---             
apiVersion: apps.3scale.net/v1alpha1
kind: APIManager
metadata:
  name: apimanager1
spec:
  wildcardDomain: apps.some-valid-domain
  resourceRequirementsEnabled: false
  system:
    fileStorage:
      simpleStorageService:
        configurationSecretRef:
          name: aws-auth
EOF

• Create policy configuration secret

oc apply -f - <<EOF                                                                                    
---                                         
apiVersion: v1                             
kind: Secret            
metadata:                   
  name: my-config-policy 
type: Opaque                
stringData:                
  configuration: "{\"http_proxy\":\"http://secret.com\"}"
EOF

• Create Product with configuration from value

oc apply -f - <<EOF                                                             
---
apiVersion: capabilities.3scale.net/v1beta1
kind: Product
metadata:
  name: product1
spec:
  name: "OperatedProduct 1"
  policies:
  - configuration:
      http_proxy: http://example.com
      https_proxy: https://example.com
    name: camel
    version: builtin
    enabled: true
EOF

• Create Product with configuration from secret

oc apply -f - <<EOF                                                             
---
apiVersion: capabilities.3scale.net/v1beta1
kind: Product
metadata:
  name: product2
spec:
  name: "OperatedProduct 2"
  policies:
  - configurationRef:
       name: my-config-policy
    name: camel
    version: builtin
    enabled: true
EOF

• Create produce with both configuration from value and from secret

oc apply -f - <<EOF                                                             
---
apiVersion: capabilities.3scale.net/v1beta1
kind: Product
metadata:
  name: product3
spec:
  name: "OperatedProduct 3"
  policies:
  - configuration:
      http_proxy: http://example.com
      https_proxy: https://example.com
    configurationRef:
           name: my-config-policy
    name: camel
    version: builtin
    enabled: true
EOF

• Verify product CRs are reconciled successfully

 oc get products -o json | jq '.items[].status'

• Sign into default tenant using password from system-seed secret
• Verify product1 policy configuration matches from value for camel policy

• Verify product2 policy configuration matches from secret for camel policy

• Verify product2 CR has {} as the default value for configuration in policies

oc get products product2 -o json | jq '.spec.policies'

• Verify product3 uses the confgiuration from value as this takes precedence over secret
  

[eguzki]

Cool! 🥇

This PR is ready to be merged

Let me drop two comments and one "request for change".

• What if spec.policies[*].configuration and spec.policies[*].configurationRef are applied at the same time? The operator does not complain and the secret takes preference. That's ok. I could argue to have the value based configuration taking preference over the secret one, but I think it is ok. However, at least we need to document what it is implemented
• The spec.policies[*].configuration goes from required to optional. It is "safe" generally speaking. In cluster wide mode install, there is no way back from CRD where configuration is optional to CRD where configuration is required. But in namespace mode, that can happen. Let's say in namespace A 2.12 is installed at Day 0. On Day 1, in namespace B, somebody installs 2.14 (making configuration optional). And creates one Product with configuration empty and filling configurationRef. Then, on Day 2, in namespace A, 3scale operator is upgrade from 2.12 -> 2.13. Effectively, making the configuration required. Products in namespace B will become invalid. Any thoughts on this use case? Is it valid? Is it something we should concern about?
• [Request for change] Can you add an example in the user guide in https://github.com/3scale/3scale-operator/blob/master/doc/operator-application-capabilities.md#product-policy-chain. Make it clear that the key of the secret content must be "configuration"

[KevFan]

• What if spec.policies[*].configuration and spec.policies[*].configurationRef are applied at the same time? The operator does not complain and the secret takes preference. That's ok. I could argue to have the value based configuration taking preference over the secret one, but I think it is ok. However, at least we need to document what it is implemented

Yes, the intention was to have the secret take precedence but it is true that typically plain values would take precedence 🤔 I can make the change for the plain value to take precendence and note it in the documentation

• The spec.policies[*].configuration goes from required to optional. It is "safe" generally speaking. In cluster wide mode install, there is no way back from CRD where configuration is optional to CRD where configuration is required. But in namespace mode, that can happen. Let's say in namespace A 2.12 is installed at Day 0. On Day 1, in namespace B, somebody installs 2.14 (making configuration optional). And creates one Product with configuration empty and filling configurationRef. Then, on Day 2, in namespace A, 3scale operator is upgrade from 2.12 -> 2.13. Effectively, making the configuration required. Products in namespace B will become invalid. Any thoughts on this use case? Is it valid? Is it something we should concern about?

Ah yes, I forgot about this scenario 🤦 Yes, unfortunately the nature of the CRDs being cluster scoped can force this to occur. Proper CRD versioning would possibly mitigate this 🤔 I'll play around with +kubebuilder:default annotation to see can I work around this or through some other means to keep the field as required to prevent this scenario.

I guess to note that even if we can work around this, Products in namespace B will still end up as being "invalid" in a sense since I guess the CRs would lose reference to the secretRef in the CR spec and use the plain value instead (which would likely be empty)? 🤔

• [Request for change] Can you add an example in the user guide in https://github.com/3scale/3scale-operator/blob/master/doc/operator-application-capabilities.md#product-policy-chain. Make it clear that the key of the secret content must be "configuration"

Sure, I'll add an example in there 👍

[eguzki]

I guess to note that even if we can work around this, Products in namespace B will still end up as being "invalid" in a sense since I guess the CRs would lose reference to the secretRef in the CR spec and use the plain value instead (which would likely be empty)?

I remember working in an issue related to this #754.
By preserving unknown fields, the unknown fields for the current CRD stored in the etcd are still served. Effectively, the operator 2.14 would still read the secret field, but the users of that namespace B would not be able to create new products with policies as secrets. Have a look at it and let's find out best thing we can do.

[KevFan]

I remember working in an issue related to this #754. By preserving unknown fields, the unknown fields for the current CRD stored in the etcd are still served. Effectively, the operator 2.14 would still read the secret field, but the users of that namespace B would not be able to create new products with policies as secrets. Have a look at it and let's find out best thing we can do.

Oh cool, wasn't aware of that. Thanks ! 👍

[codeclimate]

Code Climate has analyzed commit 734fd5c and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	2

View more on Code Climate.

[eguzki]

Congrats for the detailed and good verification steps

All the steps worked like a charm