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

Use policy name used during create #9615

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

MichalFupso
Copy link
Contributor

Description

Related issues/PRs

Todos

  • Tests
  • Documentation
  • Release note

Release Note

TBD

Reminder for the reviewer

Make sure that this PR has the correct labels and milestone set.

Every PR needs one docs-* label.

  • docs-pr-required: This change requires a change to the documentation that has not been completed yet.
  • docs-completed: This change has all necessary documentation completed.
  • docs-not-required: This change has no user-facing impact and requires no docs.

Every PR needs one release-note-* label.

  • release-note-required: This PR has user-facing changes. Most PRs should have this label.
  • release-note-not-required: This PR has no user-facing changes.

Other optional labels:

  • cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.
  • needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.

@MichalFupso MichalFupso requested a review from a team as a code owner December 18, 2024 21:25
@marvin-tigera marvin-tigera added this to the Calico v3.30.0 milestone Dec 18, 2024
@marvin-tigera marvin-tigera added release-note-required Change has user-facing impact (no matter how small) docs-pr-required Change is not yet documented labels Dec 18, 2024
@MichalFupso MichalFupso added release-note-not-required Change has no user-facing impact and removed release-note-required Change has user-facing impact (no matter how small) labels Jan 22, 2025
@MichalFupso MichalFupso changed the title WIP: store original policy name in metadata Use policy name used during create Jan 22, 2025
}

func updateNeeded(k8sClientset *kubernetes.Clientset) bool {
tierMigration, err := k8sClientset.CoreV1().ConfigMaps("calico-apiserver").Get(context.Background(), "api-server", metav1.GetOptions{})
Copy link
Member

Choose a reason for hiding this comment

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

Strictly speaking we may be running in a different namespace in some environments - I believe you can learn what namespace you are in based on a projected file mounted into the container at /var/run/secrets/kubernetes.io/serviceaccount/namespace though. Might already be loaded by the go-client? I can't remember.

Copy link
Member

Choose a reason for hiding this comment

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

Worth verifying that we have the necessary RBAC in our manifests / operator to get, create, and update this configmap.

Copy link
Member

@caseydavenport caseydavenport left a comment

Choose a reason for hiding this comment

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

Mostly my comments revolve around... adding more comments :D

This behavior is a bit weird and needs to be very cleanly documented in the code.

The other thing that I don't understand are the test changes - seems like we're changing a bunch of tests that use the name to use tier.name instead? That seems fishy to me. I would have expected most tests to stay the same, but to introduce a new set of tests that verify

  1. Policies can be created with either name format.
  2. Policies are returned using the same name format they were created with (e.g., via List and Watch operations)

"github.com/projectcalico/calico/libcalico-go/lib/winutils"
)

func migratePolicyNames() error {
Copy link
Member

Choose a reason for hiding this comment

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

A nice big comment explaining when this code is run, and why this code is run would be nice to have here. This is the sort of thing we'll look back on in a few years and go "What??"

Copy link
Member

Choose a reason for hiding this comment

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

I also wonder if we need a way to explicitly turn this off - I guess the user can create the configmap and it will do it?

Have you tried this on a mock cluster with say, 10k policy objects?

return annotations, nil
}

