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(k8s): ensure patchResources can patch namespace #5334

Merged
merged 1 commit into from Nov 2, 2023

Conversation

eysi09
Copy link
Collaborator

@eysi09 eysi09 commented Nov 1, 2023

What this PR does / why we need it:

When applying K8s resources, we create a ConfigMap with metadata about the resources for faster lookup.

This metadata includes the resource namespace among other things.

Before this fix, we'd create the ConfigMap before applying the resource patch which meant that if you patched the actual resource namespace, Garden wouldn't store that information.

Later when looking up the status of the resource, Garden would use the data from the ConfigMap and basically look for the resource in the wrong namespace and return status "not-ready" (when in fact the resource could be ready, just in a different namespace).

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

  • Are there other places in the code you can think of were we could be using the wrong namespace? (At a glance I wouldn't think so, but wanted to flag.)
  • Just out of curiosity: Why do we get the deploy status again after waiting for the resources? If you trace through the logs, you see that Garden creates the resources in the correct namespace and logs "Resources ready". We then go ahead and get the statuses again in this line and return those. (This is related to the bug because for the second time around we'd look in the wrong namespace). Shouldn't the waitForResources function return the correct status?

When applying K8s resources, we create a ConfigMap with metadata about
the resources for faster lookup.

This metadata includes the resource namespace among other things.

Before this fix, we'd create the ConfigMap before applying the resource
patch which meant that if you patched the actual resource namespace,
Garden wouldn't store that information.

Later when looking up the status of the resource, Garden would use the
data from the ConfigMap and basically look for the resource in the
wrong namespace and return status "not-ready" (when in fact the resource
could be ready, just in a different namespace).
@@ -177,6 +165,16 @@ export async function getManifests({
)
}

if (action.kind === "Deploy") {
// Add metadata ConfigMap to aid quick status check
const metadataManifest = getMetadataManifest(action, defaultNamespace, patchedManifests)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The fix here is that now we're getting the metadata for the patched manifests.

Copy link
Collaborator

@twelvemo twelvemo left a comment

Choose a reason for hiding this comment

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

Looks good! Nice that you added a test for that!

@twelvemo twelvemo added this pull request to the merge queue Nov 2, 2023
Merged via the queue into main with commit 71d45a9 Nov 2, 2023
44 checks passed
@twelvemo twelvemo deleted the fix-patch-namespace-2 branch November 2, 2023 16:12
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.

None yet

2 participants