Skip to content
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

fix: create index for pod annotation path for allowkubernetessync annotation instead of deployment #582

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

odubajDT
Copy link
Contributor

@odubajDT odubajDT commented Dec 7, 2023

Fixes: #579

Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
@odubajDT odubajDT changed the title fix: create index for deployment path instead of pod fix: create index for pod annotation path instead of deployment Dec 7, 2023
@odubajDT odubajDT changed the title fix: create index for pod annotation path instead of deployment fix: create index for pod annotation path for allowkubernetessync annotation instead of deployment Dec 7, 2023
@odubajDT odubajDT mentioned this pull request Dec 7, 2023
@odubajDT odubajDT marked this pull request as ready for review December 7, 2023 14:37
@odubajDT odubajDT requested a review from a team as a code owner December 7, 2023 14:37
Copy link

codecov bot commented Dec 7, 2023

Codecov Report

Merging #582 (aa62297) into main (70fb5d9) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #582   +/-   ##
=======================================
  Coverage   87.98%   87.98%           
=======================================
  Files          10       10           
  Lines         932      932           
=======================================
  Hits          820      820           
  Misses         89       89           
  Partials       23       23           
Flag Coverage Δ
unit-tests 87.98% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@toddbaert
Copy link
Member

toddbaert commented Dec 7, 2023

Can we add some kind of test for this? e2e or otherwise?

Another question I have is that wrote response {"webhook": "/mutate-v1-pod", "code": 200, "reason": "OpenFeature is disabled", "UID": "8f021e26-6d9b-47f2-90f2-588236ec388d", "allowed": true} (meaning the openfeature/enabled annotation isn't found) is also reported in the issue. Is that related or is that a totally different issue (I think it's different)? Note my comment here.

@beeme1mr beeme1mr merged commit a6fa04f into open-feature:main Dec 8, 2023
18 checks passed
@github-actions github-actions bot mentioned this pull request Dec 8, 2023
@odubajDT
Copy link
Contributor Author

odubajDT commented Dec 11, 2023

Can we add some kind of test for this? e2e or otherwise?

Another question I have is that wrote response {"webhook": "/mutate-v1-pod", "code": 200, "reason": "OpenFeature is disabled", "UID": "8f021e26-6d9b-47f2-90f2-588236ec388d", "allowed": true} (meaning the openfeature/enabled annotation isn't found) is also reported in the issue. Is that related or is that a totally different issue (I think it's different)? Note my comment here.

Hey, the problem comes from the webhook checking if the examinated pod has the proper annotation openfeature.dev/enabled: "true". According to Adam's manifests, this should be ok. Also if the OF is not enabled, we wouldn't receive the problems with backfilling permissions -> we won't be able to reach this part of code at all.

Maybe Argo was creating a Pod in a completely different namespace at that time (completely unrelated to this Deployment)? Here it would make sense that the podMutator kicks in, checks if this Pod is annotated, finds out it's not, writes out the message and lets the pod to be bind to a node without any mutation? I think here we need to have more info about what was happening in the system generally...

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.

OpenFeature is disabled?
5 participants