Skip to content

feat: add apl-operator #2151

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

Merged
merged 104 commits into from
May 23, 2025
Merged

feat: add apl-operator #2151

merged 104 commits into from
May 23, 2025

Conversation

CasLubbers
Copy link
Contributor

@CasLubbers CasLubbers commented May 13, 2025

📌 Summary

  • Set otomi-pipelines to false so it does not get installed
  • Added new apl-operator which takes over the applying from tekton
  • apl-operator polls for changes every second.
    • The changes are either applied for only the teams, when there are only files changed in teams directory
    • Or the whole platform gets applied
    • When all the new commits contain [ci skip] the apply is skipped
  • apl-operator reconciles every 5 minutes. It will reapply the whole platform
  • The build state is stored in a configmap

🔍 Reviewer Notes

🧹 Checklist

  • Code is readable, maintainable, and robust.
  • Unit tests added/updated

Copy link

github-actions bot commented May 13, 2025

Coverage report

St.
Category Percentage Covered / Total
🔴 Statements
47.19% (+5.93% 🔼)
983/2083
🔴 Branches
36.3% (+3.36% 🔼)
334/920
🔴 Functions
47.29% (+6.62% 🔼)
157/332
🔴 Lines
48.37% (+6.21% 🔼)
935/1933
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / apl-operator.ts
100% 91.3% 100% 100%
🟢 operator/errors.ts 100% 100% 100% 100%
🟢
... / apl-operations.ts
100% 100% 100% 100%
🟢 operator/k8s.ts 96.15% 70% 100% 100%
🟢
... / git-repository.ts
100% 81.82% 100% 100%
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🔴 cmd/migrate.ts
57.14% (-2.01% 🔻)
48.52% (-1.79% 🔻)
63.51% (-1.76% 🔻)
56.74% (-2.03% 🔻)
🔴 common/bootstrap.ts
6.38% (-0.14% 🔻)
0% 0%
6.38% (-0.14% 🔻)

Test suite run success

140 tests passing in 13 suites.

Report generated by 🧪jest coverage report action from 2839fb1

Copy link
Contributor

@ferruhcihan ferruhcihan left a comment

Choose a reason for hiding this comment

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

Tested with a cluster both deploying and upgrading scenarios, LGTM 👍🏻

@merll
Copy link
Contributor

merll commented May 22, 2025

I am seeing lots of errors in combination with the Helm file upgrade, but also the pre-upgrade from the latest release. Apparently the serviceAccount does not have enough permissions. This one for example comes from Helmfile with labels stage=prep:

  Error: unable to generate manifests: could not get information about the resource: poddisruptionbudgets.policy "ingress-nginx-platform-controller" is forbidden: User "system:serviceaccount:apl-operator:apl-operator" cannot get resource "poddisruptionbudgets" in API group "policy" in the namespace "ingress"
  Error: plugin "diff" exited with error

But the operator also needs to be allowed to do anything that the upgrade scripts need to do, e.g. managing labels and annotations, creating Jobs etc. Maybe it is too ambitious to be so granular about it. It can break again on any upgrade.

Copy link
Contributor

@merll merll left a comment

Choose a reason for hiding this comment

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

According to my tests, the operator needs more permissions to function properly.

@CasLubbers
Copy link
Contributor Author

I am seeing lots of errors in combination with the Helm file upgrade, but also the pre-upgrade from the latest release. Apparently the serviceAccount does not have enough permissions. This one for example comes from Helmfile with labels stage=prep:

  Error: unable to generate manifests: could not get information about the resource: poddisruptionbudgets.policy "ingress-nginx-platform-controller" is forbidden: User "system:serviceaccount:apl-operator:apl-operator" cannot get resource "poddisruptionbudgets" in API group "policy" in the namespace "ingress"
  Error: plugin "diff" exited with error

But the operator also needs to be allowed to do anything that the upgrade scripts need to do, e.g. managing labels and annotations, creating Jobs etc. Maybe it is too ambitious to be so granular about it. It can break again on any upgrade.

That was already something I was afraid of. It's a bit hard to guess what were gonna need in the future for RBAC. Maybe read access to everything is oke and then for updating, deleting and creating we can be more strict.
Other option is to give it the admin role...

@CasLubbers CasLubbers requested review from j-zimnowoda and merll May 22, 2025 14:47
@merll
Copy link
Contributor

merll commented May 22, 2025

There's another one:

Error: unable to generate manifests: cannot patch "istiod-default-validator" with kind ValidatingWebhookConfiguration: validatingwebhookconfigurations.admissionregistration.k8s.io "istiod-default-validator" is forbidden: User "system:serviceaccount:apl-operator:apl-operator" cannot patch resource "validatingwebhookconfigurations" in API group "admissionregistration.k8s.io" at the cluster scope

Copy link
Contributor

@merll merll left a comment

Choose a reason for hiding this comment

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

The Istiod Helmfile install still fails, due to missing permissions. Otherwise looks good to me.
I can also extend the ClusterRole as needed in the Istio feature branch, if this is holding up the PR for too long.

@j-zimnowoda j-zimnowoda enabled auto-merge (squash) May 23, 2025 08:17
@j-zimnowoda j-zimnowoda merged commit bb1623d into main May 23, 2025
14 checks passed
@j-zimnowoda j-zimnowoda deleted the APL-769 branch May 23, 2025 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants