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

Bug: Resource with reconcile-policy: skip doesn't populate ConfigMap #2985

Closed
skinny opened this issue May 16, 2023 · 6 comments · Fixed by #3103
Closed

Bug: Resource with reconcile-policy: skip doesn't populate ConfigMap #2985

skinny opened this issue May 16, 2023 · 6 comments · Fixed by #3103
Assignees
Labels
bug 🪲 Something isn't working high-priority Issues we intend to prioritize (security, outage, blocking bug)
Milestone

Comments

@skinny
Copy link

skinny commented May 16, 2023

Describe the bug
Using ASO V2 I expect an UserAssignedIdentity to populate the ConfigMap described in the operaterSpec. This works, but when I introduce the serviceoperator.azure.com/reconcile-policy: skip (because this is an already existing identity) the ConfigMap is not created/populated

To Reproduce

  • Deploy an UserAssignedIdentity with the following yaml :
apiVersion: managedidentity.azure.com/v1beta20181130
kind: UserAssignedIdentity
metadata:
  name: umi-my-app
  annotations:
    serviceoperator.azure.com/reconcile-policy: skip
spec:
  location: westeurope
  owner:
    name: rg-umi-my-app
  operatorSpec:
    configMaps:
      principalId:
        name: deploy-identity
        key: principalId
      clientId:
        name: deploy-identity
        key: clientId
      tenantId:
        name: deploy-identity
        key: tenantId

Expected behavior
The deploy-identity configmap should be created and populated just like when the UAI is created as a an ASO managed resource.

Screenshots
n/a

Additional context
n/a

@matthchr
Copy link
Member

Just confirming here that I do believe this is a bug.

@matthchr matthchr added this to the v2.1.0 milestone May 22, 2023
@matthchr matthchr added bug 🪲 Something isn't working and removed needs-triage 🔍 labels May 22, 2023
@theunrepentantgeek theunrepentantgeek modified the milestones: v2.1.0, v2.2.0 May 23, 2023
@theunrepentantgeek theunrepentantgeek modified the milestones: v2.3.0, v2.2.0 May 31, 2023
@matthchr matthchr added the high-priority Issues we intend to prioritize (security, outage, blocking bug) label Jun 2, 2023
@b1zzu
Copy link

b1zzu commented Jun 7, 2023

Hi, I just like to make sure that also the operatorSpec.secrets expected behavior should be the same as the operatorSpec.configMaps, as the problem I'm facing as to do with it

@skinny
Copy link
Author

skinny commented Jun 22, 2023

Looks like this issue is present on more resource types. Adding an existing storage account (in Azure) as an ASO resource will not populate either the ConfigMap or Secret as requested in the resource definition:

kind: StorageAccount
metadata:
  name: acmebranchdeployments
  annotations:
    serviceoperator.azure.com/reconcile-policy: skip
spec:
  location: westeurope
  kind: StorageV2
  sku:
    name: Standard_LRS
  owner:
    name: rg-test-cicd
  accessTier: Hot
  # Optional: Save the keys for the storage accokunt into a Kubernetes secret
  supportsHttpsTrafficOnly: true
  operatorSpec:
    configMaps:
      fileEndpoint:
        name: acme-app-storage-configmap
        key: fileEndpoint

The resource itself is succesfully populated with information from Azure but no ConfigMap or Secret is created.

@matthchr
Copy link
Member

Yes, I hadn't updated the title to reflect that but was aware it will impact every resource that can export ConfigMaps (and I think Secrets as well) with skip annotation.

I'll fix up this issue title to more properly reflect the scope of the bug.

The good news is, a single fix will fix all of the resources at once.

@matthchr matthchr changed the title Bug: UserAssignedIdentity with reconcile-policy: skip doesn't populate ConfigMap Bug: Resource with reconcile-policy: skip doesn't populate ConfigMap Jun 22, 2023
@skinny
Copy link
Author

skinny commented Jun 22, 2023

Thanks! And yes both ConfigMaps and Secrets are affected and by a quick glance it looks like the code that generate those resources is only called for newly created resources

@matthchr
Copy link
Member

matthchr commented Jun 22, 2023

Notes (mostly to myself) on fixing this:

We need to combine handleCreatePollerSuccess and skipReconcile methods in v2/internal/reconcilers/arm/azure_generic_arm_reconciler_instance.go.

I sorta think it makes sense to:

  1. Rename handleCreatePollerSuccess to handleCreateOrUpdatePollerSuccess (as it deals with updates too).
  2. Combine the two methods into a single method which takes a flag indicating which "flavor" it is. I'm normally not a huge fan of doing this but in this case it makes sense as the two methods should do 80% the same stuff, with only a few things being createOrUpdate specific and not happening also for the skip-reconcile case.

Obviously we'll need some tests to reproduce this issue too to confirm the fix works.

@matthchr matthchr self-assigned this Jun 22, 2023
matthchr added a commit to matthchr/azure-service-operator that referenced this issue Jun 26, 2023
matthchr added a commit to matthchr/azure-service-operator that referenced this issue Jun 26, 2023
matthchr added a commit to matthchr/azure-service-operator that referenced this issue Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🪲 Something isn't working high-priority Issues we intend to prioritize (security, outage, blocking bug)
Projects
Development

Successfully merging a pull request may close this issue.

4 participants