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: handle cluster scoped resources #79

Merged
merged 3 commits into from
Sep 29, 2023

Conversation

lukebond
Copy link
Contributor

@lukebond lukebond commented Sep 29, 2023

hit a bug in our cluster where we are trying to sync cluster scoped resources, which we've never tried before. it was getting a 404 on the creation of the resource.

at first i went and fixed the apply_mappings function, which was causing namespaces to become Some("null"). turns out that wasn't making its way into the final resource but i left it as it's a nice tidy-up anyway.

the real issue is that with namespace missing sinker assumed default namespace and creates a specifically namespaced client, that points to "default". in kube-rs this isn't the same as if you specified "default" in the yaml for a cluster-scoped resource, which would just get ignored. in kube-rs you have to use the cluster-scoped client.

i started trying to write a test for this but this code called cluster_client which requires an API, so i'd need to set up a tower mock of one and it would be a big faff.

@lukebond lukebond requested a review from mkmik September 29, 2023 12:17
Comment on lines 186 to 192
let mut template = if let Some(ns) = target_namespace {
DynamicObject::new(&target_ref.name, ar)
.within(ns)
.data(json!({}))
} else {
DynamicObject::new(&target_ref.name, ar).data(json!({}))
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let mut template = if let Some(ns) = target_namespace {
DynamicObject::new(&target_ref.name, ar)
.within(ns)
.data(json!({}))
} else {
DynamicObject::new(&target_ref.name, ar).data(json!({}))
};
let mut template = DynamicObject::new(&target_ref.name, ar).data(json!({}));
template.metadata.namespace = target_namespace;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i did it in a few places so change it in those places too.

Copy link
Contributor

@mkmik mkmik left a comment

Choose a reason for hiding this comment

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

LGTM, one minor suggestion to simplify the code

@lukebond lukebond requested a review from mkmik September 29, 2023 12:39
@lukebond lukebond enabled auto-merge September 29, 2023 12:46
Comment on lines +214 to +220
let mut source = if let Value::Null = subtree["metadata"]["namespace"] {
DynamicObject::new(&subtree["metadata"]["name"].to_string(), &ar).data(subtree)
} else {
DynamicObject::new(&subtree["metadata"]["name"].to_string(), &ar)
.within(&subtree["metadata"]["namespace"].to_string())
.data(subtree)
};
Copy link
Contributor

@mkmik mkmik Sep 29, 2023

Choose a reason for hiding this comment

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

similar situation here

Suggested change
let mut source = if let Value::Null = subtree["metadata"]["namespace"] {
DynamicObject::new(&subtree["metadata"]["name"].to_string(), &ar).data(subtree)
} else {
DynamicObject::new(&subtree["metadata"]["name"].to_string(), &ar)
.within(&subtree["metadata"]["namespace"].to_string())
.data(subtree)
};
let mut source = DynamicObject::new(&subtree["metadata"]["name"].to_string(), &ar).data(subtree);
source.metadata.namespace = subtree["metadata"]["namespace"].as_str().map(str::to_string);

Copy link
Contributor

@mkmik mkmik left a comment

Choose a reason for hiding this comment

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

another suggestion

@lukebond lukebond merged commit a30a553 into main Sep 29, 2023
2 checks passed
@lukebond lukebond deleted the fix/handle-cluster-scoped-resources branch September 29, 2023 12:52
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.

2 participants