THREESCALE-9000 Allow adding annotations to components via APIManager CR #836

valerymo · 2023-06-21T15:49:00Z

WHAT

Jira: https://issues.redhat.com/browse/THREESCALE-9000
Allow adding annotations to 3scale components via APIManager CR

Component	Fresh install adds annotations	Reconcile annotations
apicast-production	[X]	[X]
apicast-staging	[X]	[X]
backend-cron	[X]	[X]
backend-listener	[X]	[X]
backend-worker	[X]	[X]
backend-redis	[X]	[X]
system-app	[X]	[X]
system-sidekiq	[X]	[X]
system-searchd	[X]	[X]
system-memcache	[X]	[X]
system-mysql	[X ]	[ X]
system-postgresql	[X]	[X]
system-redis	[X]	[X]
zync	[X]	[X]
zync-database	[X]	[X]
zync-que	[X]	[X]

ApiManager CR example for apicast-staging and backend-listener (see Validation section for CR example for all/other DCs/pods

apiVersion: apps.3scale.net/v1alpha1
kind: APIManager
metadata:
  name: example-apimanager
spec:
  wildcardDomain: example.com
  backend:
    listenerSpec:
      annotations:
        backendListenerL1: test-1-CR9000
  apicast:
    stagingSpec:
      annotations:
        apicastStgL1: test-1-CR9000

S3 secret example

kind: Secret
apiVersion: v1
metadata: 
  name: s3-credentials
data: 
  AWS_ACCESS_KEY_ID: QUtXXXXXXXXXXXX=
  AWS_SECRET_ACCESS_KEY: bmZl=
  AWS_BUCKET: dGV=
  AWS_REGION: dXMt==
type: Opaque

Validation

Install 3scale operator

$ cd 3scale-operator
$ make install

$ export NAMESPACE=3scale-test
$ oc new-project $NAMESPACE
$ make download
$ make run

Secrets & CRs for test

Secret's AWS fields - encoded base64: echo ABC |base64

oc apply -f - <<EOF
---
kind: Secret
apiVersion: v1
metadata: 
  name: s3-credentials
data: 
  AWS_ACCESS_KEY_ID: QUtJQVYXXXXXX=
  AWS_SECRET_ACCESS_KEY: bmZX=
  AWS_BUCKET: dGX=
  AWS_REGION: dXX==
type: Opaque
EOF

Sample of CR for test 3scale pods

apiVersion: apps.3scale.net/v1alpha1
kind: APIManager
metadata:
  name: example-apimanager
spec:
  system:
    fileStorage:
      simpleStorageService:
        configurationSecretRef:
          name: s3-credentials
    appSpec:
      annotations:
        systemAppL1: test-1-CR9000
    searchdSpec:
      annotations:
        systemSearchdL1: test-1-CR9000
    sidekiqSpec:
      annotations:
        systemSidekiqL1: test-1-CR9000
    memcachedAnnotations:
      systemMemCachedL1: test-1-CR9000
    redisAnnotations:
      systemRedisL1: test-1-CR9000
    database:
      postgresql:
        annotations:
          systemDBPostgresL1: test-1-CR9000
  zync:
    appSpec:
      annotations:
        zyncAppL1: test-1-CR9000
    queSpec:
      annotations:
        zyncQueL1: test-1-CR9000
    databaseAnnotations:
      zyncDBL1: test-1-CR9000
  backend:
    cronSpec:
      annotations:
        backendCronL1: test-1-CR9000
    listenerSpec:
      annotations:
        backendListenerL1: test-1-CR9000
    workerSpec:
      annotations:
        backendWorkerL1: test-1-CR9000
    redisAnnotations:
      backendRedisL1: test-1-CR9000
  apicast:
    productionSpec:
      annotations:
        apicastProdL1: test-1-CR9000
    stagingSpec:
      annotations:
        apicastStgL1: test-1-CR9000
  wildcardDomain: apps.vmogilev01.l80z.s1.devshift.org

TEST

[vmogilev@vmogilev THREESCALE-9000]$ oc get dc
NAME                 REVISION   DESIRED   CURRENT   TRIGGERED BY
apicast-production   1          1         1         config,image(amp-apicast:2.14)
apicast-staging      1          1         1         config,image(amp-apicast:2.14)
backend-cron         1          1         1         config,image(amp-backend:2.14)
backend-listener     1          1         1         config,image(amp-backend:2.14)
backend-redis        1          1         1         config,image(backend-redis:2.14)
backend-worker       1          1         1         config,image(amp-backend:2.14)
system-app           1          1         1         config,image(amp-system:2.14)
system-memcache      1          1         1         config,image(system-memcached:2.14)
system-postgresql    1          1         1         config,image(system-postgresql:2.14)
system-redis         1          1         1         config,image(system-redis:2.14)
system-searchd       1          1         1         config,image(system-searchd:2.14)
system-sidekiq       1          1         1         config,image(amp-system:2.14)
zync                 1          1         1         config,image(amp-zync:2.14)
zync-database        1          1         1         config,image(zync-database-postgresql:2.14)
zync-que             1          1         1         config,image(amp-zync:2.14)

vmogilev@vmogilev THREESCALE-9000]$ oc get deploymentconfigs -o name | xargs -I {} bash -c "echo -n '{} -> Annotations: '; kubectl get {} -o jsonpath='{.spec.template.metadata}' | yq e -P" |grep CR900
 apicastProdL1: test-1-CR9000
  apicastStgL1: test-1-CR9000
  backendCronL1: test-1-CR9000
  backendListenerL1: test-1-CR9000
  backendRedisL1: test-1-CR9000
  backendWorkerL1: test-1-CR9000
  systemAppL1: test-1-CR9000
  systemMemCachedL1: test-1-CR9000
  systemDBPostgresL1: test-1-CR9000
  systemRedisL1: test-1-CR9000
  systemSearchdL1: test-1-CR9000
  systemSidekiqL1: test-1-CR9000
  zyncAppL1: test-1-CR9000
  zyncDBL1: test-1-CR9000
  zyncQueL1: test-1-CR9000