func defaultPolicyName(d *model.KVPair) error {
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to see more detailed comments throughout this code, explaining each step. It might be obvious to us now, since we're actively working on it, but won't be when someone comes to this later!

Comment on lines +636 to +641
annotations, err := storePolicyName(value.Name, value.Annotations)
if err != nil {
return err
}

value.Annotations = annotations
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
annotations, err := storePolicyName(value.Name, value.Annotations)
if err != nil {
return err
}
value.Annotations = annotations
// First, capture the policy name that was used on the v3 API and store it as an annotation.
// This allows us to recreate the original object in List() and Watch() calls correctly, returning the object as expected by the client.
annotations, err := storePolicyName(value.Name, value.Annotations)
if err != nil {
return err
}
value.Annotations = annotations

Here's an example of the type of comment I'm looking for.

Comment on lines +643 to +647
polName, err := names.BackendTieredPolicyName(value.Name, value.Spec.Tier)
if err != nil {
return err
}
value.Name = polName
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
polName, err := names.BackendTieredPolicyName(value.Name, value.Spec.Tier)
if err != nil {
return err
}
value.Name = polName
// Now that we've captured the original name, canonicalize the name for storage,
// adding the tier prefix to ensure policies with the same name in different tiers do not conflict.
polName, err := names.BackendTieredPolicyName(value.Name, value.Spec.Tier)
if err != nil {
return err
}
value.Name = polName

@@ -383,7 +383,7 @@ var _ = testutils.E2eDatastoreDescribe("Test UIDs and owner references", testuti
npUIDv1, err := conversion.ConvertUID(npUID)
Expect(err).NotTo(HaveOccurred())

name := "test-owner-ref-policy"
name := "default.test-owner-ref-policy"
Copy link
Member

Choose a reason for hiding this comment

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

If we didn't change this, wouldn't the test still pass? I'd expect the backend to store the name as an annotation, and canonicalize the name used for the backing CRD?

@@ -138,7 +140,6 @@ func ConvertCalicoResourceToK8sResource(resIn Resource) (Resource, error) {
// the metadata annotation.
romCopy := &metav1.ObjectMeta{}
rom.(*metav1.ObjectMeta).DeepCopyInto(romCopy)
romCopy.Name = ""
Copy link
Member

Choose a reason for hiding this comment

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

I think we probably want to only store the name in the annotation for NetworkPolicy / GlobalNetworkPolicy? i.e., objects that belong to a Tier.

@@ -175,6 +176,17 @@ func ConvertCalicoResourceToK8sResource(resIn Resource) (Resource, error) {
// Name, Namespace, ResourceVersion, UID, and Annotations (built above).
meta := &metav1.ObjectMeta{}
meta.Name = rom.GetName()
switch resIn.GetObjectKind().GroupVersionKind().Kind {
// For NetworkPolicy and GlobalNetworkPolicy, we need to prefix the name with the tier name.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// For NetworkPolicy and GlobalNetworkPolicy, we need to prefix the name with the tier name.
// For NetworkPolicy and GlobalNetworkPolicy, we need to prefix the name with the tier name.
// This ensures two policies with the same name, but in different tiers, do not resolve to the same backing object.

Copy link
Member

Choose a reason for hiding this comment

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

A bit of a nit, but could you move the switch statement to the end of the metadata populating section? It's breaking up a bunch of similar lines and makes the code a tiny bit harder to follow.

e.g.,

meta.Name = rom.GetName()
meta.Namespace = rom.GetNamespace()
meta.ResourceVersion = rom.GetResourceVersion()

switch resIn.GetObjectKind().GroupversionKind().Kind() {
 . . .

@@ -293,7 +305,9 @@ func ConvertK8sResourceToCalicoResource(res Resource) error {

// Manually write in the data not stored in the annotations: Name, Namespace, ResourceVersion,
// so that they do not get overwritten.
meta.Name = rom.GetName()
if meta.Name == "" {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if meta.Name == "" {
if meta.Name == "" {
// We store the original v3 Name in our annotation when writing NetworkPolicy and GNP objects. If that's present, use it.
// Otherwise, fill in the name from the underlying CRD.

"github.com/projectcalico/calico/libcalico-go/lib/json"
"github.com/projectcalico/calico/libcalico-go/lib/namespace"
"github.com/projectcalico/calico/libcalico-go/lib/net"
)

const (
metadataAnnotation = "projectcalico.org/metadata"
Copy link
Member

Choose a reason for hiding this comment

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

We have this defined in a few places now - should we put it in a common location instead?

@@ -155,7 +156,7 @@ var _ = testutils.E2eDatastoreDescribe("GlobalNetworkPolicy tests", testutils.Da
Expect(outError.Error()).To(Equal("resource already exists: GlobalNetworkPolicy(" + tieredGNPName(name1, tier) + ")"))

By("Getting GlobalNetworkPolicy (name1) and comparing the output against spec1")
res, outError := c.GlobalNetworkPolicies().Get(ctx, name1, options.GetOptions{})
res, outError := c.GlobalNetworkPolicies().Get(ctx, tieredGNPName(name1, tier), options.GetOptions{})
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit surprised to see all of these test changes - I would have expected this change to be compatible with our existing v3 client tests?

Namespace: "calico-apiserver",
},
Data: map[string]string{
"tierMigration": "true",
Copy link
Member

@caseydavenport caseydavenport Jan 23, 2025

Choose a reason for hiding this comment

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

Suggested change
"tierMigration": "true",
"policyNameMigrationNeeded": "true",

Copy link
Member

Choose a reason for hiding this comment

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

We should probably define a constant for this.

_, err = k8sClientset.CoreV1().ConfigMaps("calico-apiserver").Create(context.Background(), &configMap, metav1.CreateOptions{})
if err != nil {
klog.Errorf("Failed to create api-server configmap: %s", err)
return false
Copy link
Member

Choose a reason for hiding this comment

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

Why does a failure to create the configmap mean we don't need to update the tier names?

Comment on lines +99 to +102
if tierMigration.Data["tierMigration"] == "false" {
return false
}
return true
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if tierMigration.Data["tierMigration"] == "false" {
return false
}
return true
return tierMigration.Data["tierMigration"] != "false"

return err
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to update the ConfigMap after a successful tier migration has occurred so we don't run through this again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-pr-required Change is not yet documented release-note-not-required Change has no user-facing impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants