Features conf adversely affects /api/provision/:tag

[papaben]

It appears that setting features.useWhitelistOnRepurpose = true prevents the primary_role and profile from being set by the /api/provision/:tag request.

[papaben]

CC @nadeemahmad

[yl3w]

I create a PR. I went back and took a look and the useWhiteListOnRepurpose wasn't a feature we ever used internally and it dropped thru the cracks. I recall you mentioning that you wanted to move to the latest on master in production at your site. FWIW, we are about a month behind on the version from master we run in production. If you want to wait for a few more weeks, I'll roll the latest to production internally.

https://github.com/tumblr/collins/pull/376

[papaben]

Thanks @maddalab. The useWhiteListOnRepurpose feature is actually really useful for us, thanks for the diligence on that.

We'll be ready to check out the new release when it's available!

[yl3w]

Ok. I'd recommend testing thoroughly for the change. I've added some unit test to ensure it stays correct, however do not have any production use cases to test it against.

Also, I want to make sure you understand the following lines (now)

    val lowPriorityAttrs = role.attributes
    val clearProfileAttrs = role.clear_attributes.map(_ -> "").toMap
    val clearOnRepurposeAttrs = Feature.deleteSomeMetaOnRepurpose.map(_ -> "").toMap
    val keepOnRepurposeAttrs = Feature.keepSomeMetaOnRepurpose

    // make sure high priority attrs take precedence over low priority
    // and make sure any explicitly set attrs override any that are to be cleared
    if (Feature.useWhiteListOnRepurpose) {
      clearOnRepurposeAttrs ++ clearProfileAttrs ++ lowPriorityAttrs ++ highPriorityAttrs -- keepOnRepurposeAttrs
    } else {
      clearOnRepurposeAttrs ++ clearProfileAttrs ++ lowPriorityAttrs ++ highPriorityAttrs
    }

compared to (earlier)

  protected def attribs(asset: Asset, request: ProvisionerRequest, form: ProvisionForm): Map[String,String] = {
    val build_contact = form._2
    val suffix = form._3
    val role = request.profile.role
    val highPriorityAttrs =
      Map(
        "NODECLASS" -> request.profile.identifier,
        "CONTACT" -> role.contact.getOrElse(""),
        "CONTACT_NOTES" -> role.contact_notes.getOrElse(""),
        "SUFFIX" -> suffix.getOrElse(""),
        "PRIMARY_ROLE" -> role.primary_role.getOrElse(""),
        "POOL" -> role.pool.getOrElse(""),
        "SECONDARY_ROLE" -> role.secondary_role.getOrElse(""),
        "BUILD_CONTACT" -> build_contact
      )
    val lowPriorityAttrs = role.attributes
    val clearProfileAttrs = role.clear_attributes.map(a => (a -> "")).toMap

    // make sure high priority attrs take precedence over low priority
    // and make sure any explicitly set attrs override any that are to be cleared
    clearOnRepurposeAttrs(asset) ++ clearProfileAttrs ++ lowPriorityAttrs ++ highPriorityAttrs
  }

  private[this] def clearOnRepurposeAttrs(asset: Asset): Map[String, String] = {
    if (Feature.useWhiteListOnRepurpose) {
      val keepAttributes = Feature.keepSomeMetaOnRepurpose.map(_.name)
      val allAttributes = AssetMetaValue.findByAsset(asset).map(_.getName()).toSet
      val deleteAttributes = allAttributes -- keepAttributes
      deleteAttributes.map(s => (s -> "")).toMap
    } else {
      Feature.deleteSomeMetaOnRepurpose.map(_.name).map(s => (s -> "")).toMap
    }
  }

In the earlier version when using white list on repurpose we ignored delete some meta on repurpose, fetched all the attributes from AssetMeta for the asset and deleted any attribute not in the white list.

IMO, this is incorrect, as some hardware information is stored in the ATTRIBS with well known names.

The new version, obtains the list of all attributes to delete and removes the white listed attributes from the list.

In my testing, I added 2 attributes each to white list and delete on repurpose.

  keepSomeMetaOnRepurpose = [ attr1, attr2 ]
  deleteSomeMetaOnRepurpose = [ attr4, attr3 ]

Set all 4 attributes on the asset, provisioned the asset and checked that attr1 and attr2 were still set on the asset.

[yl3w]

Update, we are running the laster master in production for the past few days without any issue.

[papaben]

Very cool! Thanks for the update.