[vmogilev@vmogilev THREESCALE-9000]$

Change Annotation test

Change CR yaml file - replace annotation test-1-CR9000 to test-2-CR9000
Apply updated CR yaml file.
Check deploymentconfigs, as below. DCs revision number will be incremented from 1 to 2.

[vmogilev@vmogilev THREESCALE-9000]$ oc get dc
NAME                 REVISION   DESIRED   CURRENT   TRIGGERED BY
apicast-production   2          1         1         config,image(amp-apicast:2.14)
apicast-staging      2          1         1         config,image(amp-apicast:2.14)
backend-cron         2          1         1         config,image(amp-backend:2.14)
backend-listener     2          1         1         config,image(amp-backend:2.14)
backend-redis        2          1         1         config,image(backend-redis:2.14)
backend-worker       2          1         1         config,image(amp-backend:2.14)
system-app           2          1         1         config,image(amp-system:2.14)
system-memcache      2          1         1         config,image(system-memcached:2.14)
system-postgresql    2          1         1         config,image(system-postgresql:2.14)
system-redis         2          1         1         config,image(system-redis:2.14)
system-searchd       2          1         1         config,image(system-searchd:2.14)
system-sidekiq       2          1         1         config,image(amp-system:2.14)
zync                 2          1         1         config,image(amp-zync:2.14)
zync-database        2          1         1         config,image(zync-database-postgresql:2.14)
zync-que             2          1         1         config,image(amp-zync:2.14)

[vmogilev@vmogilev THREESCALE-9000]$ date
Sun Jun 25 11:18:10 IDT 2023

Check that annotation changed in all deploymentconfigs:

[vmogilev@vmogilev THREESCALE-9000]$ oc get deploymentconfigs -o name | xargs -I {} bash -c "echo -n '{} -> Annotations: '; oc get {} -o jsonpath='{.spec.template.metadata}' | yq e -P" |grep CR900
  apicastProdL1: test-2-CR9000
  apicastStgL1: test-2-CR9000
  backendCronL1: test-2-CR9000
  backendListenerL1: test-2-CR9000
  backendRedisL1: test-2-CR9000
  backendWorkerL1: test-2-CR9000
  systemAppL1: test-2-CR9000
  systemMemCachedL1: test-2-CR9000
  systemDBPostgresL1: test-2-CR9000
  systemRedisL1: test-2-CR9000
  systemSearchdL1: test-2-CR9000
  systemSidekiqL1: test-2-CR9000
  zyncAppL1: test-2-CR9000
  zyncDBL1: test-2-CR9000
  zyncQueL1: test-2-CR9000
[vmogilev@vmogilev THREESCALE-9000]$

pkg/3scale/amp/component/apicast.go

pkg/3scale/amp/operator/apicast_options_provider.go

Patryk-Stefanski · 2023-07-03T10:25:49Z

Verified that annotations can be created and updated via APIManager CR, code also looks good.

/lgtm

eguzki

Looks good. Only one minor comment

apis/apps/v1alpha1/apimanager_types.go

valerymo · 2023-07-06T14:20:34Z

/test test-e2e

valerymo · 2023-07-06T14:57:18Z

/test test-e2e

pkg/3scale/amp/operator/apicast_options_provider.go

eguzki · 2023-07-10T07:48:31Z

/test test-e2e

eguzki · 2023-07-10T07:53:47Z

regarding change Annotation / Update test, what is the test about?

I do not see any step. Just oc get dc, then date 🤷 , then oc get deploymentconfigs -o name | xargs -I {} bash -c "echo -n '{} -> Annotations: '; oc get {} -o jsonpath='{.spec.template.metadata}' | yq e -P" |grep CR900

valerymo · 2023-07-10T10:38:24Z

regarding change Annotation / Update test, what is the test about?

I do not see any step. Just oc get dc, then date shrug , then oc get deploymentconfigs -o name | xargs -I {} bash -c "echo -n '{} -> Annotations: '; oc get {} -o jsonpath='{.spec.template.metadata}' | yq e -P" |grep CR900

I will add CR or comment to make it more clear, but actually we can see here following (comparing with previous query for dc):

DC revision changed : 1->2 , like:
apicast-production 2 1 1 config,image(amp-apicast:2.14)
Annotation name changed from test-1-CR9000 to test-2-CR9000

valerymo · 2023-07-11T07:38:00Z

/test test-e2e

pkg/3scale/amp/operator/redis_options_provider.go

codeclimate · 2023-07-11T09:30:21Z

Code Climate has analyzed commit 0ba3cd8 and detected 26 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	4
Duplication	22

View more on Code Climate.

valerymo · 2023-07-13T09:19:38Z

regarding change Annotation / Update test, what is the test about?
I do not see any step. Just oc get dc, then date shrug , then oc get deploymentconfigs -o name | xargs -I {} bash -c "echo -n '{} -> Annotations: '; oc get {} -o jsonpath='{.spec.template.metadata}' | yq e -P" |grep CR900

I will add CR or comment to make it more clear, but actually we can see here following (comparing with previous query for dc):

DC revision changed : 1->2 , like:
apicast-production 2 1 1 config,image(amp-apicast:2.14)

Annotation name changed from test-1-CR9000 to test-2-CR9000

Hi @eguzki . Validation section - updated. Added details for Annotation change test

eguzki · 2023-07-13T09:51:31Z

@Patryk-Stefanski I was thinking about the reconciliation policy for labels and annotations. If the user can add their labels and annotations using the APIManager CR, maybe there is no need to be flexible and allow manually added labels/annotations. There is already a way to specify labels/annotations, so the user should be using that and adding labels/annotations manually is not supported. Wdyt? Related issue https://issues.redhat.com/browse/THREESCALE-9820

So my take about this would be the following approach:

When component A (could be apicast staging, zync, ...) has labels specified in the APIManager CR, then the operator does strict reconciliation for those components. Meaning that only those labels/annotations will be there in the pods (in addition to those added by the operator itself like app: 3scale-api-management.
When no labels/annotations are defined for some component B in the APIManager CR, the operator is flexible and allows adding any other label manually.

Wdyt??

cc @valerymo

eguzki · 2023-07-13T09:53:28Z

I have my doubts on When no labels/annotations are defined for some component B in the APIManager CR, the operator is flexible and allows adding any other label manually. and you might be right in the first implementation of being strict with the label reconciliation.

valerymo · 2023-07-13T11:32:11Z

@eguzki , @Patryk-Stefanski I think need separate Task/PR for discussion. I'd like suggest merge this PR as is, as it was for Labels

Patryk-Stefanski · 2023-07-13T11:37:00Z

Yeah, I think if someone sets the labels through the APIM they should be prepared that it is the single source of truth, this would mean there is less confusion when trying to figure out where to set labels.

So labels set in APIM - APIM single source of truth
No labels set in APIM - labels controlled through DC ??

eguzki · 2023-07-13T13:09:47Z

@eguzki , @Patryk-Stefanski I think need separate Task/PR for discussion. I'd like suggest merge this PR as is, as it was for Labels

We can do what you suggest. IMO, there is no rush, no need to merge something I currently think it is wrong. The reason being that the reconciliation strategy directly impacts this PR and I am proposing a different one. Hence, let's:

discuss about the right reconciliation approach.
agree on the right behavior
Implement the strategy in this PR if we decide to change the current implementation
Bring the decision to the labels in another PR

eguzki · 2023-07-13T13:13:39Z

Yeah, I think if someone sets the labels through the APIM they should be prepared that it is the single source of truth, this would mean there is less confusion when trying to figure out where to set labels.

So labels set in APIM - APIM single source of truth No labels set in APIM - labels controlled through DC ??

Yes. But keep in mind that even when the APIM does not add any labels, the 3scale operator still adds few labels (not sure if annotations as well, but I would assume it does as well).

Patryk-Stefanski · 2023-07-20T10:26:37Z

After a discussion with @eguzki we decided that were going to leave the reconciliation as is ie. users can create labels/annotations through both APIM and DC, however to delete the label/annotation created through APIM you must first delete it from the DC and then from the APIM (This has to be made clear in the documentation). The reasoning for this decision was that it keeps it simple and doesn't overly complicate any of the functionality as well as we avoid removing any labels/annotations set externally.

@valerymo Think this pr should be good to go : )

valerymo · 2023-07-23T07:05:39Z

Hi @eguzki , @Patryk-Stefanski , I did few tests (please see below), and it seems to me that the flexible approach that we have taken for labels and annotations is quite suitable.
It's not that confusing, and the user can live with it. I think it's better than restrictions.

Just one note, regarding delete first label in DC. I saw in tests - need first delete in CR (as label was not deleted from DC, was restored if it exists in PR . Yo will see notes below, in tests).

1. Define label via CR =>
    - Label appears in DC
    - Label set in POD
2. Rename label in CR =>
    - DC
        - new revision
        - label renamed
    - Pod - Label renamed
3. Delete label in CR:
    - DC - same DC version, -> label remains 
    - Pod - label remains
4. Delete label in DC: ?????
    - DC
        - new revision
        - label removed
    - Pod - Label removed
    - CR - not changed
    ## !!?? after several tests was found that unable to delete Label from DC if it exists in CR.
    ## after deletion label in CR , DC edit (delete label) - worked.
5. Add new Label via CR -> see step 1 
6. Add new Label via DC:
    - Pod - added new label
7. Edit CR - remove label (first label, second - missing in CR)
    - dc not changed
    - pod not changed
8. Add new Label via CR -> see step 1

valerymo requested a review from a team as a code owner June 21, 2023 15:49

openshift-ci bot added the do-not-merge/work-in-progress label Jun 21, 2023

valerymo force-pushed the THREESCALE-9000 branch 12 times, most recently from da04ba5 to 51003b2 Compare June 25, 2023 13:36

valerymo changed the title ~~[WIP] THREESCALE-9000 Allow adding annotations to components via APIManager CR~~ THREESCALE-9000 Allow adding annotations to components via APIManager CR Jun 25, 2023

openshift-ci bot removed the do-not-merge/work-in-progress label Jun 25, 2023

valerymo requested review from eguzki and removed request for a team June 25, 2023 14:48

Patryk-Stefanski reviewed Jun 29, 2023

View reviewed changes

pkg/3scale/amp/component/apicast.go Show resolved Hide resolved

pkg/3scale/amp/operator/apicast_options_provider.go Outdated Show resolved Hide resolved

eguzki requested changes Jul 5, 2023

View reviewed changes

apis/apps/v1alpha1/apimanager_types.go Outdated Show resolved Hide resolved

valerymo force-pushed the THREESCALE-9000 branch 2 times, most recently from 47a81c0 to 75574c2 Compare July 6, 2023 13:51

eguzki reviewed Jul 7, 2023

View reviewed changes

pkg/3scale/amp/operator/apicast_options_provider.go Outdated Show resolved Hide resolved

valerymo force-pushed the THREESCALE-9000 branch from 75574c2 to bb56273 Compare July 10, 2023 12:41

openshift-merge-robot added the needs-rebase label Jul 10, 2023

THREESCALE-9000 Allow adding annotations to components via APIManager CR

ec722ac

valerymo force-pushed the THREESCALE-9000 branch from 8d13c8e to ec722ac Compare July 11, 2023 06:48

openshift-merge-robot removed the needs-rebase label Jul 11, 2023

eguzki requested changes Jul 11, 2023

View reviewed changes

pkg/3scale/amp/operator/redis_options_provider.go Outdated Show resolved Hide resolved

pkg/3scale/amp/operator/redis_options_provider.go Outdated Show resolved Hide resolved

THREESCALE-9000 - review comments addressing

0ba3cd8

eguzki approved these changes Jul 20, 2023

View reviewed changes

eguzki merged commit 0ae9c71 into 3scale:master Jul 20, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

THREESCALE-9000 Allow adding annotations to components via APIManager CR #836

THREESCALE-9000 Allow adding annotations to components via APIManager CR #836

valerymo commented Jun 21, 2023 •

edited

Patryk-Stefanski commented Jul 3, 2023

eguzki left a comment

valerymo commented Jul 6, 2023

valerymo commented Jul 6, 2023

eguzki commented Jul 10, 2023

eguzki commented Jul 10, 2023

valerymo commented Jul 10, 2023

valerymo commented Jul 11, 2023

codeclimate bot commented Jul 11, 2023

valerymo commented Jul 13, 2023

eguzki commented Jul 13, 2023 •

edited

eguzki commented Jul 13, 2023

valerymo commented Jul 13, 2023 •

edited

Patryk-Stefanski commented Jul 13, 2023

eguzki commented Jul 13, 2023

eguzki commented Jul 13, 2023

Patryk-Stefanski commented Jul 20, 2023

valerymo commented Jul 23, 2023 •

edited

THREESCALE-9000 Allow adding annotations to components via APIManager CR #836

THREESCALE-9000 Allow adding annotations to components via APIManager CR #836

Conversation

valerymo commented Jun 21, 2023 • edited

WHAT

Validation

Install 3scale operator

Secrets & CRs for test

TEST

Change Annotation test

Patryk-Stefanski commented Jul 3, 2023

eguzki left a comment

Choose a reason for hiding this comment

valerymo commented Jul 6, 2023

valerymo commented Jul 6, 2023

eguzki commented Jul 10, 2023

eguzki commented Jul 10, 2023

valerymo commented Jul 10, 2023

valerymo commented Jul 11, 2023

codeclimate bot commented Jul 11, 2023

valerymo commented Jul 13, 2023

eguzki commented Jul 13, 2023 • edited

eguzki commented Jul 13, 2023

valerymo commented Jul 13, 2023 • edited

Patryk-Stefanski commented Jul 13, 2023

eguzki commented Jul 13, 2023

eguzki commented Jul 13, 2023

Patryk-Stefanski commented Jul 20, 2023

valerymo commented Jul 23, 2023 • edited

valerymo commented Jun 21, 2023 •

edited

eguzki commented Jul 13, 2023 •

edited

valerymo commented Jul 13, 2023 •

edited

valerymo commented Jul 23, 2023 •

edited